diff --git a/config/config.go b/config/config.go index 9fa82a86..08af54f0 100644 --- a/config/config.go +++ b/config/config.go @@ -263,6 +263,7 @@ type EnvConfig struct { TTL ct.OptDuration `conf:"LD_TTL_"` ProjKey string `conf:"LD_PROJ_KEY_"` FilterKey FilterKey // injected based on [filters] section + Offline bool // set to true if this environment was created in offline mode } type FiltersConfig struct { diff --git a/integrationtests/autoconfig_test.go b/integrationtests/autoconfig_test.go index 29cc49f2..b3973cac 100644 --- a/integrationtests/autoconfig_test.go +++ b/integrationtests/autoconfig_test.go @@ -77,7 +77,7 @@ func testPolicyUpdate(t *testing.T, manager *integrationTestManager) { manager.awaitRelayStatus(t, func(status api.StatusRep) bool { if len(status.Environments) == 1 { if envStatus, ok := status.Environments[string(remainingEnv.id)]; ok { - verifyEnvProperties(t, testData.project, remainingEnv, envStatus, true) + verifyEnvProperties(t, testData.project, remainingEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return true } } @@ -96,7 +96,7 @@ func testAddEnvironment(t *testing.T, manager *integrationTestManager) { manager.awaitRelayStatus(t, func(status api.StatusRep) bool { if len(status.Environments) == len(testData.environments)+1 { if envStatus, ok := status.Environments[string(newEnv.id)]; ok { - verifyEnvProperties(t, testData.project, newEnv, envStatus, true) + verifyEnvProperties(t, testData.project, newEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return true } } @@ -136,7 +136,7 @@ func testUpdatedSDKKeyWithoutExpiry(t *testing.T, manager *integrationTestManage manager.awaitRelayStatus(t, func(status api.StatusRep) bool { if envStatus, ok := status.Environments[string(envToUpdate.id)]; ok { - verifyEnvProperties(t, testData.project, updatedEnv, envStatus, true) + verifyEnvProperties(t, testData.project, updatedEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return last5(envStatus.SDKKey) == last5(string(newKey)) && envStatus.ExpiringSDKKey == "" } return false @@ -166,7 +166,7 @@ func testUpdatedSDKKeyWithExpiry(t *testing.T, manager *integrationTestManager) return false } if envStatus, ok := status.Environments[string(envToUpdate.id)]; ok { - verifyEnvProperties(t, testData.project, updatedEnv, envStatus, true) + verifyEnvProperties(t, testData.project, updatedEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return last5(envStatus.SDKKey) == last5(string(newKey)) && last5(envStatus.ExpiringSDKKey) == last5(string(oldKey)) } @@ -204,13 +204,13 @@ func testUpdatedSDKKeyWithExpiryBeforeStartingRelay(t *testing.T, manager *integ }) defer manager.stopRelay(t) - manager.awaitEnvironments(t, projAndEnvs, false, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, projAndEnvs, nil, func(proj projectInfo, env environmentInfo) string { return string(env.id) }) manager.awaitRelayStatus(t, func(status api.StatusRep) bool { if envStatus, ok := status.Environments[string(envToUpdate.id)]; ok { - verifyEnvProperties(t, testData.project, updatedEnv, envStatus, true) + verifyEnvProperties(t, testData.project, updatedEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return last5(envStatus.SDKKey) == last5(string(newKey)) && last5(envStatus.ExpiringSDKKey) == last5(string(oldKey)) } @@ -238,7 +238,7 @@ func testUpdatedMobileKey(t *testing.T, manager *integrationTestManager) { manager.awaitRelayStatus(t, func(status api.StatusRep) bool { if envStatus, ok := status.Environments[string(envToUpdate.id)]; ok { - verifyEnvProperties(t, testData.project, updatedEnv, envStatus, true) + verifyEnvProperties(t, testData.project, updatedEnv, envStatus, &envPropertyExpectations{nameAndKey: true}) return last5(envStatus.MobileKey) == last5(string(newKey)) } return false @@ -286,7 +286,7 @@ func withRelayAndTestData(t *testing.T, manager *integrationTestManager, action func awaitInitialState(t *testing.T, manager *integrationTestManager, testData autoConfigTestData) { projsAndEnvs := projsAndEnvs{testData.project: testData.environments} - manager.awaitEnvironments(t, projsAndEnvs, true, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, projsAndEnvs, &envPropertyExpectations{nameAndKey: true}, func(proj projectInfo, env environmentInfo) string { return string(env.id) }) } diff --git a/integrationtests/offline_mode_test.go b/integrationtests/offline_mode_test.go index d8478aad..61a329ce 100644 --- a/integrationtests/offline_mode_test.go +++ b/integrationtests/offline_mode_test.go @@ -7,10 +7,12 @@ package integrationtests import ( "fmt" "io" + "maps" "net/http" "os" "path/filepath" "testing" + "time" "github.com/launchdarkly/ld-relay/v8/config" @@ -25,8 +27,35 @@ type offlineModeTestData struct { autoConfigID autoConfigID } +// Used to configure the environment/project setup for an offline mode test. +type apiParams struct { + // How many projects to create. + numProjects int + // Within each project, how many environments to create. Note: this must be >= 2 due to the way test flag + // variations are setup. + numEnvironments int +} + func testOfflineMode(t *testing.T, manager *integrationTestManager) { - withOfflineModeTestData(t, manager, func(testData offlineModeTestData) { + t.Run("expected environments and flag values", func(t *testing.T) { + testExpectedEnvironmentsAndFlagValues(t, manager) + }) + t.Run("sdk key is rotated with deprecation after relay has started", func(t *testing.T) { + testSDKKeyRotatedAfterRelayStarted(t, manager) + }) + t.Run("sdk key is rotated with deprecation before relay has started", func(t *testing.T) { + testSDKKeyRotatedBeforeRelayStarted(t, manager) + }) + t.Run("sdk key is rotated and then expires", func(t *testing.T) { + testSDKKeyExpires(t, manager) + }) + t.Run("sdk key is rotated multiple times without deprecation after relay started", func(t *testing.T) { + testKeyIsRotatedWithoutGracePeriod(t, manager) + }) +} + +func testExpectedEnvironmentsAndFlagValues(t *testing.T, manager *integrationTestManager) { + withOfflineModeTestData(t, manager, apiParams{numEnvironments: 2, numProjects: 2}, func(testData offlineModeTestData) { helpers.WithTempDir(func(dirPath string) { fileName := "archive.tar.gz" filePath := filepath.Join(manager.relaySharedDir, fileName) @@ -40,16 +69,194 @@ func testOfflineMode(t *testing.T, manager *integrationTestManager) { }) defer manager.stopRelay(t) - manager.awaitEnvironments(t, testData.projsAndEnvs, true, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, testData.projsAndEnvs, &envPropertyExpectations{nameAndKey: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + manager.verifyFlagValues(t, testData.projsAndEnvs) + }) + }) +} + +func testSDKKeyRotatedAfterRelayStarted(t *testing.T, manager *integrationTestManager) { + // If we download an archive with a primary SDK key, and then it is subsequently updated + // with a deprecated key, we become initialized with both keys present. + withOfflineModeTestData(t, manager, apiParams{numEnvironments: 2, numProjects: 1}, func(testData offlineModeTestData) { + helpers.WithTempDir(func(dirPath string) { + fileName := "archive.tar.gz" + filePath := filepath.Join(manager.relaySharedDir, fileName) + + err := downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + // Ensure the key won't expire during this test. + const keyGracePeriod = 1 * time.Hour + // Relay will check for expired keys at this interval. + const cleanupInterval = 100 * time.Millisecond + // We'll sleep longer than the interval after rotating keys, to try and reduce test flakiness. + const cleanupIntervalBuffer = 1 * time.Second + + manager.startRelay(t, map[string]string{ + "FILE_DATA_SOURCE": filepath.Join(relayContainerSharedDir, fileName), + "EXPIRED_CREDENTIAL_CLEANUP_INTERVAL": cleanupInterval.String(), + }) + defer manager.stopRelay(t) + + manager.awaitEnvironments(t, testData.projsAndEnvs, &envPropertyExpectations{nameAndKey: true}, func(proj projectInfo, env environmentInfo) string { return string(env.id) }) manager.verifyFlagValues(t, testData.projsAndEnvs) + + // The updated map will is modified to contain expiringSdkKey field (with the old SDK key) and + // the new key set to whatever the API call returned. + updated := manager.rotateSDKKeys(t, testData.projsAndEnvs, time.Now().Add(keyGracePeriod)) + + err = downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + time.Sleep(cleanupIntervalBuffer) + + // We are now asserting that the environment credentials returned by the status endpoint contains not just + // the new SDK key, but the expiring one as well. + manager.awaitEnvironments(t, updated, &envPropertyExpectations{nameAndKey: true, sdkKeys: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + }) + }) +} + +func testSDKKeyRotatedBeforeRelayStarted(t *testing.T, manager *integrationTestManager) { + // Upon startup if an archive contains a primary and deprecated key, we become initialized with both keys. + withOfflineModeTestData(t, manager, apiParams{numEnvironments: 2, numProjects: 1}, func(testData offlineModeTestData) { + helpers.WithTempDir(func(dirPath string) { + fileName := "archive.tar.gz" + filePath := filepath.Join(manager.relaySharedDir, fileName) + + // Relay will check for expired keys at this interval. + const cleanupInterval = 100 * time.Millisecond + // We'll sleep longer than the interval after rotating keys, to try and reduce test flakiness. + const cleanupIntervalBuffer = 1 * time.Second + + // Rotation happens before starting up the relay. + updated := manager.rotateSDKKeys(t, testData.projsAndEnvs, time.Now().Add(1*time.Hour)) + + err := downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + manager.startRelay(t, map[string]string{ + "FILE_DATA_SOURCE": filepath.Join(relayContainerSharedDir, fileName), + "EXPIRED_CREDENTIAL_CLEANUP_INTERVAL": cleanupInterval.String(), + }) + defer manager.stopRelay(t) + + time.Sleep(cleanupIntervalBuffer) + + manager.awaitEnvironments(t, updated, &envPropertyExpectations{nameAndKey: true, sdkKeys: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + manager.verifyFlagValues(t, testData.projsAndEnvs) + }) + }) +} + +func testSDKKeyExpires(t *testing.T, manager *integrationTestManager) { + // If a key is deprecated and then expires, it should be removed from the environment credentials. + withOfflineModeTestData(t, manager, apiParams{numEnvironments: 2, numProjects: 1}, func(testData offlineModeTestData) { + helpers.WithTempDir(func(dirPath string) { + fileName := "archive.tar.gz" + filePath := filepath.Join(manager.relaySharedDir, fileName) + + const keyGracePeriod = 5 * time.Second + // Relay will check for expired keys at this interval. + const cleanupInterval = 100 * time.Millisecond + + // Rotation happens before starting up the relay. + updated := manager.rotateSDKKeys(t, testData.projsAndEnvs, time.Now().Add(keyGracePeriod)) + then := time.Now() + + err := downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + manager.startRelay(t, map[string]string{ + "FILE_DATA_SOURCE": filepath.Join(relayContainerSharedDir, fileName), + "EXPIRED_CREDENTIAL_CLEANUP_INTERVAL": cleanupInterval.String(), + }) + defer manager.stopRelay(t) + + manager.awaitEnvironments(t, updated, &envPropertyExpectations{nameAndKey: true, sdkKeys: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + + // This test is timing-dependant on Relay removing the expired keys before we check the /status endpoint. + // To keep the test fast, only sleep as long as necessary to ensure the keys have expired. + toSleep := keyGracePeriod - time.Since(then) + if toSleep > 0 { + time.Sleep(toSleep) + } + manager.awaitEnvironments(t, updated.withoutExpiringKeys(), &envPropertyExpectations{nameAndKey: true, sdkKeys: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + }) + }) +} + +func testKeyIsRotatedWithoutGracePeriod(t *testing.T, manager *integrationTestManager) { + // If a key is rotated without a grace period, then the old one should be revoked immediately. + // If a key is deprecated and then expires, it should be removed from the environment credentials. + withOfflineModeTestData(t, manager, apiParams{numEnvironments: 2, numProjects: 1}, func(testData offlineModeTestData) { + helpers.WithTempDir(func(dirPath string) { + fileName := "archive.tar.gz" + filePath := filepath.Join(manager.relaySharedDir, fileName) + + err := downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + // Relay will check for expired keys at this interval. + const cleanupInterval = 100 * time.Millisecond + // We'll sleep longer than the interval after rotating keys, to try and reduce test flakiness. + const cleanupIntervalBuffer = 1 * time.Second + + fmt.Println(cleanupInterval) + manager.startRelay(t, map[string]string{ + "FILE_DATA_SOURCE": filepath.Join(relayContainerSharedDir, fileName), + "EXPIRED_CREDENTIAL_CLEANUP_INTERVAL": cleanupInterval.String(), + }) + defer manager.stopRelay(t) + + manager.awaitEnvironments(t, testData.projsAndEnvs, &envPropertyExpectations{nameAndKey: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + manager.verifyFlagValues(t, testData.projsAndEnvs) + + updated := maps.Clone(testData.projsAndEnvs) + + // Check that the rotation logic holds for more than one rotation. + const numRotations = 3 + for i := 0; i < numRotations; i++ { + // time.Time{} to signify that there's no deprecation period. + updated = manager.rotateSDKKeys(t, updated, time.Time{}) + + err = downloadRelayArchive(manager, testData.autoConfigKey, filePath) + manager.apiHelper.logResult("Download data archive from /relay/latest-all to "+filePath, err) + require.NoError(t, err) + + time.Sleep(cleanupIntervalBuffer) + + // We are now asserting that the SDK key was rotated (and that there's no expiringSDKKey). + manager.awaitEnvironments(t, updated, &envPropertyExpectations{nameAndKey: true, sdkKeys: true}, func(proj projectInfo, env environmentInfo) string { + return string(env.id) + }) + } }) }) } -func withOfflineModeTestData(t *testing.T, manager *integrationTestManager, fn func(offlineModeTestData)) { - projsAndEnvs, err := manager.apiHelper.createProjectsAndEnvironments(2, 2) +func withOfflineModeTestData(t *testing.T, manager *integrationTestManager, cfg apiParams, fn func(offlineModeTestData)) { + projsAndEnvs, err := manager.apiHelper.createProjectsAndEnvironments(cfg.numProjects, cfg.numEnvironments) require.NoError(t, err) defer manager.apiHelper.deleteProjects(projsAndEnvs) diff --git a/integrationtests/projects_and_environments_test.go b/integrationtests/projects_and_environments_test.go index d33e8513..af8f571c 100644 --- a/integrationtests/projects_and_environments_test.go +++ b/integrationtests/projects_and_environments_test.go @@ -15,13 +15,14 @@ type projectInfo struct { } type environmentInfo struct { - id config.EnvironmentID - key string - name string - sdkKey config.SDKKey - mobileKey config.MobileKey - prefix string - projKey string + id config.EnvironmentID + key string + name string + sdkKey config.SDKKey + expiringSdkKey config.SDKKey + mobileKey config.MobileKey + prefix string + projKey string // this is a synthetic field, set only when this environment is a filtered environment. filterKey config.FilterKey @@ -29,6 +30,15 @@ type environmentInfo struct { type projsAndEnvs map[projectInfo][]environmentInfo +func (pe projsAndEnvs) withoutExpiringKeys() projsAndEnvs { + for _, envs := range pe { + for i := range envs { + envs[i].expiringSdkKey = "" + } + } + return pe +} + func (pe projsAndEnvs) enumerateEnvs(fn func(projectInfo, environmentInfo)) { for proj, envs := range pe { for _, env := range envs { diff --git a/integrationtests/standard_mode_payload_filters_test.go b/integrationtests/standard_mode_payload_filters_test.go index 8a767681..e6720a2a 100644 --- a/integrationtests/standard_mode_payload_filters_test.go +++ b/integrationtests/standard_mode_payload_filters_test.go @@ -42,7 +42,7 @@ func testStandardModeWithDefaultFilters(t *testing.T, manager *integrationTestMa manager.startRelay(t, envVars) defer manager.stopRelay(t) - manager.awaitEnvironments(t, testData.projsAndEnvs, false, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, testData.projsAndEnvs, nil, func(proj projectInfo, env environmentInfo) string { if env.filterKey == "" { return env.key } @@ -105,7 +105,7 @@ func testStandardModeWithSpecificFilters(t *testing.T, manager *integrationTestM manager.startRelay(t, envVars) defer manager.stopRelay(t) - manager.awaitEnvironments(t, testData.projsAndEnvs, false, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, testData.projsAndEnvs, nil, func(proj projectInfo, env environmentInfo) string { if env.filterKey == "" { return env.key } diff --git a/integrationtests/standard_mode_test.go b/integrationtests/standard_mode_test.go index 18eaf618..c50002ad 100644 --- a/integrationtests/standard_mode_test.go +++ b/integrationtests/standard_mode_test.go @@ -25,7 +25,7 @@ func testStandardMode(t *testing.T, manager *integrationTestManager) { manager.startRelay(t, envVars) defer manager.stopRelay(t) - manager.awaitEnvironments(t, testData.projsAndEnvs, false, func(proj projectInfo, env environmentInfo) string { + manager.awaitEnvironments(t, testData.projsAndEnvs, nil, func(proj projectInfo, env environmentInfo) string { return string(env.name) }) manager.verifyFlagValues(t, testData.projsAndEnvs) diff --git a/integrationtests/test_manager_test.go b/integrationtests/test_manager_test.go index e600e618..7b457e58 100644 --- a/integrationtests/test_manager_test.go +++ b/integrationtests/test_manager_test.go @@ -303,8 +303,7 @@ func (m *integrationTestManager) awaitRelayStatus(t *testing.T, fn func(api.Stat return lastStatus, success } -func (m *integrationTestManager) awaitEnvironments(t *testing.T, projsAndEnvs projsAndEnvs, - expectNameAndKey bool, envMapKeyFn func(proj projectInfo, env environmentInfo) string) { +func (m *integrationTestManager) awaitEnvironments(t *testing.T, projsAndEnvs projsAndEnvs, expectations *envPropertyExpectations, envMapKeyFn func(proj projectInfo, env environmentInfo) string) { _, success := m.awaitRelayStatus(t, func(status api.StatusRep) bool { if len(status.Environments) != projsAndEnvs.countEnvs() { return false @@ -313,7 +312,7 @@ func (m *integrationTestManager) awaitEnvironments(t *testing.T, projsAndEnvs pr projsAndEnvs.enumerateEnvs(func(proj projectInfo, env environmentInfo) { mapKey := envMapKeyFn(proj, env) if envStatus, found := status.Environments[mapKey]; found { - verifyEnvProperties(t, proj, env, envStatus, expectNameAndKey) + verifyEnvProperties(t, proj, env, envStatus, expectations) if envStatus.Status != "connected" { ok = false } @@ -328,6 +327,25 @@ func (m *integrationTestManager) awaitEnvironments(t *testing.T, projsAndEnvs pr } } +func (m *integrationTestManager) rotateSDKKeys(t *testing.T, existing projsAndEnvs, expiry time.Time) projsAndEnvs { + updated := make(projsAndEnvs) + for proj, envs := range existing { + updated[proj] = make([]environmentInfo, 0) + for _, env := range envs { + newKey, err := m.apiHelper.rotateSDKKey(proj, env, expiry) + require.NoError(t, err, "failed to rotate SDK key for environment %s", env.id) + if expiry.IsZero() { + env.expiringSdkKey = "" + } else { + env.expiringSdkKey = env.sdkKey + } + env.sdkKey = newKey + updated[proj] = append(updated[proj], env) + } + } + return updated +} + // verifyFlagValues hits Relay's polling evaluation endpoint and verifies that it returns the expected // flags and values, based on the standard way we create flags for our test environments in createFlag. func (m *integrationTestManager) verifyFlagValues(t *testing.T, projsAndEnvs projsAndEnvs) { @@ -454,14 +472,39 @@ func (m *integrationTestManager) withExtraContainer( action(container) } -func verifyEnvProperties(t *testing.T, project projectInfo, environment environmentInfo, envStatus api.EnvironmentStatusRep, expectNameAndKey bool) { +// Expectations of the test when checking the status response from Relay. +type envPropertyExpectations struct { + // The environment/project have names + keys + nameAndKey bool + // The environments have sdkKey and expiringSdkKey that match the expected values. Matching is determined + // by masking the keys and comparing those masked values, since the status response obscures most of the key except + // for the last few characters. + sdkKeys bool +} + +func verifyEnvProperties(t *testing.T, project projectInfo, environment environmentInfo, envStatus api.EnvironmentStatusRep, expectations *envPropertyExpectations) { assert.Equal(t, string(environment.id), envStatus.EnvID) - if expectNameAndKey { + if expectations == nil { + return + } + if expectations.nameAndKey { assert.Equal(t, environment.name, envStatus.EnvName) assert.Equal(t, environment.key, envStatus.EnvKey) assert.Equal(t, project.name, envStatus.ProjName) assert.Equal(t, project.key, envStatus.ProjKey) } + if expectations.sdkKeys { + if !environment.expiringSdkKey.Defined() { + assert.Empty(t, envStatus.ExpiringSDKKey, "expected no expiring SDK key to be defined") + } else { + assert.Equal(t, environment.expiringSdkKey.Masked(), config.SDKKey(envStatus.ExpiringSDKKey).Masked(), "expected expiring SDK key to match") + } + if !environment.sdkKey.Defined() { + assert.Empty(t, envStatus.SDKKey, "expected no SDK key to be defined") + } else { + assert.Equal(t, environment.sdkKey.Masked(), config.SDKKey(envStatus.SDKKey).Masked(), "expected SDK key to match") + } + } } func flagKeyForProj(proj projectInfo) string { diff --git a/internal/credential/rotator.go b/internal/credential/rotator.go index 3ece1494..da9b167f 100644 --- a/internal/credential/rotator.go +++ b/internal/credential/rotator.go @@ -145,6 +145,11 @@ type GracePeriod struct { now time.Time } +// Expired returns true if the key has already expired. +func (g *GracePeriod) Expired() bool { + return g.now.After(g.expiry) +} + // NewGracePeriod constructs a new grace period. The current time must be provided in order to // determine if the credential is already expired. func NewGracePeriod(key config.SDKKey, expiry time.Time, now time.Time) *GracePeriod { @@ -218,38 +223,53 @@ func (r *Rotator) swapPrimaryKey(newKey config.SDKKey) config.SDKKey { return previous } + +func (r *Rotator) immediatelyRevoke(key config.SDKKey) { + if key.Defined() { + r.expirations = append(r.expirations, key) + r.loggers.Infof("SDK key %s has been immediately revoked", key.Masked()) + } +} + func (r *Rotator) updateSDKKey(sdkKey config.SDKKey, grace *GracePeriod) { r.mu.Lock() defer r.mu.Unlock() + // Previous will only be .Defined() if there was a previous primary key. previous := r.swapPrimaryKey(sdkKey) - // Immediately revoke the previous SDK key if there's no explicit deprecation notice, otherwise it would - // hang around forever. - if previous.Defined() && grace == nil { - r.expirations = append(r.expirations, previous) - r.loggers.Infof("SDK key %s has been immediately revoked", previous.Masked()) + + // If there's no deprecation notice, then the previous key (if any) needs to be immediately revoked so it doesn't + // hang around forever. This case is also true when there is a grace period, but we need to inspect the grace period + // in order to find out if immediate revocation is necessary. + if grace == nil { + r.immediatelyRevoke(previous) return } - if grace != nil { - if previousExpiry, ok := r.deprecatedSdkKeys[grace.key]; ok { - if previousExpiry != grace.expiry { - r.loggers.Warnf("SDK key %s was marked for deprecation with an expiry at %v, but it was previously deprecated with an expiry at %v. The previous expiry will be used. ", grace.key.Masked(), grace.expiry, previousExpiry) - } - return - } - if grace.now.After(grace.expiry) { - r.loggers.Infof("Deprecated SDK key %s already expired; ignoring", grace.key.Masked()) - return + if previousExpiry, ok := r.deprecatedSdkKeys[grace.key]; ok { + if previousExpiry != grace.expiry { + r.loggers.Warnf("SDK key %s was marked for deprecation with an expiry at %v, but it was previously deprecated with an expiry at %v. The previous expiry will be used. ", grace.key.Masked(), grace.expiry, previousExpiry) } + // When a key is deprecated by LD, it will stick around in the deprecated field of the message until something + // else is deprecated. This means that if a key is rotated *without* a deprecation period set for the previous key, + // then we'll receive that new primary key but the deprecation message will be stale - it'll be referring to the + // last time a key was rotated with a deprecation period. We detect this case here (since we already saw the + // deprecation message in our map) and ensure the previous key is revoked. + r.immediatelyRevoke(previous) + return + } - r.loggers.Infof("SDK key %s was marked for deprecation with an expiry at %v", grace.key.Masked(), grace.expiry) - r.deprecatedSdkKeys[grace.key] = grace.expiry + if grace.Expired() { + r.loggers.Infof("Deprecated SDK key %s already expired at %v; ignoring", grace.key.Masked(), grace.expiry) + return + } - if grace.key != previous { - r.loggers.Infof("Deprecated SDK key %s was not previously managed by Relay", grace.key.Masked()) - r.additions = append(r.additions, grace.key) - } + r.loggers.Infof("SDK key %s was marked for deprecation with an expiry at %v", grace.key.Masked(), grace.expiry) + r.deprecatedSdkKeys[grace.key] = grace.expiry + + if grace.key != previous { + r.loggers.Infof("Deprecated SDK key %s was not previously managed by Relay", grace.key.Masked()) + r.additions = append(r.additions, grace.key) } } diff --git a/internal/credential/rotator_test.go b/internal/credential/rotator_test.go index 9acbc57f..1cb23336 100644 --- a/internal/credential/rotator_test.go +++ b/internal/credential/rotator_test.go @@ -111,6 +111,40 @@ func TestManyImmediateKeyExpirations(t *testing.T) { } } +func TestImmediateSDKKeyDeprecationEvenIfGracePeriodIsPresent(t *testing.T) { + mockLog := ldlogtest.NewMockLog() + rotator := NewRotator(mockLog.Loggers) + + key0 := config.SDKKey("key0") + key1 := config.SDKKey("key1") + key2 := config.SDKKey("key2") + + rotator.Initialize([]SDKCredential{key0}) + + start := time.Unix(1000, 0) + halftime := start.Add(30 * time.Minute) + expiry := start.Add(1 * time.Hour) + + rotator.RotateWithGrace(key1, NewGracePeriod(key0, expiry, start)) + + additions, expirations := rotator.StepTime(halftime) + assert.ElementsMatch(t, []SDKCredential{key1}, additions) + assert.Empty(t, expirations) + + // The deprecated key0 given here can be thought of as "stale" or otherwise already-seen by the rotator. + // In this case, it should be effectively ignored but the new key2 should still trigger rotation of the previous + // primary key. + rotator.RotateWithGrace(key2, NewGracePeriod(key0, expiry, halftime)) + + additions, expirations = rotator.StepTime(halftime) + assert.ElementsMatch(t, []SDKCredential{key2}, additions) + assert.ElementsMatch(t, []SDKCredential{key1}, expirations) + + additions, expirations = rotator.StepTime(expiry.Add(1 * time.Millisecond)) + assert.Empty(t, additions) + assert.ElementsMatch(t, []SDKCredential{key0}, expirations) +} + func TestSDKKeyDeprecation(t *testing.T) { mockLog := ldlogtest.NewMockLog() rotator := NewRotator(mockLog.Loggers) diff --git a/internal/envfactory/env_config_factory.go b/internal/envfactory/env_config_factory.go index 39cc78bd..025fb867 100644 --- a/internal/envfactory/env_config_factory.go +++ b/internal/envfactory/env_config_factory.go @@ -15,10 +15,11 @@ type EnvConfigFactory struct { // DataStorePrefix is the configured data store prefix, which may contain a per-environment placeholder. DataStorePrefix string // DataStorePrefix is the configured data store table name, which may contain a per-environment placeholder. - TableName string - // + TableName string AllowedOrigin ct.OptStringList AllowedHeader ct.OptStringList + // Whether this factory is used for offline mode or not. + Offline bool } // NewEnvConfigFactoryForAutoConfig creates an EnvConfigFactory based on the auto-configuration mode settings. @@ -28,6 +29,7 @@ func NewEnvConfigFactoryForAutoConfig(c config.AutoConfigConfig) EnvConfigFactor TableName: c.EnvDatastoreTableName, AllowedOrigin: c.EnvAllowedOrigin, AllowedHeader: c.EnvAllowedHeader, + Offline: false, } } @@ -38,6 +40,7 @@ func NewEnvConfigFactoryForOfflineMode(c config.OfflineModeConfig) EnvConfigFact TableName: c.EnvDatastoreTableName, AllowedOrigin: c.EnvAllowedOrigin, AllowedHeader: c.EnvAllowedHeader, + Offline: true, } } @@ -54,6 +57,7 @@ func (f EnvConfigFactory) MakeEnvironmentConfig(params EnvironmentParams) config AllowedHeader: f.AllowedHeader, SecureMode: params.SecureMode, FilterKey: params.Identifiers.FilterKey, + Offline: f.Offline, } if params.TTL != 0 { ret.TTL = ct.NewOptDuration(params.TTL) diff --git a/internal/relayenv/env_context_impl.go b/internal/relayenv/env_context_impl.go index a5b271f6..301ec5af 100644 --- a/internal/relayenv/env_context_impl.go +++ b/internal/relayenv/env_context_impl.go @@ -120,6 +120,7 @@ type envContextImpl struct { stopMonitoringCredentials chan struct{} doneMonitoringCredentials chan struct{} connectionMapper ConnectionMapper + offline bool } // Implementation of the DataStoreQueries interface that the streams package uses as an abstraction of @@ -190,6 +191,7 @@ func NewEnvContext( stopMonitoringCredentials: make(chan struct{}), doneMonitoringCredentials: make(chan struct{}), connectionMapper: params.ConnectionMapper, + offline: envConfig.Offline, } envContext.keyRotator.Initialize([]credential.SDKCredential{ @@ -428,12 +430,20 @@ func (c *envContextImpl) addCredential(newCredential credential.SDKCredential) { } } - // A new SDK key means 1. we should start a new SDK client, 2. we should tell all event forwarding - // components that use an SDK key to use the new one. A new mobile key does not require starting a - // new SDK client, but does requiring updating any event forwarding components that use a mobile key. + // A new SDK key means: + // 1. we should start a new SDK client* + // 2. we should tell all event forwarding components that use an SDK key to use the new one. + // A new mobile key does not require starting a new SDK client, but does requiring updating any event forwarding + // components that use a mobile key. + // *Note: we only start a new SDK client in online mode. This is somewhat of an architectural hack because EnvContextImpl + // is used for both offline and online mode, yet starting up an SDK client is only relevant in online mode. This is + // because in offline mode, we already have the data (from a file) - there's no need to open a new streaming connection. + // So, the effect in offline mode when adding/removing credentials is just setting up the new credential mappings. switch key := newCredential.(type) { case config.SDKKey: - go c.startSDKClient(key, nil, false) + if !c.offline { + go c.startSDKClient(key, nil, false) + } if c.metricsEventPub != nil { // metrics event publisher always uses SDK key c.metricsEventPub.ReplaceCredential(key) } @@ -457,11 +467,15 @@ func (c *envContextImpl) removeCredential(oldCredential credential.SDKCredential for _, handlers := range c.handlers { delete(handlers, oldCredential) } - if sdkKey, ok := oldCredential.(config.SDKKey); ok { - // The SDK client instance is tied to the SDK key, so get rid of it - if client := c.clients[sdkKey]; client != nil { - delete(c.clients, sdkKey) - _ = client.Close() + // See the comment in addCredential for more context. In offline mode, there's no need to close the SDK client + // because our data comes from a file, not a streaming connection. + if !c.offline { + if sdkKey, ok := oldCredential.(config.SDKKey); ok { + // The SDK client instance is tied to the SDK key, so get rid of it + if client := c.clients[sdkKey]; client != nil { + delete(c.clients, sdkKey) + _ = client.Close() + } } } } @@ -560,6 +574,16 @@ func (c *envContextImpl) GetDeprecatedCredentials() []credential.SDKCredential { func (c *envContextImpl) GetClient() sdks.LDClientContext { c.mu.RLock() defer c.mu.RUnlock() + // In offline mode, there's only one SDK client. This is awkward because we represent the active clients + // as a map, but in this case there's only one client in the map. A refactoring might pull this logic (along with + // differences in add/removeCredential into an interface that is injected based on the environment being + // offline or online. + if c.offline { + for _, client := range c.clients { + return client + } + return nil + } return c.clients[c.keyRotator.SDKKey()] } diff --git a/relay/filedata_actions.go b/relay/filedata_actions.go index 8e600321..1528ccd1 100644 --- a/relay/filedata_actions.go +++ b/relay/filedata_actions.go @@ -3,6 +3,8 @@ package relay import ( "time" + "github.com/launchdarkly/ld-relay/v8/internal/relayenv" + "github.com/launchdarkly/ld-relay/v8/internal/sdkauth" "github.com/launchdarkly/ld-relay/v8/internal/envfactory" @@ -49,11 +51,15 @@ func (a *relayFileDataActions) AddEnvironment(ae filedata.ArchiveEnvironment) { return config } envConfig := envfactory.NewEnvConfigFactoryForOfflineMode(a.r.config.OfflineMode).MakeEnvironmentConfig(ae.Params) - _, _, err := a.r.addEnvironment(ae.Params.Identifiers, envConfig, transformConfig) + env, _, err := a.r.addEnvironment(ae.Params.Identifiers, envConfig, transformConfig) if err != nil { a.r.loggers.Errorf(logMsgAutoConfEnvInitError, ae.Params.Identifiers.GetDisplayName(), err) return } + if ae.Params.ExpiringSDKKey.Defined() { + update := relayenv.NewCredentialUpdate(ae.Params.SDKKey) + env.UpdateCredential(update.WithGracePeriod(ae.Params.ExpiringSDKKey.Key, ae.Params.ExpiringSDKKey.Expiration)) + } select { case updates := <-updatesCh: if a.envUpdates == nil { @@ -83,6 +89,17 @@ func (a *relayFileDataActions) UpdateEnvironment(ae filedata.ArchiveEnvironment) env.SetTTL(ae.Params.TTL) env.SetSecureMode(ae.Params.SecureMode) + if ae.Params.MobileKey.Defined() { + env.UpdateCredential(relayenv.NewCredentialUpdate(ae.Params.MobileKey)) + } + if ae.Params.SDKKey.Defined() { + update := relayenv.NewCredentialUpdate(ae.Params.SDKKey) + if ae.Params.ExpiringSDKKey.Defined() { + update = update.WithGracePeriod(ae.Params.ExpiringSDKKey.Key, ae.Params.ExpiringSDKKey.Expiration) + } + env.UpdateCredential(update) + } + // SDKData will be non-nil only if the flag/segment data for the environment has actually changed. if ae.SDKData != nil { updates.Init(ae.SDKData) diff --git a/relay/filedata_actions_test.go b/relay/filedata_actions_test.go index 221239ab..d13a4118 100644 --- a/relay/filedata_actions_test.go +++ b/relay/filedata_actions_test.go @@ -1,12 +1,15 @@ package relay import ( + "fmt" "net/http" "net/http/httptest" "sort" "testing" "time" + "github.com/launchdarkly/ld-relay/v8/internal/credential" + "github.com/launchdarkly/ld-relay/v8/config" "github.com/launchdarkly/ld-relay/v8/internal/filedata" "github.com/launchdarkly/ld-relay/v8/internal/sharedtest" @@ -90,7 +93,11 @@ func offlineModeTest( } func (p offlineModeTestParams) awaitClient() testclient.CapturedLDClient { - return helpers.RequireValue(p.t, p.clientsCreatedCh, time.Second, "timed out waiting for client creation") + return p.awaitClientFor(time.Second) +} + +func (p offlineModeTestParams) awaitClientFor(duration time.Duration) testclient.CapturedLDClient { + return helpers.RequireValue(p.t, p.clientsCreatedCh, duration, "timed out waiting for client creation") } func (p offlineModeTestParams) shouldNotCreateClient(timeout time.Duration) { @@ -205,3 +212,96 @@ func TestOfflineModeEventsAreAcceptedAndDiscardedIfSendEventsIsTrue(t *testing.T }) }) } + +func TestOfflineModeDeprecatedSDKKeyIsRespectedIfExpiryInFuture(t *testing.T) { + // The goal here is to validate that if we load an offline mode archive containing a deprecated key, + // it will be added as a credential (even though it was never previously seen as a primary key.) This situation + // would happen when Relay is starting up at time T if the key was deprecated at a time before T. + + offlineModeTest(t, config.Config{}, func(p offlineModeTestParams) { + + envData := RotateSDKKeyWithGracePeriod("primary-key", "deprecated-key", time.Now().Add(1*time.Hour)) + + p.updateHandler.AddEnvironment(envData) + + client1 := p.awaitClient() + assert.Equal(t, envData.Params.SDKKey, client1.Key) + + env := p.awaitEnvironment(testFileDataEnv1.Params.EnvID) + + assert.ElementsMatch(t, []credential.SDKCredential{envData.Params.SDKKey, envData.Params.EnvID}, env.GetCredentials()) + assert.ElementsMatch(t, []credential.SDKCredential{envData.Params.ExpiringSDKKey.Key}, env.GetDeprecatedCredentials()) + }) +} + +func TestOfflineModePrimarySDKKeyIsDeprecated(t *testing.T) { + offlineModeTest(t, config.Config{}, func(p offlineModeTestParams) { + update1 := RotateSDKKey("key1") + + p.updateHandler.AddEnvironment(update1) + + expectedClient1 := p.awaitClient() + assert.Equal(t, update1.Params.SDKKey, expectedClient1.Key) + + env := p.awaitEnvironment(update1.Params.EnvID) + + assert.ElementsMatch(t, []credential.SDKCredential{update1.Params.SDKKey, update1.Params.EnvID}, env.GetCredentials()) + assert.Empty(t, env.GetDeprecatedCredentials()) + + update2 := RotateSDKKeyWithGracePeriod("key2", "key1", time.Now().Add(1*time.Hour)) + p.updateHandler.UpdateEnvironment(update2) + + assert.ElementsMatch(t, []credential.SDKCredential{update2.Params.SDKKey, update1.Params.EnvID}, env.GetCredentials()) + assert.ElementsMatch(t, []credential.SDKCredential{update2.Params.ExpiringSDKKey.Key}, env.GetDeprecatedCredentials()) + + update3 := RotateSDKKey("key3") + p.updateHandler.UpdateEnvironment(update3) + + assert.ElementsMatch(t, []credential.SDKCredential{update3.Params.SDKKey, update1.Params.EnvID}, env.GetCredentials()) + + // Note: key2 isn't in the deprecated list, because update3 was an immediate rotation (with no grace period for the + // previous key.) At the same time, key1 is still deprecated until the hour is up. + assert.ElementsMatch(t, []credential.SDKCredential{update2.Params.ExpiringSDKKey.Key}, env.GetDeprecatedCredentials()) + }) +} + +func TestOfflineModeSDKKeyCanExpire(t *testing.T) { + // This test aims to deprecate an SDK key, sleep until the expiry, and then verify that the + // key is no longer accepted. + // + // This test is extremely timing dependent, because we're unable to easily inject a mocked time + // into the lower level components under test. + + // Instead, we configure the credential cleanup interval to be as short as possible (100ms) + // and then sleep at least that amount of time after specifying a key expiry. The intention is to + // simulate a real scenario, but fast enough for a test. + + const minimumCleanupInterval = 100 * time.Millisecond + + cfg := config.Config{} + cfg.Main.ExpiredCredentialCleanupInterval = configtypes.NewOptDuration(minimumCleanupInterval) + + offlineModeTest(t, cfg, func(p offlineModeTestParams) { + + for i := 0; i < 3; i++ { + primary := config.SDKKey(fmt.Sprintf("key%v", i+1)) + expiring := config.SDKKey(fmt.Sprintf("key%v", i)) + + // It's important that the expiry be in the future (so that the key isn't ignored by the key rotator + // component), but it should also be in the near future so the test doesn't need to sleep long. + keyExpiry := time.Now().Add(10 * time.Millisecond) + update1 := RotateSDKKeyWithGracePeriod(primary, expiring, keyExpiry) + p.updateHandler.AddEnvironment(update1) + + // Waiting for the environment can take up to 1 second, but it could be much faster. In any case + // we'll still need to sleep at least the cleanup interval to ensure the key is expired. + env := p.awaitEnvironmentFor(update1.Params.EnvID, time.Second) + assert.ElementsMatch(t, []credential.SDKCredential{update1.Params.SDKKey, update1.Params.EnvID}, env.GetCredentials()) + assert.ElementsMatch(t, []credential.SDKCredential{update1.Params.ExpiringSDKKey.Key}, env.GetDeprecatedCredentials()) + + time.Sleep(minimumCleanupInterval) + assert.ElementsMatch(t, []credential.SDKCredential{update1.Params.SDKKey, update1.Params.EnvID}, env.GetCredentials()) + assert.Empty(t, env.GetDeprecatedCredentials()) + } + }) +} diff --git a/relay/filedata_testdata_test.go b/relay/filedata_testdata_test.go index 3f6b0c79..d097a4ce 100644 --- a/relay/filedata_testdata_test.go +++ b/relay/filedata_testdata_test.go @@ -1,6 +1,8 @@ package relay import ( + "time" + "github.com/launchdarkly/ld-relay/v8/config" "github.com/launchdarkly/ld-relay/v8/internal/envfactory" "github.com/launchdarkly/ld-relay/v8/internal/filedata" @@ -58,3 +60,33 @@ var testFileDataEnv2 = filedata.ArchiveEnvironment{ }, }, } + +func RotateSDKKey(primary config.SDKKey) filedata.ArchiveEnvironment { + return RotateSDKKeyWithGracePeriod(primary, "", time.Time{}) +} + +func RotateSDKKeyWithGracePeriod(primary config.SDKKey, expiring config.SDKKey, expiry time.Time) filedata.ArchiveEnvironment { + return filedata.ArchiveEnvironment{ + Params: envfactory.EnvironmentParams{ + EnvID: "env1", + SDKKey: primary, + ExpiringSDKKey: envfactory.ExpiringSDKKey{ + Key: expiring, + Expiration: expiry, + }, + Identifiers: relayenv.EnvIdentifiers{ + ProjName: "Project", + ProjKey: "project", + EnvName: "Env1", + EnvKey: "env1", + }, + }, + SDKData: []ldstoretypes.Collection{ + { + Kind: ldstoreimpl.Features(), + Items: []ldstoretypes.KeyedItemDescriptor{ + {Key: testFileDataFlag1.Key, Item: sharedtest.FlagDesc(testFileDataFlag1)}, + }, + }, + }} +} diff --git a/relay/testutils_test.go b/relay/testutils_test.go index c13225ae..e3037143 100644 --- a/relay/testutils_test.go +++ b/relay/testutils_test.go @@ -26,17 +26,21 @@ type relayTestHelper struct { relay *Relay } -func (h relayTestHelper) awaitEnvironment(envID c.EnvironmentID) relayenv.EnvContext { +func (h relayTestHelper) awaitEnvironmentFor(envID c.EnvironmentID, duration time.Duration) relayenv.EnvContext { h.t.Helper() var e relayenv.EnvContext var err error require.Eventually(h.t, func() bool { e, err = h.relay.getEnvironment(sdkauth.New(envID)) return err == nil - }, time.Second, time.Millisecond*5) + }, duration, time.Millisecond*5) return e } +func (h relayTestHelper) awaitEnvironment(envID c.EnvironmentID) relayenv.EnvContext { + return h.awaitEnvironmentFor(envID, time.Second) +} + func (h relayTestHelper) shouldNotHaveEnvironment(envID c.EnvironmentID, timeout time.Duration) { h.t.Helper() require.Eventually(h.t, func() bool {