Repository: GoogleCloudPlatform/gcp-service-broker (go)

Issue:
Unbind does not delete related IAM policy
### Annotation
**I'm not sure whether it's worth fixing this issue since it does not affect the functionality and the upcoming brokerpak-feature solve this issue by using terraform.**

I more or less just wanted to document it.

### Issue
When binding a service to an app (e.g. CloudSQL), the service broker creates a service account and an IAM policy with the requested role and member.

By unbinding the service, the service broker deletes the service account but leaves the IAM policy behind. This - over time - causes many orphaned IAM policies without any members.

Maybe it does make sense to inform the users to manually delete all orphaned IAM policies when upgrading to the brokerpak?


Pull request:
Cleanup IAM policy when deleting a service account
This PR fixes #474.

The IAM policy won't automatically be cleaned up when deleting a service
account. It is necessary to manually remove the service account from all
its role memberships. Otherwise, all created role bindings will remain
existent in the policy, leading to many orphaned bindings.

*Since I'm relatively new to programming in golang:*
I had problems finding an easy solution on how to avoid the code duplication in the methods `revokeRolesFromAccount` and `grantRoleToAccount`.
I would appreciate any hint on how I could/should (or should not?) resolve this! 🙂

Patch:
diff --git a/pkg/providers/builtin/account_managers/iam.go b/pkg/providers/builtin/account_managers/iam.go
index 8cfa91c6..e30290f8 100644
--- a/pkg/providers/builtin/account_managers/iam.go
+++ b/pkg/providers/builtin/account_managers/iam.go
@@ -51,3 +51,17 @@ func rolesToMembersMap(bindings []*cloudresourcemanager.Binding) map[string]util
 
 	return bm
 }
+
+// Remove a member from all role bindings to clean up the iam policy before deleting a service account.
+func removeMemberFromBindings(bindings []*cloudresourcemanager.Binding, memberToRemove string) []*cloudresourcemanager.Binding {
+	for _, binding := range bindings {
+		for i, member := range binding.Members {
+			if member == memberToRemove {
+				binding.Members = append(binding.Members[:i], binding.Members[i+1:]...)
+				break
+			}
+		}
+	}
+
+	return bindings
+}
diff --git a/pkg/providers/builtin/account_managers/service_account_manager.go b/pkg/providers/builtin/account_managers/service_account_manager.go
index 5fed9155..3c297adc 100644
--- a/pkg/providers/builtin/account_managers/service_account_manager.go
+++ b/pkg/providers/builtin/account_managers/service_account_manager.go
@@ -104,13 +104,18 @@ func (sam *ServiceAccountManager) DeleteCredentials(ctx context.Context, binding
 		return fmt.Errorf("Error creating IAM service: %s", err)
 	}
 
+	// Clean up account permissions
+	if err := sam.revokeRolesFromAccount(ctx, saCreds.Email); err != nil {
+		return fmt.Errorf("Error deleting role bindings from service account: %s", err)
+	}
+
 	resourceName := projectResourcePrefix + sam.ProjectId + "/serviceAccounts/" + saCreds.UniqueId
 	if _, err := iam.NewProjectsServiceAccountsService(iamService).Delete(resourceName).Do(); err != nil {
 		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == http.StatusNotFound {
 			return nil
 		}
 
-		return fmt.Errorf("error deleting service account: %s", err)
+		return fmt.Errorf("Error deleting service account: %s", err)
 	}
 
 	return nil
@@ -185,6 +190,41 @@ func (sam *ServiceAccountManager) grantRoleToAccount(ctx context.Context, role s
 	return err
 }
 
+func (sam *ServiceAccountManager) revokeRolesFromAccount(ctx context.Context, email string) error {
+	client := sam.HttpConfig.Client(ctx)
+
+	cloudresService, err := cloudres.New(client)
+	if err != nil {
+		return fmt.Errorf("Error creating new cloud resource management service: %s", err)
+	}
+
+	for attempt := 0; attempt < 3; attempt++ {
+		currPolicy, err := cloudresService.Projects.GetIamPolicy(sam.ProjectId, &cloudres.GetIamPolicyRequest{}).Do()
+		if err != nil {
+			return fmt.Errorf("Error getting current project iam policy: %s", err)
+		}
+
+		currPolicy.Bindings = mergeBindings(removeMemberFromBindings(currPolicy.Bindings, saResourcePrefix+email))
+
+		newPolicyRequest := cloudres.SetIamPolicyRequest{
+			Policy: currPolicy,
+		}
+
+		_, err = cloudresService.Projects.SetIamPolicy(sam.ProjectId, &newPolicyRequest).Do()
+		if err == nil {
+			return nil
+		}
+
+		if isConflictError(err) {
+			time.Sleep(5 * time.Second)
+			continue
+		} else {
+			return fmt.Errorf("Error updating iam policy: %s", err)
+		}
+	}
+	return err
+}
+
 func isConflictError(err error) bool {
 	gerr, ok := err.(*googleapi.Error)
 	return ok && gerr != nil && gerr.Code == 409


Test patch:
diff --git a/pkg/providers/builtin/account_managers/iam_test.go b/pkg/providers/builtin/account_managers/iam_test.go
index 5a8295cd..d6625ad4 100644
--- a/pkg/providers/builtin/account_managers/iam_test.go
+++ b/pkg/providers/builtin/account_managers/iam_test.go
@@ -79,3 +79,46 @@ func TestIamMergeBindings(t *testing.T) {
 		}
 	}
 }
+
+func TestIamRemoveMemberFromBindings(t *testing.T) {
+	table := map[string]struct {
+		input  []*cloudresourcemanager.Binding
+		expect map[string][]string
+	}{
+		"remove member from one role": {
+			input: []*cloudresourcemanager.Binding{
+				{Role: "role-1", Members: []string{"m1", "m2"}},
+			},
+			expect: map[string][]string{
+				"role-1": {"m2"},
+			},
+		},
+		"remove member from multiple roles": {
+			input: []*cloudresourcemanager.Binding{
+				{Role: "role-1", Members: []string{"m1", "m2"}},
+				{Role: "role-2", Members: []string{"m1", "m3"}},
+			},
+			expect: map[string][]string{
+				"role-1": {"m2"},
+				"role-2": {"m3"},
+			},
+		},
+	}
+
+	for tn, tc := range table {
+		updatedBindings := removeMemberFromBindings(tc.input, "m1")
+
+		if len(updatedBindings) != len(tc.expect) {
+			t.Errorf("%s) expected %d bindings, got: %d", tn, len(tc.expect), len(updatedBindings))
+		}
+
+		for _, binding := range updatedBindings {
+			expset := utils.NewStringSet(tc.expect[binding.Role]...)
+			gotset := utils.NewStringSet(binding.Members...)
+
+			if !expset.Equals(gotset) {
+				t.Errorf("%s) expected %v members in %s role, got %v", tn, expset.ToSlice(), binding.Role, gotset.ToSlice())
+			}
+		}
+	}
+}