From 3e02a504d41a23d20e1dcce316aae39049b8fb9f Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 20 Sep 2023 11:16:27 -0400 Subject: [PATCH] Add an error when apiVersion is missing The apiVersion field is generally required by kubernetes objects... it's surprising that this situation wasn't already caught by an error when decoding or getting a mapping. Refs: - https://issues.redhat.com/browse/ACM-7127 Signed-off-by: Justin Kulikauskas --- controllers/configurationpolicy_controller.go | 14 ++++++ test/e2e/case34_enforce_w_status_test.go | 18 +++++++ test/e2e/case35_no_apiversion_test.go | 48 +++++++++++++++++++ .../case35_no_apiversion/config-policy.yaml | 23 +++++++++ .../case35_no_apiversion/policy.yaml | 24 ++++++++++ 5 files changed, 127 insertions(+) create mode 100644 test/e2e/case35_no_apiversion_test.go create mode 100644 test/resources/case35_no_apiversion/config-policy.yaml create mode 100644 test/resources/case35_no_apiversion/policy.yaml diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 2e991c42..b4931e0a 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -1849,6 +1849,20 @@ func (r *ConfigurationPolicyReconciler) getMapping( return nil, result } + if gvk.Group == "" && gvk.Version == "" { + err := fmt.Errorf("object template at index [%v] in policy `%v` missing apiVersion", index, policy.Name) + + log.Error(err, "Can not get mapping for object") + + result = &objectTmplEvalResult{ + events: []objectTmplEvalEvent{ + {compliant: false, reason: "K8s object definition error", message: err.Error()}, + }, + } + + return nil, result + } + // initializes a mapping between Kind and APIVersion to a resource name r.lock.RLock() mapper := restmapper.NewDiscoveryRESTMapper(r.apiGroups) diff --git a/test/e2e/case34_enforce_w_status_test.go b/test/e2e/case34_enforce_w_status_test.go index c1c556b2..ef34aa49 100644 --- a/test/e2e/case34_enforce_w_status_test.go +++ b/test/e2e/case34_enforce_w_status_test.go @@ -61,6 +61,24 @@ var _ = Describe("Test compliance events of enforced policies that define a stat }) AfterAll(func() { + if CurrentSpecReport().Failed() { + By("Test failed, getting relevant compliance events for debugging") + events := utils.GetMatchingEvents(clientManaged, testNamespace, + policyName, cfgPlcName, ".*", defaultTimeoutSeconds) + + for _, ev := range events { + GinkgoWriter.Println("---") + GinkgoWriter.Println("Name:", ev.Name) + GinkgoWriter.Println("Reason:", ev.Reason) + GinkgoWriter.Println("Message:", ev.Message) + GinkgoWriter.Println("FirstTimestamp:", ev.FirstTimestamp) + GinkgoWriter.Println("LastTimestamp:", ev.LastTimestamp) + GinkgoWriter.Println("Count:", ev.Count) + GinkgoWriter.Println("Type:", ev.Type) + GinkgoWriter.Println("---") + } + } + utils.Kubectl("delete", "policy", policyName, "-n", "managed", "--ignore-not-found") configPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, cfgPlcName, "managed", false, defaultTimeoutSeconds, diff --git a/test/e2e/case35_no_apiversion_test.go b/test/e2e/case35_no_apiversion_test.go new file mode 100644 index 00000000..de356364 --- /dev/null +++ b/test/e2e/case35_no_apiversion_test.go @@ -0,0 +1,48 @@ +// Copyright (c) 2023 Red Hat, Inc. +// 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" +) + +var _ = Describe("Test a policy with an objectDefinition that is missing apiVersion", Ordered, func() { + const ( + rsrcPath = "../resources/case35_no_apiversion/" + policyYAML = rsrcPath + "policy.yaml" + policyName = "case34-parent" + cfgPlcYAML = rsrcPath + "config-policy.yaml" + cfgPlcName = "case34-cfgpol" + ) + + It("Should have the expected events", func() { + By("Setting up the policy") + createConfigPolicyWithParent(policyYAML, policyName, cfgPlcYAML) + + By("Checking there is a NonCompliant event on the policy") + Eventually(func() interface{} { + return utils.GetMatchingEvents(clientManaged, testNamespace, + policyName, cfgPlcName, "^NonCompliant;.*missing apiVersion", defaultTimeoutSeconds) + }, defaultTimeoutSeconds, 5).ShouldNot(BeEmpty()) + + By("Checking there are no Compliant events on the policy") + Consistently(func() interface{} { + return utils.GetMatchingEvents(clientManaged, testNamespace, + policyName, cfgPlcName, "^Compliant;", defaultTimeoutSeconds) + }, defaultTimeoutSeconds, 5).Should(BeEmpty()) + }) + + AfterAll(func() { + utils.Kubectl("delete", "policy", policyName, "-n", "managed", "--ignore-not-found") + configPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + cfgPlcName, "managed", false, defaultTimeoutSeconds, + ) + Expect(configPlc).To(BeNil()) + utils.Kubectl("delete", "event", "--field-selector=involvedObject.name="+policyName, "-n", "managed") + utils.Kubectl("delete", "event", "--field-selector=involvedObject.name="+cfgPlcName, "-n", "managed") + }) +}) diff --git a/test/resources/case35_no_apiversion/config-policy.yaml b/test/resources/case35_no_apiversion/config-policy.yaml new file mode 100644 index 00000000..7e20c441 --- /dev/null +++ b/test/resources/case35_no_apiversion/config-policy.yaml @@ -0,0 +1,23 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case35-cfgpol + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + blockOwnerDeletion: false + controller: true + kind: Policy + name: case35-parent + uid: 08bae967-4262-498a-84e9-d1f0e321b41e # to be replaced! +spec: + remediationAction: enforce + severity: low + object-templates: + - complianceType: musthave + objectDefinition: + kind: ConfigMap + metadata: + name: case35-cfgmap + namespace: default + data: + foo: bar diff --git a/test/resources/case35_no_apiversion/policy.yaml b/test/resources/case35_no_apiversion/policy.yaml new file mode 100644 index 00000000..c4f162c6 --- /dev/null +++ b/test/resources/case35_no_apiversion/policy.yaml @@ -0,0 +1,24 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: Policy +metadata: + name: case35-parent +spec: + disabled: false + remediationAction: enforce + policy-templates: + - objectDefinition: + apiVersion: policy.open-cluster-management.io/v1 + kind: ConfigurationPolicy + metadata: + name: case35-cfgpol + spec: + remediationAction: enforce + object-templates: + - complianceType: musthave + objectDefinition: + kind: ConfigMap + metadata: + name: case35-cfgmap + namespace: default + data: + foo: bar