diff --git a/Makefile b/Makefile index 9b6dd98a7..070074ecd 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,7 @@ TB ?= short LOGLEVEL ?= INFO +VERIFY_DENIALS ?= true # Set to false to skip verification of expected denials (e.g., 403 responses) in tests marked with `verify_denials` (e.g., `authorino` tests). This can be useful when debugging test failures that may be related to unexpected denials, but should generally be left enabled to ensure proper test validation. ifdef WORKSPACE # Yes, this is for jenkins resultsdir = $(WORKSPACE) @@ -16,6 +17,10 @@ ifdef junit PYTEST += --junitxml=$(resultsdir)/junit-$(@F).xml -o junit_suite_name=$(@F) endif +ifdef VERIFY_DENIALS # Only add the flag if VERIFY_DENIALS is set to a non-empty value (default is "true") +PYTEST += --verify-denials=$(VERIFY_DENIALS) +endif + # Collector PYTEST Override collect: PYTEST = poetry run python -m pytest --tb=$(TB) --junitxml=$(resultsdir)/junit-00-$(@F).xml -o junit_suite_name=info-collector diff --git a/testsuite/backend/__init__.py b/testsuite/backend/__init__.py index f0af91507..e4bc8d638 100644 --- a/testsuite/backend/__init__.py +++ b/testsuite/backend/__init__.py @@ -2,23 +2,35 @@ from abc import abstractmethod from functools import cached_property +from typing import TYPE_CHECKING, Optional -from testsuite.gateway import Referencable +from testsuite.gateway import Referencable, Exposable from testsuite.lifecycle import LifecycleObject from testsuite.kubernetes.client import KubernetesClient from testsuite.utils.constants import HTTP_API_PORT +if TYPE_CHECKING: + from testsuite.gateway import Hostname, Exposer -class Backend(LifecycleObject, Referencable): + +class Backend(LifecycleObject, Referencable, Exposable): """Backend (workload) deployed in Kubernetes""" def __init__(self, cluster: KubernetesClient, name: str, label: str): - self.cluster = cluster + self._cluster = cluster + self.name = name self.label = label self.deployment = None self.service = None + self._admin_hostname: Optional["Hostname"] = None + self._admin_service = None + + @property + def cluster(self) -> KubernetesClient: + """Returns KubernetesClient""" + return self._cluster @property def reference(self): @@ -30,6 +42,24 @@ def reference(self): "namespace": self.cluster.project, } + @property + def service_name(self) -> str: + return self.name + + @property + def match_labels(self) -> dict[str, str]: + """Pod selector labels used by this backend's deployment""" + return {"app": self.label, "deployment": self.name} + + @property + def admin_hostname(self) -> Optional["Hostname"]: + """Returns Hostname for external admin access, or None if not exposed""" + return self._admin_hostname + + def expose(self, exposer: "Exposer", name: str): + """Creates external access for admin APIs via the exposer""" + self._admin_hostname = exposer.expose_backend(name, self) + @property def url(self): """Returns internal URL for this backend""" @@ -47,6 +77,9 @@ def commit(self): def delete(self): """Clean-up the backend""" with self.cluster.context: + if self._admin_service: + self._admin_service.delete() + self._admin_service = None if self.service: self.service.delete() self.service = None diff --git a/testsuite/gateway/__init__.py b/testsuite/gateway/__init__.py index 27eea48da..c03d9a8cd 100644 --- a/testsuite/gateway/__init__.py +++ b/testsuite/gateway/__init__.py @@ -13,8 +13,8 @@ from testsuite.utils import asdict if TYPE_CHECKING: - from testsuite.kubernetes.client import KubernetesClient from testsuite.backend import Backend + from testsuite.kubernetes.client import KubernetesClient class Referencable(ABC): @@ -142,21 +142,38 @@ class GRPCRouteMatch: headers: Optional[List[HeadersMatch]] = None -class Gateway(LifecycleObject, Referencable): - """ - Abstraction layer for a Gateway sitting between end-user and Kuadrant - Simplified: Equals to Gateway Kubernetes object - """ +class Exposable(ABC): + """Object that can be exposed by an Exposer (has a cluster and a service name)""" @property @abstractmethod def cluster(self) -> "KubernetesClient": - """Returns KubernetesClient for this gateway""" + """Returns KubernetesClient""" @property @abstractmethod def service_name(self) -> str: - """Service name for this gateway""" + """Service name to route traffic to""" + + def external_ip(self) -> str: + """Returns external IP and port for external access""" + raise NotImplementedError(f"{type(self).__name__} does not support external_ip") + + @property + def port_name(self) -> str: + """Service port name used when creating external routes""" + return "api" + + def get_tls_cert(self, hostname: str) -> Optional[Certificate]: # pylint: disable=unused-argument + """Returns TLS cert or None""" + return None + + +class Gateway(LifecycleObject, Referencable, Exposable): + """ + Abstraction layer for a Gateway sitting between end-user and Kuadrant + Simplified: Equals to Gateway Kubernetes object + """ @abstractmethod def external_ip(self) -> str: @@ -276,12 +293,17 @@ def __init__(self, cluster): self.verify = None @abstractmethod - def expose_hostname(self, name, gateway: Gateway) -> Hostname: + def expose_hostname(self, name, exposable: Exposable) -> Hostname: """ Exposes hostname, so it is accessible from outside Actual hostname is generated from "name" and is returned in a form of a Hostname object """ + @property + def backend_service_type(self) -> Optional[Literal["ClusterIP", "LoadBalancer", "NodePort", "ExternalName"]]: + """Service type to create when exposing a backend directly. None means no extra service needed.""" + return None + @property @abstractmethod def base_domain(self) -> str: diff --git a/testsuite/gateway/exposers.py b/testsuite/gateway/exposers.py index 9f33688c2..7fffe17b9 100644 --- a/testsuite/gateway/exposers.py +++ b/testsuite/gateway/exposers.py @@ -1,6 +1,8 @@ """General exposers, not tied to Envoy or Gateway API""" -from testsuite.gateway import Exposer, Gateway, Hostname +from typing import Literal + +from testsuite.gateway import Exposer, Hostname from testsuite.httpx import KuadrantClient, ForceSNIClient from testsuite.kubernetes.openshift.route import OpenshiftRoute @@ -16,14 +18,14 @@ def __init__(self, cluster) -> None: def base_domain(self) -> str: return self.cluster.apps_url - def expose_hostname(self, name, gateway: Gateway) -> Hostname: + def expose_hostname(self, name, exposable) -> Hostname: tls = False termination = "edge" if self.passthrough: tls = True termination = "passthrough" route = OpenshiftRoute.create_instance( - gateway.cluster, name, gateway.service_name, "api", tls=tls, termination=termination + exposable.cluster, name, exposable.service_name, exposable.port_name, tls=tls, termination=termination ) route.verify = self.verify self.routes.append(route) @@ -68,12 +70,16 @@ def hostname(self): class LoadBalancerServiceExposer(Exposer): """Exposer using Load Balancer service for Gateway""" - def expose_hostname(self, name, gateway: Gateway) -> Hostname: + def expose_hostname(self, name, exposable) -> Hostname: hostname = f"{name}.{self.base_domain}" return StaticLocalHostname( - hostname, gateway.external_ip, lambda: gateway.get_tls_cert(hostname), force_https=self.passthrough + hostname, exposable.external_ip, lambda: exposable.get_tls_cert(hostname), force_https=self.passthrough ) + @property + def backend_service_type(self) -> Literal["LoadBalancer"]: + return "LoadBalancer" + @property def base_domain(self) -> str: return "test.com" diff --git a/testsuite/gateway/gateway_api/hostname.py b/testsuite/gateway/gateway_api/hostname.py index 2b74da9f9..a660895ea 100644 --- a/testsuite/gateway/gateway_api/hostname.py +++ b/testsuite/gateway/gateway_api/hostname.py @@ -8,7 +8,7 @@ from testsuite.certificates import Certificate from testsuite.config import settings from testsuite.httpx import KuadrantClient -from testsuite.gateway import Gateway, Hostname, Exposer +from testsuite.gateway import Exposable, Hostname, Exposer from testsuite.utils import generate_tail @@ -53,10 +53,10 @@ def base_domain(self) -> str: ) from exc return f'{generate_tail(5)}.{secret.model["metadata"]["annotations"]["base_domain"]}' - def expose_hostname(self, name, gateway: Gateway) -> Hostname: + def expose_hostname(self, name, exposable: Exposable) -> Hostname: return StaticHostname( f"{name}.{self.base_domain}", - gateway.get_tls_cert if self.verify is None else lambda: self.verify, # type: ignore + exposable.get_tls_cert if self.verify is None else lambda _hostname: self.verify, # type: ignore ) def commit(self): diff --git a/testsuite/httpx/__init__.py b/testsuite/httpx/__init__.py index 33de39777..b8f81b6b5 100644 --- a/testsuite/httpx/__init__.py +++ b/testsuite/httpx/__init__.py @@ -2,6 +2,7 @@ import ssl import typing +import uuid # I change return type of HTTPX client to Kuadrant Result # mypy: disable-error-code="override, return-value" @@ -114,6 +115,9 @@ def assert_all(self, status_code): class KuadrantClient(Client): """Httpx client which retries unstable requests""" + TRACKING_HEADER = "X-Testsuite-Tracking" + GATEWAY_DENIED_CODES = {401, 403, 429} + def __init__( self, *, @@ -123,6 +127,7 @@ def __init__( **kwargs, ): self.files = [] + self.denied_request_ids: list[str] = [] self.retry_codes = {503} if retry_codes is None else set(retry_codes) _verify = None if isinstance(verify, Certificate): @@ -172,6 +177,9 @@ def request( timeout=None, extensions=None, ) -> Result: + headers = dict(headers) if headers else {} + tracking_id = headers.setdefault(self.TRACKING_HEADER, str(uuid.uuid4())) + try: response = super().request( method, @@ -188,7 +196,10 @@ def request( timeout=timeout, extensions=extensions, ) - return Result(self.retry_codes, response=response) + result = Result(self.retry_codes, response=response) + if result.status_code in self.GATEWAY_DENIED_CODES: + self.denied_request_ids.append(tracking_id) + return result except RequestError as e: return Result(self.retry_codes, error=e) diff --git a/testsuite/mockserver.py b/testsuite/mockserver.py index 0b1d8d4a8..d56db8fe1 100644 --- a/testsuite/mockserver.py +++ b/testsuite/mockserver.py @@ -75,3 +75,10 @@ def retrieve_requests(self, expectation_id): params={"type": "REQUESTS", "format": "JSON"}, json={"path": "/" + expectation_id}, ).json() + + def retrieve_requests_by_header(self, header_name, header_value): + """Retrieve requests matching a specific header value""" + return self.client.mockserver.retrieve.put( + params={"type": "REQUESTS", "format": "JSON"}, + json={"headers": {header_name: [header_value]}}, + ).json() diff --git a/testsuite/tests/conftest.py b/testsuite/tests/conftest.py index 903533c41..349dbf584 100644 --- a/testsuite/tests/conftest.py +++ b/testsuite/tests/conftest.py @@ -31,6 +31,9 @@ def pytest_addoption(parser): "--enforce", action="store_true", default=False, help="Fails tests instead of skip, if capabilities are missing" ) parser.addoption("--standalone", action="store_true", default=False, help="Runs testsuite in standalone mode") + parser.addoption( + "--verify-denials", default="false", help="Verifies that denied requests did not leak to the upstream backend" + ) def pytest_runtest_setup(item): diff --git a/testsuite/tests/multicluster/conftest.py b/testsuite/tests/multicluster/conftest.py index 14964e078..b347fe9a8 100644 --- a/testsuite/tests/multicluster/conftest.py +++ b/testsuite/tests/multicluster/conftest.py @@ -82,7 +82,7 @@ def backends(request, cluster, cluster2, blame, label): data={"echo_expectation.json": echo_json}, ) config.commit() - mockserver = MockserverBackend(cluster_client, name, label, service_type="ClusterIP", config=config) + mockserver = MockserverBackend(cluster_client, name, label, config=config) request.addfinalizer(mockserver.delete) mockserver.commit() mockserver.wait_for_ready() diff --git a/testsuite/tests/singlecluster/conftest.py b/testsuite/tests/singlecluster/conftest.py index d378d96e8..8729000ae 100644 --- a/testsuite/tests/singlecluster/conftest.py +++ b/testsuite/tests/singlecluster/conftest.py @@ -1,8 +1,10 @@ """Configure all the components through Kuadrant, all methods are placeholders for now since we do not work with Kuadrant""" +import logging from importlib import resources +import httpx import pytest from openshift_client import selector @@ -12,11 +14,13 @@ from testsuite.gateway.envoy.route import EnvoyVirtualRoute from testsuite.gateway.gateway_api.gateway import KuadrantGateway from testsuite.gateway.gateway_api.route import HTTPRoute +from testsuite.httpx import KuadrantClient from testsuite.kuadrant import KuadrantCR from testsuite.kuadrant.policy.authorization.auth_policy import AuthPolicy from testsuite.kuadrant.policy.rate_limit import RateLimitPolicy from testsuite.kubernetes.api_key import APIKey from testsuite.kubernetes.client import KubernetesClient +from testsuite.mockserver import Mockserver @pytest.fixture(scope="session") @@ -99,14 +103,23 @@ def mockserver_config(cluster, blame, label): @pytest.fixture(scope="session") -def backend(request, cluster, blame, label, mockserver_config): +def backend_exposer(request, testconfig, cluster): + """Dedicated session-scoped exposer for the backend, avoids ScopeMismatch with module-scoped exposer overrides""" + testconfig.validators.validate(only="default_exposer") + exp = testconfig["default_exposer"](cluster) + request.addfinalizer(exp.delete) + exp.commit() + return exp + + +@pytest.fixture(scope="session") +def backend(request, cluster, blame, label, mockserver_config, backend_exposer): """Deploys MockServer backend""" - mockserver = MockserverBackend( - cluster, blame("mockserver"), label, service_type="ClusterIP", config=mockserver_config - ) + mockserver = MockserverBackend(cluster, blame("mockserver"), label, config=mockserver_config) request.addfinalizer(mockserver.delete) mockserver.commit() mockserver.wait_for_ready() + mockserver.expose(backend_exposer, blame("backend")) return mockserver @@ -166,6 +179,54 @@ def client(route, hostname): # pylint: disable=unused-argument client.close() +@pytest.hookimpl(hookwrapper=True) +def pytest_runtest_makereport(item, call): # pylint: disable=unused-argument + """Verifies that denied requests did not leak to the upstream backend""" + outcome = yield + report = outcome.get_result() + + if call.when != "call": + return + + client = item.funcargs.get("client") + if not isinstance(client, KuadrantClient): + return + + denied_ids = set(client.denied_request_ids) + client.denied_request_ids.clear() + + if item.config.getoption("--verify-denials") != "true": + return + + backend = item.funcargs.get("backend") + if not report.passed or backend is None or not isinstance(backend, MockserverBackend): + return + if backend.external_hostname is None or not denied_ids: + return + + backend_client = backend.external_hostname.client() + ms = Mockserver(backend_client) + tracking_header = KuadrantClient.TRACKING_HEADER + + leaked = [] + try: + for denied_id in denied_ids: + if ms.retrieve_requests_by_header(tracking_header, denied_id): + leaked.append(denied_id) + except (httpx.RequestError, httpx.HTTPStatusError): + logging.warning("Failed to check for upstream leaks via MockServer", exc_info=True) + return + finally: + backend_client.close() + + if leaked: + report.outcome = "failed" + report.longrepr = ( + f"{len(leaked)} denied request(s) leaked to the upstream backend. " + f"The gateway returned a denial status (401/403/429) but the request still reached MockServer." + ) + + @pytest.fixture(scope="module") def create_api_key(blame, request, cluster): """Creates API key Secret""" diff --git a/testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py b/testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py index 6af38439d..947f59ff6 100644 --- a/testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py +++ b/testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py @@ -2,7 +2,6 @@ import pytest -from testsuite.httpx import KuadrantClient from testsuite.mockserver import Mockserver from testsuite.gateway import GatewayListener from testsuite.gateway.gateway_api.gateway import KuadrantGateway @@ -40,12 +39,13 @@ def gateway(request, cluster, blame, base_domain, module_label, subdomain): @pytest.fixture(scope="module") -def backend(request, cluster, blame, label): +def backend(request, cluster, blame, label, exposer): """Use mockserver as backend for health check requests to verify additional headers""" mockserver = MockserverBackend(cluster, blame("mocksrv"), label) request.addfinalizer(mockserver.delete) mockserver.commit() mockserver.wait_for_ready() + mockserver.expose(exposer, blame("mocksrv-admin")) return mockserver @@ -61,8 +61,8 @@ def headers_secret(request, cluster, blame): @pytest.fixture(scope="module") def mockserver_client(backend): - """Returns Mockserver client from load-balanced service IP""" - return Mockserver(KuadrantClient(base_url=f"http://{backend.service.refresh().external_ip}:8080")) + """Returns Mockserver client from externally exposed backend""" + return Mockserver(backend.external_hostname.client()) @pytest.fixture(scope="module")