fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles#20966
fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles#20966mergify[bot] merged 4 commits intoaws:mainfrom haslam22:main
Conversation
|
|
|
Thank you for this PR @haslam22 :) |
|
No problem @mattiamatrix - apologies for not spotting your original PR on this and thanks for resolving the duplication :) |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Just a few quick comments inline.
| * @see Policy.fromSdkCalls | ||
| */ | ||
| readonly policy: AwsCustomResourcePolicy; | ||
| readonly policy?: AwsCustomResourcePolicy; |
There was a problem hiding this comment.
I'd like to see an integration test for when policy is not provided and role is.
There was a problem hiding this comment.
Added an integration test which creates an AwsCustomResource with a role but no policy. Got this passing locally and updated the snapshot.
| } | ||
|
|
||
| if (!props.role && !props.policy) { | ||
| throw new Error('Either `policy` or `role` must be specified.'); |
There was a problem hiding this comment.
This makes it sound like they are mutually exclusive. Can you refine this message a little more?
There was a problem hiding this comment.
This was tricky to get correct, but I've modified the message slightly now to mention both can be used together.
| // If the policy was deleted first, then the function might lose permissions to delete the custom resource | ||
| // This is here so that the policy doesn't get removed before onDelete is called | ||
| this.customResource.node.addDependency(policy); | ||
| // Create the policy statements for the custom resource function role, or use the user-provided ones |
There was a problem hiding this comment.
Why move the location of this?
There was a problem hiding this comment.
The old code created the policy first, followed by the custom resource, and finally made the custom resource dependent on the policy. It felt a bit more natural to first create the custom resource, and then create the policy + dependency on the policy together in the same if(props.policy) { ... } block.
The alternative is to not move this around and wrap both the policy creation and dependency addition with if(props.policy), but it feels neater in one block.
|
Oh, actually one more note. On a fix, you want to describe the problem, not the solution you're implementing. Take a look at our changelog for examples. |
Pull request has been modified.
Thanks for the review @TheRealAmazonKendra. I've updated the PR title and addressed the other comments. |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Looks good. Thanks for your contribution!
|
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). |
This fixes #13232 by making the
policyproperty optional inAwsCustomResourceas long asroleis specified.Previously, if an immutable role was provided to
AwsCustomResource, an IAM policy was still created with a reference to the provided role. This resulted in a call toiam:PutRolePolicyto update the immutable role when deploying the stack.The underlying motivation for using an immutable role here is to support a restricted corporate environment where IAM role changes are not allowed.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license