Fix inconsistencies in how IAM policies are stored in state (#6466) (#…

…12652)

* Add tests for existing `dataSourceGoogleIamPolicyRead` function behaviour

* Add red (non-passing) tests for new `dataSourceGoogleIamPolicyRead` behaviour

* Update tests to check both numbers of bindings from config vs in policy_data output

This is necessary because policy_data will need to consolidate bindings, and state for binding{} blocks needs to keep data separate

* Update function to pass uncommented tests

* Refactor test setup

* Change structure of test input data to minimise processing in the test function + add and update test cases

Need to check if test assumptions like `bindings on the same role with different conditions, with the same title, are next sorted by condition expression` is correct or not

* WIP: Update sorting of bindings to include: role, condition presence, condition fields.

* Formatting changes to make .erb match output .go file

* Update tests to match the sorting behaviour of the API, update sorting function to pass tests

* Refactor existing tests for clarity

* Add tests for deduplication and complex sorting scenario

* Fix sorting bug where 'lesser' roles is incorrectly sorted to a later index in a list if it has a condition

* Refactor `dataSourceGoogleIamPolicyRead` to reduce looping and pull sorting function out into a named function

* Make binding map use human-readable keys

Signed-off-by: Modular Magician <magic-modules@google.com>

Signed-off-by: Modular Magician <magic-modules@google.com>

.changelog/6466.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+iam: fixed diffs between `policy_data` from `google_iam_policy` data source and policy data in API responses
+```

google/data_source_google_iam_policy.go

@@ -16,14 +16,14 @@ import (
 // to express a Google Cloud IAM policy in a data resource. This is an example
 // of how the schema would be used in a config:
 //
-//	data "google_iam_policy" "admin" {
-//	  binding {
-//	    role = "roles/storage.objectViewer"
-//	    members = [
-//	      "user:evanbrown@google.com",
-//	    ]
-//	  }
-//	}
+// data "google_iam_policy" "admin" {
+//   binding {
+//     role = "roles/storage.objectViewer"
+//     members = [
+//       "user:evanbrown@google.com",
+//     ]
+//   }
+// }
 func dataSourceGoogleIamPolicy() *schema.Resource {
 	return &schema.Resource{
 		Read: dataSourceGoogleIamPolicyRead,
@@ -119,30 +119,52 @@ func dataSourceGoogleIamPolicyRead(d *schema.ResourceData, meta interface{}) err
 	bset := d.Get("binding").(*schema.Set)
 	aset := d.Get("audit_config").(*schema.Set)
 
-	// All binding{} blocks will be converted and stored in an array
-	bindings = make([]*cloudresourcemanager.Binding, bset.Len())
-	policy.Bindings = bindings
-
 	// Convert each config binding into a cloudresourcemanager.Binding
-	for i, v := range bset.List() {
+	// and merge member lists of equivalent binding{} blocks from the config provided by the user
+	bindingMap := map[string]*cloudresourcemanager.Binding{}
+	for _, v := range bset.List() {
 		binding := v.(map[string]interface{})
 		members := convertStringSet(binding["members"].(*schema.Set))
 		condition := expandIamCondition(binding["condition"])
 
-		// Sort members to get simpler diffs as it's what the API does
-		sort.Strings(members)
+		// Map keys are used to identify binding{} blocks that are identical except for the member lists
+		key := binding["role"].(string)
+		if condition != nil {
+			key += fmt.Sprintf("-[%s]-[%s]-[%s]-[%s]", condition.Expression, condition.Title, condition.Description, condition.Location)
+		}
 
-		policy.Bindings[i] = &cloudresourcemanager.Binding{
-			Role:      binding["role"].(string),
-			Members:   members,
-			Condition: condition,
+		if val, ok := bindingMap[key]; ok {
+			// Add members to existing cloudresourcemanager.Binding in the map
+			m := append(val.Members, members...)
+			binding := bindingMap[key]
+			binding.Members = m
+			bindingMap[key] = binding
+		} else {
+			// Add new cloudresourcemanager.Binding to the map
+			bindingMap[key] = &cloudresourcemanager.Binding{
+				Role:      binding["role"].(string),
+				Members:   members,
+				Condition: condition,
+			}
 		}
 	}
 
-	// Sort bindings by their role name to get simpler diffs as it's what the API does
-	sort.Slice(bindings, func(i, j int) bool {
-		return bindings[i].Role < bindings[j].Role
-	})
+	// All binding{} blocks, post conversion to cloudresourcemanager.Binding and combining of member lists, are stored in an array
+	bindings = []*cloudresourcemanager.Binding{}
+	for _, v := range bindingMap {
+		v := v
+		bindings = append(bindings, v)
+	}
+	policy.Bindings = bindings
+
+	// Sort bindings within the list to get simpler diffs, as it's what the API does
+	// Sorting is based on the binding's role + condition fields
+	sort.Slice(policy.Bindings, iamPolicyBindingsLessFunction(policy))
+
+	// Sort members within each binding in the list to get simpler diffs, as it's what the API does
+	for i := 0; i < len(policy.Bindings); i++ {
+		sort.Strings(policy.Bindings[i].Members)
+	}
 
 	// Convert each audit_config into a cloudresourcemanager.AuditConfig
 	policy.AuditConfigs = expandAuditConfig(aset)
@@ -185,3 +207,54 @@ func expandAuditConfig(set *schema.Set) []*cloudresourcemanager.AuditConfig {
 	}
 	return auditConfigs
 }
+
+func iamPolicyBindingsLessFunction(policy cloudresourcemanager.Policy) func(i, j int) bool {
+
+	return func(i, j int) bool {
+		// Sort bindings by role, if they're not the same
+		sameRole := policy.Bindings[i].Role == policy.Bindings[j].Role
+		if !sameRole {
+			return policy.Bindings[i].Role < policy.Bindings[j].Role
+		}
+
+		iConditionOk := policy.Bindings[i].Condition != nil
+		jConditionOk := policy.Bindings[j].Condition != nil
+
+		// If both bindings lack conditions we cannot sort them further
+		if !iConditionOk && !jConditionOk {
+			return false
+		}
+
+		// Sort by presence of a condition on only one of the two bindings
+		if !iConditionOk && jConditionOk {
+			return true
+		}
+		if iConditionOk && !jConditionOk {
+			return false
+		}
+
+		// At this point both bindings have conditions
+
+		sameExpression := policy.Bindings[i].Condition.Expression == policy.Bindings[j].Condition.Expression
+		sameTitle := policy.Bindings[i].Condition.Title == policy.Bindings[j].Condition.Title
+		sameDescription := policy.Bindings[i].Condition.Description == policy.Bindings[j].Condition.Description
+
+		// Don't sort if conditions are the same
+		if sameExpression && sameTitle && sameDescription {
+			return false
+		}
+
+		// Sort by both bindings' conditions' expressions, if they're not equivalent
+		if !sameExpression {
+			return policy.Bindings[i].Condition.Expression < policy.Bindings[j].Condition.Expression
+		}
+
+		// Sort by both bindings' conditions' titles, if they're not equivalent
+		if !sameTitle {
+			return policy.Bindings[i].Condition.Title < policy.Bindings[j].Condition.Title
+		}
+
+		// Comparing conditions' descriptions is the last available way to sort
+		return policy.Bindings[i].Condition.Description < policy.Bindings[j].Condition.Description
+	}
+}