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

Pass in strong password to OSD integ test if version >= 2.12.0 #4334

Merged
merged 25 commits into from
Jan 10, 2024

Conversation

derek-ho
Copy link
Contributor

Description

OSD health check for integ test is using admin:admin, this changes it to admin:myStrongPassword123! if version >= 2.12.0

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -38,6 +38,7 @@ def __init__(
super().__init__(work_dir, version, distribution, security_enabled, additional_config, dependency_installer)
self.dist = Distributions.get_distribution("opensearch-dashboards", distribution, version, work_dir)
self.install_dir = self.dist.install_dir
self.password = 'myStrongPassword123!' if float('.'.join(self.version.split('.')[:2])) >= 2.12 else 'admin'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR should address this failure: https://build.ci.opensearch.org/blue/organizations/jenkins/integ-test-opensearch-dashboards/detail/integ-test-opensearch-dashboards/5025/pipeline, however I am not sure how the OS binary is pulled - is it given myStrongPassword123! as the password somehow?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @derek-ho , can you try semver package instead of float check? See #4304

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaiksaya done. Question still stands though, I am confused about the overall setup of: https://build.ci.opensearch.org/blue/organizations/jenkins/integ-test-opensearch-dashboards/detail/integ-test-opensearch-dashboards/5025/pipeline/, would this change be sufficient to pass the integ test? Would the backend be spun up with the strong password as expected if version >= 2.12.0?

Copy link
Member

@gaiksaya gaiksaya Jan 10, 2024

Choose a reason for hiding this comment

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

The integration test set up can be found here: https://github.com/opensearch-project/opensearch-build/blob/main/src/test_workflow/integ_test/ for both OS and OSD.
Maybe try to parse this directory to check for anything admin:admin?
If it feels there is a lot of if/else going on each module, maybe extract the logic into a common library (utils?) (something like get_password(version)) that can be used universally. Example being this system module.

WDYT @prudhvigodithi ?

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6eb6f8b) 91.34% compared to head (6bd68e9) 91.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4334   +/-   ##
=======================================
  Coverage   91.34%   91.35%           
=======================================
  Files         189      190    +1     
  Lines        6169     6175    +6     
=======================================
+ Hits         5635     5641    +6     
  Misses        534      534           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -33,7 +33,7 @@ def install(self, bundle_name: str) -> None:
@property
def start_cmd(self) -> str:
start_cmd_map = {
"opensearch": "export OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123! && ./opensearch-tar-install.sh",
"opensearch": "env OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123! ./opensearch-tar-install.sh",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to env from suggestion from @peterzhuamazon. Can somebody with insight explain why this might work better? From the jenkins failures it looks like either the cluster is not being spun up successfully with strong password, or the health check is not using the correct credential, even though I have made changes that I think should solve both of these. It seems like this command is run with python subprocess, might that have something to do with it?

Signed-off-by: Derek Ho <dxho@amazon.com>
@prudhvigodithi
Copy link
Collaborator

prudhvigodithi commented Jan 10, 2024

@derek-ho I see the error from OSD as 401: {"statusCode":401,"error":"Unauthorized","message":"Authentication Exception"}, please confirm if this the exact error? https://build.ci.opensearch.org/blue/organizations/jenkins/integ-test-opensearch-dashboards/detail/integ-test-opensearch-dashboards/5025/pipeline

@derek-ho
Copy link
Contributor Author

derek-ho commented Jan 10, 2024

@derek-ho I see the error from OSD as 401: {"statusCode":401,"error":"Unauthorized","message":"Authentication Exception"}, please confirm if this the exact error? https://build.ci.opensearch.org/blue/organizations/jenkins/integ-test-opensearch-dashboards/detail/integ-test-opensearch-dashboards/5025/pipeline

@prudhvigodithi Yes I believe so. I see logs from that run: https://ci.opensearch.org/ci/dbc/integ-test-opensearch-dashboards/3.0.0/7076/linux/arm64/tar/test-results/5025/integ-test/reportsDashboards/with-security/local-cluster-logs/id-1/stdout.txt, indicating that the admin password was successfully set to a strong password. Then authentication fails for user. Thus I expect that the only change needed to is to change the password the user uses in integ test.

…rd succcesfully set

Signed-off-by: Derek Ho <dxho@amazon.com>
@gaiksaya
Copy link
Member

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

@prudhvigodithi
Copy link
Collaborator

LGTM! Needs changes in https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/integtest.sh

Looks like this is already updated right @gaiksaya ?

@prudhvigodithi prudhvigodithi merged commit eb1d8fe into opensearch-project:main Jan 10, 2024
13 checks passed
@prudhvigodithi
Copy link
Collaborator

Also @derek-ho dont want to block this, but please open a new PR for this change mentioned by Sayali.

Also @derek-ho Do we need to parse all the files again to call this method or have you taken care of it already? https://github.com/search?q=repo%3Aopensearch-project%2Fopensearch-build%20myStrongPassword123&type=code

To do this move the utils.py to to a common file under https://github.com/opensearch-project/opensearch-build/tree/main/src and use it for other workflows as well.

Thank you

@gaiksaya
Copy link
Member

@derek-ho
Copy link
Contributor Author

@prudhvigodithi
Copy link
Collaborator

True not for 2.x, but for 3.0.0 since its already on main, I just triggered a new build https://build.ci.opensearch.org/job/integ-test-opensearch-dashboards/5034/console and dont see 401 errors anymore.

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

Successfully merging this pull request may close these issues.

4 participants