fix(codebuild): cannot use immutable roles for Project by rix0rrr · Pull Request #5608 · aws/aws-cdk

rix0rrr · 2020-01-02T11:27:16Z

Commit Message

fix(codebuild): cannot use immutable roles for Project

Immutably imported Roles could not be used for CodeBuild
Projects, because they would create a policy but be unable
to attach it to the Role. That leaves an unattached Policy,
which is invalid.

Fix this by making Policy objects only render to an AWS::IAM::Policy
resource if they actually have any effect. It is perfectly allowed to
create new unattached Policy objects, or have empty Policy objects.
Only if and when they actually need to mutate the policy of an IAM
identity will they render themselves to the CloudFormation template.
Being able to abstract away these kinds of concerns is exactly the value
of a higher-level programming model.

To allow for the rare cases where an empty Policy object would be
considered a programming error, we still have the flag mustCreate
which triggers the legacy behavior of alwyas creating the
AWS::IAM::Policy resource which must have a statement and be
attached to an identity.

Fixes #1408.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Immutably imported `Role`s could not be used for CodeBuild `Project`s, because they would create a policy but be unable to attach it to the Role. That leaves an unattached Policy, which is invalid. Fix this by making `Policy` objects only render to an `AWS::IAM::Policy` resource if they actually have any effect. It is perfectly allowed to create new unattached Policy objects, or have empty Policy objects. Only if and when they actually need to mutate the policy of an IAM identity will they render themselves to the CloudFormation template. Being able to abstract away these kinds of concerns is exactly the value of a higher-level programming model. To allow for the rare cases where an empty Policy object would be considered a programming error, we still have the flag `mustCreate` which triggers the legacy behavior of alwyas creating the `AWS::IAM::Policy` resource which must have a statement and be attached to an identity.

aws-cdk-automation · 2020-01-02T11:54:26Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb · 2020-01-02T12:03:41Z

packages/@aws-cdk/aws-iam/lib/policy.ts

+  private get shouldExist() {
+    return this.mustCreate || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
+  }


since this is temporal (i.e. only relevant after the tree has been fully initialized) and used only in one place, I think this code should just be part of prepare to avoid accidentally using it in the future from the wrong context.

eladb · 2020-01-02T12:08:45Z

packages/@aws-cdk/core/lib/construct.ts

+   *
+   * It is not an error if a child with the given name does not exist.
+   */
+  public removeChild(childName: string) {


Maybe rename to tryRemoveChild to indicate it won't throw, and return a boolean indicating if the child was actually removed or not.

I am a bit worried by potential abuse of this API. Maybe we can make it a bit safer and only allow this during "prepare" somehow? What do you think? Can you please also mark this as @experimental?

This also resolves #1408

I'm not too worried about misuse. The methods on node are pretty much "plumbing sticking out" anyway, not sure they're part of people's day-to-day operations with the API.

I would hope that people venturing there will be aware they're messing with internals which can do what they want, or have unintentional effects they themselves will be responsible for cleaning up. Not sure that limiting to prepare() is necessary.

aws-cdk-automation · 2020-01-02T13:19:26Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-02T13:20:05Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-02T13:28:35Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-02T14:36:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85 · 2020-01-02T18:58:04Z

packages/@aws-cdk/aws-iam/lib/policy.ts

+   *
+   * @default false
+   */
+  readonly mustCreate?: boolean;


I still don't love this name... I proposed changing it to lazy?: boolean in the previous PR, but I'm also open to other suggestions 🙂

forceResource?

mustExist?

I feel "lazy" is usually used to communicate something else.

Maybe just force?

I was considering force, slightly on the fence but I also don't have a better alternative. I can do force.

forceReify? forceRender?

aws-cdk-automation · 2020-01-03T14:34:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Stale

skinny85 · 2020-01-03T18:29:57Z

packages/@aws-cdk/aws-iam/lib/policy.ts

    if (this.document.isEmpty) {
-      result.push('Policy is empty. You must add statements to the policy');
+      if (this.force) {
+        result.push('Policy created with mustCreate=true is empty. You must add statements to the policy');


References old property name.

skinny85 · 2020-01-03T18:30:18Z

packages/@aws-cdk/aws-iam/lib/policy.ts

+        result.push('Policy created with mustCreate=true is empty. You must add statements to the policy');
+      }
+      if (!this.force && this.referenceTaken) {
+        result.push('Policy name has been read of empty policy. You must add statements to the policy so it can exist.');


See #5569 (comment)

skinny85 · 2020-01-03T18:30:34Z

packages/@aws-cdk/aws-iam/lib/policy.ts

+        result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`);
+      }
+      if (!this.force && this.referenceTaken) {
+        result.push('Policy name has been read of unattached policy. Attach to at least one principal: user, group or role.');


See here: #5569 (comment)

skinny85 · 2020-01-03T18:30:43Z

packages/@aws-cdk/aws-iam/lib/policy.ts

-      result.push(`Policy must be attached to at least one principal: user, group or role`);
+    if (!this.isAttached) {
+      if (this.force) {
+        result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`);


References old property name.

aws-cdk-automation · 2020-01-06T09:01:40Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-06T10:01:52Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-06T15:55:20Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-06T16:40:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-06T16:41:36Z

Thank you for contributing! Your pull request is now being automatically merged.

rix0rrr requested review from eladb and skinny85 January 2, 2020 11:27

rix0rrr self-assigned this Jan 2, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 2, 2020

rix0rrr mentioned this pull request Jan 2, 2020

feat(iam): Role.withoutPolicyUpdates() #5569

Merged

eladb previously requested changes Jan 2, 2020

View reviewed changes

Rico Huijbers added 2 commits January 2, 2020 14:11

Review comments

6ff1ad9

Make 'tryRemoveChild' experimental.

87c908c

rix0rrr requested a review from eladb January 2, 2020 13:12

Don't forget to rename invocation in IAM library as well

b13905a

Re-add erroneously refactored-away exclamation mark

360bdf1

skinny85 reviewed Jan 2, 2020

View reviewed changes

Rename mustCreate => force

cab660c

skinny85 suggested changes Jan 3, 2020

View reviewed changes

Review comments

72eacc1

rix0rrr requested a review from skinny85 January 6, 2020 08:53

Fix expected error message

f3e6be4

skinny85 approved these changes Jan 6, 2020

View reviewed changes

Merge branch 'master' into huijbers/lazy-policy

3b1663f

mergify bot merged commit 6103180 into master Jan 6, 2020

mergify bot deleted the huijbers/lazy-policy branch January 6, 2020 16:41

eladb mentioned this pull request Jan 22, 2020

Removing constructs from a stack #4848

Closed

pahud mentioned this pull request May 17, 2024

stabilize tryRemoveChild() aws/constructs#2267

Closed

Conversation

rix0rrr commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commit Message

Uh oh!

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jan 3, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Jan 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 6, 2020

Uh oh!

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rix0rrr commented Jan 2, 2020 •

edited

Loading

skinny85 Jan 3, 2020 •

edited

Loading