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 in 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 in 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 in 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