Skip to content

feat: assert denied requests are not received by upstream on data plane tests#972

Open
crstrn13 wants to merge 18 commits into
Kuadrant:mainfrom
crstrn13:issues/947
Open

feat: assert denied requests are not received by upstream on data plane tests#972
crstrn13 wants to merge 18 commits into
Kuadrant:mainfrom
crstrn13:issues/947

Conversation

@crstrn13
Copy link
Copy Markdown
Contributor

@crstrn13 crstrn13 commented May 7, 2026

Important

This PR modifies shared/core testsuite code that could potentially affect multiple test areas. 2 reviewers should review this PR to ensure adequate coverage.

Closes #947

Description

Adds automatic detection of upstream request leaks — when the gateway denies a request (401/403/429), the testsuite now verifies that the request never reached the backend. This catches bugs where denied requests bypass the gateway and leak to the upstream service.

Changes

Upstream Leak Detection

  • KuadrantClient injects an X-Testsuite-Tracking header (UUID) on every request and records tracking IDs for denied responses (401/403/429)
  • A pytest_runtest_makereport hook in the singlecluster conftest queries MockServer after each passing test to verify none of the denied requests leaked to the upstream
  • Mockserver.retrieve_requests_by_header() supports querying by header value for leak checks

Exposable Abstraction

  • Extracted Exposable base class from Gateway (testsuite/gateway/__init__.py) so that both Gateway and Backend can be exposed by an Exposer
  • Backend now implements Exposable with an expose() method that creates an admin service and hostname for external access
  • LoadBalancerServiceExposer gains a backend_service_type property returning "LoadBalancer" so backends get externally accessible services on Kind clusters
  • All exposers (OpenShiftExposer, LoadBalancerServiceExposer, DNSPolicyExposer) updated to accept Exposable instead of Gateway

Backend Changes

  • MockserverBackend no longer accepts a service_type parameter — service type is now determined by the exposer
  • The session-scoped backend fixture in singlecluster/conftest.py now exposes the backend via the default exposer for admin access

Verification

make kuadrant

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added automatic request tracking header injection in HTTP client.
    • Added denial-leak verification to detect if denied requests reach upstream backends.
    • Added method to query mock server by request header.
    • Improved backend exposure with flexible service strategies.
  • Refactor

    • Decoupled exposure APIs from gateway-specific implementation.
  • Chores

    • Added configuration flag for denial verification control.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Introduces Exposable abstraction for backend and gateway exposure via refactored exposer APIs, implements MockserverBackend exposure with external Service provisioning, adds per-request UUID tracking to KuadrantClient for denied requests, and implements a pytest hook that detects whether denied requests leaked to upstream backends.

Changes

Backend exposure and denied request leak detection

Layer / File(s) Summary
Exposable abstraction and Exposer API
testsuite/gateway/__init__.py, testsuite/gateway/exposers.py, testsuite/gateway/gateway_api/hostname.py
Introduces Exposable with abstract cluster and service_name properties and concrete defaults for external_ip, port_name, and get_tls_cert. Gateway now inherits from Exposable. All exposers change expose_hostname to accept Exposable instead of Gateway, and Exposer gains optional backend_service_type property for specifying Kubernetes Service types when exposing backends.
Backend storage and service name contract
testsuite/backend/__init__.py
Backend stores Kubernetes client privately in _cluster and exposes via cluster property; adds service_name property returning self.name; updates reference to use self.cluster.project for namespace.
MockserverBackend exposure implementation
testsuite/backend/mockserver.py
MockserverBackend implements Exposable, removes service_type constructor parameter, introduces pod selector labels and external exposure state, adds expose() method to provision external Kubernetes Service using exposer.backend_service_type, updates internal Service to use match_labels as selector, and cleans up external Service on deletion.
Request tracking and upstream query
testsuite/httpx/__init__.py, testsuite/mockserver.py
KuadrantClient injects X-Testsuite-Tracking UUID per request and records tracking IDs of denied responses (401/403/429) in denied_request_ids. Mockserver gains retrieve_requests_by_header() to query upstream requests filtered by header name and value.
Pytest configuration and test fixtures
testsuite/tests/conftest.py, Makefile, testsuite/tests/singlecluster/conftest.py
Adds --verify-denials pytest option and Makefile VERIFY_DENIALS variable. Introduces session-scoped backend_exposer fixture and updates backend fixture to expose MockServer via exposer. Implements pytest_runtest_makereport hook that verifies denied requests did not leak to upstream backends and fails tests when leaks are detected.
Test file updates
testsuite/tests/multicluster/conftest.py, testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py
Updates backend fixtures to use explicit backend_exposer and expose via mockserver.expose(); constructs Mockserver client from exposed hostname instead of hardcoded external IP.

Sequence Diagram

sequenceDiagram
  participant Test
  participant KuadrantClient
  participant Gateway
  participant Upstream as Upstream Backend
  Test->>KuadrantClient: request with tracking
  KuadrantClient->>KuadrantClient: inject X-Testsuite-Tracking UUID
  KuadrantClient->>Gateway: request with header
  alt Request denied
    Gateway-->>KuadrantClient: 401/403/429
    KuadrantClient->>KuadrantClient: record tracking ID in denied_request_ids
  else Request allowed
    Gateway->>Upstream: forward request
    Upstream-->>Gateway: response
    Gateway-->>KuadrantClient: 200/2xx
  end
  Test->>Test: snapshot denied_request_ids
  Test->>Upstream: retrieve_requests_by_header(X-Testsuite-Tracking, id)
  Upstream-->>Test: request list or empty
  alt Requests found upstream
    Test->>Test: fail with leak message
  else No leaked requests
    Test->>Test: pass
  end
Loading

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Kuadrant/testsuite#952: Both PRs modify testsuite/backend/mockserver.py constructor and Service setup; this PR removes service_type storage whilst the related PR makes it configurable and config-driven.

Suggested labels

Test case, Service Protection, enhancement, refactor

Suggested reviewers

  • azgabur
  • emmaaroche

Poem

🐰 A tracking header now marks every request so keen,
Denial or passage—each tale can be seen.
MockServer exposed through the test suite's new way,
We'll catch any leaks that attempt to betray!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature: automatic assertion that denied requests do not reach upstream services in data plane tests.
Description check ✅ Passed The PR description comprehensively covers all required sections with clear explanations of the upstream leak detection, exposable abstraction, backend changes, and verification instructions.
Linked Issues check ✅ Passed All coding requirements from issue #947 are addressed: tracking header injection [httpx/init.py], denied request recording [httpx/init.py], pytest hook for leak verification [singlecluster/conftest.py], backend querying by header [mockserver.py], and automatic verification on data plane tests [singlecluster/conftest.py].
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue objectives: implementing leak detection, creating the Exposable abstraction to enable backend exposure, updating exposer APIs, and modifying backend/gateway fixtures to support the new functionality.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@crstrn13 crstrn13 changed the title feat: makereport checking for leaking denied requests. feat: assert denied requests are not received by upstream on data plane tests May 19, 2026
@crstrn13 crstrn13 self-assigned this May 20, 2026
@crstrn13 crstrn13 marked this pull request as ready for review May 20, 2026 10:48
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
testsuite/backend/mockserver.py (1)

87-92: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the backend service-port contract (http).

Line 91 renames the Service port to api, but Backend.port still resolves get_port("http") (Line 84 in testsuite/backend/__init__.py). This will break callers using backend.port.

🐛 Proposed fix
-            ports=[ServicePort(name="api", port=8080, targetPort="api")],
+            ports=[ServicePort(name="http", port=8080, targetPort="api")],
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/backend/mockserver.py` around lines 87 - 92, The Service port name
was changed to "api" in Service.create_instance (ServicePort(name="api", ...))
which breaks Backend.port/get_port("http"); restore the expected port name by
renaming the ServicePort to "http" (or add an additional ServicePort with name
"http") so that Backend.port (and get_port("http")) can resolve the service port
correctly.
🧹 Nitpick comments (2)
testsuite/gateway/gateway_api/gateway.py (1)

18-18: ⚡ Quick win

Avoid unqualified pylint suppression for inheritance depth.

Please either address the root inheritance complexity or add a short justification explaining why this specific suppression is necessary and why a structural alternative is not practical here.

As per coding guidelines "Always look for a more correct solution before disabling a pylint warning".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/gateway/gateway_api/gateway.py` at line 18, The class
KuadrantGateway currently uses a blanket pylint suppression "# pylint:
disable=too-many-ancestors"; either remove the suppression by refactoring the
inheritance chain used by KuadrantGateway (e.g., extract common behavior into
composition or smaller base classes so KuadrantGateway no longer inherits from
many ancestors) or keep the suppression but replace it with a one-line
justification comment immediately above the class declaration referencing
KuadrantGateway and explaining why multiple inheritance is necessary and why
refactoring is impractical (for example: compatibility with KubernetesObject and
Gateway frameworks), so the intent is documented rather than silently
suppressing the warning.
testsuite/gateway/__init__.py (1)

162-162: ⚡ Quick win

Avoid fixture-only pylint suppression on a regular method.

Line 162 uses # pylint: disable=unused-argument in a non-fixture method. Rename the parameter to _hostname and remove the suppression.

♻️ Proposed fix
-    def get_tls_cert(self, hostname: str) -> Optional[Certificate]:  # pylint: disable=unused-argument
+    def get_tls_cert(self, _hostname: str) -> Optional[Certificate]:
         """Returns TLS cert or None"""
         return None

As per coding guidelines: Use # pylint: disable=unused-argument only for fixtures that need to be in the signature for pytest dependency ordering but aren't directly used in the function body.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/gateway/__init__.py` at line 162, The method get_tls_cert currently
disables pylint for an unused parameter; rename the parameter from hostname to
_hostname in the get_tls_cert method signature and remove the inline "# pylint:
disable=unused-argument" comment, and update any internal references inside
get_tls_cert to use _hostname if needed so the code remains correct while
satisfying pylint rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testsuite/gateway/gateway_api/hostname.py`:
- Around line 56-60: The TLS-verify branch in expose_hostname creates a zero-arg
lambda but StaticHostname.client() invokes the getter with one argument, causing
a TypeError; change the getter to accept one parameter (e.g. lambda _arg:
self.verify) so its signature matches what StaticHostname.client() calls,
keeping the conditional logic around self.verify and preserving the return of
StaticHostname from expose_hostname.

In `@testsuite/tests/singlecluster/conftest.py`:
- Around line 106-114: The backend fixture accesses
testconfig["default_exposer"] without validating config; add a validation call
before using it by invoking
testconfig.validators.validate(only='default_exposer') in the backend function
(before the line that reads testconfig["default_exposer"]) so the required
section is checked and a clearer error is raised; leave the rest of the logic
(MockserverBackend creation, request.addfinalizer, commit, wait_for_ready, and
backend_exposer deletion) unchanged.
- Around line 188-199: The current early return prevents clearing
client.denied_request_ids for failed tests and can cause AttributeError if the
fixture doesn't have that attribute; remove or move the "if call.when != 'call'
or not report.passed: return" so the cleanup runs for every call, then perform
type/attribute checks before accessing attributes: fetch client and backend from
item.funcargs, if client is None skip; if hasattr(client, "denied_request_ids")
then capture denied_ids = set(client.denied_request_ids) and clear it
(client.denied_request_ids.clear()); only after cleanup check report.passed (or
use call.when) to decide whether to proceed with the rest of the logic that
depends on a passing test and backend/admin_hostname.

---

Outside diff comments:
In `@testsuite/backend/mockserver.py`:
- Around line 87-92: The Service port name was changed to "api" in
Service.create_instance (ServicePort(name="api", ...)) which breaks
Backend.port/get_port("http"); restore the expected port name by renaming the
ServicePort to "http" (or add an additional ServicePort with name "http") so
that Backend.port (and get_port("http")) can resolve the service port correctly.

---

Nitpick comments:
In `@testsuite/gateway/__init__.py`:
- Line 162: The method get_tls_cert currently disables pylint for an unused
parameter; rename the parameter from hostname to _hostname in the get_tls_cert
method signature and remove the inline "# pylint: disable=unused-argument"
comment, and update any internal references inside get_tls_cert to use _hostname
if needed so the code remains correct while satisfying pylint rules.

In `@testsuite/gateway/gateway_api/gateway.py`:
- Line 18: The class KuadrantGateway currently uses a blanket pylint suppression
"# pylint: disable=too-many-ancestors"; either remove the suppression by
refactoring the inheritance chain used by KuadrantGateway (e.g., extract common
behavior into composition or smaller base classes so KuadrantGateway no longer
inherits from many ancestors) or keep the suppression but replace it with a
one-line justification comment immediately above the class declaration
referencing KuadrantGateway and explaining why multiple inheritance is necessary
and why refactoring is impractical (for example: compatibility with
KubernetesObject and Gateway frameworks), so the intent is documented rather
than silently suppressing the warning.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e1c0740-b0a4-4376-9c12-0423f495e5d8

📥 Commits

Reviewing files that changed from the base of the PR and between 8c24e1e and 55aa287.

📒 Files selected for processing (15)
  • Makefile
  • testsuite/backend/__init__.py
  • testsuite/backend/mockserver.py
  • testsuite/gateway/__init__.py
  • testsuite/gateway/exposers.py
  • testsuite/gateway/gateway_api/gateway.py
  • testsuite/gateway/gateway_api/hostname.py
  • testsuite/httpx/__init__.py
  • testsuite/mockserver.py
  • testsuite/tests/multicluster/conftest.py
  • testsuite/tests/singlecluster/authorino/conftest.py
  • testsuite/tests/singlecluster/authorino/identity/x509/gateway_validation/test_multi_ca_trust.py
  • testsuite/tests/singlecluster/conftest.py
  • testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py
  • testsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing.py

Comment thread testsuite/gateway/gateway_api/hostname.py Outdated
Comment thread testsuite/tests/singlecluster/conftest.py Outdated
Comment thread testsuite/tests/singlecluster/conftest.py Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testsuite/backend/mockserver.py`:
- Line 129: The ServicePort name in Mockserver's manifest is "api" but
Backend.port expects "http", causing MockserverBackend.port resolution to fail;
update the ServicePort declaration in testsuite/backend/mockserver.py (the
ServicePort(...) entry) to use name="http" so MockserverBackend.port and the
base Backend.port lookup succeed. Ensure only the port name is changed and leave
port and targetPort values unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d74fe89f-5c36-4783-8082-21214d05f08c

📥 Commits

Reviewing files that changed from the base of the PR and between 55aa287 and 39ef692.

📒 Files selected for processing (4)
  • testsuite/backend/__init__.py
  • testsuite/backend/mockserver.py
  • testsuite/tests/singlecluster/conftest.py
  • testsuite/tests/singlecluster/gateway/dnspolicy/health_check/test_additional_headers.py

Comment thread testsuite/backend/mockserver.py Outdated
@crstrn13 crstrn13 requested a review from a team May 20, 2026 13:30
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
testsuite/tests/singlecluster/conftest.py (1)

212-214: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail the test when upstream leak verification cannot run.

On Line 212-Line 214, the hook logs and returns on MockServer/httpx errors, leaving a passing test unchanged even though the leak check was not completed. That can mask regressions in the exact gate this hook is meant to enforce.

Suggested minimal fix
-    except (httpx.RequestError, httpx.HTTPStatusError):
-        logging.warning("Failed to check for upstream leaks via MockServer", exc_info=True)
-        return
+    except (httpx.RequestError, httpx.HTTPStatusError) as exc:
+        logging.warning("Failed to check for upstream leaks via MockServer", exc_info=True)
+        report.outcome = "failed"
+        report.longrepr = (
+            "Denied-request leak verification could not be completed due to a MockServer check failure: "
+            f"{exc!r}"
+        )
+        return
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/tests/singlecluster/conftest.py` around lines 212 - 214, The
current except block catching httpx.RequestError and httpx.HTTPStatusError in
testsuite/tests/singlecluster/conftest.py silently logs and returns, allowing
the test to pass when the upstream leak check didn't run; change that behavior
to fail the test instead by replacing the logging+return with a pytest.fail(...)
call (and import pytest if missing) that includes a clear message and the
exception details (use exc_info or format the exception) so the test is marked
failed when MockServer/httpx errors prevent the leak verification from running.
♻️ Duplicate comments (1)
testsuite/tests/singlecluster/conftest.py (1)

106-109: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate default_exposer config before reading it.

backend_exposer accesses testconfig["default_exposer"] on Line 108 without validating that section first, so failures here can be less actionable than needed.

Suggested minimal fix
 `@pytest.fixture`(scope="session")
 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

As per coding guidelines: "Test fixtures should validate their required config sections before use by calling testconfig.validators.validate(only='section_name')".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/tests/singlecluster/conftest.py` around lines 106 - 109,
backend_exposer reads testconfig["default_exposer"] without validating config
first; call the testconfig validator to ensure the "default_exposer" section
exists before accessing it (e.g., invoke
testconfig.validators.validate(only="default_exposer") at the start of
backend_exposer), then continue to create exp via
testconfig["default_exposer"](cluster) and register
request.addfinalizer(exp.delete) as before.
🧹 Nitpick comments (1)
testsuite/gateway/gateway_api/hostname.py (1)

56-56: ⚡ Quick win

Consider adding a type hint for the exposable parameter.

Since this method signature introduces the new Exposable abstraction, adding an explicit type annotation would make the contract clearer for future maintainers.

📝 Suggested type hint addition

First, import Exposable at the top:

-from testsuite.gateway import Hostname, Exposer
+from testsuite.gateway import Hostname, Exposer, Exposable

Then add the type hint:

-    def expose_hostname(self, name, exposable) -> Hostname:
+    def expose_hostname(self, name, exposable: Exposable) -> Hostname:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/gateway/gateway_api/hostname.py` at line 56, The expose_hostname
method currently lacks a type annotation for the exposable parameter; add an
explicit Exposable type hint by importing Exposable at the top of the file and
changing the signature of expose_hostname to accept exposable: Exposable (i.e.,
update the function definition for expose_hostname to reference the Exposable
type so callers and linters get the correct contract).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@testsuite/tests/singlecluster/conftest.py`:
- Around line 212-214: The current except block catching httpx.RequestError and
httpx.HTTPStatusError in testsuite/tests/singlecluster/conftest.py silently logs
and returns, allowing the test to pass when the upstream leak check didn't run;
change that behavior to fail the test instead by replacing the logging+return
with a pytest.fail(...) call (and import pytest if missing) that includes a
clear message and the exception details (use exc_info or format the exception)
so the test is marked failed when MockServer/httpx errors prevent the leak
verification from running.

---

Duplicate comments:
In `@testsuite/tests/singlecluster/conftest.py`:
- Around line 106-109: backend_exposer reads testconfig["default_exposer"]
without validating config first; call the testconfig validator to ensure the
"default_exposer" section exists before accessing it (e.g., invoke
testconfig.validators.validate(only="default_exposer") at the start of
backend_exposer), then continue to create exp via
testconfig["default_exposer"](cluster) and register
request.addfinalizer(exp.delete) as before.

---

Nitpick comments:
In `@testsuite/gateway/gateway_api/hostname.py`:
- Line 56: The expose_hostname method currently lacks a type annotation for the
exposable parameter; add an explicit Exposable type hint by importing Exposable
at the top of the file and changing the signature of expose_hostname to accept
exposable: Exposable (i.e., update the function definition for expose_hostname
to reference the Exposable type so callers and linters get the correct
contract).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a9c19e6c-35ab-4137-806e-c85bf7a63260

📥 Commits

Reviewing files that changed from the base of the PR and between 39ef692 and 20e8ca8.

📒 Files selected for processing (4)
  • testsuite/backend/__init__.py
  • testsuite/backend/mockserver.py
  • testsuite/gateway/gateway_api/hostname.py
  • testsuite/tests/singlecluster/conftest.py

Comment thread testsuite/tests/singlecluster/conftest.py
Comment on lines +212 to +214
except (httpx.RequestError, httpx.HTTPStatusError):
logging.warning("Failed to check for upstream leaks via MockServer", exc_info=True)
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hook silently logs MockServer errors and lets test pass. What happens if MockServer admin is unreachable - should test fail or at least be skipped (leak check couldn't run, can't guarantee no leak) or pass (don't break tests when admin unavailable)? Current choice might hide real leaks if MockServer down.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I guess the mockserver should never be down as it is the sole backend in this case. I wasn't too sure myself either which would be the best course of action here. I guess I wanted to see it work and to not fail as this is more of a supplementary check. Thinking of wrapping it under a feature flag so to do the extra check only when instructed to. I plan on adding the feature flag, but not sure if I should the fail the test instead of warning. What do you reckon?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature flag approach sounds good 👍 Thinking about it more, I'd go with warning instead of failing the test as that way you'd still see when the check didn't run without blocking tests that are otherwise passing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going with warning, maybe it could be enabled by default instead of when instructed to? Since it won't fail tests, just flags potential issues in output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think this is what I had in mind as well. It will just warn, the flag will turn on/off the hook at the end.

Comment on lines +38 to +41
def authorization(authorization, oidc_provider, gateway, authorization_name, cluster, label) -> AuthConfig:
"""In case of Authorino, AuthConfig used for authorization"""
if authorization is None:
authorization = AuthConfig.create_instance(cluster, authorization_name, route, labels={"testRun": label})
authorization = AuthConfig.create_instance(cluster, authorization_name, gateway, labels={"testRun": label})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from route to gateway breaks all standalone Authorino tests.

AuthConfig.create_instance calls target.add_auth_config() on its third argument (testsuite/kuadrant/policy/authorization/auth_config.py:57), but add_auth_config is only defined on EnvoyVirtualRoute (testsuite/gateway/envoy/route.py:61), not on Envoy/TLSEnvoy. In standalone mode the gateway is an Envoy instance, so this raises AttributeError: 'Envoy' object has no attribute 'add_auth_config'.
This needs to be reverted back to route

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my, thanks a lot for finding this one. It slipped me completely

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testsuite/tests/singlecluster/conftest.py`:
- Around line 198-199: The check that gates the leak verification currently
compares item.config.getoption("--verify-denials") exactly to "true", which
misses variants like "True" or surrounding whitespace; update the condition to
normalize the option value (e.g., call .strip().lower() on the returned string
or otherwise parse it as a boolean) before comparing to "true" so inputs like "
True " or "TRUE" still enable verification; modify the code around the existing
item.config.getoption("--verify-denials") usage to perform this normalization
and keep the early return behavior intact.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 086d5732-da00-45d2-ad3b-1af624c500bd

📥 Commits

Reviewing files that changed from the base of the PR and between 0f8c3d0 and ea1ab90.

📒 Files selected for processing (3)
  • Makefile
  • testsuite/tests/conftest.py
  • testsuite/tests/singlecluster/conftest.py

Comment on lines +198 to +199
if item.config.getoption("--verify-denials") != "true":
return
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalise the --verify-denials value before gating the leak check.

Line 198 only enables verification for the exact string "true". Inputs like "True" or " true " will silently skip the check.

Suggested fix
-    if item.config.getoption("--verify-denials") != "true":
+    verify_denials = str(item.config.getoption("--verify-denials")).strip().lower() == "true"
+    if not verify_denials:
         return
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if item.config.getoption("--verify-denials") != "true":
return
verify_denials = str(item.config.getoption("--verify-denials")).strip().lower() == "true"
if not verify_denials:
return
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@testsuite/tests/singlecluster/conftest.py` around lines 198 - 199, The check
that gates the leak verification currently compares
item.config.getoption("--verify-denials") exactly to "true", which misses
variants like "True" or surrounding whitespace; update the condition to
normalize the option value (e.g., call .strip().lower() on the returned string
or otherwise parse it as a boolean) before comparing to "true" so inputs like "
True " or "TRUE" still enable verification; modify the code around the existing
item.config.getoption("--verify-denials") usage to perform this normalization
and keep the early return behavior intact.

@crstrn13 crstrn13 requested review from emmaaroche and silvi-t May 25, 2026 15:00
crstrn13 added 16 commits May 27, 2026 21:19
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>

# Conflicts:
#	testsuite/backend/mockserver.py
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
crstrn13 added 2 commits May 27, 2026 21:20
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert denied requests are not received by the upstream on every data plane test

3 participants