Skip to content
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

Fix Logstash keystore performance #7642

Merged
merged 8 commits into from
Mar 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -997,8 +997,6 @@ spec:
You can specify sensitive settings with Kubernetes secrets. ECK automatically injects these settings into the keystore before it starts Logstash.
The ECK operator continues to watch the secrets for changes and will restart Logstash Pods when it detects a change.

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.

The Logstash Keystore can be password protected by setting an environment variable called `LOGSTASH_KEYSTORE_PASS`. Check out https://www.elastic.co/guide/en/logstash/current/keystore.html#keystore-password[Logstash Keystore] documentation for details.

[source,yaml,subs="attributes,+macros,callouts"]
Expand Down
12 changes: 11 additions & 1 deletion pkg/controller/common/keystore/initcontainer.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ type InitContainerParameters struct {
KeystoreAddCommand string
// Keystore create command
KeystoreCreateCommand string
// CustomScript is the bash script to overrides the default Keystore script
CustomScript string
// Resources for the init container
Resources corev1.ResourceRequirements
// SkipInitializedFlag when true do not use a flag to ensure the keystore is created only once. This should only be set
Expand Down Expand Up @@ -82,7 +84,7 @@ func initContainer(
privileged := false
tplBuffer := bytes.Buffer{}

if err := scriptTemplate.Execute(&tplBuffer, parameters); err != nil {
if err := getScriptTemplate(parameters.CustomScript).Execute(&tplBuffer, parameters); err != nil {
return corev1.Container{}, err
}

Expand All @@ -107,3 +109,11 @@ func initContainer(

return container, nil
}

func getScriptTemplate(customScript string) *template.Template {
if customScript == "" {
return scriptTemplate
}

return template.Must(template.New("").Parse(customScript))
}
113 changes: 53 additions & 60 deletions pkg/controller/common/keystore/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ var (
SecureSettings: []commonv1.SecretSource{testSecureSettingsSecretRef},
},
}

testResourceRequirements = corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
Limits: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
}
)

func fakeFlagInitContainersParameters(skipInitializedFlag bool) InitContainerParameters {
Expand All @@ -59,6 +70,30 @@ func fakeFlagInitContainersParameters(skipInitializedFlag bool) InitContainerPar
KeystoreAddCommand: `/keystore/bin/keystore add "$key" "$filename"`,
SecureSettingsVolumeMountPath: "/foo/secret",
KeystoreVolumePath: "/bar/data",
Resources: testResourceRequirements,
SkipInitializedFlag: skipInitializedFlag,
}
}

func wantContainer(wantScript string) *corev1.Container {
varFalse := false
return &corev1.Container{
Command: []string{
"/usr/bin/env",
"bash",
"-c",
wantScript,
},
VolumeMounts: []corev1.VolumeMount{
{
Name: "elastic-internal-secure-settings",
ReadOnly: true,
MountPath: "/mnt/elastic-internal/secure-settings",
},
},
SecurityContext: &corev1.SecurityContext{
Privileged: &varFalse,
},
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
Expand All @@ -69,12 +104,10 @@ func fakeFlagInitContainersParameters(skipInitializedFlag bool) InitContainerPar
corev1.ResourceCPU: resource.MustParse("100m"),
},
},
SkipInitializedFlag: skipInitializedFlag,
}
}

func TestReconcileResources(t *testing.T) {
varFalse := false
tests := []struct {
name string
client k8s.Client
Expand All @@ -98,12 +131,7 @@ func TestReconcileResources(t *testing.T) {
client: k8s.NewFakeClient(&testSecureSettingsSecret),
kb: testKibanaWithSecureSettings,
initContainerParameters: fakeFlagInitContainersParameters(false),
wantContainers: &corev1.Container{
Command: []string{
"/usr/bin/env",
"bash",
"-c",
`#!/usr/bin/env bash
wantContainers: wantContainer(`#!/usr/bin/env bash

set -eux

Expand All @@ -129,29 +157,7 @@ done

touch /bar/data/elastic-internal-init-keystore.ok
echo "Keystore initialization successful."
`,
},
VolumeMounts: []corev1.VolumeMount{
{
Name: "elastic-internal-secure-settings",
ReadOnly: true,
MountPath: "/mnt/elastic-internal/secure-settings",
},
},
SecurityContext: &corev1.SecurityContext{
Privileged: &varFalse,
},
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
Limits: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
},
},
`),
// since this will be created, it will be incremented
wantVersion: "1",
wantNil: false,
Expand All @@ -161,12 +167,7 @@ echo "Keystore initialization successful."
client: k8s.NewFakeClient(&testSecureSettingsSecret),
initContainerParameters: fakeFlagInitContainersParameters(true),
kb: testKibanaWithSecureSettings,
wantContainers: &corev1.Container{
Command: []string{
"/usr/bin/env",
"bash",
"-c",
`#!/usr/bin/env bash
wantContainers: wantContainer(`#!/usr/bin/env bash

set -eux

Expand All @@ -184,29 +185,7 @@ for filename in /foo/secret/*; do
done

echo "Keystore initialization successful."
`,
},
VolumeMounts: []corev1.VolumeMount{
{
Name: "elastic-internal-secure-settings",
ReadOnly: true,
MountPath: "/mnt/elastic-internal/secure-settings",
},
},
SecurityContext: &corev1.SecurityContext{
Privileged: &varFalse,
},
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
Limits: map[corev1.ResourceName]resource.Quantity{
corev1.ResourceMemory: resource.MustParse("128Mi"),
corev1.ResourceCPU: resource.MustParse("100m"),
},
},
},
`),
// since this will be created, it will be incremented
wantVersion: "1",
wantNil: false,
Expand All @@ -219,6 +198,20 @@ echo "Keystore initialization successful."
wantVersion: "",
wantNil: true,
},
{
name: "use custom script",
client: k8s.NewFakeClient(&testSecureSettingsSecret),
kb: testKibanaWithSecureSettings,
initContainerParameters: InitContainerParameters{
CustomScript: `echo "custom script"`,
Resources: testResourceRequirements,
SkipInitializedFlag: false,
},
wantContainers: wantContainer(`echo "custom script"`),
// since this will be created, it will be incremented
wantVersion: "1",
wantNil: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
49 changes: 46 additions & 3 deletions pkg/controller/logstash/keystore.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,53 @@ const (
)

var (
keystoreCommand = "echo 'y' | /usr/share/logstash/bin/logstash-keystore"
// containerCommand runs in every pod creation to regenerate keystore.
// `logstash-keystore` allows for adding multiple keys in a single operation.
// All keys and values must be ASCII and non-empty string. Values are input via stdin, delimited by \n.
containerCommand = `#!/usr/bin/env bash

set -eu

{{ if not .SkipInitializedFlag -}}
keystore_initialized_flag={{ .KeystoreVolumePath }}/elastic-internal-init-keystore.ok

if [[ -f "${keystore_initialized_flag}" ]]; then
echo "Keystore already initialized."
exit 0
fi

{{ end -}}
echo "Initializing keystore."

# create a keystore in the default data path
{{ .KeystoreCreateCommand }}

# add all existing secret entries to keys (Array), vals (String).
for filename in {{ .SecureSettingsVolumeMountPath }}/*; do
[[ -e "$filename" ]] || continue # glob does not match
key=$(basename "$filename")
keys+=("$key")
vals+=$(cat "$filename")
vals+="\n"
done

# remove the trailing '\n' from the end of the vals
vals=${vals%'\n'}

# add multiple keys to keystore
{{ .KeystoreAddCommand }}

{{ if not .SkipInitializedFlag -}}
touch {{ .KeystoreVolumePath }}/elastic-internal-init-keystore.ok
{{ end -}}

echo "Keystore initialization successful."
`

initContainersParameters = keystore.InitContainerParameters{
KeystoreCreateCommand: keystoreCommand + " create",
KeystoreAddCommand: keystoreCommand + ` add "$key" < "$filename"`,
KeystoreCreateCommand: "echo 'y' | /usr/share/logstash/bin/logstash-keystore create",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, we might want to think about using both KeystoreCreateCommand and KeystoreAddCommand here, and in the script, or just calling out to logstash-keystore directly in the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the command for consistency

KeystoreAddCommand: `echo -e "$vals" | /usr/share/logstash/bin/logstash-keystore add "${keys[@]}"`,
CustomScript: containerCommand,
SecureSettingsVolumeMountPath: keystore.SecureSettingsVolumeMountPath,
KeystoreVolumePath: volume.ConfigMountPath,
Resources: corev1.ResourceRequirements{
Expand Down
51 changes: 36 additions & 15 deletions test/e2e/logstash/keystore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,6 @@ import (
)

var (
pipelineConfig = commonv1.Config{
Data: map[string]interface{}{
"pipeline.id": "main",
"config.string": `
input { generator { count => 1 } }
filter {
if ("${HELLO:}" != "") {
mutate { add_tag => ["ok"] }
}
}
`,
},
}

request = logstash.Request{
Name: "pipeline [main]",
Path: "/_node/stats/pipelines/main",
Expand All @@ -46,6 +32,7 @@ filter {
)

// TestLogstashKeystoreWithoutPassword Logstash should resolve ${VAR} in pipelines.yml using keystore key value
// When unexpected variable values occur, the event will be dropped, resulting in a test failure.
func TestLogstashKeystoreWithoutPassword(t *testing.T) {
secretName := "ls-keystore-secure-settings"

Expand All @@ -56,6 +43,26 @@ func TestLogstashKeystoreWithoutPassword(t *testing.T) {
},
StringData: map[string]string{
"HELLO": "HALLO",
"A": "a",
"B": "b",
"C": "c",
},
}

pipelineConfig := commonv1.Config{
Data: map[string]interface{}{
"pipeline.id": "main",
"config.string": `
input { generator { count => 1 } }
filter {
if ("${HELLO:}" != "") {
mutate { add_tag => ["ok"] }
}
if ("${A}" != "a") or ("${B}" != "b") or ("${C}" != "c") {
drop {}
}
}
`,
},
}

Expand All @@ -68,7 +75,7 @@ func TestLogstashKeystoreWithoutPassword(t *testing.T) {
})
})

b := logstash.NewBuilder("test-keystore-with-default-pw").
b := logstash.NewBuilder("test-keystore-without-pw").
WithNodeCount(1).
WithSecureSettings(commonv1.SecretSource{SecretName: secretName}).
WithPipelines([]commonv1.Config{pipelineConfig})
Expand Down Expand Up @@ -115,6 +122,20 @@ func TestLogstashKeystoreWithPassword(t *testing.T) {
},
}

pipelineConfig := commonv1.Config{
Data: map[string]interface{}{
"pipeline.id": "main",
"config.string": `
input { generator { count => 1 } }
filter {
if ("${HELLO:}" != "") {
mutate { add_tag => ["ok"] }
}
}
`,
},
}

before := test.StepsFunc(func(k *test.K8sClient) test.StepList {
return test.StepList{}.WithStep(test.Step{
Name: "Create secret for keystore",
Expand Down