test: add extension PipelinePolicy tests#991
Conversation
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>
📝 WalkthroughWalkthroughThis PR introduces comprehensive end-to-end testing for PipelinePolicy, an out-of-tree Kuadrant extension. It adds a Python model for PipelinePolicy with request/response action builders, shared test fixtures including threat assessment service deployment, and test modules covering basic routing, action types (deny, fail, gRPC, headers), targeting scopes, lifecycle operations, validation, and cross-policy interactions. ChangesPipelinePolicy Extension Testing Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
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>
3556628 to
ce39290
Compare
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
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
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018") |
There was a problem hiding this comment.
this one has been merged, shouldn't be failing anymore.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018") |
There was a problem hiding this comment.
issue has been merged, you could remove the xfail.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018") |
There was a problem hiding this comment.
issue has been merged, you could remove the xfail.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2023") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2023") |
There was a problem hiding this comment.
issue has been merged, you could remove the xfail.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2023") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2023") |
There was a problem hiding this comment.
issue has been merged, you could remove the xfail.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2009") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2009") |
There was a problem hiding this comment.
issue has been merged, you could remove the xfail.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2022") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2022") |
There was a problem hiding this comment.
this was an issue in the PipelinePolicy reconciler. I just pushed a fix that should validate if the targetRef exits or not before proceeding with comitting the pipeline.
|
|
||
|
|
||
| @pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2015") | ||
| @pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2015") |
There was a problem hiding this comment.
issue has been closed. should remove the xfail.
| policy.add_action_method( | ||
| name="assess", | ||
| url=svc_url, | ||
| service="threat.v1.ThreatAssessmentService", | ||
| method="AssessRequest", | ||
| message_template="threat.v1.ThreatRequest{uri: request.path}", |
There was a problem hiding this comment.
this bit is reused in a lot of tests below. Thinking maybe we could wrap this into some sort of function, maybe the whole pipelinepolicy creation, similar to how other fixtures, such as api_key are being handels.
| assert response.headers.get("x-delete-test") == "active" | ||
|
|
||
| policy.delete() | ||
| assert policy.wait_until(lambda obj: not obj.exists(), timelimit=30), "PipelinePolicy was not deleted" |
There was a problem hiding this comment.
This will not work, because obj.exists() returns a tuple. One way I managed to make it work
| assert policy.wait_until(lambda obj: not obj.exists(), timelimit=30), "PipelinePolicy was not deleted" | |
| assert not policy.committed, "PipelinePolicy was not deleted" |
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)
withBodyshould be a CEL expressionVerification steps
Run all PipelinePolicy tests sequentially:
Closes #974, #975, #976
Summary by CodeRabbit
Release Notes
New Features
Tests