Respect trust_env on AsyncHttpConnection

[rsmeral]

Description

Respect trust_env on AsyncHttpConnection same as we do in AIOHttpConnection.
This lets us use the standard proxy environment variables (e.g. HTTPS_PROXY).

Issues Resolved

It was not possible to connect to OpenSearch asynchronously while using AWSV4SignerAsyncAuth (which is not supported in AIOHttpConnection) from a network which requires the use of a proxy.

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.39%. Comparing base (ba715b9) to head (0fd0db8).
Report is 82 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #886      +/-   ##
==========================================
- Coverage   71.95%   70.39%   -1.56%     
==========================================
  Files          91      125      +34     
  Lines        8001     9290    +1289     
==========================================
+ Hits         5757     6540     +783     
- Misses       2244     2750     +506     

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

[dblock]

Thanks for this.

Would be nice to find a way to share/standardize setup like this and tests to make sure all the connection implementations are aligned.

[dblock]

Looks like some possibly unrelated tests are failing, appreciate if you could help take a look?

[rsmeral]

Thanks for this.

Would be nice to find a way to share/standardize setup like this and tests to make sure all the connection implementations are aligned.

Agreed. I'm really new to using OpenSearch and especially this library, so I guess I'm missing some context here – why are there two classes with seemingly the same purpose – AsyncHttpConnection and AIOHttpConnection?

• They are both based on aiohttp
• They have the same general purpose (asynchronous connection) and a confusingly similar name
• AsyncHttpConnection inherits from AIOHttpConnection
  • Despite that, AsyncHttpConnection is a 90% copy of AIOHttpConnection
• As a result, they do nearly the same thing, just with sneaky, subtle differences in behaviour (e.g. trust_env, AWSv4 signing, ...)

It really feels like it should be a single class.

Looks like some possibly unrelated tests are failing, appreciate if you could help take a look?

I see this test failure:

FAILED test_opensearchpy/test_server/test_rest_api_spec.py::test_rest_api_spec[OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/search_shards/20_slice[0]] - TypeError: OpenSearch.search_shards() got an unexpected keyword argument 'body'

Definitely seems unrelated, has been failing some time before my PR: https://github.com/opensearch-project/opensearch-py/actions/workflows/integration-unreleased.yml

As far as I can tell:

• Related to https://github.com/opensearch-project/OpenSearch/pull/17031/files
• It looks like it's a feature (body parameter for search_shards) that was added recently (planned for OpenSearch 2.19), but this opensearch-py client just hasn't caught up with that yet, so the test is failing
• The test for search_shards body should be skipped for versions of OpenSearch <= 2.18.99 (https://github.com/opensearch-project/OpenSearch/pull/17031/files#diff-e9e4811bdc398ddb287301731f9685f2bafa0a476276d497a70dfb66d4e71d2eR4) but for some reason, it's not getting skipped – perhaps there's something wrong with the skip version matching (https://github.com/opensearch-project/opensearch-py/blob/main/test_opensearchpy/test_server/test_rest_api_spec.py#L272-L280 and https://github.com/opensearch-project/opensearch-py/blob/main/test_opensearchpy/test_server/test_rest_api_spec.py#L143-L151) but I don't know what exactly
• What's interesting is:
  • There's two versions of this test suite – sync and async
  • The async doesn't seem to be failing, even though they're set up 99% the same
  • One difference (which could mess with the skip-version matching):
    • https://github.com/opensearch-project/opensearch-py/blob/main/test_opensearchpy/test_server/test_rest_api_spec.py#L150
    • https://github.com/opensearch-project/opensearch-py/blob/main/test_opensearchpy/test_async/test_server/test_rest_api_spec.py#L97

All of that considered, I think the above shouldn't be fixed in this PR. WDYT?

[dblock]

Agreed. I'm really new to using OpenSearch and especially this library, so I guess I'm missing some context here – why are there two classes with seemingly the same purpose – AsyncHttpConnection and AIOHttpConnection?

Honestly, no idea. We should assume that anything can change moving forward. I am guessing that Sigv4 was introduced in a hacky way by bringing in the second implementation from somewhere else, instead of doing the work of adding it to the existing one and nobody objected (they should have). Would absolutely take a merged single version!

All of that considered, I think the above shouldn't be fixed in this PR. WDYT?

Yes, but we need a green CI before we merge more code. Appreciate it if you have time to look at this, I won't for a bit.