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

[BUG] initializing an async client without http_auth results in an error #283

Closed
liran-cohen-hs opened this issue Feb 8, 2023 · 4 comments · Fixed by #424
Closed

[BUG] initializing an async client without http_auth results in an error #283

liran-cohen-hs opened this issue Feb 8, 2023 · 4 comments · Fixed by #424
Labels
bug Something isn't working

Comments

@liran-cohen-hs
Copy link

liran-cohen-hs commented Feb 8, 2023

What is the bug?

With local/tests setup of the library, without any http auth, the client produces the following error on request:
File ".../.venv/lib/python3.9/site-packages/opensearchpy/connection/http_async.py", line 194, in perform_request **self._http_auth(method, url, query_string, body), TypeError: 'NoneType' object is not callable

How can one reproduce the bug?

Initialize and use AsyncOpenSearch client without http_auth (i.e with use of local ES on docker for example) and make a call to async_bulk

What is the expected behavior?

Ignore http_auth if it is None

What is your host/environment?

MacOS 12.5.1
Python 3.9.5
opensearch-py 2.1.1

Do you have any additional context?

The fix should be relatively easy
on opensearchpy/connection/http_async.py line 194 we should add http_auth call to the request headers only if it is not None
i.e replace lines 192-195 with

if self._http_auth is not None:
    req_headers.update(**self._http_auth(method, url, query_string, body))
@liran-cohen-hs liran-cohen-hs added bug Something isn't working untriaged Need triage labels Feb 8, 2023
@liran-cohen-hs liran-cohen-hs changed the title [BUG] initializing a client without http_auth results in an error [BUG] initializing an async client without http_auth results in an error Feb 12, 2023
@wbeckler wbeckler removed the untriaged Need triage label Feb 15, 2023
@wbeckler
Copy link
Contributor

Feel free to raise a PR if you're up for it.

@ReinGrad
Copy link

ReinGrad commented Apr 3, 2023

The error appears to be related to the AsyncOpenSearch client of the opensearchpy library. When the client initializes without any HTTP authentication and a request is made to async_bulk, the client throws a TypeError error with the message "The 'NoneType' object is unavailable for calling". The error occurs in line 194 http_async.py when the http_auth method is called.

To reproduce the error, you can initialize the AsyncOpenSearch client without any HTTP authentication (for example, using a local instance of Elasticsearch in Docker), and then call async_bulk.

The expected behavior in this case would be for the client to ignore HTTP authentication if it is missing.

Decisions

The suggested bug fix would be to change the code in line 194 http_async.py update request headers only if HTTP authentication is not None. This can be done by adding an if statement to check if self._http_auth is the value None, and updating the request headers with the result of calling http_auth only if this is true. Here is an example of what the modified code might look like:

if self._http_auth is not None:
    req_headers.update(**self._http_auth(method, url, query_string, body))

The host/environment for this error is macOS 12.5.1, Python 3.9.5 and opensearch-py 2.1.1.

@GRomR1
Copy link

GRomR1 commented May 8, 2023

I also have a problem with auth in async mode. Open an issue and PR.

@liran-cohen-hs may be this PR will fix and your problem to

@dannosaur
Copy link
Contributor

if self._http_auth is not None:
req_headers.update(**self._http_auth(method, url, query_string, body))

This won't actually solve all the issues, as there is guard code in the class's __init__ function that tests if the http_auth parameter is a list, tuple, or string type;

if http_auth is not None:
    if isinstance(http_auth, (tuple, list)):
        http_auth = ":".join(http_auth)
    elif isinstance(http_auth, string_types):
        http_auth = tuple(http_auth.split(":", 1))

This makes no sense as it is, and then it later tries to call whatever is in that value. Therefore, in situations where a basic username/password authentication is used, this will still fail.

It's also unclear what exactly this guard code is trying to do if the provided argument isn't a callable, and can't seem to make up its mind as to what format it wants the credentials in.

I'm gonna fork and try to fix this properly. Maybe in the PR someone can shed some light on why it's this way after I've changed it.

dannosaur added a commit to dannosaur/opensearch-py that referenced this issue Jun 29, 2023
…ch-project#283

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
dblock added a commit that referenced this issue Jul 6, 2023
* Fix string/tuple/no auth on AsyncHttpConnection class. Fixes #283

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Update for PR comments. Add tests.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Moving tests to its own file.

Also had to install asynctest into the dev-requirements to get access to the context managers necessary to mock out aiohttp.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Update CHANGELOG

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Linter fixes. Add license text to new file.

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Move AsyncContextManagerMock to utils package for future re-use

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Lint

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Refactor async tests - remove asynctest package

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Switch out to using aiounittest for async testing prior to py3.8

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Use RequestContextManager from opensearchpy._asycn._extra_imports

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Simplify test somewhat, move to `test_async` since all other async tests are ignored on runners <3.6

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

* Lint

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>

---------

Signed-off-by: dannosaur <461956+dannosaur@users.noreply.github.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants