-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(core): support adding additional cache key to cdk context values #26984
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Some open questions/areas I am particularly looking for feedback on (especially being less familiar with standards as a new contributor):
|
One more potential question: should |
d6f96bd
to
edbccad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the idea and the contribution!
I have to say that I'm reflexively experiencing an aversion to this proposed change 🤢😝 because why would those values ever be different? But I can also appreciate that this is necessary to evolve a stateful app over time.
The solution of including the scope is clever... but I wonder if it doesn't give more flexibility at the cost of a vague contract.
Could we do something blindingly obvious instead, have the user include a cache busting key? And then everyone can reuse/invalidate that key on a per-location basis as they desire?
ec2.MachineImage.latestAmazonLinux2023({
cachedInContext: true,
additionalCacheKey: 'asdf', // Can change this to qwerty whenever I feel like updating it
});
They can still make that be the scope if they want to (achieving the current behavior) by passing this.node.path
.
Hey @rix0rrr! I do think that makes sense, and now that you say that I realize that did feel a bit weird. I imagine that cache key would be used as the value of a prop passed to the context, I'm thinking maybe call they key of that prop |
Pull request has been modified.
I also wound up moving additionalCacheKey into the lookup props - it seems like it wound up being cleaner. I'll adjust the PR title and description to match the change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks (and sorry for the late review) 👍
I left you some comments as a first revision.
aws_ec2 as ec2, | ||
// aws_autoscaling as asg, | ||
// aws_ssm as ssm, | ||
} from 'aws-cdk-lib'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws_ec2 as ec2, | |
// aws_autoscaling as asg, | |
// aws_ssm as ssm, | |
} from 'aws-cdk-lib'; | |
} from 'aws-cdk-lib'; | |
import * as ec2 from 'aws-cdk-lib/aws-ec2'; |
const app = new App(); | ||
new IntegTest(app, 'integ-test', { | ||
testCases: [new TestCase(app, 'integ-ec2-machine-image-cached-test', { env })], | ||
enableLookups: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const app = new App(); | |
new IntegTest(app, 'integ-test', { | |
testCases: [new TestCase(app, 'integ-ec2-machine-image-cached-test', { env })], | |
enableLookups: true, | |
}); | |
const app = new App(); | |
const stack = new TestCase(app, 'cdk-ec2-machine-image-cached', { env }); | |
new IntegTest(app, 'integ-ec2-machine-image-cached', { | |
testCases: [stack], | |
enableLookups: true, | |
}); | |
app.synth(); |
constructor(private readonly props: AmazonLinuxImageProps = {}) { | ||
super(AmazonLinuxImage.ssmParameterName(props), OperatingSystemType.LINUX, props.userData); | ||
|
||
this.cachedInContext = props.cachedInContext ?? false; | ||
if (props.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (props.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; | |
this.additionalCacheKey = props.additionalCacheKey; |
@@ -145,13 +152,14 @@ export class EcsOptimizedAmi implements ec2.IMachineImage { | |||
+ (this.windowsVersion ? 'image_id' : 'recommended/image_id'); | |||
|
|||
this.cachedInContext = props?.cachedInContext ?? false; | |||
if (props?.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (props?.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; | |
this.additionalCacheKey = props?.additionalCacheKey; |
@@ -385,8 +410,8 @@ Object.defineProperty(BottleRocketImage.prototype, BR_IMAGE_SYMBOL, { | |||
writable: false, | |||
}); | |||
|
|||
function lookupImage(scope: Construct, cachedInContext: boolean | undefined, parameterName: string) { | |||
function lookupImage(scope: Construct, cachedInContext: boolean, parameterName: string, additionalCacheKey?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function lookupImage(scope: Construct, cachedInContext: boolean, parameterName: string, additionalCacheKey?: string) { | |
function lookupImage(scope: Construct, cachedInContext?: boolean, parameterName: string, additionalCacheKey?: string) { |
Why change cachedInContext
's type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a result of an intermediate change. (Also has to be | undefined
since later properties cant be required if an earlier one is not, but that's no matter)
Out of curiosity: This appears to be an "internal" function that is never called with undefined - does this still have a backward-compat issue just by virtue of it being exported?
@@ -1292,6 +1292,7 @@ export class Vpc extends VpcBase { | |||
returnAsymmetricSubnets: true, | |||
returnVpnGateways: options.returnVpnGateways, | |||
subnetGroupNameTag: options.subnetGroupNameTag, | |||
...(options.additionalCacheKey ? { additionalCacheKey: options.additionalCacheKey } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...(options.additionalCacheKey ? { additionalCacheKey: options.additionalCacheKey } : {}), | |
additionalCacheKey: options.additionalCacheKey, |
The parameter should be added in the corresponding interface (same for other getValue
calls).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that undefined values are stripped in context-provider. props
is just { [key: string]: any }
, so I think there's nothing to do beyond that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...never mind, I see the interface this is being specified as.
@@ -258,14 +273,15 @@ export class EcsOptimizedImage implements ec2.IMachineImage { | |||
+ (this.hwType === AmiHardwareType.ARM ? 'arm64/' : '') | |||
+ (this.windowsVersion ? 'image_id' : 'recommended/image_id'); | |||
|
|||
this.cachedInContext = props?.cachedInContext ?? false; | |||
this.cachedInContext = props.cachedInContext ?? false; | |||
if (props.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (props.additionalCacheKey) this.additionalCacheKey = props.additionalCacheKey; | |
this.additionalCacheKey = props.additionalCacheKey; |
If you do not want the value of the context variable to be global (ie, having the same | ||
value returned any time you call `fromLookup` across your entire app), | ||
you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | ||
to the scope of the current construct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not want the value of the context variable to be global (ie, having the same | |
value returned any time you call `fromLookup` across your entire app), | |
you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | |
to the scope of the current construct) | |
To customize the cache key use the `additionalCacheKey` parameter. | |
This can be useful if you want to scope the context variable to a construct | |
(eg, using `additionalCacheKey: this.node.path`). |
Feel free to improve (will need to be copied on other READMEs).
const stack = new cdk.Stack(app, 'TestStack', { | ||
env: { account: '12345', region: 'us-east-1' }, | ||
}); | ||
const filter = { domainName: 'test.com' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const filter = { domainName: 'test.com' }; | |
const filter = { | |
domainName: 'test.com', | |
additionalCacheKey: 'cacheKey', | |
}; |
Missing parameter.
@@ -81,5 +81,43 @@ describe('hosted zone provider', () => { | |||
expect(fakeZoneId).toEqual(zoneId); | |||
|
|||
}); | |||
|
|||
test('HostedZoneProvider will return context values if available', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('HostedZoneProvider will return context values if available', () => { | |
test('HostedZoneProvider will return context values if available with additional cache key', () => { |
@lpizzinidev Thanks for the suggestions - some good callouts there. Changes made, though left a couple questions about potential further adjustments. Also given the cloud-assembly-schema is now conflicting, let me know if I should merge or rebase to latest main and regen that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes.
I left some notes on possible documentation improvements.
Also given the cloud-assembly-schema is now conflicting, let me know if I should merge or rebase to latest main and regen that
You should merge main
into the PR branch.
> If you do not want the value of the context variable to be to be global | ||
> (ie, having the same value returned any time you call `lookup` across your entire app), | ||
> you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | ||
> to the scope of the current construct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> If you do not want the value of the context variable to be to be global | |
> (ie, having the same value returned any time you call `lookup` across your entire app), | |
> you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | |
> to the scope of the current construct) | |
> To customize the cache key use the `additionalCacheKey` parameter. | |
> This can be useful if you want to scope the context variable to a construct | |
> (eg, using `additionalCacheKey: this.node.path`). |
If you do not want the value of the context variable to be global (ie, having the same | ||
value returned any time you call `EcsOptimizedImage.amazonLinux` across your entire app), | ||
you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | ||
to the scope of the current construct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not want the value of the context variable to be global (ie, having the same | |
value returned any time you call `EcsOptimizedImage.amazonLinux` across your entire app), | |
you can set the `additionalCacheKey` argument (eg, to `this.node.path` to tie it | |
to the scope of the current construct) | |
To customize the cache key use the `additionalCacheKey` parameter. | |
This can be useful if you want to scope the context variable to a construct | |
(eg, using `additionalCacheKey: this.node.path`). |
* If `props.additionalCacheKey` is set, the context key uses that value as a discriminator | ||
* rather than the cached value being global across all lookups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If `props.additionalCacheKey` is set, the context key uses that value as a discriminator | |
* rather than the cached value being global across all lookups | |
* | |
* If `props.additionalCacheKey` is set, the context key uses that value as a discriminator | |
* rather than the cached value being global across all lookups. |
@@ -744,7 +747,7 @@ If the security group ID is known and configuration details are unknown, use met | |||
const sg = ec2.SecurityGroup.fromLookupById(this, 'SecurityGroupLookup', 'sg-1234'); | |||
``` | |||
|
|||
The result of `SecurityGroup.fromLookupByName` and `SecurityGroup.fromLookupById` operations will be written to a file called `cdk.context.json`. You must commit this file to source control so that the lookup values are available in non-privileged environments such as CI build steps, and to ensure your template builds are repeatable. | |||
The result of `SecurityGroup.fromLookupByName` and `SecurityGroup.fromLookupById` operations will be written to a file called `cdk.context.json`. You must commit this file to source control so that the lookup values are available in non-privileged environments such as CI build steps, and to ensure your template builds are repeatable. To customize the cache key use the `additionalCacheKey` parameter. This can be useful if you want to scope the context variable to a construct (eg, using `additionalCacheKey: this.node.path`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of `SecurityGroup.fromLookupByName` and `SecurityGroup.fromLookupById` operations will be written to a file called `cdk.context.json`. You must commit this file to source control so that the lookup values are available in non-privileged environments such as CI build steps, and to ensure your template builds are repeatable. To customize the cache key use the `additionalCacheKey` parameter. This can be useful if you want to scope the context variable to a construct (eg, using `additionalCacheKey: this.node.path`). | |
The result of `SecurityGroup.fromLookupByName` and `SecurityGroup.fromLookupById` operations will be | |
written to a file called `cdk.context.json`. | |
You must commit this file to source control so that the lookup values are available in non-privileged | |
environments such as CI build steps, and to ensure your template builds are repeatable. | |
To customize the cache key use the `additionalCacheKey` parameter. | |
This can be useful if you want to scope the context variable to a construct (eg, using `additionalCacheKey: this.node.path`). | |
```ts | |
const sg = ec2.SecurityGroup.fromLookupById(this, 'SecurityGroupLookup', 'sg-1234', this.node.path); |
Formatting and example.
@@ -673,6 +690,7 @@ export class LookupMachineImage implements IMachineImage { | |||
props: { | |||
owners: this.props.owners, | |||
filters, | |||
...(this.props.additionalCacheKey ? { additionalCacheKey: this.props.additionalCacheKey } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...(this.props.additionalCacheKey ? { additionalCacheKey: this.props.additionalCacheKey } : {}), | |
additionalCacheKey: this.props.additionalCacheKey, |
AmiContextQuery
will need a new property.
* When the image is cached in cdk.context.json, adds an additional discriminator to the | ||
* cache key so that separate lookups with the same parameters can have separate cache lifecycles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* When the image is cached in cdk.context.json, adds an additional discriminator to the | |
* cache key so that separate lookups with the same parameters can have separate cache lifecycles | |
* Adds an additional discriminator to the `cdk.context.json` cache key. | |
* | |
* @default - no additional cache key |
Other variable comments in this PR will need an update as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity I'm thinking "When cachedInContext is true, adds an additional discriminator..." in situations where caching does not always happen (since this variable does nothing on its own)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, unfortunately cachedInContext does not always appear on the lookup parameters object. Probably better not to do it for the sake of consistency then. If anything it would be "adds an additional discriminator... if cached", but not sure how helpful that really is at that point
@@ -519,6 +519,14 @@ following lookup methods: | |||
- `NetworkListener.fromLookup(options)` - Look up a network load balancer | |||
listener. | |||
|
|||
The result of a `fromLookup()` operation will be written to a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added at the end of the Load Balancer lookup options
section with an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general description is applicable to both load balancers and load balancer listeners so I think that should be in the top-level section (and I'll fix the heading level for LBL lookup options while I'm at it). Will add the examples
To customize the cache key use the `additionalCacheKey` parameter. | ||
This can be useful if you want to scope the context variable to a construct | ||
(eg, using `additionalCacheKey: this.node.path`). | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To customize the cache key use the `additionalCacheKey` parameter. | |
This can be useful if you want to scope the context variable to a construct | |
(eg, using `additionalCacheKey: this.node.path`). | |
To customize the cache key use the `additionalCacheKey` parameter. | |
This can be useful if you want to scope the context variable to a construct | |
(eg, using `additionalCacheKey: this.node.path`). | |
```ts | |
const stringValue = ssm.StringParameter.valueFromLookup(this, '/My/Public/Parameter', this.node.path); |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! You did a good job updating the docs explaining WHAT people can now do, but what's missing is WHY they would want to do this.
Add that and it's golden.
@@ -156,6 +156,19 @@ const autoScalingGroup = new autoscaling.AutoScalingGroup(this, 'ASG', { | |||
}); | |||
``` | |||
|
|||
To customize the cache key use the `additionalCacheKey` parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to explain WHY a user might want to do this. If I didn't know what this was already, reading this bit of the documentation certainly wouldn't help me.
```ts | ||
const loadBalancer = elbv2.ApplicationLoadBalancer.fromLookup(this, 'ALB', { | ||
loadBalancerArn: 'arn:aws:elasticloadbalancing:us-east-2:123456789012:loadbalancer/app/my-load-balancer/1234567890123456', | ||
additionalCacheKey: this.node.path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This will ensure that ...(what)...?
* | ||
* @default - no additional cache key | ||
*/ | ||
readonly additionalCacheKey?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the context provider sends out the query, it also picks the context cache key. We can just mix the additional string into the context key; it never needs to make it to the cloud assembly as a separate field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the reason I did this was so that when the queries are constructed like so:
const attributes: cxapi.KeyContextResponse = ContextProvider.getValue(scope, {
provider: cxschema.ContextProvider.KEY_PROVIDER,
props: {
aliasName: options.aliasName,
additionalCacheKey: options.additionalCacheKey,
} as cxschema.KeyContextQuery,
...
});
it would be surfaced as a valid member of the object passed to props. You're saying this is not necessary?
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. |
@rix0rrr Could you provide clarification on the comment above so that I can finish this up? Thanks! |
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. |
|
The
ContextProvider
mechanism and various "lookup" functions of a number of constructs support caching resolved values in the cdk.context.json. The context keys are constructed from the parameters of the lookup, which for lookup functions means whenever a resource with the same parameters is resolved, it is resolved as the same value across the entire app. However when a value may change over time, the user may wish to use the latest value when creating creating a new reference to the construct, effectively tying the cached context value to the scope - this patch enables this.The primary use case is looking up an AMI parameter for a "stateful" EC2 instance. Currently if you specify
cachedInContext
, any future images created would use the same cached AMI, and updating the value would require updating all usages of the image across the entire app.Closes #26982
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license