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

andreialecu · 2024-03-06T16:26:45Z

Issue # (if applicable)

Reason for this change

There are cases when the ssm value should not be cached because it can accidentally roll back certain resources. See discussion in #12366

In my case, I'm using the AWS CLI to update a Cloudfront origin, but it's also part of a CDK stack. I want to avoid deploying the CDK stack and accidentally reverting the origin (which is also persisted to SSM, so the CDK stack can read it if needed).

Description of changes

I added an optional parameter to valueFromLookup(this, 'param', { disableContextCaching: true })

It works by setting an adjacent context key of key + TRANSIENT_CONTEXT_KEY, piggybacking on similar behavior in the CDK.

This seemed like the most straightforward implementation because value is usually a primitive value, and there was no way to attach this metadata.

I didn't want to grow the scope of the change, but ideally, value should've been an object so that { [TRANSIENT_CONTEXT_KEY]: true, value } could've been used directly. I wasn't sure of that behavior's implications and potential breaking changes.

Description of how you validated changes

Added tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

closes aws#12366

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

andreialecu · 2024-03-06T16:35:22Z

Exemption Request: I don't think an integration test is required here because there's no change to the synthesis. This is just a change relating to what gets saved in cdk.context.json

~~Also, not sure if a readme change is necessary?~~ (edit: updated the readme)

packages/aws-cdk-lib/core/lib/context-provider.ts

aaythapa

Thank you for the PR!

A couple of comments, mostly from my own lack of understanding for this part of the code base

aaythapa · 2024-03-15T20:12:23Z

packages/aws-cdk-lib/cloud-assembly-schema/schema/cloud-assembly.schema.json

+ },
+ "disableContextCaching": {
+ "description": "Whether to disable persisting this to the context file.",
+ "default": false,
+ "type": "boolean"


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

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.

aaythapa · 2024-03-15T20:21:27Z

packages/aws-cdk-lib/aws-ssm/test/parameter.test.ts

+ const config = await new Configuration({ readUserContext: false }).load();
+
+ // THEN
+ expect(value).toEqual('dummy-value-for-my-param-name');


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

See here:

aws-cdk/packages/aws-cdk-lib/aws-ssm/README.md

Lines 61 to 65 in ab94070

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()`

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

aaythapa · 2024-03-15T20:25:41Z

packages/aws-cdk-lib/aws-ssm/test/parameter.test.ts

+ 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 () => {


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

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.

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

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

aaythapa · 2024-03-15T20:27:14Z

packages/aws-cdk/lib/context-providers/index.ts

+ if (missingContext.disableContextCaching) {
+ debug(`Skipping context caching for ${key}`);
+ context.set(key + TRANSIENT_CONTEXT_KEY, { [TRANSIENT_CONTEXT_KEY]: true });


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

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.

aaythapa · 2024-03-15T20:28:20Z

packages/aws-cdk/test/context.test.ts

@@ -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 () => {


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

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.

aws-cdk-automation · 2024-03-15T22:15:13Z

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/29372/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

aws-cdk-automation · 2024-03-15T22:41:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: da5a439
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2024-03-16T08:45:17Z

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/29372/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

TheRealAmazonKendra · 2024-03-28T05:37:04Z

These two PRs (#29372) and (#28766) are not doing the same thing, but there is definitely overlap. Given that fact, it changes the feedback I was going to give to both a bit.

I absolutely see the value in both these changes but want to make sure we're getting the contract right across both. I know you've both been waiting for feedback for some time but I'm going to tax your patience a tad further so I can discuss with the team tomorrow.

aros3rg3d · 2024-04-12T08:43:59Z

Just came across this feature while searching how to disable caching for ssm lookup. Thanks @andreialecu, can't wait to use it !

feat(ssm): allow disabling context caching in valueFromLookup

babb36b

closes aws#12366

aws-cdk-automation requested a review from a team March 6, 2024 16:26

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Mar 6, 2024

andreialecu had a problem deploying to test-pipeline March 6, 2024 16:27 — with GitHub Actions Failure

aws-cdk-automation requested changes Mar 6, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Mar 6, 2024

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 6, 2024

fixup! feat(ssm): allow disabling context caching in valueFromLookup

ed439fc

andreialecu had a problem deploying to test-pipeline March 6, 2024 16:42 — with GitHub Actions Failure

fixup! feat(ssm): allow disabling context caching in valueFromLookup

45b644b

andreialecu had a problem deploying to test-pipeline March 6, 2024 18:02 — with GitHub Actions Failure

andreialecu mentioned this pull request Mar 6, 2024

(aws-ssm): valueFromLookup should have an option to skip context caching #12366

Open

2 tasks

docs: document disableContextCaching

f36f903

andreialecu had a problem deploying to test-pipeline March 6, 2024 18:32 — with GitHub Actions Failure

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 6, 2024

andreialecu commented Mar 7, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/context-provider.ts Outdated Show resolved Hide resolved

fix: update doc comment

f7a7ae6

andreialecu had a problem deploying to test-pipeline March 7, 2024 11:48 — with GitHub Actions Failure

andreialecu mentioned this pull request Mar 8, 2024

fix(cli): diff allow specifying changeSet via cdk.json #29375

Open

1 task

aaythapa reviewed Mar 15, 2024

View reviewed changes

text: verify that context value is used

196f8d9

andreialecu had a problem deploying to test-pipeline March 15, 2024 21:36 — with GitHub Actions Failure

Merge branch 'main' into ssm-skipcache

da5a439

andreialecu had a problem deploying to test-pipeline March 15, 2024 22:14 — with GitHub Actions Failure

TheRealAmazonKendra mentioned this pull request Mar 28, 2024

feat(core): support adding additional cache key to cdk context values #28766

Closed

github-actions bot mentioned this pull request Apr 1, 2024

Monthly PR metrics report #29668

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andreialecu commented Mar 6, 2024 •

edited

Loading

aws-cdk-automation left a comment

andreialecu commented Mar 6, 2024 •

edited

Loading

aaythapa left a comment

aaythapa Mar 15, 2024

andreialecu Mar 15, 2024

aaythapa Mar 15, 2024

andreialecu Mar 15, 2024

andreialecu Mar 15, 2024

aaythapa Mar 15, 2024

andreialecu Mar 15, 2024

andreialecu Mar 15, 2024

andreialecu Mar 16, 2024 •

edited

Loading

aaythapa Mar 15, 2024

andreialecu Mar 15, 2024

aaythapa Mar 15, 2024

andreialecu Mar 15, 2024

aws-cdk-automation commented Mar 15, 2024

aws-cdk-automation commented Mar 15, 2024

aws-cdk-automation commented Mar 16, 2024

TheRealAmazonKendra commented Mar 28, 2024

aros3rg3d commented Apr 12, 2024

	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()`

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

Are you sure you want to change the base?

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

Conversation

andreialecu commented Mar 6, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

andreialecu commented Mar 6, 2024 • edited Loading

aaythapa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreialecu Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 15, 2024

aws-cdk-automation commented Mar 15, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 16, 2024

TheRealAmazonKendra commented Mar 28, 2024

aros3rg3d commented Apr 12, 2024

andreialecu commented Mar 6, 2024 •

edited

Loading

andreialecu commented Mar 6, 2024 •

edited

Loading

andreialecu Mar 16, 2024 •

edited

Loading