feat: add support for extraEnvFrom #85

M0NsTeRRR · 2024-05-09T16:16:40Z

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.

Signed-off-by: Ludovic Ortega <[email protected]>

rdettai

Thanks for this contribution! This is definitively a gap we need to fill. I have proposed an alternative approach in the comment below, I'm curious about your opinion 🙂

charts/quickwit/values.yaml

Signed-off-by: Ludovic Ortega <[email protected]>

rdettai

Huge thanks for persevering on this PR! I think we are almost there and it will be a great addition.

charts/quickwit/templates/_helpers.tpl

charts/quickwit/values.yaml

rdettai · 2024-05-14T09:44:56Z

charts/quickwit/templates/indexer-statefulset.yaml

@@ -50,7 +50,7 @@ spec:
          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          env:
-            {{- include "quickwit.environment" . | nindent 12 }}
+            {{- include "quickwit.metastore.environment" . | nindent 12 }}


Thanks for adding this. Even though the description in #81 only mentions the searcher/indexer, I think the janitor and control plane should also get the metastore configuration.

Okay, I will update this :)

quickwit.environment is now unused with this change do you want to keep it separated for a future feature in an other PR ? Or i merge metastore definition in environment ?

Co-authored-by: Remi Dettai <[email protected]>

rdettai · 2024-05-16T20:02:17Z

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

tcassaert · 2024-05-22T12:02:31Z

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 · 2024-05-22T12:52:16Z

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.

Signed-off-by: Ludovic Ortega <[email protected]>

M0NsTeRRR · 2024-05-22T12:59:54Z

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 · 2024-05-22T13:12:16Z

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 · 2024-05-22T13:25:21Z

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 · 2024-05-22T13:31:36Z

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.

Signed-off-by: Ludovic Ortega <[email protected]>

M0NsTeRRR · 2024-05-22T13:54:12Z

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

rdettai

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.

charts/quickwit/templates/secret.yaml

idrissneumann

LGTM (I'll perform some test on kind tomorrow and publish the result here)

idrissneumann · 2024-06-19T10:15:51Z

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 :)

rdettai

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!

charts/quickwit/values.yaml

Co-authored-by: Remi Dettai <[email protected]>

M0NsTeRRR · 2024-06-20T08:34:43Z

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.

…ngly Signed-off-by: Idriss Neumann <[email protected]>

idrissneumann · 2024-06-20T09:35:25Z

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 :)

…ngly

M0NsTeRRR · 2024-06-20T09:38:50Z

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 :)

rdettai · 2024-06-24T07:41:56Z

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 :)

seed is not the only nor the most important breaking change. We have changed the way the Values.config section works entirely. The config section is now copied as is to the ConfigMap of the Quickwit nodes. In particular:

the config.postgres section does not support the following attributes:

host: ""
port: 5432
database: metastore
username: quickwit
assword: ""

s3.secret_access_key and azure.access_key are not copied to secrets anymore. We now recommend using extraEnvFrom to provide secrets.

README.md

charts/quickwit/values.yaml

Co-authored-by: Remi Dettai <[email protected]>

rdettai · 2024-06-24T08:30:31Z

@M0NsTeRRR I think this change #85 (comment) didn´t get in 😄

M0NsTeRRR · 2024-06-24T08:32:14Z

@M0NsTeRRR I think this change #85 (comment) didn´t get in 😄

Oh yes, I used github web commit and it only added a few lines

fmassot · 2024-06-24T11:00:19Z

question: is it possible to use config.metastore_uri in the chart? (instead of using QW_METASTORE_URI env variable)

M0NsTeRRR · 2024-06-24T12:11:24Z

question: is it possible to use config.metastore_uri in the chart? (instead of using QW_METASTORE_URI env variable)

Using

config:
  version: 0.8
  listen_address: 0.0.0.0
  gossip_listen_port: 7282
  data_dir: /quickwit/qwdata
  default_index_root_uri: s3://quickwit/indexes
  metastore_uri: s3://quickwit/indexes

render

---
# Source: quickwit/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: release-name-quickwit
  labels:
    helm.sh/chart: quickwit-0.6.0
    app.kubernetes.io/name: quickwit
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "v0.8.1"
    app.kubernetes.io/managed-by: Helm
data:
  node.yaml: |-
    data_dir: /quickwit/qwdata
    default_index_root_uri: s3://quickwit/indexes
    gossip_listen_port: 7282
    listen_address: 0.0.0.0
    metastore_uri: s3://quickwit/indexes
    version: 0.8

The configmap is mounted to :

quickwit-control-plane
quickwit-janitor
quickwit-metastore
quickwit-indexer
quickwit-searcher

M0NsTeRRR · 2024-06-25T15:48:39Z

I think we can close #81 #62 #59 #48 with this PR

PS : Thanks everyone for the job done here :)

M0NsTeRRR added 2 commits May 9, 2024 18:13

feat: add support for extraEnvFrom

d14e5d6

Signed-off-by: Ludovic Ortega <[email protected]>

fix: remove empty line

605728c

Signed-off-by: Ludovic Ortega <[email protected]>

guilload requested a review from rdettai May 9, 2024 16:26

rdettai reviewed May 13, 2024

View reviewed changes

charts/quickwit/values.yaml Outdated Show resolved Hide resolved

M0NsTeRRR added 3 commits May 13, 2024 17:08

feat: add discussed changes

6c0145f

Signed-off-by: Ludovic Ortega <[email protected]>

fix: don't set metastore uri when not needed

a2cf849

Signed-off-by: Ludovic Ortega <[email protected]>

fix: azure account placeholder

4475a88

Signed-off-by: Ludovic Ortega <[email protected]>

rdettai reviewed May 14, 2024

View reviewed changes

M0NsTeRRR and others added 11 commits May 14, 2024 11:51

fix: typo

875a08f

Co-authored-by: Remi Dettai <[email protected]>

fix: typo

6b7e435

Co-authored-by: Remi Dettai <[email protected]>

fix: typo

f3f4be4

Co-authored-by: Remi Dettai <[email protected]>

fix: typo

23fd121

Co-authored-by: Remi Dettai <[email protected]>

fix: secret ref placeholder

68c4c1f

Co-authored-by: Remi Dettai <[email protected]>

fix: secret ref placeholder

30f49df

Co-authored-by: Remi Dettai <[email protected]>

fix: secret ref placeholder

4a345da

Co-authored-by: Remi Dettai <[email protected]>

fix: env ordering & postgres host required

59c8eb9

fix: postgres default values

0ae5993

feat: update janitor deployment to support metastore env vars

5a78d5c

feat: update control-plane deployment to support metastore env vars

e60b5cc

fix: quickwit configmap

5b5f2dc

Signed-off-by: Ludovic Ortega <[email protected]>

fix: quickwit secret empty

2316288

Signed-off-by: Ludovic Ortega <[email protected]>

rdettai reviewed May 24, 2024

View reviewed changes

charts/quickwit/templates/secret.yaml Outdated Show resolved Hide resolved

idrissneumann approved these changes Jun 18, 2024

View reviewed changes

idrissneumann mentioned this pull request Jun 18, 2024

feat(node-config): add rest section #89

Closed

rdettai reviewed Jun 20, 2024

View reviewed changes

charts/quickwit/values.yaml Show resolved Hide resolved

charts/quickwit/values.yaml Show resolved Hide resolved

fix: add QW_METASTORE_URI doc

9055a0f

Co-authored-by: Remi Dettai <[email protected]>

chore: document the breaking change and change the versioning accordi…

8f0d1ed

…ngly Signed-off-by: Idriss Neumann <[email protected]>

chore: document the breaking change and change the versioning accordi…

3ae4589

…ngly

rdettai reviewed Jun 24, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

charts/quickwit/values.yaml Outdated Show resolved Hide resolved

fix: readme depreciation notice

01f86d2

Co-authored-by: Remi Dettai <[email protected]>

rdettai mentioned this pull request Jun 24, 2024

Add config.postgres.existingSecret #88

Closed

fix: add back indexer & search settings example

9ae752d

M0NsTeRRR requested a review from rdettai June 24, 2024 08:13

M0NsTeRRR added 2 commits June 24, 2024 10:33

fix: readme suggested changes

cbd1ddb

fix: readme missing newlines

e4ca142

rdettai approved these changes Jun 24, 2024

View reviewed changes

rdettai merged commit 172b111 into quickwit-oss:main Jun 25, 2024
1 check passed

M0NsTeRRR deleted the feat/support-extra-env-from branch June 25, 2024 11:34

idrissneumann mentioned this pull request Jun 25, 2024

add rest labels for v0.7 #62

Closed

This was referenced Jun 26, 2024

Load secrets from an existing secret #48

Closed

Allow to configure metastore config postgres num_max_connections #59

Closed

metastore uri should be set on non-metastore nodes. #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for extraEnvFrom #85

feat: add support for extraEnvFrom #85

M0NsTeRRR commented May 9, 2024 •

edited by rdettai

Loading

rdettai left a comment

rdettai left a comment

rdettai May 14, 2024

M0NsTeRRR May 14, 2024

M0NsTeRRR May 14, 2024 •

edited

Loading

rdettai commented May 16, 2024 •

edited

Loading

tcassaert commented May 22, 2024

tcassaert commented May 22, 2024

M0NsTeRRR commented May 22, 2024

M0NsTeRRR commented May 22, 2024 •

edited

Loading

tcassaert commented May 22, 2024

tcassaert commented May 22, 2024

M0NsTeRRR commented May 22, 2024

rdettai left a comment

idrissneumann left a comment •

edited

Loading

idrissneumann commented Jun 19, 2024 •

edited

Loading

rdettai left a comment

M0NsTeRRR commented Jun 20, 2024

idrissneumann commented Jun 20, 2024

M0NsTeRRR commented Jun 20, 2024

rdettai commented Jun 24, 2024

rdettai commented Jun 24, 2024 •

edited

Loading

M0NsTeRRR commented Jun 24, 2024

fmassot commented Jun 24, 2024

M0NsTeRRR commented Jun 24, 2024 •

edited

Loading

M0NsTeRRR commented Jun 25, 2024

feat: add support for extraEnvFrom #85

feat: add support for extraEnvFrom #85

Conversation

M0NsTeRRR commented May 9, 2024 • edited by rdettai Loading

rdettai left a comment

Choose a reason for hiding this comment

rdettai left a comment

Choose a reason for hiding this comment

rdettai May 14, 2024

Choose a reason for hiding this comment

M0NsTeRRR May 14, 2024

Choose a reason for hiding this comment

M0NsTeRRR May 14, 2024 • edited Loading

Choose a reason for hiding this comment

rdettai commented May 16, 2024 • edited Loading

tcassaert commented May 22, 2024

tcassaert commented May 22, 2024

M0NsTeRRR commented May 22, 2024

M0NsTeRRR commented May 22, 2024 • edited Loading

tcassaert commented May 22, 2024

tcassaert commented May 22, 2024

M0NsTeRRR commented May 22, 2024

rdettai left a comment

Choose a reason for hiding this comment

idrissneumann left a comment • edited Loading

Choose a reason for hiding this comment

idrissneumann commented Jun 19, 2024 • edited Loading

rdettai left a comment

Choose a reason for hiding this comment

M0NsTeRRR commented Jun 20, 2024

idrissneumann commented Jun 20, 2024

M0NsTeRRR commented Jun 20, 2024

rdettai commented Jun 24, 2024

rdettai commented Jun 24, 2024 • edited Loading

M0NsTeRRR commented Jun 24, 2024

fmassot commented Jun 24, 2024

M0NsTeRRR commented Jun 24, 2024 • edited Loading

M0NsTeRRR commented Jun 25, 2024

M0NsTeRRR commented May 9, 2024 •

edited by rdettai

Loading

M0NsTeRRR May 14, 2024 •

edited

Loading

rdettai commented May 16, 2024 •

edited

Loading

M0NsTeRRR commented May 22, 2024 •

edited

Loading

idrissneumann left a comment •

edited

Loading

idrissneumann commented Jun 19, 2024 •

edited

Loading

rdettai commented Jun 24, 2024 •

edited

Loading

M0NsTeRRR commented Jun 24, 2024 •

edited

Loading