puppetlabs-aws icon indicating copy to clipboard operation
puppetlabs-aws copied to clipboard

iam_policy_attachment needs to implement ensure parameter

Open reubenavery opened this issue 9 years ago • 8 comments
trafficstars

We need to be able to set ensure => absent for iam_policy_attachments. Seems like a deliberate oversight though, am I missing something?

reubenavery avatar Aug 05 '16 14:08 reubenavery

So in the case that you describe, this would detach the policy from all users, groups, and roles?

prozach avatar Aug 05 '16 17:08 prozach

well, only as defined within the resource declaration.

reubenavery avatar Aug 05 '16 17:08 reubenavery

@reubenavery Well, if you pass an empty array to any of the params, the entries should get removed, effectively achieving what you desire.

prozach avatar Aug 05 '16 17:08 prozach

Thank you @zleswomp. This isn't in keeping with a fundamental Puppet design pattern however, and makes for messy code. Resource declarations should be able to maintain all the same attributes but with an ensure => absent, ignore all these and effectively go away.

In particular with our usage, we have a considerable amount of abstraction going on around this using defined resource types, many of which reside within other defined resources, driven by Hiera, and leveraging Puppet's design by doing things like passing $ensure down through the stack.

Having to switch to different code for "if we're creating, use this code, otherwise use that code" makes for very messy code as well.

Illustrating this, if you're interested, please see: https://github.com/bitswarmlabs/puppetmaster-envs/blob/core/hieradata/nodes/puppet.bitswarm.internal.yaml#L35 and related Puppet manifests within https://github.com/bitswarmlabs/puppetmaster-envs/tree/core/modules/bsl_infrastructure/manifests

reubenavery avatar Aug 05 '16 17:08 reubenavery

Totally understand. Ensurable should be pretty easy to implement, I just didn't because of the workaround above. The attachment type is a bit different in that its not real, since its only drawing connections between existing relationships. I'd thought that it might make sense to have a default like the following somewhere pretty high in the scope:

Iam_policy_attachment {
  groups => [],
  users => [],
  roles => [],
}

An implementation of ensure => absent could effectively do just that, too.

prozach avatar Aug 05 '16 18:08 prozach

If I were to have started this from scratch, I would've probably gone the route instead of adding a "policies" parameter to iam_user, iam_group, iam_role, instead of creating this as its own free standing resource type.

reubenavery avatar Aug 05 '16 18:08 reubenavery

@reubenavery Thats fair, and you still could but I think it will be less flexible in the end. For my use case, I don't want to care about user or group permissions individually, but instead wish to think about the access that is being granted, then assigning that grant to the resources that require it. This way of thinking was adapted from the POSIX user management. To manage which groups a user should belong to on a user resource becomes cumbersome if you would like not to treat users like snowflakes. If you manage policy attachment on the user, then you end up spreading the access control tunable around your code base instead of centralizing it, and in my experience it becomes harder to keep in sync when access changes are necessary. Centralizing the attachment of a policy in the way that I've done allows for centralized access control rather than digging around in the code. It may require that you structure your code a bit differently, but I didn't dig into the links you posted too deep. Just explaining my way of thinking.

prozach avatar Aug 05 '16 18:08 prozach

Just briefly looking at your code and hiera data, if you are removing the iam_policy when you receive an ensure => absent, then perhaps you just skip the iam_policy_attachment resource entirely unless you have recieved an ensure => present. Thats a single conditional that would get you out of this.

Taking a quick look at what it would take to implement ensurable for this type, I'm not sure what it would mean to have an exists? method. If any attachments are present then return true. If none are found return false? And to ensure absent would remove all attachments, or just the attachments specified?

prozach avatar Aug 08 '16 21:08 prozach