Fixing failing tests

[VachaShah]

Signed-off-by: Vacha Shah vachshah@amazon.com

Description

This is the first attempt at fixing tests failing in the CI. The issue is described in detail in #58.

Issues Resolved

Fixes #58

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.

[dblock]

Still failing :(

[VachaShah]

Still failing :(

Yeah :(. The tests with security are failing, this PR currently just fixes the ones without security. I missed to convert this to a draft.

[VachaShah]

There is some issue with the setup for tests with security, all the tests have the following error:

{ "type": "test", "name": "free::update::_80_source_filtering::source_filtering", "event": "failed", "stdout": "thread 'main' panicked at 'expected response to be successful but was 403 Forbidden. Response: {\"error\":{\"root_cause\":[{\"type\":\"security_exception\",\"reason\":\"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]\"}],\"type\":\"security_exception\",\"reason\":\"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]\"},\"status\":403}', yaml_test_runner/tests/common/client.rs:194:5\n" }

I tried a few ways to fix it but haven't found a solution yet. Will continue looking.

[dblock]

Maybe we can ask @davidlago to help out?

[VachaShah]

The permissions issue is happening when calling the delete indices api for tests with secure OpenSearch Cluster. The tests run successfully for the OpenSearch cluster without security.

I found opensearch-project/security#15 which states that the admin user might need a certificate.

I looked through other clients and they all have the same kind of setup, call the delete indices api for cleanup but don't have this issue.

@davidlago Any suggestions on how to fix this?

[cwperks]

Hi @VachaShah,

Deleting all indices fails because it includes the security plugin configuration indices and the security plugin prohibits that action even for admin users (The error message is not great for this). By using an admin certificate, you become a "super admin" and have the ability to delete/restore all indices including the security plugin indices.

A few options to consider:

1. One straightforward solution is to add the following lines in .ci/opensearch/Dockerfile:

(Run this if SECURE_INTEGRATION is true)

RUN chmod +x $opensearch_path/plugins/opensearch-security/tools/install_demo_configuration.sh
RUN yes | $opensearch_path/plugins/opensearch-security/tools/install_demo_configuration.sh
RUN echo "plugins.security.filter_securityindex_from_all_requests: true" >> $opensearch_yml

The first 2 commands are necessary to perform the demo configuration because the install_demo_configuration.sh script is skipped if there are any plugins.security.* in the opensearch.yml file. This install_demo_configuration.sh file is normally executed as part of the entrypoint to the Opensearch docker container.

2. The next option is to use an admin certificate to connect to the cluster as you suggested.

3. The other option to consider to to start/stop the cluster in between tests to get a pristine cluster at the beginning of every test.

The tests are currently running locally and get past the reported error with admin not having permissions. I will push a branch and try with CI as well.

[VachaShah]

Hi @VachaShah,

Deleting all indices fails because it includes the security plugin configuration indices and the security plugin prohibits that action even for admin users (The error message is not great for this). By using an admin certificate, you become a "super admin" and have the ability to delete/restore all indices including the security plugin indices.

A few options to consider:

1. One straightforward solution is to add the following lines in .ci/opensearch/Dockerfile:

(Run this if SECURE_INTEGRATION is true)

RUN chmod +x $opensearch_path/plugins/opensearch-security/tools/install_demo_configuration.sh
RUN yes | $opensearch_path/plugins/opensearch-security/tools/install_demo_configuration.sh
RUN echo "plugins.security.filter_securityindex_from_all_requests: true" >> $opensearch_yml

The first 2 commands are necessary to perform the demo configuration because the install_demo_configuration.sh script is skipped if there are any plugins.security.* in the opensearch.yml file. This install_demo_configuration.sh file is normally executed as part of the entrypoint to the Opensearch docker container.

2. The next option is to use an admin certificate to connect to the cluster as you suggested.
3. The other option to consider to to start/stop the cluster in between tests to get a pristine cluster at the beginning of every test.

The tests are currently running locally and get past the reported error with admin not having permissions. I will push a branch and try with CI as well.

Awesome! Thank you @cwperks, the first option works perfectly to fix this issue. There are quite a few failing tests after that but it goes past the security permissions error which is great!

[saratvemulapalli]

Thanks @cwperks for the explanation. We did guess using certificates is going to provide super admin permissions and solve the problem.
The error reason "reason\":\"no permissions for [] is not self explanatory, could we do verbose logging ?(I can open up an issue in security if we are on the same page).
@VachaShah why was this not a problem for rest of the clients? Aren't they deleting the internal security plugin indices?

[VachaShah]

Thanks @cwperks for the explanation. We did guess using certificates is going to provide super admin permissions and solve the problem. The error reason "reason\":\"no permissions for [] is not self explanatory, could we do verbose logging ?(I can open up an issue in security if we are on the same page). @VachaShah why was this not a problem for rest of the clients? Aren't they deleting the internal security plugin indices?

I still don't know why the other clients don't face this issue that have the same setup (for example: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L83).

Another part here is that the tests are still failing after the first option (@cwperks suggested to try the admin certificate to resolve that).

On a separate note for tests, the test cases used in the Rust client are very extensive and heavy since they use the test cases from yaml tests from OpenSearch. I feel we could also decouple this and write required test cases for the client since all tests from OpenSearch might not be relevant. WDYT @saratvemulapalli @dblock?

[cwperks]

Hey @VachaShah I could be wrong, but this issue is probably also affecting the python client. There is only 1 test run when security is set to True for the python client and it doesn't extend OpenSearchTestCase that you linked to. See the screenshots below.

When security is set to false:

When security is set to true:

[VachaShah]

Hey @VachaShah I could be wrong, but this issue is probably also affecting the python client. There is only 1 test run when security is set to True for the python client and it doesn't extend OpenSearchTestCase that you linked to. See the screenshots below.

When security is set to false:

When security is set to true:

Ah!! So thats why the python client is unaffected since it is not trying to delete the security indices for the tests. Thank you so much for this finding @cwperks!

[VachaShah]

After adding the admin certificate to the tests for security (latest changes in this PR), some tests from the yaml test suite still fail with assertion errors like

{ "type": "test", "name": "free::search_aggregation::_220_filters_bucket::anonymous_filters_test", "event": "failed", "stdout": "thread 'main' panicked at 'assertion failed: `(left == right)`\n  left: `Number(5)`,\n right: `4`: expected value json[\"hits\"][\"total\"] to match 4 but was Number(5)', yaml_test_runner/tests/free/search_aggregation/_220_filters_bucket.rs:203:5\n" }

[dblock]

@VachaShah is this a small amount that gets different search results? Maybe we can mark them ignore and open an issue?

[VachaShah]

@VachaShah is this a small amount that gets different search results? Maybe we can mark them ignore and open an issue?

Yeah its a small percentage of the whole test suite. I will create an issue for the same so that we can unblock the PRs to this repo.

[VachaShah]

@dblock I have updated the PR with the changes and the CI is green now.

[VachaShah]

Also, many thanks to @cwperks for helping with the security exception issue, the code in this PR for adding admin certificate is entirely due to his effort 🙇🏼‍♀️

[dblock]

@VachaShah What is the fall out from this change (tests removed?), open an issue for followup items?

[VachaShah]

@VachaShah What is the fall out from this change (tests removed?), open an issue for followup items?

Created issue #66 for the fall out.

[jayaddison]

Hey @VachaShah I could be wrong, but this issue is probably also affecting the python client. There is only 1 test run when security is set to True for the python client and it doesn't extend OpenSearchTestCase that you linked to. See the screenshots below.

Hi @cwperks @VachaShah @dblock - I've landed here after scouring some of the bugtracker to check on the status of opensearch-project/opensearch-clients#58 (sidenote / bonus: a few items there are now complete, and can be checked-off).

As a consumer of the Python client library I think I'd be marginally more comfortable with the test coverage if the same tests ran in both HTTP and HTTPS modes. Given that the test suite runs twice, once for each, the coverage and behaviour should be fine, but I'm wondering if I could offer (and whether you would accept) any contribution time to get rid of that branch condition, likely by using a similar client-certificate approach to here.

[jayaddison]

Oops, I forgot to mention (partly for my own potential future reference): I think that the key place where the tests branch in the Python client test suite is this section of code.