From 97339d9336ec67568e9e7fd079b3cfe006da1bba Mon Sep 17 00:00:00 2001 From: Brian Krygsman Date: Tue, 14 Mar 2023 06:14:11 -0400 Subject: [PATCH] feat(parameters): AppConfigProvider to return the last valid value when the API returns empty value on subsequent calls (#1365) * Closes 1363 Add a local cache to AppConfigProvider to handle the scenario where the maxAge is expired, but the AppConfig session still has the latest configuration. In this case, AppConfig returns an empty value. We don't want to return empty values to our callers. This uses the same data structure we have in place to track NextPollConfigurationToken. This approach roughly the Python library's behavior. * Add E2E test coverage checking return values pre- and post-expiration Includes update to e2e lambda config type that allows test writers to specify a test function duration. Needed because to wait for parameter expiration, we needed a longer-than-default timeout. * Use timestamp/millisecond based math for checking expiration. * Revert "Use timestamp/millisecond based math for checking expiration." This reverts commit 3fea0d7e826a6d184b04f6adc8efc62bc1751069. Further testing shows that the original algorithm was correct. * Update appConfig integration tests to use single log line. Drop wait, use maxAge instead. Add comment for Test case 7. Fix test case naming (test '4' was skipped on one of the files). * Revert increase of Lambda function duration back to default. Not needed anymore since we're not waiting for Test 7. * Focus value test integration tests on Test 7 * Comma formatting * Add unit tests for ExpirableValue Doesn't refactor ExpirableValue to take an injection of Date.now, just implements tests based on what we can do with the existing interface. * Address formatting feedback and updates comments to indicate we're no longer waiting during Test 7 runs. * Adjust log message structure to match pattern Specfically test/value as top-level properties. * Move client reset to afterEach for consistency. * Drop old reset --- packages/commons/tests/utils/e2eUtils.ts | 4 +- .../src/appconfig/AppConfigProvider.ts | 20 +++- ...pConfigProvider.class.test.functionCode.ts | 37 +++++++- .../tests/e2e/appConfigProvider.class.test.ts | 29 +++++- .../tests/unit/AppConfigProvider.test.ts | 93 +++++++++++++++++++ 5 files changed, 176 insertions(+), 7 deletions(-) diff --git a/packages/commons/tests/utils/e2eUtils.ts b/packages/commons/tests/utils/e2eUtils.ts index 5d2e75f4a5..ae5c1441ae 100644 --- a/packages/commons/tests/utils/e2eUtils.ts +++ b/packages/commons/tests/utils/e2eUtils.ts @@ -5,7 +5,7 @@ * E2E utils is used by e2e tests. They are helper function that calls either CDK or SDK * to interact with services. */ -import { App, CfnOutput, Stack } from 'aws-cdk-lib'; +import { App, CfnOutput, Stack, Duration } from 'aws-cdk-lib'; import { NodejsFunction, NodejsFunctionProps @@ -37,6 +37,7 @@ export type StackWithLambdaFunctionOptions = { runtime: string bundling?: NodejsFunctionProps['bundling'] layers?: NodejsFunctionProps['layers'] + timeout?: Duration }; type FunctionPayload = {[key: string]: string | boolean | number}; @@ -55,6 +56,7 @@ export const createStackWithLambdaFunction = (params: StackWithLambdaFunctionOpt bundling: params.bundling, layers: params.layers, logRetention: RetentionDays.ONE_DAY, + timeout: params.timeout }); if (params.logGroupOutputKey) { diff --git a/packages/parameters/src/appconfig/AppConfigProvider.ts b/packages/parameters/src/appconfig/AppConfigProvider.ts index 9e9e2b0152..ce42545055 100644 --- a/packages/parameters/src/appconfig/AppConfigProvider.ts +++ b/packages/parameters/src/appconfig/AppConfigProvider.ts @@ -13,6 +13,7 @@ import type { class AppConfigProvider extends BaseProvider { public client: AppConfigDataClient; protected configurationTokenStore: Map = new Map(); + protected valueStore: Map = new Map(); private application?: string; private environment: string; @@ -79,6 +80,10 @@ class AppConfigProvider extends BaseProvider { * The new AppConfig APIs require two API calls to return the configuration * First we start the session and after that we retrieve the configuration * We need to store { name: token } pairs to use in the next execution + * We also need to store { name : value } pairs because AppConfig returns + * an empty value if the session already has the latest configuration + * but, we don't want to return an empty value to our callers. + * {@link https://docs.aws.amazon.com/appconfig/latest/userguide/appconfig-retrieving-the-configuration.html} **/ if (!this.configurationTokenStore.has(name)) { @@ -106,14 +111,25 @@ class AppConfigProvider extends BaseProvider { }); const response = await this.client.send(getConfigurationCommand); - + if (response.NextPollConfigurationToken) { this.configurationTokenStore.set(name, response.NextPollConfigurationToken); } else { this.configurationTokenStore.delete(name); } - return response.Configuration; + /** When the response is not empty, stash the result locally before returning + * See AppConfig docs: + * {@link https://docs.aws.amazon.com/appconfig/latest/userguide/appconfig-retrieving-the-configuration.html} + **/ + if (response.Configuration !== undefined && response.Configuration?.length > 0 ) { + this.valueStore.set(name, response.Configuration); + + return response.Configuration; + } + + // Otherwise, use a stashed value + return this.valueStore.get(name); } /** diff --git a/packages/parameters/tests/e2e/appConfigProvider.class.test.functionCode.ts b/packages/parameters/tests/e2e/appConfigProvider.class.test.functionCode.ts index 11d8feb01f..3633b7d5d6 100644 --- a/packages/parameters/tests/e2e/appConfigProvider.class.test.functionCode.ts +++ b/packages/parameters/tests/e2e/appConfigProvider.class.test.functionCode.ts @@ -73,10 +73,10 @@ export const handler = async (_event: unknown, _context: Context): Promise // Test 3 - get a free-form base64-encoded plain text and apply binary transformation (should return a decoded string) await _call_get(freeFormBase64encodedPlainText, 'get-freeform-base64-plaintext-binary', { transform: 'binary' }); - // Test 5 - get a feature flag and apply json transformation (should return an object) + // Test 4 - get a feature flag and apply json transformation (should return an object) await _call_get(featureFlagName, 'get-feature-flag-binary', { transform: 'json' }); - // Test 6 + // Test 5 // get parameter twice with middleware, which counts the number of requests, we check later if we only called AppConfig API once try { providerWithMiddleware.clearCache(); @@ -94,7 +94,7 @@ export const handler = async (_event: unknown, _context: Context): Promise }); } - // Test 7 + // Test 6 // get parameter twice, but force fetch 2nd time, we count number of SDK requests and check that we made two API calls try { providerWithMiddleware.clearCache(); @@ -111,4 +111,35 @@ export const handler = async (_event: unknown, _context: Context): Promise error: err.message }); } + // Test 7 + // get parameter twice, using maxAge to avoid primary cache, count SDK calls and return values + try { + providerWithMiddleware.clearCache(); + middleware.counter = 0; + const expiredResult1 = await providerWithMiddleware.get( + freeFormBase64encodedPlainText, { + maxAge: 0, + transform: 'base64' + } + ); + const expiredResult2 = await providerWithMiddleware.get( + freeFormBase64encodedPlainText, { + maxAge: 0, + transform: 'base64' + } + ); + logger.log({ + test: 'get-expired', + value: { + counter: middleware.counter, // should be 2 + result1: expiredResult1, + result2: expiredResult2 + }, + }); + } catch (err) { + logger.log({ + test: 'get-expired', + error: err.message + }); + } }; \ No newline at end of file diff --git a/packages/parameters/tests/e2e/appConfigProvider.class.test.ts b/packages/parameters/tests/e2e/appConfigProvider.class.test.ts index 02b2de3291..26b6210ea2 100644 --- a/packages/parameters/tests/e2e/appConfigProvider.class.test.ts +++ b/packages/parameters/tests/e2e/appConfigProvider.class.test.ts @@ -54,6 +54,7 @@ const freeFormJsonValue = { const freeFormYamlValue = `foo: bar `; const freeFormPlainTextValue = 'foo'; +const freeFormBase64PlainTextValue = toBase64(new TextEncoder().encode(freeFormPlainTextValue)); const featureFlagValue = { version: '1', flags: { @@ -111,6 +112,12 @@ let stack: Stack; * Test 6 * get parameter twice, but force fetch 2nd time, we count number of SDK requests and * check that we made two API calls + * check that we got matching results + * + * Test 7 + * get parameter twice, using maxAge to avoid primary cache + * we count number of SDK requests and check that we made two API calls + * and check that the values match * * Note: To avoid race conditions, we add a dependency between each pair of configuration profiles. * This allows us to influence the order of creation and ensure that each configuration profile @@ -191,7 +198,7 @@ describe(`parameters E2E tests (appConfigProvider) for runtime ${runtime}`, () = name: freeFormBase64PlainTextName, type: 'AWS.Freeform', content: { - content: toBase64(new TextEncoder().encode(freeFormPlainTextValue)), + content: freeFormBase64PlainTextValue, contentType: 'text/plain', } }); @@ -314,6 +321,26 @@ describe(`parameters E2E tests (appConfigProvider) for runtime ${runtime}`, () = }, TEST_CASE_TIMEOUT); + // Test 7 - get parameter twice, using maxAge to avoid primary cache + // we count number of SDK requests and check that we made two API calls + // and check that the values match + it('should retrieve single parameter twice, with expiration between and matching values', async () => { + + const logs = invocationLogs[0].getFunctionLogs(); + const testLog = InvocationLogs.parseFunctionLog(logs[6]); + const result = freeFormBase64PlainTextValue; + + expect(testLog).toStrictEqual({ + test: 'get-expired', + value: { + counter: 2, + result1: result, + result2: result + } + }); + + }, TEST_CASE_TIMEOUT); + }); afterAll(async () => { diff --git a/packages/parameters/tests/unit/AppConfigProvider.test.ts b/packages/parameters/tests/unit/AppConfigProvider.test.ts index 56365d882c..5bc16e3191 100644 --- a/packages/parameters/tests/unit/AppConfigProvider.test.ts +++ b/packages/parameters/tests/unit/AppConfigProvider.test.ts @@ -4,6 +4,7 @@ * @group unit/parameters/AppConfigProvider/class */ import { AppConfigProvider } from '../../src/appconfig/index'; +import { ExpirableValue } from '../../src/ExpirableValue'; import { AppConfigProviderOptions } from '../../src/types/AppConfigProvider'; import { AppConfigDataClient, @@ -21,6 +22,10 @@ describe('Class: AppConfigProvider', () => { jest.clearAllMocks(); }); + afterEach(() => { + client.reset(); + }); + describe('Method: constructor', () => { test('when the class instantiates without SDK client and client config it has default options', async () => { // Prepare @@ -225,6 +230,49 @@ describe('Class: AppConfigProvider', () => { // Act & Assess await expect(provider.get(name)).rejects.toThrow(); }); + + test('when session returns an empty configuration on the second call, it returns the last value', async () => { + + // Prepare + const options: AppConfigProviderOptions = { + application: 'MyApp', + environment: 'MyAppProdEnv', + }; + const provider = new AppConfigProvider(options); + const name = 'MyAppFeatureFlag'; + + const fakeInitialToken = 'aW5pdGlhbFRva2Vu'; + const fakeNextToken1 = 'bmV4dFRva2Vu'; + const fakeNextToken2 = 'bmV4dFRva2Vq'; + const mockData = encoder.encode('myAppConfiguration'); + + client + .on(StartConfigurationSessionCommand) + .resolves({ + InitialConfigurationToken: fakeInitialToken, + }) + .on(GetLatestConfigurationCommand) + .resolvesOnce({ + Configuration: mockData, + NextPollConfigurationToken: fakeNextToken1, + }) + .resolvesOnce({ + Configuration: undefined, + NextPollConfigurationToken: fakeNextToken2, + }); + + // Act + + // Load local cache + const result1 = await provider.get(name, { forceFetch: true }); + + // Read from local cache, given empty response from service + const result2 = await provider.get(name, { forceFetch: true }); + + // Assess + expect(result1).toBe(mockData); + expect(result2).toBe(mockData); + }); }); describe('Method: _getMultiple', () => { @@ -243,3 +291,48 @@ describe('Class: AppConfigProvider', () => { }); }); }); + +describe('Class: ExpirableValue', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('Method: constructor', () => { + test('when created, it has ttl set to at least maxAge seconds from test start', () => { + // Prepare + const seconds = 10; + const nowTimestamp = Date.now(); + const futureTimestampSeconds = nowTimestamp/1000+(seconds); + + // Act + const expirableValue = new ExpirableValue('foo', seconds); + + // Assess + expect(expirableValue.ttl).toBeGreaterThan(futureTimestampSeconds); + }); + }); + + describe('Method: isExpired', () => { + test('when called, it returns true when maxAge is in the future', () => { + // Prepare + const seconds = 60; + + // Act + const expirableValue = new ExpirableValue('foo', seconds); + + // Assess + expect(expirableValue.isExpired()).toBeFalsy(); + }); + + test('when called, it returns false when maxAge is in the past', () => { + // Prepare + const seconds = -60; + + // Act + const expirableValue = new ExpirableValue('foo', seconds); + + // Assess + expect(expirableValue.isExpired()).toBeTruthy(); + }); + }); +}); \ No newline at end of file