-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
fd453a1
to
efdcb6c
Compare
efdcb6c
to
9305a54
Compare
elasticsearch/README.md
Outdated
path: /usr/share/elasticsearch/config/elasticsearch.keystore | ||
subPath: elasticsearch.keystore | ||
``` | ||
Take a look a the [config example](/elasticsearch/examples/config/values.yaml) which has a tested version of adding strings and files to the keystore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be helpful to have a note explicitly stating that any external values (secrets, etc) need to be explicitly set as env vars (which is what the example does). Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved as-is. Small comments inline for optional changes.
strings: | ||
bootstrap.password: '${ELASTIC_PASSWORD}' | ||
xpack.notification.slack.account.monitoring.secure_url: '${SLACK_URL}' | ||
files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cloud-on-k8s doesn't have a specific concept of file vs string keystore entries: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-es-secure-settings.html
I don't recall why that is, but I wonder if we can have the same concepts for this on both the chart and the operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember a change was made (initiated by the cloud-on-k8s team) to have the Elasticsearch keystore treat all strings and files the same internally.
My original assumption was that the operator would be doing something special to take these secrets, generate a keystore and create that as a secret. But their approach is also fully init container based. All secrets are mounted as files and then the init script loops over them and adds them to the keystore.
I like this a lot more than my solution, mainly because you don't need to think about mapping the files and strings into the container yourself. And the other huge security advantage is that these secrets won't still exist as environment variables or mounts in the actual running Elasticsearch container.
One downside to this approach is that it looks like the spec only allows a single secret. The downside of this being that you now need to fully recreate your keystore secret everytime you want to add something new.
Making it a list would solve this so I'll ping the team and see what their thoughts on it are.
spec:
secureSettings:
- secretName: your-secure-settings-secret
- secretName: other-secure-settings-secret
I'm going to take a stab at implementing this in the same way.
9305a54
to
3e99a26
Compare
@jordansissel I have updated it to match how cloud-on-k8s is doing it (which is a much better approach). Issues have also been opened to implement the suggestions I had around allowing multiple secrets, and the ability to set all parts of the volumeMount spec to allow specifying a different path than the key of the secret. elastic/cloud-on-k8s#1457 Please take another look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found an edge case on the bootstrap.password. Other comments are non-problem/style.
I like the approach to allow multiple secret mounts to provide keystore values. 👍
- name: keystore | ||
emptyDir: {} | ||
{{ end }} | ||
{{- range .Values.keystore }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why not have this {{- range ...}}
inside the {{ if ...
block above? I I don't think there'd be any behavior changes, just curious about the style choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a leftover from the original refactoring. But you are right that it doesn't change anything. I fixed it up to make it as explicit as possible.
@@ -111,6 +111,14 @@ spec: | |||
configMap: | |||
name: {{ template "uname" . }}-config | |||
{{- end }} | |||
{{ if .Values.keystore }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to indent this and use {{- if ...
? Not sure if we're concerned with indentation style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! It would be nice to add a "white space detection" check in the pull request testing and do a one off PR to get things back in line. Right now its a bit of a mess. It's a shame that helm lint --strict
doesn't already cover this.
done | ||
|
||
# Add the bootstrap password since otherwise the Elasticsearch entrypoint tries to do this on startup | ||
[ ! -z "$ELASTIC_PASSWORD" ] && echo $ELASTIC_PASSWORD | elasticsearch-keystore add -x bootstrap.password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs quotation marks around the second var, echo "$ELASTIC_PASSWORD"
otherwise a password with whitespace won't be set correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be too complicated to add to the tests, but maybe we could add a test to cover this case (whitespace in a password, such as ELASTIC_PASSWORD="hello world "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Fixed! I thought I had copy pasted this from the ELasticsearch docker file, but looks like my copy paste skills failed since it is properly quoted in there: https://github.com/elastic/elasticsearch/blob/31f6e78acdb8dfc7c042be4ad0bc0bdea003e055/distribution/docker/src/docker/bin/docker-entrypoint.sh#L88
Thanks for the review @jordansissel, all comments have been addressed. Please take another look :) |
Closes: #90 Adds a kubernetes native way to add strings and files to the Elasticsearch keystore. Previously you needed to manually create the keystore and upload it as a secret. There were a couple of issues with this approach. 1. The Elasticsearch keystore has an internal version for the format. If this is changed it meant needing to recreate each keystore again. 2. If you wanted to add a single new value it meant recreating the entire keystore again
9583f93
to
d025a9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎈 Ship it!
Thanks for being patient with my having forgotten to re-review this periodically :)
❤️ And thank you for the great suggestions and finding bugs! |
@Crazybus What is the correct usage for "keystore: []" in values.yaml? I try several different format for '[]' format and it was not working.
|
The simpliest example is: helm-charts/elasticsearch/examples/config/values.yaml Lines 25 to 26 in a499955
With the secret created like this:
|
@Crazybus Thanks. |
i have tried adding GCS service account key in keystone but getting error created kubernetes secret and added file into it and added secret into keystore but pod are crashing also my i am using custom docker image with plugin installed but getting error like :
created secret using
here
Update Image running and ready 1/1.
please correct me if missing anything or wrong way i want to set cronjob for setting regular backup let me know if missing anything thanks in advance for help. |
${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml