feat: add support for extraEnvFrom

[M0NsTeRRR]

Here is an implementation for #48 without any breaking changes.

You can now pass two additional extra environment variables to define secrets. This implementation allows the user to pass the Postgres secret only to the required container if needed.

[rdettai]

Sorry I didn't have the time to test this PR yet, I'll do it early next week. Looking good overall!

[tcassaert]

I'm probably doing something wrong, but I can't get this PR to work.

My values.yaml looks as follows:

---
config:
  default_index_root_uri: s3://quickwit/indices
  postgres:
    host: pgsql.home.tcassaert.com
    port: 5433
    database: quickwit
    username: quickwit
    password_secret_key_ref:
      name: quickwit-credentials
      key: postgres_password
  storage:
    s3:
      endpoint: https://s3.service.tcassaert.com
      flavor: minio
      region: us-east-1
      access_key_id: quickwit
      secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key
      force_path_style_access: true
indexer:
  replicaCount: 1
searcher:
  replicaCount: 1

This works correctly to set for example the AWS_SECRET_ACCESS_KEY env var:

            - name: AWS_SECRET_ACCESS_KEY
              valueFrom:
                secretKeyRef:
                  key: aws_secret_access_key
                  name: quickwit-credentials

However, following snippet is then included in the node.yaml ConfigMap:

secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key

Which of course fails as this is not valid config for Quickwit.

We should remove that key from the config.storage.s3 before using it in the ConfigMap.

[tcassaert]

There are some AWS env vars we could also set with the Quickwit environment _helper:

--- a/charts/quickwit/templates/_helpers.tpl
+++ b/charts/quickwit/templates/_helpers.tpl
@@ -147,6 +147,10 @@ Quickwit environment
   value: node.yaml
 - name: QW_CLUSTER_ID
   value: {{ .Release.Namespace }}-{{ include "quickwit.fullname" . }}
+{{- if ((.Values.config.storage).s3).endpoint }}
+- name: AWS_ENDPOINT_URL
+  value: {{ .Values.config.storage.s3.endpoint }}
+{{- end }}
 {{- if ((.Values.config.storage).s3).access_key_id }}
 - name: AWS_ACCESS_KEY_ID
   value: {{ .Values.config.storage.s3.access_key_id }}
@@ -162,6 +166,14 @@ Quickwit environment
     secretKeyRef:
       name: {{ include "quickwit.fullname" $ }}
       key: storage.s3.secret_access_key
+{{- if ((.Values.config.storage).s3).region }}
+- name: AWS_REGION
+  value: {{ .Values.config.storage.s3.region }}
+{{- end }}
+{{- if ((.Values.config.storage).s3).force_path_style_access }}
+- name: AWS_S3_FORCE_PATH_STYLE
+  value: {{ .Values.config.storage.s3.force_path_style_access | quote }}
+{{- end }}
 {{- end }}
 {{- if ((.Values.config.storage).azure).account }}
 - name: QW_AZURE_STORAGE_ACCOUNT

But there's also non-s3 variables like flavor.

In the configmap.yaml, this works:

    {{- with .Values.config.storage }}
    storage:
      {{- with .s3 }}
      s3:
        {{- with .flavor }}
        flavor: {{ . }}
        {{- end }}
      {{- end }}
    {{- end }}

But if the user doesn't add flavor, it will fail of course because s3 is empty. Also didn't check the other storage providers yet as I don't have azure experience for example.

[M0NsTeRRR]

I'm probably doing something wrong, but I can't get this PR to work.

My values.yaml looks as follows:

---
config:
  default_index_root_uri: s3://quickwit/indices
  postgres:
    host: pgsql.home.tcassaert.com
    port: 5433
    database: quickwit
    username: quickwit
    password_secret_key_ref:
      name: quickwit-credentials
      key: postgres_password
  storage:
    s3:
      endpoint: https://s3.service.tcassaert.com
      flavor: minio
      region: us-east-1
      access_key_id: quickwit
      secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key
      force_path_style_access: true
indexer:
  replicaCount: 1
searcher:
  replicaCount: 1

This works correctly to set for example the AWS_SECRET_ACCESS_KEY env var:

            - name: AWS_SECRET_ACCESS_KEY
              valueFrom:
                secretKeyRef:
                  key: aws_secret_access_key
                  name: quickwit-credentials

However, following snippet is then included in the node.yaml ConfigMap:

secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key

Which of course fails as this is not valid config for Quickwit.

We should remove that key from the config.storage.s3 before using it in the ConfigMap.

Hello,

This PR is in working progress (haven't tested since my first commit as I'm waiting feedback for full implementation and test, don't consider it as stable). I've fixed the case you mentioned thanks for your feedback :).

Regards,

[M0NsTeRRR]

There are some AWS env vars we could also set with the Quickwit environment _helper:

--- a/charts/quickwit/templates/_helpers.tpl
+++ b/charts/quickwit/templates/_helpers.tpl
@@ -147,6 +147,10 @@ Quickwit environment
   value: node.yaml
 - name: QW_CLUSTER_ID
   value: {{ .Release.Namespace }}-{{ include "quickwit.fullname" . }}
+{{- if ((.Values.config.storage).s3).endpoint }}
+- name: AWS_ENDPOINT_URL
+  value: {{ .Values.config.storage.s3.endpoint }}
+{{- end }}
 {{- if ((.Values.config.storage).s3).access_key_id }}
 - name: AWS_ACCESS_KEY_ID
   value: {{ .Values.config.storage.s3.access_key_id }}
@@ -162,6 +166,14 @@ Quickwit environment
     secretKeyRef:
       name: {{ include "quickwit.fullname" $ }}
       key: storage.s3.secret_access_key
+{{- if ((.Values.config.storage).s3).region }}
+- name: AWS_REGION
+  value: {{ .Values.config.storage.s3.region }}
+{{- end }}
+{{- if ((.Values.config.storage).s3).force_path_style_access }}
+- name: AWS_S3_FORCE_PATH_STYLE
+  value: {{ .Values.config.storage.s3.force_path_style_access | quote }}
+{{- end }}
 {{- end }}
 {{- if ((.Values.config.storage).azure).account }}
 - name: QW_AZURE_STORAGE_ACCOUNT

But there's also non-s3 variables like flavor.

In the configmap.yaml, this works:

    {{- with .Values.config.storage }}
    storage:
      {{- with .s3 }}
      s3:
        {{- with .flavor }}
        flavor: {{ . }}
        {{- end }}
      {{- end }}
    {{- end }}

But if the user doesn't add flavor, it will fail of course because s3 is empty. Also didn't check the other storage providers yet as I don't have azure experience for example.

I think it should be handled in another PR since this PR is solely focused on supporting extraEnvFrom to facilite review and merge.

[tcassaert]

I'm probably doing something wrong, but I can't get this PR to work. My values.yaml looks as follows:

---
config:
  default_index_root_uri: s3://quickwit/indices
  postgres:
    host: pgsql.home.tcassaert.com
    port: 5433
    database: quickwit
    username: quickwit
    password_secret_key_ref:
      name: quickwit-credentials
      key: postgres_password
  storage:
    s3:
      endpoint: https://s3.service.tcassaert.com
      flavor: minio
      region: us-east-1
      access_key_id: quickwit
      secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key
      force_path_style_access: true
indexer:
  replicaCount: 1
searcher:
  replicaCount: 1

This works correctly to set for example the AWS_SECRET_ACCESS_KEY env var:

            - name: AWS_SECRET_ACCESS_KEY
              valueFrom:
                secretKeyRef:
                  key: aws_secret_access_key
                  name: quickwit-credentials

However, following snippet is then included in the node.yaml ConfigMap:

secret_access_key_secret_key_ref:
        name: quickwit-credentials
        key: aws_secret_access_key

Which of course fails as this is not valid config for Quickwit. We should remove that key from the config.storage.s3 before using it in the ConfigMap.

Hello,

This PR is in working progress (haven't tested since my first commit as I'm waiting feedback for full implementation and test, don't consider it as stable). I've fixed the case you mentioned thanks for your feedback :).

Regards,

I don't expect anything stable, don't worry. I just wanted to help to see if it works as expected. Your fix seems to do the trick without having to make other changes, great!

[tcassaert]

One other thing I noticed is that a (empty) secret is created, despite the conditional on top of the secret.yaml template. Now we're checking if any of the secret_key_refs are missing, but that will almost always be the case, as I don't think anyone will define Azure and S3 storage.

[M0NsTeRRR]

One other thing I noticed is that a (empty) secret is created, despite the conditional on top of the secret.yaml template. Now we're checking if any of the secret_key_refs are missing, but that will almost always be the case, as I don't think anyone will define Azure and S3 storage.

Thanks, it should be fixed also

[M0NsTeRRR]

thanks @tcassaert, your tests are most appreciated.

I didn't notice that the config.storage section was copied into the config map. This is actually very bad because:

* it means that currently secrets are leaked to the config map

* it makes our current approach with `xxx_secret_key_ref` quite confusing.

I need a bit more time to figure out the best way to do this, as I'm starting to fear that a breaking change will be required.

I don't think we need a breaking change.

We use environment variables in config and instruct the user to replace the current value with ${SECRET_VARIABLE}.

[M0NsTeRRR]

@rdettai I've pushed a commit to address the issue, I let you check and tell me if it's fine for you :)

[rdettai]

I don't think we need a breaking change.

The problem we have here is that the existing experience is already confusing.

In general, merging configurations from multiple sources is error prone, so we need try to make them as simple as possible. Quickwit does this by having a single config file that can be tuned using environment variables:

• some parameters are exposed out of the box as env variable (e.g QW_CLUSTER_ID)
• others can be manually exposed using the environment variables in config mentioned by @M0NsTeRRR

The helm charts does some re-configurations on top of this by:

• moving some values from the config.storage into env variables, trying to force them into secret
• reassembling fields from config.postgres that don't exist in the original configuration into the QW_METASTORE_URI env variable
• taking the config.indexes and config.sources that also don't exist in the original configuration and running a seeding task from them

I think this is confusing and hard to maintain (as we can see from the very lengthy discussions in this PR, sorry for that!). The breaking change is required to streamline all of this. My suggestion would be to switch to the following:

• the config section is exactly what is copied to the node.yaml value of the config map
• the only values that are injected automatically by Helm as env variables are QW_CLUSTER_ID, QW_NODE_ID, QW_PEER_SEEDS and QW_ADVERTISE_ADDRESS because those are filled using K8S provided metadata.namespace, metadata.name and status.podIP field refs. We write an explicit warning so that users are not surprised that these configs get overridden if they try to set them through the config file.
• env variables can be set either using:
  • the existing environment section
  • a new extraEnvFrom section
  • the existing [searcher|indexer|...]ExtraEnv sections
  • new [searcher|indexer|...]ExtraEnvFrom sections
• users now need to build the QW_METASTORE_URI themselves (postgres://$(USERNAME):$(PASSWORD)@$(HOST):$(PORT)/$(DATABASE)), it's treated as any other environment variable
• the config.indexes and config.sources seed sections become seed.indexes and seed.sources

@M0NsTeRRR this if functionally very close to your initial proposition (i.e you can now provide any config as secret). The difference is that by accepting a breaking change, we make the user experience simpler and improve maintainability. Would this work for you?

@guilload @tcassaert any opinion about this?

[M0NsTeRRR]

(as we can see from the very lengthy discussions in this PR, sorry for that!)

No worries, testing is part of the work to get a better implementation. It will help us build a better Helm chart. 😉

It's hard to maintain, yes, but for user experience, it's better to limit breaking changes. I mean, if we introduce a breaking change now, we should do it for others too (I don't know if there are other breaking changes planned).

Letting users manage environment variables themselves will allow them to handle new configurations easily without having to change the Helm chart (they'll just need to change the image tag). As I've mentioned before, this is a good point, but the documentation should be precise about which environment variables are needed for which components. That is the only caveat.

[M0NsTeRRR]

Let me know if I've misunderstood something from your last comment, @rdettai. I'll let you review this and tell me what I need to adjust regarding the structure. 😊

[tcassaert]

I haven't tested it out just yet, I only found something that might confuse users about the seeding.

First they need to enable bootstrap to then set the config for that under seed. It might be more clear to have it all under the same root key? Wether seed or bootstrap is the most clear option is up to you guys to decide of course.

[M0NsTeRRR]

@tcassaert thanks for your review :)

[tcassaert]

@rdettai I think we're on the good way!

One thing that I found not that user-friendly is the configuration for the metastore. First having to define all Postgres env variable to then also build the QW_METASTORE_URI is not ideal. Can't we have a metastore key in values.yaml with the config under it, to then build the QW_METASTORE_URI automatically?

[M0NsTeRRR]

@rdettai I think we're on the good way!

One thing that I found not that user-friendly is the configuration for the metastore. First having to define all Postgres env variable to then also build the QW_METASTORE_URI is not ideal. Can't we have a metastore key in values.yaml with the config under it, to then build the QW_METASTORE_URI automatically?

You could guess it sometimes but not everytime. If you use extraEnvFrom with a secret you can't build the QW_METASTORE_URI.

[tcassaert]

@rdettai I think we're on the good way! One thing that I found not that user-friendly is the configuration for the metastore. First having to define all Postgres env variable to then also build the QW_METASTORE_URI is not ideal. Can't we have a metastore key in values.yaml with the config under it, to then build the QW_METASTORE_URI automatically?

You could guess it sometimes but not everytime. If you use extraEnvFrom with a secret you can't build the QW_METASTORE_URI.

Good point. Then it's probably a matter of documenting it.

[mzupan]

the envFrom is really helpful. I want to deploy postgresql bitnami helm chart in the same namespace. It has the ability to autocreate a password and set it as a secret. I just want to tell quickwit helm chart to pull this secret as this ENV var.

So just formatting it isn't the best option and we should have a way to load using envFrom or load secrets into env vars in the chart

[rdettai]

Yes, I agree this is a very important feature. We just want to be sure to get it right. @idrissneumann did you get a chance to take a look, what is your opinion on this?

[maxsargentdev]

I also would like to see this get merged soon, you cant be expecting people to supply a sensitive piece of configuration in the values file....

The chart should allow you to provide a secret reference, this then gets mounted and read from disk during intiialization to avoid having the password stored in the environment variables. This means that you can pull secrets with any method you want, i.e external secrets operator, sealed secrets, vault agent and so on.

That being said, envFrom should be supported anyway as a generally useful feature of a helm chart :)

[M0NsTeRRR]

Hello,

Thanks for your message, but it generates extra noise (like emails) and clutters the history we're using to track current work. I would appreciate it if we could keep this history clean. I think the issue is more appropriate or you can add a thumbs-up on the PR. On PRs, we should focus on technical debate and enhancement of the current contribution.

The maintainers of the helm chart agree with the idea; it's just a matter of time. However, as @rdettai mentioned, we need to do it properly to avoid reworking on it.

PS: This is not a comment about your message in particular, but since this is the second time, I prefer to post it to avoid more messages like this. Like you, I hope it gets merged soon 👍.

[idrissneumann]

Hi.

Yes, I agree this is a very important feature. We just want to be sure to get it right. @idrissneumann did you get a chance to take a look, what is your opinion on this?

I'll have a more deep look on this, but I think we should provide those three ways to inject environment variables (it's a standard coming from the bitnami charts style):

# Classic way to inject plain text values as environment variable
extraEnvVars:
   - name: FOO
     value: bar

# Inject all the environment variables contained in the secrets references as environment variables
extraEnvVarsSecrets:
  - postgresql-secret
  - redis-secret

# Inject all the environment variables contained in the configmaps references as environment variables
extraEnvVarsCMs:
  - my-config

This way, it'll be easy to delegate the creation of your secrets by something more secured (vault, sealedsecret, etc) and re-use the unsealed secrets without leaking sensitive data in your values.yaml.

The template should looks like:

env:
    - name: POD_NAME
        valueFrom:
        fieldRef:
            fieldPath: metadata.name
    {{- if .Values.extraEnvVars }}
    {{- include "common.tplvalues.render" (dict "value" .Values.extraEnvVars "context" $) | nindent 12 }}
    {{- end }}
    {{- if or .Values.extraEnvVarsCMs .Values.extraEnvVarsSecrets }}
    envFrom:
    {{- if .Values.extraEnvVarsCMs }}
    {{- range .Values.extraEnvVarsCMs }}
    - configMapRef:
        name: {{ . }}
    {{- end }}
    {{- end }}
    {{- if .Values.extraEnvVarsSecrets }}
    {{- range .Values.extraEnvVarsSecrets }}
    - secretRef:
        name: {{ . }}
    {{- end }}
    {{- end }}
    {{- end }}

We can find more example in the bitnami helmcharts like the PostgreSQL's one for example: https://github.com/bitnami/charts/blob/main/bitnami/postgresql/templates/primary/statefulset.yaml

And we can re-use the bitnami common templates as dependancy:

dependencies:
  - name: common
    repository: https://charts.bitnami.com/bitnami
    tags:
      - bitnami-common
    version: 1.x.x

I think this way we'll follow a well-known standard at least and it'l be easier for the user.

[M0NsTeRRR]

Hello @idrissneumann thanks for your review :)

And we can re-use the bitnami common templates as dependancy

I'm personally not a big fan of using Bitnami Helm charts due to many breaking changes in the past. Our use case is pretty simple, and I don't think we need a dependency for that anyway.

I think this way we'll follow a well-known standard at least and it'l be easier for the user.

I haven't seen this way of writing Helm charts except in Bitnami's implementation. I've always seen the current implementation in the PR (e.g., ArgoCD, Grafana, kube-prom-stack, CNPG, cert-manager, etc.).

I find the current implementation more native as it exposes it as raw Kubernetes config and lets the user handle configMap and secret mapping. I will let you decide which one to pick as both end in the same result.

[idrissneumann]

Hi @M0NsTeRRR .

I understand also your point of view. It's a non-mandatory suggestion, your way looks good to me too.

No issue on my side to keep extraEnvFrom in the value file and let the user of the chart decide to do the mapping with existing secrets or configmap references :)

And your comment on the values file is also useful to help the user to know how it works (maybe adding one with configMapRef could be useful as well).

[M0NsTeRRR]

Hi @M0NsTeRRR .

I understand also your point of view. It's a non-mandatory suggestion, your way looks good to me too.

No issue on my side to keep extraEnvFrom in the value file and let the user of the chart decide to do the mapping with existing secrets or configmap references :)

And your comment on the values file is also useful to help the user to know how it works (maybe adding one with configMapRef could be useful as well).

Okay, I just added my thoughts to let @rdettai and you decide which one to implement :)

Adding configMapRef with extraEnvFrom is redundant, no ? as you have two ways to achieve the same things or I didn't understand your answer

[idrissneumann]

Test ok:

Value to reproduce the test:

---
config:
  default_index_root_uri: s3://qw-indexes
  storage:
    s3:
      endpoint: https://fr-par.scw.cloud
      region: fr-par
      access_key_id: SCWXXXXXX
      secret_access_key: XXXXX

environment:
  QW_METASTORE_URI: s3://qw-indexes

# Testing purpose
my_secret: bar

indexer:
  extraEnvFrom:
    - secretRef:
        name: my-secret

And with a temporary secret template:

---
apiVersion: v1
kind: Secret
metadata:
  name: my-secret
type: Opaque
data:
  FOO: {{ .Values.my_secret | b64enc | quote }}

And the shell command:

# Check the yaml output of the helm template
helm template . --values values.yaml --values values2.yaml | grep -i envFrom -A2

# Deploy the current helm template output on kind
kubectl create ns quickwit && helm template . --values values.yaml --values values2.yaml --namespace quickwit | kubectl -n quickwit apply -f - > /dev/null 2>&1

# Check if the FOO environment variable is present in the indexer pod
kubectl -n quickwit exec -it release-name-quickwit-indexer-0 env 2>/dev/null|grep "FOO="

And quickwit is still working fine :)

[M0NsTeRRR]

Looks great! I think we need an entry in the readme like this one to help users adjust to the breaking change and we are good to go!

Could you do it? I haven't played much with Quickwit yet as I was waiting for this PR to be merged. Therefore, I don't have much knowledge or history about it (and the Helm chart). You should have the right to edit my PR or provide suggested changes.

[idrissneumann]

Could you do it? I haven't played much with Quickwit yet as I was waiting for this PR to be merged. Therefore, I don't have much knowledge or history about it (and the Helm chart). You should have the right to edit my PR or provide suggested changes.

@M0NsTeRRR I opened a PR to be merged inside your branch if you want: https://github.com/M0NsTeRRR/quickwit-charts/pull/1

The seed is the only breaking change I identify so far, and this PR is documenting it. The version also is changed to 0.6.0 because there is a breaking change :)

[M0NsTeRRR]

Could you do it? I haven't played much with Quickwit yet as I was waiting for this PR to be merged. Therefore, I don't have much knowledge or history about it (and the Helm chart). You should have the right to edit my PR or provide suggested changes.

@M0NsTeRRR I opened a PR to be merged inside your branch if you want: M0NsTeRRR#1

The seed is the only breaking change I identify so far, and this PR is documenting it. The version also is changed to 0.6.0 because there is a breaking change :)

Thanks you :)