Verify with the API server if an empty map is equal to nil

If a policy specified an empty map but the object didn't return a value
for the map, it was assumed that API server was just not returning an
empty value.

This is true in most cases, however, if the underlying Go type of the
map is a pointer to a struct, an empty map may have a different meaning
than nil. One example is the `emptyDir` key in the
"configs.imageregistry.operator.openshift.io" resource.

This commit changes the local comparison logic from considering empty
maps being the same as a nil value. The controller then performs a dry
run update request to see if the API server returns an empty map or
omits the value entirely (i.e. seen as nil).

The result of the object comparison is now cached to not continuously
making dry run update requests on every policy evaluation.

Relates:
https://issues.redhat.com/browse/ACM-7810

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

controllers/configurationpolicy_controller_test.go

@@ -91,7 +91,7 @@ func TestCompareSpecs(t *testing.T) {
 		},
 	}
 
-	merged, err := compareSpecs(spec1, spec2, "mustonlyhave")
+	merged, err := compareSpecs(spec1, spec2, "mustonlyhave", true)
 	if err != nil {
 		t.Fatalf("compareSpecs: (%v)", err)
 	}
@@ -123,7 +123,7 @@ func TestCompareSpecs(t *testing.T) {
 		},
 	}
 
-	merged, err = compareSpecs(spec1, spec2, "musthave")
+	merged, err = compareSpecs(spec1, spec2, "musthave", true)
 	if err != nil {
 		t.Fatalf("compareSpecs: (%v)", err)
 	}
@@ -291,7 +291,7 @@ func TestMergeArraysMustHave(t *testing.T) {
 		t.Run(testName, func(t *testing.T) {
 			t.Parallel()
 
-			actualMergedList := mergeArrays(test.desiredList, test.currentList, "musthave")
+			actualMergedList := mergeArrays(test.desiredList, test.currentList, "musthave", true)
 			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
 			assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
 		})
@@ -378,7 +378,7 @@ func TestMergeArraysMustOnlyHave(t *testing.T) {
 		t.Run(testName, func(t *testing.T) {
 			t.Parallel()
 
-			actualMergedList := mergeArrays(test.desiredList, test.currentList, "mustonlyhave")
+			actualMergedList := mergeArrays(test.desiredList, test.currentList, "mustonlyhave", true)
 			assert.Equal(t, fmt.Sprintf("%+v", test.expectedList), fmt.Sprintf("%+v", actualMergedList))
 			assert.True(t, checkListsMatch(test.expectedList, actualMergedList))
 		})
@@ -569,14 +569,14 @@ status:
 	existingObjOrderOne := unstructured.Unstructured{Object: orderOneObj}
 	existingObjOrderTwo := unstructured.Unstructured{Object: orderTwoObj}
 
-	errormsg, updateNeeded, _, _ := handleSingleKey("status", desiredObj, &existingObjOrderOne, "musthave")
+	errormsg, updateNeeded, _, _ := handleSingleKey("status", desiredObj, &existingObjOrderOne, "musthave", true)
 	if len(errormsg) != 0 {
 		t.Error("Got unexpected error message", errormsg)
 	}
 
 	assert.False(t, updateNeeded)
 
-	errormsg, updateNeeded, _, _ = handleSingleKey("status", desiredObj, &existingObjOrderTwo, "musthave")
+	errormsg, updateNeeded, _, _ = handleSingleKey("status", desiredObj, &existingObjOrderTwo, "musthave", true)
 	if len(errormsg) != 0 {
 		t.Error("Got unexpected error message", errormsg)
 	}
@@ -1332,7 +1332,7 @@ func TestShouldHandleSingleKeyFalse(t *testing.T) {
 		unstruct.Object = test.input
 		unstructObj.Object = test.fromAPI
 		key := test.expectResult.key
-		_, update, _, skip = handleSingleKey(key, unstruct, &unstructObj, "musthave")
+		_, update, _, skip = handleSingleKey(key, unstruct, &unstructObj, "musthave", true)
 		assert.Equal(t, update, test.expectResult.expect)
 		assert.False(t, skip)
 	}

controllers/configurationpolicy_utils.go

@@ -106,11 +106,11 @@ func updateRelatedObjectsStatus(
 
 // 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{}) (areEqual bool) {
+func equalObjWithSort(mergedObj interface{}, oldObj interface{}, zeroValueEqualsNil bool) (areEqual bool) {
 	switch mergedObj := mergedObj.(type) {
 	case map[string]interface{}:
 		if oldObjMap, ok := oldObj.(map[string]interface{}); ok {
-			return checkFieldsWithSort(mergedObj, oldObjMap)
+			return checkFieldsWithSort(mergedObj, oldObjMap, zeroValueEqualsNil)
 		}
 		// this includes the case where oldObj is nil
 		return false
@@ -125,20 +125,22 @@ func equalObjWithSort(mergedObj interface{}, oldObj interface{}) (areEqual bool)
 
 		return false
 	default: // when mergedObj's type is string, int, bool, or nil
-		if oldObj == nil && mergedObj != nil {
-			// compare the zero value of mergedObj's type to mergedObj
-			ref := reflect.ValueOf(mergedObj)
-			zero := reflect.Zero(ref.Type()).Interface()
+		if zeroValueEqualsNil {
+			if oldObj == nil && mergedObj != nil {
+				// compare the zero value of mergedObj's type to mergedObj
+				ref := reflect.ValueOf(mergedObj)
+				zero := reflect.Zero(ref.Type()).Interface()
 
-			return fmt.Sprint(zero) == fmt.Sprint(mergedObj)
-		}
+				return fmt.Sprint(zero) == fmt.Sprint(mergedObj)
+			}
 
-		if mergedObj == nil && oldObj != nil {
-			// compare the zero value of oldObj's type to oldObj
-			ref := reflect.ValueOf(oldObj)
-			zero := reflect.Zero(ref.Type()).Interface()
+			if mergedObj == nil && oldObj != nil {
+				// compare the zero value of oldObj's type to oldObj
+				ref := reflect.ValueOf(oldObj)
+				zero := reflect.Zero(ref.Type()).Interface()
 
-			return fmt.Sprint(zero) == fmt.Sprint(oldObj)
+				return fmt.Sprint(zero) == fmt.Sprint(oldObj)
+			}
 		}
 
 		return fmt.Sprint(mergedObj) == fmt.Sprint(oldObj)
@@ -147,7 +149,9 @@ func equalObjWithSort(mergedObj interface{}, oldObj interface{}) (areEqual bool)
 
 // checkFieldsWithSort is a check for maps that uses an arbitrary sort to ensure it is
 // comparing the right values
-func checkFieldsWithSort(mergedObj map[string]interface{}, oldObj map[string]interface{}) (matches bool) {
+func checkFieldsWithSort(
+	mergedObj map[string]interface{}, oldObj map[string]interface{}, zeroValueEqualsNil bool,
+) (matches bool) {
 	// needed to compare lists, since merge messes up the order
 	if len(mergedObj) < len(oldObj) {
 		return false
@@ -159,14 +163,14 @@ func checkFieldsWithSort(mergedObj map[string]interface{}, oldObj map[string]int
 			// if field is a map, recurse to check for a match
 			oVal, ok := oldObj[i].(map[string]interface{})
 			if !ok {
-				if len(mVal) == 0 {
+				if zeroValueEqualsNil && len(mVal) == 0 {
 					break
 				}
 
 				return false
 			}
 
-			if !checkFieldsWithSort(mVal, oVal) {
+			if !checkFieldsWithSort(mVal, oVal, zeroValueEqualsNil) {
 				return false
 			}
 		case []interface{}:
@@ -280,7 +284,7 @@ func checkListsMatch(oldVal []interface{}, mergedVal []interface{}) (m bool) {
 		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 {
-				if !checkFieldsWithSort(mVal, oNestedVal) {
+				if !checkFieldsWithSort(mVal, oNestedVal, true) {
 					return false
 				}
 

controllers/configurationpolicy_utils_test.go

@@ -116,7 +116,30 @@ func TestCheckFieldsWithSort(t *testing.T) {
 		"resources":       []interface{}{},
 	}
 
-	assert.True(t, checkFieldsWithSort(mergedObj, oldObj))
+	assert.True(t, checkFieldsWithSort(mergedObj, oldObj, false))
+}
+
+func TestCheckFieldsWithSortEmptyMap(t *testing.T) {
+	oldObj := map[string]interface{}{
+		"spec": map[string]interface{}{
+			"storage": map[string]interface{}{
+				"s3": map[string]interface{}{
+					"bucket": "some-bucket",
+				},
+			},
+		},
+	}
+	mergedObj := map[string]interface{}{
+		"spec": map[string]interface{}{
+			"storage": map[string]interface{}{
+				"emptyDir": map[string]interface{}{},
+			},
+		},
+	}
+
+	assert.False(t, checkFieldsWithSort(mergedObj, oldObj, false))
+
+	assert.True(t, checkFieldsWithSort(mergedObj, oldObj, true))
 }
 
 func TestEqualObjWithSort(t *testing.T) {
@@ -133,8 +156,8 @@ func TestEqualObjWithSort(t *testing.T) {
 		"resources":       []interface{}{},
 	}
 
-	assert.True(t, equalObjWithSort(mergedObj, oldObj))
-	assert.False(t, equalObjWithSort(mergedObj, nil))
+	assert.True(t, equalObjWithSort(mergedObj, oldObj, true))
+	assert.False(t, equalObjWithSort(mergedObj, nil, true))
 
 	oldObj = map[string]interface{}{
 		"nonResourceURLs": []string{"/version", "/healthz"},
@@ -147,5 +170,30 @@ func TestEqualObjWithSort(t *testing.T) {
 		"resources":       []interface{}{},
 	}
 
-	assert.False(t, equalObjWithSort(mergedObj, oldObj))
+	assert.False(t, equalObjWithSort(mergedObj, oldObj, true))
+}
+
+func TestEqualObjWithSortString(t *testing.T) {
+	t.Parallel()
+
+	assert.True(t, equalObjWithSort("", nil, true))
+	assert.False(t, equalObjWithSort("", nil, false))
+	assert.True(t, equalObjWithSort(nil, "", true))
+	assert.False(t, equalObjWithSort(nil, "", false))
+}
+
+func TestEqualObjWithSortEmptyMap(t *testing.T) {
+	t.Parallel()
+
+	oldObj := map[string]interface{}{
+		"cities": map[string]interface{}{},
+	}
+	mergedObj := map[string]interface{}{
+		"cities": map[string]interface{}{
+			"raleigh": map[string]interface{}{},
+		},
+	}
+
+	assert.True(t, equalObjWithSort(mergedObj, oldObj, true))
+	assert.False(t, equalObjWithSort(mergedObj, oldObj, false))
 }

main.go

@@ -19,6 +19,7 @@ import (
 	operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/spf13/pflag"
 	"github.com/stolostron/go-log-utils/zaputil"
+	"golang.org/x/mod/semver"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	extensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -27,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/fields"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -329,9 +331,28 @@ func main() {
 		os.Exit(1)
 	}
 
+	discoveryClient := discovery.NewDiscoveryClientForConfigOrDie(targetK8sConfig)
+
+	serverVersion, err := discoveryClient.ServerVersion()
+	if err != nil {
+		log.Error(err, "unable to detect the managed cluster's Kubernetes version")
+		os.Exit(1)
+	}
+
+	dryRunSupported := semver.Compare(serverVersion.GitVersion, "v1.18.0") >= 0
+	if dryRunSupported {
+		log.Info("The managed cluster supports dry run API requests")
+	} else {
+		log.Info(
+			"The managed cluster does not support dry run API requests. Will assume that empty values are equal to " +
+				"not being set.",
+		)
+	}
+
 	reconciler := controllers.ConfigurationPolicyReconciler{
 		Client:                 mgr.GetClient(),
 		DecryptionConcurrency:  opts.decryptionConcurrency,
+		DryRunSupported:        dryRunSupported,
 		EvaluationConcurrency:  opts.evaluationConcurrency,
 		Scheme:                 mgr.GetScheme(),
 		Recorder:               mgr.GetEventRecorderFor(controllers.ControllerName),

test/e2e/case36_empty_map_test.go

@@ -0,0 +1,70 @@
+// Copyright Contributors to the Open Cluster Management project
+
+package e2e
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"open-cluster-management.io/config-policy-controller/test/utils"
+)
+
+var _ = Describe("Test resource creation when there are empty labels in configurationPolicy", Ordered, func() {
+	const (
+		case36AddEmptyMap     = "../resources/case36_empty_map/policy-should-add-empty-map.yaml"
+		case36AddEmptyMapName = "case36-check-selinux-options"
+		case36NoEmptyMap      = "../resources/case36_empty_map/policy-should-not-add-empty-map.yaml"
+		case36NoEmptyMapName  = "case36-check-node-selector"
+		case36Deployment      = "../resources/case36_empty_map/deployment.yaml"
+		case36DeploymentName  = "case36-deployment"
+	)
+
+	BeforeEach(func() {
+		By("creating the deployment " + case36DeploymentName)
+		utils.Kubectl("apply", "-f", case36Deployment)
+	})
+
+	AfterEach(func() {
+		By("deleting the deployment " + case36DeploymentName)
+		utils.Kubectl("delete", "-f", case36Deployment, "--ignore-not-found")
+		deleteConfigPolicies([]string{case36AddEmptyMapName, case36NoEmptyMapName})
+	})
+
+	It("verifies the policy "+case36AddEmptyMapName+"is NonCompliant", func() {
+		By("creating the policy " + case36AddEmptyMapName)
+		utils.Kubectl("apply", "-f", case36AddEmptyMap, "-n", testNamespace)
+
+		By("checking if the policy " + case36AddEmptyMapName + " is NonCompliant")
+		Eventually(func() interface{} {
+			managedPlc := utils.GetWithTimeout(
+				clientManagedDynamic,
+				gvrConfigPolicy,
+				case36AddEmptyMapName,
+				testNamespace,
+				true,
+				defaultTimeoutSeconds,
+			)
+
+			return utils.GetComplianceState(managedPlc)
+		}, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))
+	})
+
+	It("verifies the policy "+case36NoEmptyMapName+"is Compliant", func() {
+		By("creating the policy " + case36NoEmptyMapName)
+		utils.Kubectl("apply", "-f", case36NoEmptyMap, "-n", testNamespace)
+
+		By("checking if the policy " + case36NoEmptyMapName + " is Compliant")
+		Eventually(func() interface{} {
+			managedPlc := utils.GetWithTimeout(
+				clientManagedDynamic,
+				gvrConfigPolicy,
+				case36NoEmptyMapName,
+				testNamespace,
+				true,
+				defaultTimeoutSeconds,
+			)
+
+			return utils.GetComplianceState(managedPlc)
+		}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+	})
+})

test/resources/case36_empty_map/deployment.yaml

@@ -0,0 +1,22 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: case36-deployment
+  namespace: default
+  labels:
+    test: case36-deployment
+spec:
+  replicas: 0
+  selector:
+    matchLabels:
+      test: case36-deployment
+  template:
+    metadata:
+      labels:
+        test: case36-deployment
+    spec:
+      securityContext: {}
+      containers:
+        - image: nginx:1.7.9
+          imagePullPolicy: Never
+          name: nginx

test/resources/case36_empty_map/policy-should-add-empty-map.yaml

@@ -0,0 +1,21 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: case36-check-selinux-options
+spec:
+  remediationAction: inform
+  object-templates:
+    - complianceType: musthave
+      objectDefinition:
+        apiVersion: apps/v1
+        kind: Deployment
+        metadata:
+          name: case36-deployment
+          namespace: default
+        spec:
+          template:
+            spec:
+              # securityContext defaults to `{}` and seLinuxOptions has omitempty on the API server but is a pointer,
+              # so setting this to an empty object will cause the empty object to be returned by the API.
+              securityContext:
+                seLinuxOptions: {}