Add upgradeApproval field to OperatorPolicy

Breaking change: setting `InstallPlanApproval` on the subscription is
invalid, and will result in a noncompliant policy that does not perform
any other actions. Previously, *not* setting that field was invalid.

Being able to set `InstallPlanApproval` in the policy was somewhat
confusing, because the controller would override the supplied value in
many cases, in order to have control over which upgrades would be
approved. Additionally, there was a desire to separate the approval for
the initial installation from the approval for later upgrades.

The new field addresses these concerns. Initial installs will be
approved whenever the policy is enforced (as long as it matches the
policy's specified allowed versions), and upgrades will only be approved
when `upgradeApproval` is set to Automatic.

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

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

api/v1beta1/operatorpolicy_types.go

@@ -135,6 +135,14 @@ type OperatorPolicySpec struct {
 	// When in inform mode, any resources that would be deleted if the policy was enforced will
 	// be causes for NonCompliance, but resources that would be kept will be considered Compliant.
 	RemovalBehavior RemovalBehavior `json:"removalBehavior,omitempty"`
+
+	//+kubebuilder:validation:Required
+	//+kubebuilder:validation:Enum=None;Automatic
+	// UpgradeApproval determines whether 'upgrade' InstallPlans for the operator will be approved
+	// by the controller when the policy is enforced and in 'musthave' mode. The initial InstallPlan
+	// approval is not affected by this setting. This setting has no effect when the policy is in
+	// 'mustnothave' mode. Allowed values are "None" or "Automatic".
+	UpgradeApproval string `json:"upgradeApproval"`
 }
 
 // OperatorPolicyStatus defines the observed state of OperatorPolicy

config/samples/policy_v1beta1_operatorpolicy.yaml

@@ -16,9 +16,9 @@ spec:
     channel: stable
     name: strimzi-kafka-operator
     namespace: openshift-operators
-    installPlanApproval: Manual
     source: community-operators
     sourceNamespace: openshift-marketplace
     startingCSV: strimzi-cluster-operator.v0.35.0
+  upgradeApproval: None
   versions:
     - strimzi-cluster-operator.v0.35.0

controllers/operatorpolicy_controller.go

@@ -635,16 +635,16 @@ func buildSubscription(
 	subscription.ObjectMeta.Namespace = ns
 	subscription.Spec = spec
 
-	// This is not validated by the CRD, so validate it here to prevent unexpected behavior.
-	if !(spec.InstallPlanApproval == "Manual" || spec.InstallPlanApproval == "Automatic") {
-		return nil, fmt.Errorf("the policy spec.subscription.installPlanApproval ('%v') is invalid: "+
-			"must be 'Automatic' or 'Manual'", spec.InstallPlanApproval)
+	if spec.InstallPlanApproval != "" {
+		return nil, fmt.Errorf("installPlanApproval is prohibited in spec.subscription")
 	}
 
-	// If the policy is in `enforce` mode and the allowed CSVs are restricted,
-	// the InstallPlanApproval will be set to Manual so that upgrades can be controlled.
-	if policy.Spec.RemediationAction.IsEnforce() && len(policy.Spec.Versions) > 0 {
-		subscription.Spec.InstallPlanApproval = operatorv1alpha1.ApprovalManual
+	// Usually set InstallPlanApproval to manual so that upgrades can be controlled
+	spec.InstallPlanApproval = operatorv1alpha1.ApprovalManual
+	if policy.Spec.RemediationAction.IsEnforce() &&
+		policy.Spec.UpgradeApproval == "Automatic" &&
+		len(policy.Spec.Versions) == 0 {
+		spec.InstallPlanApproval = operatorv1alpha1.ApprovalAutomatic
 	}
 
 	return subscription, nil
@@ -1093,6 +1093,12 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
 		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, string(policy.Spec.ComplianceType))
 	if err != nil {
 		return nil, nil, false, fmt.Errorf("error checking if the Subscription needs an update: %w", err)
@@ -1384,9 +1390,12 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
 		allUpgradeVersions = append(allUpgradeVersions, fmt.Sprintf("%v", csvNames))
 	}
 
-	// Only report this status in `inform` mode, because otherwise it could easily oscillate between this and
-	// another condition below when being enforced.
-	if policy.Spec.RemediationAction.IsInform() {
+	initialInstall := sub.Status.InstalledCSV == ""
+	autoUpgrade := policy.Spec.UpgradeApproval == "Automatic"
+
+	// Only report this status when not approving an InstallPlan, because otherwise it could easily
+	// oscillate between this and another condition.
+	if policy.Spec.RemediationAction.IsInform() || (!initialInstall && !autoUpgrade) {
 		// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
 		// For now this condition assumes it is set to 'NonCompliant'
 		return updateStatus(policy, installPlanUpgradeCond(allUpgradeVersions, nil), relatedInstallPlans...), nil

controllers/operatorpolicy_controller_test.go

@@ -30,10 +30,10 @@ func TestBuildSubscription(t *testing.T) {
 					"sourceNamespace": "my-ns",
 					"name": "my-operator",
 					"channel": "stable",
-					"startingCSV": "my-operator-v1",
-					"installPlanApproval": "Automatic"
+					"startingCSV": "my-operator-v1"
 				}`),
 			},
+			UpgradeApproval: "None",
 		},
 	}
 	desiredGVK := schema.GroupVersionKind{
@@ -48,6 +48,7 @@ func TestBuildSubscription(t *testing.T) {
 	assert.Equal(t, ret.GroupVersionKind(), desiredGVK)
 	assert.Equal(t, ret.ObjectMeta.Name, "my-operator")
 	assert.Equal(t, ret.ObjectMeta.Namespace, "default")
+	assert.Equal(t, ret.Spec.InstallPlanApproval, operatorv1alpha1.ApprovalManual)
 }
 
 func TestBuildSubscriptionInvalidNames(t *testing.T) {
@@ -94,10 +95,10 @@ func TestBuildSubscriptionInvalidNames(t *testing.T) {
 								"sourceNamespace": "my-ns",
 								"name": "` + test.name + `",
 								"channel": "stable",
-								"startingCSV": "my-operator-v1",
-								"installPlanApproval": "Automatic"
+								"startingCSV": "my-operator-v1"
 							}`),
 						},
+						UpgradeApproval: "None",
 					},
 				}
 
@@ -125,8 +126,7 @@ func TestBuildOperatorGroup(t *testing.T) {
 					"sourceNamespace": "my-ns",
 					"name": "my-operator",
 					"channel": "stable",
-					"startingCSV": "my-operator-v1",
-					"installPlanApproval": "Automatic"
+					"startingCSV": "my-operator-v1"
 				}`),
 			},
 		},
@@ -277,8 +277,7 @@ func TestMessageContentOrderMatching(t *testing.T) {
 					"sourceNamespace": "my-ns",
 					"name": "my-operator",
 					"channel": "stable",
-					"startingCSV": "my-operator-v1",
-					"installPlanApproval": "Automatic"
+					"startingCSV": "my-operator-v1"
 				}`),
 			},
 		},

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

@@ -129,6 +129,16 @@ spec:
                   https://olm.operatorframework.io/docs/concepts/crds/subscription/
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
+              upgradeApproval:
+                description: |-
+                  UpgradeApproval determines whether 'upgrade' InstallPlans for the operator will be approved
+                  by the controller when the policy is enforced and in 'musthave' mode. The initial InstallPlan
+                  approval is not affected by this setting. This setting has no effect when the policy is in
+                  'mustnothave' mode. Allowed values are "None" or "Automatic".
+                enum:
+                - None
+                - Automatic
+                type: string
               versions:
                 description: |-
                   Versions is a list of nonempty strings that specifies which installed versions are compliant when
@@ -140,6 +150,7 @@ spec:
             required:
             - complianceType
             - subscription
+            - upgradeApproval
             type: object
           status:
             description: OperatorPolicyStatus defines the observed state of OperatorPolicy

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

@@ -124,6 +124,16 @@ spec:
                   https://olm.operatorframework.io/docs/concepts/crds/subscription/
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
+              upgradeApproval:
+                description: |-
+                  UpgradeApproval determines whether 'upgrade' InstallPlans for the operator will be approved
+                  by the controller when the policy is enforced and in 'musthave' mode. The initial InstallPlan
+                  approval is not affected by this setting. This setting has no effect when the policy is in
+                  'mustnothave' mode. Allowed values are "None" or "Automatic".
+                enum:
+                - None
+                - Automatic
+                type: string
               versions:
                 description: |-
                   Versions is a list of nonempty strings that specifies which installed versions are compliant when
@@ -135,6 +145,7 @@ spec:
             required:
             - complianceType
             - subscription
+            - upgradeApproval
             type: object
           status:
             description: OperatorPolicyStatus defines the observed state of OperatorPolicy

test/e2e/case38_install_operator_test.go

@@ -1259,7 +1259,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				"there are no relevant InstallPlans in the namespace",
 			)
 		})
-		It("Should report an available upgrade", func(ctx SpecContext) {
+		It("Should report an available install when informing", func(ctx SpecContext) {
 			goodVersion := "strimzi-cluster-operator.v0.36.0"
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
 				`[{"op": "replace", "path": "/spec/subscription/startingCSV", "value": "`+goodVersion+`"},`+
@@ -1295,7 +1295,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				"an InstallPlan to update .* is available for approval",
 			)
 		})
-		It("Should do the upgrade when enforced, and stop at the next version", func(ctx SpecContext) {
+		It("Should do the initial install when enforced, and stop at the next version", func(ctx SpecContext) {
 			ipList, err := targetK8sDynamic.Resource(gvrInstallPlan).Namespace(opPolTestNS).
 				List(ctx, metav1.ListOptions{})
 			Expect(err).NotTo(HaveOccurred())
@@ -1304,7 +1304,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			firstInstallPlanName = ipList.Items[0].GetName()
 
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
-				`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`)
+				`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"},`+
+					`{"op": "replace", "path": "/spec/upgradeApproval", "value": "Automatic"}]`)
 
 			Eventually(func(ctx SpecContext) int {
 				ipList, err = targetK8sDynamic.Resource(gvrInstallPlan).Namespace(opPolTestNS).
@@ -1329,6 +1330,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 			// This check covers several situations that occur quickly: the first InstallPlan eventually
 			// progresses to Complete after it is approved, and the next InstallPlan is created and
 			// recognized by the policy (but not yet approved).
+			// This check may need to be adjusted in the future for new strimzi releases.
 			check(
 				opPolName,
 				false,
@@ -1354,9 +1356,37 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				"the InstallPlan.*36.0.*was approved",
 			)
 		})
-		It("Should approve the next version when it's added to the spec", func(ctx SpecContext) {
+		It("Should not approve an upgrade while upgradeApproval is None", func(ctx SpecContext) {
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
-				`[{"op": "add", "path": "/spec/versions/-", "value": "strimzi-cluster-operator.v0.36.1"}]`)
+				`[{"op": "add", "path": "/spec/versions/-", "value": "strimzi-cluster-operator.v0.36.1"},`+
+					`{"op": "replace", "path": "/spec/upgradeApproval", "value": "None"}]`)
+			check(
+				opPolName,
+				false,
+				[]policyv1.RelatedObject{{
+					Object: policyv1.ObjectResource{
+						Kind:       "InstallPlan",
+						APIVersion: "operators.coreos.com/v1alpha1",
+						Metadata: policyv1.ObjectMetadata{
+							Namespace: opPolTestNS,
+							Name:      secondInstallPlanName,
+						},
+					},
+					Compliant: "NonCompliant",
+					Reason:    "The InstallPlan is RequiresApproval",
+				}},
+				metav1.Condition{
+					Type:    "InstallPlanCompliant",
+					Status:  metav1.ConditionFalse,
+					Reason:  "InstallPlanRequiresApproval",
+					Message: "an InstallPlan to update to [strimzi-cluster-operator.v0.36.1] is available for approval",
+				},
+				"an InstallPlan.*36.*is available for approval",
+			)
+		})
+		It("Should approve the upgrade when upgradeApproval is changed to Automatic", func(ctx SpecContext) {
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/upgradeApproval", "value": "Automatic"}]`)
 
 			Eventually(func(ctx SpecContext) string {
 				ip, _ := targetK8sDynamic.Resource(gvrInstallPlan).Namespace(opPolTestNS).
@@ -1387,7 +1417,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 					Reason:  "NoInstallPlansRequiringApproval",
 					Message: "no InstallPlans requiring approval were found",
 				},
-				"the InstallPlan.*36.1.*was approved",
+				"the InstallPlan.*36.*was approved",
 			)
 		})
 	})
@@ -1522,7 +1552,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				`the status of the OperatorGroup could not be determined because the policy is invalid`,
 			)
 		})
-		It("Should report about the invalid installPlanApproval value", func() {
+		It("Should report about the prohibited installPlanApproval value", func() {
 			// remove the "unknown" fields
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
 				`[{"op": "remove", "path": "/spec/operatorGroup/foo"}, `+
@@ -1532,19 +1562,18 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				true,
 				[]policyv1.RelatedObject{},
 				metav1.Condition{
-					Type:   "ValidPolicySpec",
-					Status: metav1.ConditionFalse,
-					Reason: "InvalidPolicySpec",
-					Message: "spec.subscription.installPlanApproval ('Incorrect') is invalid: " +
-						"must be 'Automatic' or 'Manual'",
+					Type:    "ValidPolicySpec",
+					Status:  metav1.ConditionFalse,
+					Reason:  "InvalidPolicySpec",
+					Message: "installPlanApproval is prohibited in spec.subscription",
 				},
-				"NonCompliant",
+				"installPlanApproval is prohibited in spec.subscription",
 			)
 		})
 		It("Should report about the namespaces not matching", func() {
-			// Fix the `installPlanApproval` value
+			// Remove the `installPlanApproval` value
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
-				`[{"op": "replace", "path": "/spec/subscription/installPlanApproval", "value": "Automatic"}]`)
+				`[{"op": "remove", "path": "/spec/subscription/installPlanApproval"}]`)
 			check(
 				opPolName,
 				true,

test/resources/case38_operator_install/operator-policy-all-defaults.yaml

@@ -17,4 +17,4 @@ spec:
   subscription:
     name: project-quay
     namespace: operator-policy-testns
-    installPlanApproval: Manual
+  upgradeApproval: Automatic

test/resources/case38_operator_install/operator-policy-authorino.yaml

@@ -18,6 +18,6 @@ spec:
     channel: stable
     name: authorino-operator
     namespace: operator-policy-testns
-    installPlanApproval: Automatic
     source: operatorhubio-catalog
     sourceNamespace: olm
+  upgradeApproval: Automatic

test/resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml

@@ -17,5 +17,5 @@ spec:
   subscription:
     name: project-quay
     namespace: operator-policy-testns
-    installPlanApproval: Manual
     source: wrong
+  upgradeApproval: None

test/resources/case38_operator_install/operator-policy-manual-upgrades.yaml

@@ -16,11 +16,11 @@ spec:
   complianceType: musthave
   subscription:
     channel: strimzi-0.36.x
-    installPlanApproval: Manual
     name: strimzi-kafka-operator
     namespace: operator-policy-testns
     source: operatorhubio-catalog
     sourceNamespace: olm
     startingCSV: strimzi-cluster-operator.v0.0.0.1337 # shouldn't match a real version
   versions:
     - strimzi-cluster-operator.v0.36.0
+  upgradeApproval: None

test/resources/case38_operator_install/operator-policy-mustnothave-any-version.yaml

@@ -18,9 +18,9 @@ spec:
     channel: stable-3.10
     name: project-quay
     namespace: operator-policy-testns
-    installPlanApproval: Manual
     source: operatorhubio-catalog
     sourceNamespace: olm
+  upgradeApproval: Automatic
   removalBehavior:
     operatorGroups: DeleteIfUnused
     subscriptions: Delete

test/resources/case38_operator_install/operator-policy-mustnothave.yaml

@@ -18,12 +18,12 @@ spec:
     channel: stable-3.10
     name: project-quay
     namespace: operator-policy-testns
-    installPlanApproval: Manual
     source: operatorhubio-catalog
     sourceNamespace: olm
     startingCSV: quay-operator.v3.10.0
   versions:
     - quay-operator.v3.10.0
+  upgradeApproval: Automatic
   removalBehavior:
     operatorGroups: DeleteIfUnused
     subscriptions: Delete

test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml

@@ -18,6 +18,6 @@ spec:
     channel: stable-3.10
     name: project-quay-does-not-exist
     namespace: operator-policy-testns
-    installPlanApproval: Automatic
     source: operatorhubio-catalog
     sourceNamespace: olm
+  upgradeApproval: None

test/resources/case38_operator_install/operator-policy-no-group-csv-fail.yaml

@@ -18,7 +18,7 @@ spec:
     channel: singlenamespace-alpha
     name: etcd
     namespace: operator-policy-testns
-    installPlanApproval: Automatic
     source: operatorhubio-catalog
     sourceNamespace: olm
     startingCSV: etcdoperator.v0.9.2
+  upgradeApproval: None

test/resources/case38_operator_install/operator-policy-no-group-enforce-one-version.yaml

@@ -18,9 +18,9 @@ spec:
     channel: stable-3.10
     name: project-quay
     namespace: operator-policy-testns
-    installPlanApproval: Automatic
     source: operatorhubio-catalog
     sourceNamespace: olm
     startingCSV: quay-operator.v3.10.0
   versions:
   - quay-operator.v3.10.0
+  upgradeApproval: Automatic