Add upgradeApproval field to OperatorPolicy

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

@@ -1256,7 +1256,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+`"},`+
@@ -1292,7 +1292,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())
@@ -1301,7 +1301,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).
@@ -1326,6 +1327,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,
@@ -1351,9 +1353,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).
@@ -1384,7 +1414,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",
 			)
 		})
 	})
@@ -1519,7 +1549,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"}, `+
@@ -1529,19 +1559,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