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>

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"