Fix items in nested lists not always being matched

When a policy is looking for a field in a list, which is itself nested
in an object in a list, and when there are multiple items in these lists
on the cluster, the controller was not always matching things correctly.
This could result in the policy's compliance depending on the order of
the items in the nested list on the cluster. (The new test case may be
more clear than this problem description.)

This situation is difficult to reproduce: because of the way some lists
were compared before, fields that don't seem like they should affect the
result can change the outcome.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

controllers/configurationpolicy_controller_test.go

@@ -18,6 +18,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/yaml"
 
 	policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
 )
@@ -470,6 +471,91 @@ func TestCheckListsMatch(t *testing.T) {
 	assert.False(t, checkListsMatch(oneBigOneSmall, twoFullItems))
 }
 
+func TestNestedUnsortedLists(t *testing.T) {
+	objDefYaml := `
+kind: FakeOperator
+status:
+  refs:
+    - conditions:
+        - status: "False"
+          type: CatalogSourcesUnhealthy
+      kind: Subscription
+`
+
+	orderOneYaml := `
+kind: FakeOperator
+status:
+  refs:
+    - apiVersion: operators.coreos.com/v1alpha1
+      conditions:
+      - lastTransitionTime: "2023-10-22T06:49:54Z"
+    - apiVersion: operators.coreos.com/v1alpha1
+      conditions:
+        - lastTransitionTime: "2023-10-22T06:40:14Z"
+          status: "False"
+          type: CatalogSourcesUnhealthy
+        - status: "False"
+          type: BundleUnpacking
+      kind: Subscription
+`
+
+	orderTwoYaml := `
+kind: FakeOperator
+status:
+  refs:
+    - apiVersion: operators.coreos.com/v1alpha1
+      conditions:
+      - lastTransitionTime: "2023-10-22T06:49:54Z"
+    - apiVersion: operators.coreos.com/v1alpha1
+      conditions:
+        - status: "False"
+          type: BundleUnpacking
+        - lastTransitionTime: "2023-10-22T06:40:14Z"
+          status: "False"
+          type: CatalogSourcesUnhealthy
+      kind: Subscription
+`
+
+	policyObjDef := make(map[string]interface{})
+
+	err := yaml.UnmarshalStrict([]byte(objDefYaml), &policyObjDef)
+	if err != nil {
+		t.Error(err)
+	}
+
+	orderOneObj := make(map[string]interface{})
+
+	err = yaml.UnmarshalStrict([]byte(orderOneYaml), &orderOneObj)
+	if err != nil {
+		t.Error(err)
+	}
+
+	orderTwoObj := make(map[string]interface{})
+
+	err = yaml.UnmarshalStrict([]byte(orderTwoYaml), &orderTwoObj)
+	if err != nil {
+		t.Error(err)
+	}
+
+	desiredObj := unstructured.Unstructured{Object: policyObjDef}
+	existingObjOrderOne := unstructured.Unstructured{Object: orderOneObj}
+	existingObjOrderTwo := unstructured.Unstructured{Object: orderTwoObj}
+
+	errormsg, updateNeeded, _, _ := handleSingleKey("status", desiredObj, &existingObjOrderOne, "musthave")
+	if len(errormsg) != 0 {
+		t.Error("Got unexpected error message", errormsg)
+	}
+
+	assert.False(t, updateNeeded)
+
+	errormsg, updateNeeded, _, _ = handleSingleKey("status", desiredObj, &existingObjOrderTwo, "musthave")
+	if len(errormsg) != 0 {
+		t.Error("Got unexpected error message", errormsg)
+	}
+
+	assert.False(t, updateNeeded)
+}
+
 func TestAddRelatedObject(t *testing.T) {
 	compliant := true
 	rsrc := policyv1.SchemeBuilder.GroupVersion.WithResource("ConfigurationPolicy")

controllers/configurationpolicy_utils.go

@@ -226,6 +226,34 @@ func checkFieldsWithSort(mergedObj map[string]interface{}, oldObj map[string]int
 	return true
 }
 
+// sortAndSprint sorts any lists in the input, and formats the resulting object as a string
+func sortAndSprint(item interface{}) string {
+	switch item := item.(type) {
+	case map[string]interface{}:
+		sorted := make(map[string]string, len(item))
+
+		for key, val := range item {
+			sorted[key] = sortAndSprint(val)
+		}
+
+		return fmt.Sprintf("%v", sorted)
+	case []interface{}:
+		sorted := make([]string, len(item))
+
+		for i, val := range item {
+			sorted[i] = sortAndSprint(val)
+		}
+
+		sort.Slice(sorted, func(x, y int) bool {
+			return sorted[x] < sorted[y]
+		})
+
+		return fmt.Sprintf("%v", sorted)
+	default:
+		return fmt.Sprintf("%v", item)
+	}
+}
+
 // checkListsMatch is a generic list check that uses an arbitrary sort to ensure it is comparing the right values
 func checkListsMatch(oldVal []interface{}, mergedVal []interface{}) (m bool) {
 	if (oldVal == nil && mergedVal != nil) || (oldVal != nil && mergedVal == nil) {
@@ -241,10 +269,10 @@ func checkListsMatch(oldVal []interface{}, mergedVal []interface{}) (m bool) {
 	mVal := append([]interface{}{}, mergedVal...)
 
 	sort.Slice(oVal, func(i, j int) bool {
-		return fmt.Sprintf("%v", oVal[i]) < fmt.Sprintf("%v", oVal[j])
+		return sortAndSprint(oVal[i]) < sortAndSprint(oVal[j])
 	})
 	sort.Slice(mVal, func(x, y int) bool {
-		return fmt.Sprintf("%v", mVal[x]) < fmt.Sprintf("%v", mVal[y])
+		return sortAndSprint(mVal[x]) < sortAndSprint(mVal[y])
 	})
 
 	for idx, oNestedVal := range oVal {