ExtraEnvVars does not support secrets for Open WebUI Chart

[saip92]

The Open WebUI Chart places the extraEnvVars into a config map which doesn't allow secrets to be used though the values.yaml mentions otherwise.

I suppose the simplest fix would be to move the environment variables directly into the stateful set instead of config map like the pipelines chart.

[ocraviotto]

Just came to say something similar. The changed introduced with #85 removed the option to have env vars directly templated into the containeres[0].env object. As reported, the main use case is secrets like this:

extraEnvVars:
  - name: GOOGLE_PSE_ENGINE_ID
    valueFrom:
      secretKeyRef:
        name: open-webui-setup
        key: google-pse-engine-id

I am not sure what is the advantage of ConfigMaps other than they can be referenced by multiple containers without having to repeat the same variables (I could imagine an init container requiring some values in some cases, but so far that's not the case with OpenWebUI), but given the amount of sensitive values that can be used here, and that some users might have external secrets to consume them, I'd like to see #88 merged to revert the change.

If someone would still like to see ConfigMaps used, then the ConfigMap added in #85 could be reused, but offering a new extraEnvFromConfigMap (or similar) dictionary to append to the CM instead, while keeping the original extraEnvVars to add to containeres[0].env (no issues with moving non-sensitive variables such as URLs to the config map).

To consider when using envFrom, and why extraEnvVars should be templated last into containers[0].env`:

When a key exists in multiple sources, the value associated with the last source will take precedence. Values defined by an Env with a duplicate key will take precedence.

[saip92]

Thanks for adding that context! I noticed the config map was not used and just kind of assumed it wasn't intentional. I should've checked the history 😅.

Does the config map approach offer anything more compared to direct environment variables that I might be missing apart from reuse? I'm not sure what the intention was in #85.

Looks like #22 talks about sharing the config map between ollama and open-webui, but I suppose that is not the default?

I could update my PR to your suggestion so both exist and secrets would have to be only in extraEnvVars but unless they really need to be shared, it could be confusing to users on where to put which environment variables IMO.

[ocraviotto]

#22 talks about the OLLAMA_BASE_URLS env var. I don't see it's used anywhere else than in Open WebUI, so not about sharing anything with Ollama (which is a dependency, a child chart that could share variables with Open WebUI but so far does not and IMHO should not).

So I'd keep your PR as is TBH, unless someone like the author of #85 , explains how it is that the ConfigMap makes for easier management, particularly if one considers the issues we mentioned above.

[saip92]

Agreed! Thanks for helping to confirm that. I had the same notion but wanted to make sure I wasn't missing anything obvious considering I was just playing around with this for the first time yesterday. 😅

[0xThresh]

Thanks for the discussion and sorry for the trouble @saip92 and @ocraviotto. I didn't consider the impact to secrets when I made the change to the ConfigMap, and that should have been a major version change to signify the potential breaking change made there.

The main reasons I opted for the ConfigMap implementation were readability and ease of editing - the deployment/ statefulset in a cluster can have a very long list of environment variables added in addition to all the other configuration options, so I thought it would be easier to make updates to a ConfigMap instead when environment vars needed to be updated/ added/ removed.

Sticking with the "second dictionary" idea, what if we had two values such as:

extraEnvVars: [] # Added to ConfigMap
sensitiveEnvVars: [] # Mounted from existing secrets to deployment/ statefulset as env vars using the same config  previously used in extraEnvVars

This would separate the non-sensitive values from sensitive ones on the deployment, while still enabling the use of required secrets. Let me know your thoughts and thanks again for reporting the issue.

[ocraviotto]

@0xThresh ConfigMaps are indeed meant to store application configuration, so nothing against its use.
However they weren't meant to store confidential data.

If you still want to allow users to use ConfigMaps for non-confidential data, I'd still suggest to have a new list (I suggested extraEnvFromConfigMap but anything other than extraEnvVars would do) while keeping extraEnvVars as currently use.

If you want to add a new sensitiveEnvVars you'd expect to have the EnvVar kind of object, or come up with something new to parse on your end.
I still fee it's not ideal to do so and would stick to a way to support ConfigMaps and appending to the `containers[*].env' EnvVar object only.

If you do it this way, you can make it backwards compatible as users don't need to migrate current values, while you can move non-confidential data to the ConfigMap already and let users know about tha change. This is so because:

When a key exists in multiple sources, the value associated with the last source will take precedence. Values defined by an Env with a duplicate key will take precedence.

Hope it makes sense.

[saip92]

I agree with @ocraviotto especially in terms of backwards compatibility. Creating a new list for config maps allows existing deployments to continue working, giving users an option to move non-sensitive information by their own accord.

Personally, and only because I currently just deploy this at home, I prefer the direct envVars setup since that triggers the pods to restart. With a config map, you will have to do that yourself as of today (in the works).

[0xThresh]

Totally fair feedback on all fronts. If I push forward with the ConfigMap/ sensitive values separation in the future, it will be under a major version change to help signify that it is a breaking change to anyone who decides to upgrade to it (which I should have done to begin with on this change 🥲).

@saip92 please update the appVersion of the chart in your open PR, and I'll merge that in to resolve this issue. Thanks again!