-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(cognito): changing installLatestAwsSdk
breaks Client Secret reference
#23798
Conversation
…erence Because there wasn't previously a handler for `onUpdate` events, an empty object would be returned. When `installLatestAwsSdk` was changed to `false`, this was an update. Typically, updates aren't an issue because basically any other property being updated signifies a replacement. `installLatestAwsSdk` is just a very unique case where it doesn't (and where a user usually can't update it). When the empty object is returned, this results in an update failure in CloudFormation because the specific property isn't 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.
Thanks for the fix! I think this looks good but I just have one question inline.
@@ -461,6 +461,16 @@ export class UserPoolClient extends Resource implements IUserPoolClient { | |||
}, | |||
physicalResourceId: PhysicalResourceId.of(this.userPoolClientId), | |||
}, | |||
onUpdate: { | |||
region: Stack.of(this).region, |
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.
Is there any circumstance where the custom resources wouldn't be in the same region? Probably a dumb question on my end, but I just want to check.
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.
@TheRealAmazonKendra That's a great question and honestly I am not entirely sure... So the User Pool and the Client always have to be in the same region. There also isn't (currently) a fromXXX
static method on UserPoolClient
that lets you specify region (there's fromUserPoolClientId
but not fromUserPoolClientArn
). And while User Pool has a fromUserPoolArn
and IUserPool
has addClient
, I actually feel like that would fail in CloudFormation since the pool is in another region.
And then fromUserPoolClientId
actually has an implementation of get userPoolClientSecret()
that always throws (probably because it doesn't take an IUserPool
as a parameter to make the API call).
I am not sure that there's any situation where the region
parameter's value could be different from the region where the pool/client exist (and have things actually work with the interfaces that exist today).
So I guess region
could actually probably be omitted? But frankly, I just copied what had been done for onCreate
.
Which, I think, means the options are to either:
- Keep
region
as-is - Remove
region
fromonCreate
andonUpdate
- Change both to
this.userPool.env.region
(which at the moment should always be the stack's region)
Let me know which you'd prefer in this PR and if you'd like a second PR to make another change!
Probably shouldn't have actually made that changes requested. Can you tag me when you respond so I see it even if it doesn't require another code change? |
Per the documentation, `onUpdate` is used when `onCreate` is not defined. Since they're the same, we can just define `onUpdate`. https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources.AwsCustomResource.html#oncreate
Pull request has been modified.
So I actually did just push a change... I forgot that when |
Hi @TheRealAmazonKendra! I just wanted to follow up again (in case the previous mention as a review comment didn't work) to see if there was anything further to work on here? I am a little worried that other resources may need to have a similar fix applied (#23796 (comment)) but I don't know those other services well enough to start poking at them. Other users seem to be experiencing the same behavior which is causing issues when adopting version v2.60.0 or newer. |
onCreate: { | ||
onUpdate: { |
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.
Should onCreate
be completely removed?
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.
Yep! It seems to be relatively unknown that onCreate
defaults to the value of on onUpdate
when not specified (see @default
for https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources.AwsCustomResource.html#oncreate).
Please check me on this and confirm that the changes to integration test snapshots add Update
but do not remove Create
?
aws-cdk/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts
Lines 435 to 446 in 4c62f0b
const create = props.onCreate || props.onUpdate; | |
this.customResource = new cdk.CustomResource(this, 'Resource', { | |
resourceType: props.resourceType || 'Custom::AWS', | |
serviceToken: provider.functionArn, | |
pascalCaseProperties: true, | |
properties: { | |
create: create && this.encodeJson(create), | |
update: props.onUpdate && this.encodeJson(props.onUpdate), | |
delete: props.onDelete && this.encodeJson(props.onDelete), | |
installLatestAwsSdk, | |
}, | |
}); |
I think this is actually a common bug (specifying onCreate
but not onUpdate
for AwsCustomResource
), even within @aws-cdk/*
.
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 did not know that, thanks for the clarification!
Yep, OnCreate
is still in the snapshots
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
It looks like mergify is upset because the auto-update will result in changing something in |
Pull request has been modified.
Mergify's gonna mergify... or not. Re-approving this. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…y Group reference (#29620) ### Issue # (if applicable) Closes #23796 ### Reason for this change In #23591 `installLatestAwsSdk`. This results in a resource update for custom resources. The custom resource that fetches the security groups does not have an onUpdate handler (https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-globalaccelerator/lib/_accelerator-security-group.ts#L32). When the empty object is returned, this results in an update failure in CloudFormation because the specific property isn't available and so it will fail with error below: ``` CustomResource attribute error: Vendor response doesn't contain SecurityGroups.0.GroupId key in object ``` When the update occurs, the response object does not have a `SecurityGroups.0.GroupId` field, resulting in failures when `SecurityGroups` is referenced. ### Description of changes Update the onCreate to onUpdate for custom resources to mitigate the CloudFormation update failure. Documentations: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources.AwsCustomResource.html#oncreate. Similar fix for Cognito: #23798 ### Description of how you validated changes The integration test is updated with the latest assets. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Because there wasn't previously a handler for
onUpdate
events, anempty object would be returned. When
installLatestAwsSdk
was changedto
false
, this was an update. Typically, updates aren't an issuebecause basically any other property being updated signifies a
replacement.
installLatestAwsSdk
is just a very unique case where itdoesn't (and where a user usually can't update it).
When the empty object is returned, this results in an update failure in
CloudFormation because the specific property isn't available.
Fixes: #23796
All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license