Pass in initial admin password and remove admin:admin references #631
Conversation
Signed-off-by: Derek Ho <[email protected]>
derek-ho
requested review from
VachaShah,
dblock,
harshavamsi,
axeoman,
Shephalimittal,
saimedhi and
florianvazelle
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 71.95% 72.14% +0.19%
==========================================
Files 91 89 -2
Lines 8001 7945 -56
==========================================
- Hits 5757 5732 -25
+ Misses 2244 2213 -31 ☔ View full report in Codecov by Sentry. |
|
Hello @derek-ho, Is this pull request ready for review? |
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
|
Some logic may need to be added after 2.12.0 release in test files like here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37 to replace this with new password conditionally, based on the version the CI is running against. |
|
@saimedhi @VachaShah this PR is ready for review now - I am not sure what approach you folks think is appropriate. I think this PR should be able to be merged as is, but after 2.12.0 I would expect some tests to fail for 2.12.0 release, such as here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37. What do you folks think? |
If it's not urgent, we can wait until the 2.12.0 release to make the code changes. This allows us to align with the latest release. @VachaShah what do you think? |
@VachaShah I believe those tests are run against 2.x OpenSearch without security, so thus we are not seeing those fail yet. |
I am ok with waiting for now, if the regular CI is not broken. I will turn this PR to draft for now. |
Are you able to help enable that? |
|
Now that 2.12 is released this should be unblocked. Would the maintainers review and merge this? |
|
@derek-ho Is this ready for review? |
|
@DarshitChanpura also needs a rebase, please |
|
@derek-ho Would you please rebase this and mark as ready for review? I don't have write access to push to your branch. |
Signed-off-by: Derek Ho <[email protected]>
I am pretty sure the without security tests are not because of this change - I see them failing on other PRs as well. I am working on fixing the 2.12 with security test case, but running into issues with extracting version information within the test, as it seems to only be available outside of the test case. I will try a bit longer, but may need maintainers help for that. |
Yes, other failures are unrelated to this PR. Apologies for any confusion caused. I specifically referred to version (2.12.0, true) as seen here: link. Thank you :) |
|
Hello @derek-ho, Could you please take a look at finishing this PR when you have a moment? Thanks a lot! |
|
Hello @derek-ho, could you please take a look at this PR and let me know if it's ready for review? opensearch-py currently lacks compatibility with the latest OpenSearch versions 2.12.0, 2.13.0, and 2.14.0. It would be great if we could resolve this issue and get the PR merged soon. Are you facing any difficulties or need assistance in this regard? |
| else | ||
| CREDENTIAL="admin:myStrongPassword123!" | ||
| fi | ||
|
|
There was a problem hiding this comment.
This won't work for 2.13, will it?
There was a problem hiding this comment.
It should since admin credentials changes were released in 2.12. If you are referencing script execution in general, it works as expected returning admin:admin for versions <2.12 and admin:myStronPassword123! for those >= 2.12
There was a problem hiding this comment.
Ok, but wouldn't it be simpler to just set OPENSEARCH_USERNAME and OPENSEARCH_PASSWORD in env: in the workflow?
There was a problem hiding this comment.
yes, it should be, however on line 64 we make a status check call that would require the correct credentials to be passed.
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
| else | ||
| CREDENTIAL="admin:myStrongPassword123!" | ||
| fi | ||
|
|
There was a problem hiding this comment.
It should since admin credentials changes were released in 2.12. If you are referencing script execution in general, it works as expected returning admin:admin for versions <2.12 and admin:myStronPassword123! for those >= 2.12
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
|
@derek-ho I'm good with what's here if you can get it to green. Without trying it, I think we can specify the username/password in .github workflow and remove all the logic from the .sh scripts. We can also do this later. |
|
Agree @dblock there is probably a simpler way to do it, but I am not an expert in Python and just focused on getting it to a good state that we can start testing against the newer versions. It can be cleaned up in a follow up, but I may need assistance for that part. |
| @@ -39,9 +59,11 @@ docker run \ | |||
| --env "PYTHON_CONNECTION_CLASS=${PYTHON_CONNECTION_CLASS}" \ | |||
| --env "TEST_TYPE=server" \ | |||
| --env "TEST_PATTERN=${TEST_PATTERN}" \ | |||
| --env "OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123!" \ | |||
There was a problem hiding this comment.
You could avoid the entire if, versions < 2.12 don't care if you set this.
There was a problem hiding this comment.
Yea, we can skip it but I was having trouble reading the env correctly within the testing code itself to determine which credentials to use when setting up the connection. It seems that I needed to set the env. in the sh file right before the tests ran for it to work. Not sure if there is a simpler way.
I'm good with whatever works. We can improve later. |
|
@dblock the CI seems to pass with the latest changes, can we merge this as is? Do you want me to create a follow up issue to clean up the logic/code for CI? |
|
Created follow up issue here: #764 |
…nsearch-project#631) * Update to pass in initial admin password Signed-off-by: Derek Ho <[email protected]> * Add changelog and logic to distinguish between versions Signed-off-by: Derek Ho <[email protected]> * fix syntax Signed-off-by: Derek Ho <[email protected]> * Revert tests Signed-off-by: Derek Ho <[email protected]> * Add 2.12 to the matrix and fix testing logic Signed-off-by: Derek Ho <[email protected]> * Fix version logic Signed-off-by: Derek Ho <[email protected]> * Try to split job into two batches Signed-off-by: Derek Ho <[email protected]> * Fix lint Signed-off-by: Derek Ho <[email protected]> * Change name Signed-off-by: Derek Ho <[email protected]> * Remove period Signed-off-by: Derek Ho <[email protected]> * Pull password dynamically Signed-off-by: Derek Ho <[email protected]> * Change to proper env var Signed-off-by: Derek Ho <[email protected]> * Try passing through Signed-off-by: Derek Ho <[email protected]> --------- Signed-off-by: Derek Ho <[email protected]> Signed-off-by: Derek Ho <[email protected]>
Description
Starting with 2.12.0, security plugin requires an initial admin password to be set via env. variable when installing the demo configuration. This PR is to update documentation and CI to accommodate that change.
Issues Resolved
Closes: #759
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.