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

Remove references to default admin creds #3869

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

derek-ho
Copy link
Contributor

Description

Starting in 2.12.0, security plugin is mandating an initial admin password to be provided, via env variable. This PR updates the documentation and CI to run with that change.

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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: Derek Ho <dxho@amazon.com>
@derek-ho
Copy link
Contributor Author

@derek-ho
Copy link
Contributor Author

@dlvenable @kkondaka I might need some help in understanding how the CI is run in this repo. Basically docker images/release binaries < 2.12.0 would spin up with "admin:admin", and >= 2.12.0 you have to provide a password - I am using myStrongPassword123! as placeholder. I would expect CI to fail on this PR until the images are released - are you ok with just leaving this open until that point after which we merge it to fix the CI? Any other cases I am missing? I do see that this repo is running against older versions to so where would be the best place to put logic to differentiate credentials in case of different versions? I also see admin:admin references in some java test code, not sure what to do about those.

@dlvenable
Copy link
Member

@derek-ho , Our main CI is configured in the .github directory and I don't see any PR changes to that.

Here is where we set the admin credentials.

./gradlew :data-prepper-plugins:opensearch:integrationTest -Dtests.opensearch.host=localhost:9200 -Dtests.opensearch.user=admin -Dtests.opensearch.password=admin -Dtests.opensearch.bundle=true -Dtests.opensearch.version=opensearch:${{ matrix.opensearch }}

You can see all the versions we test against here:

opensearch: [1.0.1, 1.1.0, 1.2.4, 1.3.14, 2.0.1, 2.1.0, 2.3.0, 2.5.0, 2.7.0, 2.9.0, 2.11.1]

And then we'd need to set the password here:

docker run -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" -d opensearchproject/opensearch:${{ matrix.opensearch }}


Might it be better to just set the password to admin so that, for testing, all passwords are the same?

Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

I think the current set of changes are all workable. As noted in the other comment, we may need to make other changes. But, that need not hold this up.

@DarshitChanpura
Copy link
Member

@dlvenable Agreed. The change need to be made once 2.12 is added to the list of versions. But that doesn't block this PR, so can we get reviews/merge it?

@dlvenable dlvenable merged commit a64f409 into opensearch-project:main Jan 17, 2024
68 checks passed
@dlvenable
Copy link
Member

Thanks @derek-ho for this contribution!

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

Successfully merging this pull request may close these issues.

4 participants