resource/aws_iam_user_group_membership: allow non-exclusive group memberships #3365

devonbleak · 2018-02-14T01:03:25Z

Resolves #2881 - allows multiple aws_iam_group_memberships to co-exist peacefully by adding a new computed field group_members to manage adding/removing users.

Adds a remove_unmanaged_members property that can be set to true to restore previous functionality and documentation caveat about flapping with that setting.

bflad

Hi @devonbleak 👋 thanks for this contribution! This is a nice enhancement, but I think the implementation is more complex than it needs to be. I may be missing the use case that requires non-exclusively managing a list of users in one resource though, please let me know!

To simplify this, I would prefer to introduce a singular user attribute that manages a single user-to-group membership (almost like how it used to be 2 years ago). Everything becomes much simpler afterwards:

No CustomizeDiff
No additional flag attribute (remove_unmanaged_members)
No sidecar storage attribute (group_members)

The implementation would entail something like:

Set user to ForceNew: true
Set users to Optional: true
Set users to ConflictsWith: []string{"user"}
Use d.GetOk() to switch between the user list of 1 item or the full user lists for addition/removal in Create/Delete
Use d.GetOk() to switch to only checking if the singular user is a member in Read
No update logic necessary

What do you think?

bflad · 2018-02-27T02:05:55Z

aws/resource_aws_iam_group_membership.go

+			"remove_unmanaged_members": &schema.Schema{
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,


This is a breaking change for the resource as written. It should default to true to keep backwards compatibility.

bflad · 2018-02-27T02:07:00Z

aws/resource_aws_iam_group_membership.go

@@ -35,6 +36,18 @@ func resourceAwsIamGroupMembership() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
+
+			"remove_unmanaged_members": &schema.Schema{


Minor nitpick: As of Go 1.7+, the &schema.Schema after the attribute name declarations are unnecessary

bflad · 2018-02-27T02:18:12Z

aws/resource_aws_iam_group_membership_test.go

@@ -242,3 +324,14 @@ resource "aws_iam_user" "user" {
 }
 `, groupName, membershipName, userNamePrefix)
 }
+
+func testAccAWSGroupMemberAddUnmanagedUser(groupName string, userName string) func() {


We should probably create a complimentary testAccAWSGroupMemberRemoveManagedUser

devonbleak · 2018-02-27T04:15:55Z

Thanks @bflad, this makes sense and I'll get the feedback incorporated.

…

On Mon, Feb 26, 2018 at 6:46 PM, Brian Flad ***@***.***> wrote: ***@***.**** commented on this pull request. Hi @devonbleak <https://github.com/devonbleak> 👋 thanks for this contribution! This is a nice enhancement, but I think the implementation is more complex than it needs to be. I may be missing the use case that requires non-exclusively managing a *list* of users in one resource though, please let me know! To simplify this, I would prefer to introduce a singular user attribute that manages a single user-to-group membership (almost like how it used to be 2 years ago). Everything becomes much simpler afterwards: - No CustomizeDiff - No additional flag attribute (remove_unmanaged_members) - No sidecar storage attribute (group_members) The implementation would entail something like: - Set user to ForceNew: true - Set users to Optional: true - Set users to ConflictsWith: []string{"user"} - Use d.GetOk() to switch between the user list of 1 item or the full user lists for addition/removal in Create/Delete - Use d.GetOk() to switch to only checking if the singular user is a member in Read - No update logic necessary What do you think? ------------------------------ In aws/resource_aws_iam_group_membership.go <#3365 (comment)> : > @@ -35,6 +36,18 @@ func resourceAwsIamGroupMembership() *schema.Resource { Required: true, ForceNew: true, }, + + "remove_unmanaged_members": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, This is a breaking change for the resource as written. It should default to true to keep backwards compatibility. ------------------------------ In aws/resource_aws_iam_group_membership.go <#3365 (comment)> : > @@ -35,6 +36,18 @@ func resourceAwsIamGroupMembership() *schema.Resource { Required: true, ForceNew: true, }, + + "remove_unmanaged_members": &schema.Schema{ Minor nitpick: As of Go 1.7+, the &schema.Schema after the attribute name declarations are unnecessary ------------------------------ In aws/resource_aws_iam_group_membership_test.go <#3365 (comment)> : > @@ -242,3 +324,14 @@ resource "aws_iam_user" "user" { } `, groupName, membershipName, userNamePrefix) } + +func testAccAWSGroupMemberAddUnmanagedUser(groupName string, userName string) func() { We should probably create a complimentary testAccAWSGroupMemberRemoveMan agedUser — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3365 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3vI2TKEJblfpKR4KtT0zPuqAd3Rwbfks5tY2yegaJpZM4SEpYu> .

devonbleak · 2018-03-05T21:58:14Z

@bflad thinking about this more, would it make sense to just create a new aws_iam_user_group_membership resource rather than having a single resource that may conflict with itself even with the changes to user vs users?

devonbleak · 2018-03-08T17:55:44Z

@bflad the more I think about having a 1:1 user-group membership resource the more it breaks down. Consider the case where we have a list of users and a list of groups - there wouldn't be a way to add all the users to all the groups if the lengths of those lists has a common factor. I'm going to go ahead with creating a new resource that will take a list of users and a single group and will be non-exclusive with group memberships.

bflad · 2018-03-14T20:45:40Z

@devonbleak sorry for my lack of reply earlier. Do you need any help?

Just to confirm the use case you're thinking about, it is something like this?

variable "users" {
  default = [
    "a",
    "b",
  ]
}

variable "groups" {
  default = [
    "z",
    "y",
    "x",
  ]
}

resource "aws_iam_user" "example" {
  count = "${length(var.users)}"
  # ... other configuration ...
}

resource "aws_iam_group" "example" {
  count = "${length(var.groups)}"
  # ... other configuration ...
}

resource "aws_iam_user_group_membership" "example" {
  # yikes what would we put here? :)
}

What do you think about a groups attribute on aws_iam_user instead? The above becomes something like:

variable "users" {
  default = [
    "a",
    "b",
  ]
}

variable "groups" {
  default = [
    "z",
    "y",
    "x",
  ]
}

resource "aws_iam_user" "example" {
  count = "${length(var.users)}"
  # ... other configuration ...
  # add user to all groups
  groups = ["${aws_iam_group.example.*.id}"]
}

resource "aws_iam_group" "example" {
  count = "${length(var.groups)}"
  # ... other configuration ...
}

devonbleak · 2018-03-15T00:51:34Z

@bflad The aws_iam_user_group_membership would basically work the same as aws_iam_group_membership except it wouldn't touch user memberships that were added outside of it (similar to how the original PR works here with remove_unmanaged_users = false but without requiring the extra logic in CustomizeDiff or the sidecar). It would probably need a better name.

Adding a groups argument to aws_iam_user is an interesting solution but I think we'll still be missing a resource that just adds a list of users to a group and keeps them there without doing anything else (or vice versa adding a list of groups to a user).

bflad · 2018-04-06T16:18:02Z

@devonbleak I guess we have come full circle. 😄 Do you think you could implement this PR without the sidecar and defaulting the flag attribute to keep existing behavior (exclusive management)?

devonbleak · 2018-04-06T20:49:06Z

@bflad been thinking about this again the past couple days and thinking about implementing something like an aws_iam_user_group_memberships that would do the non-exclusive thing without the sidecar or the CustomizeDiff. The analogy I have in my head is like aws_iam_policy_attachment vs aws_iam_{group,role,user}_policy_attachment - the former behaves like aws_iam_group_membership while the latter ignores things it doesn't know about.

My original implementation of this basically just ignored things it didn't know about in the Read function and worked great except that it took two applies for a change to remove_unmanaged_users to actually complete which is why I ended up going the sidecar/CustomizeDiff route. It looks like we have what we need from the Go SDK in ListGroupsForUser.

I'll start working on this approach, but let me know if you'd like to see something different.

resource "aws_iam_user_group_memberships" "test" {
  user   = "${aws_iam_user.test.name}"
  groups = [
    "${aws_iam_group.test1.name}", 
    "${aws_iam_group.test2.name}"
  ]
}

bflad · 2018-04-07T00:12:18Z

I'd drop the s pluralization on the resource name, but sounds good.

devonbleak · 2018-04-13T23:12:01Z

@bflad implementation is done on resource/aws_iam_user_group_membership. I also added a note to aws_iam_group_membership about the conflicts behavior so new users aren't caught by surprise with it. Let me know if you'd like any additional changes!

bflad

Hi @devonbleak 👋 This is looking pretty good -- can you see my comments below and let me know if you have any questions or do not have time to implement the feedback? Thanks!

bflad · 2018-04-30T16:15:21Z