From 5a67c4ae993200bb58069874909937ce20c127f5 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 20 Sep 2023 13:53:44 -0400 Subject: [PATCH 1/2] Add an error when the 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 (cherry picked from commit f997dbcdc8bc09fb2a1cf80fb30537ca84a991ac) --- controllers/configurationpolicy_controller.go | 14 ++++++ test/e2e/case35_no_apiversion_test.go | 48 +++++++++++++++++++ .../case35_no_apiversion/config-policy.yaml | 23 +++++++++ .../case35_no_apiversion/policy.yaml | 24 ++++++++++ 4 files changed, 109 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/case35_no_apiversion_test.go b/test/e2e/case35_no_apiversion_test.go new file mode 100644 index 00000000..0748d8fa --- /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", func() { + const ( + rsrcPath = "../resources/case35_no_apiversion/" + policyYAML = rsrcPath + "policy.yaml" + policyName = "case35-parent" + cfgPlcYAML = rsrcPath + "config-policy.yaml" + cfgPlcName = "case35-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()) + }) + + AfterEach(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 From ecb7a1905ee369fddbc993d69685ece688266679 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 20 Sep 2023 13:54:25 -0400 Subject: [PATCH 2/2] Add debug to enforce with status test This test has been flakey - the last check for Compliant events has been coming back empty unexpectedly. This will print information about all of the events to help understand what is happening. Signed-off-by: Justin Kulikauskas (cherry picked from commit 62a3ac4182b2868eb1a260ba053142d0720b5bef) --- test/e2e/case34_enforce_w_status_test.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/test/e2e/case34_enforce_w_status_test.go b/test/e2e/case34_enforce_w_status_test.go index c1c556b2..99ca5046 100644 --- a/test/e2e/case34_enforce_w_status_test.go +++ b/test/e2e/case34_enforce_w_status_test.go @@ -4,13 +4,15 @@ package e2e import ( + "strconv" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "open-cluster-management.io/config-policy-controller/test/utils" ) -var _ = Describe("Test compliance events of enforced policies that define a status", Ordered, func() { +var _ = Describe("Test compliance events of enforced policies that define a status", func() { const ( rsrcPath = "../resources/case34_enforce_w_status/" policyYAML = rsrcPath + "policy.yaml" @@ -60,7 +62,25 @@ var _ = Describe("Test compliance events of enforced policies that define a stat }, defaultTimeoutSeconds, 5).ShouldNot(BeEmpty()) }) - AfterAll(func() { + AfterEach(func() { + if CurrentSpecReport().Failed() { + events := utils.GetMatchingEvents(clientManaged, testNamespace, + policyName, ".*", ".*", defaultTimeoutSeconds) + + By("Test failed, printing compliance events for debugging, event count = " + strconv.Itoa(len(events))) + 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,