Check and report on overlapping subs

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"