Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deploy to arbitrary namespace #176

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 15, 2021

This allows the user to (un)deploy the Operator to arbitrary namespaces, not only container-jfr-operator-system. This can be done like so:

make DEPLOY_NAMESPACE=some_namespace deploy (or undeploy)

Deployment to the current namespace would then look like make DEPLOY_NAMESPACE=$(oc project -q) deploy. I could also set the default value for DEPLOY_NAMESPACE to the current namespace, but I thought it might be better to require this to be explicit.

I tried to use the config/default/kustomization.yaml's namespace property for this purpose with kustomize, but this ended up modifying the file in an unexpected way and changing it beyond simply replacing the namespace value. So I have simply used a piped sed here, which is similar to what we did before the 1.x migration. I leave the default container-jfr-operator-system namespace value intact however, rather than replacing it with something like REPLACE_NAMESPACE, so that the default value is something reasonable looking when make bundle is run.

This doesn't seem to work for the default namespace - I see an error that the operator pod cannot be started because it has runAsNonRoot but will be run as root. It does deploy successfully in other namespaces, however. I'm looking into what the reason is here.

Fixes #172

@andrewazores andrewazores force-pushed the make-deploy-namespace branch from fd7a45c to 07bda38 Compare March 15, 2021 21:07
@andrewazores andrewazores requested a review from ebaron March 15, 2021 21:08
@andrewazores andrewazores marked this pull request as ready for review March 15, 2021 21:10
@andrewazores andrewazores force-pushed the make-deploy-namespace branch from 07bda38 to da8d0e1 Compare March 15, 2021 21:27
@ebaron
Copy link
Member

ebaron commented Mar 15, 2021

I tried to use the config/default/kustomization.yaml's namespace property for this purpose with kustomize, but this ended up modifying the file in an unexpected way and changing it beyond simply replacing the namespace value. So I have simply used a piped sed here, which is similar to what we did before the 1.x migration.

I tried doing this, and it seems to work okay. What kind of problems did you see?

pushd config/default && $(KUSTOMIZE) edit set namespace $(DEPLOY_NAMESPACE) && popd

@andrewazores
Copy link
Member Author

Hmm. I end up with a diff like this:

diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml
index a0587e5..c74ad67 100644
--- a/config/default/kustomization.yaml
+++ b/config/default/kustomization.yaml
@@ -1,5 +1,5 @@
 # Adds namespace to all resources.
-namespace: container-jfr-operator-system
+namespace: foo-namespace
 
 # Value of this field is prepended to the
 # names of all resources, e.g. a deployment named
@@ -12,10 +12,6 @@ namePrefix: container-jfr-operator-
 #commonLabels:
 #  someName: someValue
 
-bases:
-- ../crd
-- ../rbac
-- ../manager
 # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
 # crd/kustomization.yaml
 #- ../webhook
@@ -24,10 +20,10 @@ bases:
 # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
 #- ../prometheus
 
-patchesStrategicMerge:
 # Protect the /metrics endpoint by putting it behind auth.
 # If you want your controller-manager to expose the /metrics
 # endpoint w/o any authn/z, please comment the following line.
+patchesStrategicMerge:
 - manager_auth_proxy_patch.yaml
 
 # Mount the controller config file for loading manager configurations
@@ -44,31 +40,9 @@ patchesStrategicMerge:
 #- webhookcainjection_patch.yaml
 
 # the following config is for teaching kustomize how to do var substitution
-vars:
-# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
-#- name: CERTIFICATE_NAMESPACE # namespace of the certificate CR
-#  objref:
-#    kind: Certificate
-#    group: cert-manager.io
-#    version: v1
-#    name: serving-cert # this name should match the one in certificate.yaml
-#  fieldref:
-#    fieldpath: metadata.namespace
-#- name: CERTIFICATE_NAME
-#  objref:
-#    kind: Certificate
-#    group: cert-manager.io
-#    version: v1
-#    name: serving-cert # this name should match the one in certificate.yaml
-#- name: SERVICE_NAMESPACE # namespace of the service
-#  objref:
-#    kind: Service
-#    version: v1
-#    name: webhook-service
-#  fieldref:
-#    fieldpath: metadata.namespace
-#- name: SERVICE_NAME
-#  objref:
-#    kind: Service
-#    version: v1
-#    name: webhook-service
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../crd
+- ../rbac
+- ../manager

@andrewazores
Copy link
Member Author

Removing the commented blocks is a bit annoying but fine, but this part is troublesome:

-bases:
-- ../crd
-- ../rbac
-- ../manager
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../crd
+- ../rbac
+- ../manager

@ebaron
Copy link
Member

ebaron commented Mar 15, 2021

Removing the commented blocks is a bit annoying but fine, but this part is troublesome:

-bases:
-- ../crd
-- ../rbac
-- ../manager
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../crd
+- ../rbac
+- ../manager

I see. I hadn't noticed that. I think it's doing this because bases is deprecated:
https://kubernetes-sigs.github.io/kustomize/blog/2019/06/18/v2.1.0/#resources-expanded-bases-deprecated

Still, I understand the concern.

ebaron
ebaron previously approved these changes Mar 15, 2021
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

This approach also works, if we want to avoid Kustomize altering our files.

@andrewazores
Copy link
Member Author

I do prefer to do it this way than to have file contents edited upon deployment, although it isn't a big deal.

I will add a commit shortly to update the README.

@ebaron ebaron merged commit 5fa4074 into cryostatio:main Mar 16, 2021
@ebaron
Copy link
Member

ebaron commented Mar 16, 2021

This doesn't seem to work for the default namespace - I see an error that the operator pod cannot be started because it has runAsNonRoot but will be run as root. It does deploy successfully in other namespaces, however. I'm looking into what the reason is here.

This seems to be due to kubernetes-sigs/kubebuilder#1637, and will be fixed when we upgrade to a newer kube-rbac-proxy.

@andrewazores andrewazores deleted the make-deploy-namespace branch March 17, 2021 13:08
@andrewazores
Copy link
Member Author

This doesn't seem to work for the default namespace - I see an error that the operator pod cannot be started because it has runAsNonRoot but will be run as root. It does deploy successfully in other namespaces, however. I'm looking into what the reason is here.

This seems to be due to kubernetes-sigs/kubebuilder#1637, and will be fixed when we upgrade to a newer kube-rbac-proxy.

Nice find. Looks like operator-sdk 1.5.x should clear it up for us: https://github.com/operator-framework/operator-sdk/releases/tag/v1.5.0 .

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

Successfully merging this pull request may close these issues.

make deploy should deploy the Operator into the current namespace
2 participants