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

Resource definitions for managed policies. #71

Merged
merged 2 commits into from
Mar 16, 2015
Merged

Resource definitions for managed policies. #71

merged 2 commits into from
Mar 16, 2015

Conversation

DenverJ
Copy link
Contributor

@DenverJ DenverJ commented Mar 10, 2015

This PR adds resource support for IAM Managed policies.

There are 3 main areas I wasn't sure about so it would be good to get some feedback.

1 - The create method on iam.Policy is a bit strange and I'm thinking I should just remove it. This is because the GetPolicy call to load the object requires an ARN but the CreatePolicy call expects a friendly name (and I couldn't see a way to use a regular expression of another value as an identifier). That means it would be something like this to create a new policy.

iam.Policy(arn="arn:aws:iam::123456789:policy/example").create(PolicyName="example", PolicyDocument="{...}")

I think it's much clearer to simply call...

iam.create_policy(PolicyName="example", PolicyDocument="{...}")

(I actually think this is generally clearer in all cases that I have encountered so far)

2 - I have configured methods on the Group/User/Role objects to attach/detach a policy. Eg.

group.attach_policy(PolicyArn="arn:aws:iam::123456789:policy/example")

Does it make sense to also add a method for each of Group/User/Role to the Policy object? Eg.

policy.attach_group(GroupName="examplegroup")

3 - The call to get attached entities of a Policy is ListEntitiesForPolicy. It returns Group/User/Role names within the response, but I could not see a way to split out the response into different resources, so I am making a filtered call for each type. Eg. The following code makes 3 separate calls to ListEntitiesForPolicy.

policy.attached_groups.all()
policy.attached_users.all()
policy.attached_roles.all()

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.7% when pulling e57a125 on DenverJ:iam_managed_policies into b270599 on boto:develop.

@danielgtaylor danielgtaylor added the enhancement This issue requests an improvement to a current feature. label Mar 10, 2015
@danielgtaylor danielgtaylor self-assigned this Mar 10, 2015
@danielgtaylor
Copy link
Member

@DenverJ thanks for contributing to the resource definitions! 👍 This looks great so far, but I think we'll want to make just a couple minor changes related to the questions you had above.

  1. I agree we should remove the Policy.Create operation. Since you don't create with an ARN, but the resource requires an ARN to exist, it provides a very strange interface.

  2. Given how easy it is to create e.g. a Group instance I'm not sure we need these extra methods, but I'm not against adding them either. They are slightly nicer:

    policy = iam.Policy('....')
    
    iam.Group('devs').attach_policy(PolicyArn=policy.arn)
    # vs.
    policy.attach_group(GroupName='devs')
  3. I think that the collections you've added are useful and we should keep them. I also think it might make sense to provide an Entity resource that could be returned via something like policy.entities.all() or policy.attached_entities.all(). These entities should probably have a detach policy action and anything else that overlaps between all the entities and makes sense to add. Thoughts?

@trevorrowe any other feedback?

@trevorrowe
Copy link

  1. As Daniel mentioned, the self create methods are only possible when the resource identifier is user-provided, such as a User name or Group name. ARNs are generated by the service.
  2. I'm in favor of adding the complimentary methods from Group -> AttachPolicy and Policy -> AttachGroup.
  3. I agree with adding a collection to enumerate all attached entities in a single pass. This would require an additional resource type, but would provide good parity with the API feature of listing all entities. Please do keep the specific typed enumerations as well.

I think it warrants reviewing some of the method naming schemes with the IAM service team to ensure we are in-line with their expectations, but overall this looks great!

Add reverse attach/detach Group/Role/User actions for Policy.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.7% when pulling d1301c8 on DenverJ:iam_managed_policies into b270599 on boto:develop.

@DenverJ
Copy link
Contributor Author

DenverJ commented Mar 12, 2015

Thanks guys.

I have fixed up the first two points as per comments but I'm struggling with the last.

Assuming that I can work out the correct JMESPath expression to pull out the entity names from the response that looks like this...

<PolicyRoles>
  <member>
    <RoleName>DevRole</RoleName>
  </member>
</PolicyRoles>
<PolicyGroups>
  <member>
    <GroupName>Dev</GroupName>
  </member>
</PolicyGroups>
<IsTruncated>false</IsTruncated>
<PolicyUsers>
  <member>
    <UserName>Alice</UserName>
  </member>
  <member>
    <UserName>Bob</UserName>
  </member>
</PolicyUsers>

Then I would have an eg Entity(name=somename) but how then could I then define the actions on Entity such that it would make the correct call to attach/detach given that the actions are type specific. Eg. AttachGroupPolicy/AttachRolePolicy/AttachUserPolicy

Is there an existing example of doing something like this?

@danielgtaylor
Copy link
Member

@DenverJ, we've had a good discussion on this internally and I think I will avoid adding the generic Entity resource for the time being. It's possible that I could create a JMESPath expression to create a list of items with a name and type, but it's unlikely that I could model any useful actions on such a resource without hand-written customizations. It's an additive change so I can always take another look at adding it in the future.

For now I'd like to merge in your updated model and share it with the other SDKs. It looks great! 👍

danielgtaylor added a commit that referenced this pull request Mar 16, 2015
Resource definitions for managed policies.
@danielgtaylor danielgtaylor merged commit ebc0f95 into boto:develop Mar 16, 2015
@scriptsrc
Copy link

Can we get a version bump so this managed policies code is pushed out to pypi ?

@danielgtaylor
Copy link
Member

@MonkeySecurity a new release is coming soon. We've put this code through an internal review and there's minor change coming, then we're good to release.

@DenverJ DenverJ deleted the iam_managed_policies branch April 10, 2015 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue requests an improvement to a current feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants