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 reference to default credentials #217

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

derek-ho
Copy link
Contributor

@derek-ho derek-ho commented Dec 18, 2023

Description

Removes references to default admin credentials, which were removed in 2.12

Issues Resolved

Closes #222

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>
@dblock
Copy link
Member

dblock commented Dec 20, 2023

Let's add a CHANGELOG line?

@derek-ho
Copy link
Contributor Author

@dblock wondering what your thoughts are here - until OpenSearch's latest image is 2.12.0, the CI will be working fine, but as soon as it is released it will fail. I was looking into adding logic into the bash script running OpenSearch is < 2.12.0 it will spin up with https://admin:admin@os1:$PORT, else spin up with https://admin:myStrongPassword123!@os1:$PORT, but that logic will break when using the latest tag. Do you think it's appropriate to simply leave it as is, until 2.12 is released?

Signed-off-by: Derek Ho <dxho@amazon.com>
@derek-ho
Copy link
Contributor Author

Update: added a changelog and changed the PR according to my thought process - we should keep this PR open but not merged until 2.12.0 release, at which point it should be merged. I left a comment explaining the extra logic that I added. It is expected for the CI checks involving security to fail until the latest docker image is 2.12.0. @dblock let me know if this approach makes sense.

@dblock
Copy link
Member

dblock commented Dec 29, 2023

@derek-ho Agreed 100%, keep this around until we actually need it in 2.12. Turning this into a draft.

@dblock dblock marked this pull request as draft December 29, 2023 15:29
@DarshitChanpura
Copy link
Member

Now that 2.12 is released this should be unblocked. Would the maintainers please review this?

@dblock
Copy link
Member

dblock commented Feb 24, 2024

@DarshitChanpura rebase to re-kick CI, and mark it ready for review?

@nhtruong nhtruong marked this pull request as ready for review February 25, 2024 22:27
@nhtruong nhtruong marked this pull request as draft February 25, 2024 22:39
@DarshitChanpura
Copy link
Member

@derek-ho Would you mark this PR as ready of review after rebasing with main?

@derek-ho derek-ho marked this pull request as ready for review February 27, 2024 14:58
Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -80,7 +80,7 @@ jobs:

test-opensearch-security:
env:
TEST_OPENSEARCH_SERVER: https://admin:admin@localhost:9200
TEST_OPENSEARCH_SERVER: https://admin:myStrongPassword123!@localhost:9200
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok to hard code this here since from now on, "latest" will be > 2.12

Signed-off-by: Derek Ho <dxho@amazon.com>
@derek-ho
Copy link
Contributor Author

PR is updated, but I can't install rubocop on my local and not sure if the lint checker failure is even related to this change. @nhtruong can you help me out here?

@dblock
Copy link
Member

dblock commented Feb 27, 2024

It may be related, something changed in a docker image? The error is strange. (the problem is not rubocop)

exec /entrypoint.sh: no such file or directory

Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
@derek-ho
Copy link
Contributor Author

It may be related, something changed in a docker image? The error is strange. (the problem is not rubocop)

exec /entrypoint.sh: no such file or directory

Very weird. Although the artifact should be the same for the given version, I just upgraded it to the latest release and that seemed to solve the problem. I think this PR is ready for review. There is one failing check, but it seems flaky. I ran the logic through a bash editor and it should work for the version, and it passes for everything else. Can the CI be re-run?

Comment on lines +59 to +71
# Starting in 2.12.0, security demo configuration script requires an initial admin password which is set to
# myStrongPassword123!
OPENSEARCH_REQUIRED_VERSION="2.12.0"
if [ "$CLUSTER_VERSION" == "latest" ]; then
CREDENTIAL="admin:myStrongPassword123!"
else
COMPARE_VERSION=`echo $OPENSEARCH_REQUIRED_VERSION $CLUSTER_VERSION | tr ' ' '\n' | sort -V | uniq | head -n 1`
if [ "$COMPARE_VERSION" != "$OPENSEARCH_REQUIRED_VERSION" ]; then
CREDENTIAL="admin:admin"
else
CREDENTIAL="admin:myStrongPassword123!"
fi
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ported this same logic from the build repo

@dblock
Copy link
Member

dblock commented Feb 27, 2024

@derek-ho
Copy link
Contributor Author

The 1.3.2 failure doesn't look like a flake, https://github.com/opensearch-project/opensearch-ruby/actions/runs/8067723096/job/22038855221?pr=217.

Reason why I think it is flaky is because the action is working on the action run on my fork: https://github.com/derek-ho/opensearch-ruby/actions/runs/8067722368/job/22038843163, but is failing on different versions there. From the logs it seems like it is stopping at the echoing of the version, and not when it is trying to access opensearch with the wrong credentials or something. Has this repo had flakiness there in the past? I also don't want to introduce flakiness in this PR.

@dblock dblock merged commit b1246f8 into opensearch-project:main Feb 27, 2024
58 checks passed
@dblock
Copy link
Member

dblock commented Feb 27, 2024

I retried and it worked. Thanks. Merged.

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.

[v2.12.0] Ensure CI/documentation reflect changes to default admin credentials
3 participants