Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Fix installing Gatekeeper when operator is deployed via OLM #95

Merged
merged 9 commits into from
Dec 19, 2020

Conversation

font
Copy link
Member

@font font commented Dec 19, 2020

This adds the ability to use a default canonical namespace to install Gatekeeper when running on OpenShift, or default to using the same namespace as the operator on vanilla Kubernetes. As a result, the WATCH_NAMESPACE feature has been completely removed as this cluster scoped operator does not currently have a need for it.

Closes #93

@font font requested review from ruromero and ycao56 December 19, 2020 01:20

var (
DefaultGatekeeperNamespace = "gatekeeper-system"
DefaultOpenShiftGatekeeperNamespace = "openshift-gatekeeper-system"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any technical reason why on openshift the default is openshift-gatekeeper-system?

Copy link

@ycao56 ycao56 Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to override this default value on openshift?

Copy link

@ycao56 ycao56 Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest let openshift behave the same as vanilla k8s with OLM and provide the option to allow user to custom the namespace to install gatekeeper through gatekeeper CR. This should behave the same no matter it is openshift or vanilla k8s with OLM unless there is some technical. limitation that I am not aware of

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use any name we'd like here, but it has to be decided at compile time until #87 is resolved at least (I don't think resolving that issue is trivial until CRDs have built-in support for immuable fields). Whatever name we pick, it needs to be a canonical namespace to install Gatekeeper on OpenShift as I believe a cluster-scoped operator will land in openshift-operators by default, and we don't want to pollute that namespace unnecessarily. I opted to prefix openshift- to the normal default GK namespace as is typically the convention to indicate a reserved namespace, but there may be other things we need to do to officially reserve it.

On Kubernetes we default to using the same namespace the operator is running in. If we can remove the limitation that the operator will be installed to openshift-operators, then we can choose the same behavior as vanilla K8s. I'm open to other suggestions or ideas!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I was not aware of that on openshift the cluster-scoped operator will land in openshift-operators. With latest bundle-index image in quay.io/gatekeeper, I was able to create catalogsource, operatorgroup, subscription in gatekeeper-system ns and then the operator was deployed to gatekeeper-system ns. I guess it goes to gatekeeper-system ns because it was deployed by the custom catalog source not from ocp catalog.

Copy link

@ycao56 ycao56 Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still go with field in CRD approach even the field is not readOnly. I expect if the user decides to update the namespace field, the gatekeeper will be uninstalled and reinstalled in the new namespace by the operator. Maybe we can just document it for now to warn user the expected behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is good to go. We can discuss the CRD enhancement in issue #87

Copy link

@ycao56 ycao56 Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I just did some testing. I found that the operator would be installed in the namespace where subscription was created. e.g. catalog source in openshift-marketplace, subscription in gatekeeper-system, then operator was created in gatekeeper-system. if subscription was in openshift-operators, then operator was created in openshift-operators.
Not sure if it would work the same for ocp catalog, will have to test once it is published.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still go with field in CRD approach even the field is not readOnly. I expect if the user decides to update the namespace field, the gatekeeper will be uninstalled and reinstalled in the new namespace by the operator. Maybe we can just document it for now to warn user the expected behavior.

I had also thought about doing that, but convinced myself against it because it seemed like a rather dangerous operation and possibly unintended from the user. Whereas making the field immutable would guarantee the user performs the intended delete and create action. If we agree to document it well with the plan to make that field immutable in the future, I could be amenable to this approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I just did some testing. I found that the operator would be installed in the namespace where subscription was created. e.g. catalog source in openshift-marketplace, subscription in gatekeeper-system, then operator was created in gatekeeper-system. if subscription was in openshift-operators, then operator was created in openshift-operators.
Not sure if it would work the same for ocp catalog, will have to test once it is published.

Correct, it is driven by the namespace of the subscription. The thing is the openshift-operators ns is how it's officially documented (https://docs.openshift.com/container-platform/4.6/operators/admin/olm-adding-operators-to-cluster.html) . It may still be configurable, but I'm not entirely sure if it can be done from the console.

@font font merged commit 0d05b51 into gatekeeper:master Dec 19, 2020
@ycao56
Copy link

ycao56 commented Dec 19, 2020

I think we should still consider making the default gatekeeper namespace on openshift gatekeeper-system because this is the namespace gatekeeper users familiar with. openshift-gatekeeper-system might be confusing.

@font
Copy link
Member Author

font commented Dec 19, 2020

I think we should still consider making the default gatekeeper namespace on openshift gatekeeper-system because this is the namespace gatekeeper users familiar with. openshift-gatekeeper-system might be confusing.

I'm happy to make the change if we think this would be the best approach. Examples like OpenShift Pipelines (Tekton) using openshift-pipelines as the namespace on OCP, and tekton-pipelines on Kubernetes, convinced me to go with this option. But I don't have a strong opinion either way, aside from what I believe is the general convention that a canonical namespace should likely be a reserved namespace i.e. openshift- prefix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatekeeper operator failed to deploy gatekeeper on OLM with error
2 participants