From 43a178b95544677d22331be35cadc43bfc4172c3 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Thu, 9 Jun 2022 13:04:23 -0700 Subject: [PATCH] Reestablish old behavior of okta_group_memberships resource and add new property to signal all users should be tracked. --- okta/resource_okta_group_memberships.go | 198 +++++++++++++----- okta/resource_okta_group_memberships_test.go | 87 ++++++-- .../docs/r/group_memberships.html.markdown | 15 +- 3 files changed, 219 insertions(+), 81 deletions(-) diff --git a/okta/resource_okta_group_memberships.go b/okta/resource_okta_group_memberships.go index d5d7a3f09..8899deae2 100644 --- a/okta/resource_okta_group_memberships.go +++ b/okta/resource_okta_group_memberships.go @@ -35,6 +35,12 @@ func resourceGroupMemberships() *schema.Resource { Description: "The list of Okta user IDs which the group should have membership managed for.", Elem: &schema.Schema{Type: schema.TypeString}, }, + "track_all_users": { + Type: schema.TypeBool, + Optional: true, + Default: false, + Description: "The resource concerns itself with all users added/deleted to the group; even those managed outside of the resource.", + }, }, } } @@ -60,6 +66,7 @@ func resourceGroupMembershipsCreate(ctx context.Context, d *schema.ResourceData, // adding users to a group. Use a backoff to wait for at list one user to be // associated with the group. err = backoff.Retry(func() error { + // TODO, should we wait for all users to be added to the group? ok, err := checkIfGroupHasUsers(ctx, client, groupId, users) if err != nil { return backoff.Permanent(err) @@ -77,36 +84,35 @@ func resourceGroupMembershipsCreate(ctx context.Context, d *schema.ResourceData, } func resourceGroupMembershipsRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - groupId := d.Get("group_id").(string) client := getOktaClientFromMetadata(m) - expected_users := convertInterfaceToStringSetNullable(d.Get("users")) + groupId := d.Get("group_id").(string) + oldUsers := convertInterfaceToStringSetNullable(d.Get("users")) + trackAllUsers := d.Get("track_all_users").(bool) - // handle import edge case - if len(expected_users) == 0 { - logger(m).Info("reading group membership", "id", d.Id()) - userIDList, err := listGroupUserIDs(ctx, m, d.Id()) + // New behavior, tracking all users. + if trackAllUsers { + changed, newUserIDs, err := checkIfUsersHaveChanged(ctx, client, groupId, &oldUsers) if err != nil { - return diag.Errorf("unable to return membership for group (%s) from API", d.Id()) + return diag.Errorf("An error occured checking user ids for group %q, error: %+v", groupId, err) } - d.Set("group_id", d.Id()) - d.Set("users", convertStringSliceToSet(userIDList)) + if changed { + // set the new user ids if users have changed + d.Set("users", convertStringSliceToSet(*newUserIDs)) + } + return nil } - ok, err := checkIfGroupHasUsers(ctx, client, groupId, expected_users) + // Legacy behavior is just to check if any users have left the group. + changed, newUserIDs, err := checkIfUsersHaveBeenRemoved(ctx, client, groupId, &oldUsers) if err != nil { - return diag.Errorf("unable to complete group check for user: %v", err) + return diag.Errorf("An error occured checking user ids for group %q, error: %+v", groupId, err) } - if ok { - d.Set("group_id", d.Id()) - userIDList, _ := listGroupUserIDs(ctx, m, d.Id()) - d.Set("users", convertStringSliceToSet(userIDList)) - return nil - } else { - d.SetId("") - logger(m).Info("group (%s) did not have expected memberships or did not exist", groupId) - return nil + if changed { + // The user list has changed, set the new user ids to the users value. + d.Set("users", convertStringSliceToSet(*newUserIDs)) } + return nil } func resourceGroupMembershipsDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { @@ -145,60 +151,142 @@ func resourceGroupMembershipsUpdate(ctx context.Context, d *schema.ResourceData, return nil } -// checkIfGroupHasUsers firstly checks if the group has users given an immediate -// API call to list group users. It will additionally compare the current list -// of users found in the API call with a slice of known users passed in to the -// function. This second comparison is a stateful comparison. -func checkIfGroupHasUsers(ctx context.Context, client *okta.Client, groupId string, users []string) (bool, error) { - // TODO: This method should be renamed and/or refactored. Its name implies - // it is only checking for users but it can return false to signal that - // users have changed. +// checkIfUsersHaveChanged If the function returns true then users have been +// changed and the returned user ids should be considered the new set of users. +// Returns error for API errors. Returns false if no users have changed and the +// slice of returned strings will be empty. +func checkIfUsersHaveChanged(ctx context.Context, client *okta.Client, groupId string, users *[]string) (bool, *[]string, error) { + noop := []string{} + if users == nil || len(*users) == 0 { + return false, &noop, nil + } + + // We are using the old users map as a ledger to find users that have been + // removed from the user list. + oldUsers := toStrIndexedMap(users) + changed := false + // Collect all user ids that are returned from the API + usersFromAPI := []string{} + groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit}) if err := suppressErrorOn404(resp, err); err != nil { - return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId) + return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err) + } + + for _, user := range groupUsers { + // if the new user id is not in the old users map then the list of users has changed + if _, found := (*oldUsers)[user.Id]; !found { + changed = true + } + usersFromAPI = append(usersFromAPI, user.Id) } for resp.HasNextPage() { - var additionalUsers []*okta.User - resp, err = resp.Next(context.Background(), &additionalUsers) + groupUsers = nil + resp, err = resp.Next(context.Background(), &groupUsers) if err != nil { - return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId) + return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err) } - groupUsers = append(groupUsers, additionalUsers...) + + for _, user := range groupUsers { + // if the new user id is not in the old users map then the list of users has changed + if _, found := (*oldUsers)[user.Id]; !found { + changed = true + } + usersFromAPI = append(usersFromAPI, user.Id) + } + } + if len(*oldUsers) != len(usersFromAPI) { + changed = true } - // We need to return false if there isn't any users present. For eventual - // consistency issues create has a backoff/retry when we return false - // without an error here. Read occurs in the future and so it doesn't guard - // against eventual consistency. - if len(groupUsers) == 0 { - return false, nil + var result *[]string = &noop + if changed { + result = &usersFromAPI + } + + return changed, result, nil +} + +// checkIfUsersHaveBeenRemoved If the function returns true then users have been +// removed and the subset of returned user ids should be considered the new set +// of users. Returns error for API errors. Returns false if no users have been +// removed and the slice of returned strings will be empty. +func checkIfUsersHaveBeenRemoved(ctx context.Context, client *okta.Client, groupId string, users *[]string) (bool, *[]string, error) { + noop := []string{} + if users == nil || len(*users) == 0 { + return false, &noop, nil } - // Create a set to compare the users slice passed into the check to compare - // with what the api returns from list group users API call. - expectedUserSet := make(map[string]bool) + // We are using the old users map as a ledger to find users that have been + // removed from the user list. If it ever becomes sized 0 then we've found + // all of our user ids and no longer have to make API calls. + oldUsers := toStrIndexedMap(users) - // We train the set with false values for our previously known users. - for _, user := range users { - expectedUserSet[user] = false + groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit}) + if err := suppressErrorOn404(resp, err); err != nil { + return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err) } - // We confirm the latest user ids from list group users API call are still - // present in the set. for _, user := range groupUsers { - if _, ok := expectedUserSet[user.Id]; ok { - expectedUserSet[user.Id] = true + // Deleting user from API from the old users map + delete(*oldUsers, user.Id) + if len(*oldUsers) == 0 { + // All old users have been accounted for. + return false, &noop, nil + } + } + + for resp.HasNextPage() { + groupUsers = nil + resp, err = resp.Next(context.Background(), &groupUsers) + if err != nil { + return false, &noop, fmt.Errorf("unable to list users for group (%s) from API, error: %+v", groupId, err) + } + for _, user := range groupUsers { + // Deleting user from API from the old users map + delete(*oldUsers, user.Id) + if len(*oldUsers) == 0 { + // All old users have been accounted for. + return false, &noop, nil + } } } - // If one of the known users in the call to the check function is no longer - // in the list group users API call we return false. - for _, state := range expectedUserSet { - if !state { - return false, nil + // Any old users left are the IDs that have been removed from group. + + // This loop keeps the returned new user list in the same order as the users + // list passed in minus the now missing user IDs. + newUsers := []string{} + for _, userId := range *users { + // Any single user id found in old users should not be appended to the + // new users result. + if _, found := (*oldUsers)[userId]; !found { + newUsers = append(newUsers, userId) } } - return true, nil + return true, &newUsers, nil +} + +func checkIfGroupHasUsers(ctx context.Context, client *okta.Client, groupId string, users []string) (bool, error) { + groupUsers, resp, err := client.Group.ListGroupUsers(ctx, groupId, &query.Params{Limit: defaultPaginationLimit}) + if err := suppressErrorOn404(resp, err); err != nil { + return false, fmt.Errorf("unable to return membership for group (%s) from API", groupId) + } + return (len(groupUsers) > 0), nil +} + +func toStrIndexedMap(strs *[]string) *map[string]int { + result := map[string]int{} + if strs == nil { + return &result + } + + length := len(*strs) + for i := 0; i < length; i++ { + result[(*strs)[i]] = i + } + + return &result } diff --git a/okta/resource_okta_group_memberships_test.go b/okta/resource_okta_group_memberships_test.go index 643a0362e..d5d2d82e0 100644 --- a/okta/resource_okta_group_memberships_test.go +++ b/okta/resource_okta_group_memberships_test.go @@ -10,7 +10,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" ) -func TestAccOktaGroupMemberships_crud(t *testing.T) { +func TestAccResourceOktaGroupMemberships_crud(t *testing.T) { ri := acctest.RandInt() mgr := newFixtureManager(groupMemberships) @@ -36,8 +36,8 @@ func TestAccOktaGroupMemberships_crud(t *testing.T) { }) } -// TestAccOktaGroupMembershipsIssue1072 addresses https://github.com/okta/terraform-provider-okta/issues/1072 -func TestAccOktaGroupMembershipsIssue1072(t *testing.T) { +// TestAccResourceOktaGroupMemberships_Issue1072 addresses https://github.com/okta/terraform-provider-okta/issues/1072 +func TestAccResourceOktaGroupMemberships_Issue1072(t *testing.T) { resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProviderFactories: testAccProvidersFactories, @@ -62,8 +62,51 @@ resource "okta_group_memberships" "test" { }) } -// TestAccOktaGroupMembershipsIssue1094 addresses https://github.com/okta/terraform-provider-okta/issues/1094 -func TestAccOktaGroupMembershipsIssue1094(t *testing.T) { +// TestAccResourceOktaGroupMemberships_ClassicBehavior +// https://github.com/okta/terraform-provider-okta/issues/1094 +// https://github.com/okta/terraform-provider-okta/issues/1119 +// https://github.com/okta/terraform-provider-okta/issues/1149 +// https://github.com/okta/terraform-provider-okta/issues/1155 +func TestAccResourceOktaGroupMemberships_ClassicBehavior(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProvidersFactories, + CheckDestroy: testAccCheckUserDestroy, + Steps: []resource.TestStep{ + { + // Before the apply the state will be: + // Group A without users. + // Users 1, 2 without a group association. + // Group B will have users 3, 4, 5 from their creation. + // There is a group memberships that will place users 1, 2 into Group A. + // There is a group rule that will assign all group B users to Group A. + // After the apply: + // Group A has users 1, 2 by okta_group_memberships resource. + // The rule that okta_group_memberships.test_a_direct + // describes has been run at Okta associating users 3, 4, and 5 + // with Group A. + // Upon the next plan: + // The state of okta_group_memberships.test_a_direct is unchanged (expect + // empty plan) as no users it is concerned with have been removed from the + // group even if other users have been added to the group. + ExpectNonEmptyPlan: false, + Config: testOktaGroupMembershipsConfig(false), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("okta_group_memberships.test_a_direct", "users.#", "2"), + resource.TestCheckResourceAttr("data.okta_group.test_a", "users.#", "5"), + resource.TestCheckResourceAttr("data.okta_group.test_b", "users.#", "3"), + ), + }, + }, + }) +} + +// TestAccResourceOktaGroupMemberships_TrackAllUsersBehavior +// https://github.com/okta/terraform-provider-okta/issues/1094 +// https://github.com/okta/terraform-provider-okta/issues/1119 +// https://github.com/okta/terraform-provider-okta/issues/1149 +// https://github.com/okta/terraform-provider-okta/issues/1155 +func TestAccResourceOktaGroupMemberships_TrackAllUsersBehavior(t *testing.T) { resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProviderFactories: testAccProvidersFactories, @@ -83,24 +126,28 @@ func TestAccOktaGroupMembershipsIssue1094(t *testing.T) { // with Group A. // Upon the next plan: // The state of okta_group_memberships.test_a_direct - // will appear to have drifed from having only two - // users to five users becuase + // will appear to have drifted (expect non empty plan) from having + // only two users to five users becuase // okta_group_rule.group_b_to_a_rule will have run and // associated the three users from Group B to aslo be in // Group A. ExpectNonEmptyPlan: true, - - Config: configIssue1094, + // Even with a read delay of 5 seconds it can take awhile for group rules to fire in turn causing this test to fail. + Config: testOktaGroupMembershipsConfig(true), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("data.okta_group.test_b", "users.#", "3"), - resource.TestCheckResourceAttr("data.okta_group.test_a", "users.#", "5"), + resource.TestCheckResourceAttr("okta_group_memberships.test_a_direct", "users.#", "2"), ), }, }, }) } -var configIssue1094 = ` +// testOktaGroupMembershipsConfig Creat a config that has 5 users all assigned +// to the same group. Two users are assigned by okta_group_memberships and three +// are assigned explicitely. trackAllUsers is a flag to add `track_all_users = +// true` to the okta_group_memberships resource. +func testOktaGroupMembershipsConfig(trackAllUsers bool) string { + prepend := ` # Given group a, b resource "okta_group" "test_a" { name = "TestACC Group A" @@ -156,14 +203,16 @@ resource "okta_user" "test_5" { } # Group A should have users 1, 2 assigned via okta_group_memberships resource "okta_group_memberships" "test_a_direct" { +` + var clause string + if trackAllUsers { + clause = " track_all_users = true\n" + } + + append := ` group_id = okta_group.test_a.id users = [okta_user.test_1.id, okta_user.test_2.id] depends_on = [okta_user.test_1, okta_user.test_2, okta_group.test_a] - - # Ignore changes on users if group members will be changed outside of it - # lifecycle { - # ignore_changes = [users] - # } } # Group A should have users 3, 4, 5 assigned via okta_group_rule resource "okta_group_rule" "group_b_to_a_rule" { @@ -172,7 +221,6 @@ resource "okta_group_rule" "group_b_to_a_rule" { group_assignments = [okta_group.test_a.id] expression_type = "urn:okta:expression:1.0" expression_value = "isMemberOfAnyGroup(\"${okta_group.test_b.id}\")" - remove_assigned_users = true depends_on = [okta_user.test_3, okta_user.test_4, okta_user.test_5, okta_group.test_a, okta_group.test_b, okta_group_memberships.test_a_direct] } # Use a data source to read back in the state of each gorup for testing @@ -191,6 +239,9 @@ data "okta_group" "test_b" { } ` + return fmt.Sprintf("%s%s%s", prepend, clause, append) +} + // TestAccOktaGroupMembershipsIssue1119 addresses https://github.com/okta/terraform-provider-okta/issues/1119 func TestAccOktaGroupMembershipsIssue1119(t *testing.T) { configUsers := "" diff --git a/website/docs/r/group_memberships.html.markdown b/website/docs/r/group_memberships.html.markdown index aeaae0dea..18b0b44cb 100644 --- a/website/docs/r/group_memberships.html.markdown +++ b/website/docs/r/group_memberships.html.markdown @@ -17,10 +17,12 @@ call, for better API resource usage. Effectively this is the same as using the users. If you need a relationship of a single user to many groups, please use the `okta_user_group_memberships` resource. -**Important**: When the group memberships resource is used in an environment -where other resources or services can add users to the group it will make this -resource appear to drift. If that is the case make use of a lifecycle ignore for -the `users` argument to avoid conflicts in desired state. +**Important**: The default behavior of the resource is to only maintain the +state of user ids that are assigned it. This behavior will signal drift only if +those users stop being part of the group. If the desired behavior is track all +users that are added/removed from the group make use of the `track_all_users` +argument with this resource. + ## Example Usage @@ -36,10 +38,6 @@ resource "okta_group_memberships" "test" { okta_user.test1.id, okta_user.test2.id, ] - - # lifecycle { - # ignore_changes = [users] - # } } ``` @@ -49,6 +47,7 @@ The following arguments are supported: - `group_id` - (Required) Okta group ID. - `users` - (Required) The list of Okta user IDs which the group should have membership managed for. +- `track_all_users` - (Optional) The resource will concern itself with all users added/deleted to the group; even those managed outside of the resource. ## Attributes Reference