address pr comment

Description of problem:
When Pod security policy is created and the status is changed from inform to enforce, the status is toggling. Not able to enforce pod security policy.

How to fix
Set default value when Kube API value omitted

ref: https://issues.redhat.com/browse/ACM-3109

Signed-off-by: Yi Rae Kim <yikim@redhat.com>

controllers/configurationpolicy_controller_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -754,3 +755,88 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		)
 	}
 }
+
+func TestShouldHandleSingleKeyFalse(t *testing.T) {
+	t.Parallel()
+
+	var unstruct unstructured.Unstructured
+	var unstructObj unstructured.Unstructured
+
+	var update, skip bool
+
+	tests := [][]map[string]interface{}{
+		{
+			{
+				"hostIPC":   false,
+				"container": "test",
+			},
+			{
+				"container": "test",
+			},
+			{
+				"key":    "hostIPC",
+				"expect": false,
+			},
+		},
+		{
+			{
+				"container": map[string]interface{}{
+					"image":   "nginx1.7.9",
+					"name":    "nginx",
+					"hostIPC": false,
+				},
+			},
+			{
+				"container": map[string]interface{}{
+					"image": "nginx1.7.9",
+					"name":  "nginx",
+				},
+			},
+			{
+				"key":    "container",
+				"expect": false,
+			},
+		},
+		{
+			{
+				"hostIPC":   true,
+				"container": "test",
+			},
+			{
+				"container": "test",
+			},
+			{
+				"key":    "hostIPC",
+				"expect": true,
+			},
+		},
+		{
+			{
+				"container": map[string]interface{}{
+					"image":   "nginx1.7.9",
+					"name":    "nginx",
+					"hostIPC": true,
+				},
+			},
+			{
+				"container": map[string]interface{}{
+					"image": "nginx1.7.9",
+					"name":  "nginx",
+				},
+			},
+			{
+				"key":    "container",
+				"expect": true,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		unstruct.Object = test[0]
+		unstructObj.Object = test[1]
+		key := test[2]["key"]
+		_, update, _, skip = handleSingleKey(key.(string), unstruct, &unstructObj, "musthave")
+		assert.Equal(t, update, test[2]["expect"])
+		assert.False(t, skip)
+	}
+}

controllers/configurationpolicy_utils.go

@@ -119,6 +119,16 @@ func equalObjWithSort(mergedObj interface{}, oldObj interface{}) (areEqual bool)
 			return false
 		}
 	default:
+		// NOTE: when type is string, int, bool
+		var oVal interface{}
+
+		if oldObj == nil && mergedObj != nil {
+			ref := reflect.ValueOf(mergedObj)
+			oVal = reflect.Zero(ref.Type()).Interface()
+
+			return fmt.Sprint(oVal) == fmt.Sprint(mergedObj)
+		}
+
 		if !reflect.DeepEqual(fmt.Sprint(mergedObj), fmt.Sprint(oldObj)) {
 			return false
 		}
@@ -175,8 +185,12 @@ func checkFieldsWithSort(mergedObj map[string]interface{}, oldObj map[string]int
 			// extra check to see if value is a byte value
 			mQty, err := apiRes.ParseQuantity(mVal)
 			if err != nil {
+				oVal := oldObj[i]
+				if oVal == nil {
+					oVal = ""
+				}
 				// An error indicates the value is a regular string, so check equality normally
-				if fmt.Sprint(oldObj[i]) != fmt.Sprint(mVal) {
+				if fmt.Sprint(oVal) != fmt.Sprint(mVal) {
 					return false
 				}
 			} else {
@@ -194,6 +208,12 @@ func checkFieldsWithSort(mergedObj map[string]interface{}, oldObj map[string]int
 		default:
 			// if field is not an object, just do a basic compare to check for a match
 			oVal := oldObj[i]
+			// When oVal value omitted because of omitempty
+			if oVal == nil && mVal != nil {
+				ref := reflect.ValueOf(mVal)
+				oVal = reflect.Zero(ref.Type()).Interface()
+			}
+
 			if fmt.Sprint(oVal) != fmt.Sprint(mVal) {
 				return false
 			}

controllers/configurationpolicy_utils_test.go

@@ -118,3 +118,34 @@ func TestCheckFieldsWithSort(t *testing.T) {
 
 	assert.True(t, checkFieldsWithSort(mergedObj, oldObj))
 }
+
+func TestEqualObjWithSort(t *testing.T) {
+	t.Parallel()
+
+	oldObj := map[string]interface{}{
+		"nonResourceURLs": []string{"/version", "/healthz"},
+		"verbs":           []string{"get"},
+	}
+	mergedObj := map[string]interface{}{
+		"nonResourceURLs": []string{"/version", "/healthz"},
+		"verbs":           []string{"get"},
+		"apiGroups":       []interface{}{},
+		"resources":       []interface{}{},
+	}
+
+	assert.True(t, equalObjWithSort(mergedObj, oldObj))
+	assert.False(t, equalObjWithSort(mergedObj, nil))
+
+	oldObj = map[string]interface{}{
+		"nonResourceURLs": []string{"/version", "/healthz"},
+		"verbs":           []string{"get"},
+	}
+	mergedObj = map[string]interface{}{
+		"nonResourceURLs": []string{"/version", "/healthz"},
+		"verbs":           []string{"post"},
+		"apiGroups":       []interface{}{},
+		"resources":       []interface{}{},
+	}
+
+	assert.False(t, equalObjWithSort(mergedObj, oldObj))
+}

go.mod

@@ -103,4 +103,5 @@ replace (
 	golang.org/x/text => golang.org/x/text v0.3.8 // CVE-2022-32149
 	gopkg.in/yaml.v2 => gopkg.in/yaml.v2 v2.4.0 // CVE-2022-3064
 	k8s.io/client-go => k8s.io/client-go v0.23.9
+	open-cluster-management.io/governance-policy-propagator => github.com/stolostron/governance-policy-propagator v0.0.0-20220727212642-86a318ab17cf
 )

test/e2e/case12_list_compare_test.go

@@ -356,7 +356,18 @@ var _ = Describe("Test list handling for musthave", func() {
 			deleteConfigPolicies(policies)
 		})
 	})
-	Describe("Create a statefulset object with a byte quantity field on managed cluster in ns:"+testNamespace, func() {
+	Describe("Create a statefulset object with a byte quantity field "+
+		"on managed cluster in ns:"+testNamespace, Ordered, func() {
+		cleanup := func() {
+			// Delete the policies and ignore any errors (in case it was deleted previously)
+			policies := []string{
+				case12ByteCreate,
+				case12ByteInform,
+			}
+
+			deleteConfigPolicies(policies)
+		}
+		BeforeAll(cleanup)
 		It("should only add the list item with the rounded byte value once", func() {
 			By("Creating " + case12ByteCreate + " and " + case12ByteInform + " on managed")
 			utils.Kubectl("apply", "-f", case12ByteCreateYaml, "-n", testNamespace)
@@ -389,14 +400,6 @@ var _ = Describe("Test list handling for musthave", func() {
 				return utils.GetComplianceState(managedPlc)
 			}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
 		})
-
-		It("Cleans up", func() {
-			policies := []string{
-				case12ByteCreate,
-				case12ByteInform,
-			}
-
-			deleteConfigPolicies(policies)
-		})
+		AfterAll(cleanup)
 	})
 })

test/e2e/case20_delete_objects_test.go

@@ -527,7 +527,8 @@ var _ = Describe("Test objects that should be deleted are actually being deleted
 				return nil
 			}, defaultTimeoutSeconds, 1).Should(BeNil())
 		})
-		It("deletes the pod after the policy is deleted", func() {
+		AfterAll(func() {
+			By("deletes the pod after the policy is deleted")
 			deleteConfigPolicies([]string{case20ConfigPolicyNameCreate})
 			Eventually(func() interface{} {
 				pod := utils.GetWithTimeout(clientManagedDynamic, gvrPod,

test/e2e/case25_related_object_metric_test.go

@@ -56,23 +56,19 @@ var _ = Describe("Test related object metrics", Ordered, func() {
 		// Delete the policies and ignore any errors (in case it was deleted previously)
 		cmd := exec.Command("kubectl", "delete",
 			"-f", policyYaml,
-			"-n", testNamespace)
+			"-n", testNamespace, "--ignore-not-found")
 		_, _ = cmd.CombinedOutput()
 		opt := metav1.ListOptions{}
 		utils.ListWithTimeout(
 			clientManagedDynamic, gvrConfigPolicy, opt, 0, false, defaultTimeoutSeconds)
 		utils.GetWithTimeout(
 			clientManagedDynamic, gvrConfigMap, relatedObject, "default", false, defaultTimeoutSeconds)
-	}
-
-	It("should clean up", cleanup)
 
-	It("should have no common related object metrics after clean up", func() {
 		By("Checking metric endpoint for related object gauges")
 		Eventually(func() interface{} {
 			return utils.GetMetrics("common_related_objects")
 		}, defaultTimeoutSeconds, 1).Should(Equal([]string{}))
-	})
+	}
 
 	AfterAll(cleanup)
 })

test/e2e/case26_user_error_metric_test.go

@@ -15,6 +15,21 @@ var _ = Describe("Test related object metrics", Ordered, func() {
 		policy1Name = "case26-test-policy-1"
 		policyYaml  = "../resources/case26_user_error_metric/case26-missing-crd.yaml"
 	)
+	cleanup := func() {
+		// Delete the policies and ignore any errors (in case it was deleted previously)
+		cmd := exec.Command("kubectl", "delete",
+			"-f", policyYaml,
+			"-n", testNamespace, "--ignore-not-found")
+		_, _ = cmd.CombinedOutput()
+		opt := metav1.ListOptions{}
+		utils.ListWithTimeout(
+			clientManagedDynamic, gvrConfigPolicy, opt, 0, false, defaultTimeoutSeconds)
+		By("Checking metric endpoint for related object gauges")
+		Eventually(func() interface{} {
+			return utils.GetMetrics("policy_user_errors")
+		}, defaultTimeoutSeconds, 1).Should(Equal([]string{}))
+	}
+
 	It("should create policy", func() {
 		By("Creating " + policyYaml)
 		utils.Kubectl("apply",
@@ -36,25 +51,5 @@ var _ = Describe("Test related object metrics", Ordered, func() {
 		}, defaultTimeoutSeconds, 1).Should(Equal([]string{"policies", "counter", "1"}))
 	})
 
-	cleanup := func() {
-		// Delete the policies and ignore any errors (in case it was deleted previously)
-		cmd := exec.Command("kubectl", "delete",
-			"-f", policyYaml,
-			"-n", testNamespace)
-		_, _ = cmd.CombinedOutput()
-		opt := metav1.ListOptions{}
-		utils.ListWithTimeout(
-			clientManagedDynamic, gvrConfigPolicy, opt, 0, false, defaultTimeoutSeconds)
-	}
-
-	It("should clean up", cleanup)
-
-	It("should have no common related object metrics after clean up", func() {
-		By("Checking metric endpoint for related object gauges")
-		Eventually(func() interface{} {
-			return utils.GetMetrics("policy_user_errors")
-		}, defaultTimeoutSeconds, 1).Should(Equal([]string{}))
-	})
-
 	AfterAll(cleanup)
 })

test/e2e/case28_evauluation_metric_test.go

@@ -102,7 +102,6 @@ var _ = Describe("Test config policy evaluation metrics", Ordered, func() {
 		opt := metav1.ListOptions{}
 		utils.ListWithTimeout(
 			clientManagedDynamic, gvrConfigPolicy, opt, 0, false, defaultTimeoutSeconds)
-
 		Eventually(func() interface{} {
 			return utils.GetMetrics(
 				"config_policy_evaluation_total", fmt.Sprintf(`name=\"%s\"`, policyName))