fix(test): Enhancing test_http_timeout to include CCT-506

[zpetrace]

Enhancing test_http_timeout to include CCT-506 - insights-client --test-connection not catching ReadTimeoutError Exception

So far this PR is a draft as the CCT card is still in refinement

[Archana-PandeyM]

LGTM

[Glutexo]

I ran the test on my RHEL 9.4 VM and it didn’t fail.

$ sudo venv/bin/pytest integration-tests/test_connection.py::test_http_timeout
================================================ test session starts ================================================
platform linux -- Python 3.9.18, pytest-8.3.2, pluggy-1.5.0
rootdir: /home/insights/insights-client
configfile: pyproject.toml
plugins: subtests-0.13.1, client-tools-0.0.1
collected 1 item                                                                                                    

integration-tests/test_connection.py .                                                                        [100%]

=========================================== 1 passed in 71.86s (0:01:11) ============================================

Is it possible that I am doing something wrong?

@Archana-PandeyM Did the test fail correctly during your review?

[Glutexo]

I think I understood the PR wrong. The test it adds is actually verifying that the ReadTimeoutError is not caught and its traceback is printed. In such case, the test is correct and works as intended. Can you please confirm that, @zpetrace and @Archana-PandeyM?

In such case, I think we can undraft, rebase, review and merge this pull request. I am working on a fix, so we can consider CCT-506 accepted. What does @m-horky think?

[Glutexo]

I need to update my thoughs once again. I understood the original intention well: this integration test is supposed to fail.

assert "Traceback" not in output.stdout

ensures that it’s not the case when the exception is properly caught and concisely printed.

⚠️ However, even without the fix in place, the test still yields false positive in case a connection is not established in time and the request fails on ConnectionTimeoutError rather than ReadTimeoutError.

# INSIGHTS_HTTP_TIMEOUT=0.01 insights-client --test-connection
Running Connection Tests...
=== Begin Upload URL Connection Test ===
Testing https://cert-api.access.redhat.com/r/insights/platform/ingress/v1/upload
POST https://cert-api.access.redhat.com/r/insights/platform/ingress/v1/upload
Could not successfully connect to: https://cert-api.access.redhat.com/r/insights/platform/ingress/v1/upload
HTTPSConnectionPool(host='cert-api.access.redhat.com', port=443): Max retries exceeded with url: /r/insights/platform/ingress/v1/upload (Caused by ConnectTimeoutError(<urllib3.connection.HTTPSConnection object at 0x7f03b2c91be0>, 'Connection to cert-api.access.redhat.com timed out. (connect timeout=0.01)'))
HTTPSConnectionPool(host='cert-api.access.redhat.com', port=443): Max retries exceeded with url: /r/insights/platform/ingress/v1/upload (Caused by ConnectTimeoutError(<urllib3.connection.HTTPSConnection object at 0x7f03b2c91be0>, 'Connection to cert-api.access.redhat.com timed out. (connect timeout=0.01)'))
Connectivity test failed! Please check your network configuration
Additional information may be in /var/log/insights-client/insights-client.log

You can see there is no traceback, but the error is ConnectTimeoutError, which has been caught and concisely printed already.

The actual wrong output this tests verifies that does not get printed is:

# INSIGHTS_HTTP_TIMEOUT=0.1 insights-client --test-connection
Running Connection Tests...
=== Begin Upload URL Connection Test ===
Testing https://cert-api.access.redhat.com/r/insights/platform/ingress/v1/upload
POST https://cert-api.access.redhat.com/r/insights/platform/ingress/v1/upload
Fatal error
Traceback (most recent call last):
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 446, in _make_request
    six.raise_from(e, None)
  File "<string>", line 3, in raise_from
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 441, in _make_request
    httplib_response = conn.getresponse()
  File "/usr/lib64/python3.9/http/client.py", line 1377, in getresponse
    response.begin()
  File "/usr/lib64/python3.9/http/client.py", line 320, in begin
    version, status, reason = self._read_status()
  File "/usr/lib64/python3.9/http/client.py", line 281, in _read_status
    line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
  File "/usr/lib64/python3.9/socket.py", line 704, in readinto
    return self._sock.recv_into(b)
  File "/usr/lib64/python3.9/ssl.py", line 1275, in recv_into
    return self.read(nbytes, buffer)
  File "/usr/lib64/python3.9/ssl.py", line 1133, in read
    return self._sslobj.read(len, buffer)
socket.timeout: The read operation timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.9/site-packages/requests/adapters.py", line 439, in send
    resp = conn.urlopen(
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 756, in urlopen
    retries = retries.increment(
  File "/usr/lib/python3.9/site-packages/urllib3/util/retry.py", line 532, in increment
    raise six.reraise(type(error), error, _stacktrace)
  File "/usr/lib/python3.9/site-packages/urllib3/packages/six.py", line 709, in reraise
    raise value
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 700, in urlopen
    httplib_response = self._make_request(
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 448, in _make_request
    self._raise_timeout(err=e, url=url, timeout_value=read_timeout)
  File "/usr/lib/python3.9/site-packages/urllib3/connectionpool.py", line 337, in _raise_timeout
    raise ReadTimeoutError(
urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host='cert-api.access.redhat.com', port=443): Read timed out. (read timeout=0.1)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/insights/insights-core/insights/client/phase/v1.py", line 36, in _f
    func(client, config)
  File "/home/insights/insights-core/insights/client/phase/v1.py", line 102, in pre_update
    rc = client.test_connection()
  File "/home/insights/insights-core/insights/client/__init__.py", line 69, in _init_connection
    return func(self, *args, **kwargs)
  File "/home/insights/insights-core/insights/client/__init__.py", line 87, in test_connection
    return self.connection.test_connection()
  File "/home/insights/insights-core/insights/client/connection.py", line 399, in test_connection
    upload_success = self._test_urls(self.upload_url, "POST")
  File "/home/insights/insights-core/insights/client/connection.py", line 374, in _test_urls
    test_req = self.post(url, files=test_files)
  File "/home/insights/insights-core/insights/client/connection.py", line 191, in post
    return self._http_request(url, 'POST', **kwargs)
  File "/home/insights/insights-core/insights/client/connection.py", line 175, in _http_request
    res = self.session.request(url=url, method=method, timeout=self.config.http_timeout, **kwargs)
  File "/usr/lib/python3.9/site-packages/requests/sessions.py", line 544, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/lib/python3.9/site-packages/requests/sessions.py", line 657, in send
    r = adapter.send(request, **kwargs)
  File "/usr/lib/python3.9/site-packages/requests/adapters.py", line 529, in send
    raise ReadTimeout(e, request=request)
requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='cert-api.access.redhat.com', port=443): Read timed out. (read timeout=0.1)

In this case, it’s ReadTimeoutError, not ConnectionTimeoutError that occured. This error has not been caught and thus a traceback has been printed, which is fixed by my newly opened pull request RedHatInsights/insights-core#4191.

The false positives can be easily avoided by making the error type assert more specific:

- assert "timeout=0.01" in output.stdout
+ assert "read timeout=0.01" in output.stdout

It however doesn’t resolve the problem of incorrectly testing ConnectionTimeoutError instead of ReadTimeoutError. Both need to be tested and you need to find a way how to reliably trigger each of those. If mocking/patching is not acceptable in integration tests, it may require a library like Responses to do the trick. (Technically, it’s still mocking, but on the requests layer and done by a well-established tool.) Doing so is generally a wise idea rather than connecting to real servers that may be down or have trouble responding.

For the time being, it’s at least possible to distinguish between the two error types by making the asserts more explicit as mentioned above: check the output for read timeout and read timeout rather than just timeout.

[Glutexo]

I’d still consider mocking the response rather than setting a HTTP timeout. The timeout value is dependent on the actual state of the environment and as such is unreliable: it doesn’t guarantee a read error, a connection error or any error at all. If not replacing it with a mock, the two solutions can complement each other.

An error raised by a mock is no different from a real error, it’s only 100% reliable. The error doesn’t contain any response, status code, headers or body that may differ from what the actual service returns; it only emulates the network condition. Without mocking, the only reliable way to test would be to make the target service not respond in time.

That’s why I think in this case, mocking makes sense even in the integration tests.

[m-horky]

Unblocked! To make this work, http_timeout needs to be set to 0.1 instead of 0.01.

Value of 0.01 effectively tests when there is no network at all, but it's rather a hack and it should be set up differently (perhaps with misconfigured proxy).

[m-horky]

LGTM