Skip to content
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

feat(iam): immutable IAM roles #4501

Closed
wants to merge 6 commits into from

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Oct 14, 2019

This adds an IRole implementation that ignores all mutating operations.
Useful when you want to turn off CDK's automatic permissions management,
and have full control over a role's policy
(and/or if the CDK generated policy exceeds the size limit).
To accommodate this new behavior, add a new method to IIdentity:
addPolicy, which is meant to replace attachInlinePolicy,
which can leave Policy resources unattached, resulting in an invalid CloudFormation template.

Fixes #2985
Fixes #4465


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

@skinny85 skinny85 requested a review from rix0rrr October 14, 2019 23:40
@mergify
Copy link
Contributor

mergify bot commented Oct 14, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@skinny85
Copy link
Contributor Author

@rix0rrr this contains a proposal of adding a new method to IIdentity, and an example of using it inside CodeBuild's Project class here. Let me know what you think of this approach!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@SomayaB SomayaB added the contribution/core This is a PR that came from AWS. label Oct 15, 2019
@@ -901,8 +901,7 @@ export class Project extends ProjectBase {
},
}));

const policy = new iam.Policy(this, 'PolicyDocument', {
policyName: 'CodeBuildEC2Policy',
const policy = this.role.createAndAttachPolicy('CodeBuildVpcPolicy', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that this can return nothing is a bit of a gotcha and feels like an unnatural API.

Can it not return a special kind of Policy object that doesn't render to CFN, or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the change should be that unattached Policy objects just don't render? This new method feels unnatural.

Copy link
Contributor Author

@skinny85 skinny85 Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Why is returning something optionally "unnatural"...?

Returning a magical Policy that won't render if it's unattached seems a lot more unnatural to me (we don't have a single resource in the library that behaves that way - doesn't render under some conditions), and also won't work in this case - we are making the Project depend on the Policy, so we can't not render it (the resulting template would be invalid).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Why is returning something optionally "unnatural"...?

How is it to be expected that a function that is called createThing doesn't actually create and return a thing? What I don't like about this is:

  • Consumers have to write additional ifs. I will fight against any design that requires my consumers to add an if to their code.
  • We could only conceivably have typed this function this way once we have the requirement that there may be a class which doesn't do the thing. If we had made this function BEFORE we had the requirement of the no-op class, the API would have been locked in to the normal API (which always returns a thing) and then we wouldn't have been able to add this in this way. That leads me to think we are confusing layers of functionality.

we don't have a single resource in the library that behaves that way - doesn't render under some conditions

We do have objects that render differently under certain conditions (references turn into { Ref: xxx } or { Fn::ImportValue: Yyy } under certain conditions) so that is not without precedent. And in fact, we also have policy objects that automatically get added to the document if they're non-empty (see BucketPolicy, QueuePolicy, TopicPolicy).

Automatic adding on non-empty is roughly what I'm asking for here as well. The difference being that in the existing case, bucket.addToResourcePolicy() adds the AWS::IAM::Policy resource to the template, and in this case it would be policy.addToPolicy() that adds the AWS::IAM::Policy to the template.

Yes, the letter of the code will be different (because the class structure is slightly different), but I would argue it's in the same spirit.

And in any case, CDK is intended to make it easier to work with CloudFormation templates, not push the same problems and need for case analysis down to the user if we can deal with them.

we are making the Project depend on the Policy, so we can't not render it

Obviously we would have to not render the dependency in the case we're not rendering the resource. Yes, I understand it is more work, but it's not like that is impossible to do.

Copy link
Contributor Author

@skinny85 skinny85 Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumers have to write additional ifs. I will fight against any design that requires my consumers to add an if to their code.

That's a noble point, only slightly undercut by the fact that we're adding a method to an interface with an existing method addToPolicy(statement: PolicyStatement): boolean...

Obviously we would have to not render the dependency in the case we're not rendering the resource. Yes, I understand it is more work, but it's not like that is impossible to do.

How? CfnResource.addDependsOn(resource) adds resource to a dependsOn Set field of CfnResource. How would you not render that?

And other cases - what if somebody does CfnOutput(this, 'Output', { value: policy.ref })? How could we possibly not renderpolicy in that case?

Copy link
Contributor

@rix0rrr rix0rrr Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So probably this is a bad idea for CfnPolicy, but it could be true for Policy.

As for the second question, we could render it as { Ref: AWS::NoValue }. Not saying it's the best answer but it is an answer. Another thing we could do is reify the policy when someone refers to it. There is precedent for something like this btw, see iam.LazyRole.

As for addDependsOn()... that is an issue. We would really have to use addDependency, which has a mechanism (in IDependable) to control what gets depended on when you take a dependency on a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just spent basically the entire day implementing LazyPolicy (which I believe is basically what you asked). And it was horrible. Feel free to judge yourself (I've updated the PR with what I came up with).

We turned a clean, simple API into a Frankenstein monster full of hacks, throwing exceptions, and weird edge cases. I don't even know how to write the docs for this new method, it's so convoluted what it does now. And I'm still not able to actually make it work - the build fails on the codebuild package.

I really urge you to reconsider. I think we're going down the wrong path here.

}

public addToPolicy(_statement: PolicyStatement): boolean {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might need to return true. I'm sorry :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  /**
   * Add to the policy of this principal.
   *
   * @returns true if the statement was added, false if the principal in
   * question does not have a policy document to add the statement to.
   */
  addToPolicy(statement: PolicyStatement): boolean;

I know that the statement wasn't really added, but false means that a policy statement could NEVER be added and one should be added to the resource instead.

In this case I guess it's a toss-up between "the user has already prepared all statements so don't do anything" and "this role is immutable so add to the resource instead."

I would say we either make it configurable between those 2, or default to the do-less option (with an option to make it configurable later). Right now, the do-less default is returning "true" (pretend everything worked fine attaching to the principal).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be true here as well then...? (for roles imported with { mutable: false })

class ImmutableImport extends Import {
public addToPolicy(_statement: PolicyStatement): boolean {
return false;
}
public attachInlinePolicy(_policy: Policy): void {
// do nothing
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should.

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR description to include the motivation for the change

// otherwise, creating the Project fails,
// as it requires these permissions to be already attached to the Project's Role
const cfnPolicy = policy.node.defaultChild as CfnResource;
project.addDependsOn(cfnPolicy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it work to add the dependency on the policy object? I believe it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • project.addDependsOn(policy) doesn't compile (Policy is not a CfnResource)
  • this.node.addDependency(policy) adds way too much dependencies, including a circular one between policy and itself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this.node.addDependency(cfnPolicy)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this.node.addDependency(cfnPolicy)?

Nope. Same as this.node.addDependency(policy) - a million dependencies, including a cyclical one.

* It will be created in the scope of this principal (if it's mutable)
* @param props the construction properties of the new policy
*/
createAndAttachPolicy(id: string, props?: PolicyProps): Policy | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is weird... "xAndY" is usually a sign of a bad name. Maybe just addPolicy? Semantically it means the same thing: role.addPolicy('foo', { xxx })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking createAttachedPolicy might be a better name. What do you think?

For now, changed to addPolicy.

Copy link
Contributor Author

@skinny85 skinny85 Oct 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also worried that addPolicy looks too similar to addToPolicy (which is a method present on the superinterface of IIdentity, IPrincipal).

packages/@aws-cdk/aws-iam/lib/identity-base.ts Outdated Show resolved Hide resolved
@skinny85
Copy link
Contributor Author

Changed the name of the new method to addPolicy and rebased.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Contributor Author

Changed the name of the new method to addPolicy and rebased.

This time, actually changed it.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

This adds an IRole implementation that ignores all mutating operations.
To accommodate this new behavior, add a new method to IIdentity:
createAndAttachPolicy, which is meant to replace attachInlinePolicy,
which can leave Policy resources unattached, which is illegal in CloudFormation.

Fixes aws#2985
Fixes aws#4465
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

* This is the same as calling `policy.addToXxx(principal)`.
* @param policy The policy resource to attach to this principal [disable-awslint:ref-via-interface]
* Creates and attaches a Policy to this principal, if it is mutable.
* If this principal is immutable, does nothing, and returns undefined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring can't be correct anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm actually struggling to write these docs, like I said here: #4501 (comment)

import { IRole } from './role';
import { IUser } from './user';

export class LazyPolicy extends Resource implements IPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need a docstring explaining what it does.

Does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not currently, because of the magical overrides of ref and logicalId of the custom L1 here: https://github.com/aws/aws-cdk/pull/4501/files?file-filters%5B%5D=.ts#diff-f60a11b2b65df07bd241981fe0a626d7R37-R55

@skinny85
Copy link
Contributor Author

Hey @rix0rrr,

like I said before, I believe we're heading down the wrong path here. Even putting aside the fact that I can't get the code working with the hacky overrides needed for the L1 in LazyRole, I think the code that I had to write here, to make the vision of addPolicy(): IPolicy a reality, is bad. It's ugly, fragile, and I don't even know what exactly to write in the documentation of addPolicy(), it became so complicated.

Is there any way I can convince you to back out from this direction? I strongly feel this is the wrong thing to do.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 28, 2019

I implemented LazyPolicy by doing a deleteNode instead if it turned out to be empty. This probably also has nasty side effects somewhere, but so far seems to work? (haven't finished the build yet because it keeps failing for unrelated reasons).

If this approach does turn out to work, I'm still not sure this shouldn't be the default behavior of Policy (maybe using a flag, mustCreate: true|false or something?)

@aws-cdk-automation
Copy link
Collaborator

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 added a commit to skinny85/aws-cdk that referenced this pull request Oct 29, 2019
The DynamoDB global table construct was incorrectly declaring its dependencies.
That resulted in resources in the coordinator stack depending on the global tables,
which cannot work, as those are from different stacks.
Changed the logic to declare dependencies between the stacks explicitly.

Fixes aws#4501
Copy link
Contributor Author

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things:

  1. I don't believe this does what we need in CodeBuild's Project class.
  2. If we're going down the LazyPolicy route, maybe adding addToPolicy doesn't make much sense, and we should instead have clients of attachInlinePolicy create a LazyPolicy, and pass it there?

}
}
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test that shows the dependency is there if the policy was attached to anything?

// otherwise, creating the Project fails,
// as it requires these permissions to be already attached to the Project's Role
const cfnPolicy = policy.node.findChild('Resource') as CfnResource;
const cfnPolicy = policy.node.defaultChild as CfnResource;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work in the current version? From what I can tell, LazyPolicy extends Policy, and so this will leave a dependency in the template to a resource that will be later removed, if I'm correct.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 30, 2019

Please stop overwriting previous commits with git push -f.

I'm trying to find old comments back to see what your problems with addDependency were (why you needed to use addDependsOn) and I can't find it back anymore because for whatever reason GitHub has eaten the comment.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 30, 2019

I can't retrieve why addDependency wasn't good enough, but I switched the code back to it and it seems to work.... ?

2. If we're going down the LazyPolicy route, maybe adding addToPolicy doesn't make much sense, and we should instead have clients of attachInlinePolicy create a LazyPolicy, and pass it there?

Not exactly sure what you mean here, but if it's up to me we roll Policy and LazyPolicy into one and the distinction disappears.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@skinny85
Copy link
Contributor Author

skinny85 commented Oct 30, 2019

Please stop overwriting previous commits with git push -f.

I'm trying to find old comments back to see what your problems with addDependency were (why you needed to use addDependsOn) and I can't find it back anymore because for whatever reason GitHub has eaten the comment.

I doubt it has to do anything with git push -f (a lot of comments from the first revision are still there...). I think it's because you changed the code in that part relative to the first revision, and GitHub removed those comments as out-of-date.

I can't retrieve why addDependency wasn't good enough, but I switched the code back to it and it seems to work.... ?

The issue was I was doing this.node.addDependency(policy), instead of project.node.addDependency(policy) (project is the L1) - the latter does indeed work.

@skinny85
Copy link
Contributor Author

  1. If we're going down the LazyPolicy route, maybe adding addToPolicy doesn't make much sense, and we should instead have clients of attachInlinePolicy create a LazyPolicy, and pass it there?

Not exactly sure what you mean here, but if it's up to me we roll Policy and LazyPolicy into one and the distinction disappears.

I'm saying that:

  1. We should export LazyPolicy from the IAM package (right now it's private to it).
  2. We should remove the addPolicy method that was added in this PR.
  3. In CodeBuild's Project, we should use LazyPolicy and attatchInlinePolicy instead of addToPolicy.

If you want to roll the capabilities of LazyPolicy into Policy itself, that's fine by me; in that case, substitute "LazyPolicy" with "Policy with mustCreate: false" in the above description.

Is this clearer now?

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 31, 2019

If you want to roll the capabilities of LazyPolicy into Policy itself, that's fine by me; in that case, substitute "LazyPolicy" with "Policy with mustCreate: false" in the above description.

Yep, although I would have mustCreate: false by default.

I am fine with removing addPolicy() (though it could still be useful in the same way we have addListener() etc)

Who is going to finish this PR, you or me? I have a feeling our roles got reversed over the last few commits :)

I think it's because you changed the code in that part relative to the first revision, and GitHub removed those comments as out-of-date.

Yes but I've also seen it show old comments as "out-of-date". My current theory is that because the commit the comments are linked to is gone, it's not showing them anymore.

@skinny85
Copy link
Contributor Author

Yep, although I would have mustCreate: false by default.

I think that's fine. It's not strictly backwards-compatible, but I think the old behavior was purely an irritation, and didn't add much value.

Who is going to finish this PR, you or me? I have a feeling our roles got reversed over the last few commits :)

Go for it. I think that's fine that we switched roles, that's what collaboration is all about :).

I think you can also roll back all of the changes I did to IPolicy, and method signatures that I've changed from Policy to IPolicy - it made sense when LazyPolicy was another implementation of IPolicy, but since LazyPolicy will now be folded into Policy, they are not needed.

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

5 similar comments
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 27, 2019

Subsumed by new PR.

@rix0rrr rix0rrr closed this Dec 27, 2019
rix0rrr added a commit that referenced this pull request Jan 15, 2020
Add a new method to `Role` called `withoutPolicyUpdates()`, which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK.

Needs only be used with roles that are created in the same CDK application; for imported roles, supplying `mutable=false` when calling `Role.fromRoleArn()` is sufficient.

Fixes #2985.
Fixes #4465.
Closes #4501.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
5 participants