-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for Logstash secure settings from Kubernetes Secrets using keystore #7024
Conversation
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.
One question about whether we need to set a default password for the keystore.
pkg/controller/logstash/keystore.go
Outdated
}, | ||
} | ||
|
||
DefaultKeystorePass = corev1.EnvVar{Name: KeystorePassKey, Value: "changeit"} |
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.
Do we need to set a default value? The keystore should would work without a password (even though we don't recommend it), and I'm not sure what this buys us.
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.
Without the password in env, logstash-keystore
prompts user to input [y/N] to continue.
So, we need to either give [N] to stdin or set the default 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.
We should be able to use yes
to send the input to stdin, so you could change the keystoreCommand
to
keystoreCommand = "yes | /usr/share/logstash/bin/logstash-keystore"
to pipe y
in
or
keystoreCommand = "yes n | /usr/share/logstash/bin/logstash-keystore"
to pipe n
in.
I'm not sure it matters a huge amount, but it might be easier to document if we don't set a default that doesn't really do anything
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.
oh! I am such a fool :(
will change the command
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.
Not at all!
And thank you!
[id="{p}-logstash-keystore"] | ||
=== Setting keystore | ||
|
||
You can specify sensitive settings with Kubernetes secrets. ECK automatically injects these settings into the keystore on each Logstash before it starts Logstash. The ingestion process could extend the startup time of Logstash pod significantly. |
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 would add this as a technical preview NOTE, rather than in the main text, and add that the startup time increases linearly with the number of variables added.
Something like:
NOTE: For the technical preview, the use of settings in the Logstash keystore may impact startup time for Logstash Pods. Startup time will increase linearly for each entry added to the keystore, and this could extend startup time significantly.
pkg/controller/logstash/keystore.go
Outdated
}, | ||
} | ||
|
||
DefaultKeystorePass = corev1.EnvVar{Name: KeystorePassKey, Value: "changeit"} |
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.
We should be able to use yes
to send the input to stdin, so you could change the keystoreCommand
to
keystoreCommand = "yes | /usr/share/logstash/bin/logstash-keystore"
to pipe y
in
or
keystoreCommand = "yes n | /usr/share/logstash/bin/logstash-keystore"
to pipe n
in.
I'm not sure it matters a huge amount, but it might be easier to document if we don't set a default that doesn't really do anything
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
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.
Code LGTM - couple of suggestions on doc.
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
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.
LGTM I guess another alternative to allowing batch adding to the keystore would be to optimise the startup time of the keystore commend with JVM techniques like CDS or Graal's native image.
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
docs/orchestrating-elastic-stack-applications/logstash.asciidoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
…keystore # Conflicts: # docs/orchestrating-elastic-stack-applications/logstash.asciidoc
@pebrc Could you merge the PR? I don't have the permission to do so. |
This commit adds support for keystore to Logstash operator.
The key values in keystore are available to Logstash pipelines as environment variables, which can resolve by ${KEY} notation. The keystore can be password protected by setting an environment variable called
LOGSTASH_KEYSTORE_PASS
. The password is expected to be declared in the main container in env.Sample config with customized keystore password
A known issue is that the keystore command
logstash-keystore
is very slow in proportion to the number of key values to add. In my local machine, adding 10 keys needs 6 minutes to start Logstsah.Adding or updating key values in keystore triggers pod rotation, while deleting a key does not.
This commit adds e2e tests
TestLogstashKeystoreWithoutPassword
andTestLogstashKeystoreWithPassword
A follow-up issue for improving the keystore command performance