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): customer managed policies #3578

Merged
merged 12 commits into from
Aug 9, 2019

Conversation

IainCole
Copy link
Contributor

@IainCole IainCole commented Aug 8, 2019

Closes #2974

Adds support for customer managed IAM policies.

This was a little weird because the ManagedPolicy class works for both AWS and Customer managed policies, with the latter being something the user can create and the former being something they can only look up. I tried to emulate Policy/IPolicy as much as seemed appropriate.

The only way I could see to get this working without changing the signature of the fromAwsManagedPolicyName function was to disable the construct-interface-extends-iconstruct and resource-interface-extends-resource lint rules and implement it like this.

@IainCole
Copy link
Contributor Author

IainCole commented Aug 8, 2019

Build is failing due to the change in signature of the protected constructor of the placeholder class that was there before. Will defer to someone with more domain knowledge as to the correct path forward.

@eladb eladb changed the title feat(aws-iam): support customer managed policies feat(iam): customer managed policies Aug 8, 2019
public static fromManagedPolicyName(scope: Construct, id: string, managedPolicyName: string): IManagedPolicy {
class Import extends Resource implements IManagedPolicy {
public readonly managedPolicyArn = Lazy.stringValue({
produce(ctx: IResolveContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this have to be a lazy string and not just:

const stack = Stack.of(scope);

class Import extends Resource implements IManagedPolicy {
  public readonly managedPolicyArn = stack.formatArn({...});

  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the ARN consists of the path plus the name? path is not even a parameter here.

Copy link
Contributor Author

@IainCole IainCole Aug 8, 2019

Choose a reason for hiding this comment

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

I used the fromAwsManagedPolicyName function as a reference. If we want to change stuff I can do that. For the path thing, I think you're right. I'll update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't the ARN consists of the path plus the name? path is not even a parameter here.

So for the import class I'm not sure if it needs to be a separate parameter on this function or if you can just pass the whole thing in as the managedPolicyName. Similarly to how fromAwsManagedPolicyName accepts the service-role prefix as part of the managedPolicyName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at other Principals that can have a path, the Group class uses the ARN for lookup with a fromGroupArn method, the User class doesn't have a lookup. Perhaps we should make this lookup fromPolicyArn so that it's consistent with Group?

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 guess not

Lazy.stringValue({ produce: () => generatePolicyName(scope, resource.logicalId) }),
});

this.managedPolicyName = this.physicalName;
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to use this.getResourceNameAttribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also based off Policy, does that need to be changed too?

@@ -82,5 +82,11 @@
"engines": {
"node": ">= 8.10.0"
},
"awslint": {
"exclude": [
"construct-interface-extends-iconstruct:@aws-cdk/aws-iam.IManagedPolicy",
Copy link
Contributor

Choose a reason for hiding this comment

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

we really need a way to add comments here

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 8, 2019

Aha, looks like we made a mistake in the past, so we're going to need to override the breaking change detector:

err  - INITIALIZER @aws-cdk/aws-iam.ManagedPolicy.<initializer>: argument scope, newly required argument. [new-argument:@aws-cdk/aws-iam.ManagedPolicy.<initializer>]
err  - INITIALIZER @aws-cdk/aws-iam.ManagedPolicy.<initializer>: argument id, newly required argument. [new-argument:@aws-cdk/aws-iam.ManagedPolicy.<initializer>]

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 9, 2019

EDIT: Nevermind, wrong PR.

@rix0rrr rix0rrr closed this Aug 9, 2019
@rix0rrr rix0rrr reopened this Aug 9, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 9, 2019

@IainCole managedPolicyName seems to not be required, so CFN can make a name. Any particular reason you chose to model it this way?

@mergify mergify bot merged commit 4681d01 into aws:master Aug 9, 2019
@IainCole
Copy link
Contributor Author

IainCole commented Aug 9, 2019

@IainCole managedPolicyName seems to not be required, so CFN can make a name. Any particular reason you chose to model it this way?

Just because I was following what Policy was doing, but it seems like the name is required for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot create IManagedPolicy from a customer managed policy
3 participants