Allow configuring a default namespace for operators #204

JustinKuli · 2024-02-15T15:48:50Z

This adds a command line flag for setting a default namespace for subscriptions/operatorgroups when not specified in an OperatorPolicy spec. This will allow (for example) OpenShift clusters to default to the "openshift-operators" namespace, which is conventionally used for cluster-wide operator installations.

Note to reviewers: the first commit is just re-arranging code blocks to a more consistent order; going through the other 2 commit by commit is probably easier to read.

This sets the command-line flag added by the companion PR open-cluster-management-io/config-policy-controller#204 This only sets that flag when the managed cluster is using OpenShift v4. It should be noted that this flag is *not* currently configurable by the user (it is not for example exposed through an annotation). Refs: - https://issues.redhat.com/browse/ACM-9896 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

mprahl · 2024-02-15T21:36:31Z

controllers/operatorpolicy_controller.go

+
+ err := json.Unmarshal(policy.Spec.Subscription.Raw, &sub)
+ if err != nil {
+ return nil, fmt.Errorf("unable to unmarshal from spec.subscription: %w", err)


Since this becomes user facing in the status (I think), how about?

Suggested change

return nil, fmt.Errorf("unable to unmarshal from spec.subscription: %w", err)

return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err)

mprahl · 2024-02-15T21:51:26Z

controllers/operatorpolicy_controller.go

+// buildOperatorGroup bootstraps the OperatorGroup spec defined in the operator policy
+// with the apiversion and kind in preparation for resource creation
+func buildOperatorGroup(
+ policy *policyv1beta1.OperatorPolicy, defaultNS string,


Suggested change

policy *policyv1beta1.OperatorPolicy, defaultNS string,

policy *policyv1beta1.OperatorPolicy, namespace string,

It would still use the namespace specified by the user if one is provided, so I feel like defaultNS is a more accurate name. But, thinking about it, we should probably stop and alert the user if the specified namespace for the operator group does not match what the subscription will use.

In which case, the parameter should just be called namespace 😆

JustinKuli · 2024-02-15T22:40:39Z

I adjusted the way the relatedObjects is marked as optional, so the controller shouldn't get stuck if the cluster still has the old version of the CRD where the field is required: https://github.com/open-cluster-management-io/config-policy-controller/compare/4187afd2e0bd400b5160e0104c093675187ca9d1..d76d6b5eee52b5f3d7ed8789c0958468e7a8ad75#diff-c1ba6e7a77ddcb50f7ecdd75bd3556d38d1d58414b685c17a84953b947e5cb1cR103-R104

This sets the command-line flag added by the companion PR open-cluster-management-io/config-policy-controller#204 This only sets that flag when the managed cluster is using OpenShift v4. It should be noted that this flag is *not* currently configurable by the user (it is not for example exposed through an annotation). Refs: - https://issues.redhat.com/browse/ACM-9896 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

controllers/operatorpolicy_status.go

mprahl

/hold for the optional minor wording change

This commit only moves some definitions around so that things are in a more consistent order, which may help other refactoring work. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

This will allow OperatorPolicies to use a default namespace when the user does not specify one in the policy. For example, on OpenShift, it's conventional to install operators to the openshift-operators namespace when they are cluster-wide. This default namespace is set via a command line flag, so it can be customized per deployment. This commit also adds a validation condition to OperatorPolicy, for reporting when the subscription or operatorgroup specs are invalid. Refs: - https://issues.redhat.com/browse/ACM-9896 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Other status fields are not required, and this field was specifically causing an issue for the new validation condition, since it does not have any relatedObjects associated with it. This both fixes the CRD and the behavior of the controller (so that field is always set) so that if the CRD on the cluster is still the old one, the controller will not get stuck. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

openshift-ci · 2024-02-20T14:11:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JustinKuli,mprahl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JustinKuli · 2024-02-20T14:20:45Z

/unhold

This sets the command-line flag added by the companion PR open-cluster-management-io/config-policy-controller#204 This only sets that flag when the managed cluster is using OpenShift v4. It should be noted that this flag is *not* currently configurable by the user (it is not for example exposed through an annotation). Refs: - https://issues.redhat.com/browse/ACM-9896 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

This sets the command-line flag added by the companion PR open-cluster-management-io/config-policy-controller#204 This only sets that flag when the managed cluster is using OpenShift v4. It should be noted that this flag is *not* currently configurable by the user (it is not for example exposed through an annotation). Refs: - https://issues.redhat.com/browse/ACM-9896 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com> (cherry picked from commit 010070d5d134801cb3a257bf865f216318156b58)

openshift-ci bot added the dco-signoff: yes label Feb 15, 2024

openshift-ci bot requested review from mprahl and yiraeChristineKim February 15, 2024 15:49

openshift-ci bot added the approved label Feb 15, 2024

JustinKuli mentioned this pull request Feb 15, 2024

Set OperatorPolicy default namespace option open-cluster-management-io/governance-policy-addon-controller#130

Merged

mprahl reviewed Feb 15, 2024

View reviewed changes

JustinKuli force-pushed the 9896-oppol-defaulting-namespace branch from 4187afd to d76d6b5 Compare February 15, 2024 22:37

JustinKuli requested review from mprahl, JeffeyL and zyjjay February 19, 2024 14:20

mprahl reviewed Feb 19, 2024

View reviewed changes

controllers/operatorpolicy_status.go Outdated Show resolved Hide resolved

mprahl previously approved these changes Feb 19, 2024

View reviewed changes

openshift-ci bot assigned mprahl Feb 19, 2024

openshift-ci bot added do-not-merge/hold lgtm labels Feb 19, 2024

JustinKuli added 3 commits February 19, 2024 16:39

Rearrange code

cad22dd

This commit only moves some definitions around so that things are in a more consistent order, which may help other refactoring work. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli dismissed mprahl’s stale review via 98f2a68 February 19, 2024 21:40

JustinKuli force-pushed the 9896-oppol-defaulting-namespace branch from d76d6b5 to 98f2a68 Compare February 19, 2024 21:40

openshift-ci bot removed the lgtm label Feb 19, 2024

mprahl approved these changes Feb 20, 2024

View reviewed changes

openshift-ci bot added the lgtm label Feb 20, 2024

openshift-ci bot removed the do-not-merge/hold label Feb 20, 2024

openshift-merge-bot bot merged commit 296b1b7 into open-cluster-management-io:main Feb 20, 2024
4 checks passed

This was referenced Feb 20, 2024

🤖 Sync from open-cluster-management-io/config-policy-controller: #204 stolostron/config-policy-controller#724

Merged

😿 Failed to sync the upstream PRs: #204 stolostron/config-policy-controller#725

Closed

JustinKuli deleted the 9896-oppol-defaulting-namespace branch July 25, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring a default namespace for operators #204

Allow configuring a default namespace for operators #204

JustinKuli commented Feb 15, 2024

mprahl Feb 15, 2024

mprahl Feb 15, 2024

JustinKuli Feb 15, 2024

JustinKuli Feb 15, 2024

JustinKuli commented Feb 15, 2024

mprahl left a comment

openshift-ci bot commented Feb 20, 2024

JustinKuli commented Feb 20, 2024

	return nil, fmt.Errorf("unable to unmarshal from spec.subscription: %w", err)
	return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err)

	policy *policyv1beta1.OperatorPolicy, defaultNS string,
	policy *policyv1beta1.OperatorPolicy, namespace string,

Allow configuring a default namespace for operators #204

Allow configuring a default namespace for operators #204

Conversation

JustinKuli commented Feb 15, 2024

mprahl Feb 15, 2024

Choose a reason for hiding this comment

mprahl Feb 15, 2024

Choose a reason for hiding this comment

JustinKuli Feb 15, 2024

Choose a reason for hiding this comment

JustinKuli Feb 15, 2024

Choose a reason for hiding this comment

JustinKuli commented Feb 15, 2024

mprahl left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 20, 2024

JustinKuli commented Feb 20, 2024