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

Do not manage keystore if already initialized #3295

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Jun 18, 2020

Fix #3294 by creating a file to record if the keystore has been initialized.

@barkbay barkbay added >bug Something isn't working v1.2.0 labels Jun 18, 2020
@anyasabo anyasabo changed the title Do no manage keystore if already initialized Do not manage keystore if already initialized Jun 18, 2020
@@ -101,6 +101,13 @@ func TestResources(t *testing.T) {

set -eux

keystore_initialized_flag=/bar/data/elastic-internal-init-keystore.ok
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessarily your change but it seems odd we're not reusing the script constant here

@@ -61,7 +61,7 @@ func initContainerParameters(typ string) keystore.InitContainerParameters {
KeystoreCreateCommand: fmt.Sprintf("%s keystore create --force", typ),
KeystoreAddCommand: fmt.Sprintf(`cat "$filename" | %s keystore add "$key" --stdin --force`, typ),
SecureSettingsVolumeMountPath: keystore.SecureSettingsVolumeMountPath,
DataVolumePath: fmt.Sprintf(DataPathTemplate, typ),
KeystoreVolumePath: fmt.Sprintf(DataPathTemplate, typ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not your change, but typ is a typo of type, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

type is a reserved word though, that's probably why it's typ.

Copy link
Contributor

Choose a reason for hiding this comment

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

typ is acceptable even I'm agree it's disturbing at the first glance. Alternative ideas: kind, beatType, specType, beatSpecType.

Copy link
Contributor

@anyasabo anyasabo left a comment

Choose a reason for hiding this comment

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

LGTM, though I think we may also want to raise an issue with ES so that this call can be idempotent. We would still need this fix for old versions but would be nice for future ones.

@barkbay
Copy link
Contributor Author

barkbay commented Jun 23, 2020

LGTM, though I think we may also want to raise an issue with ES so that this call can be idempotent. We would still need this fix for old versions but would be nice for future ones.

We would also need to raise some issues for all the other stack components to ensure that there is a consistent behaviour across the stack. I agree that it would be nice, but I'm not sure we want to coordinate this effort.

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Init containers should tolerate restarts
4 participants