test: add extension PipelinePolicy tests#991
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:
📝 WalkthroughWalkthroughAdds a PipelinePolicy SDK model, configuration, fixtures, a threat-assessment gRPC service fixture, and comprehensive integration coverage for actions, composition, targeting, lifecycle, validation, and interactions with authentication and rate limiting. ChangesPipelinePolicy testing infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PipelinePolicy
participant ThreatAssessmentService
Client->>PipelinePolicy: Send HTTP request
PipelinePolicy->>ThreatAssessmentService: Invoke configured gRPC action
ThreatAssessmentService-->>PipelinePolicy: Return threat assessment
PipelinePolicy-->>Client: Apply deny, fail, or response headers
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
3556628 to
ce39290
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.py (1)
69-73: TODO: assert the expected validation message once available.Until the message is asserted, this test only checks
Accepted=False, which could pass for an unrelated rejection reason. Tighten it when the upstream validation lands.Would you like me to open a tracking issue for adding the message assertion here?
🤖 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/extensions/pipeline_policy/test_pipeline_policy_validation.py` around lines 69 - 73, Test currently only asserts policy.wait_until(has_condition("Accepted", "False"), ...) which can match unrelated rejections; update the test to also assert the expected validation message once the upstream validation is implemented by replacing the TODO with an assertion that the policy's condition contains the exact expected message (use policy.refresh().model.status.conditions to read conditions) — for example, extend has_condition usage or add an assert that any(c.type == "Accepted" and c.status == "False" and expected_message in c.message for c in policy.refresh().model.status.conditions) where expected_message is the upstream validation text; leave a TODO/issue reference if the message is not yet available.testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.py (1)
8-22: ⚖️ Poor tradeoffOptional: consider centralising the duplicated
pipeline_policy/commitfixtures.The header-only
pipeline_policyfixture and thecommitautouse fixture are repeated (with small variations in committed components) across the three interaction modules. A shared parametrised helper ininteractions/conftest.pycould reduce drift, though the per-module differences make this a low priority.🤖 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/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.py` around lines 8 - 22, Extract the duplicated pipeline_policy fixture and the autouse commit fixture into a shared interactions/conftest.py: move the header-only pipeline_policy setup (the pipeline_policy fixture that calls pipeline_policy.on_http_response.add_headers([...]) and returns pipeline_policy) and the commit fixture (which iterates components, calls request.addfinalizer(component.delete), component.commit(), and component.wait_for_ready()) into conftest.py, and make commit parametrisable so callers can pass which components to commit (e.g., via pytest param or an injected fixture) while preserving autouse behavior where needed; update the three interaction test modules to import/rely on the shared pipeline_policy and to call the commit fixture with the appropriate component list to avoid duplication.testsuite/kuadrant/extensions/pipeline_policy.py (1)
77-83: 💤 Low valueInconsistent header serialisation between
add_headersandadd_deny.
add_headersserialises theheaderslist viastr(headers), producing a Python repr with single quotes (e.g.[['x-single', 'one']]), whereasadd_deny(with_headers=...)expects a caller-supplied double-quoted string (e.g.'[["x-deny-reason", "blocked"]]'). This only works if the extension parsesheadersToAdd/withHeadersas CEL (single quotes valid) rather than strict JSON. Consider accepting aList[List[str]]in both methods and serialising consistently to avoid surprises for future callers.Please confirm the extension accepts a Python-style single-quoted list for
headersToAdd(i.e. CEL evaluation rather than JSON parsing); the response-header tests should fail if it does not.🤖 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/kuadrant/extensions/pipeline_policy.py` around lines 77 - 83, The add_headers method currently serialises headers with Python's str(headers) producing single quotes, causing inconsistency with add_deny's expected double-quoted JSON; change add_headers (and ensure add_deny) to accept headers as a List[List[str]] and serialise them with json.dumps before placing into the action dict (keys "headersToAdd" and "withHeaders"), preserving predicate handling and the `@modify` decorator so both methods produce consistent JSON-style double-quoted strings for downstream parsing.
🤖 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.
Nitpick comments:
In `@testsuite/kuadrant/extensions/pipeline_policy.py`:
- Around line 77-83: The add_headers method currently serialises headers with
Python's str(headers) producing single quotes, causing inconsistency with
add_deny's expected double-quoted JSON; change add_headers (and ensure add_deny)
to accept headers as a List[List[str]] and serialise them with json.dumps before
placing into the action dict (keys "headersToAdd" and "withHeaders"), preserving
predicate handling and the `@modify` decorator so both methods produce consistent
JSON-style double-quoted strings for downstream parsing.
In
`@testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.py`:
- Around line 8-22: Extract the duplicated pipeline_policy fixture and the
autouse commit fixture into a shared interactions/conftest.py: move the
header-only pipeline_policy setup (the pipeline_policy fixture that calls
pipeline_policy.on_http_response.add_headers([...]) and returns pipeline_policy)
and the commit fixture (which iterates components, calls
request.addfinalizer(component.delete), component.commit(), and
component.wait_for_ready()) into conftest.py, and make commit parametrisable so
callers can pass which components to commit (e.g., via pytest param or an
injected fixture) while preserving autouse behavior where needed; update the
three interaction test modules to import/rely on the shared pipeline_policy and
to call the commit fixture with the appropriate component list to avoid
duplication.
In
`@testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.py`:
- Around line 69-73: Test currently only asserts
policy.wait_until(has_condition("Accepted", "False"), ...) which can match
unrelated rejections; update the test to also assert the expected validation
message once the upstream validation is implemented by replacing the TODO with
an assertion that the policy's condition contains the exact expected message
(use policy.refresh().model.status.conditions to read conditions) — for example,
extend has_condition usage or add an assert that any(c.type == "Accepted" and
c.status == "False" and expected_message in c.message for c in
policy.refresh().model.status.conditions) where expected_message is the upstream
validation text; leave a TODO/issue reference if the message is not yet
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d003d1c4-2f1a-4a81-ac35-7eac9f6ba6e9
📒 Files selected for processing (23)
Makefileconfig/settings.local.yaml.tplconfig/settings.yamltestsuite/kuadrant/extensions/pipeline_policy.pytestsuite/tests/singlecluster/extensions/pipeline_policy/__init__.pytestsuite/tests/singlecluster/extensions/pipeline_policy/conftest.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/__init__.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/conftest.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth_ratelimit.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_basic.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_composition.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_deny.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_fail.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_grpc.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_grpc_errors.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_isolation.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_lifecycle.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_response.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_targeting.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.pytestsuite/utils/constants.py
89a0f0f to
edb84c8
Compare
crstrn13
left a comment
There was a problem hiding this comment.
Suggestion: Extract gRPC-dependent tests into a grpc/ submodule
The gRPC service URL construction + add_action_method boilerplate is repeated across 4 files (~5 times total):
svc_url = (
f"grpc://{threat_assessment_service.name()}.{threat_assessment_service.namespace()}.svc.cluster.local:8080"
)
policy.add_action_method(
name="assess",
url=svc_url,
service="threat.v1.ThreatAssessmentService",
method="AssessRequest",
message_template="threat.v1.ThreatRequest{uri: request.path}",
)The majority of tests (basic, deny, response, isolation, lifecycle, targeting, validation, interactions) don't need the threat assessment service at all. Moving the gRPC-dependent tests into a grpc/ subdirectory avoids deploying the service (Deployment + Service) for those modules and centralizes the shared setup:
pipeline_policy/
...existing non-gRPC modules...
grpc/
__init__.py
conftest.py
test_grpc_basic.py # ← from test_pipeline_policy_grpc.py
test_grpc_errors.py # ← from test_pipeline_policy_grpc_errors.py
test_grpc_fail.py # ← from test_pipeline_policy_fail.py
test_grpc_composition.py # ← gRPC-dependent tests from test_pipeline_policy_composition.py
The grpc/conftest.py would provide:
@pytest.fixture(scope="module")
def threat_service_url(threat_assessment_service):
"""gRPC URL for the threat assessment service."""
svc = threat_assessment_service
return f"grpc://{svc.name()}.{svc.namespace()}.svc.cluster.local:8080"
@pytest.fixture(scope="module")
def pipeline_policy(pipeline_policy, threat_service_url):
"""PipelinePolicy with the threat assessment gRPC action method pre-registered."""
pipeline_policy.add_action_method(
name="assess",
url=threat_service_url,
service="threat.v1.ThreatAssessmentService",
method="AssessRequest",
message_template="threat.v1.ThreatRequest{uri: request.path}",
)
return pipeline_policyIndividual test modules then only configure their deny/fail/headers actions on top — no URL construction or add_action_method calls.
Note: test_grpc_errors.py creates its own policies with intentionally bad configurations (wrong URL, wrong service name, wrong method name), so it wouldn't use the shared pipeline_policy fixture. It would still benefit from threat_service_url for the 2 tests that point at the real service, and it overrides commit() to empty as it already does.
The 4 non-gRPC composition tests (test_first_deny_wins, test_response_action_ordering, test_empty_pipeline, test_request_only_pipeline, test_response_only_pipeline) stay in test_pipeline_policy_composition.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>
This reverts commit 468dddc. 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: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
…tests to subdirectory Signed-off-by: Silvia Tarabova <starabov@redhat.com>
…e test isolation Signed-off-by: Silvia Tarabova <starabov@redhat.com>
…top-level-fail validation tests Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
edb84c8 to
30664e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testsuite/kuadrant/extensions/pipeline_policy.py (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse implicit Optional typing for parameters that default to
None.Please use implicit optional typing (e.g.,
predicate: str = None) instead ofOptional[str]to align with the rest of the codebase.
testsuite/kuadrant/extensions/pipeline_policy.py#L40-L40: Changevar: Optional[str] = Noneandpredicate: Optional[str] = Nonetovar: str = Noneandpredicate: str = None.testsuite/kuadrant/extensions/pipeline_policy.py#L50-L56: Updatepredicate,with_status,with_headers, andwith_bodyto use implicit optional typing.testsuite/kuadrant/extensions/pipeline_policy.py#L70-L70: Changepredicate: Optional[str] = Nonetopredicate: str = None.testsuite/kuadrant/extensions/pipeline_policy.py#L78-L78: Changepredicate: Optional[str] = Nonetopredicate: str = None.testsuite/kuadrant/extensions/pipeline_policy.py#L6-L6: RemoveOptionalfrom the typing import if it is no longer required.Based on learnings, follow the project-wide convention to use implicit/unspecified Optional typing for parameters/variables that default to None (e.g.,
namespace: str = None) rather thanOptional[str].🤖 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/kuadrant/extensions/pipeline_policy.py` at line 40, Update testsuite/kuadrant/extensions/pipeline_policy.py at lines 40-40, 50-56, 70-70, and 78-78 to replace Optional[str] annotations for parameters defaulting to None—including add_grpc_method’s var and predicate, plus predicate, with_status, with_headers, and with_body—with implicit str = None typing; remove Optional from the typing import at lines 6-6 if unused.Source: Learnings
🤖 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/extensions/pipeline_policy/grpc/conftest.py`:
- Around line 11-13: Update the threat_assessment_service fixture to validate
its required test configuration section before using testconfig or deploying the
service, by calling testconfig.validators.validate with only set to the
fixture’s required section name. Keep the existing deployment flow unchanged
after validation.
In
`@testsuite/tests/singlecluster/extensions/pipeline_policy/grpc/test_grpc_composition.py`:
- Line 27: Update the deny predicate in the policy setup to target the `/admin`
request path used by the test, while leaving the fail action and request flow
unchanged so both predicates match and action precedence is exercised.
---
Nitpick comments:
In `@testsuite/kuadrant/extensions/pipeline_policy.py`:
- Line 40: Update testsuite/kuadrant/extensions/pipeline_policy.py at lines
40-40, 50-56, 70-70, and 78-78 to replace Optional[str] annotations for
parameters defaulting to None—including add_grpc_method’s var and predicate,
plus predicate, with_status, with_headers, and with_body—with implicit str =
None typing; remove Optional from the typing import at lines 6-6 if unused.
🪄 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: 33a2acc4-36ab-42e5-974e-9eb3d7574b10
📒 Files selected for processing (26)
Makefileconfig/settings.local.yaml.tplconfig/settings.yamltestsuite/kuadrant/extensions/pipeline_policy.pytestsuite/tests/singlecluster/extensions/pipeline_policy/__init__.pytestsuite/tests/singlecluster/extensions/pipeline_policy/conftest.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/__init__.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/conftest.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/test_grpc_basic.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/test_grpc_composition.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/test_grpc_errors.pytestsuite/tests/singlecluster/extensions/pipeline_policy/grpc/test_grpc_fail.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/__init__.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/conftest.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth_ratelimit.pytestsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_basic.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_composition.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_deny.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_isolation.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_lifecycle.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_response.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_targeting.pytestsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.pytestsuite/utils/constants.py
🚧 Files skipped from review as they are similar to previous changes (15)
- config/settings.yaml
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_basic.py
- testsuite/utils/constants.py
- config/settings.local.yaml.tpl
- testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth_ratelimit.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/conftest.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_ratelimit.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_targeting.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_lifecycle.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_deny.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_response.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.py
- testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_isolation.py
- Makefile
30664e5 to
c891624
Compare
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
c891624 to
581dd53
Compare
crstrn13
left a comment
There was a problem hiding this comment.
All 18 PipelinePolicy.create_instance() calls are missing labels={"testRun": module_label}. Without this label, make clean cannot find and delete orphaned PipelinePolicies from interrupted test runs. Every other policy type in the testsuite passes this label — PipelinePolicy should too.
See inline comments for each location with suggested fixes.
crstrn13
left a comment
There was a problem hiding this comment.
Hardcoded gRPC port 8080 in three places in grpc/conftest.py — should use HTTP_API_PORT from testsuite.utils.constants for consistency with the magic-number cleanup done across the rest of this PR.
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
f39c8b9 to
87c7722
Compare
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
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.
Description
PipelinePolicyclient implementation for creating and managing PipelinePolicy CRDspipeline_policy_extension_serviceimage configuration for the ThreatAssessmentService gRPC backendxfaillinked to upstream issues for policy isolation, deletion reconciliation, top-level fail action, deny with CEL body, and auth+deny timeoutChanges
New: PipelinePolicy Client
testsuite/kuadrant/extensions/pipeline_policy.py—PipelinePolicyclass with methods for deny, fail, response headers, gRPC method actions, and action method definitionsNew: Test Infrastructure
conftest.py— shared fixtures:pipeline_policy,threat_assessment_service, CRD existence check (check_pipeline_policy_crd)interactions/conftest.py— fixtures for AuthPolicy/RateLimitPolicy interaction tests:authorization,auth,rate_limitconfig/settings.yaml/config/settings.local.yaml.tpl— addedpipeline_policy_extension_service.imagesettingtestsuite/utils/constants.py— addedTHREAT_ASSESSMENT_THRESHOLDandEXTENSION_POLICY_PROPAGATION_WAITKnown Issues (xfail) - xfails has been removed; these issues no longer exist
withBodyshould be a CEL expressionVerification steps
Run all PipelinePolicy tests sequentially:
Closes #974, #975, #976
Summary by CodeRabbit
New Features
pipeline_policy_extension_serviceconfiguration with a default image, plus an optional commented local override.Tests
Chores