jenkinsci / kubernetes-operator

Kubernetes native Jenkins Operator
https://jenkinsci.github.io/kubernetes-operator
Other
596 stars 233 forks source link

Unable to reference an existing Jenkins CR in a different namespace #922

Closed valenca closed 4 months ago

valenca commented 9 months ago

Describe the bug When setting the jenkins.enabled configuration to false (i.e. telling the operator to look at an existing Jenkins CR, as opposed to creating a new one from the values), we can no longer set the namespace, as it will read the operator's metadata to compute the Jenkins namespace.

To Reproduce Create a Jenkins CR in namespace "jenkins" Install the jenkins operator in the namespace operators, with jenkins.enabled = false and jenkins.namespace = jenkins. It will not be able to find the Jenkins CR in the jenkins ns, and will idle waiting for one in the operator namespace.

Additional information It was introduced by this commit, specifically: https://github.com/jenkinsci/kubernetes-operator/commit/1b6e944ae63b4c2ec9bc915cb2627f3442d0f90d

Kubernetes version: Jenkins Operator version: 0.8.0

Workaround: Since we are deploying the operator with kustomize, we can do a post-render step to change the WATCH_NAMESPACE operator env var:

  postRenderers:
  - kustomize:
      patchesStrategicMerge:
        - kind: Deployment
          apiVersion: apps/v1
          metadata:
            name: jenkins-jenkins-operator
          spec:
            template:
              spec:
                containers:
                  - name: jenkins-operator
                    env:
                      - name: WATCH_NAMESPACE
                        $patch: delete
        - kind: Deployment
          apiVersion: apps/v1
          metadata:
            name: jenkins-jenkins-operator
          spec:
            template:
              spec:
                containers:
                  - name: jenkins-operator
                    env:
                      - name: WATCH_NAMESPACE
                        value: jenkins

It's always possible to change the deployment directly, but as far as we tried, it wasn't possible to do it within the values.yaml file.

brokenpip3 commented 9 months ago

yeah make sense, should be an easy helm chart fix, do you want to try to make a PR?

brokenpip3 commented 9 months ago

(this commit https://github.com/jenkinsci/kubernetes-operator/commit/1b6e944ae63b4c2ec9bc915cb2627f3442d0f90d is necessary for the kubectl apply manifest installation option that we have)

valenca commented 9 months ago

I'd be happy to help, I'm just not sure of all the ramifications. I assume that just removing the {{- if .Values.jenkins.enabled }} will have ill effects on some cases, like the kubectl apply manifest you mention.

What would be the approach here? Add a new parameter to the operator CRD that reads from there instead?

brokenpip3 commented 9 months ago

No I think that this should be resolved at helm chart level only, something like adding a watched-namespace key here: https://github.com/jenkinsci/kubernetes-operator/blob/3682468b5f75257292ce2a4c76ae429062f201c8/chart/jenkins-operator/values.yaml#L293 and adding the proper if logic in the operator deployment, wdyt?

valenca commented 9 months ago

Ah right. I don't know why I mentioned the CRD, I meant exactly that, the operator object in the values. I'll see what I can do.

valenca commented 9 months ago

https://github.com/jenkinsci/kubernetes-operator/pull/923 Here we go. I'm not entirely sure if I can manage any test implementation nor if the PR text is as expected. It's been a while since I contributed to open source :P