Fix status reporting inconsistency in mustnothave mode

Changes:
- Added new conditions and related objects to account for scenarios where resources are not handled in mustnothave mode
- Modified and refactored code for handling CatalogSource and Deployment
- Modified e2e test to account for status reporting changes

ref: https://issues.redhat.com/browse/ACM-11410
Signed-off-by: Jason Zhang <jaszhang@redhat.com>

controllers/configurationpolicy_controller.go

@@ -68,16 +68,16 @@ var (
 )
 
 const (
-	reasonWantFoundExists     = "Resource found as expected"
-	reasonWantFoundCreated    = "K8s creation success"
-	reasonUpdateSuccess       = "K8s update success"
-	reasonDeleteSuccess       = "K8s deletion success"
-	reasonWantFoundNoMatch    = "Resource found but does not match"
-	reasonWantFoundDNE        = "Resource not found but should exist"
-	reasonWantNotFoundExists  = "Resource found but should not exist"
-	reasonWantNotFoundDNE     = "Resource not found as expected"
-	reasonCleanupError        = "Error cleaning up child objects"
-	reasonIgnoreNotApplicable = "Resource found but will not be handled in mustnothave mode"
+	reasonWantFoundExists    = "Resource found as expected"
+	reasonWantFoundCreated   = "K8s creation success"
+	reasonUpdateSuccess      = "K8s update success"
+	reasonDeleteSuccess      = "K8s deletion success"
+	reasonWantFoundNoMatch   = "Resource found but does not match"
+	reasonWantFoundDNE       = "Resource not found but should exist"
+	reasonWantNotFoundExists = "Resource found but should not exist"
+	reasonWantNotFoundDNE    = "Resource not found as expected"
+	reasonCleanupError       = "Error cleaning up child objects"
+	reasonFoundNotApplicable = "Resource found but will not be handled in mustnothave mode"
 )
 
 // SetupWithManager sets up the controller with the Manager.

controllers/operatorpolicy_controller.go

@@ -1515,14 +1515,14 @@ func (r *OperatorPolicyReconciler) handleDeployment(
 	policy *policyv1beta1.OperatorPolicy,
 	csv *operatorv1alpha1.ClusterServiceVersion,
 ) (bool, error) {
-	if policy.Spec.ComplianceType.IsMustNotHave() {
-		return updateStatus(policy, notApplicableCond("Deployment")), nil
-	}
-
 	// case where csv is nil
 	if csv == nil {
 		// need to report lack of existing Deployments
-		return updateStatus(policy, noDeploymentsCond, noExistingDeploymentObj), nil
+		if policy.Spec.ComplianceType.IsMustHave() {
+			return updateStatus(policy, noDeploymentsCond, noExistingDeploymentObj), nil
+		}
+
+		return updateStatus(policy, notApplicableCond("Deployment")), nil
 	}
 
 	OpLog := ctrl.LoggerFrom(ctx)
@@ -1542,7 +1542,8 @@ func (r *OperatorPolicyReconciler) handleDeployment(
 
 		// report missing deployment in relatedObjects list
 		if foundDep == nil {
-			relatedObjects = append(relatedObjects, missingDeploymentObj(dep.Name, csv.Namespace))
+			relatedObjects = append(relatedObjects,
+				missingObj(dep.Name, csv.Namespace, policy.Spec.ComplianceType, deploymentGVK))
 
 			continue
 		}
@@ -1564,7 +1565,15 @@ func (r *OperatorPolicyReconciler) handleDeployment(
 
 		depNum++
 
-		relatedObjects = append(relatedObjects, existingDeploymentObj(&dep))
+		if policy.Spec.ComplianceType.IsMustNotHave() {
+			relatedObjects = append(relatedObjects, foundNotApplicableObj(&dep))
+		} else {
+			relatedObjects = append(relatedObjects, existingDeploymentObj(&dep))
+		}
+	}
+
+	if policy.Spec.ComplianceType.IsMustNotHave() {
+		return updateStatus(policy, notApplicableCond("Deployment"), relatedObjects...), nil
 	}
 
 	return updateStatus(policy, buildDeploymentCond(depNum > 0, unavailableDeployments), relatedObjects...), nil
@@ -1668,18 +1677,20 @@ func (r *OperatorPolicyReconciler) handleCatalogSource(
 	policy *policyv1beta1.OperatorPolicy,
 	subscription *operatorv1alpha1.Subscription,
 ) (bool, error) {
-	if policy.Spec.ComplianceType.IsMustNotHave() {
-		cond := notApplicableCond("CatalogSource")
-		cond.Status = metav1.ConditionFalse // CatalogSource condition has the opposite polarity
-
-		return updateStatus(policy, cond), nil
-	}
-
 	watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
 	if subscription == nil {
 		// Note: existing related objects will not be removed by this status update
-		return updateStatus(policy, invalidCausingUnknownCond("CatalogSource")), nil
+		if policy.Spec.ComplianceType.IsMustHave() {
+			return updateStatus(policy, invalidCausingUnknownCond("CatalogSource")), nil
+		}
+
+		// CatalogSource may be available
+		// related objects will remain the same to report last known state
+		cond := notApplicableCond("CatalogSource")
+		cond.Status = metav1.ConditionFalse
+
+		return updateStatus(policy, cond), nil
 	}
 
 	catalogName := subscription.Spec.CatalogSource
@@ -1692,22 +1703,64 @@ func (r *OperatorPolicyReconciler) handleCatalogSource(
 		return false, fmt.Errorf("error getting CatalogSource: %w", err)
 	}
 
-	isMissing := foundCatalogSrc == nil
-	isUnhealthy := isMissing
+	var catalogSrc *operatorv1alpha1.CatalogSource
 
-	if !isMissing {
+	if foundCatalogSrc != nil {
 		// CatalogSource is found, initiate health check
-		catalogSrc := new(operatorv1alpha1.CatalogSource)
+		catalogSrc = new(operatorv1alpha1.CatalogSource)
 
 		err := runtime.DefaultUnstructuredConverter.
 			FromUnstructured(foundCatalogSrc.Object, catalogSrc)
 		if err != nil {
 			return false, fmt.Errorf("error converting the retrieved CatalogSource to the Go type: %w", err)
 		}
+	}
+
+	if policy.Spec.ComplianceType.IsMustNotHave() {
+		return r.mustnothaveCatalogSource(policy, catalogSrc, catalogName, catalogNS)
+	}
+
+	return r.musthaveCatalogSource(policy, catalogSrc, catalogName, catalogNS)
+}
+
+func (r *OperatorPolicyReconciler) mustnothaveCatalogSource(
+	policy *policyv1beta1.OperatorPolicy,
+	catalogSrc *operatorv1alpha1.CatalogSource,
+	catalogName string,
+	catalogNS string,
+) (bool, error) {
+	var relObj policyv1.RelatedObject
+
+	cond := notApplicableCond("CatalogSource")
+	cond.Status = metav1.ConditionFalse // CatalogSource condition has the opposite polarity
 
+	if catalogSrc == nil {
+		relObj = missingObj(catalogName, catalogNS, policyv1.MustNotHave, catalogSrcGVK)
+	} else {
+		relObj = foundNotApplicableObj(catalogSrc)
+	}
+
+	return updateStatus(policy, cond, relObj), nil
+}
+
+func (r *OperatorPolicyReconciler) musthaveCatalogSource(
+	policy *policyv1beta1.OperatorPolicy,
+	catalogSrc *operatorv1alpha1.CatalogSource,
+	catalogName string,
+	catalogNS string,
+) (bool, error) {
+	isMissing := catalogSrc == nil
+	isUnhealthy := isMissing
+
+	if catalogSrc != nil {
+		// CatalogSource is found, initiate health check
 		if catalogSrc.Status.GRPCConnectionState == nil {
 			// Unknown State
-			changed := updateStatus(policy, catalogSourceUnknownCond, catalogSrcUnknownObj(catalogName, catalogNS))
+			changed := updateStatus(
+				policy,
+				catalogSourceUnknownCond,
+				catalogSrcUnknownObj(catalogSrc.Name, catalogSrc.Namespace),
+			)
 
 			return changed, nil
 		}

controllers/operatorpolicy_status.go

@@ -12,6 +12,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
@@ -856,19 +857,55 @@ func foundNotWantedObj(obj client.Object) policyv1.RelatedObject {
 	}
 }
 
-// foundNotWantedIpObj returns a Compliant RelatedObject with
+// foundNotApplicableObj returns a Compliant RelatedObject with
 // reason = 'Resource found but will not be handled in mustnothave mode'
 func foundNotApplicableObj(obj client.Object) policyv1.RelatedObject {
 	return policyv1.RelatedObject{
 		Object:    policyv1.ObjectResourceFromObj(obj),
 		Compliant: string(policyv1.Compliant),
-		Reason:    reasonIgnoreNotApplicable,
+		Reason:    reasonFoundNotApplicable,
 		Properties: &policyv1.ObjectProperties{
 			UID: string(obj.GetUID()),
 		},
 	}
 }
 
+// missingObj returns a conditionally Compliant RelatedObject with
+// reason = "Resource not found but should exist"/"Resource not found as expected"
+// based on the complianceType specified by the policy
+func missingObj(
+	name string,
+	namespace string,
+	complianceType policyv1.ComplianceType,
+	gvk schema.GroupVersionKind,
+) policyv1.RelatedObject {
+	var compliance policyv1.ComplianceState
+	var reason string
+
+	if complianceType.IsMustHave() {
+		compliance = policyv1.NonCompliant
+		reason = reasonWantFoundDNE
+	} else {
+		// Non-Applicables are not handled by controller
+		// However if the're not found -> report NA or report not found as expected?
+		compliance = policyv1.Compliant
+		reason = reasonWantNotFoundDNE
+	}
+
+	return policyv1.RelatedObject{
+		Object: policyv1.ObjectResource{
+			Kind:       gvk.Kind,
+			APIVersion: gvk.GroupVersion().String(),
+			Metadata: policyv1.ObjectMetadata{
+				Name:      name,
+				Namespace: namespace,
+			},
+		},
+		Compliant: string(compliance),
+		Reason:    reason,
+	}
+}
+
 // createdObj returns a Compliant RelatedObject with reason = 'K8s creation success'
 func createdObj(obj client.Object) policyv1.RelatedObject {
 	created := true
@@ -1116,23 +1153,6 @@ var noExistingCRDObj = policyv1.RelatedObject{
 	Reason:    "No relevant CustomResourceDefinitions found",
 }
 
-// missingDeploymentObj returns a NonCompliant RelatedObject for a Deployment,
-// with Reason 'Resource not found but should exist'
-func missingDeploymentObj(name string, namespace string) policyv1.RelatedObject {
-	return policyv1.RelatedObject{
-		Object: policyv1.ObjectResource{
-			Kind:       deploymentGVK.Kind,
-			APIVersion: deploymentGVK.GroupVersion().String(),
-			Metadata: policyv1.ObjectMetadata{
-				Name:      name,
-				Namespace: namespace,
-			},
-		},
-		Compliant: string(policyv1.NonCompliant),
-		Reason:    reasonWantFoundDNE,
-	}
-}
-
 // existingDeploymentObj returns a RelatedObject for a Deployment, which will
 // be Compliant if there are no unavailable replicas on the deployment.
 func existingDeploymentObj(dep *appsv1.Deployment) policyv1.RelatedObject {