Rename and document functions used during merging

These explanations especially try to cover the new `missingKey` return
value which has been added a bunch of places. These functions might also
benefit from other refactors. In particular, the doc'd returns of
`handleKeys` make it clear that there is some overlap of
`throwSpecViolation` and `statusMismatch`.

Signed-off-by: Justin Kulikauskas <[email protected]>

Author: JustinKuli

controllers/configurationpolicy_controller.go

@@ -2821,8 +2821,15 @@ type countedVal struct {
 	count int
 }
 
-// mergeArrays is a helper function that takes a list from the existing object and merges in all the data that is
-// different in the template.
+// mergeArrays performs a deep merge operation, combining the given lists. It
+// specially considers items with "names", which it will merge from each list.
+// It preserves duplicates that are already in `existingArr`, but does not
+// introduce *new* duplicate items if they are in `desiredArr`. When merging
+// items or nested items which are maps, the `zeroValueEqualsNil` parameter
+// determines how to handle certain "zero value" cases (see `deeplyEquivalent`).
+//
+// It returns the merged list, and indicates whether any of the nested maps were
+// considered equivalent due to "zero values".
 func mergeArrays(
 	desiredArr []interface{}, existingArr []interface{}, ctype policyv1.ComplianceType, zeroValueEqualsNil bool,
 ) (result []interface{}, missingKey bool) {
@@ -2892,12 +2899,12 @@ func mergeArrays(
 				}
 
 				// use map compare helper function to check equality on lists of maps
-				mergedObj, missingKey, _ = compareSpecs(val1, val2, ctype, zeroValueEqualsNil)
+				mergedObj, missingKey, _ = mergeMaps(val1, val2, ctype, zeroValueEqualsNil)
 			default:
 				mergedObj = val1
 			}
 			// if a match is found, this field is already in the template, so we can skip it in future checks
-			equal, missing := equalObjWithSort(mergedObj, val2, zeroValueEqualsNil)
+			equal, missing := deeplyEquivalent(mergedObj, val2, zeroValueEqualsNil)
 			missingKey = missingKey || missing
 
 			if sameNamedObjects || equal {
@@ -2925,9 +2932,15 @@ func mergeArrays(
 	return desiredArr, missingKey
 }
 
-// compareSpecs is a wrapper function that creates a merged map for mustHave
-// and returns the template map for mustonlyhave
-func compareSpecs(
+// mergeMaps performs a deep merge operation, combining data from `oldSpec` and
+// `newSpec`, prioritizing the data in `newSpec`. It specially handles lists
+// (see `mergeArrays`) and whether "zero values" should be considered equivalent
+// if only found in one of the inputs (see `deeplyEquivalent`). The "zero value"
+// handling can be configured with the `zeroValueEqualsNil` parameter.
+//
+// It returns the merged object, and indicates if it introduced data for "zero
+// values" from `newSpec` that were not present in `oldSpec`.
+func mergeMaps(
 	newSpec, oldSpec map[string]interface{}, ctype policyv1.ComplianceType, zeroValueEqualsNil bool,
 ) (updatedSpec map[string]interface{}, missingKey bool, err error) {
 	if ctype.IsMustOnlyHave() {
@@ -2939,8 +2952,15 @@ func compareSpecs(
 	return merged.(map[string]interface{}), missing, err
 }
 
-// handleSingleKey checks whether a key/value pair in an object template matches with that in the existing
-// resource on the cluster
+// handleSingleKey compares and merges a single field in the given objects. It
+// specially considers lists (allowing for different orderings) and whether some
+// "zero values" only found in one of the inputs causes the inputs to not be
+// considered equivalent (see `deeplyEquivalent`). The `zeroValueEqualsNil`
+// parameter can be used to tweak that situation slightly.
+//
+// It returns whether an update is needed, a merged version of the field, whether
+// the field was skipped entirely, and whether any "zero values" were merged in
+// which were not in the `existingObj`.
 func handleSingleKey(
 	key string,
 	desiredObj *unstructured.Unstructured,
@@ -2988,7 +3008,7 @@ func handleSingleKey(
 	case map[string]interface{}:
 		switch existingValue := existingValue.(type) {
 		case map[string]interface{}:
-			mergedValue, missing, err = compareSpecs(desiredValue, existingValue, complianceType, zeroValueEqualsNil)
+			mergedValue, missing, err = mergeMaps(desiredValue, existingValue, complianceType, zeroValueEqualsNil)
 			missingKey = missingKey || missing
 		case nil:
 			mergedValue = desiredValue
@@ -3049,7 +3069,7 @@ func handleSingleKey(
 	}
 
 	// sort objects before checking equality to ensure they're in the same order
-	equal, missing := equalObjWithSort(mergedValue, existingValue, zeroValueEqualsNil)
+	equal, missing := deeplyEquivalent(mergedValue, existingValue, zeroValueEqualsNil)
 	missingKey = missingKey || missing
 
 	if !equal {
@@ -3382,6 +3402,14 @@ func handleDiff(
 // handleKeys goes through all of the fields in the desired object and checks if the existing object
 // matches. When a field is a map or slice, the value in the existing object will be updated with
 // the result of merging its current value with the desired value.
+//
+// It returns:
+//   - throwSpecViolation: true if the status has a discrepancy from what is desired
+//   - message: information when an error occurs
+//   - updateNeeded: true if the object should be updated on the cluster to be enforced
+//   - statusMismatch: true if the status has a discrepancy from what is desired
+//   - missingKey: true if some nested fields don't strictly match between the existingObj and
+//     desiredObj, but the difference is just a "zero" value or omission.
 func handleKeys(
 	desiredObj *unstructured.Unstructured,
 	existingObj *unstructured.Unstructured,

controllers/configurationpolicy_controller_test.go

@@ -80,7 +80,7 @@ func TestReconcile(t *testing.T) {
 	t.Log(res)
 }
 
-func TestCompareSpecs(t *testing.T) {
+func TestMergeMaps(t *testing.T) {
 	spec1 := map[string]interface{}{
 		"containers": map[string]string{
 			"image": "nginx1.7.9",
@@ -94,7 +94,7 @@ func TestCompareSpecs(t *testing.T) {
 		},
 	}
 
-	merged, _, err := compareSpecs(spec1, spec2, "mustonlyhave", true)
+	merged, _, err := mergeMaps(spec1, spec2, "mustonlyhave", true)
 	if err != nil {
 		t.Fatalf("compareSpecs: (%v)", err)
 	}
@@ -126,7 +126,7 @@ func TestCompareSpecs(t *testing.T) {
 		},
 	}
 
-	merged, _, err = compareSpecs(spec1, spec2, "musthave", true)
+	merged, _, err = mergeMaps(spec1, spec2, "musthave", true)
 	if err != nil {
 		t.Fatalf("compareSpecs: (%v)", err)
 	}
@@ -293,7 +293,7 @@ func TestMergeArraysMustHave(t *testing.T) {
 
 			actualMergedList, _ := mergeArrays(test.desiredList, test.currentList, "musthave", true)
 			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
-			check, _ := checkListsMatch(test.expectedList, actualMergedList)
+			check, _ := checkListsAreEquivalent(test.expectedList, actualMergedList)
 			assert.True(t, check)
 		})
 	}
@@ -378,13 +378,13 @@ func TestMergeArraysMustOnlyHave(t *testing.T) {
 
 			actualMergedList, _ := mergeArrays(test.desiredList, test.currentList, "mustonlyhave", true)
 			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
-			check, _ := checkListsMatch(test.expectedList, actualMergedList)
+			check, _ := checkListsAreEquivalent(test.expectedList, actualMergedList)
 			assert.True(t, check)
 		})
 	}
 }
 
-func TestCheckListsMatch(t *testing.T) {
+func TestCheckListsAreEquivalent(t *testing.T) {
 	twoFullItems := []interface{}{
 		map[string]interface{}{
 			"a": "apple",
@@ -396,7 +396,7 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ := checkListsMatch(twoFullItems, twoFullItems)
+	check, _ := checkListsAreEquivalent(twoFullItems, twoFullItems)
 	assert.True(t, check)
 
 	twoFullItemsDifferentOrder := []interface{}{
@@ -410,9 +410,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, twoFullItemsDifferentOrder)
+	check, _ = checkListsAreEquivalent(twoFullItems, twoFullItemsDifferentOrder)
 	assert.True(t, check)
-	check, _ = checkListsMatch(twoFullItemsDifferentOrder, twoFullItems)
+	check, _ = checkListsAreEquivalent(twoFullItemsDifferentOrder, twoFullItems)
 	assert.True(t, check)
 
 	oneFullItem := []interface{}{
@@ -422,9 +422,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, oneFullItem)
+	check, _ = checkListsAreEquivalent(twoFullItems, oneFullItem)
 	assert.False(t, check)
-	check, _ = checkListsMatch(oneFullItem, twoFullItems)
+	check, _ = checkListsAreEquivalent(oneFullItem, twoFullItems)
 	assert.False(t, check)
 
 	oneSmallItem := []interface{}{
@@ -433,9 +433,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, oneSmallItem)
+	check, _ = checkListsAreEquivalent(twoFullItems, oneSmallItem)
 	assert.False(t, check)
-	check, _ = checkListsMatch(oneSmallItem, twoFullItems)
+	check, _ = checkListsAreEquivalent(oneSmallItem, twoFullItems)
 	assert.False(t, check)
 
 	twoSmallItems := []interface{}{
@@ -447,9 +447,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, twoSmallItems)
+	check, _ = checkListsAreEquivalent(twoFullItems, twoSmallItems)
 	assert.False(t, check)
-	check, _ = checkListsMatch(twoSmallItems, twoFullItems)
+	check, _ = checkListsAreEquivalent(twoSmallItems, twoFullItems)
 	assert.False(t, check)
 
 	oneSmallOneBig := []interface{}{
@@ -462,9 +462,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, oneSmallOneBig)
+	check, _ = checkListsAreEquivalent(twoFullItems, oneSmallOneBig)
 	assert.False(t, check)
-	check, _ = checkListsMatch(oneSmallOneBig, twoFullItems)
+	check, _ = checkListsAreEquivalent(oneSmallOneBig, twoFullItems)
 	assert.False(t, check)
 
 	oneBigOneSmall := []interface{}{
@@ -477,9 +477,9 @@ func TestCheckListsMatch(t *testing.T) {
 		},
 	}
 
-	check, _ = checkListsMatch(twoFullItems, oneBigOneSmall)
+	check, _ = checkListsAreEquivalent(twoFullItems, oneBigOneSmall)
 	assert.False(t, check)
-	check, _ = checkListsMatch(oneBigOneSmall, twoFullItems)
+	check, _ = checkListsAreEquivalent(oneBigOneSmall, twoFullItems)
 	assert.False(t, check)
 }
 
@@ -508,7 +508,7 @@ func TestCheckListsMatchDiffMapLength(t *testing.T) {
 		},
 	}
 
-	check, _ := checkListsMatch(existingObject, mergedObject)
+	check, _ := checkListsAreEquivalent(existingObject, mergedObject)
 	assert.True(t, check)
 }
 

controllers/configurationpolicy_utils.go

@@ -146,13 +146,18 @@ func addOrUpdateRelatedObject(
 	return list
 }
 
-// equalObjWithSort is a wrapper function that calls the correct function to check equality depending on what
-// type the objects to compare are
-func equalObjWithSort(mergedObj interface{}, oldObj interface{}, zeroValueEqualsNil bool) (areEqual, missingKey bool) {
+// deeplyEquivalent deeply compares the first two inputs, considering them
+// equivalent even if they have lists which are in different orders. When the
+// `zeroValueEqualsNil` parameter is true, it will allow nested maps to have
+// omitted fields that are "zero values" to be considered equivalent.
+//
+// It returns whether the first two inputs are equivalent, and if any of the
+// nested maps were considered equivalent due to "zero values".
+func deeplyEquivalent(mergedObj interface{}, oldObj interface{}, zeroValueEqualsNil bool) (areEqual, missingKey bool) {
 	switch mergedObj := mergedObj.(type) {
 	case map[string]interface{}:
 		if oldObjMap, ok := oldObj.(map[string]interface{}); ok {
-			return checkFieldsWithSort(mergedObj, oldObjMap, zeroValueEqualsNil)
+			return checkFieldsAreEquivalent(mergedObj, oldObjMap, zeroValueEqualsNil)
 		}
 		// this includes the case where oldObj is nil
 		return false, false
@@ -162,7 +167,7 @@ func equalObjWithSort(mergedObj interface{}, oldObj interface{}, zeroValueEquals
 		}
 
 		if oldObjList, ok := oldObj.([]interface{}); ok {
-			return checkListsMatch(mergedObj, oldObjList)
+			return checkListsAreEquivalent(mergedObj, oldObjList)
 		}
 
 		return false, false
@@ -189,9 +194,15 @@ func equalObjWithSort(mergedObj interface{}, oldObj interface{}, zeroValueEquals
 	}
 }
 
-// checkFieldsWithSort is a check for maps that uses an arbitrary sort to ensure it is
-// comparing the right values
-func checkFieldsWithSort(
+// checkFieldsAreEquivalent deeply compares two maps and determines whether they
+// contain equivalent fields. During the comparison, it specially considers fields
+// which may be omitted because they are "zero values" and lists which might have
+// different orderings. The `zeroValueEqualsNil` parameter may be used to disable
+// those "zero value" allowances.
+//
+// It returns whether the two maps are considered equivalent, and if there are
+// "extra" fields in `mergedObj` that are just "zero values".
+func checkFieldsAreEquivalent(
 	mergedObj map[string]interface{}, oldObj map[string]interface{}, zeroValueEqualsNil bool,
 ) (matches, missingKey bool) {
 	// needed to compare lists, since merge messes up the order
@@ -212,7 +223,7 @@ func checkFieldsWithSort(
 				return false, missingKey
 			}
 
-			match, missing := checkFieldsWithSort(mVal, oVal, zeroValueEqualsNil)
+			match, missing := checkFieldsAreEquivalent(mVal, oVal, zeroValueEqualsNil)
 			missingKey = missingKey || missing
 
 			if !match {
@@ -233,7 +244,7 @@ func checkFieldsWithSort(
 				return false, missingKey
 			}
 
-			match, miss := checkListsMatch(oVal, mVal)
+			match, miss := checkListsAreEquivalent(oVal, mVal)
 			missingKey = missingKey || miss
 
 			if !match {
@@ -311,8 +322,14 @@ func sortAndSprint(item interface{}) string {
 	}
 }
 
-// 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{}) (matches, missingKey bool) {
+// checkListsAreEquivalent deeply compares two lists and determines whether they
+// contain the same items, regardless of order. When comparing items which are
+// maps, it specially considers fields which may be omitted because they are
+// "zero values".
+//
+// It returns whether the two lists are considered equivalent, and if there are
+// "extra" fields in items of the `mergedVal` list which are just "zero values".
+func checkListsAreEquivalent(oldVal []interface{}, mergedVal []interface{}) (matches, missingKey bool) {
 	if (oldVal == nil && mergedVal != nil) || (oldVal != nil && mergedVal == nil) {
 		return false, false
 	}
@@ -337,7 +354,7 @@ func checkListsMatch(oldVal []interface{}, mergedVal []interface{}) (matches, mi
 		case map[string]interface{}:
 			// if list contains maps, recurse on those maps to check for a match
 			if mVal, ok := mVal[idx].(map[string]interface{}); ok {
-				match, miss := checkFieldsWithSort(mVal, oNestedVal, true)
+				match, miss := checkFieldsAreEquivalent(mVal, oNestedVal, true)
 				missingKey = missingKey || miss
 
 				if !match {