Skip to content

test(egress): add egress AuthPolicy+RateLimitPolicy test#914

Merged
averevki merged 1 commit intoKuadrant:mainfrom
averevki:egress-gateway-both-policies
Apr 14, 2026
Merged

test(egress): add egress AuthPolicy+RateLimitPolicy test#914
averevki merged 1 commit intoKuadrant:mainfrom
averevki:egress-gateway-both-policies

Conversation

@averevki
Copy link
Copy Markdown
Contributor

@averevki averevki commented Mar 27, 2026

Summary

Add E2E test for Kuadrant data plane policies on Istio egress gateway, based on the egress gateway investigation from Kuadrant/architecture#147.

  • Egress gateway setup: Gateway + ServiceEntry + DestinationRule (TLS origination) + HTTPRoute with kind: Hostname backendRef and URLRewrite filter
  • Policy validation: AuthPolicy (OIDC identity) rejects unauthenticated requests with 401, RateLimitPolicy enforces rate limits on egress traffic with 429

Changes

  • New ServiceEntry KubernetesObject class (testsuite/kubernetes/istio/service_entry.py)
  • New DestinationRule KubernetesObject class (testsuite/kubernetes/istio/destination_rule.py)
  • URLRewriteFilter dataclass for HTTPRoute filters (testsuite/gateway/__init__.py)
  • Added filters parameter to HTTPRoute.add_rule() (testsuite/gateway/gateway_api/route.py)
  • Added labels parameter to Httpbin backend (testsuite/backend/httpbin.py)
  • test_egress.py — egress gateway test with OIDC auth and rate limiting

Verification

make testsuite/tests/singlecluster/test_egress.py

Closes #912

Summary by CodeRabbit

Release Notes

  • New Features

    • Added URL rewrite filter support for gateway routes, enabling hostname rewrites in HTTP traffic.
    • Introduced Istio DestinationRule configuration with TLS origination support.
    • Introduced Istio ServiceEntry configuration for external service registration.
    • Enhanced egress traffic policy enforcement with authorisation and rate limiting capabilities.
  • Tests

    • Added comprehensive egress traffic enforcement test coverage, including authorisation and rate-limit validation scenarios.

@averevki averevki self-assigned this Mar 27, 2026
@averevki averevki added the Test case New test case label Mar 27, 2026
@averevki averevki added the Service Protection Issues for Service Protection tests label Mar 27, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 27, 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

The changes introduce testsuite support for egress traffic policy enforcement through Istio integration. New ServiceEntry and DestinationRule resource wrappers enable external service registration and TLS configuration. Gateway API HTTPRoute gains URLRewrite filter support for hostname rewriting. A comprehensive egress test module validates authorization and rate-limiting policy enforcement on egress gateway routes.

Changes

Cohort / File(s) Summary
Istio Resource Wrappers
testsuite/kubernetes/istio/service_entry.py, testsuite/kubernetes/istio/destination_rule.py
Added two new KubernetesObject subclasses with create_instance factory methods. ServiceEntry registers external hosts with configurable location and resolution. DestinationRule configures TLS origination with optional mode, SNI, and credential settings.
Gateway API Extensions
testsuite/gateway/__init__.py, testsuite/gateway/gateway_api/route.py
Introduced URLRewriteFilter dataclass for hostname rewriting. Extended HTTPRoute.add_rule() with optional filters parameter to conditionally attach filters to route rules.
Service Labelling
testsuite/backend/httpbin.py
Updated Httpbin.commit() to explicitly set labels={"app": self.label} when creating Kubernetes Service resources.
Egress Test Implementation
testsuite/tests/singlecluster/test_egress.py
Added comprehensive test module validating egress traffic policies. Provisions egress gateway, external service registration, TLS configuration, and dual-policy enforcement (authorization and rate limiting) through configurable fixtures and two test scenarios.

Sequence Diagram

sequenceDiagram
    participant Client
    participant EgressGateway as Egress Gateway
    participant AuthPolicy as Auth Policy
    participant RateLimitPolicy as Rate Limit Policy
    participant URLRewrite as URLRewrite Filter
    participant DestinationRule as DestinationRule (TLS)
    participant ServiceEntry as ServiceEntry
    participant Backend as External Backend

    Client->>EgressGateway: HTTP Request (egress.local)
    EgressGateway->>AuthPolicy: Check Authorization
    AuthPolicy-->>EgressGateway: Auth Result (401/200)
    alt Auth Denied
        EgressGateway-->>Client: 401 Unauthorized
    else Auth Allowed
        EgressGateway->>RateLimitPolicy: Check Rate Limit
        RateLimitPolicy-->>EgressGateway: Quota Status
        alt Rate Limited
            EgressGateway-->>Client: 429 Too Many Requests
        else Quota Available
            EgressGateway->>URLRewrite: Rewrite Host Header
            URLRewrite->>ServiceEntry: Route via ServiceEntry
            ServiceEntry->>DestinationRule: Initiate TLS
            DestinationRule->>Backend: HTTPS Request (rewritten host)
            Backend-->>DestinationRule: Response
            DestinationRule-->>EgressGateway: Response
            EgressGateway-->>Client: 200 OK
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Poem

🐰 Through the egress gate now hops the request,
With URLRewrite and TLS, we manifest the best!
ServiceEntry whispers where to find the way,
While auth and limits keep the chaos at bay. 🌐✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'test(egress): add egress AuthPolicy+RateLimitPolicy test' follows conventional commits format with the 'test' type and '(egress)' scope, clearly describing the addition of an egress test for dual policy enforcement.
Linked Issues check ✅ Passed All code changes align with the primary objectives from issue #912: ServiceEntry and DestinationRule KubernetesObject wrappers are implemented, HTTPRoute now supports URLRewrite filters, and the comprehensive test validates both AuthPolicy and RateLimitPolicy on egress traffic.
Out of Scope Changes check ✅ Passed All modifications are directly scoped to support the egress gateway test requirements: infrastructure classes (ServiceEntry, DestinationRule, URLRewriteFilter), HTTPRoute enhancements, backend service labelling, and the test implementation itself.
Docstring Coverage ✅ Passed Docstring coverage is 98.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed PR description follows the template with clear Summary, Changes, and Verification sections, and the title follows Conventional Commits format.

✏️ 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.

@averevki averevki moved this to Ready For Review in Kuadrant Mar 27, 2026
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.

🧹 Nitpick comments (4)
testsuite/gateway/gateway_api/route.py (1)

93-93: Use explicit | None type annotation.

Static analysis correctly flags this: PEP 484 prohibits implicit Optional. The default None value requires an explicit optional type annotation.

Proposed fix
     `@modify`
-    def add_rule(self, backend: "Backend", *route_matches: RouteMatch, filters: list[URLRewriteFilter] = None):
+    def add_rule(self, backend: "Backend", *route_matches: RouteMatch, filters: list[URLRewriteFilter] | None = None):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/gateway/gateway_api/route.py` at line 93, The parameter type for
filters in add_rule is implicitly Optional but currently annotated as
list[URLRewriteFilter] with default None; update the add_rule method signature
(def add_rule(self, backend: "Backend", *route_matches: RouteMatch, filters:
list[URLRewriteFilter] = None):) to explicitly allow None by changing the type
to list[URLRewriteFilter] | None (or Optional equivalent) so the default None
matches the annotation and fixes the static-analysis warning; ensure any
internal code that uses filters still handles the None case.
testsuite/kubernetes/istio/service_entry.py (1)

19-19: Use explicit | None type annotation.

Static analysis correctly flags this: PEP 484 prohibits implicit Optional.

Proposed fix
-        labels: dict[str, str] = None,
+        labels: dict[str, str] | None = None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/kubernetes/istio/service_entry.py` at line 19, Change the parameter
annotation for labels to an explicit optional type instead of relying on a bare
default None: update the parameter declaration where labels: dict[str, str] =
None to use the union form labels: dict[str, str] | None = None (or
typing.Optional[dict[str, str]] if using older syntax), e.g., in the
function/class that declares the labels parameter in service_entry.py; this
makes the Optional explicit for static analysis.
testsuite/kubernetes/istio/destination_rule.py (1)

16-19: Use explicit | None type annotations.

Static analysis correctly flags these: PEP 484 prohibits implicit Optional. All parameters with = None defaults require explicit optional type annotations.

Proposed fix
     `@classmethod`
     def create_instance(
         cls,
         cluster: KubernetesClient,
         name: str,
         host: str,
-        tls_mode: str = None,
-        sni: str = None,
-        credential_name: str = None,
-        labels: dict[str, str] = None,
+        tls_mode: str | None = None,
+        sni: str | None = None,
+        credential_name: str | None = None,
+        labels: dict[str, str] | None = None,
     ):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/kubernetes/istio/destination_rule.py` around lines 16 - 19, Update
the parameter type annotations to use explicit union-with-None annotations
instead of implicit Optional: change tls_mode: str = None to tls_mode: str |
None = None, sni: str = None to sni: str | None = None, credential_name: str =
None to credential_name: str | None = None, and labels: dict[str, str] = None to
labels: dict[str, str] | None = None (leave defaults as = None). Locate these
parameters where they are defined (tls_mode, sni, credential_name, labels) and
apply the new annotations.
testsuite/tests/singlecluster/test_egress.py (1)

135-142: Test logic is correct but consider adding a boundary verification.

The test correctly verifies:

  1. Unauthenticated requests return 401
  2. Authenticated requests within limit return 200
  3. Authenticated requests exceeding limit return 429

Consider adding a brief time.sleep() or retry mechanism before the final rate-limited request to account for any timing variability, though this may not be necessary if the rate limiter is synchronous.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/tests/singlecluster/test_egress.py` around lines 135 - 142, The
test test_egress should add a small boundary check to avoid flaky timing races:
before the final assertion that the authenticated request returns 429, pause
briefly (e.g. time.sleep(0.1)) or perform a short retry loop (attempt
client.get("/get", auth=auth) a few times with tiny sleeps) to ensure the rate
limiter window has advanced to the expected state; update the test_egress
function (around the client.get_many("/get", LIMIT.limit - 1, auth=auth) and the
subsequent assert client.get("/get", auth=auth).status_code == 429) to include
this sleep/retry so the final rate-limited request reliably observes the limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@testsuite/gateway/gateway_api/route.py`:
- Line 93: The parameter type for filters in add_rule is implicitly Optional but
currently annotated as list[URLRewriteFilter] with default None; update the
add_rule method signature (def add_rule(self, backend: "Backend",
*route_matches: RouteMatch, filters: list[URLRewriteFilter] = None):) to
explicitly allow None by changing the type to list[URLRewriteFilter] | None (or
Optional equivalent) so the default None matches the annotation and fixes the
static-analysis warning; ensure any internal code that uses filters still
handles the None case.

In `@testsuite/kubernetes/istio/destination_rule.py`:
- Around line 16-19: Update the parameter type annotations to use explicit
union-with-None annotations instead of implicit Optional: change tls_mode: str =
None to tls_mode: str | None = None, sni: str = None to sni: str | None = None,
credential_name: str = None to credential_name: str | None = None, and labels:
dict[str, str] = None to labels: dict[str, str] | None = None (leave defaults as
= None). Locate these parameters where they are defined (tls_mode, sni,
credential_name, labels) and apply the new annotations.

In `@testsuite/kubernetes/istio/service_entry.py`:
- Line 19: Change the parameter annotation for labels to an explicit optional
type instead of relying on a bare default None: update the parameter declaration
where labels: dict[str, str] = None to use the union form labels: dict[str, str]
| None = None (or typing.Optional[dict[str, str]] if using older syntax), e.g.,
in the function/class that declares the labels parameter in service_entry.py;
this makes the Optional explicit for static analysis.

In `@testsuite/tests/singlecluster/test_egress.py`:
- Around line 135-142: The test test_egress should add a small boundary check to
avoid flaky timing races: before the final assertion that the authenticated
request returns 429, pause briefly (e.g. time.sleep(0.1)) or perform a short
retry loop (attempt client.get("/get", auth=auth) a few times with tiny sleeps)
to ensure the rate limiter window has advanced to the expected state; update the
test_egress function (around the client.get_many("/get", LIMIT.limit - 1,
auth=auth) and the subsequent assert client.get("/get", auth=auth).status_code
== 429) to include this sleep/retry so the final rate-limited request reliably
observes the limit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b7a62e86-e0e9-4c79-8647-361138daf1be

📥 Commits

Reviewing files that changed from the base of the PR and between 1aee51b and a14ab05.

📒 Files selected for processing (7)
  • testsuite/backend/httpbin.py
  • testsuite/gateway/__init__.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/kubernetes/istio/__init__.py
  • testsuite/kubernetes/istio/destination_rule.py
  • testsuite/kubernetes/istio/service_entry.py
  • testsuite/tests/singlecluster/test_egress.py

Copy link
Copy Markdown

@maksymvavilov maksymvavilov left a comment

Choose a reason for hiding this comment

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

Looks good 🤷
Two questions that are just that - questions to help me understand it better.

Comment thread testsuite/tests/singlecluster/test_egress.py Outdated
Comment thread testsuite/tests/singlecluster/test_egress.py
@averevki averevki moved this from Ready For Review to In Review in Kuadrant Apr 8, 2026
@averevki averevki force-pushed the egress-gateway-both-policies branch from a14ab05 to 5b2fdfe Compare April 8, 2026 10:29
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: 2

🧹 Nitpick comments (1)
testsuite/tests/singlecluster/test_egress.py (1)

118-118: Use a limit key that matches the configured window.

"3_20s" is misleading because LIMIT is 3 requests per 5s. Renaming the key improves test readability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/tests/singlecluster/test_egress.py` at line 118, The test uses a
misleading limit key string—update the call to rate_limit.add_limit so the key
reflects the actual window used by LIMIT (3 requests per 5s); change the key
passed to rate_limit.add_limit from "3_20s" to something matching the configured
window (e.g., "3_5s" or "3_per_5s") so the entry for
rate_limit.add_limit("3_20s", [LIMIT]) accurately documents LIMIT's 3-per-5s
restriction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@testsuite/tests/singlecluster/test_egress.py`:
- Around line 103-105: The fixture returns immediately after calling
route.commit(), which can race reconciliation; modify the fixture around
route.commit() so after committing you wait for the HTTPRoute to become ready
before returning and before the finalizer runs (e.g., call a readiness helper
like route.wait_for_ready() or wait_for_condition(route, "ready") after
route.commit() and only then return route), keeping
request.addfinalizer(route.delete) in place to ensure cleanup.
- Line 146: Replace the brittle sleep(5) call that aligns exactly with the 5s
rate-limit window with a non-boundary wait (e.g., sleep(5.1) or sleep(5.5)) so
the test doesn't race the quota reset; locate the literal sleep(5) in
testsuite/tests/singlecluster/test_egress.py and adjust it to sleep for a small
margin longer than 5s (or implement a short polling loop checking the
quota/reset condition instead of a fixed 5s sleep).

---

Nitpick comments:
In `@testsuite/tests/singlecluster/test_egress.py`:
- Line 118: The test uses a misleading limit key string—update the call to
rate_limit.add_limit so the key reflects the actual window used by LIMIT (3
requests per 5s); change the key passed to rate_limit.add_limit from "3_20s" to
something matching the configured window (e.g., "3_5s" or "3_per_5s") so the
entry for rate_limit.add_limit("3_20s", [LIMIT]) accurately documents LIMIT's
3-per-5s restriction.
🪄 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: db697301-6751-45e7-8482-0cb03fdd6f16

📥 Commits

Reviewing files that changed from the base of the PR and between a14ab05 and 5b2fdfe.

📒 Files selected for processing (7)
  • testsuite/backend/httpbin.py
  • testsuite/gateway/__init__.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/kubernetes/istio/__init__.py
  • testsuite/kubernetes/istio/destination_rule.py
  • testsuite/kubernetes/istio/service_entry.py
  • testsuite/tests/singlecluster/test_egress.py
✅ Files skipped from review due to trivial changes (1)
  • testsuite/backend/httpbin.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • testsuite/gateway/init.py

Comment thread testsuite/tests/singlecluster/test_egress.py
Comment thread testsuite/tests/singlecluster/test_egress.py Outdated
@averevki averevki force-pushed the egress-gateway-both-policies branch from 5b2fdfe to 3ff3cfb Compare April 8, 2026 10:41
@averevki
Copy link
Copy Markdown
Contributor Author

averevki commented Apr 8, 2026

I split the single basic egress test to authpolicy and ratelimit egress tests, now it should be easier to determine what policy experiencing an issue based on the according test failing.

@averevki averevki force-pushed the egress-gateway-both-policies branch from 9d5cc31 to 3ff3cfb Compare April 8, 2026 15:32
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

🧹 Nitpick comments (2)
testsuite/kuadrant/policy/authorization/sections.py (1)

196-215: Consider explicit Optional type hints for default None parameters.

The credentials and shared_secret_ref parameters use implicit Optional (default None without type annotation). PEP 484 recommends explicit typing for clarity.

♻️ Suggested type annotation fix
     `@modify`
     def add_http(
         self,
         name,
         endpoint,
         method: Literal["GET", "POST"],
-        credentials: Credentials = None,
-        shared_secret_ref: dict[str, str] = None,
+        credentials: Credentials | None = None,
+        shared_secret_ref: dict[str, str] | None = None,
         **common_features,
     ):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/kuadrant/policy/authorization/sections.py` around lines 196 - 215,
The add_http method currently uses implicit Optional defaults for credentials
and shared_secret_ref; update the signature to use explicit typing: change
credentials: Credentials = None to credentials: Optional[Credentials] = None and
change shared_secret_ref: dict[str, str] = None to shared_secret_ref:
Optional[dict[str, str]] = None, and add the Optional import from typing at the
top of the module. Keep the rest of add_http (http_config construction, asdict
usage, sharedSecretRef assignment, and self.add_item call) unchanged.
testsuite/tests/singlecluster/egress/test_egress.py (1)

78-85: Rate limit test may be flaky due to timing assumptions.

The test uses sleep(6) to wait for quota reset, but LIMIT is defined as Limit(3, "5s"). The 6-second sleep should be sufficient, but this approach is inherently timing-sensitive. Consider adding a small buffer or documenting why 6 seconds was chosen (5s window + 1s buffer).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/tests/singlecluster/egress/test_egress.py` around lines 78 - 85,
The test test_egress_ratelimit is using a fragile fixed sleep(6) to wait for the
rate-limit window defined by LIMIT (Limit(3, "5s")); update the wait to derive
the sleep time from LIMIT plus a small buffer instead of a hardcoded 6s (e.g.,
compute the window duration from LIMIT (LIMIT.window or LIMIT.window_seconds)
and call sleep(window_seconds + 1) or otherwise add a 1–2s buffer), and add a
brief comment explaining the buffer to avoid flaky timing assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py`:
- Around line 54-69: The route fixture creates and commits an HTTPRoute but
doesn't wait for reconciliation; update the route fixture (defined around
HTTPRoute.create_instance and route.commit) to follow the lifecycle pattern by
calling route.wait_for_ready() after route.commit() and before returning (and
keep request.addfinalizer(route.delete) as-is) so tests don't race with an
unreconciled HTTPRoute.
- Around line 72-87: The route2 fixture does not call wait_for_ready() after
committing the HTTPRoute; update the fixture so after route.commit() you call
route.wait_for_ready() (keeping request.addfinalizer(route.delete) in place) to
follow the lifecycle pattern used by the route fixture—i.e., create via
HTTPRoute.create_instance, configure add_hostname/add_rule, commit via
route.commit(), then call route.wait_for_ready() before returning the route.

In `@testsuite/tests/singlecluster/egress/test_egress.py`:
- Around line 50-61: Add a module-scoped fixture named commit that actually
applies and waits for the configured policies to be enforced: call the shared
commit operation (or explicitly call authorization.commit() and
rate_limit.commit() if available) and then wait for readiness (same pattern used
in test_credential_injection.py and
test_credential_injection_by_destination.py), so the authorization and
rate_limit fixtures take effect before tests run.

---

Nitpick comments:
In `@testsuite/kuadrant/policy/authorization/sections.py`:
- Around line 196-215: The add_http method currently uses implicit Optional
defaults for credentials and shared_secret_ref; update the signature to use
explicit typing: change credentials: Credentials = None to credentials:
Optional[Credentials] = None and change shared_secret_ref: dict[str, str] = None
to shared_secret_ref: Optional[dict[str, str]] = None, and add the Optional
import from typing at the top of the module. Keep the rest of add_http
(http_config construction, asdict usage, sharedSecretRef assignment, and
self.add_item call) unchanged.

In `@testsuite/tests/singlecluster/egress/test_egress.py`:
- Around line 78-85: The test test_egress_ratelimit is using a fragile fixed
sleep(6) to wait for the rate-limit window defined by LIMIT (Limit(3, "5s"));
update the wait to derive the sleep time from LIMIT plus a small buffer instead
of a hardcoded 6s (e.g., compute the window duration from LIMIT (LIMIT.window or
LIMIT.window_seconds) and call sleep(window_seconds + 1) or otherwise add a 1–2s
buffer), and add a brief comment explaining the buffer to avoid flaky timing
assumptions.
🪄 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: b9572ad8-718e-40e7-a94c-4fa415ffe354

📥 Commits

Reviewing files that changed from the base of the PR and between 3ff3cfb and 9d5cc31.

📒 Files selected for processing (9)
  • testsuite/gateway/__init__.py
  • testsuite/kuadrant/policy/authorization/sections.py
  • testsuite/tests/singlecluster/egress/__init__.py
  • testsuite/tests/singlecluster/egress/conftest.py
  • testsuite/tests/singlecluster/egress/credentials_injection/__init__.py
  • testsuite/tests/singlecluster/egress/credentials_injection/conftest.py
  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py
  • testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py
  • testsuite/tests/singlecluster/egress/test_egress.py

Comment thread testsuite/tests/singlecluster/test_egress.py
@averevki averevki force-pushed the egress-gateway-both-policies branch from 3ff3cfb to 889aafe Compare April 8, 2026 17:53
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.

🧹 Nitpick comments (3)
testsuite/gateway/gateway_api/route.py (1)

93-93: Use explicit Optional type annotation.

The default None requires an explicit union type to comply with PEP 484.

Suggested fix
-    def add_rule(self, backend: "Backend", *route_matches: RouteMatch, filters: list[URLRewriteFilter] = None):
+    def add_rule(self, backend: "Backend", *route_matches: RouteMatch, filters: list[URLRewriteFilter] | None = None):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/gateway/gateway_api/route.py` at line 93, The signature for
add_rule uses a default None for filters but lacks an explicit Optional union;
update the parameter annotation in add_rule (function name: add_rule, parameter:
filters) to use typing.Optional (e.g. Optional[list[URLRewriteFilter]]) and
ensure Optional is imported from typing at the top of the module so the None
default is type-correct per PEP 484.
testsuite/kubernetes/istio/destination_rule.py (1)

16-19: Use explicit Optional type annotations.

Parameters with None defaults require explicit union types per PEP 484.

Suggested fix
-        tls_mode: str = None,
-        sni: str = None,
-        credential_name: str = None,
-        labels: dict[str, str] = None,
+        tls_mode: str | None = None,
+        sni: str | None = None,
+        credential_name: str | None = None,
+        labels: dict[str, str] | None = None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/kubernetes/istio/destination_rule.py` around lines 16 - 19, Update
the function/class signature so parameters with default None use explicit
Optional types per PEP 484: change tls_mode, sni, credential_name to
Optional[str] and labels to Optional[dict[str, str]] and add the required import
(from typing import Optional) at the top of the module; locate and update the
signature that currently lists tls_mode: str = None, sni: str = None,
credential_name: str = None, labels: dict[str, str] = None to use the
Optional[...] annotations.
testsuite/kubernetes/istio/service_entry.py (1)

19-19: Use explicit Optional type annotation.

The default None requires an explicit union type per PEP 484.

Suggested fix
-        labels: dict[str, str] = None,
+        labels: dict[str, str] | None = None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsuite/kubernetes/istio/service_entry.py` at line 19, The parameter
annotation for labels currently uses a bare default None with type dict[str,
str]; change it to an explicit optional type like Optional[dict[str, str]] (or
dict[str, str] | None if using Python 3.10+), and ensure Optional is imported
from typing if needed; update the signature where labels is declared (labels:
dict[str, str] = None) to use the explicit union and run linters/type-checker to
confirm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@testsuite/gateway/gateway_api/route.py`:
- Line 93: The signature for add_rule uses a default None for filters but lacks
an explicit Optional union; update the parameter annotation in add_rule
(function name: add_rule, parameter: filters) to use typing.Optional (e.g.
Optional[list[URLRewriteFilter]]) and ensure Optional is imported from typing at
the top of the module so the None default is type-correct per PEP 484.

In `@testsuite/kubernetes/istio/destination_rule.py`:
- Around line 16-19: Update the function/class signature so parameters with
default None use explicit Optional types per PEP 484: change tls_mode, sni,
credential_name to Optional[str] and labels to Optional[dict[str, str]] and add
the required import (from typing import Optional) at the top of the module;
locate and update the signature that currently lists tls_mode: str = None, sni:
str = None, credential_name: str = None, labels: dict[str, str] = None to use
the Optional[...] annotations.

In `@testsuite/kubernetes/istio/service_entry.py`:
- Line 19: The parameter annotation for labels currently uses a bare default
None with type dict[str, str]; change it to an explicit optional type like
Optional[dict[str, str]] (or dict[str, str] | None if using Python 3.10+), and
ensure Optional is imported from typing if needed; update the signature where
labels is declared (labels: dict[str, str] = None) to use the explicit union and
run linters/type-checker to confirm.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 48dd09b2-0b57-4a48-bd93-512568458cbb

📥 Commits

Reviewing files that changed from the base of the PR and between 9d5cc31 and 889aafe.

📒 Files selected for processing (7)
  • testsuite/backend/httpbin.py
  • testsuite/gateway/__init__.py
  • testsuite/gateway/gateway_api/route.py
  • testsuite/kubernetes/istio/__init__.py
  • testsuite/kubernetes/istio/destination_rule.py
  • testsuite/kubernetes/istio/service_entry.py
  • testsuite/tests/singlecluster/test_egress.py
✅ Files skipped from review due to trivial changes (1)
  • testsuite/backend/httpbin.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • testsuite/gateway/init.py

Comment thread testsuite/tests/singlecluster/test_egress.py
Comment thread testsuite/backend/httpbin.py
Comment thread testsuite/tests/singlecluster/test_egress.py Outdated
@averevki averevki force-pushed the egress-gateway-both-policies branch 2 times, most recently from 50c4a89 to d2f3f47 Compare April 13, 2026 14:17
Signed-off-by: averevki <sandyverevkin@gmail.com>
@averevki averevki force-pushed the egress-gateway-both-policies branch from d2f3f47 to dec02b2 Compare April 13, 2026 14:24
Copy link
Copy Markdown
Contributor

@silvi-t silvi-t left a comment

Choose a reason for hiding this comment

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

LGTM

@averevki averevki merged commit a905ac4 into Kuadrant:main Apr 14, 2026
5 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in Kuadrant Apr 14, 2026
@averevki averevki deleted the egress-gateway-both-policies branch April 14, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Service Protection Issues for Service Protection tests Test case New test case

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Egress] AuthPolicy + RateLimitPolicy on Egress Gateway

3 participants