Set state as Compliant when ignorePending is true

Previously, individual templates were always marked as "Pending" if they
had an unsatisfied dependency. The `ignorePending` field was only
considered when calculating the overall compliance of a Policy. But that
meant that it was not possible to correctly calculate the overall state
with just the status object, which is confusing.

Now, the `ignorePending` field is considered when marking the compliance
of each template. This makes how the overall compliance is calculated
more clear, and the new messages may be more helpful for users.

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

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

Author: JustinKuli

controllers/statussync/policy_status_sync.go

@@ -205,8 +205,6 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 
 	reqLogger.Info("Updating status for policy templates")
 
-	shouldSkipPending := map[*policiesv1.DetailsPerTemplate]bool{}
-
 	for _, policyT := range instance.Spec.PolicyTemplates {
 		object, _, err := unstructured.UnstructuredJSONScheme.Decode(policyT.ObjectDefinition.Raw, nil, nil)
 		if err != nil {
@@ -344,9 +342,6 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 		// append existingDpt to status
 		newStatus.Details = append(newStatus.Details, existingDpt)
 
-		// build a map to determine whether to skip pending status from this template
-		shouldSkipPending[existingDpt] = policyT.IgnorePending
-
 		reqLogger.Info("Status update complete", "PolicyTemplate", tName)
 	}
 
@@ -360,7 +355,7 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 			isCompliant = false
 
 			break
-		} else if dpt.ComplianceState == "Pending" && !shouldSkipPending[dpt] {
+		} else if dpt.ComplianceState == "Pending" {
 			instance.Status.ComplianceState = policiesv1.Pending
 			isCompliant = false
 		} else if dpt.ComplianceState == "" {

controllers/templatesync/template_sync.go

@@ -299,11 +299,7 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 		if err != nil {
 			if len(dependencyFailures) > 0 {
 				// template must be pending, do not create it
-				pendingErr := generatePendingErr(dependencyFailures)
-				resultError = pendingErr
-				errMsg := fmt.Sprintf("Dependencies were not satisfied: %s", pendingErr)
-
-				r.emitTemplatePending(instance, tIndex, tName, errMsg)
+				r.emitTemplatePending(instance, tIndex, tName, generatePendingMsg(dependencyFailures))
 				tLogger.Info("Dependencies were not satisfied for the policy template",
 					"namespace", instance.GetNamespace(),
 					"kind", gvk.Kind,
@@ -378,11 +374,7 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 
 		if len(dependencyFailures) > 0 {
 			// template must be pending, need to delete it and error
-			pendingErr := generatePendingErr(dependencyFailures)
-			resultError = pendingErr
-			errMsg := fmt.Sprintf("Dependencies were not satisfied: %s", pendingErr)
-
-			r.emitTemplatePending(instance, tIndex, tName, errMsg)
+			r.emitTemplatePending(instance, tIndex, tName, generatePendingMsg(dependencyFailures))
 			tLogger.Info("Dependencies were not satisfied for the policy template",
 				"namespace", instance.GetNamespace(),
 				"kind", gvk.Kind,
@@ -394,6 +386,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
 					"namespace", instance.GetNamespace(),
 					"name", tName,
 				)
+
+				resultError = err
 			}
 
 			continue
@@ -555,20 +549,22 @@ func (r *PolicyReconciler) processDependencies(ctx context.Context, dClient dyna
 	return dependencyFailures
 }
 
-// generatePendingErr formats the list of failed dependencies into a readable error
-func generatePendingErr(dependencyFailures []depclient.ObjectIdentifier) error {
+// generatePendingMsg formats the list of failed dependencies into a readable error.
+// Example: `Dependencies were not satisfied: 1 is still pending (FooPolicy foo)`
+func generatePendingMsg(dependencyFailures []depclient.ObjectIdentifier) string {
 	names := make([]string, len(dependencyFailures))
 	for i, dep := range dependencyFailures {
 		names[i] = fmt.Sprintf("%s %s", dep.Kind, dep.Name)
 	}
 
 	nameStr := strings.Join(names, ", ")
 
-	return fmt.Errorf(
-		"%d dependencies are still pending (%s)",
-		len(dependencyFailures),
-		nameStr,
-	)
+	fmtStr := "Dependencies were not satisfied: %d are still pending (%s)"
+	if len(dependencyFailures) == 1 {
+		fmtStr = "Dependencies were not satisfied: %d is still pending (%s)"
+	}
+
+	return fmt.Sprintf(fmtStr, len(dependencyFailures), nameStr)
 }
 
 func overrideRemediationAction(instance *policiesv1.Policy, tObjectUnstructured *unstructured.Unstructured) {
@@ -600,21 +596,29 @@ func (r *PolicyReconciler) emitTemplateError(pol *policiesv1.Policy, tIndex int,
 	r.Recorder.Event(pol, "Warning", "PolicyTemplateSync", errMsg)
 }
 
-// emitTemplatePending performs actions that ensure correct reporting of dependency errors in the
-// policy framework. If the policy's status already reflects the current error, then no actions
+// emitTemplatePending performs actions that ensure correct reporting of pending dependencies in the
+// policy framework. If the policy's status already reflects the current status, then no actions
 // are taken.
-func (r *PolicyReconciler) emitTemplatePending(pol *policiesv1.Policy, tIndex int, tName, errMsg string) {
+func (r *PolicyReconciler) emitTemplatePending(pol *policiesv1.Policy, tIndex int, tName, msg string) {
+	statusMsg := "Pending; " + msg
+	eventType := "Warning"
+
+	if pol.Spec.PolicyTemplates[tIndex].IgnorePending {
+		statusMsg = "Compliant; " + msg + " but ignorePending is true"
+		eventType = "Normal"
+	}
+
 	// check if the error is already present in the policy status - if so, return early
-	if strings.Contains(getLatestStatusMessage(pol, tIndex), errMsg) {
+	if strings.Contains(getLatestStatusMessage(pol, tIndex), statusMsg) {
 		return
 	}
 
 	// emit the non-compliance event
 	policyComplianceReason := fmt.Sprintf(policyFmtStr, pol.GetNamespace(), tName)
-	r.Recorder.Event(pol, "Warning", policyComplianceReason, "Pending; template-error; "+errMsg)
+	r.Recorder.Event(pol, eventType, policyComplianceReason, statusMsg)
 
 	// emit an informational event
-	r.Recorder.Event(pol, "Warning", "PolicyTemplateSync", errMsg)
+	r.Recorder.Event(pol, eventType, "PolicyTemplateSync", statusMsg)
 }
 
 // handleSyncSuccess performs common actions that should be run whenever a template is in sync,

test/e2e/case12_ordering_test.go

@@ -11,18 +11,20 @@ import (
 )
 
 const (
-	case12PolicyName          string = "case12-test-policy"
-	case12PolicyYaml          string = "../resources/case12_ordering/case12-plc.yaml"
-	case12PolicyNameInvalid   string = "case12-test-policy-invalid"
-	case12PolicyYamlInvalid   string = "../resources/case12_ordering/case12-plc-invalid-dep.yaml"
-	case12ExtraDepsPolicyName string = "case12-test-policy-multi"
-	case12ExtraDepsPolicyYaml string = "../resources/case12_ordering/case12-plc-multiple-deps.yaml"
-	case12Plc2TemplatesName   string = "case12-test-policy-2-templates"
-	case12Plc2TemplatesYaml   string = "../resources/case12_ordering/case12-plc-2-template.yaml"
-	case12DepName             string = "namespace-foo-setup-policy"
-	case12DepYaml             string = "../resources/case12_ordering/case12-dependency.yaml"
-	case12DepBName            string = "namespace-foo-setup-policy-b"
-	case12DepBYaml            string = "../resources/case12_ordering/case12-dependency-b.yaml"
+	case12PolicyName              string = "case12-test-policy"
+	case12PolicyYaml              string = "../resources/case12_ordering/case12-plc.yaml"
+	case12PolicyIgnorePendingName string = "case12-test-policy-ignorepending"
+	case12PolicyIgnorePendingYaml string = "../resources/case12_ordering/case12-plc-ignorepending.yaml"
+	case12PolicyNameInvalid       string = "case12-test-policy-invalid"
+	case12PolicyYamlInvalid       string = "../resources/case12_ordering/case12-plc-invalid-dep.yaml"
+	case12ExtraDepsPolicyName     string = "case12-test-policy-multi"
+	case12ExtraDepsPolicyYaml     string = "../resources/case12_ordering/case12-plc-multiple-deps.yaml"
+	case12Plc2TemplatesName       string = "case12-test-policy-2-templates"
+	case12Plc2TemplatesYaml       string = "../resources/case12_ordering/case12-plc-2-template.yaml"
+	case12DepName                 string = "namespace-foo-setup-policy"
+	case12DepYaml                 string = "../resources/case12_ordering/case12-dependency.yaml"
+	case12DepBName                string = "namespace-foo-setup-policy-b"
+	case12DepBYaml                string = "../resources/case12_ordering/case12-dependency-b.yaml"
 )
 
 // Helper function to create events
@@ -127,7 +129,8 @@ var _ = Describe("Test dependency logic in template sync", Ordered, func() {
 			true,
 			defaultTimeoutSeconds)
 		Expect(hubPlc).NotTo(BeNil())
-		By("Generating a noncompliant event on the policy")
+
+		By("Generating a noncompliant event on the dependency")
 		generateEventOnPolicy(
 			case12DepName,
 			"managed/namespace-foo-setup-configpolicy",
@@ -149,6 +152,53 @@ var _ = Describe("Test dependency logic in template sync", Ordered, func() {
 		_, err = kubectlHub("delete", "-f", case12PolicyYaml, "-n", clusterNamespaceOnHub)
 		Expect(err).To(BeNil())
 	})
+	It("Should set to Compliant when dep status is NonCompliant and ignorePending is true", func() {
+		By("Creating a dep on hub cluster in ns:" + clusterNamespaceOnHub)
+		_, err := kubectlHub("apply", "-f", case12DepYaml, "-n", clusterNamespaceOnHub)
+		Expect(err).To(BeNil())
+		hubPlc := utils.GetWithTimeout(
+			clientHubDynamic,
+			gvrPolicy,
+			case12DepName,
+			clusterNamespaceOnHub,
+			true,
+			defaultTimeoutSeconds)
+		Expect(hubPlc).NotTo(BeNil())
+
+		By("Creating a policy on hub cluster in ns:" + clusterNamespaceOnHub)
+		_, err = kubectlHub("apply", "-f", case12PolicyIgnorePendingYaml, "-n", clusterNamespaceOnHub)
+		Expect(err).To(BeNil())
+		hubPlc = utils.GetWithTimeout(
+			clientHubDynamic,
+			gvrPolicy,
+			case12PolicyIgnorePendingName,
+			clusterNamespaceOnHub,
+			true,
+			defaultTimeoutSeconds)
+		Expect(hubPlc).NotTo(BeNil())
+
+		By("Generating a noncompliant event on the dependency")
+		generateEventOnPolicy(
+			case12DepName,
+			"managed/namespace-foo-setup-configpolicy",
+			"Warning",
+			"NonCompliant; there is violation",
+		)
+
+		By("Checking if dependency status is noncompliant")
+		Eventually(checkCompliance(case12DepName), defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))
+
+		By("Checking if policy status is pending")
+		Eventually(checkCompliance(case12PolicyIgnorePendingName), defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+
+		By("Deleting dependency on hub cluster in ns:" + clusterNamespaceOnHub)
+		_, err = kubectlHub("delete", "-f", case12DepYaml, "-n", clusterNamespaceOnHub)
+		Expect(err).To(BeNil())
+
+		By("Deleting the policy on hub cluster in ns:" + clusterNamespaceOnHub)
+		_, err = kubectlHub("delete", "-f", case12PolicyIgnorePendingYaml, "-n", clusterNamespaceOnHub)
+		Expect(err).To(BeNil())
+	})
 	It("Should set to Compliant when dep status is resolved", func() {
 		By("Creating a dep on hub cluster in ns:" + clusterNamespaceOnHub)
 		_, err := kubectlHub("apply", "-f", case12DepYaml, "-n", clusterNamespaceOnHub)

test/resources/case12_ordering/case12-plc-ignorepending.yaml

@@ -0,0 +1,38 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case12-test-policy-ignorepending
+  labels:
+    policy.open-cluster-management.io/cluster-name: managed
+    policy.open-cluster-management.io/cluster-namespace: managed
+    policy.open-cluster-management.io/root-policy: case12-test-policy
+spec:
+  remediationAction: inform
+  disabled: false
+  dependencies:
+    - apiVersion: policy.open-cluster-management.io/v1
+      kind: Policy
+      name: namespace-foo-setup-policy
+      namespace: ""
+      compliance: Compliant
+  policy-templates:
+    - ignorePending: true
+      objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case12-config-policy
+        spec:
+          remediationAction: inform
+          object-templates:
+            - complianceType: musthave
+              objectDefinition:
+                apiVersion: v1
+                kind: Pod
+                metadata:
+                  name: nginx-pod-e2e
+                  namespace: default
+                spec:
+                  containers:
+                    - name: nginx
+