Burn legacy config - #412
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR replaces specialised authentication and rate-limit services with dynamic gRPC services. It unifies action configuration, removes legacy request-data and token-usage paths, updates pipeline compilation, and migrates tests, examples, and documentation. ChangesUnified dynamic gRPC action migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RequestContext
participant PipelineFactory
participant Blueprint
participant DynamicService
RequestContext->>PipelineFactory: Create tasks
PipelineFactory->>Blueprint: Compile unified actions
Blueprint->>DynamicService: Invoke configured gRPC method
DynamicService-->>Blueprint: Return gRPC response
Blueprint-->>RequestContext: Apply reply actions
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
0b7e4ab to
1193aca
Compare
0b7a5da to
2d86c85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
116-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
executionfield.The common fields table does not list
execution. This PR's own examples use"execution": "sequential"(see e2e/basic/envoy.yaml and examples/ratelimit_check_report/envoy.yaml). TheConfigActionstruct incrates/kuadrant-filter/src/kuadrant/pipeline/blueprint.rsconfirmsexecutionis a real field with a default value.Add a row for
executionto this table. State the accepted values (for exampleparallel,sequential) and the default behaviour.🤖 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 `@README.md` around lines 116 - 124, Add an `execution` row to the common typed-action fields table in README.md, documenting the accepted values `parallel` and `sequential` and the field’s default behavior. Keep the existing table entries unchanged and align the description with the default defined by `ConfigAction`.
🧹 Nitpick comments (10)
crates/wasm-shim/tests/auth.rs (2)
578-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the service key with the other tests.
This test names the service
auth, whileconfig()and the other migrated tests name itauthorino. The endpoint staysauthorino-clusterin both. Useauthorinohere for consistency.🤖 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 `@crates/wasm-shim/tests/auth.rs` around lines 578 - 587, Update the service key in the test configuration used by auth_check_request_cel from "auth" to "authorino", preserving the existing endpoint and all other configuration values.
204-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the auth gRPC action block into a shared test helper. The migration copies the same gRPC action and the same five
onReplyhandlers into every test in this file. The shared root cause is a missing builder for the auth action block, next toauth_check_request_celincrates/wasm-shim/tests/util/common.rs. The same blocks also appear incrates/wasm-shim/tests/multi.rs, so a shared helper pays back across files.Also rename
it_passes_request_data. TherequestDataconfiguration no longer exists. The test now verifies that filter metadata reaches theCheckRequestmessage.
crates/wasm-shim/tests/auth.rs#L204-L331: replace the inlineauth_msgwith a call to an extendedauth_check_request_celthat accepts themetadata_contextfragment, and build the action from the shared helper. Rename the test, for example toit_passes_filter_metadata.crates/wasm-shim/tests/auth.rs#L598-L689: build both sequential actions from the shared helper, and pass"execution": "sequential"as a parameter instead of repeating the two identical blocks.🤖 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 `@crates/wasm-shim/tests/auth.rs` around lines 204 - 331, The duplicated auth gRPC action and onReply handlers need a shared builder. In crates/wasm-shim/tests/util/common.rs, extend auth_check_request_cel to accept a metadata_context fragment and add a helper for constructing the auth action block with configurable execution; in crates/wasm-shim/tests/auth.rs lines 204-331, use both helpers, rename it_passes_request_data to it_passes_filter_metadata, and replace the inline auth message; in crates/wasm-shim/tests/auth.rs lines 598-689, build both sequential actions through the shared helper and pass execution as "sequential".crates/wasm-shim/tests/util/common.rs (1)
18-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
serde_jsonfor CEL JSON escaping.
json_escape_celonly replaces"after collapsing whitespace, so a literal backslash in CEL text is emitted as invalid JSON. Since this helper builds strings that are later embedded as JSON values, delegate escaping toserde_json::to_stringand avoid manual replacements.🤖 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 `@crates/wasm-shim/tests/util/common.rs` around lines 18 - 23, Update json_escape_cel to delegate CEL string escaping to serde_json::to_string instead of collapsing whitespace and manually replacing quotes, ensuring backslashes and other JSON-special characters are escaped correctly while preserving the helper’s returned string contract.crates/kuadrant-filter/src/configuration.rs (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
executionis accepted but ignored insideonReply.
on_replynow holds fullActionvalues, so each nested action deserialises anexecutionfield.Blueprint::compileonly callscompute_dependenciesfor the top-levelconfig.actions; nested reply actions are compiled throughAction::compileand always receivedependencies: vec![]. A user can set"execution": "sequential"on a reply action and get no effect and no warning.Consider documenting this limitation, or rejecting a non-default
executioninsideon_replyduring compilation.🤖 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 `@crates/kuadrant-filter/src/configuration.rs` at line 55, Update Blueprint::compile to validate nested actions in on_reply and reject any non-default execution value before Action::compile runs; ensure the error identifies the unsupported reply action and preserves default execution behavior.README.md (1)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
requestDatadocumentation row.
PluginConfigurationdoes not definerequestData, and the configuration deserialisation ignores it, so the top-level field table should not advertise this field as supported.🤖 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 `@README.md` at line 76, Remove the `requestData` row from the top-level configuration field table in README.md, since PluginConfiguration does not support or deserialize this field.crates/wasm-shim/tests/failuremode.rs (1)
70-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two actions share the same
varname.Both gRPC actions write to
ratelimit_response. The test relies on the second action overwriting the value from the first, failed, call. That behaviour is correct for this scenario, but the shared variable name hides which reply theonReplypredicates evaluate. Consider distinct names, for exampleratelimit_response_aandratelimit_response_b, to make the assertion intent explicit.🤖 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 `@crates/wasm-shim/tests/failuremode.rs` around lines 70 - 133, Use distinct response variable names for the two gRPC actions in the test configuration, such as ratelimit_response_a for limitador-unreachable and ratelimit_response_b for limitador. Update each action’s onReply predicates, headers, and denyWith references consistently so the test still evaluates the second call’s response explicitly.crates/wasm-shim/tests/rate_limited.rs (2)
286-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant outer
string()call.
string(3 + 2)already produces"5". The outerstring()adds no effect and makes the expected byte comment at line 407 harder to follow.♻️ Proposed simplification
- value: string(string(3 + 2)) + value: string(3 + 2)🤖 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 `@crates/wasm-shim/tests/rate_limited.rs` at line 286, In the test fixture’s value expression, remove the redundant outer string conversion around string(3 + 2), while preserving the resulting "5" value and its expected byte representation.
139-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the standard rate-limit gRPC action into
tests/util/common.rs. The same dynamic service block and the same three-branchonReplychain (deny onoverall_code == 2, response headers onoverall_code == 1, fail otherwise) are now repeated more than fifteen times across five test files. The shared root cause is that no helper exists for it, so every migrated test inlines the full JSON. Any later change to the deny body, the header target, or the failure message must be applied in every copy. Add helpers next to the existingjson_escape_celandauth_check_request_cel, for exampledynamic_service_json(name, endpoint, failure_mode, grpc_service, grpc_method)andratelimit_grpc_action_json(var, service, predicate, execution, message).
crates/wasm-shim/tests/rate_limited.rs#L139-L169: replace the inline action with the new helper in all five tests in this file.crates/wasm-shim/tests/failuremode.rs#L70-L133: build both sequential actions from the helper, passing the differing service names.crates/wasm-shim/tests/failures.rs#L62-L92: build the action from the helper in all four tests in this file.crates/wasm-shim/tests/remote_address.rs#L65-L98: build the single action from the helper.crates/wasm-shim/tests/multi.rs#L106-L136: build the rate-limit action from the helper, and add a matching helper for the auth action.🤖 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 `@crates/wasm-shim/tests/rate_limited.rs` around lines 139 - 169, The standard rate-limit gRPC action is duplicated instead of being generated by shared test helpers. Add the requested dynamic-service and rate-limit action helpers beside json_escape_cel and auth_check_request_cel in crates/wasm-shim/tests/util/common.rs, preserving the shared deny, header, and failure branches; then replace the inline JSON in all five tests at crates/wasm-shim/tests/rate_limited.rs#L139-L169, both sequential actions at crates/wasm-shim/tests/failuremode.rs#L70-L133, all four actions at crates/wasm-shim/tests/failures.rs#L62-L92, the single action at crates/wasm-shim/tests/remote_address.rs#L65-L98, and the rate-limit action at crates/wasm-shim/tests/multi.rs#L106-L136. Also add and use the matching auth-action helper in multi.rs.crates/wasm-shim/tests/response_body.rs (1)
627-639: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe expected log chain documents the root cause well.
The sequence records the JSON parse failure, the failed
kuadrant.internal.response.bodystore, and the resultingUndeclaredReference("kuadrant")in the message builder. That chain proves the message-builder error follows from the store failure rather than from a separate defect.One caution: the assertions on
Task failed: "0"andTask failed: "1"bind the test to the internal task numbering. If blueprint compilation reorders tasks, this test fails for an unrelated reason. Consider asserting only the two descriptive error messages.🤖 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 `@crates/wasm-shim/tests/response_body.rs` around lines 627 - 639, Update the log assertions in the response-body test to remove the order-dependent “Task failed: "0"” and “Task failed: "1"” checks, while preserving the descriptive JSON parse, body-store, and message-builder error assertions.crates/wasm-shim/tests/multi.rs (1)
412-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameterise
config()so both tests stop duplicating the auth configuration. The newconfig()helper at lines 11-141 captures the dynamic service block and the five-step authonReplychain, but the two later tests re-declare the whole configuration inline. The shared root cause is thatconfig()takes no parameters, so a test that needs a different rate-limit predicate must copy everything.
crates/wasm-shim/tests/multi.rs#L412-L452: changeconfig()to accept the rate-limit predicate and the rate-limit message template, then call it here with the source-address predicate and template.crates/wasm-shim/tests/multi.rs#L671-L705: call the parameterisedconfig()with the group-membership predicate and the group descriptor template.🤖 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 `@crates/wasm-shim/tests/multi.rs` around lines 412 - 452, Parameterize the shared config() helper with the rate-limit predicate and message template, preserving its existing dynamic service block and auth onReply chain. In crates/wasm-shim/tests/multi.rs lines 412-452, replace the duplicated inline configuration with config() using the source-address predicate and template; in lines 671-705, call config() with the group-membership predicate and group descriptor template.
🤖 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 `@e2e/basic/README.md`:
- Around line 19-30: Update the actions JSON example in the README to match the
migrated actions configuration in e2e/basic/envoy.yaml: use the grpc type and
its var, messageBuilder, and onReply fields instead of the legacy service,
scope, conditionalData, data, and expression schema, while preserving the
example’s intended behavior.
---
Outside diff comments:
In `@README.md`:
- Around line 116-124: Add an `execution` row to the common typed-action fields
table in README.md, documenting the accepted values `parallel` and `sequential`
and the field’s default behavior. Keep the existing table entries unchanged and
align the description with the default defined by `ConfigAction`.
---
Nitpick comments:
In `@crates/kuadrant-filter/src/configuration.rs`:
- Line 55: Update Blueprint::compile to validate nested actions in on_reply and
reject any non-default execution value before Action::compile runs; ensure the
error identifies the unsupported reply action and preserves default execution
behavior.
In `@crates/wasm-shim/tests/auth.rs`:
- Around line 578-587: Update the service key in the test configuration used by
auth_check_request_cel from "auth" to "authorino", preserving the existing
endpoint and all other configuration values.
- Around line 204-331: The duplicated auth gRPC action and onReply handlers need
a shared builder. In crates/wasm-shim/tests/util/common.rs, extend
auth_check_request_cel to accept a metadata_context fragment and add a helper
for constructing the auth action block with configurable execution; in
crates/wasm-shim/tests/auth.rs lines 204-331, use both helpers, rename
it_passes_request_data to it_passes_filter_metadata, and replace the inline auth
message; in crates/wasm-shim/tests/auth.rs lines 598-689, build both sequential
actions through the shared helper and pass execution as "sequential".
In `@crates/wasm-shim/tests/failuremode.rs`:
- Around line 70-133: Use distinct response variable names for the two gRPC
actions in the test configuration, such as ratelimit_response_a for
limitador-unreachable and ratelimit_response_b for limitador. Update each
action’s onReply predicates, headers, and denyWith references consistently so
the test still evaluates the second call’s response explicitly.
In `@crates/wasm-shim/tests/multi.rs`:
- Around line 412-452: Parameterize the shared config() helper with the
rate-limit predicate and message template, preserving its existing dynamic
service block and auth onReply chain. In crates/wasm-shim/tests/multi.rs lines
412-452, replace the duplicated inline configuration with config() using the
source-address predicate and template; in lines 671-705, call config() with the
group-membership predicate and group descriptor template.
In `@crates/wasm-shim/tests/rate_limited.rs`:
- Line 286: In the test fixture’s value expression, remove the redundant outer
string conversion around string(3 + 2), while preserving the resulting "5" value
and its expected byte representation.
- Around line 139-169: The standard rate-limit gRPC action is duplicated instead
of being generated by shared test helpers. Add the requested dynamic-service and
rate-limit action helpers beside json_escape_cel and auth_check_request_cel in
crates/wasm-shim/tests/util/common.rs, preserving the shared deny, header, and
failure branches; then replace the inline JSON in all five tests at
crates/wasm-shim/tests/rate_limited.rs#L139-L169, both sequential actions at
crates/wasm-shim/tests/failuremode.rs#L70-L133, all four actions at
crates/wasm-shim/tests/failures.rs#L62-L92, the single action at
crates/wasm-shim/tests/remote_address.rs#L65-L98, and the rate-limit action at
crates/wasm-shim/tests/multi.rs#L106-L136. Also add and use the matching
auth-action helper in multi.rs.
In `@crates/wasm-shim/tests/response_body.rs`:
- Around line 627-639: Update the log assertions in the response-body test to
remove the order-dependent “Task failed: "0"” and “Task failed: "1"” checks,
while preserving the descriptive JSON parse, body-store, and message-builder
error assertions.
In `@crates/wasm-shim/tests/util/common.rs`:
- Around line 18-23: Update json_escape_cel to delegate CEL string escaping to
serde_json::to_string instead of collapsing whitespace and manually replacing
quotes, ensuring backslashes and other JSON-special characters are escaped
correctly while preserving the helper’s returned string contract.
In `@README.md`:
- Line 76: Remove the `requestData` row from the top-level configuration field
table in README.md, since PluginConfiguration does not support or deserialize
this field.
🪄 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 Plus
Run ID: ff675a23-c3f6-448c-937e-f12cf0b6138b
📒 Files selected for processing (25)
README.mdcrates/kuadrant-filter/src/configuration.rscrates/kuadrant-filter/src/configuration/legacy_translation.rscrates/kuadrant-filter/src/data/cel.rscrates/kuadrant-filter/src/kuadrant/context.rscrates/kuadrant-filter/src/kuadrant/pipeline/blueprint.rscrates/kuadrant-filter/src/kuadrant/pipeline/factory.rscrates/kuadrant-filter/src/kuadrant/pipeline/tasks/mod.rscrates/kuadrant-filter/src/kuadrant/pipeline/tasks/token_usage.rscrates/kuadrant-filter/src/services/mod.rscrates/wasm-shim/src/filter/root_context.rscrates/wasm-shim/tests/auth.rscrates/wasm-shim/tests/failuremode.rscrates/wasm-shim/tests/failures.rscrates/wasm-shim/tests/multi.rscrates/wasm-shim/tests/rate_limited.rscrates/wasm-shim/tests/remote_address.rscrates/wasm-shim/tests/response_body.rscrates/wasm-shim/tests/streaming.rscrates/wasm-shim/tests/util/common.rse2e/basic/README.mde2e/basic/envoy.yamle2e/remote-address/envoy.yamlexamples/ratelimit/envoy.yamlexamples/ratelimit_check_report/envoy.yaml
💤 Files with no reviewable changes (4)
- crates/kuadrant-filter/src/kuadrant/pipeline/tasks/token_usage.rs
- crates/kuadrant-filter/src/kuadrant/pipeline/tasks/mod.rs
- crates/kuadrant-filter/src/configuration/legacy_translation.rs
- crates/kuadrant-filter/src/data/cel.rs
1193aca to
7412955
Compare
2d86c85 to
9193b93
Compare
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
9193b93 to
323832a
Compare
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
323832a to
d366605
Compare
1 participant
Removes all of the legacy config and translation in favour of the new config style
response/requestBodyJsonandTokenUsageTaskdropped in favour of explicitStoreTaskSummary by CodeRabbit
New Features
Documentation