feat(ssm): allow disabling context caching in valueFromLookup by andreialecu · Pull Request #29372 · aws/aws-cdk

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 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.

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.

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

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

  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: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

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 !

sakurai-ryo · 2024-10-11T02:31:23Z

@andreialecu @aaythapa @TheRealAmazonKendra
I’m really interested in using this feature.
Could I ask if there are any updates or blockers on this PR?
I would be happy to help and potentially take over the work to get it across the finish line.

GavinZZ · 2025-01-22T19:31:57Z

@andreialecu I apologize that this PR was just sitting here since Kendra reviewed and we did not make any new progress. Are you still interested in contributing this feature? If yes, please address the conflicts and ping me for another review. I'll make sure to prioritize reviewing this PR once the conflicts are addressed.

andreialecu · 2025-01-22T20:58:29Z

@andreialecu I apologize that this PR was just sitting here since Kendra reviewed and we did not make any new progress. Are you still interested in contributing this feature? If yes, please address the conflicts and ping me for another review. I'll make sure to prioritize reviewing this PR once the conflicts are addressed.

The changes merged while this PR was overlooked have introduced some awkwardness in usage. Someone added functionality to return a default value, which could have been integrated into the options object I proposed in this PR under a defaultValue field.

I must note my disappointment with the turnaround on this.

I'll try to fix the conflicts.

andreialecu · 2025-01-22T21:58:55Z

@GavinZZ this PR introduced changes to the MissingContext type which now appears to have been moved elsewhere:
https://github.com/cdklabs/cloud-assembly-schema/blob/main/lib/cloud-assembly/schema.ts

I'm not sure how to proceed, should I open a PR there first and wait for that to be merged before doing this?

It won't work without the change.

aws-cdk-automation · 2025-01-23T00:10:12Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

GavinZZ · 2025-01-23T21:56:17Z

@andreialecu Thanks for getting back to me. I had an internal discussion with my colleague and we have some questions on why the disable context caching is needed given that CloudFormation supports dynamic reference. My colleague will post more detail on this discussion shortly in this issue #12366 and let's continue the discussion from there.

aws-cdk-automation · 2025-01-30T00:10:20Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2025-01-30T00:10:38Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

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

Closed

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

Closed

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

GavinZZ added pr-linter/exempt-test The PR linter will not require test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Jan 22, 2025

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 22, 2025

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jan 30, 2025

aws-cdk-automation closed this Jan 30, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2025

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

Conversation

andreialecu commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreialecu commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aaythapa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreialecu Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Mar 15, 2024

AWS CodeBuild CI Report

Uh oh!

TheRealAmazonKendra commented Mar 28, 2024

Uh oh!

aros3rg3d commented Apr 12, 2024

Uh oh!

sakurai-ryo commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GavinZZ commented Jan 22, 2025

Uh oh!

andreialecu commented Jan 22, 2025

Uh oh!

andreialecu commented Jan 22, 2025

Uh oh!

aws-cdk-automation commented Jan 23, 2025

Uh oh!

GavinZZ commented Jan 23, 2025

Uh oh!

aws-cdk-automation commented Jan 30, 2025

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

andreialecu commented Mar 6, 2024 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

andreialecu commented Mar 6, 2024 •

edited

Loading

andreialecu Mar 16, 2024 •

edited

Loading

sakurai-ryo commented Oct 11, 2024 •

edited

Loading