Raise a violation when given invalid metadata

Previously, if labels or annotations inside an objectTemplate used
values that were not strings, the ConfigurationPolicy might try to
remove all labels or annotations on that object. That behavior was
potentially destructive, and it was easy for a policy author to
accidentally trigger by getting some quotes wrong in their template, or
by using `yes` as a value (which is interpreted by some parsers as a
boolean `true`).

Now, the ConfigurationPolicy will emit a special error if the input
object template has invalid labels or annotations.

Refs:
 - https://issues.redhat.com/browse/ACM-26186

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

Author: JustinKuli

controllers/configurationpolicy_controller.go

@@ -3553,17 +3553,34 @@ func handleKeys(
 			continue
 		}
 
-		// only look at labels and annotations for metadata - configurationPolicies do not update other metadata fields
 		if key == "metadata" {
-			// if it's not the right type, the map will be empty
-			mdMap, _ := mergedObj.(map[string]interface{})
+			if mdMap, ok := mergedObj.(map[string]any); ok {
+				// ConfigurationPolicies should not affect metadata fields other than labels and
+				// annotations, and if the desired state is invalid, a violation should be raised.
+				mergedAnnos, found, err := unstructured.NestedStringMap(mdMap, "annotations")
+				if err != nil {
+					// A test verifies that this error text is stable
+					errMsg, _ := strings.CutPrefix(err.Error(), ".annotations accessor error: ")
+
+					return true, fmt.Sprintf("invalid annotation, error: %v", errMsg), true, statusMismatch, missingKey
+				}
+
+				if found {
+					existingObj.SetAnnotations(mergedAnnos)
+				}
 
-			// if either isn't found, they'll just be empty
-			mergedAnnotations, _, _ := unstructured.NestedStringMap(mdMap, "annotations")
-			mergedLabels, _, _ := unstructured.NestedStringMap(mdMap, "labels")
+				mergedLabels, found, err := unstructured.NestedStringMap(mdMap, "labels")
+				if err != nil {
+					// A test verifies that this error text is stable
+					errMsg, _ := strings.CutPrefix(err.Error(), ".labels accessor error: ")
+
+					return true, fmt.Sprintf("invalid label, error: %v", errMsg), true, statusMismatch, missingKey
+				}
 
-			existingObj.SetAnnotations(mergedAnnotations)
-			existingObj.SetLabels(mergedLabels)
+				if found {
+					existingObj.SetLabels(mergedLabels)
+				}
+			}
 		} else {
 			existingObj.UnstructuredContent()[key] = mergedObj
 		}

test/dryrun/kind_field/kind_field_test.go

@@ -23,3 +23,17 @@ var podAnnotationMismatch embed.FS
 func TestPodAnnotationMisMatch(t *testing.T) {
 	t.Run("Test pod annotation mismatch", dryrun.Run(podAnnotationMismatch))
 }
+
+//go:embed pod_label_invalid/*
+var podLabelInvalid embed.FS
+
+func TestPodLabelInvalid(t *testing.T) {
+	t.Run("Test invalid pod label in policy", dryrun.Run(podLabelInvalid))
+}
+
+//go:embed pod_annotation_invalid/*
+var podAnnotationInvalid embed.FS
+
+func TestPodAnnotationInvalid(t *testing.T) {
+	t.Run("Test invalid pod annotation in policy", dryrun.Run(podAnnotationInvalid))
+}

test/dryrun/kind_field/pod_annotation_invalid/input_1.yaml

@@ -0,0 +1,15 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: nginx-pod-e2e-10
+  namespace: managed
+  annotations:
+    test: e2e10
+  labels:
+    test: e2e10
+spec:
+  containers:
+    - image: nginx:1.7.9
+      name: nginx
+      ports:
+        - containerPort: 80

test/dryrun/kind_field/pod_annotation_invalid/input_ns.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: managed
+spec: {}

test/dryrun/kind_field/pod_annotation_invalid/output.txt

@@ -0,0 +1,6 @@
+# Diffs:
+v1 Pod managed/nginx-pod-e2e-10:
+
+# Compliance messages:
+NonCompliant; violation - pods [nginx-pod-e2e-10] found but not as specified in namespace managed
+NonCompliant; violation - invalid annotation, error: contains non-string value in the map under key "case": 10 is of the type int64, expected string

test/dryrun/kind_field/pod_annotation_invalid/policy.yaml

@@ -0,0 +1,18 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: policy-kind-labels-fail
+spec:
+  remediationAction: inform # will be overridden by remediationAction in parent policy
+  severity: low
+  object-templates:
+    - complianceType: musthave
+      objectDefinition:
+        apiVersion: v1
+        kind: Pod
+        metadata:
+          namespace: managed
+          name: nginx-pod-e2e-10
+          annotations:
+            case: 10
+            test: fail

test/dryrun/kind_field/pod_label_invalid/input_1.yaml

@@ -0,0 +1,15 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: nginx-pod-e2e-10
+  namespace: managed
+  annotations:
+    test: e2e10
+  labels:
+    test: e2e10
+spec:
+  containers:
+    - image: nginx:1.7.9
+      name: nginx
+      ports:
+        - containerPort: 80

test/dryrun/kind_field/pod_label_invalid/input_ns.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: managed
+spec: {}

test/dryrun/kind_field/pod_label_invalid/output.txt

@@ -0,0 +1,6 @@
+# Diffs:
+v1 Pod managed/nginx-pod-e2e-10:
+
+# Compliance messages:
+NonCompliant; violation - pods [nginx-pod-e2e-10] found but not as specified in namespace managed
+NonCompliant; violation - invalid label, error: contains non-string value in the map under key "problem": true is of the type bool, expected string

test/dryrun/kind_field/pod_label_invalid/policy.yaml

@@ -0,0 +1,18 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: policy-kind-labels-fail
+spec:
+  remediationAction: inform # will be overridden by remediationAction in parent policy
+  severity: low
+  object-templates:
+    - complianceType: musthave
+      objectDefinition:
+        apiVersion: v1
+        kind: Pod
+        metadata:
+          namespace: managed
+          name: nginx-pod-e2e-10
+          labels:
+            problem: yes
+            test: fail