test(egress): add egress AuthPolicy+RateLimitPolicy test#914
test(egress): add egress AuthPolicy+RateLimitPolicy test#914averevki merged 1 commit intoKuadrant:mainfrom
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes introduce testsuite support for egress traffic policy enforcement through Istio integration. New Changes
Sequence DiagramsequenceDiagram
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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
testsuite/gateway/gateway_api/route.py (1)
93-93: Use explicit| Nonetype annotation.Static analysis correctly flags this: PEP 484 prohibits implicit
Optional. The defaultNonevalue 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| Nonetype 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| Nonetype annotations.Static analysis correctly flags these: PEP 484 prohibits implicit
Optional. All parameters with= Nonedefaults 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:
- Unauthenticated requests return 401
- Authenticated requests within limit return 200
- 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
📒 Files selected for processing (7)
testsuite/backend/httpbin.pytestsuite/gateway/__init__.pytestsuite/gateway/gateway_api/route.pytestsuite/kubernetes/istio/__init__.pytestsuite/kubernetes/istio/destination_rule.pytestsuite/kubernetes/istio/service_entry.pytestsuite/tests/singlecluster/test_egress.py
maksymvavilov
left a comment
There was a problem hiding this comment.
Looks good 🤷
Two questions that are just that - questions to help me understand it better.
a14ab05 to
5b2fdfe
Compare
There was a problem hiding this comment.
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 becauseLIMITis3requests per5s. 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
📒 Files selected for processing (7)
testsuite/backend/httpbin.pytestsuite/gateway/__init__.pytestsuite/gateway/gateway_api/route.pytestsuite/kubernetes/istio/__init__.pytestsuite/kubernetes/istio/destination_rule.pytestsuite/kubernetes/istio/service_entry.pytestsuite/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
5b2fdfe to
3ff3cfb
Compare
|
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. |
9d5cc31 to
3ff3cfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
testsuite/kuadrant/policy/authorization/sections.py (1)
196-215: Consider explicitOptionaltype hints for defaultNoneparameters.The
credentialsandshared_secret_refparameters use implicitOptional(defaultNonewithout 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, butLIMITis defined asLimit(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
📒 Files selected for processing (9)
testsuite/gateway/__init__.pytestsuite/kuadrant/policy/authorization/sections.pytestsuite/tests/singlecluster/egress/__init__.pytestsuite/tests/singlecluster/egress/conftest.pytestsuite/tests/singlecluster/egress/credentials_injection/__init__.pytestsuite/tests/singlecluster/egress/credentials_injection/conftest.pytestsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.pytestsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.pytestsuite/tests/singlecluster/egress/test_egress.py
3ff3cfb to
889aafe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
testsuite/gateway/gateway_api/route.py (1)
93-93: Use explicitOptionaltype annotation.The default
Nonerequires 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 explicitOptionaltype annotations.Parameters with
Nonedefaults 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 explicitOptionaltype annotation.The default
Nonerequires 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
📒 Files selected for processing (7)
testsuite/backend/httpbin.pytestsuite/gateway/__init__.pytestsuite/gateway/gateway_api/route.pytestsuite/kubernetes/istio/__init__.pytestsuite/kubernetes/istio/destination_rule.pytestsuite/kubernetes/istio/service_entry.pytestsuite/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
50c4a89 to
d2f3f47
Compare
Signed-off-by: averevki <sandyverevkin@gmail.com>
d2f3f47 to
dec02b2
Compare
Summary
Add E2E test for Kuadrant data plane policies on Istio egress gateway, based on the egress gateway investigation from Kuadrant/architecture#147.
kind: HostnamebackendRef and URLRewrite filterChanges
ServiceEntryKubernetesObject class (testsuite/kubernetes/istio/service_entry.py)DestinationRuleKubernetesObject class (testsuite/kubernetes/istio/destination_rule.py)URLRewriteFilterdataclass for HTTPRoute filters (testsuite/gateway/__init__.py)filtersparameter toHTTPRoute.add_rule()(testsuite/gateway/gateway_api/route.py)labelsparameter toHttpbinbackend (testsuite/backend/httpbin.py)test_egress.py— egress gateway test with OIDC auth and rate limitingVerification
Closes #912
Summary by CodeRabbit
Release Notes
New Features
DestinationRuleconfiguration with TLS origination support.ServiceEntryconfiguration for external service registration.Tests