Skip to content

feat(parameters): AppConfigProvider to return the last valid value when the API returns empty value on subsequent calls #1365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
4 changes: 3 additions & 1 deletion packages/commons/tests/utils/e2eUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -37,6 +37,7 @@ export type StackWithLambdaFunctionOptions = {
runtime: string
bundling?: NodejsFunctionProps['bundling']
layers?: NodejsFunctionProps['layers']
timeout?: Duration
};

type FunctionPayload = {[key: string]: string | boolean | number};
Expand All @@ -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) {
Expand Down
20 changes: 18 additions & 2 deletions packages/parameters/src/appconfig/AppConfigProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
class AppConfigProvider extends BaseProvider {
public client: AppConfigDataClient;
protected configurationTokenStore: Map<string, string> = new Map();
protected valueStore: Map<string, Uint8Array> = new Map();
private application?: string;
private environment: string;

Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ export const handler = async (_event: unknown, _context: Context): Promise<void>
// 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();
Expand All @@ -94,7 +94,7 @@ export const handler = async (_event: unknown, _context: Context): Promise<void>
});
}

// 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();
Expand All @@ -111,4 +111,24 @@ export const handler = async (_event: unknown, _context: Context): Promise<void>
error: err.message
});
}

// Test 7
// get parameter twice, but wait long enough that cache expires count SDK calls and return values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to reword this to reflect that we are no longer using the delay but instead avoiding cache with maxAge: 0.

try {
providerWithMiddleware.clearCache();
middleware.counter = 0;
const expiredResult1 = await providerWithMiddleware.get(freeFormBase64encodedPlainText, { maxAge: 0, transform: 'base64' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const expiredResult1 = await providerWithMiddleware.get(freeFormBase64encodedPlainText, { maxAge: 0, transform: 'base64' });
const expiredResult1 = await providerWithMiddleware.get(
freeFormBase64encodedPlainText, {
maxAge: 0,
transform: 'base64'
}
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I've just put the line on multiple lines so that it's not too long

const expiredResult2 = await providerWithMiddleware.get(freeFormBase64encodedPlainText, { maxAge: 0, transform: 'base64' });
logger.log({
test: 'get-expired',
value: middleware.counter, // should be 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance that we can keep the original test/value structure? i.e.

logger.log({
  test: 'get-expired',
  value: {
    counter: middleware.counter,
    results: [
      expiredResult1,
      expiredResult2,
    ]
  },
});

or similar

result1: expiredResult1,
result2: expiredResult2
});
} catch (err) {
logger.log({
test: 'get-expired',
error: err.message
});
}
};
27 changes: 26 additions & 1 deletion packages/parameters/tests/e2e/appConfigProvider.class.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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, wait for expiration betweenn
* 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
Expand Down Expand Up @@ -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',
}
});
Expand Down Expand Up @@ -314,6 +321,24 @@ describe(`parameters E2E tests (appConfigProvider) for runtime ${runtime}`, () =

}, TEST_CASE_TIMEOUT);

// Test 7 - get parameter twice, wait for expiration betweenn
// 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: 2,
result1: result,
result2: result
});

}, TEST_CASE_TIMEOUT);

});

afterAll(async () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/parameters/tests/unit/AppConfigProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -225,6 +226,51 @@ 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 () => {

client.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only test using this reset. We should possibly add a afterEach() block to the whole test fixture that does it so that it gets applied after every test.

Maybe we can do this for this file, and then possibly in a future PR we can also apply the change to all other tests under packages/parameters/tests/unit/*.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that works for me. I added it here to be surgical - the resolvesOnce chain was acting weirdly without it. I like the consistency of using an afterEach instead.


// 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', () => {
Expand All @@ -243,3 +289,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();
});
});
});