Prevent unintended changes to operator name/space

Previously, if the name or namespace of the subscription was changed in
an OperatorPolicy, previously related resources would be orphaned and
cause confusion. This is especially important because these fields can
be set by templates, which could change unexpectedly.

Now, such changes will not take effect unless a special annotation,
'operatorpolicy.policy.open-cluster-management.io/namechange' is set
by the user (conventionally with a timestamp, but any new value would
suffice). When that annotation is not set, or has not been reset for a
new change, the policy will be considered invalid, and noncompliant.

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

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

api/v1beta1/operatorpolicy_types.go

@@ -204,14 +204,19 @@ type OperatorPolicyStatus struct {
 	//+optional
 	RelatedObjects []policyv1.RelatedObject `json:"relatedObjects"`
 
-	// The resolved name.namespace of the subscription
+	// The resolved name.namespace of the subscription.
 	ResolvedSubscriptionLabel string `json:"resolvedSubscriptionLabel,omitempty"`
 
+	// The value of the 'operatorpolicy.policy.open-cluster-management.io/namechange' annotation
+	// when the operator name and namespace were most recently changed. This field is used to
+	// prevent some unintentional changes, for example caused by templates.
+	ResolvedSubscriptionLock string `json:"resolvedSubscriptionLock,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"`
 
-	// Timestamp for a possible intervention to help a Subscription stuck with a
+	// The timestamp for a possible intervention to help a Subscription stuck with a
 	// ConstraintsNotSatisfiable condition. Can be in the future, indicating the
 	// policy is waiting for OLM to resolve the situation. If in the recent past,
 	// the policy may update the status of the Subscription.

controllers/operatorpolicy_controller.go

@@ -55,6 +55,8 @@ const (
 	ManagedByAnnotation    string = ManagedByLabel
 	ClusterNameLabel       string = "policy.open-cluster-management.io/cluster-name"
 	ClusterNamespaceLabel  string = "policy.open-cluster-management.io/cluster-namespace"
+	SubLockAnnotation      string = "operatorpolicy.policy.open-cluster-management.io/namechange"
+	initialLockSentinel    string = "<empty>"
 )
 
 var (
@@ -124,7 +126,9 @@ func (r *OperatorPolicyReconciler) SetupWithManager(mgr ctrl.Manager, depEvents
 		Named(OperatorControllerName).
 		For(
 			&policyv1beta1.OperatorPolicy{},
-			builder.WithPredicates(predicate.GenerationChangedPredicate{})).
+			builder.WithPredicates(predicate.Or(
+				predicate.GenerationChangedPredicate{},
+				predicate.AnnotationChangedPredicate{}))).
 		Watches(
 			&policyv1beta1.OperatorPolicy{},
 			handler.EnqueueRequestsFromMapFunc(overlapMapper)).
@@ -436,7 +440,7 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 		}
 	}
 
-	changed, overlapErr, apiErr := r.checkSubOverlap(ctx, policy, sub)
+	changed, overlapErr, apiErr := r.checkSubResolution(ctx, policy, sub)
 	if apiErr != nil && returnedErr == nil {
 		returnedErr = apiErr
 	}
@@ -455,7 +459,7 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
 	return sub, opGroup, changed, returnedErr
 }
 
-func (r *OperatorPolicyReconciler) checkSubOverlap(
+func (r *OperatorPolicyReconciler) checkSubResolution(
 	ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription,
 ) (statusChanged bool, validationErr error, apiErr error) {
 	resolvedSubLabel := ""
@@ -464,8 +468,44 @@ func (r *OperatorPolicyReconciler) checkSubOverlap(
 	}
 
 	if policy.Status.ResolvedSubscriptionLabel != resolvedSubLabel {
-		policy.Status.ResolvedSubscriptionLabel = resolvedSubLabel
-		statusChanged = true
+		// Changing the resolved subscription needs to be opted-into.
+		if policy.Status.ResolvedSubscriptionLock == "" {
+			// This is the first time subscription has been resolved (or something else has cleared
+			// this part of the status, which still counts as opting in to the change)
+			policy.Status.ResolvedSubscriptionLabel = resolvedSubLabel
+			statusChanged = true
+
+			if existingLock := policy.Annotations[SubLockAnnotation]; existingLock != "" {
+				policy.Status.ResolvedSubscriptionLock = existingLock
+			} else {
+				policy.Status.ResolvedSubscriptionLock = initialLockSentinel
+			}
+		} else {
+			// This is not the first time the subscription has been resolved - check the annotation
+			// to see if this operation should be allowed.
+			existingLock := policy.Annotations[SubLockAnnotation]
+
+			if existingLock == initialLockSentinel {
+				invalidLockErr := fmt.Errorf("value '%v' is not allowed for annotation %v - choose another value",
+					initialLockSentinel, SubLockAnnotation)
+
+				return false, invalidLockErr, nil
+			}
+
+			if existingLock != "" && existingLock != policy.Status.ResolvedSubscriptionLock {
+				// The user has provided an annotation that was not previously used - do the change
+				policy.Status.ResolvedSubscriptionLabel = resolvedSubLabel
+				policy.Status.ResolvedSubscriptionLock = existingLock
+				statusChanged = true
+			} else {
+				// The user has not opted-in to this specific change yet
+				notOptedInErr := fmt.Errorf("the resolved operator name cannot change from '%v' to '%v' without a "+
+					"new value in the %v annotation to prevent unintentional updates",
+					policy.Status.ResolvedSubscriptionLabel, resolvedSubLabel, SubLockAnnotation)
+
+				return false, notOptedInErr, nil
+			}
+		}
 	}
 
 	if resolvedSubLabel == "" {

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

@@ -377,11 +377,17 @@ spec:
                   type: object
                 type: array
               resolvedSubscriptionLabel:
-                description: The resolved name.namespace of the subscription
+                description: The resolved name.namespace of the subscription.
+                type: string
+              resolvedSubscriptionLock:
+                description: |-
+                  The value of the 'operatorpolicy.policy.open-cluster-management.io/namechange' annotation
+                  when the operator name and namespace were most recently changed. This field is used to
+                  prevent some unintentional changes, for example caused by templates.
                 type: string
               subscriptionInterventionTime:
                 description: |-
-                  Timestamp for a possible intervention to help a Subscription stuck with a
+                  The timestamp for a possible intervention to help a Subscription stuck with a
                   ConstraintsNotSatisfiable condition. Can be in the future, indicating the
                   policy is waiting for OLM to resolve the situation. If in the recent past,
                   the policy may update the status of the Subscription.

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

@@ -367,11 +367,17 @@ spec:
                   type: object
                 type: array
               resolvedSubscriptionLabel:
-                description: The resolved name.namespace of the subscription
+                description: The resolved name.namespace of the subscription.
+                type: string
+              resolvedSubscriptionLock:
+                description: |-
+                  The value of the 'operatorpolicy.policy.open-cluster-management.io/namechange' annotation
+                  when the operator name and namespace were most recently changed. This field is used to
+                  prevent some unintentional changes, for example caused by templates.
                 type: string
               subscriptionInterventionTime:
                 description: |-
-                  Timestamp for a possible intervention to help a Subscription stuck with a
+                  The timestamp for a possible intervention to help a Subscription stuck with a
                   ConstraintsNotSatisfiable condition. Can be in the future, indicating the
                   policy is waiting for OLM to resolve the situation. If in the recent past,
                   the policy may update the status of the Subscription.

test/e2e/case38_install_operator_test.go

@@ -3541,4 +3541,115 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			}, olmWaitTimeout, 1).Should(Succeed())
 		})
 	})
+	Describe("Testing changes to the operator namespace", Ordered, func() {
+		const (
+			opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
+			opPolName = "oppol-no-group"
+			subName   = "project-quay"
+		)
+
+		BeforeAll(func() {
+			preFunc()
+
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				opPolYAML, testNamespace, gvrPolicy, gvrOperatorPolicy)
+		})
+
+		It("Should initially set the sentinel value for the lock in the status", func(ctx SpecContext) {
+			Eventually(func(g Gomega) {
+				unstructPolicy, err := clientManagedDynamic.Resource(gvrOperatorPolicy).Namespace(testNamespace).
+					Get(ctx, opPolName, metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(unstructPolicy).NotTo(BeNil())
+
+				lock, _, err := unstructured.NestedString(unstructPolicy.Object, "status", "resolvedSubscriptionLock")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(lock).To(Equal("<empty>"))
+
+				label, _, err := unstructured.NestedString(unstructPolicy.Object, "status", "resolvedSubscriptionLabel")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(label).To(Equal("project-quay.operator-policy-testns"))
+			}, eventuallyTimeout, 3).Should(Succeed())
+		})
+
+		It("Should be marked as invalid if the namespace is changed", func() {
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", testNamespace, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/subscription/namespace", "value": "operator-policy-testns-two"}]`)
+
+			check(
+				opPolName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:   "ValidPolicySpec",
+					Status: metav1.ConditionFalse,
+					Reason: "InvalidPolicySpec",
+					Message: "cannot change from 'project-quay.operator-policy-testns' to " +
+						"'project-quay.operator-policy-testns-two' without a new value in the " +
+						"operatorpolicy.policy.open-cluster-management.io/namechange annotation",
+				},
+				`the resolved operator name cannot change`,
+			)
+		})
+
+		It("Should have a special message if the annotation is set to the sentinel value", func() {
+			annoPath := `/metadata/annotations/operatorpolicy.policy.open-cluster-management.io~1namechange`
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", testNamespace, "--type=json", "-p",
+				`[{"op": "add", "path": "`+annoPath+`", "value": "<empty>"}]`)
+
+			check(
+				opPolName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:    "ValidPolicySpec",
+					Status:  metav1.ConditionFalse,
+					Reason:  "InvalidPolicySpec",
+					Message: "value '<empty>' is not allowed",
+				},
+				`choose another value`,
+			)
+		})
+
+		It("Should make the change when a proper annotation is set", func(ctx SpecContext) {
+			annoPath := `/metadata/annotations/operatorpolicy.policy.open-cluster-management.io~1namechange`
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", testNamespace, "--type=json", "-p",
+				`[{"op": "add", "path": "`+annoPath+`", "value": "accepted-2024"}]`)
+
+			Eventually(func(g Gomega) {
+				unstructPolicy, err := clientManagedDynamic.Resource(gvrOperatorPolicy).Namespace(testNamespace).
+					Get(ctx, opPolName, metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(unstructPolicy).NotTo(BeNil())
+
+				lock, _, err := unstructured.NestedString(unstructPolicy.Object, "status", "resolvedSubscriptionLock")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(lock).To(Equal("accepted-2024"))
+
+				label, _, err := unstructured.NestedString(unstructPolicy.Object, "status", "resolvedSubscriptionLabel")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(label).To(Equal("project-quay.operator-policy-testns-two"))
+			}, eventuallyTimeout, 3).Should(Succeed())
+		})
+
+		It("Should be marked as invalid if the namespace is changed again", func() {
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", testNamespace, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/subscription/namespace", "value": "operator-policy-testns-three"}]`)
+
+			check(
+				opPolName,
+				true,
+				[]policyv1.RelatedObject{},
+				metav1.Condition{
+					Type:   "ValidPolicySpec",
+					Status: metav1.ConditionFalse,
+					Reason: "InvalidPolicySpec",
+					Message: "cannot change from 'project-quay.operator-policy-testns-two' to " +
+						"'project-quay.operator-policy-testns-three' without a new value in the " +
+						"operatorpolicy.policy.open-cluster-management.io/namechange annotation",
+				},
+				`the resolved operator name cannot change`,
+			)
+		})
+	})
 })