cloudnative-pg / charts

CloudNativePG Helm Charts
Apache License 2.0
174 stars 82 forks source link

[BUG]: Operator chart "WATCH_NAMESPACE" cannot be set correctly #299

Closed Stevenpc3 closed 4 months ago

Stevenpc3 commented 4 months ago

Setting the WATCH_NAMESPACE env variable does not work via configmap as it is loaded after the operator starts and chooses namespaces to watch. Also there is not other way to set it currently.

In reference to the issue at https://github.com/cloudnative-pg/cloudnative-pg/issues/3649

The operator helm chart supports setting ENV via --set config.data and passing a list. You can use this as a file override. See here that mentions it in the values.yaml https://github.com/cloudnative-pg/charts/blob/main/charts/cloudnative-pg/values.yaml#L64 and here that passes in the ENV via a configmap https://github.com/cloudnative-pg/charts/blob/f7e6ddc2b3a1211588d07d723e9e3dfcb4f66485/charts/cloudnative-pg/templates/config.yaml#L29

I made an override.yaml to pass in with the contents

config:
  data:
    WATCH_NAMESPACE: namespace-a,namespace-b

then I made two namespaces to match: namespace-a and namespace-b and one to ignore "ignoreme"

image

Then I deployed the operator using the helm chart https://github.com/cloudnative-pg/charts/tree/main/charts/cloudnative-pg to the default namespace

helm repo add cnpg https://cloudnative-pg.github.io/charts
helm upgrade -i cnpg -n default cnpg/cloudnative-pg -f override.yaml

image

You can see in the logs that it watches ALL namespaces but the configuration was passed in to the operator.

image

The configmap has the correct values image

I was unable to exec into the pod to verify the ENV values but you can at least see that the values listed in decribe pod do not include WATCH_NAMESPACE

image

If you check the logs above you can see that it prints "wathcing all namespaces" BEFORE it loads the configurations!!!

It might (for the helm charts process describe) at least be an order of operations thing. The operator should load configurations before deciding which namespace to watch.

I also tried various ways of passing this in as an additonalArg and it does not start or print errors to the log, but it does not appear that "watch-namespace" is any sort of valid commandline arg for the operator. Below is one of MANY ways I tried to pass this list in.

image

EDIT:

I did fork and update the helm chart to pass the ENV "WATCH_NAMESPACE" and that fixed the log output

image

It also detected the namespace in the logs but did not deploy the resources. Which is good.

image

Though the cluster still deployed in the namespace without pods and that is a bit confusing. I guess that will just ahve to be though since that is on the helm user and not the operator since the operator prevented pods from coming up and did its part.

image

Deploying to "namespace-a" worked as expected once the ENV in the chart was fixed.

Stevenpc3 commented 4 months ago

I have forked and created a local way to set this for testing by updating deployment.yaml container section to have

- name: WATCH_NAMESPACE
   value: "{{ .Values.watchNamespace }}"

and adding the following to values.yaml

# -- Namespaces to watch as comma separated list i.e namespace-a,namespace-b. Empty will default to all namespaces.
watchNamespace: ""

this is very ugly though as it requires a --set flag like this where the comma needs to be escaped, but is fine via a yaml file.

 helm upgrade -i cnpg -n default charts/cnpg/ --set watchNamespace="namespace-a\,namespace-b"

This works well for single namespace deploys though which is likely the most used case.

Stevenpc3 commented 4 months ago

PR created https://github.com/cloudnative-pg/charts/pull/301

itay-grudev commented 4 months ago

I do have to note that the Operator doesn't list WATCH_NAMESPACE under the list of Available Options for this config map.

itay-grudev commented 4 months ago

If I am not mistaken (and this is by no means my area of expertise), the WATCH_NAMESPACE comes from the operator framework and requires that it is set via an environment variable.

Since it's not an officially accepted configuration option by the CloudNativePG Operator, I believe we'll accept a PR that adds an additionalEnv option, so that you can set it manually at your own risk.

Stevenpc3 commented 4 months ago

Ok I can make a new PR that uses "additionalEnv" instead. This is fine for now. It appears that it was intended since the chart does list it under config.data https://github.com/cloudnative-pg/charts/blob/main/charts/cloudnative-pg/values.yaml#L64

Perhaps it was a regression of the Operator because it is actually broken because they load the config after they load the namespaces to watch as laid out here https://github.com/cloudnative-pg/cloudnative-pg/issues/3649

Stevenpc3 commented 4 months ago

@itay-grudev I have created a new PR that uses additional env vars https://github.com/cloudnative-pg/charts/pull/303

itay-grudev commented 4 months ago

Closed as completed in #303.

You can now supply your own env to the operator at your own risk.

Stevenpc3 commented 4 months ago

It is supported though. https://cloudnative-pg.io/documentation/1.23/kubectl-plugin/#generation-of-installation-manifests allows you to set it during cnpg generate, which creates a yaml with the WATCH_NAMESPACE env set. It is also mentioned here https://github.com/cloudnative-pg/cloudnative-pg/issues/3649#issuecomment-1873695889 as being supported.