Closed M0NsTeRRR closed 4 months ago
Sorry I didn't have the time to test this PR yet, I'll do it early next week. Looking good overall!
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.
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'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,
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 becauses3
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.
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!
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_ref
s are missing, but that will almost always be the case, as I don't think anyone will define Azure and S3 storage.
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 thesecret_key_ref
s 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
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}
.
@rdettai I've pushed a commit to address the issue, I let you check and tell me if it's fine for you :)
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:
QW_CLUSTER_ID
)The helm charts does some re-configurations on top of this by:
config.storage
into env variables, trying to force them into secretconfig.postgres
that don't exist in the original configuration into the QW_METASTORE_URI
env variableconfig.indexes
and config.sources
that also don't exist in the original configuration and running a seeding task from themI 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:
config
section is exactly what is copied to the node.yaml
value of the config mapQW_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.environment
sectionextraEnvFrom
section[searcher|indexer|...]ExtraEnv
sections[searcher|indexer|...]ExtraEnvFrom
sectionsQW_METASTORE_URI
themselves (postgres://$(USERNAME):$(PASSWORD)@$(HOST):$(PORT)/$(DATABASE)
), it's treated as any other environment variableconfig.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?
(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.
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. 😊
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.
@tcassaert thanks for your review :)
@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?
@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 ametastore
key invalues.yaml
with the config under it, to then build theQW_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
.
@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 ametastore
key invalues.yaml
with the config under it, to then build theQW_METASTORE_URI
automatically?You could guess it sometimes but not everytime. If you use
extraEnvFrom
with a secret you can't build theQW_METASTORE_URI
.
Good point. Then it's probably a matter of documenting it.
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
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 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 :)
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 👍.
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.
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.
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).
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
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 :)
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.
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 :)
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 to0.6.0
because there is a breaking change :)
Thanks you :)
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:
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. @M0NsTeRRR I think this change https://github.com/quickwit-oss/helm-charts/pull/85#discussion_r1650528350 didn´t get in :smile:
@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
question: is it possible to use config.metastore_uri
in the chart? (instead of using QW_METASTORE_URI env variable)
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 :
I think we can close #81 #62 #59 #48 with this PR
PS : Thanks everyone for the job done here :)
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.