fix(cognito): changing `installLatestAwsSdk` breaks Client Secret reference by laurelmay · Pull Request #23798 · aws/aws-cdk

laurelmay · 2023-01-23T22:11:47Z

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.

Fixes: #23796

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use 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

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

gitpod-io · 2023-01-23T22:11:58Z

TheRealAmazonKendra

Thanks for the fix! I think this looks good but I just have one question inline.

TheRealAmazonKendra · 2023-01-24T04:40:56Z

packages/@aws-cdk/aws-cognito/lib/user-pool-client.ts

            physicalResourceId: PhysicalResourceId.of(this.userPoolClientId),
          },
+          onUpdate: {
+            region: Stack.of(this).region,


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.

@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 from onCreate and onUpdate

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!

TheRealAmazonKendra · 2023-01-24T04:42:46Z

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.

laurelmay · 2023-01-24T14:55:35Z

So I actually did just push a change... I forgot that when onCreate is not defined, onUpdate is used instead. So I think we can just replace onCreate with onUpdate. The region question is still a good one though.

laurelmay · 2023-02-09T17:04:47Z

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.

comcalvi · 2023-02-13T23:41:48Z

packages/@aws-cdk/aws-cognito/lib/user-pool-client.ts

        {
          resourceType: 'Custom::DescribeCognitoUserPoolClient',
-          onCreate: {
+          onUpdate: {


Should onCreate be completely removed?

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

I did not know that, thanks for the clarification!

Yep, OnCreate is still in the snapshots

mergify · 2023-02-14T19:31:29Z

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

laurelmay · 2023-02-15T18:50:02Z

It looks like mergify is upset because the auto-update will result in changing something in .github/workflows/. I believe that will require maintainer action anyway, so I am going to go ahead and merge in main (which I think will end up dismissing the review...).

Pull request has been modified.

TheRealAmazonKendra · 2023-02-16T17:08:35Z

Mergify's gonna mergify... or not. Re-approving this.

mergify · 2023-02-16T17:09:20Z

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-cdk-automation · 2023-02-16T17:49:38Z

AWS CodeBuild CI Report

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

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

mergify · 2023-02-16T17:49:54Z

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*

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jan 23, 2023

aws-cdk-automation requested a review from a team January 23, 2023 22:12

github-actions bot added the p2 label Jan 23, 2023

TheRealAmazonKendra previously requested changes Jan 24, 2023

View reviewed changes

Replace onCreate with onUpdate

ce0ddfb

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

comcalvi reviewed Feb 13, 2023

View reviewed changes

comcalvi previously approved these changes Feb 14, 2023

View reviewed changes

Merge branch 'main' into cognito-update-error

d5ff298

TheRealAmazonKendra approved these changes Feb 16, 2023

View reviewed changes

Merge branch 'main' into cognito-update-error

2a98289

TheRealAmazonKendra approved these changes Feb 16, 2023

View reviewed changes

mergify bot merged commit 844d407 into aws:main Feb 16, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

jingwy mentioned this pull request Mar 26, 2024

fix(globalaccelerator): changing installLatestAwsSdk breaks Security Group reference #29620

Merged

1 task

	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,
	},
	});

Conversation

laurelmay commented Jan 23, 2023

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jan 23, 2023

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

laurelmay Jan 24, 2023 • 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!

TheRealAmazonKendra commented Jan 24, 2023

Uh oh!

laurelmay commented Jan 24, 2023

Uh oh!

laurelmay commented Feb 9, 2023

Uh oh!

comcalvi Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

laurelmay Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

comcalvi Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 14, 2023

Uh oh!

laurelmay commented Feb 15, 2023

Uh oh!

TheRealAmazonKendra commented Feb 16, 2023

Uh oh!

mergify bot commented Feb 16, 2023

Uh oh!

aws-cdk-automation commented Feb 16, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laurelmay Jan 24, 2023 •

edited

Loading