Skip to content
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

feat(ssm): allow disabling context caching in valueFromLookup #29372

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/aws-cdk-lib/aws-ssm/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,20 @@ const arnLookup = ssm.StringParameter.valueFromLookup(this, '/my/role/arn');
iam.Role.fromRoleArn(this, 'role', Lazy.string({ produce: () => arnLookup }));
```


#### Enhanced Control over Context Caching

The `ssm.StringParameter.valueFromLookup` method in AWS CDK supports an optional `disableContextCaching` flag, allowing developers to control the caching of context values fetched from AWS SSM Parameter Store. This feature is aimed at providing flexibility for scenarios where parameters may frequently change or when ensuring the freshness of data is critical.

```ts
const parameterValue = ssm.StringParameter.valueFromLookup(this, 'parameterName', {
disableContextCaching: true,
});
```

Setting `disableContextCaching` to `true` bypasses the default caching mechanism, ensuring the parameter value is retrieved directly from the source on each synthesis.


## Creating new SSM Parameters in your CDK app

You can create either `ssm.StringParameter` or `ssm.StringListParameter`s in
Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk-lib/aws-ssm/lib/parameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as kms from '../../aws-kms';
import * as cxschema from '../../cloud-assembly-schema';
import {
CfnDynamicReference, CfnDynamicReferenceService, CfnParameter,
ContextProvider, Fn, IResource, Resource, Stack, Token,
ContextProvider, ContextProviderConfig, Fn, IResource, Resource, Stack, Token,
Tokenization,
} from '../../core';

Expand Down Expand Up @@ -532,11 +532,12 @@ export class StringParameter extends ParameterBase implements IStringParameter {
* Requires that the stack this scope is defined in will have explicit
* account/region information. Otherwise, it will fail during synthesis.
*/
public static valueFromLookup(scope: Construct, parameterName: string): string {
public static valueFromLookup(scope: Construct, parameterName: string, contextOpts?: ContextProviderConfig): string {
const value = ContextProvider.getValue(scope, {
provider: cxschema.ContextProvider.SSM_PARAMETER_PROVIDER,
props: { parameterName },
dummyValue: `dummy-value-for-${parameterName}`,
disableContextCaching: contextOpts?.disableContextCaching,
}).value;

return value;
Expand Down
98 changes: 98 additions & 0 deletions packages/aws-cdk-lib/aws-ssm/test/parameter.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/* eslint-disable max-len */

import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { Deployments } from '../../../aws-cdk/lib/api/deployments';
import { CdkToolkit } from '../../../aws-cdk/lib/cdk-toolkit';
import { Configuration } from '../../../aws-cdk/lib/settings';
import { MockCloudExecutable } from '../../../aws-cdk/test/util';
import { Template } from '../../assertions';
import * as iam from '../../aws-iam';
import * as kms from '../../aws-kms';
Expand Down Expand Up @@ -652,6 +656,100 @@ test('fromLookup will use the SSM context provider to read value during synthesi
]);
});

test('fromLookup will persist value in context by default', async () => {
// GIVEN
const app = new cdk.App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new cdk.Stack(app, 'my-staq', { env: { region: 'us-east-1', account: '12344' } });

// WHEN
const value = ssm.StringParameter.valueFromLookup(stack, 'my-param-name');

const synth = app.synth();

const mockCloudExecutable = new MockCloudExecutable({
stacks: synth.stacks,
missing: synth.manifest.missing,
});

let requestedParameterName: string | undefined;
mockCloudExecutable.sdkProvider.stubSSM({
getParameter(request) {
requestedParameterName = request.Name;
return {
Parameter: {
Value: '123',
},
};
},
});

const cdkToolkit = new CdkToolkit({
cloudExecutable: mockCloudExecutable,
configuration: mockCloudExecutable.configuration,
sdkProvider: mockCloudExecutable.sdkProvider,
deployments: new Deployments({ sdkProvider: mockCloudExecutable.sdkProvider }),
});

await mockCloudExecutable.configuration.load();

await cdkToolkit.assembly();

await mockCloudExecutable.configuration.saveContext();
const config = await new Configuration({ readUserContext: false }).load();

// THEN
expect(value).toEqual('dummy-value-for-my-param-name');
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused by why this is true, did you set value to dummy-value-for-my-param-name somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here:

When using `valueFromLookup` an initial value of 'dummy-value-for-${parameterName}'
(`dummy-value-for-/My/Public/Parameter` in the above example)
is returned prior to the lookup being performed. This can lead to errors if you are using this
value in places that require a certain format. For example if you have stored the ARN for a SNS
topic in a SSM Parameter which you want to lookup and provide to `Topic.fromTopicArn()`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, if you look at the test before this one, a similar assertion is made (line 641)

expect(requestedParameterName).toEqual('my-param-name');
expect(config.context.keys).toEqual(['ssm:account=12344:parameterName=my-param-name:region=us-east-1']);
});

test('fromLookup will not persist value in context if requested', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

With disableContextCaching would the value in cdk context get overwritten when the parameter is resolved or is it the value never gets saved in cdk context?

If its the former I'd prefer if this test showed that with disableContextCaching the context will have a new value everytime it resolves the parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's a previous value in the context, valueFromLookup() will use that one and not do another lookup.

This PR only allows disabling persisting to the context cache if the value needs to be looked up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new test to confirm what I mentioned in my comment above.

Copy link
Contributor Author

@andreialecu andreialecu Mar 16, 2024

Choose a reason for hiding this comment

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

I'm open to renaming this to something like skipContextPersistence or some other name that would be more descriptive.

// GIVEN
const app = new cdk.App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new cdk.Stack(app, 'my-staq', { env: { region: 'us-east-1', account: '12344' } });

// WHEN
const value = ssm.StringParameter.valueFromLookup(stack, 'my-param-name', { disableContextCaching: true });

const synth = app.synth();

const mockCloudExecutable = new MockCloudExecutable({
stacks: synth.stacks,
missing: synth.manifest.missing,
});

let requestedParameterName: string | undefined;
mockCloudExecutable.sdkProvider.stubSSM({
getParameter(request) {
requestedParameterName = request.Name;
return {
Parameter: {
Value: '123',
},
};
},
});

const cdkToolkit = new CdkToolkit({
cloudExecutable: mockCloudExecutable,
configuration: mockCloudExecutable.configuration,
sdkProvider: mockCloudExecutable.sdkProvider,
deployments: new Deployments({ sdkProvider: mockCloudExecutable.sdkProvider }),
});

await mockCloudExecutable.configuration.load();

await cdkToolkit.assembly();

await mockCloudExecutable.configuration.saveContext();
const config = await new Configuration({ readUserContext: false }).load();

// THEN
expect(value).toEqual('dummy-value-for-my-param-name');
expect(requestedParameterName).toEqual('my-param-name');
expect(config.context.keys).toEqual([]);
});

describe('from string list parameter', () => {
testDeprecated('valueForTypedStringParameter list type throws error', () => {
// GIVEN
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ export interface MissingContext {
* A set of provider-specific options.
*/
readonly props: ContextQueryProperties;

/**
* Whether to disable persisting this to the context file.
*
* @default false
*/
readonly disableContextCaching?: boolean;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,11 @@
"props": {
"$ref": "#/definitions/ContextQueryProperties",
"description": "A set of provider-specific options."
},
"disableContextCaching": {
"description": "Whether to disable persisting this to the context file.",
"default": false,
"type": "boolean"
Comment on lines +485 to +489
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about the schema changes, did you add these manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not manual, no. If I remember correctly this was the result of running yarn update-schema which I think was required for getting tests to pass.

}
},
"required": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"36.0.0"}
{"version":"37.0.0"}
15 changes: 14 additions & 1 deletion packages/aws-cdk-lib/core/lib/context-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,20 @@ export interface GetContextKeyOptions {
}

/**
* Configuration options for context providers.
*/
export interface GetContextValueOptions extends GetContextKeyOptions {
export interface ContextProviderConfig {
/**
* Whether the context provider is allowed to cache the value.
andreialecu marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false
*/
readonly disableContextCaching?: boolean;
}

/**
*/
export interface GetContextValueOptions extends GetContextKeyOptions, ContextProviderConfig {
/**
* The value to return if the context value was not found and a missing
* context is reported. This should be a dummy value that should preferably
Expand Down Expand Up @@ -105,6 +117,7 @@ export class ContextProvider {
key,
provider: options.provider as cxschema.ContextProvider,
props: props as cxschema.ContextQueryProperties,
disableContextCaching: options.disableContextCaching,
});

if (providerError !== undefined) {
Expand Down
5 changes: 5 additions & 0 deletions packages/aws-cdk/lib/context-providers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ export async function provideContextValues(
}, resolvedEnvironment, sdk);

value = await provider.getValue({ ...missingContext.props, lookupRoleArn: arns.lookupRoleArn });

if (missingContext.disableContextCaching) {
debug(`Skipping context caching for ${key}`);
context.set(key + TRANSIENT_CONTEXT_KEY, { [TRANSIENT_CONTEXT_KEY]: true });
Comment on lines +73 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this property work for any context lookup? Like it was suggested in this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be able to be used it in other context lookups, but this PR only implements it for SSM. I'm not sure what other context lookups would benefit from this, but with the underlaying changes in this PR, it would be easy to make it work elsewhere.

}
} catch (e: any) {
// Set a specially formatted provider value which will be interpreted
// as a lookup failure in the toolkit.
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ function expandHomeDir(x: string) {
function stripTransientValues(obj: {[key: string]: any}) {
const ret: any = {};
for (const [key, value] of Object.entries(obj)) {
if (!isTransientValue(value)) {
if (!isTransientValue(value) && !isTransientValue(obj[key + TRANSIENT_CONTEXT_KEY])) {
ret[key] = value;
}
}
Expand Down
15 changes: 15 additions & 0 deletions packages/aws-cdk/test/context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,18 @@ test('transient values arent saved to disk', async () => {
// THEN
expect(config2.context.get('some_key')).toEqual(undefined);
});

test('transient keys arent saved to disk', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the purpose of this test since it doesn't use disableContextCaching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of the test above it ("transient values arent saved to disk") but it checks that the specified keys/values don't get saved to the context, regardless of what the value is.

// GIVEN
const config1 = await new Configuration({ readUserContext: false }).load();
config1.context.set('some_key', 'some_value');
config1.context.set('some_key' + TRANSIENT_CONTEXT_KEY, { [TRANSIENT_CONTEXT_KEY]: true });
await config1.saveContext();
expect(config1.context.get('some_key')).toEqual('some_value');

// WHEN
const config2 = await new Configuration({ readUserContext: false }).load();

// THEN
expect(config2.context.get('some_key')).toEqual(undefined);
});
Loading