Specially handle some OperatorPolicy fields

The optional `config` field in a Subscription, and the optional
`targetNamespaces` and `selector` fields in an OperatorGroup often need
to exactly match what the user has specified, rather than being merged
with what might already be there. That is, when they are specified, it
is usually better to treat them with `mustonlyhave` semantics.

Previously, if a user adjusted the `targetNamespaces` to change it from
one namespace to another (maybe they had a typo originally), the
existing list and the new list would be merged, and the operator would
likely not deploy as intended. Similarly, if a user had a nodeSelector
in their `config`, adjustments would often result in a selector that did
not match any nodes.

Now, when one of those specific fields is set, it is handled better.
When the field is not set, the existing configuration is still used.

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

Signed-off-by: Justin Kulikauskas <[email protected]>

Author: JustinKuli

controllers/operatorpolicy_controller.go

@@ -1138,13 +1138,7 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 			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 false, nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
-		}
-
-		updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, &opGroup, policy.Spec.ComplianceType)
+		updateNeeded, skipUpdate, err := r.mergeOpGroups(ctx, desiredOpGroup, &opGroup)
 		if err != nil {
 			return false, nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
 		}
@@ -1420,18 +1414,7 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
 	}
 
 	// Subscription found; check if specs match
-	desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)
-	if err != nil {
-		return nil, nil, false, fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
-	}
-
-	// Clear `installPlanApproval` from the desired subscription when in inform mode - since that field can not
-	// be set in the policy, we should not check it on the object in the cluster.
-	if policy.Spec.RemediationAction.IsInform() {
-		unstructured.RemoveNestedField(desiredUnstruct, "spec", "installPlanApproval")
-	}
-
-	updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, foundSub, policy.Spec.ComplianceType)
+	updateNeeded, skipUpdate, err := r.mergeSubscriptions(ctx, desiredSub, foundSub, policy.Spec.RemediationAction)
 	if err != nil {
 		return nil, nil, false, fmt.Errorf("error checking if the Subscription needs an update: %w", err)
 	}
@@ -2857,14 +2840,118 @@ func opPolIdentifier(namespace, name string) depclient.ObjectIdentifier {
 	}
 }
 
+func (r *OperatorPolicyReconciler) mergeOpGroups(
+	ctx context.Context,
+	desired *operatorv1.OperatorGroup,
+	existing *unstructured.Unstructured,
+) (updateNeeded, updateIsForbidden bool, err error) {
+	forceUpdate := false
+
+	desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desired)
+	if err != nil {
+		return false, false, fmt.Errorf("unable to convert desired OperatorGroup to an Unstructured: %w", err)
+	}
+
+	// when specified, manually handle targetNamespaces as if it's "mustonlyhave"
+	if len(desired.Spec.TargetNamespaces) > 0 {
+		desiredTarget, _, err := unstructured.NestedStringSlice(desiredUnstruct, "spec", "targetNamespaces")
+		if err != nil {
+			return false, false, fmt.Errorf("unable to get targetNamespaces from desired OperatorGroup: %w", err)
+		}
+
+		existingTarget, found, err := unstructured.NestedStringSlice(existing.Object, "spec", "targetNamespaces")
+		if err != nil {
+			return false, false, fmt.Errorf("unable to get targetNamespaces from existing OperatorGroup: %w", err)
+		} else if !found {
+			forceUpdate = true
+			unstructured.SetNestedStringSlice(existing.Object,
+				desired.Spec.TargetNamespaces, "spec", "targetNamespaces")
+		} else if eq, _ := deeplyEquivalent(desiredTarget, existingTarget, true); !eq {
+			forceUpdate = true
+			unstructured.SetNestedStringSlice(existing.Object,
+				desired.Spec.TargetNamespaces, "spec", "targetNamespaces")
+		}
+	}
+
+	usesExpressions := desired.Spec.Selector != nil && len(desired.Spec.Selector.MatchExpressions) > 0
+	usesLabels := desired.Spec.Selector != nil && len(desired.Spec.Selector.MatchLabels) > 0
+
+	// when specified, manually handle selector as if it's "mustonlyhave"
+	if usesExpressions || usesLabels {
+		desiredSelector, _, err := unstructured.NestedMap(desiredUnstruct, "spec", "selector")
+		if err != nil {
+			return false, false, fmt.Errorf("unable to get selector from desired OperatorGroup: %w", err)
+		}
+
+		existingSelector, found, err := unstructured.NestedMap(existing.Object, "spec", "selector")
+		if err != nil {
+			return false, false, fmt.Errorf("unable to get selector from existing OperatorGroup: %w", err)
+		} else if !found {
+			forceUpdate = true
+			unstructured.SetNestedMap(existing.Object, desiredSelector, "spec", "selector")
+		} else if eq, _ := deeplyEquivalent(desiredSelector, existingSelector, true); !eq {
+			forceUpdate = true
+			unstructured.SetNestedMap(existing.Object, desiredSelector, "spec", "selector")
+		}
+	}
+
+	updateNeeded, forbidden, err := r.mergeObjects(ctx, desiredUnstruct, existing)
+
+	return updateNeeded || forceUpdate, forbidden, err
+}
+
+func (r *OperatorPolicyReconciler) mergeSubscriptions(
+	ctx context.Context,
+	desired *operatorv1alpha1.Subscription,
+	existing *unstructured.Unstructured,
+	action policyv1.RemediationAction,
+) (updateNeeded, updateIsForbidden bool, err error) {
+	forceUpdate := false
+
+	desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desired)
+	if err != nil {
+		return false, false, fmt.Errorf("unable to convert desired Subscription to an Unstructured: %w", err)
+	}
+
+	// when specified, manually handle config as if it's "mustonlyhave"
+	if desired.Spec.Config != nil {
+		desiredConfig, _, err := unstructured.NestedMap(desiredUnstruct, "spec", "config")
+		if err != nil {
+			return false, false, fmt.Errorf("unable to get config from desired Subscription")
+		}
+
+		if len(desiredConfig) > 0 {
+			existingConfig, found, err := unstructured.NestedMap(existing.Object, "spec", "config")
+			if err != nil {
+				return false, false, fmt.Errorf("unable to get config from existing Subscription")
+			} else if !found {
+				forceUpdate = true
+				unstructured.SetNestedMap(existing.Object, desiredConfig, "spec", "config")
+			} else if eq, _ := deeplyEquivalent(desiredConfig, existingConfig, true); !eq {
+				forceUpdate = true
+				unstructured.SetNestedMap(existing.Object, desiredConfig, "spec", "config")
+			}
+		}
+	}
+
+	// Clear `installPlanApproval` from the desired subscription when in inform mode - since that field can not
+	// be set in the policy, we should not check it on the object in the cluster.
+	if action.IsInform() {
+		unstructured.RemoveNestedField(desiredUnstruct, "spec", "installPlanApproval")
+	}
+
+	updateNeeded, forbidden, err := r.mergeObjects(ctx, desiredUnstruct, existing)
+
+	return updateNeeded || forceUpdate, forbidden, err
+}
+
 // mergeObjects takes fields from the desired object and sets/merges them on the
 // existing object. It checks and returns whether an update is really necessary
 // with a server-side dry-run.
 func (r *OperatorPolicyReconciler) mergeObjects(
 	ctx context.Context,
-	desired map[string]interface{},
+	desired map[string]any,
 	existing *unstructured.Unstructured,
-	complianceType policyv1.ComplianceType,
 ) (updateNeeded, updateIsForbidden bool, err error) {
 	desiredObj := &unstructured.Unstructured{Object: desired}
 
@@ -2873,7 +2960,7 @@ func (r *OperatorPolicyReconciler) mergeObjects(
 	removeFieldsForComparison(existingObjectCopy)
 
 	//nolint:dogsled
-	_, errMsg, updateNeeded, _, _ := handleKeys(desiredObj, existing, existingObjectCopy, complianceType, "")
+	_, errMsg, updateNeeded, _, _ := handleKeys(desiredObj, existing, existingObjectCopy, policyv1.MustHave, "")
 	if errMsg != "" {
 		return updateNeeded, false, errors.New(errMsg)
 	}

test/e2e/case38_install_operator_test.go

@@ -3827,4 +3827,154 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				checkCompliance(bundlePolName, testNamespace, olmWaitTimeout*2, policyv1.Compliant)
 			})
 	})
+
+	Describe("Test changes to subscription config and operator group selector", Ordered, func() {
+		const (
+			policyYAML = "../resources/case38_operator_install/operator-policy-with-group-and-config.yaml"
+			policyName = "oppol-with-group-and-config"
+		)
+
+		BeforeEach(func() {
+			preFunc()
+		})
+
+		It("Should be able to update the subscription when the policy changes", func(ctx SpecContext) {
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				policyYAML, testNamespace, gvrPolicy, gvrOperatorPolicy)
+
+			By("Verifying the initial state of the policy")
+			check(
+				policyName,
+				false,
+				[]policyv1.RelatedObject{{
+					Object: policyv1.ObjectResource{
+						Kind:       "Subscription",
+						APIVersion: "operators.coreos.com/v1alpha1",
+					},
+					Compliant: "Compliant",
+					Reason:    "Resource found as expected",
+				}},
+				metav1.Condition{
+					Type:    "SubscriptionCompliant",
+					Status:  metav1.ConditionTrue,
+					Reason:  "SubscriptionMatches",
+					Message: "the Subscription matches what is required by the policy",
+				},
+				"the Subscription required by the policy was created",
+			)
+
+			By("Verifying the initial state of the config in the subscription")
+			Eventually(func(g Gomega) {
+				sub, err := targetK8sDynamic.Resource(gvrSubscription).Namespace(opPolTestNS).
+					Get(ctx, "example-operator", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(sub).NotTo(BeNil())
+
+				sel, found, err := unstructured.NestedStringMap(sub.Object, "spec", "config", "nodeSelector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(sel).To(HaveKeyWithValue("node-role.kubernetes.io/infra", ""))
+			}, olmWaitTimeout, 5).Should(Succeed())
+
+			By("Verifying the initial state of the operator group")
+			Eventually(func(g Gomega) {
+				group, err := targetK8sDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					Get(ctx, "scoped-operator-group", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(group).NotTo(BeNil())
+
+				target, found, err := unstructured.NestedStringSlice(group.Object, "spec", "targetNamespaces")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(target).To(ContainElement("operator-policy-testns"))
+
+				_, found, err = unstructured.NestedMap(group.Object, "spec", "selector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeFalse())
+			}, olmWaitTimeout, 5).Should(Succeed())
+
+			By("Adjusting the policy to observe how the subscription and operator group change")
+			utils.Kubectl("patch", "operatorpolicy", policyName, "-n", testNamespace, "--type=json", "-p", "["+
+				`{"op": "replace", "path": "/spec/subscription/config/nodeSelector",`+
+				` "value": {"node-role.kubernetes.io/worker": ""} },`+
+				`{"op": "replace", "path": "/spec/operatorGroup/targetNamespaces",`+
+				` "value": ["another-namespace"] },`+
+				`{"op": "add", "path": "/spec/operatorGroup/selector",`+
+				` "value": {"matchLabels": {"foo": "bar"}}}`+
+				"]")
+
+			By("Verifying the new state of the config in the subscription")
+			Eventually(func(g Gomega) {
+				sub, err := targetK8sDynamic.Resource(gvrSubscription).Namespace(opPolTestNS).
+					Get(ctx, "example-operator", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(sub).NotTo(BeNil())
+
+				sel, found, err := unstructured.NestedStringMap(sub.Object, "spec", "config", "nodeSelector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(sel).NotTo(HaveKeyWithValue("node-role.kubernetes.io/infra", ""))
+				g.Expect(sel).To(HaveKeyWithValue("node-role.kubernetes.io/worker", ""))
+			}, olmWaitTimeout, 5).Should(Succeed())
+
+			By("Verifying the new state of the operator group")
+			Eventually(func(g Gomega) {
+				group, err := targetK8sDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					Get(ctx, "scoped-operator-group", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(group).NotTo(BeNil())
+
+				target, found, err := unstructured.NestedStringSlice(group.Object, "spec", "targetNamespaces")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(target).NotTo(ContainElement("operator-policy-testns"))
+				g.Expect(target).To(ContainElement("another-namespace"))
+
+				sel, found, err := unstructured.NestedMap(group.Object, "spec", "selector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(sel).To(HaveKey("matchLabels"))
+			}, olmWaitTimeout, 5).Should(Succeed())
+
+			By("Removing some fields from the policy to verify previous configurations are preserved")
+			utils.Kubectl("patch", "operatorpolicy", policyName, "-n", testNamespace, "--type=json", "-p", "["+
+				`{"op": "remove", "path": "/spec/subscription/config/nodeSelector"},`+
+				`{"op": "remove", "path": "/spec/operatorGroup/targetNamespaces"},`+
+				`{"op": "replace", "path": "/spec/operatorGroup/selector",`+
+				` "value": {"matchExpressions": [{"key": "foo", "operator": "In", "values": ["bar"]}]}}`+
+				"]")
+
+			By("Verifying the final state of the config in the subscription")
+			Eventually(func(g Gomega) {
+				sub, err := targetK8sDynamic.Resource(gvrSubscription).Namespace(opPolTestNS).
+					Get(ctx, "example-operator", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(sub).NotTo(BeNil())
+
+				sel, found, err := unstructured.NestedStringMap(sub.Object, "spec", "config", "nodeSelector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(sel).To(HaveKeyWithValue("node-role.kubernetes.io/worker", ""))
+			}, olmWaitTimeout, 5).Should(Succeed())
+
+			By("Verifying the final state of the operator group")
+			Eventually(func(g Gomega) {
+				group, err := targetK8sDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					Get(ctx, "scoped-operator-group", metav1.GetOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(group).NotTo(BeNil())
+
+				target, found, err := unstructured.NestedStringSlice(group.Object, "spec", "targetNamespaces")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(target).To(ContainElement("another-namespace"))
+
+				sel, found, err := unstructured.NestedMap(group.Object, "spec", "selector")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+				g.Expect(sel).NotTo(HaveKey("matchLabels"))
+				g.Expect(sel).To(HaveKey("matchExpressions"))
+			}, olmWaitTimeout, 5).Should(Succeed())
+		})
+	})
 })

test/resources/case38_operator_install/operator-policy-with-group-and-config.yaml

@@ -0,0 +1,31 @@
+apiVersion: policy.open-cluster-management.io/v1beta1
+kind: OperatorPolicy
+metadata:
+  name: oppol-with-group-and-config
+  labels:
+    policy.open-cluster-management.io/cluster-name: "managed"
+    policy.open-cluster-management.io/cluster-namespace: "managed"
+  ownerReferences:
+  - apiVersion: policy.open-cluster-management.io/v1
+    kind: Policy
+    name: parent-policy
+    uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation
+spec:
+  remediationAction: enforce
+  severity: medium
+  complianceType: musthave
+  operatorGroup:
+    name: scoped-operator-group
+    namespace: operator-policy-testns
+    targetNamespaces: # will be changed
+      - operator-policy-testns
+  subscription:
+    channel: stable
+    config:
+      nodeSelector: # will be changed
+        node-role.kubernetes.io/infra: ''
+    name: example-operator
+    namespace: operator-policy-testns
+    source: grc-mock-source
+    sourceNamespace: olm
+  upgradeApproval: Automatic