From 2bd6142beeabef447146e0610138fbade72fdc79 Mon Sep 17 00:00:00 2001
From: Jeffrey Luo <jeluo@redhat.com>
Date: Tue, 20 Jun 2023 13:30:11 -0400
Subject: [PATCH] Bug: ACM-5052, Policy gets shortly into non-compliant state
 Solution: decode automatically converted stringData prior to evaluation

Signed-off-by: Jeffrey Luo <jeluo@redhat.com>
---
 controllers/configurationpolicy_controller.go | 25 +++++++
 test/e2e/case32_secret_stringdata_test.go     | 66 +++++++++++++++++++
 .../case32_create_secret.yaml                 | 21 ++++++
 3 files changed, 112 insertions(+)
 create mode 100644 test/e2e/case32_secret_stringdata_test.go
 create mode 100644 test/resources/case32_secret_stringdata/case32_create_secret.yaml

diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
index 1da37943..b2eed383 100644
--- a/controllers/configurationpolicy_controller.go
+++ b/controllers/configurationpolicy_controller.go
@@ -5,6 +5,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"reflect"
@@ -2462,6 +2463,30 @@ func handleSingleKey(
 			mergedValue.(map[string]interface{}), existingValue.(map[string]interface{}))
 	}
 
+	if key == "stringData" && existingObj.GetKind() == "Secret" {
+		// override automatic conversion from stringData to data prior to evaluation
+		existingValue = existingObj.UnstructuredContent()["data"]
+
+		encodedValue, _, err := unstructured.NestedStringMap(existingObj.Object, "data")
+		if err != nil {
+			message := "Error accessing encoded data"
+
+			return message, false, mergedValue, false
+		}
+
+		for k, value := range encodedValue {
+			decodedVal, err := base64.StdEncoding.DecodeString(value)
+			if err != nil {
+				secretName := existingObj.GetName()
+				message := fmt.Sprintf("Error decoding secret: %s", secretName)
+
+				return message, false, mergedValue, false
+			}
+
+			existingValue.(map[string]interface{})[k] = string(decodedVal)
+		}
+	}
+
 	// sort objects before checking equality to ensure they're in the same order
 	if !equalObjWithSort(mergedValue, existingValue) {
 		updateNeeded = true
diff --git a/test/e2e/case32_secret_stringdata_test.go b/test/e2e/case32_secret_stringdata_test.go
new file mode 100644
index 00000000..d503fbde
--- /dev/null
+++ b/test/e2e/case32_secret_stringdata_test.go
@@ -0,0 +1,66 @@
+// 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"
+)
+
+const (
+	case32ConfigPolicyName string = "case32config"
+	case32CreatePolicyYaml string = "../resources/case32_secret_stringdata/case32_create_secret.yaml"
+)
+
+var _ = Describe("Test converted stringData being decoded before comparison for Secrets", Ordered, func() {
+	It("Config should be created properly on the managed cluster", func() {
+		By("Creating " + case32ConfigPolicyName + " on managed")
+		utils.Kubectl("apply", "-f", case32CreatePolicyYaml, "-n", testNamespace)
+		cfg := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+			case32ConfigPolicyName, testNamespace, true, defaultTimeoutSeconds)
+		Expect(cfg).NotTo(BeNil())
+	})
+
+	It("Verifies the config policy is initially compliant "+case32ConfigPolicyName+" in "+testNamespace, func() {
+		By("Waiting for " + case32ConfigPolicyName + " to become Compliant")
+		Eventually(func() interface{} {
+			cfgplc := utils.GetWithTimeout(
+				clientManagedDynamic, gvrConfigPolicy,
+				case32ConfigPolicyName, testNamespace,
+				true, defaultTimeoutSeconds,
+			)
+
+			return utils.GetComplianceState(cfgplc)
+		}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+	})
+
+	It("Verifies that a secret is created by "+case32ConfigPolicyName+" in openshift-config", func() {
+		By("Grabbing htpasswd-secret from namespace openshift-config")
+		scrt := utils.GetWithTimeout(
+			clientManagedDynamic, gvrSecret, "htpasswd-secret", testNamespace, true, defaultTimeoutSeconds,
+		)
+
+		Expect(scrt).NotTo(BeNil())
+	})
+
+	It("Verifies the config policy "+case32ConfigPolicyName+" does not update in "+testNamespace, func() {
+		By("Checking the events on the configuration policy" + case32ConfigPolicyName)
+		Consistently(func() int {
+			eventlen := len(utils.GetMatchingEvents(
+				clientManaged, testNamespace,
+				case32ConfigPolicyName, case32ConfigPolicyName,
+				"updated", defaultTimeoutSeconds))
+
+			return eventlen
+		}, 60, 2).Should(BeNumerically("<", 1))
+	})
+
+	AfterAll(func() {
+		utils.Kubectl("delete", "configurationpolicy", case32ConfigPolicyName, "-n", testNamespace)
+		utils.Kubectl("delete", "secret", "htpasswd-secret", "-n", testNamespace)
+		utils.Kubectl("delete", "event",
+			"--field-selector=involvedObject.name="+case32ConfigPolicyName, "-n", "managed")
+	})
+})
diff --git a/test/resources/case32_secret_stringdata/case32_create_secret.yaml b/test/resources/case32_secret_stringdata/case32_create_secret.yaml
new file mode 100644
index 00000000..88ff3a6a
--- /dev/null
+++ b/test/resources/case32_secret_stringdata/case32_create_secret.yaml
@@ -0,0 +1,21 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: case32config
+spec:
+  object-templates:
+  - complianceType: musthave
+    objectDefinition:
+      apiVersion: v1
+      kind: Secret
+      metadata:
+        name: htpasswd-secret
+        namespace: managed
+      stringData:
+        htpasswd: |
+          alice:$QGJM$2qku.rrSEWRFD/tjOf.dZQ1q.RaR5tTmfNyZn0$1uI6eDx/Ont37ws
+          bob:S1KPovd0N/iDb9$dlTBH7weXG5rQV$5EPf9KrbJjw2HXy35ujhPw2y0$SIto
+          cadence:W$MeWCMWuKi0ApUyZ8byG5$DNL.3FD$c72kFhyZIX8Sf0yJH2AEOGFsRgEHJ
+  pruneObjectBehavior: None
+  remediationAction: enforce
+  severity: medium