Small improvements to recordDiff

api/v1/configurationpolicy_types.go

@@ -199,9 +199,13 @@ type ObjectTemplate struct {
 	// +kubebuilder:pruning:PreserveUnknownFields
 	ObjectDefinition runtime.RawExtension `json:"objectDefinition"`
 
-	// RecordDiff specifies whether (and where) to log the diff between the object on the
-	// cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is
-	// ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise.
+	// RecordDiff specifies whether and where to log the difference between the object on the cluster
+	// and the `objectDefinition` parameter in the policy. The supported options are `InStatus` to record the
+	// difference in the policy status field, `Log` to log the difference in the
+	// `config-policy-controller` pod, and `None` to not log the difference. The default value is `None` for
+	// object kinds that include sensitive data such as `ConfigMap`, `OAuthAccessToken`,
+	// `OAuthAuthorizeTokens`, `Route`, and `Secret`, or when a templated `objectDefinition` references sensitive
+	// data. For all other kinds, the default value is `InStatus`.
 	RecordDiff RecordDiff `json:"recordDiff,omitempty"`
 }
 

controllers/configurationpolicy_controller.go

@@ -2767,7 +2767,7 @@ func handleDiff(
 	existingObject *unstructured.Unstructured,
 	mergedObject *unstructured.Unstructured,
 ) string {
-	if !isInform && recordDiff == policyv1.RecordDiffInStatus {
+	if !isInform && (recordDiff == policyv1.RecordDiffInStatus || recordDiff == policyv1.RecordDiffCensored) {
 		return ""
 	}
 
@@ -2792,8 +2792,10 @@ func handleDiff(
 	case policyv1.RecordDiffInStatus:
 		return computedDiff
 	case policyv1.RecordDiffCensored:
-		return `# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` +
-			`to record a diff.`
+		return `# The difference is redacted because it contains sensitive data. To override, the ` +
+			`spec["object-templates"][].recordDiff field must be set to "InStatus" for the difference to be recorded ` +
+			`in the policy status. Consider existing access to the ConfigurationPolicy objects and the etcd ` +
+			`encryption configuration before you proceed with an override.`
 	}
 
 	return ""

deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -165,9 +165,13 @@ spec:
                       x-kubernetes-preserve-unknown-fields: true
                     recordDiff:
                       description: |-
-                        RecordDiff specifies whether (and where) to log the diff between the object on the
-                        cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is
-                        ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise.
+                        RecordDiff specifies whether and where to log the difference between the object on the cluster
+                        and the `objectDefinition` parameter in the policy. The supported options are `InStatus` to record the
+                        difference in the policy status field, `Log` to log the difference in the
+                        `config-policy-controller` pod, and `None` to not log the difference. The default value is `None` for
+                        object kinds that include sensitive data such as `ConfigMap`, `OAuthAccessToken`,
+                        `OAuthAuthorizeTokens`, `Route`, and `Secret`, or when a templated `objectDefinition` references sensitive
+                        data. For all other kinds, the default value is `InStatus`.
                       enum:
                       - Log
                       - InStatus

deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -172,9 +172,13 @@ spec:
                       x-kubernetes-preserve-unknown-fields: true
                     recordDiff:
                       description: |-
-                        RecordDiff specifies whether (and where) to log the diff between the object on the
-                        cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is
-                        ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise.
+                        RecordDiff specifies whether and where to log the difference between the object on the cluster
+                        and the `objectDefinition` parameter in the policy. The supported options are `InStatus` to record the
+                        difference in the policy status field, `Log` to log the difference in the
+                        `config-policy-controller` pod, and `None` to not log the difference. The default value is `None` for
+                        object kinds that include sensitive data such as `ConfigMap`, `OAuthAccessToken`,
+                        `OAuthAuthorizeTokens`, `Route`, and `Secret`, or when a templated `objectDefinition` references sensitive
+                        data. For all other kinds, the default value is `InStatus`.
                       enum:
                       - Log
                       - InStatus

test/e2e/case39_diff_generation_test.go

@@ -148,8 +148,10 @@ var _ = Describe("Diff generation with sensitive input", Ordered, func() {
 
 		diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff")
 		Expect(diff).To(Equal(
-			`# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` +
-				`to record a diff.`,
+			`# The difference is redacted because it contains sensitive data. To override, the ` +
+				`spec["object-templates"][].recordDiff field must be set to "InStatus" for the difference to be ` +
+				`recorded in the policy status. Consider existing access to the ConfigurationPolicy objects and the ` +
+				`etcd encryption configuration before you proceed with an override.`,
 		))
 	})
 
@@ -179,8 +181,10 @@ var _ = Describe("Diff generation with sensitive input", Ordered, func() {
 
 		diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff")
 		Expect(diff).To(Equal(
-			`# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` +
-				`to record a diff.`,
+			`# The difference is redacted because it contains sensitive data. To override, the ` +
+				`spec["object-templates"][].recordDiff field must be set to "InStatus" for the difference to be ` +
+				`recorded in the policy status. Consider existing access to the ConfigurationPolicy objects and the ` +
+				`etcd encryption configuration before you proceed with an override.`,
 		))
 	})
 
@@ -210,9 +214,38 @@ var _ = Describe("Diff generation with sensitive input", Ordered, func() {
 
 		diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff")
 		Expect(diff).To(Equal(
-			`# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` +
-				`to record a diff.`,
+			`# The difference is redacted because it contains sensitive data. To override, the ` +
+				`spec["object-templates"][].recordDiff field must be set to "InStatus" for the difference to be ` +
+				`recorded in the policy status. Consider existing access to the ConfigurationPolicy objects and the ` +
+				`etcd encryption configuration before you proceed with an override.`,
 		))
+
+		By("Enforcing the policy removes the diff message")
+		utils.Kubectl(
+			"patch", "configurationpolicy", noDiffOnSecret, `--type=json`,
+			`-p=[{"op":"replace","path":"/spec/remediationAction","value":"enforce"}]`, "-n", testNamespace,
+		)
+
+		By("Verifying the diff in the status no longer contains instructions to set recordDiff")
+		Eventually(func() interface{} {
+			managedPlc = utils.GetWithTimeout(
+				clientManagedDynamic,
+				gvrConfigPolicy,
+				noDiffOnSecret,
+				testNamespace,
+				true,
+				defaultTimeoutSeconds,
+			)
+
+			return utils.GetComplianceState(managedPlc)
+		}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+
+		relatedObjects, _, err = unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(relatedObjects).To(HaveLen(1))
+
+		diff, _, _ = unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff")
+		Expect(diff).To(BeEmpty())
 	})
 })