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

Avoid dialog in terminal during s3 key creation #936

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

markbaumgarten
Copy link
Contributor

Description

Fixes error when creating cluster with snapshot and s3 keys.

Heres the "issue":

What is the bug?

When using snapshot feature to s3 via the operator we have to add s3 keys.

When specifying these (at least in my k8s cluster) i have issues with these secrets already exsisting. The current command is basically asking the user to confirm the creation of keystore file and also for each already existing key in this keystore. We never get our s3 accessKey/secretKey written.

How can one reproduce the bug?

Create an opensearch cluster using the operator. Enable snapshot and s3 key mapping like shown in the attached cluster.yaml (feel free to use this in the docs?)

What is the expected behavior?

s3 keys would work so snapshots can be created

What is your host/environment?

kubernetes

Do you have any screenshots?

No

Do you have any additional context?

See attached cluster.txt (yaml)
cluster.txt

Issues Resolved

Unable to create an issue (Green button not working in my browser)...

Check List

  • [no] Commits are signed per the DCO using --signoff
  • [no] Unittest added for the new/changed functionality and all unit tests are successful
  • [no] Customer-visible features documented
  • [no] No linter warnings (make lint)

Please refer to the PR guidelines before submitting this pull request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mark Baumgarten <mark.baumgarten@gmail.com>
@swoehrl-mw
Copy link
Collaborator

Hi @markbaumgarten. Please provide more details what the problem is and what is happening, because from what I can tell the keystore can only be created in the init container so should not exist beforehand.

Also, please give the PR a meaningful name.

@markbaumgarten
Copy link
Contributor Author

The error I saw was happening in an attempt to add the S3 snapshot config to an already existing cluster. The error message was from the creation of S3 keys, where the commands wants user input from the terminal asking questions like: "Keystore file exists. Are you sure?" and "Key exists....are you sure".

There seems to be issues of the same kind reported in the ES operator where the --force flag was resolving this....back in 2019...I think.

I can try to reproduce on Monday so you can see the specific error message. Alternatively exec into an open search container to create a new keyfile or overwrite an existing key.

I see your point that the init container should not have an existing keyfile and keys, but I have mapped rook-ceph volumes as persistence layer.

I don't see the harm in the three changes I have added. If your statement that there is no keyfiles present at init time holds true, then the code changes don't have any negative impact IMHO.

@markbaumgarten
Copy link
Contributor Author

Found the error message from my browser history here on my phone:

operator Exception in thread "main" java.lang.IllegalStateException: unable to read from standard input; is standard input open and a tty attached? at org.opensearch.cli.Terminal$SystemTerminal.readText(Terminal.java:286) at org.opensearch.cli.Terminal.promptYesNo(Terminal.java:165) at org.opensearch.common.settings.CreateKeyStoreCommand.execute(CreateKeyStoreCommand.java:64) at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104) at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138) at org.opensearch.cli.MultiCommand.execute(MultiCommand.java:104) at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138) at org.opensearch.cli.Command.main(Command.java:101) at org.opensearch.common.settings.KeyStoreCli.main(KeyStoreCli.java:56)

@markbaumgarten markbaumgarten changed the title Main Avoid dialog in terminal during s3 key creation Jan 10, 2025
@markbaumgarten
Copy link
Contributor Author

There - fixed the title - was fooling around in codespace on my phone for the first time.

@markbaumgarten
Copy link
Contributor Author

Just to be clear: The error occurs when adding s3 config to an already existing cluster not having any s3 config.

@swoehrl-mw
Copy link
Collaborator

@markbaumgarten I agree that adding this should not be harmful, but I'd like to make sure you are not doing something strange that interferes with the operator in other ways. Could you post the cluster spec you are using (with s3 options)?

@markbaumgarten
Copy link
Contributor Author

@markbaumgarten I agree that adding this should not be harmful, but I'd like to make sure you are not doing something strange that interferes with the operator in other ways. Could you post the cluster spec you are using (with s3 options)?

Sure,

I added the cluster.txt in the first message.

@swoehrl-mw
Copy link
Collaborator

@markbaumgarten I had a look at your manifest and don't see any problems. Maybe there is some weird behaviour with emptydirs not properly cleaned/reset, not sure. Your change doesn't introduce problems and might fix them for some users, so I'm fine with it.

@swoehrl-mw
Copy link
Collaborator

@markbaumgarten Code LGTM, can you please fix the DCO/signoff for your last commit, then I can approve and merge.

Signed-off-by: Mark Baumgarten <mark.baumgarten@gmail.com>
@markbaumgarten
Copy link
Contributor Author

@markbaumgarten Code LGTM, can you please fix the DCO/signoff for your last commit, then I can approve and merge.

Sorry - still getting used to using signoff... hoping my rebase was done correctly?

@swoehrl-mw
Copy link
Collaborator

Sorry - still getting used to using signoff... hoping my rebase was done correctly?

You somehow added a commit from main from someone else. I took the liberty of force-pushing to your PR to remove that commit. But signoff looks good now.

@swoehrl-mw swoehrl-mw merged commit d8c71cc into opensearch-project:main Jan 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants