fix(iam): only create policies for imported roles if they're in the same account as the owning stack

[skinny85]

We also allow adding policies to the imported role if the owning stack does not have an account specified.

Fixes #2985
Fixes #3025

---

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

[mergify]

Pull Request Checklist

• Testing

• Unit test added (prefer to add a new test rather than modify existing tests)
• CLI change? Re-run/add CLI integration tests

• Documentation

• Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
• README: update module README
• Design: for significant features, follow the design process

• Title uses the format type(scope): text

• Type: fix, feat, refactor go into CHANGELOG, chore is hidden
• Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
• Style: use all lower-case, do not end with a period

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

• Sensitive Modules (requires 2 PR approvers)

• IAM document library (in @aws-cdk/aws-iam)
• EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
• Grant APIs (if not based on official documentation with a reference)

[skinny85]

I'm actually wondering whether the logic around stacks without account set should not be a little different....

When providing the roleArn to fromRoleArn, you can use the AWS::AccountId pseudo-parameter inside, right? So, we can now say that if your stack does not have account set, we will add new policies to the Role imported in its scope only if the account part of roleArn is equal to AWS::AccountId. If you have a concrete account put there, we will ignore all calls to add a policy.

So:

const stack = new Stack(app, 'S');

// this role will create new policies
iam.Role.fromRoleArn(stack, 'R1',
  `arn:aws:iam::${Aws.ACCOUNT_ID}:role/MyRole`);

// this role will ignore all calls to `addToRolePolicy` and `attachInlinePolicy`
iam.Role.fromRoleArn(stack, 'R2',
  'arn:aws:iam::123456789012:role/MyRole');

Thoughts on this @rix0rrr ?

[skinny85]

@rix0rrr can you address my question from above?

[mergify]

Pull Request Checklist

• Testing

• Unit test added (prefer to add a new test rather than modify existing tests)
• CLI change? Re-run/add CLI integration tests

• Documentation

• Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
• README: update module README
• Design: for significant features, follow the design process

• Title uses the format type(scope): text

• Type: fix, feat, refactor go into CHANGELOG, chore is hidden
• Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
• Style: use all lower-case, do not end with a period

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

• Sensitive Modules (requires 2 PR approvers)

• IAM document library (in @aws-cdk/aws-iam)
• EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
• Grant APIs (if not based on official documentation with a reference)

[rix0rrr]

Thoughts on this @rix0rrr ?

Don't like it very much, because it can still be the case that the accounts are specified and the same.

All in all--isn't it easier to give control to the user, by providing them a class as described in #3753?

[skinny85]

Thoughts on this @rix0rrr ?

Don't like it very much, because it can still be the case that the accounts are specified and the same.

All in all--isn't it easier to give control to the user, by providing them a class as described in #3753?

I don't think the 2 are exclusive. I want to do #3753 as well. This is more about changing the current behavior, which is obviously incorrect for the case where the ARN account and the Stack account are different.

[michaeljfazio]

Hanging out to see this issue fixed. Code pipeline basically useless in cross-account environments :(

[skinny85]

After talking with @rix0rrr offline, submitted a new revision. I've covered a lot more cases, included inline policies added with attachInlinePolicy in the logic, and also added the option of specifying that you want the returned Role to be immutable by passing { mutable: false } to the fromRoleArn function (I called it mutable, as I didn't like that immutable in the default (false) was a double negative).

I would recommend starting the review with looking at the unit tests, as they document what is the behavior we expect for the various cases (and there is a lot of different cases here!).

[rix0rrr]

This PR closes #2985, by the way.

[skinny85]

I've re-worked the tests completely; @rix0rrr let me know how you like their structure now.

[rix0rrr]

Love it! Still a shit-ton to read, but this structure is great!

[rix0rrr]

Still prefer that we change the mutable terminology.

[skinny85]

Submitted a new revision.

@rix0rrr can you go through the tests, and confirm all of them have (in your mind) the correct behavior right now? I feel like a few could go either way, so I just wanted to confirm the expectations for the behavior are the same.

[skinny85]

Actually, if you could confirm that as well @michaeljfazio , that would be super helpful :)

[rix0rrr]

@rix0rrr can you go through the tests, and confirm all of them have (in your mind) the correct behavior right now?

I believe they do (or at least, the most important ones I looked at). Do you have any particular ones in mind you're concerned about?

[skinny85]

Included last comments and rebased.

[mergify]

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

[mergify]

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