From f997dbcdc8bc09fb2a1cf80fb30537ca84a991ac Mon Sep 17 00:00:00 2001
From: Justin Kulikauskas <jkulikau@redhat.com>
Date: Wed, 20 Sep 2023 13:53:44 -0400
Subject: [PATCH] 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 <jkulikau@redhat.com>
---
 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