fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles #20966

haslam22 · 2022-07-02T17:12:15Z

This fixes #13232 by making the policy property optional in AwsCustomResource as long as role is 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 to iam:PutRolePolicy to 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:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional 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

gitpod-io · 2022-07-02T17:12:22Z

mattiamatrix · 2022-07-14T07:37:31Z

Thank you for this PR @haslam22 :)

haslam22 · 2022-07-14T11:00:32Z

No problem @mattiamatrix - apologies for not spotting your original PR on this and thanks for resolving the duplication :)

TheRealAmazonKendra

Thank you for your contribution! Just a few quick comments inline.

TheRealAmazonKendra · 2022-08-02T06:12:49Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

   * @see Policy.fromStatements
   * @see Policy.fromSdkCalls
   */
-  readonly policy: AwsCustomResourcePolicy;
+  readonly policy?: AwsCustomResourcePolicy;


I'd like to see an integration test for when policy is not provided and role is.

Added an integration test which creates an AwsCustomResource with a role but no policy. Got this passing locally and updated the snapshot.

TheRealAmazonKendra · 2022-08-02T06:18:48Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

@@ -344,6 +350,10 @@ export class AwsCustomResource extends Construct implements iam.IGrantable {
      throw new Error('At least `onCreate`, `onUpdate` or `onDelete` must be specified.');
    }

+    if (!props.role && !props.policy) {
+      throw new Error('Either `policy` or `role` must be specified.');


This makes it sound like they are mutually exclusive. Can you refine this message a little more?

This was tricky to get correct, but I've modified the message slightly now to mention both can be used together.

TheRealAmazonKendra · 2022-08-02T06:20:23Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

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


Why move the location of this?

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.

TheRealAmazonKendra · 2022-08-02T06:22:00Z

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.

haslam22 · 2022-08-04T23:19:25Z

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.

Thanks for the review @TheRealAmazonKendra. I've updated the PR title and addressed the other comments.

TheRealAmazonKendra

Looks good. Thanks for your contribution!

mergify · 2022-08-05T17:30:00Z

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 · 2022-08-05T18:06:08Z

AWS CodeBuild CI Report

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

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

mergify · 2022-08-05T18:06:16Z

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

…tes immutable roles (aws#20966) This fixes aws#13232 by making the `policy` property optional in `AwsCustomResource` as long as `role` is 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 to `iam:PutRolePolicy` to 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: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 2, 2022

aws-cdk-automation requested a review from a team July 2, 2022 17:12

mattiamatrix mentioned this pull request Jul 14, 2022

feat(custom-resources): allow for no policy to be specified #19997

Closed

4 tasks

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

haslam22 added 3 commits August 4, 2022 22:51

fix(custom-resources): make policy optional if role specified

459bc3f

add integration test for role without policy

f302317

refine error message

67d00ce

haslam22 changed the title ~~fix(custom-resources): make policy optional if role specified in AwsCustomResource~~ fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles Aug 4, 2022

TheRealAmazonKendra approved these changes Aug 5, 2022

View reviewed changes

Merge branch 'main' into main

e09ebf5

mergify bot merged commit a02ef9c into aws:main Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles #20966

fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles #20966

haslam22 commented Jul 2, 2022

gitpod-io bot commented Jul 2, 2022

mattiamatrix commented Jul 14, 2022

haslam22 commented Jul 14, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 2, 2022

haslam22 Aug 4, 2022

TheRealAmazonKendra Aug 2, 2022

haslam22 Aug 4, 2022

TheRealAmazonKendra Aug 2, 2022

haslam22 Aug 4, 2022

TheRealAmazonKendra commented Aug 2, 2022

haslam22 commented Aug 4, 2022

TheRealAmazonKendra left a comment

mergify bot commented Aug 5, 2022

aws-cdk-automation commented Aug 5, 2022

mergify bot commented Aug 5, 2022

fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles #20966

fix(custom-resources): AwsCustomResource requires a policy which updates immutable roles #20966

Conversation

haslam22 commented Jul 2, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 2, 2022

mattiamatrix commented Jul 14, 2022

haslam22 commented Jul 14, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 2, 2022

Choose a reason for hiding this comment

haslam22 Aug 4, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 2, 2022

Choose a reason for hiding this comment

haslam22 Aug 4, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 2, 2022

Choose a reason for hiding this comment

haslam22 Aug 4, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra commented Aug 2, 2022

haslam22 commented Aug 4, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 5, 2022

aws-cdk-automation commented Aug 5, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 5, 2022