From a6d60db4226ea77e80e5698156dc5f1866c0fe6f Mon Sep 17 00:00:00 2001 From: phala Date: Thu, 2 Nov 2023 10:16:59 +0100 Subject: [PATCH] Add has_dns_error() and has_tls_error() assert to mgc tests --- testsuite/httpx/__init__.py | 71 +++++++++++++------ .../openshift/objects/gateway_api/route.py | 6 +- testsuite/openshift/objects/route.py | 4 +- .../authorino/operator/http/conftest.py | 4 +- testsuite/tests/mgc/test_basic.py | 10 +-- 5 files changed, 63 insertions(+), 32 deletions(-) diff --git a/testsuite/httpx/__init__.py b/testsuite/httpx/__init__.py index 3a555112..58adc191 100644 --- a/testsuite/httpx/__init__.py +++ b/testsuite/httpx/__init__.py @@ -1,9 +1,11 @@ """Common classes for Httpx""" +# I change return type of HTTPX client to Kuadrant Result +# mypy: disable-error-code="override, return-value" from tempfile import NamedTemporaryFile from typing import Union import backoff -from httpx import Client, Response, ConnectError +from httpx import Client, ConnectError from testsuite.certificates import Certificate @@ -18,15 +20,46 @@ def create_tmp_file(content: str): return file -class UnexpectedResponse(Exception): - """Slightly different response attributes were expected or no response was given""" +class Result: + """Result from HTTP request""" - def __init__(self, msg, response): - super().__init__(msg) + def __init__(self, retry_codes, response=None, error=None): self.response = response - - -class HttpxBackoffClient(Client): + self.error = error + self.retry_codes = retry_codes + + def should_backoff(self): + """True, if the Result can be considered an instability and should be retried""" + return self.has_dns_error() or (not self.has_error() and self.status_code in self.retry_codes) + + def has_error(self): + """True, if the request failed and an error was returned""" + return self.error is not None + + def has_dns_error(self): + """True, if the result failed due to DNS failure""" + return ( + self.has_error() + and len(self.error.args) > 0 + and any("Name or service not known" in arg for arg in self.error.args) + ) + + def has_tls_error(self): + """True, if the result failed due to TLS failure""" + return ( + self.has_error() + and len(self.error.args) > 0 + and any("SSL: CERTIFICATE_VERIFY_FAILED" in arg for arg in self.error.args) + ) + + def __getattr__(self, item): + """For backwards compatibility""" + if self.response is not None: + return getattr(self.response, item) + return None + + +class KuadrantClient(Client): """Httpx client which retries unstable requests""" def __init__(self, *, verify: Union[Certificate, bool] = True, cert: Certificate = None, **kwargs): @@ -59,7 +92,7 @@ def add_retry_code(self, code): self.retry_codes.add(code) # pylint: disable=too-many-locals - @backoff.on_exception(backoff.fibo, UnexpectedResponse, max_tries=8, jitter=None) + @backoff.on_predicate(backoff.fibo, lambda result: result.should_backoff(), max_tries=8, jitter=None) def request( self, method: str, @@ -76,7 +109,7 @@ def request( follow_redirects=None, timeout=None, extensions=None, - ) -> Response: + ) -> Result: try: response = super().request( method, @@ -93,18 +126,14 @@ def request( timeout=timeout, extensions=extensions, ) - if response.status_code in self.retry_codes: - raise UnexpectedResponse(f"Didn't expect '{response.status_code}' status code", response) - return response + return Result(self.retry_codes, response=response) except ConnectError as e: - # note: when the code reaches this point, negative caching might have been triggered, - # negative caching TTL of SOA record of the zone must be set accordingly, - # otherwise retry will fail if the value is too high - if len(e.args) > 0 and any("Name or service not known" in arg for arg in e.args): - raise UnexpectedResponse("Didn't expect 'Name or service not known' error", None) from e - raise - - def get_many(self, url, count, *, params=None, headers=None, auth=None) -> list[Response]: + return Result(self.retry_codes, error=e) + + def get(self, *args, **kwargs) -> Result: + return super().get(*args, **kwargs) + + def get_many(self, url, count, *, params=None, headers=None, auth=None) -> list[Result]: """Send multiple `GET` requests.""" responses = [] for _ in range(count): diff --git a/testsuite/openshift/objects/gateway_api/route.py b/testsuite/openshift/objects/gateway_api/route.py index f50789af..4ece3ae7 100644 --- a/testsuite/openshift/objects/gateway_api/route.py +++ b/testsuite/openshift/objects/gateway_api/route.py @@ -6,7 +6,7 @@ from httpx import Client -from testsuite.httpx import HttpxBackoffClient +from testsuite.httpx import KuadrantClient from testsuite.openshift.client import OpenShiftClient from testsuite.openshift.objects import modify, OpenShiftObject from testsuite.openshift.objects.route import Route @@ -23,7 +23,7 @@ class HTTPRoute(OpenShiftObject, Referencable): def client(self, **kwargs) -> Client: """Returns HTTPX client""" - return HttpxBackoffClient(base_url=f"http://{self.hostnames[0]}", **kwargs) + return KuadrantClient(base_url=f"http://{self.hostnames[0]}", **kwargs) @classmethod def create_instance( @@ -106,7 +106,7 @@ def hostname(self) -> str: return self._hostname def client(self, **kwargs) -> Client: - return HttpxBackoffClient(base_url=f"http://{self.hostname}", **kwargs) + return KuadrantClient(base_url=f"http://{self.hostname}", **kwargs) @property def reference(self) -> dict[str, str]: diff --git a/testsuite/openshift/objects/route.py b/testsuite/openshift/objects/route.py index a83f31d2..fbb3090d 100644 --- a/testsuite/openshift/objects/route.py +++ b/testsuite/openshift/objects/route.py @@ -4,7 +4,7 @@ from httpx import Client -from testsuite.httpx import HttpxBackoffClient +from testsuite.httpx import KuadrantClient from testsuite.openshift.objects import OpenShiftObject @@ -53,7 +53,7 @@ def client(self, **kwargs) -> Client: protocol = "http" if "tls" in self.model.spec: protocol = "https" - return HttpxBackoffClient(base_url=f"{protocol}://{self.hostname}", **kwargs) + return KuadrantClient(base_url=f"{protocol}://{self.hostname}", **kwargs) @cached_property def hostname(self): diff --git a/testsuite/tests/kuadrant/authorino/operator/http/conftest.py b/testsuite/tests/kuadrant/authorino/operator/http/conftest.py index 8edd2c09..91f303b8 100644 --- a/testsuite/tests/kuadrant/authorino/operator/http/conftest.py +++ b/testsuite/tests/kuadrant/authorino/operator/http/conftest.py @@ -2,7 +2,7 @@ import pytest from testsuite.objects import Value, JsonResponse -from testsuite.httpx import HttpxBackoffClient +from testsuite.httpx import KuadrantClient from testsuite.openshift.objects.auth_config import AuthConfig from testsuite.openshift.objects.route import OpenshiftRoute @@ -20,7 +20,7 @@ def authorization(authorization, wildcard_domain, openshift, module_label) -> Au @pytest.fixture(scope="module") def client(authorization, authorino_route): """Returns httpx client to be used for requests, it also commits AuthConfig""" - client = HttpxBackoffClient(base_url=f"http://{authorino_route.model.spec.host}", verify=False) + client = KuadrantClient(base_url=f"http://{authorino_route.model.spec.host}", verify=False) yield client client.close() diff --git a/testsuite/tests/mgc/test_basic.py b/testsuite/tests/mgc/test_basic.py index 24dfea72..82661d63 100644 --- a/testsuite/tests/mgc/test_basic.py +++ b/testsuite/tests/mgc/test_basic.py @@ -15,7 +15,7 @@ """ import pytest -from testsuite.httpx import HttpxBackoffClient +from testsuite.httpx import KuadrantClient pytestmark = [pytest.mark.mgc] @@ -34,7 +34,9 @@ def test_smoke(route, upstream_gateway): tls_cert = upstream_gateway.get_tls_cert() # assert that tls_cert is used by the server - backend_client = HttpxBackoffClient(base_url=f"https://{route.hostnames[0]}", verify=tls_cert) + backend_client = KuadrantClient(base_url=f"https://{route.hostnames[0]}", verify=tls_cert) - response = backend_client.get("get") - assert response.status_code == 200 + result = backend_client.get("get") + assert not result.has_dns_error() + assert not result.has_tls_error() + assert result.status_code == 200