Prevent subscription creation if opgroup incorrect

Previously, the subscription would be created by an enforced musthave
OperatorPolicy regardless of the status of the OperatorGroup. But this
could lead to a bad result where the operator is running in the wrong
mode because it is not using the correct operatorgroup. While the policy
was correctly non-compliant in this case, it should not create the
subscription.

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

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit 414cd18)

controllers/operatorpolicy_controller.go

@@ -233,7 +233,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 		desiredSubName = desiredSub.Name
 	}
 
-	earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
+	ogCorrect, earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
 	earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
 	condChanged = condChanged || changed
 
@@ -243,7 +243,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 		return earlyComplianceEvents, condChanged, err
 	}
 
-	subscription, earlyConds, changed, err := r.handleSubscription(ctx, policy, desiredSub)
+	subscription, earlyConds, changed, err := r.handleSubscription(ctx, policy, desiredSub, ogCorrect)
 	earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
 	condChanged = condChanged || changed
 
@@ -620,40 +620,43 @@ func (r *OperatorPolicyReconciler) handleOpGroup(
 	policy *policyv1beta1.OperatorPolicy,
 	desiredOpGroup *operatorv1.OperatorGroup,
 	desiredSubName string,
-) ([]metav1.Condition, bool, error) {
+) (bool, []metav1.Condition, bool, error) {
 	watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
 	if desiredOpGroup == nil || desiredOpGroup.Namespace == "" {
 		// Note: existing related objects will not be removed by this status update
-		return nil, updateStatus(policy, invalidCausingUnknownCond("OperatorGroup")), nil
+		return false, nil, updateStatus(policy, invalidCausingUnknownCond("OperatorGroup")), nil
 	}
 
 	foundOpGroups, err := r.DynamicWatcher.List(
 		watcher, operatorGroupGVK, desiredOpGroup.Namespace, labels.Everything())
 	if err != nil {
-		return nil, false, fmt.Errorf("error listing OperatorGroups: %w", err)
+		return false, nil, false, fmt.Errorf("error listing OperatorGroups: %w", err)
 	}
 
 	if policy.Spec.ComplianceType.IsMustHave() {
 		return r.musthaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
 	}
 
-	return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)
+	earlyConds, changed, err := r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)
+
+	// In mustnothave mode, we can always act as if the OperatorGroup is not correct
+	return false, earlyConds, changed, err
 }
 
 func (r *OperatorPolicyReconciler) musthaveOpGroup(
 	ctx context.Context,
 	policy *policyv1beta1.OperatorPolicy,
 	desiredOpGroup *operatorv1.OperatorGroup,
 	foundOpGroups []unstructured.Unstructured,
-) ([]metav1.Condition, bool, error) {
+) (bool, []metav1.Condition, bool, error) {
 	switch len(foundOpGroups) {
 	case 0:
 		// Missing OperatorGroup: report NonCompliance
 		changed := updateStatus(policy, missingWantedCond("OperatorGroup"), missingWantedObj(desiredOpGroup))
 
 		if policy.Spec.RemediationAction.IsInform() {
-			return nil, changed, nil
+			return false, nil, changed, nil
 		}
 
 		earlyConds := []metav1.Condition{}
@@ -664,15 +667,15 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 
 		err := r.createWithNamespace(ctx, policy, desiredOpGroup)
 		if err != nil {
-			return nil, changed, fmt.Errorf("error creating the OperatorGroup: %w", err)
+			return false, nil, changed, fmt.Errorf("error creating the OperatorGroup: %w", err)
 		}
 
 		desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Create stripped this information
 
 		// Now the OperatorGroup should match, so report Compliance
 		updateStatus(policy, createdCond("OperatorGroup"), createdObj(desiredOpGroup))
 
-		return earlyConds, true, nil
+		return true, earlyConds, true, nil
 	case 1:
 		opGroup := foundOpGroups[0]
 
@@ -685,8 +688,8 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 				// The policy doesn't specify what the OperatorGroup should look like, but what is already
 				// there is not the default one the policy would create.
 				// FUTURE: check if the one operator group is compatible with the desired subscription.
-				// For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
-				return nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
+				// For now, assume if an OperatorGroup already exists, then it's a good one.
+				return true, nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
 			}
 
 			// There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
@@ -695,25 +698,25 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 			missing := missingWantedObj(desiredOpGroup)
 			badExisting := mismatchedObj(&opGroup)
 
-			return nil, updateStatus(policy, mismatchCond("OperatorGroup"), missing, badExisting), nil
+			return false, nil, updateStatus(policy, mismatchCond("OperatorGroup"), missing, badExisting), nil
 		}
 
 		// check whether the specs match
 		desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
 		if err != nil {
-			return nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
+			return false, nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
 		}
 
 		updateNeeded, skipUpdate, err := r.mergeObjects(
 			ctx, desiredUnstruct, &opGroup, string(policy.Spec.ComplianceType),
 		)
 		if err != nil {
-			return nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
+			return false, nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
 		}
 
 		if !updateNeeded {
 			// Everything relevant matches!
-			return nil, updateStatus(policy, matchesCond("OperatorGroup"), matchedObj(&opGroup)), nil
+			return true, nil, updateStatus(policy, matchesCond("OperatorGroup"), matchedObj(&opGroup)), nil
 		}
 
 		// Specs don't match.
@@ -722,19 +725,21 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 			// The policy doesn't specify what the OperatorGroup should look like, but what is already
 			// there is not the default one the policy would create.
 			// FUTURE: check if the one operator group is compatible with the desired subscription.
-			// For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
-			return nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
+			// For now, assume if an OperatorGroup already exists, then it's a good one.
+			return true, nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
 		}
 
 		if policy.Spec.RemediationAction.IsEnforce() && skipUpdate {
-			return nil, updateStatus(policy, mismatchCondUnfixable("OperatorGroup"), mismatchedObj(&opGroup)), nil
+			changed := updateStatus(policy, mismatchCondUnfixable("OperatorGroup"), mismatchedObj(&opGroup))
+
+			return false, nil, changed, nil
 		}
 
 		// The names match, but the specs don't: report NonCompliance
 		changed := updateStatus(policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))
 
 		if policy.Spec.RemediationAction.IsInform() {
-			return nil, changed, nil
+			return false, nil, changed, nil
 		}
 
 		earlyConds := []metav1.Condition{}
@@ -747,18 +752,18 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 
 		err = r.Update(ctx, &opGroup)
 		if err != nil {
-			return nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
+			return false, nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
 		}
 
 		desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information
 
 		updateStatus(policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))
 
-		return earlyConds, true, nil
+		return true, earlyConds, true, nil
 	default:
 		// This situation will always lead to a "TooManyOperatorGroups" failure on the CSV.
 		// Consider improving this in the future: perhaps this could suggest one of the OperatorGroups to keep.
-		return nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(foundOpGroups)...), nil
+		return false, nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(foundOpGroups)...), nil
 	}
 }
 
@@ -912,7 +917,10 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
 }
 
 func (r *OperatorPolicyReconciler) handleSubscription(
-	ctx context.Context, policy *policyv1beta1.OperatorPolicy, desiredSub *operatorv1alpha1.Subscription,
+	ctx context.Context,
+	policy *policyv1beta1.OperatorPolicy,
+	desiredSub *operatorv1alpha1.Subscription,
+	ogCorrect bool,
 ) (*operatorv1alpha1.Subscription, []metav1.Condition, bool, error) {
 	watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
@@ -927,7 +935,7 @@ func (r *OperatorPolicyReconciler) handleSubscription(
 	}
 
 	if policy.Spec.ComplianceType.IsMustHave() {
-		return r.musthaveSubscription(ctx, policy, desiredSub, foundSub)
+		return r.musthaveSubscription(ctx, policy, desiredSub, foundSub, ogCorrect)
 	}
 
 	return r.mustnothaveSubscription(ctx, policy, desiredSub, foundSub)
@@ -938,12 +946,14 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
 	policy *policyv1beta1.OperatorPolicy,
 	desiredSub *operatorv1alpha1.Subscription,
 	foundSub *unstructured.Unstructured,
+	ogCorrect bool,
 ) (*operatorv1alpha1.Subscription, []metav1.Condition, bool, error) {
 	if foundSub == nil {
 		// Missing Subscription: report NonCompliance
 		changed := updateStatus(policy, missingWantedCond("Subscription"), missingWantedObj(desiredSub))
 
-		if policy.Spec.RemediationAction.IsInform() {
+		// If informing, or if the OperatorGroup is not correct, don't make any changes
+		if policy.Spec.RemediationAction.IsInform() || !ogCorrect {
 			return desiredSub, nil, changed, nil
 		}
 

test/e2e/case38_install_operator_test.go

@@ -590,9 +590,11 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 	})
 	Describe("Testing Subscription behavior for musthave mode while enforcing", Ordered, func() {
 		const (
-			opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
-			opPolName = "oppol-no-group"
-			subName   = "project-quay"
+			opPolYAML        = "../resources/case38_operator_install/operator-policy-with-group.yaml"
+			opPolName        = "oppol-with-group"
+			subName          = "project-quay"
+			extraOpGroupYAML = "../resources/case38_operator_install/extra-operator-group.yaml"
+			extraOpGroupName = "extra-operator-group"
 		)
 
 		BeforeAll(func() {
@@ -601,6 +603,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 				utils.Kubectl("delete", "ns", opPolTestNS)
 			})
 
+			utils.Kubectl("apply", "-f", extraOpGroupYAML, "-n", opPolTestNS)
+
 			createObjWithParent(parentPolicyYAML, parentPolicyName,
 				opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
 		})
@@ -630,9 +634,70 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 				"the Subscription required by the policy was not found",
 			)
 		})
-		It("Should create the Subscription when enforced", func() {
+		It("Should not create the Subscription when another OperatorGroup already exists", func() {
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
 				`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`)
+			check(
+				opPolName,
+				true,
+				[]policyv1.RelatedObject{{
+					Object: policyv1.ObjectResource{
+						Kind:       "OperatorGroup",
+						APIVersion: "operators.coreos.com/v1",
+						Metadata: policyv1.ObjectMetadata{
+							Name:      extraOpGroupName,
+							Namespace: opPolTestNS,
+						},
+					},
+					Compliant: "NonCompliant",
+					Reason:    "Resource found but does not match",
+				}, {
+					Object: policyv1.ObjectResource{
+						Kind:       "OperatorGroup",
+						APIVersion: "operators.coreos.com/v1",
+						Metadata: policyv1.ObjectMetadata{
+							Name:      "scoped-operator-group",
+							Namespace: opPolTestNS,
+						},
+					},
+					Compliant: "NonCompliant",
+					Reason:    "Resource not found but should exist",
+				}},
+				metav1.Condition{
+					Type:    "OperatorGroupCompliant",
+					Status:  metav1.ConditionFalse,
+					Reason:  "OperatorGroupMismatch",
+					Message: "the OperatorGroup found on the cluster does not match the policy",
+				},
+				"the OperatorGroup found on the cluster does not match the policy",
+			)
+			check(
+				opPolName,
+				true,
+				[]policyv1.RelatedObject{{
+					Object: policyv1.ObjectResource{
+						Kind:       "Subscription",
+						APIVersion: "operators.coreos.com/v1alpha1",
+						Metadata: policyv1.ObjectMetadata{
+							Name:      subName,
+							Namespace: opPolTestNS,
+						},
+					},
+					Compliant: "NonCompliant",
+					Reason:    "Resource not found but should exist",
+				}},
+				metav1.Condition{
+					Type:    "SubscriptionCompliant",
+					Status:  metav1.ConditionFalse,
+					Reason:  "SubscriptionMissing",
+					Message: "the Subscription required by the policy was not found",
+				},
+				"the Subscription required by the policy was not found",
+			)
+		})
+
+		It("Should create the Subscription after the additional OperatorGroup is removed", func() {
+			utils.Kubectl("delete", "operatorgroup", extraOpGroupName, "-n", opPolTestNS)
 			check(
 				opPolName,
 				false,
@@ -2895,7 +2960,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 			}, consistentlyDuration, 3, ctx).ShouldNot(BeEmpty())
 		})
 	})
-	Describe("Testing defaulted values in an OperatorPolicy", func() {
+	Describe("Testing defaulted values of removalBehavior in an OperatorPolicy", func() {
 		const (
 			opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
 			opPolName = "oppol-no-group"