Check and report on overlapping subs

This adds two new status fields to OperatorPolicy, which are used to
detect when two OperatorPolicies manage the same subscription.
Overlapping OperatorPolicies will go into a non-compliant state, report
the overlap, and will not compete with each other.

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

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

api/v1beta1/operatorpolicy_types.go

@@ -148,6 +148,13 @@ type OperatorPolicyStatus struct {
 	// List of resources processed by the policy
 	// +optional
 	RelatedObjects []policyv1.RelatedObject `json:"relatedObjects"`
+
+	// The resolved name.namespace of the subscription
+	ResolvedSubscriptionLabel string `json:"resolvedSubscriptionLabel,omitempty"`
+
+	// The list of overlapping OperatorPolicies (as name.namespace) which all manage the same
+	// subscription, including this policy. When no overlapping is detected, this list will be empty.
+	OverlappingPolicies []string `json:"overlappingPolicies,omitempty"`
 }
 
 func (status OperatorPolicyStatus) RelatedObjsOfKind(kind string) map[int]policyv1.RelatedObject {

controllers/operatorpolicy_controller.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/dynamic"
@@ -114,12 +115,42 @@ func (r *OperatorPolicyReconciler) SetupWithManager(mgr ctrl.Manager, depEvents
 		For(
 			&policyv1beta1.OperatorPolicy{},
 			builder.WithPredicates(predicate.GenerationChangedPredicate{})).
+		Watches(
+			&policyv1beta1.OperatorPolicy{},
+			handler.EnqueueRequestsFromMapFunc(overlapMapper)).
 		WatchesRawSource(
 			depEvents,
 			&handler.EnqueueRequestForObject{}).
 		Complete(r)
 }
 
+func overlapMapper(_ context.Context, obj client.Object) []reconcile.Request {
+	//nolint:forcetypeassert
+	pol := obj.(*policyv1beta1.OperatorPolicy)
+
+	var result []reconcile.Request
+
+	for _, overlap := range pol.Status.OverlappingPolicies {
+		name, ns, ok := strings.Cut(overlap, ".")
+		// skip invalid items in the status
+		if !ok {
+			continue
+		}
+
+		// skip 'this' policy; it will be reconciled (if needed) through another watch
+		if name == pol.Name && ns == pol.Namespace {
+			continue
+		}
+
+		result = append(result, reconcile.Request{NamespacedName: types.NamespacedName{
+			Name:      name,
+			Namespace: ns,
+		}})
+	}
+
+	return result
+}
+
 // blank assignment to verify that OperatorPolicyReconciler implements reconcile.Reconciler
 var _ reconcile.Reconciler = &OperatorPolicyReconciler{}
 
@@ -308,7 +339,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 // checks if the policy's spec is valid. It returns:
 //   - the built Subscription
 //   - the built OperatorGroup
-//   - whether the status has changed because of the validity condition
+//   - whether the status has changed
 //   - an error if an API call failed
 //
 // The built objects can be used to find relevant objects for a 'mustnothave' policy.
@@ -376,7 +407,86 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 		}
 	}
 
-	return sub, opGroup, updateStatus(policy, validationCond(validationErrors)), returnedErr
+	changed, overlapErr, apiErr := r.checkSubOverlap(ctx, policy, sub)
+	if apiErr != nil && returnedErr == nil {
+		returnedErr = apiErr
+	}
+
+	if overlapErr != nil {
+		// When an overlap is detected, the generated subscription and operatorgroup
+		// will be considered to be invalid to prevent creations/updates.
+		sub = nil
+		opGroup = nil
+
+		validationErrors = append(validationErrors, overlapErr)
+	}
+
+	changed = updateStatus(policy, validationCond(validationErrors)) || changed
+
+	return sub, opGroup, changed, returnedErr
+}
+
+func (r *OperatorPolicyReconciler) checkSubOverlap(
+	ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription,
+) (statusChanged bool, validationErr error, apiErr error) {
+	resolvedSubLabel := ""
+	if sub != nil {
+		resolvedSubLabel = sub.Name + "." + sub.Namespace
+	}
+
+	if policy.Status.ResolvedSubscriptionLabel != resolvedSubLabel {
+		policy.Status.ResolvedSubscriptionLabel = resolvedSubLabel
+		statusChanged = true
+	}
+
+	if resolvedSubLabel == "" {
+		// No possible overlap if the subscription could not be determined
+		if len(policy.Status.OverlappingPolicies) != 0 {
+			policy.Status.OverlappingPolicies = []string{}
+			statusChanged = true
+		}
+
+		return statusChanged, nil, nil
+	}
+
+	opList := &policyv1beta1.OperatorPolicyList{}
+	if err := r.List(ctx, opList); err != nil {
+		return statusChanged, nil, err
+	}
+
+	// In the list, 'this' policy may or may not have the sub label yet, so always
+	// put it in here, and skip it in the loop.
+	overlappers := []string{policy.Name + "." + policy.Namespace}
+
+	for _, otherPolicy := range opList.Items {
+		if otherPolicy.Status.ResolvedSubscriptionLabel == resolvedSubLabel {
+			if !(otherPolicy.Name == policy.Name && otherPolicy.Namespace == policy.Namespace) {
+				overlappers = append(overlappers, otherPolicy.Name+"."+otherPolicy.Namespace)
+			}
+		}
+	}
+
+	// No overlap
+	if len(overlappers) == 1 {
+		if len(policy.Status.OverlappingPolicies) != 0 {
+			policy.Status.OverlappingPolicies = []string{}
+			statusChanged = true
+		}
+
+		return statusChanged, nil, nil
+	}
+
+	slices.Sort(overlappers)
+
+	overlapError := fmt.Errorf("the specified operator is managed by multiple policies (%v)",
+		strings.Join(overlappers, ", "))
+
+	if !slices.Equal(overlappers, policy.Status.OverlappingPolicies) {
+		policy.Status.OverlappingPolicies = overlappers
+		statusChanged = true
+	}
+
+	return statusChanged, overlapError, nil
 }
 
 // applySubscriptionDefaults will set the subscription channel, source, and sourceNamespace when they are unset by

deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -220,6 +220,13 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              overlappingPolicies:
+                description: |-
+                  The list of overlapping OperatorPolicies (as name.namespace) which all manage the same
+                  subscription, including this policy. When no overlapping is detected, this list will be empty.
+                items:
+                  type: string
+                type: array
               relatedObjects:
                 description: List of resources processed by the policy
                 items:
@@ -270,6 +277,9 @@ spec:
                       type: string
                   type: object
                 type: array
+              resolvedSubscriptionLabel:
+                description: The resolved name.namespace of the subscription
+                type: string
             type: object
         type: object
     served: true

deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -215,6 +215,13 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              overlappingPolicies:
+                description: |-
+                  The list of overlapping OperatorPolicies (as name.namespace) which all manage the same
+                  subscription, including this policy. When no overlapping is detected, this list will be empty.
+                items:
+                  type: string
+                type: array
               relatedObjects:
                 description: List of resources processed by the policy
                 items:
@@ -265,6 +272,9 @@ spec:
                       type: string
                   type: object
                 type: array
+              resolvedSubscriptionLabel:
+                description: The resolved name.namespace of the subscription
+                type: string
             type: object
         type: object
     served: true

test/e2e/case38_install_operator_test.go

@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"regexp"
 	"slices"
+	"strconv"
 	"strings"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -2954,6 +2955,157 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			Expect(remBehavior).To(HaveKeyWithValue("customResourceDefinitions", "Keep"))
 		})
 	})
+	Describe("Testing operator policies that specify the same subscription", Ordered, func() {
+		const (
+			musthaveYAML    = "../resources/case38_operator_install/operator-policy-no-group.yaml"
+			musthaveName    = "oppol-no-group"
+			mustnothaveYAML = "../resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml"
+			mustnothaveName = "oppol-mustnothave"
+		)
+
+		BeforeAll(func() {
+			preFunc()
+
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				musthaveYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				mustnothaveYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+		})
+
+		It("Should display a validation error in both", func() {
+			check(
+				mustnothaveName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:   "ValidPolicySpec",
+					Status: metav1.ConditionFalse,
+					Reason: "InvalidPolicySpec",
+					Message: `the specified operator is managed by multiple policies ` +
+						`(oppol-mustnothave.operator-policy-testns, oppol-no-group.operator-policy-testns)`,
+				},
+				`the specified operator is managed by multiple policies`,
+			)
+			check(
+				musthaveName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:   "ValidPolicySpec",
+					Status: metav1.ConditionFalse,
+					Reason: "InvalidPolicySpec",
+					Message: `the specified operator is managed by multiple policies ` +
+						`(oppol-mustnothave.operator-policy-testns, oppol-no-group.operator-policy-testns)`,
+				},
+				`the specified operator is managed by multiple policies`,
+			)
+		})
+
+		// This test requires that no other OperatorPolicies are active. Ideally it would be marked
+		// with the Serial decorator, but then this whole file would need to be marked that way,
+		// which would slow down the suite. As long as no other test files use OperatorPolicy, the
+		// Ordered property on this file should ensure this is stable.
+		It("Should not cause an infinite reconcile loop", func() {
+			recMetrics := utils.GetMetrics("controller_runtime_reconcile_total",
+				`controller=\"operator-policy-controller\"`)
+
+			totalReconciles := 0
+			for _, metric := range recMetrics {
+				val, err := strconv.Atoi(metric)
+				Expect(err).NotTo(HaveOccurred())
+
+				totalReconciles += val
+			}
+
+			Consistently(func(g Gomega) int {
+				loopMetrics := utils.GetMetrics("controller_runtime_reconcile_total",
+					`controller=\"operator-policy-controller\"`)
+
+				loopReconciles := 0
+				for _, metric := range loopMetrics {
+					val, err := strconv.Atoi(metric)
+					g.Expect(err).NotTo(HaveOccurred())
+
+					loopReconciles += val
+				}
+
+				return loopReconciles
+			}, "10s", "1s").Should(Equal(totalReconciles))
+		})
+
+		It("Should remove the validation error when the overlapping policy is removed", func() {
+			utils.Kubectl("delete", "operatorpolicy", musthaveName, "-n", opPolTestNS)
+
+			check(
+				mustnothaveName,
+				false,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:    "ValidPolicySpec",
+					Status:  metav1.ConditionTrue,
+					Reason:  "PolicyValidated",
+					Message: `the policy spec is valid`,
+				},
+				`the policy spec is valid`,
+			)
+		})
+
+		// This test requires that no other OperatorPolicies are active. Ideally it would be marked
+		// with the Serial decorator, but then this whole file would need to be marked that way,
+		// which would slow down the suite. As long as no other test files use OperatorPolicy, the
+		// Ordered property on this file should ensure this is stable.
+		It("Should not cause an infinite reconcile loop when enforced", func() {
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				musthaveYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+
+			// enforce the policies
+			utils.Kubectl("patch", "operatorpolicy", mustnothaveName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`)
+			utils.Kubectl("patch", "operatorpolicy", musthaveName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`)
+
+			check(
+				mustnothaveName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:   "ValidPolicySpec",
+					Status: metav1.ConditionFalse,
+					Reason: "InvalidPolicySpec",
+					Message: `the specified operator is managed by multiple policies ` +
+						`(oppol-mustnothave.operator-policy-testns, oppol-no-group.operator-policy-testns)`,
+				},
+				`the specified operator is managed by multiple policies`,
+			)
+
+			recMetrics := utils.GetMetrics("controller_runtime_reconcile_total",
+				`controller=\"operator-policy-controller\"`)
+
+			totalReconciles := 0
+			for _, metric := range recMetrics {
+				val, err := strconv.Atoi(metric)
+				Expect(err).NotTo(HaveOccurred())
+
+				totalReconciles += val
+			}
+
+			Consistently(func(g Gomega) int {
+				loopMetrics := utils.GetMetrics("controller_runtime_reconcile_total",
+					`controller=\"operator-policy-controller\"`)
+
+				loopReconciles := 0
+				for _, metric := range loopMetrics {
+					val, err := strconv.Atoi(metric)
+					g.Expect(err).NotTo(HaveOccurred())
+
+					loopReconciles += val
+				}
+
+				return loopReconciles
+			}, "10s", "1s").Should(Equal(totalReconciles))
+		})
+	})
 	Describe("Testing templates in an OperatorPolicy", Ordered, func() {
 		const (
 			opPolYAML     = "../resources/case38_operator_install/operator-policy-with-templates.yaml"