From c02784b4455a7082a400f622076f1ecb2e0f49cf Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 12 Nov 2025 14:09:20 -0500 Subject: [PATCH] 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 (cherry picked from commit c20f5551aa38a480afd90aa3071e1fbbad821481) --- controllers/configurationpolicy_controller.go | 33 ++++++++++++++----- test/dryrun/kind_field/kind_field_test.go | 14 ++++++++ .../pod_annotation_invalid/input_1.yaml | 15 +++++++++ .../pod_annotation_invalid/input_ns.yaml | 5 +++ .../pod_annotation_invalid/output.txt | 6 ++++ .../pod_annotation_invalid/policy.yaml | 18 ++++++++++ .../kind_field/pod_label_invalid/input_1.yaml | 15 +++++++++ .../pod_label_invalid/input_ns.yaml | 5 +++ .../kind_field/pod_label_invalid/output.txt | 6 ++++ .../kind_field/pod_label_invalid/policy.yaml | 18 ++++++++++ 10 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 test/dryrun/kind_field/pod_annotation_invalid/input_1.yaml create mode 100644 test/dryrun/kind_field/pod_annotation_invalid/input_ns.yaml create mode 100644 test/dryrun/kind_field/pod_annotation_invalid/output.txt create mode 100644 test/dryrun/kind_field/pod_annotation_invalid/policy.yaml create mode 100644 test/dryrun/kind_field/pod_label_invalid/input_1.yaml create mode 100644 test/dryrun/kind_field/pod_label_invalid/input_ns.yaml create mode 100644 test/dryrun/kind_field/pod_label_invalid/output.txt create mode 100644 test/dryrun/kind_field/pod_label_invalid/policy.yaml diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index f9cb0664..80c5096e 100644 --- a/controllers/configurationpolicy_controller.go +++ b/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 } diff --git a/test/dryrun/kind_field/kind_field_test.go b/test/dryrun/kind_field/kind_field_test.go index 4974fe58..4191d90a 100644 --- a/test/dryrun/kind_field/kind_field_test.go +++ b/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)) +} diff --git a/test/dryrun/kind_field/pod_annotation_invalid/input_1.yaml b/test/dryrun/kind_field/pod_annotation_invalid/input_1.yaml new file mode 100644 index 00000000..8c267b91 --- /dev/null +++ b/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 diff --git a/test/dryrun/kind_field/pod_annotation_invalid/input_ns.yaml b/test/dryrun/kind_field/pod_annotation_invalid/input_ns.yaml new file mode 100644 index 00000000..e32c3ced --- /dev/null +++ b/test/dryrun/kind_field/pod_annotation_invalid/input_ns.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: managed +spec: {} diff --git a/test/dryrun/kind_field/pod_annotation_invalid/output.txt b/test/dryrun/kind_field/pod_annotation_invalid/output.txt new file mode 100644 index 00000000..4e1a6623 --- /dev/null +++ b/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 diff --git a/test/dryrun/kind_field/pod_annotation_invalid/policy.yaml b/test/dryrun/kind_field/pod_annotation_invalid/policy.yaml new file mode 100644 index 00000000..8f8e38f1 --- /dev/null +++ b/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 diff --git a/test/dryrun/kind_field/pod_label_invalid/input_1.yaml b/test/dryrun/kind_field/pod_label_invalid/input_1.yaml new file mode 100644 index 00000000..8c267b91 --- /dev/null +++ b/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 diff --git a/test/dryrun/kind_field/pod_label_invalid/input_ns.yaml b/test/dryrun/kind_field/pod_label_invalid/input_ns.yaml new file mode 100644 index 00000000..e32c3ced --- /dev/null +++ b/test/dryrun/kind_field/pod_label_invalid/input_ns.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: managed +spec: {} diff --git a/test/dryrun/kind_field/pod_label_invalid/output.txt b/test/dryrun/kind_field/pod_label_invalid/output.txt new file mode 100644 index 00000000..c4ad8436 --- /dev/null +++ b/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 diff --git a/test/dryrun/kind_field/pod_label_invalid/policy.yaml b/test/dryrun/kind_field/pod_label_invalid/policy.yaml new file mode 100644 index 00000000..54d535d5 --- /dev/null +++ b/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