Report Pending status despite hub template error

A previous change allowing hub-templates for all policy types, and
handling any hub-template errors in this controller, caused a change in
behavior. Before that change, when a policy both had a hub-template
error *and* an unsatisfied dependency, the policy would be marked as
Pending. The change made the hub-template error have priority.

This commit gives the Pending status priority, and adds a test for that
specific behavior. It also handles ConfigurationPolicies particularly
carefully: setting `PruneObjectBehavior` to `None` when the template
will be deleted specifically because of a hub-template error.

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

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

Author: JustinKuli

controllers/templatesync/template_sync.go

@@ -50,6 +50,8 @@ import (
 
 const (
 	ControllerName string = "policy-template-sync"
+
+	hubTmplErrorKey = "policy.open-cluster-management.io/hub-templates-error"
 )
 
 var log = ctrl.Log.WithName(ControllerName)
@@ -458,17 +460,6 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 			continue
 		}
 
-		// check for hub template error
-		if errAnno := metaObj.GetAnnotations()["policy.open-cluster-management.io/hub-templates-error"]; errAnno != "" {
-			_ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errAnno)
-
-			tLogger.Error(k8serrors.NewBadRequest(errAnno), "Failed to process the policy template")
-
-			policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc()
-
-			continue
-		}
-
 		dependencyFailures := r.processDependencies(ctx, dClient, discoveryClient, templateDeps, tLogger)
 
 		// Instantiate a dynamic client -- if it's a clusterwide resource, then leave off the namespace
@@ -522,26 +513,37 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 		// Attempt to fetch the resource
 		eObject, err := res.Get(ctx, tName, metav1.GetOptions{})
 		if err != nil {
-			if len(dependencyFailures) > 0 {
-				// template must be pending, do not create it
-				emitErr := r.emitTemplatePending(ctx, instance, tIndex, tName, isClusterScoped,
-					generatePendingMsg(dependencyFailures))
-				if emitErr != nil {
-					resultError = emitErr
+			// not found should consider creating it
+			if k8serrors.IsNotFound(err) {
+				if len(dependencyFailures) > 0 {
+					// template must be pending, do not create it
+					emitErr := r.emitTemplatePending(ctx, instance, tIndex, tName, isClusterScoped,
+						generatePendingMsg(dependencyFailures))
+					if emitErr != nil {
+						resultError = emitErr
+
+						continue
+					}
+
+					tLogger.Info("Dependencies were not satisfied for the policy template",
+						"namespace", instance.GetNamespace(),
+						"kind", gvk.Kind,
+					)
 
 					continue
 				}
 
-				tLogger.Info("Dependencies were not satisfied for the policy template",
-					"namespace", instance.GetNamespace(),
-					"kind", gvk.Kind,
-				)
+				// check for hub template error before creating
+				if errAnno := metaObj.GetAnnotations()[hubTmplErrorKey]; errAnno != "" {
+					_ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errAnno)
 
-				continue
-			}
+					tLogger.Error(k8serrors.NewBadRequest(errAnno), "Failed to process the policy template")
+
+					policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc()
+
+					continue
+				}
 
-			// not found should create it
-			if k8serrors.IsNotFound(err) {
 				// Handle setting the owner reference (this is skipped for clusterwide objects since our
 				// namespaced policy can't own a clusterwide object)
 				if !isClusterScoped {
@@ -676,6 +678,45 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 			continue
 		}
 
+		// check for hub template error
+		if errAnno := metaObj.GetAnnotations()[hubTmplErrorKey]; errAnno != "" {
+			_ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errAnno)
+
+			tLogger.Error(k8serrors.NewBadRequest(errAnno), "Failed to process the policy template")
+
+			policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc()
+
+			if rsrc.Resource == "configurationpolicies" {
+				// Patch it so that it doesn't clean up resources in the case of a formatting error
+				jsonPatch := []byte(`[{"op":"replace","path":"/spec/pruneObjectBehavior","value":"None"}]`)
+
+				_, err := res.Patch(ctx, tName, types.JSONPatchType, jsonPatch, metav1.PatchOptions{})
+				if err != nil {
+					tLogger.Error(err, "Failed to patch a ConfigurationPolicy that entered hub-template-error state",
+						"namespace", instance.GetNamespace(),
+						"name", tName,
+					)
+
+					resultError = err
+
+					continue
+				}
+			}
+
+			err = res.Delete(ctx, tName, metav1.DeleteOptions{})
+			if err != nil {
+				tLogger.Error(err, "Failed to delete a template that entered hub-template-error state",
+					"namespace", instance.GetNamespace(),
+					"name", tName,
+				)
+				policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "delete-error").Inc()
+
+				resultError = err
+			}
+
+			continue
+		}
+
 		// Handle owner references: Owned objects should be labeled with the parent policy name, and
 		// namespaced objects should have the policy for an owner reference (cluster scoped object will
 		// not have this owner reference because a namespaced object can't own a cluster scoped object).

test/e2e/case10_error_test.go

@@ -9,6 +9,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -255,32 +256,111 @@ var _ = Describe("Test error handling", func() {
 			return found
 		}, defaultTimeoutSeconds*2, 1).Should(BeFalse())
 	})
-	It("should throw a noncompliance event for hub template errors", func() {
-		By("Deploying a test policy CRD")
-		_, err := kubectlManaged("apply", "-f", yamlBasePath+"mock-crd.yaml")
-		Expect(err).ToNot(HaveOccurred())
-		DeferCleanup(func() error {
-			_, err := kubectlManaged("delete", "-f", yamlBasePath+"mock-crd.yaml")
-
-			return err
+	Describe("testing hub template errors cases", Ordered, func() {
+		AfterEach(func(ctx SpecContext) {
+			cfgInt := clientManagedDynamic.Resource(gvrConfigurationPolicy).Namespace(clusterNamespace)
+			Eventually(func() error {
+				_, err := cfgInt.Patch(ctx, "case10-bad-hubtemplate-notyet", types.JSONPatchType,
+					[]byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`),
+					metav1.PatchOptions{})
+				if k8serrors.IsNotFound(err) {
+					return nil
+				}
+
+				return err
+			}, defaultTimeoutSeconds, 1).ShouldNot(HaveOccurred())
 		})
 
-		hubApplyPolicy("case10-bad-hubtemplate",
-			yamlBasePath+"error-hubtemplate.yaml")
-
-		By("Checking for the error event")
-		Eventually(
-			checkForEvent("case10-bad-hubtemplate", "must be aboveground"),
-			defaultTimeoutSeconds,
-			1,
-		).Should(BeTrue())
+		It("should throw a noncompliance event for hub template errors", func() {
+			hubApplyPolicy("case10-bad-hubtemplate",
+				yamlBasePath+"error-hubtemplate.yaml")
+
+			By("Checking for the error event")
+			Eventually(
+				checkForEvent("case10-bad-hubtemplate", "must be aboveground"),
+				defaultTimeoutSeconds,
+				1,
+			).Should(BeTrue())
+
+			By("Checking for the compliance message formatting")
+			Eventually(
+				checkForEvent("case10-bad-hubtemplate", nonCompliantPrefix+nonCompliantPrefix),
+				defaultTimeoutSeconds,
+				1,
+			).Should(BeFalse())
+		})
+		It("should patch the pruneObjectBehavior to None during a hub-template-error", func(ctx SpecContext) {
+			hubApplyPolicy("case10-bad-hubtemplate-notyet",
+				yamlBasePath+"error-hubtemplate-notyet.yaml")
+
+			cfgInt := clientManagedDynamic.Resource(gvrConfigurationPolicy).Namespace(clusterNamespace)
+
+			By("Checking the pruneObjectBehavior")
+			Eventually(func() string {
+				cfgpol, err := cfgInt.Get(ctx, "case10-bad-hubtemplate-notyet", metav1.GetOptions{})
+				if err != nil {
+					return ""
+				}
+
+				prune, found, err := unstructured.NestedString(cfgpol.Object, "spec", "pruneObjectBehavior")
+				if !found || err != nil {
+					return ""
+				}
+
+				return prune
+			}, defaultTimeoutSeconds, 1).Should(Equal("DeleteAll"))
+
+			By("Applying a finalizer to the configuration policy")
+			Eventually(func() error {
+				_, err := cfgInt.Patch(ctx, "case10-bad-hubtemplate-notyet", types.JSONPatchType,
+					[]byte(`[{"op":"replace","path":"/metadata/finalizers","value":["policy.test/hold"]}]`),
+					metav1.PatchOptions{})
+
+				return err
+			}, defaultTimeoutSeconds, 1).ShouldNot(HaveOccurred())
+
+			polInt := clientHubDynamic.Resource(gvrPolicy).Namespace(clusterNamespaceOnHub)
+
+			By("Adding the hub-template-error annotation to the policy")
+			Eventually(func() error {
+				annoPatch := []byte(`[{"op":"replace",` +
+					`"path":"/spec/policy-templates/0/objectDefinition/metadata/annotations",` +
+					`"value":{"policy.open-cluster-management.io/hub-templates-error": "must be aboveground"}}]`)
+				_, err := polInt.Patch(ctx, "case10-bad-hubtemplate-notyet", types.JSONPatchType,
+					annoPatch, metav1.PatchOptions{})
+
+				return err
+			}, defaultTimeoutSeconds, 1).ShouldNot(HaveOccurred())
+
+			By("Checking for the error event")
+			Eventually(
+				checkForEvent("case10-bad-hubtemplate-notyet", "must be aboveground"),
+				defaultTimeoutSeconds,
+				1,
+			).Should(BeTrue())
+
+			By("Checking the pruneObjectBehavior")
+			Eventually(func() string {
+				cfgpol, err := cfgInt.Get(ctx, "case10-bad-hubtemplate-notyet", metav1.GetOptions{})
+				if err != nil {
+					return ""
+				}
+
+				prune, found, err := unstructured.NestedString(cfgpol.Object, "spec", "pruneObjectBehavior")
+				if !found || err != nil {
+					return ""
+				}
+
+				return prune
+			}, defaultTimeoutSeconds, 1).Should(Equal("None"))
+		})
+		It("should mark the policy as pending instead of throwing a hub-template error event", func() {
+			hubApplyPolicy("case10-bad-hubtemplate-pending",
+				yamlBasePath+"error-hubtemplate-pending.yaml")
 
-		By("Checking for the compliance message formatting")
-		Eventually(
-			checkForEvent("case10-bad-hubtemplate", nonCompliantPrefix+nonCompliantPrefix),
-			defaultTimeoutSeconds,
-			1,
-		).Should(BeFalse())
+			Eventually(checkCompliance("case10-bad-hubtemplate-pending"),
+				defaultTimeoutSeconds, 1).Should(Equal("Pending"))
+		})
 	})
 	It("should throw a noncompliance event if the template object is invalid", func() {
 		hubApplyPolicy("case10-invalid-severity",

test/resources/case10_template_sync_error_test/error-hubtemplate-notyet.yaml

@@ -0,0 +1,34 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case10-bad-hubtemplate-notyet
+  labels:
+    policy.open-cluster-management.io/cluster-name: managed
+    policy.open-cluster-management.io/cluster-namespace: managed
+    policy.open-cluster-management.io/root-policy: case10-bad-hubtemplate
+spec:
+  remediationAction: inform
+  disabled: false
+  policy-templates:
+    - objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case10-bad-hubtemplate-notyet
+          # annotations:
+          #   policy.open-cluster-management.io/hub-templates-error: "must be aboveground"
+        spec:
+          pruneObjectBehavior: DeleteAll
+          object-templates:
+            - complianceType: musthave
+              objectDefinition:
+                apiVersion: v1
+                kind: Pod
+                metadata:
+                  name: nginx-pod-e2e
+                  namespace: default
+                  annotations:
+                    policy.test/location: 'I come from {{hub the land down under hub}}'
+                spec:
+                  containers:
+                    - name: nginx

test/resources/case10_template_sync_error_test/error-hubtemplate-pending.yaml

@@ -0,0 +1,39 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case10-bad-hubtemplate-pending
+  labels:
+    policy.open-cluster-management.io/cluster-name: managed
+    policy.open-cluster-management.io/cluster-namespace: managed
+    policy.open-cluster-management.io/root-policy: case10-bad-hubtemplate
+spec:
+  remediationAction: inform
+  disabled: false
+  dependencies:
+  - apiVersion: policy.open-cluster-management.io/v1
+    kind: Policy
+    name: australia
+    namespace: default
+    compliance: Compliant
+  policy-templates:
+    - objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case10-bad-hubtemplate-pending
+          annotations:
+            policy.open-cluster-management.io/hub-templates-error: "must be aboveground"
+        spec:
+          object-templates:
+            - complianceType: musthave
+              objectDefinition:
+                apiVersion: v1
+                kind: Pod
+                metadata:
+                  name: nginx-pod-e2e
+                  namespace: default
+                  annotations:
+                    policy.test/location: 'I come from {{hub the land down under hub}}'
+                spec:
+                  containers:
+                    - name: nginx

test/resources/case10_template_sync_error_test/error-hubtemplate.yaml

@@ -12,10 +12,22 @@ spec:
   policy-templates:
     - objectDefinition:
         apiVersion: policy.open-cluster-management.io/v1
-        kind: MockPolicy
+        kind: ConfigurationPolicy
         metadata:
           name: case10-bad-hubtemplate-policy
           annotations:
             policy.open-cluster-management.io/hub-templates-error: "must be aboveground"
         spec:
-          foo: 'I come from {{hub the land down under hub}}'
+          object-templates:
+            - complianceType: musthave
+              objectDefinition:
+                apiVersion: v1
+                kind: Pod
+                metadata:
+                  name: nginx-pod-e2e
+                  namespace: default
+                  annotations:
+                    policy.test/location: 'I come from {{hub the land down under hub}}'
+                spec:
+                  containers:
+                    - name: nginx