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 initial admin password and remove admin:admin references #631

Merged
merged 16 commits into from
Jun 18, 2024

Conversation

derek-ho
Copy link
Contributor

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

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.

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.14%. Comparing base (ba715b9) to head (9d2c999).
Report is 26 commits behind head on main.

Current head 9d2c999 differs from pull request most recent head e5cd848

Please upload reports for the commit e5cd848 to get more accurate results.

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.
📢 Have feedback on the report? Share it here.

@derek-ho derek-ho marked this pull request as draft December 14, 2023 15:33
@saimedhi
Copy link
Collaborator

saimedhi commented Dec 14, 2023

Hello @derek-ho,

Is this pull request ready for review?

@derek-ho
Copy link
Contributor Author

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.

@derek-ho derek-ho marked this pull request as ready for review December 28, 2023 21:04
@derek-ho
Copy link
Contributor Author

@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?

@saimedhi
Copy link
Collaborator

saimedhi commented Jan 9, 2024

@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
Copy link
Collaborator

I am wondering why we can't make all the changes now since we already test this client against the developing 2.x line. @derek-ho I believe you should be able to add the version checks in as well and test the code out. LMK if I am missing something here.

@derek-ho
Copy link
Contributor Author

I am wondering why we can't make all the changes now since we already test this client against the developing 2.x line. @derek-ho I believe you should be able to add the version checks in as well and test the code out. LMK if I am missing something here.

@VachaShah I believe those tests are run against 2.x OpenSearch without security, so thus we are not seeing those fail yet.

@derek-ho
Copy link
Contributor Author

@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?

I am ok with waiting for now, if the regular CI is not broken. I will turn this PR to draft for now.

@derek-ho derek-ho marked this pull request as draft January 18, 2024 15:08
@dblock
Copy link
Member

dblock commented Jan 18, 2024

@VachaShah I believe those tests are run against 2.x OpenSearch without security, so thus we are not seeing those fail yet.

Are you able to help enable that?

@DarshitChanpura
Copy link
Member

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

@VachaShah
Copy link
Collaborator

@derek-ho Is this ready for review?

@dblock
Copy link
Member

dblock commented Feb 24, 2024

@DarshitChanpura also needs a rebase, please

@DarshitChanpura
Copy link
Member

@derek-ho Would you please rebase this and mark as ready for review? I don't have write access to push to your branch.

@derek-ho derek-ho marked this pull request as ready for review February 27, 2024 22:08
@derek-ho
Copy link
Contributor Author

@derek-ho Please fix these failing tests https://github.com/opensearch-project/opensearch-py/actions/runs/8072055726/job/22087332746

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.

@saimedhi
Copy link
Collaborator

@derek-ho Please fix these failing tests https://github.com/opensearch-project/opensearch-py/actions/runs/8072055726/job/22087332746

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 :)

@saimedhi
Copy link
Collaborator

Hello @derek-ho,

Could you please take a look at finishing this PR when you have a moment?

Thanks a lot!

@saimedhi
Copy link
Collaborator

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

Copy link
Member

Choose a reason for hiding this comment

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

This won't work for 2.13, will it?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but wouldn't it be simpler to just set OPENSEARCH_USERNAME and OPENSEARCH_PASSWORD in env: in the workflow?

Copy link
Member

Choose a reason for hiding this comment

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

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]>
else
CREDENTIAL="admin:myStrongPassword123!"
fi

Copy link
Member

Choose a reason for hiding this comment

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

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

dblock
dblock previously approved these changes Jun 18, 2024
Signed-off-by: Derek Ho <[email protected]>
@dblock
Copy link
Member

dblock commented Jun 18, 2024

@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.

@derek-ho
Copy link
Contributor Author

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!" \
Copy link
Member

Choose a reason for hiding this comment

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

You could avoid the entire if, versions < 2.12 don't care if you set this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@dblock
Copy link
Member

dblock commented Jun 18, 2024

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.

I'm good with whatever works. We can improve later.

@derek-ho
Copy link
Contributor Author

@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?

@derek-ho
Copy link
Contributor Author

Created follow up issue here: #764

@dblock dblock merged commit d3177a8 into opensearch-project:main Jun 18, 2024
61 checks passed
dblock pushed a commit to dblock/opensearch-py that referenced this pull request Aug 15, 2024
…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]>
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.

[BUG] Support Latest OpenSearch versions 2.12, 2.13, 2.14
5 participants