Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

[elasticsearch] Keystore integration #154

Merged
merged 2 commits into from
Aug 23, 2019
Merged

Conversation

Crazybus
Copy link
Contributor

@Crazybus Crazybus commented Jun 6, 2019

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

@Crazybus Crazybus force-pushed the configurator_mapinator branch 2 times, most recently from fd453a1 to efdcb6c Compare July 22, 2019 08:19
@Crazybus Crazybus changed the title WIP: Proof of concept: Keystore integration [elasticsearch] Keystore integration Jul 22, 2019
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.
Copy link
Contributor

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?

jordansissel
jordansissel previously approved these changes Aug 1, 2019
Copy link
Contributor

@jordansissel jordansissel left a 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:
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@Crazybus
Copy link
Contributor Author

Crazybus commented Aug 2, 2019

@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
elastic/cloud-on-k8s#1458

Please take another look :)

Copy link
Contributor

@jordansissel jordansissel left a 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 }}
Copy link
Contributor

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.

Copy link
Contributor Author

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 }}
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor

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 "

Copy link
Contributor Author

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

@Crazybus
Copy link
Contributor Author

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
Copy link
Contributor

@jordansissel jordansissel left a 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 :)

@Crazybus
Copy link
Contributor Author

❤️

And thank you for the great suggestions and finding bugs!

@Crazybus Crazybus merged commit 1ad3826 into master Aug 23, 2019
@Crazybus Crazybus deleted the configurator_mapinator branch August 23, 2019 21:31
@dcvtruong
Copy link

dcvtruong commented Sep 12, 2019

@Crazybus What is the correct usage for "keystore: []" in values.yaml? I try several different format for '[]' format and it was not working.

keystore: 
  - xpack.notification.slack.account.monitoring.secure_url: 'https://webhook_url.....'

@Crazybus
Copy link
Contributor Author

The simpliest example is:

keystore:
- secretName: elastic-config-secret

With the secret created like this:

kubectl create secret generic elastic-config-slack --from-literal=xpack.notification.slack.account.monitoring.secure_url='https://hooks.slack.com/services/asdasdasd/asdasdas/asdasd'

@dcvtruong
Copy link

@Crazybus Thanks.

@harsh4870
Copy link

harsh4870 commented Jul 28, 2020

@Crazybus

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 :

csearch", "node.name": "elasticsearch-master-0",  "message": "uncaught exception in thread [main]" ,
"stacktrace": ["org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: unknown secure setting [gcs_backup_key.json] please check that any required plugins are installed, or check the breaking changes documentation for removed settings",
"at org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:163) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Elasticsearch.execute(Elasticsearch.java:150) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:86) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:124) ~[elasticsearch-cli-7.3.2.jar:7.3.2]",
"at org.elasticsearch.cli.Command.main(Command.java:90) ~[elasticsearch-cli-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:115) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:92) ~[elasticsearch-7.3.2.jar:7.3.2]",
"Caused by: java.lang.IllegalArgumentException: unknown secure setting [gcs_backup_key.json] please check that any required plugins are installed, or check the breaking changes documentation for removed settings",
"at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:531) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:476) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:447) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:418) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.common.settings.SettingsModule.<init>(SettingsModule.java:149) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.node.Node.<init>(Node.java:357) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.node.Node.<init>(Node.java:258) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:221) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Bootstrap.setup(Bootstrap.java:221) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Bootstrap.init(Bootstrap.java:349) ~[elasticsearch-7.3.2.jar:7.3.2]",
"at org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:159) ~[elasticsearch-7.3.2.jar:7.3.2]",
"... 6 more"] }

created secret using

kubectl create secret generic gcs-backup-key --from-file=gcs_backup_key.json=gcs_backup_key.json

here

keystore:
  - secretName: gcs-backup-key

Update

Image running and ready 1/1.

secretMounts:
  - name: gcs-backup-key
    secretName: gcs-backup-key
    path: /usr/share/elasticsearch/config/gcs_backup_key.json
    subPath: gcs_backup_key.json

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants