test: Add tests for named HTTPRouteRule sectionName targeting#943
test: Add tests for named HTTPRouteRule sectionName targeting#943Tiago-Vier-Preto wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds optional ChangesNamed HTTPRoute rule support and tests
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPRoute
participant Policy
participant Backend
Client->>HTTPRoute: HTTP request (e.g. /get)
HTTPRoute->>HTTPRoute: Match rule and identify section name (e.g. get-rule)
HTTPRoute->>Policy: Evaluate section_name targeting
alt Section matches
Policy->>Policy: Apply policy (auth or rate-limit)
alt Policy permits
HTTPRoute->>Backend: Forward request
Backend->>Client: HTTP 200
else Policy denies
Policy->>Client: HTTP 401 or 429
end
else Section doesn't match
HTTPRoute->>Backend: Forward to other rule (e.g. anything-rule)
Backend->>Client: HTTP 200
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
testsuite/tests/singlecluster/limitador/section/test_named_route_rule.py (2)
27-34: Minor: test leaves the limit counter in a 429 state.After the third
/getrequest the counter is exhausted for the remaining window (10s). If any follow-up assertion or retry is added later within the same module, it will hit unexpected 429s. Not a bug in the current test, but worth keeping in mind — you could either reorder to check/anythingfirst, or size the limit/window to tolerate a small buffer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/limitador/section/test_named_route_rule.py` around lines 27 - 34, The test_limit_match_named_route_rule test exhausts the /get rate limit and leaves the counter in a 429 state for the remainder of the window; to avoid impacting later assertions, either move the `/anything` check to run before the get_many()/additional get() calls (i.e., call client.get("/anything") first) or increase the limit/window used by the test so a single extra request won't hit 429; update the test function test_limit_match_named_route_rule to perform the non-limited assertion before exhausting the /get counter or adjust the rate-limit parameters accordingly.
22-22: Prefer keyword argument forsection_name.Passing
"get-rule"as a positional argument works today but is fragile:RateLimitPolicy.create_instancesignature is(cluster, name, target, section_name=None, labels=None), so any future reordering of kwargs would silently break this. Using a keyword argument matches the style used intest_authpolicy_named_http_route_rule.py.Proposed change
- rlp = RateLimitPolicy.create_instance(cluster, blame("limit"), route, "get-rule", labels={"testRun": module_label}) + rlp = RateLimitPolicy.create_instance( + cluster, blame("limit"), route, section_name="get-rule", labels={"testRun": module_label} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/limitador/section/test_named_route_rule.py` at line 22, The test passes the section name "get-rule" positionally to RateLimitPolicy.create_instance which has signature (cluster, name, target, section_name=None, labels=None) and is fragile; change the call to pass section_name="get-rule" as a keyword argument to match the existing style (see RateLimitPolicy.create_instance and the call site rlp = RateLimitPolicy.create_instance(...)) so future argument reordering won't break the test.testsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_named_http_route_rule.py (2)
9-9: Consider linking the related issue.Per the PR description, this test covers issue
#942. Addingpytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/942")topytestmark(here and in the limitador test) improves traceability in Report Portal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_named_http_route_rule.py` at line 9, Update the pytest marks to include the issue link by adding pytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/942") into the pytestmark list used in this test (the pytestmark variable at top of test_authpolicy_named_http_route_rule.py) and mirror the same addition in the corresponding limitador test file; ensure the new pytest.mark.issue entry is included alongside pytest.mark.authorino and pytest.mark.kuadrant_only so Report Portal can trace the test to issue `#942`.
42-57: Consider asserting policy enforcement status before sending traffic.Relying solely on HTTP responses can produce flaky failures if the policy hasn't been fully enforced yet. Other section-targeting tests in this suite typically verify
route.is_affected_by(authorization)(or equivalent readiness) before sending requests. The autousecommitfixture waits for the policy to be ready, which should cover this, but an explicitis_affected_byassertion makes the intent clearer and the diagnostic signal on failure more specific.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_named_http_route_rule.py` around lines 42 - 57, The test test_authpolicy_section_name_targeting_named_http_route_rule relies only on HTTP responses and should assert the policy is active first; add an explicit readiness check using the route object's is_affected_by(authorization) (or the equivalent helper used in other tests) before issuing requests so the test verifies that the named HTTPRoute rule is affected by the AuthPolicy section; locate the route/authorization objects used in this test and insert a short assert like route.is_affected_by(authorization) (or call the same helper used elsewhere in the suite) prior to the client.get calls to avoid flakiness.testsuite/gateway/gateway_api/route.py (1)
93-95: Nit: PEP 484 implicit Optional.Ruff RUF013 flags
name: str = None(and the same on Line 115). Prefername: str | None = Nonefor consistency with modern typing.Proposed change
- def add_rule( - self, backend: "Backend", *route_matches: RouteMatch, filters: list[URLRewriteFilter] = None, name: str = None - ): + def add_rule( + self, + backend: "Backend", + *route_matches: RouteMatch, + filters: list[URLRewriteFilter] = None, + name: str | None = None, + ):- def add_backend(self, backend: "Backend", prefix="/", name: str = None): + def add_backend(self, backend: "Backend", prefix="/", name: str | 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` around lines 93 - 95, The parameter annotations use implicit Optional by assigning None (e.g., in add_rule); update the signature(s) to explicitly allow None using union types (for example change name: str = None to name: str | None = None) and do the same for the other occurrence noted (the similar parameter on Line 115) so typing is PEP 484 / modern-typing compliant; locate the add_rule function and the other flagged function and replace the `: str = None` patterns with `: str | None = None`.
🤖 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`:
- Around line 93-95: The parameter annotations use implicit Optional by
assigning None (e.g., in add_rule); update the signature(s) to explicitly allow
None using union types (for example change name: str = None to name: str | None
= None) and do the same for the other occurrence noted (the similar parameter on
Line 115) so typing is PEP 484 / modern-typing compliant; locate the add_rule
function and the other flagged function and replace the `: str = None` patterns
with `: str | None = None`.
In
`@testsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_named_http_route_rule.py`:
- Line 9: Update the pytest marks to include the issue link by adding
pytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/942") into the
pytestmark list used in this test (the pytestmark variable at top of
test_authpolicy_named_http_route_rule.py) and mirror the same addition in the
corresponding limitador test file; ensure the new pytest.mark.issue entry is
included alongside pytest.mark.authorino and pytest.mark.kuadrant_only so Report
Portal can trace the test to issue `#942`.
- Around line 42-57: The test
test_authpolicy_section_name_targeting_named_http_route_rule relies only on HTTP
responses and should assert the policy is active first; add an explicit
readiness check using the route object's is_affected_by(authorization) (or the
equivalent helper used in other tests) before issuing requests so the test
verifies that the named HTTPRoute rule is affected by the AuthPolicy section;
locate the route/authorization objects used in this test and insert a short
assert like route.is_affected_by(authorization) (or call the same helper used
elsewhere in the suite) prior to the client.get calls to avoid flakiness.
In `@testsuite/tests/singlecluster/limitador/section/test_named_route_rule.py`:
- Around line 27-34: The test_limit_match_named_route_rule test exhausts the
/get rate limit and leaves the counter in a 429 state for the remainder of the
window; to avoid impacting later assertions, either move the `/anything` check
to run before the get_many()/additional get() calls (i.e., call
client.get("/anything") first) or increase the limit/window used by the test so
a single extra request won't hit 429; update the test function
test_limit_match_named_route_rule to perform the non-limited assertion before
exhausting the /get counter or adjust the rate-limit parameters accordingly.
- Line 22: The test passes the section name "get-rule" positionally to
RateLimitPolicy.create_instance which has signature (cluster, name, target,
section_name=None, labels=None) and is fragile; change the call to pass
section_name="get-rule" as a keyword argument to match the existing style (see
RateLimitPolicy.create_instance and the call site rlp =
RateLimitPolicy.create_instance(...)) so future argument reordering won't break
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35e43c2b-d8db-412c-9cbb-857f7564e86a
📒 Files selected for processing (3)
testsuite/gateway/gateway_api/route.pytestsuite/tests/singlecluster/gateway/authpolicy/test_authpolicy_named_http_route_rule.pytestsuite/tests/singlecluster/limitador/section/test_named_route_rule.py
Tiago-Vier-Preto
force-pushed
the
feature/add-tests-for-named-httprouterule
branch
from
7a48117 to
0b25dad
Compare
azgabur
left a comment
azgabur
left a comment
There was a problem hiding this comment.
Please run make commit-acceptance or/and make reformat and fix some formatting issues but otherwise I say LGTM, thanks for contribution!
averevki
left a comment
averevki
left a comment
There was a problem hiding this comment.
Looks great! Only left the smallest nit. Please make sure that linters and other github actions are successful so it can allow us to merge 👍
Tiago-Vier-Preto
force-pushed
the
feature/add-tests-for-named-httprouterule
branch
from
03ba389 to
78e36e1
Compare
emmaaroche
left a comment
emmaaroche
left a comment
There was a problem hiding this comment.
Hey @Tiago-Vier-Preto, thank you for your contribution! The PR is failing mypy type checking on line 118: error: Incompatible types in assignment (expression has type "str", target has type "list[Any]")
I left a suggestion of a fix, but also feel free to find your own work around. Once you make changes, run make commit-acceptance locally to verify the fix! 👍
|
Hi @Tiago-Vier-Preto, are you still working on this? |
|
Yes, I am! I'm really sorry for the delay guys! |
Signed-off-by: Tiago Vier Preto <tvpreto@gmail.com> Signed-off-by: Tiago Vier Preto <83984051+Tiago-Vier-Preto@users.noreply.github.com>
Signed-off-by: Tiago Vier Preto <83984051+Tiago-Vier-Preto@users.noreply.github.com>
Co-authored-by: Emma Roche <eroche@redhat.com> Signed-off-by: Tiago Vier Preto <83984051+Tiago-Vier-Preto@users.noreply.github.com>
Signed-off-by: Tiago Vier Preto <83984051+Tiago-Vier-Preto@users.noreply.github.com>
Tiago-Vier-Preto
force-pushed
the
feature/add-tests-for-named-httprouterule
branch
from
9070085 to
3bda78a
Compare
4 participants
Description
This PR adds support for explicitly named HTTPRoute rules in the testsuite models and introduces test coverage to verify that Kuadrant policies can correctly target these named rules via the
sectionNamefield. This aligns the testsuite with the Gateway API's introduction of explicit naming for HTTPRouteRule entries (kubernetes-sigs/gateway-api#995) and Kuadrant's policy-machinery support for it (Kuadrant/policy-machinery#63).Issue #942
Changes
HTTPRoute.add_backend`()` andHTTPRoute.add_rule()intestsuite/gateway/gateway_api/route.pyto accept an optionalnameparameter, allowing rules to be explicitly named instead of relying solely on auto-generated names.test_named_route_rule.pyin the Limitador section tests to verify that aRateLimitPolicycorrectly applies to a specific named rule (section_name="get-rule").test_authpolicy_named_http_route_rule.pyin the AuthPolicy section tests to verify that anAuthPolicycorrectly protects traffic for a specific named rule without affecting untargeted rules.Verification
test_limit_match_named_route_ruleandtest_authpolicy_section_name_targeting_named_http_route_rule) which create anHTTPRoutewith explicitly named rules (e.g.,get-ruleandanything-rule).get-rulesection name, the policy is enforced exclusively on that rule's route, while the other route remains unaffected.Summary by CodeRabbit
New Features
Tests