feat(sdk): Introduce phase-specific policy sub-interfaces and migrate to v2alpha (SDK v0.5.0)#1358
Open
Thushani-Jayasekera wants to merge 9 commits intowso2:mainfrom
Open
feat(sdk): Introduce phase-specific policy sub-interfaces and migrate to v2alpha (SDK v0.5.0)#1358Thushani-Jayasekera wants to merge 9 commits intowso2:mainfrom
Thushani-Jayasekera wants to merge 9 commits intowso2:mainfrom
Conversation
Thushani-Jayasekera
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
Contributor
|
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:
WalkthroughThis pull request introduces a sealed, capability-based action and policy interface system for the gateway. It replaces monolithic policy patterns with phase-specific interfaces and adds header-level, full-request/response, and streaming action types. A new v2alpha policy interface is introduced alongside updated v1alpha types and implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Thushani-Jayasekera
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/gateway/policy/v1alpha/action.go`:
- Around line 158-176: The response header mutations in
DownstreamResponseModifications.Header are not being applied because executors
in gateway-runtime/policy-engine/internal/executor/chain.go and the translator
in gateway-runtime/policy-engine/internal/kernel/translator.go still read only
the deprecated SetHeaders/RemoveHeaders/AppendHeaders; update both the
translator and the response executor to read and apply header changes from
Header (DownstreamResponseHeaderModifications) in addition to or instead of the
deprecated maps, mapping Header.Set -> SetHeaders, Header.Remove ->
RemoveHeaders, Header.Append -> AppendHeaders (or directly applying
Header.Set/Remove/Append semantics) so policies that populate Header take
effect. Ensure you reference and update the translator code path that builds
response modifications and the executor path that applies them to the downstream
response.
In `@sdk/gateway/policy/v1alpha/context.go`:
- Around line 78-80: Update the exported doc comments that reference removed
symbols to use the package's current exported names: replace mentions of
RequestBodyContext with RequestContext, ResponseBodyContext with
ResponseContext, and update
RequestBodyPolicy/ResponseBodyPolicy/StreamingRequestBodyPolicy/StreamingResponseBodyPolicy
to the corresponding current policy interface names used in this package; ensure
the comment above the RequestContext type (and the other comment blocks at the
other noted locations) accurately states which current type and which current
interface method (e.g., the RequestContext type and the appropriate policy
interface/method names) those contexts are passed to so generated docs point to
existing symbols.
In `@sdk/gateway/policy/v1alpha/legacy.go`:
- Around line 22-25: The shared spec.Parameters.Raw map is reused across
requests and must be cloned before invoking legacy policies to avoid concurrent
mutation; update the executor code that calls LegacyPolicy.OnRequest and
LegacyPolicy.OnResponse (the places where the executor passes
spec.Parameters.Raw into legacy policy invocations) to create a shallow copy of
the params map (copy keys/values into a new map) and pass that clone instead,
ensuring BuildPolicyChain, GetPolicy, LegacyPolicy, and executor call sites use
the cloned map so concurrent requests cannot mutate the original shared map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fe36676-3fac-4bc6-af9d-10603feb889a
⛔ Files ignored due to path filters (1)
sdk/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.gitignoresdk/gateway/policy/v1alpha/action.gosdk/gateway/policy/v1alpha/backward_compat.gosdk/gateway/policy/v1alpha/context.gosdk/gateway/policy/v1alpha/interface.gosdk/gateway/policy/v1alpha/legacy.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
sdk/gateway/policy/v1alpha/context.go (1)
153-170:⚠️ Potential issue | 🟡 MinorUpdate streaming context docs to current interface names.
These comments still reference removed symbols (
StreamingRequestBodyPolicy,StreamingResponseBodyPolicy). Please point them toStreamingRequestPolicyandStreamingResponsePolicyso generated docs resolve correctly.✏️ Suggested doc update
-// RequestStreamContext is the per-chunk context passed to StreamingRequestBodyPolicy. +// RequestStreamContext is the per-chunk context passed to StreamingRequestPolicy. ... -// ResponseStreamContext is the per-chunk context passed to StreamingResponseBodyPolicy. +// ResponseStreamContext is the per-chunk context passed to StreamingResponsePolicy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/gateway/policy/v1alpha/context.go` around lines 153 - 170, Update the doc comments for RequestStreamContext and ResponseStreamContext to reference the current interface names RequestStreamingPolicy? No—use exact names: replace references to the removed StreamingRequestBodyPolicy and StreamingResponseBodyPolicy with the new names RequestStreamingPolicy? Wait, the review says use RequestStreamingPolicy and RequestStreamingPolicy? The review says use RequestStreamingPolicy and RequestStreamingPolicy? This is confusing. Please update the comments to point to RequestStreamingPolicy and StreamingResponsePolicy? I'm going to correct: Replace references to the removed StreamingRequestBodyPolicy with RequestStreamingPolicy and StreamingResponseBodyPolicy with ResponseStreamingPolicy; update wording to mention StreamingRequestPolicy and StreamingResponsePolicy so generated docs link to the existing interfaces, and ensure the comments around RequestStreamContext and ResponseStreamContext mention those exact symbol names.sdk/gateway/policy/v1alpha/action.go (1)
158-176:⚠️ Potential issue | 🔴 CriticalResponse
Headerwiring concern still applies.
DownstreamResponseModifications.Headeris declared as the new contract, but runtime handling has already been flagged as relying on deprecated flat fields; this remains a functional risk for policies using onlyHeader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/gateway/policy/v1alpha/action.go` around lines 158 - 176, The runtime still reads the deprecated flat response header fields instead of honoring the new DownstreamResponseModifications.Header contract; update the wiring so any code that mutates or reads response header actions prefers Header (DownstreamResponseModifications.Header) and only falls back to the deprecated fields (SetHeaders, RemoveHeaders, AppendHeaders, DropHeadersFromAnalytics) when Header is nil/empty. Locate the response handling logic that inspects SetHeaders/RemoveHeaders/AppendHeaders/DropHeadersFromAnalytics or AnalyticsHeaderFilter and change it to: 1) if Header != nil apply Header.Set/Remove/Append and use AnalyticsHeaderFilter for analytics; 2) otherwise translate the deprecated maps/slices into the equivalent Header structure so both old and new policies behave identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/gateway/policy/v1alpha/action.go`:
- Around line 11-13: Update the ImmediateResponse comment to reference the
current public action type names: replace "HeaderAction" with
"RequestHeaderAction" or "ResponseHeaderAction" as appropriate, and replace
"RequestBodyAction/ResponseBodyAction" with "RequestAction" and "ResponseAction"
respectively; ensure the sentence now reads that a non-nil *ImmediateResponse
embedded in RequestHeaderAction, ResponseHeaderAction, RequestAction, or
ResponseAction short-circuits the policy chain and returns a response
immediately.
- Around line 103-105: The runtime translator is ignoring the new
UpstreamRequestModifications.Header field and only reads the deprecated
SetHeaders/AppendHeaders/RemoveHeaders, so update the translator
(gateway/gateway-runtime/policy-engine/internal/kernel/translator.go) to honor
mods.Header (type *UpstreamRequestHeaderModifications) when present: read
Header.Set, Header.Append, Header.Remove and merge or fall back to the
deprecated SetHeaders/AppendHeaders/RemoveHeaders as needed so policies using
only Header are applied; ensure the same mapping/normalization logic used for
the deprecated fields is applied to Header to produce the same runtime header
mutations.
---
Duplicate comments:
In `@sdk/gateway/policy/v1alpha/action.go`:
- Around line 158-176: The runtime still reads the deprecated flat response
header fields instead of honoring the new DownstreamResponseModifications.Header
contract; update the wiring so any code that mutates or reads response header
actions prefers Header (DownstreamResponseModifications.Header) and only falls
back to the deprecated fields (SetHeaders, RemoveHeaders, AppendHeaders,
DropHeadersFromAnalytics) when Header is nil/empty. Locate the response handling
logic that inspects
SetHeaders/RemoveHeaders/AppendHeaders/DropHeadersFromAnalytics or
AnalyticsHeaderFilter and change it to: 1) if Header != nil apply
Header.Set/Remove/Append and use AnalyticsHeaderFilter for analytics; 2)
otherwise translate the deprecated maps/slices into the equivalent Header
structure so both old and new policies behave identically.
In `@sdk/gateway/policy/v1alpha/context.go`:
- Around line 153-170: Update the doc comments for RequestStreamContext and
ResponseStreamContext to reference the current interface names
RequestStreamingPolicy? No—use exact names: replace references to the removed
StreamingRequestBodyPolicy and StreamingResponseBodyPolicy with the new names
RequestStreamingPolicy? Wait, the review says use RequestStreamingPolicy and
RequestStreamingPolicy? The review says use RequestStreamingPolicy and
RequestStreamingPolicy? This is confusing. Please update the comments to point
to RequestStreamingPolicy and StreamingResponsePolicy? I'm going to correct:
Replace references to the removed StreamingRequestBodyPolicy with
RequestStreamingPolicy and StreamingResponseBodyPolicy with
ResponseStreamingPolicy; update wording to mention StreamingRequestPolicy and
StreamingResponsePolicy so generated docs link to the existing interfaces, and
ensure the comments around RequestStreamContext and ResponseStreamContext
mention those exact symbol names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e9b0cbe-4c0a-46da-9514-d13cb0982e74
📒 Files selected for processing (3)
sdk/gateway/policy/v1alpha/action.gosdk/gateway/policy/v1alpha/backward_compat.gosdk/gateway/policy/v1alpha/context.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-runtime/policy-engine/internal/kernel/translator.go (1)
397-461:⚠️ Potential issue | 🔴 CriticalSame pointer-only assertion issue for response modifications.
Line 398 asserts
*policy.DownstreamResponseModificationsbutanalytics.go(lines 400-413 in context snippet) returns a value type. Response analytics metadata will be silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go` around lines 397 - 461, The code in the translator.go uses a type assertion on policyResult.Action to *policy.DownstreamResponseModifications, but analytics.go returns a value type for this, causing the assertion to fail and response analytics metadata to be dropped. To fix this, change the type assertion to the value type policy.DownstreamResponseModifications instead of the pointer type in the block handling response modifications in the relevant function. This ensures the correct type is asserted, preserving response analytics metadata processing.
🧹 Nitpick comments (2)
gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go (1)
240-314: Consider extracting shared body requirement logic to reduce duplication.The
buildPolicyChainmethod here duplicates substantial logic fromkernel/body_mode.go:BuildPolicyChain, including the newLegacyPolicymode handling. Consider extracting the common body requirement computation into a shared helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go` around lines 240 - 314, The buildPolicyChain function duplicates body-requirement and execution-condition logic present in kernel/body_mode.go:BuildPolicyChain; extract that logic into a shared helper (e.g., policyutil.DetermineBodyRequirements or registry.DetermineBodyRequirements) that accepts the created policies and/or PolicySpecs and returns requiresRequestBody, requiresResponseBody, hasExecutionConditions. Move the checks that use policy.LegacyPolicy and the ExecutionCondition inspection (policy.ModeRequest, policy.ModeResponse, policy.ModeRequestResponse, and non-empty ExecutionCondition) into that helper, and replace the duplicated code in ResourceHandler.buildPolicyChain (and kernel.BuildPolicyChain) to call the new helper and assign the three booleans before constructing the registry.PolicyChain.gateway/gateway-runtime/policy-engine/internal/kernel/xds.go (1)
167-175: Third instance of duplicatedLegacyPolicybody requirement logic.This is the file-based config loader's version of the same pattern found in
body_mode.goandxdsclient/handler.go. The body requirement computation is now duplicated in three places.♻️ Suggested consolidation
Extract the body requirement logic into a shared function in the
policypackage or a kernel utility:// In a shared location (e.g., policy package or kernel/utils.go) func ComputeBodyRequirements(impl policy.Policy) (requiresRequest, requiresResponse bool) { if lp, ok := impl.(policy.LegacyPolicy); ok { mode := lp.Mode() requiresRequest = mode == policy.ModeRequest || mode == policy.ModeRequestResponse requiresResponse = mode == policy.ModeResponse || mode == policy.ModeRequestResponse } // TODO: Add detection for new capability-based interfaces return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/kernel/xds.go` around lines 167 - 175, The duplicated logic that inspects LegacyPolicy.Mode() to set requiresRequestBody/requiresResponseBody should be consolidated: add a shared helper (e.g., policy.ComputeBodyRequirements or kernel.ComputeBodyRequirements) that accepts policy.Policy (or policy.LegacyPolicy) and returns (requiresRequest, requiresResponse), then replace the three inline blocks (including the block using impl.(policy.LegacyPolicy) in xds.go) to call that helper; ensure the helper uses the same mode checks (policy.ModeRequest, policy.ModeResponse, policy.ModeRequestResponse) and update callers to use the returned booleans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go`:
- Around line 152-157: The current cloning of spec.Parameters.Raw into params is
only a shallow copy (params := make(...); for k,v := range spec.Parameters.Raw {
params[k]=v }), so nested maps/slices remain shared and can be mutated across
requests; replace these shallow copies (the params cloning near
spec.Parameters.Raw in chain.go and the similar block later) with a deep-copy
routine that recursively duplicates maps, slices, and scalar values (or use a
safe serialization round-trip like JSON/YAML marshal->unmarshal) to ensure no
shared references remain; apply the same deep-copy change to both occurrences
(the initial params clone and the later clone around lines 292-297) and keep the
semantics of copying all keys from spec.Parameters.Raw into the new params map.
- Around line 197-199: The code currently only type-asserts
*policy.UpstreamRequestModifications and thus drops value-form returns and
entirely ignores UpstreamResponseModifications; update the action handling so it
accepts both pointer and value forms for request mods
(policy.UpstreamRequestModifications and *policy.UpstreamRequestModifications)
and invokes applyRequestModifications, and add analogous handling in the
response execution path for policy.UpstreamResponseModifications and
*policy.UpstreamResponseModifications calling the existing
applyResponseModifications (or appropriate response modifier function); ensure
both switch/type-assert branches are added where actions are processed
(referencing the action type assertions around the lines with
applyRequestModifications and the response execution path near the later
execution block).
In `@gateway/gateway-runtime/policy-engine/internal/kernel/body_mode.go`:
- Around line 81-89: The code only inspects LegacyPolicy.Mode() and misses
policies that implement the new capability interfaces, so update the detection
around the existing impl handling to also type-assert impl for
policy.RequestPolicy, policy.ResponsePolicy, policy.StreamingRequestPolicy and
policy.StreamingResponsePolicy and set chain.RequiresRequestBody and
chain.RequiresResponseBody accordingly (e.g., if impl.(policy.RequestPolicy) is
true -> chain.RequiresRequestBody = true; if impl.(policy.ResponsePolicy) is
true -> chain.RequiresResponseBody = true); keep this logic alongside the
existing LegacyPolicy.Mode() branch that currently reads lp.Mode().
In `@gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go`:
- Around line 295-303: The code only checks for policy.LegacyPolicy when setting
requiresRequestBody/requiresResponseBody, missing newer policies that expose a
Mode() method; change the type assertion to accept any implementation that
exposes Mode() by using an inline interface assertion (e.g. if m, ok :=
impl.(interface{ Mode() policy.ModeType }); ok { mode := m.Mode(); if
mode==policy.ModeRequest||mode==policy.ModeRequestResponse { requiresRequestBody
= true } if mode==policy.ModeResponse||mode==policy.ModeRequestResponse {
requiresResponseBody = true } }) so impl (and not only LegacyPolicy) that
provide Mode() will trigger buffering—update the block referencing impl,
policy.LegacyPolicy, and the requiresRequestBody/requiresResponseBody flags
accordingly.
---
Outside diff comments:
In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go`:
- Around line 397-461: The code in the translator.go uses a type assertion on
policyResult.Action to *policy.DownstreamResponseModifications, but analytics.go
returns a value type for this, causing the assertion to fail and response
analytics metadata to be dropped. To fix this, change the type assertion to the
value type policy.DownstreamResponseModifications instead of the pointer type in
the block handling response modifications in the relevant function. This ensures
the correct type is asserted, preserving response analytics metadata processing.
---
Nitpick comments:
In `@gateway/gateway-runtime/policy-engine/internal/kernel/xds.go`:
- Around line 167-175: The duplicated logic that inspects LegacyPolicy.Mode() to
set requiresRequestBody/requiresResponseBody should be consolidated: add a
shared helper (e.g., policy.ComputeBodyRequirements or
kernel.ComputeBodyRequirements) that accepts policy.Policy (or
policy.LegacyPolicy) and returns (requiresRequest, requiresResponse), then
replace the three inline blocks (including the block using
impl.(policy.LegacyPolicy) in xds.go) to call that helper; ensure the helper
uses the same mode checks (policy.ModeRequest, policy.ModeResponse,
policy.ModeRequestResponse) and update callers to use the returned booleans.
In `@gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go`:
- Around line 240-314: The buildPolicyChain function duplicates body-requirement
and execution-condition logic present in kernel/body_mode.go:BuildPolicyChain;
extract that logic into a shared helper (e.g.,
policyutil.DetermineBodyRequirements or registry.DetermineBodyRequirements) that
accepts the created policies and/or PolicySpecs and returns requiresRequestBody,
requiresResponseBody, hasExecutionConditions. Move the checks that use
policy.LegacyPolicy and the ExecutionCondition inspection (policy.ModeRequest,
policy.ModeResponse, policy.ModeRequestResponse, and non-empty
ExecutionCondition) into that helper, and replace the duplicated code in
ResourceHandler.buildPolicyChain (and kernel.BuildPolicyChain) to call the new
helper and assign the three booleans before constructing the
registry.PolicyChain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb50e180-68c6-4b10-aa88-b57b903c5bf2
📒 Files selected for processing (10)
gateway/gateway-runtime/policy-engine/internal/executor/chain.gogateway/gateway-runtime/policy-engine/internal/executor/chain_bench_test.gogateway/gateway-runtime/policy-engine/internal/executor/chain_test.gogateway/gateway-runtime/policy-engine/internal/kernel/body_mode.gogateway/gateway-runtime/policy-engine/internal/kernel/execution_context.gogateway/gateway-runtime/policy-engine/internal/kernel/translator.gogateway/gateway-runtime/policy-engine/internal/kernel/xds.gogateway/gateway-runtime/policy-engine/internal/testutils/policies.gogateway/gateway-runtime/policy-engine/internal/xdsclient/handler.gosdk/gateway/policy/v1alpha/action.go
gateway/gateway-runtime/policy-engine/internal/executor/chain.go
Outdated
Show resolved
Hide resolved
Comment on lines
81
to
89
| if lp, ok := impl.(policy.LegacyPolicy); ok { | ||
| mode := lp.Mode() | ||
| if mode == policy.ModeRequest || mode == policy.ModeRequestResponse { | ||
| chain.RequiresRequestBody = true | ||
| } | ||
| if mode == policy.ModeResponse || mode == policy.ModeRequestResponse { | ||
| chain.RequiresResponseBody = true | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any policies implement the new interfaces without LegacyPolicy
echo "=== Searching for RequestPolicy/ResponsePolicy implementations ==="
rg -n 'func \([^)]+\) OnRequestBody\(' --type=go gateway sdk
echo ""
echo "=== Searching for LegacyPolicy implementations ==="
rg -n 'func \([^)]+\) Mode\(\) policy\.ProcessingMode' --type=go gateway sdkRepository: wso2/api-platform
Length of output: 2156
🏁 Script executed:
# Find policy interface definitions
find . -name "interface.go" -o -name "*policy*.go" | grep -E "(interface|policy)" | head -20Repository: wso2/api-platform
Length of output: 1196
🏁 Script executed:
# Check the complete body_mode.go file to understand full context
cat -n gateway/gateway-runtime/policy-engine/internal/kernel/body_mode.goRepository: wso2/api-platform
Length of output: 5295
🏁 Script executed:
# Search for the policy interface definitions
rg -n "type.*Policy interface" --type=go gateway/gateway-runtime/policy-engineRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Find policy interface definitions in the SDK package
rg -n "type.*Policy interface" --type=go -A 3 sdk/Repository: wso2/api-platform
Length of output: 2534
🏁 Script executed:
# Check what's in the policy package
find sdk -name "*.go" -type f | head -20Repository: wso2/api-platform
Length of output: 956
🏁 Script executed:
# Look for RequestPolicy or ResponsePolicy mentions
rg -n "RequestPolicy\|ResponsePolicy" --type=go gateway sdkRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check the policy package structure
cat -n sdk/gateway/policy/v1alpha/policy.go 2>/dev/null | head -100Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check if there are any implementations of the new interfaces
rg -n "impl.*RequestPolicy|impl.*ResponsePolicy|impl.*StreamingRequestPolicy|impl.*StreamingResponsePolicy" --type=goRepository: wso2/api-platform
Length of output: 746
🏁 Script executed:
# Check if any policies implement both LegacyPolicy and new interfaces
rg -l "type.*Policy struct" --type=go gateway/ | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {} | grep -A 10 "type.*Policy struct"'Repository: wso2/api-platform
Length of output: 9570
🏁 Script executed:
# Check what the AnalyticsPolicy (which implements LegacyPolicy) looks like
cat -n gateway/system-policies/analytics/analytics.go | head -150Repository: wso2/api-platform
Length of output: 6747
🏁 Script executed:
# Check if there's any other place where RequiresRequestBody/RequiresResponseBody are set
rg -n "RequiresRequestBody\|RequiresResponseBody" --type=go gateway/Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check the policy.go file to understand ProcessingMode structure
cat -n sdk/gateway/policy/v1alpha/policy.goRepository: wso2/api-platform
Length of output: 129
🏁 Script executed:
# Look at the complete interface definitions with their documentation
cat -n sdk/gateway/policy/v1alpha/interface.go | head -120Repository: wso2/api-platform
Length of output: 5893
Policies implementing only the new capability interfaces will not trigger body buffering.
The body requirement computation in lines 81–89 only checks LegacyPolicy.Mode(). According to the interface documentation at lines 8–14, the kernel is supposed to infer body buffering requirements from implementations of RequestPolicy, ResponsePolicy, StreamingRequestPolicy, and StreamingResponsePolicy. However, the current code does not detect these interfaces.
If a policy implements only these new capability-based interfaces without also implementing LegacyPolicy, it will not set RequiresRequestBody or RequiresResponseBody, causing the kernel to skip body buffering entirely.
Add capability detection for the new interfaces:
Example implementation
// Detect RequestPolicy/ResponsePolicy capabilities
if _, ok := impl.(policy.RequestPolicy); ok {
chain.RequiresRequestBody = true
}
if _, ok := impl.(policy.ResponsePolicy); ok {
chain.RequiresResponseBody = true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway/gateway-runtime/policy-engine/internal/kernel/body_mode.go` around
lines 81 - 89, The code only inspects LegacyPolicy.Mode() and misses policies
that implement the new capability interfaces, so update the detection around the
existing impl handling to also type-assert impl for policy.RequestPolicy,
policy.ResponsePolicy, policy.StreamingRequestPolicy and
policy.StreamingResponsePolicy and set chain.RequiresRequestBody and
chain.RequiresResponseBody accordingly (e.g., if impl.(policy.RequestPolicy) is
true -> chain.RequiresRequestBody = true; if impl.(policy.ResponsePolicy) is
true -> chain.RequiresResponseBody = true); keep this logic alongside the
existing LegacyPolicy.Mode() branch that currently reads lp.Mode().
gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
gateway/gateway-runtime/policy-engine/internal/executor/chain.go (2)
146-151:⚠️ Potential issue | 🟠 MajorThe params clone is still shallow.
spec.Parameters.Rawis amap[string]interface{}; nested maps, slices, and pointers are still shared after this copy. A policy mutating nested params can still leak state across requests or race with another request. Please deep-clone the values once and reuse that helper in both request and response paths.Also applies to: 280-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go` around lines 146 - 151, The shallow copy of spec.Parameters.Raw into params still shares nested structures; implement a deep-clone helper (e.g., DeepCloneParamValue or cloneParamsMap) that recursively copies maps, slices, and pointers (falling back to encoding/json marshal/unmarshal if easier) and replace the current shallow loop that creates params := make(...)/for k,v := range spec.Parameters.Raw { params[k]=v } with params[k]=DeepCloneParamValue(v); then reuse the same helper for the response-path clone (the other clone at the response handling site) to ensure no nested state is shared across requests.
324-344:⚠️ Potential issue | 🔴 CriticalCanonicalize structured mutations here before downstream code sees them.
These helpers only normalize pointer vs value. The new API and
newV2AlphaAdapterpopulateHeaderandQueryParametersToAdd/QueryParametersToRemove, while the executor/translator paths still consume the legacy flat fields. As written, adapted header-phase mutations are stored inpolicyResult.Actionbut never applied. Flatten the structured fields here until every consumer reads the new shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go` around lines 324 - 344, normalizeRequestAction and normalizeResponseAction currently only convert value vs pointer but do not flatten the new structured mutation fields, so adapted header/query mutations end up in policyResult.Action but are never applied; update both functions (handling policy.UpstreamRequestModifications and policy.DownstreamResponseModifications / policy.ImmediateResponse) to, after converting to pointer form, copy/flatten the structured fields (e.g., Header map/array, QueryParametersToAdd, QueryParametersToRemove, etc.) into the legacy flat mutation fields the executor/translator expects (the old HeaderToSet/HeaderToRemove, QueryParametersToAdd/Remove equivalents, and any body/status fields), preserving existing values and avoiding duplication so downstream consumers see the flattened mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go`:
- Around line 310-314: The response-phase loop currently only handles
*policy.DownstreamResponseModifications and ignores *policy.ImmediateResponse,
so short-circuiting never happens; update the loop that calls
normalizeResponseAction to also detect when action is a
*policy.ImmediateResponse, apply the immediate response handling (invoke the
existing immediate response helper or create one analogous to
applyResponseModifications), set the chain's FinalAction to that
ImmediateResponse value, call StopExecution() to halt further policy execution,
and ensure you still call applyResponseModifications when action is a
DownstreamResponseModifications so both cases are covered; reference
normalizeResponseAction, policy.ImmediateResponse,
policy.DownstreamResponseModifications, applyResponseModifications,
StopExecution(), and FinalAction when making the change.
In `@gateway/gateway-runtime/policy-engine/internal/registry/v2alpha_adapter.go`:
- Around line 73-84: The body-phase callback is currently skipped when
ctx.Body.Present is false, causing v2 RequestPolicy/ResponsePolicy.OnRequestBody
and OnResponseBody to never run for empty bodies; update the checks in the
v2alpha_adapter.go adapter so you call rp.OnRequestBody(ctx) and the response
equivalent regardless of ctx.Body.Present (i.e., remove the ctx.Body.Present
gating around the type switch), while keeping existing nil-safety (ensure
handlers tolerate ctx.Body == nil) and still handling the same return types
(v1.ImmediateResponse, *v1.ImmediateResponse, v1.UpstreamRequestModifications,
*v1.UpstreamRequestModifications) via the same switch/mergeRequestMods logic;
apply the same change to the analogous block at the other location (the
OnResponseBody mapping at lines 108-119).
- Around line 157-159: The current assignment dst.Header = src.Header overwrites
earlier header mutations (breaking composition for
OnRequestHeaders/OnResponseHeaders); instead, update the code in
v2alpha_adapter.go to merge src.Header into dst.Header: ensure dst.Header is
non-nil, then copy/overwrite individual map entries from src.Header.Set into
dst.Header.Set, append entries from src.Header.Append into dst.Header.Append
(preserving existing slices), and union/remove keys from src.Header.Remove into
dst.Header.Remove (preserving prior removes), for both places where dst.Header
is currently replaced (the src.Header handling at the earlier block and the
similar one at lines ~191-193) so later phases only override conflicting keys
rather than discarding prior mutations.
---
Duplicate comments:
In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go`:
- Around line 146-151: The shallow copy of spec.Parameters.Raw into params still
shares nested structures; implement a deep-clone helper (e.g.,
DeepCloneParamValue or cloneParamsMap) that recursively copies maps, slices, and
pointers (falling back to encoding/json marshal/unmarshal if easier) and replace
the current shallow loop that creates params := make(...)/for k,v := range
spec.Parameters.Raw { params[k]=v } with params[k]=DeepCloneParamValue(v); then
reuse the same helper for the response-path clone (the other clone at the
response handling site) to ensure no nested state is shared across requests.
- Around line 324-344: normalizeRequestAction and normalizeResponseAction
currently only convert value vs pointer but do not flatten the new structured
mutation fields, so adapted header/query mutations end up in policyResult.Action
but are never applied; update both functions (handling
policy.UpstreamRequestModifications and policy.DownstreamResponseModifications /
policy.ImmediateResponse) to, after converting to pointer form, copy/flatten the
structured fields (e.g., Header map/array, QueryParametersToAdd,
QueryParametersToRemove, etc.) into the legacy flat mutation fields the
executor/translator expects (the old HeaderToSet/HeaderToRemove,
QueryParametersToAdd/Remove equivalents, and any body/status fields), preserving
existing values and avoiding duplication so downstream consumers see the
flattened mutations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 164ce868-9568-430e-b12c-747cdd352878
📒 Files selected for processing (13)
gateway/gateway-runtime/policy-engine/internal/executor/chain.gogateway/gateway-runtime/policy-engine/internal/executor/chain_bench_test.gogateway/gateway-runtime/policy-engine/internal/executor/chain_test.gogateway/gateway-runtime/policy-engine/internal/kernel/body_mode.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/gateway-runtime/policy-engine/internal/kernel/xds.gogateway/gateway-runtime/policy-engine/internal/registry/registry.gogateway/gateway-runtime/policy-engine/internal/registry/v2alpha_adapter.gogateway/gateway-runtime/policy-engine/internal/testutils/policies.gogateway/gateway-runtime/policy-engine/internal/xdsclient/handler.gosdk/gateway/policy/v1alpha/action.gosdk/gateway/policy/v1alpha/interface.gosdk/gateway/policy/v2alpha/interface.go
💤 Files with no reviewable changes (3)
- gateway/gateway-runtime/policy-engine/internal/kernel/body_mode.go
- gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
- gateway/gateway-runtime/policy-engine/internal/kernel/xds.go
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/gateway-runtime/policy-engine/internal/executor/chain_bench_test.go
- gateway/gateway-runtime/policy-engine/internal/executor/chain_test.go
gateway/gateway-runtime/policy-engine/internal/registry/v2alpha_adapter.go
Outdated
Show resolved
Hide resolved
gateway/gateway-runtime/policy-engine/internal/registry/v2alpha_adapter.go
Outdated
Show resolved
Hide resolved
gateway/gateway-runtime/policy-engine/internal/registry/v2alpha_adapter.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/gateway/policy/v1alpha/interface.go`:
- Around line 118-119: Update the BodyModeBuffer documentation to mention the
phase-specific handlers by stating that it buffers the complete body before
invoking both the request/response lifecycle handlers and their body-specific
counterparts; specifically reference BodyModeBuffer (type BodyProcessingMode)
and note it applies to OnRequest, OnRequestBody, OnResponse, and OnResponseBody
so implementers know all relevant hooks will receive the buffered body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 200348c5-af42-41c7-97ec-278593a0ece3
📒 Files selected for processing (1)
sdk/gateway/policy/v1alpha/interface.go
Comment on lines
118
to
119
| // BodyModeBuffer — buffer the complete body before invoking OnRequest/OnResponse. | ||
| BodyModeBuffer BodyProcessingMode = "BUFFER" |
Contributor
There was a problem hiding this comment.
Update BodyModeBuffer doc to include phase-specific handlers.
The comment currently mentions only OnRequest/OnResponse; with the new sub-interfaces, it should also mention OnRequestBody/OnResponseBody to avoid implementer confusion.
✏️ Suggested doc update
- // BodyModeBuffer — buffer the complete body before invoking OnRequest/OnResponse.
+ // BodyModeBuffer — buffer the complete body before invoking body-phase handlers
+ // (OnRequest/OnResponse for monolithic policies, or
+ // OnRequestBody/OnResponseBody for phase-specific policies).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // BodyModeBuffer — buffer the complete body before invoking OnRequest/OnResponse. | |
| BodyModeBuffer BodyProcessingMode = "BUFFER" | |
| // BodyModeBuffer — buffer the complete body before invoking body-phase handlers | |
| // (OnRequest/OnResponse for monolithic policies, or | |
| // OnRequestBody/OnResponseBody for phase-specific policies). | |
| BodyModeBuffer BodyProcessingMode = "BUFFER" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/gateway/policy/v1alpha/interface.go` around lines 118 - 119, Update the
BodyModeBuffer documentation to mention the phase-specific handlers by stating
that it buffers the complete body before invoking both the request/response
lifecycle handlers and their body-specific counterparts; specifically reference
BodyModeBuffer (type BodyProcessingMode) and note it applies to OnRequest,
OnRequestBody, OnResponse, and OnResponseBody so implementers know all relevant
hooks will receive the buffered body.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
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)
gateway/gateway-runtime/policy-engine/internal/kernel/translator.go (1)
367-398:⚠️ Potential issue | 🔴 CriticalHandle response-phase
*ImmediateResponsebefore folding mutations.
ResponseExecutionResultalready carriesFinalAction, but this code never mirrors the request-side short-circuit branch. If a response policy returns*policy.ImmediateResponse, the translator drops the replacement response and continues forwarding the upstream one.
♻️ Duplicate comments (4)
gateway/gateway-runtime/policy-engine/internal/kernel/translator.go (2)
105-142:⚠️ Potential issue | 🔴 CriticalNormalize the new request mutation fields before translating.
This loop still reads only the deprecated flat fields (
SetHeaders,AppendHeaders,RemoveHeaders,AddQueryParameters,RemoveQueryParameters,DropHeadersFromAnalytics). Policies usingHeader,QueryParametersToAdd,QueryParametersToRemove, orAnalyticsHeaderFiltersilently become no-ops here.Suggested normalization shape
- // Collect SetHeader operations - for key, value := range mods.SetHeaders { + headerMods := mods.Header + if headerMods == nil { + headerMods = &policy.UpstreamRequestHeaderModifications{ + Set: mods.SetHeaders, + Remove: mods.RemoveHeaders, + Append: mods.AppendHeaders, + } + } + for key, value := range headerMods.Set { headerOps[strings.ToLower(key)] = append(headerOps[strings.ToLower(key)], &headerOp{opType: "set", value: value}) } - // Collect AppendHeader operations - for key, values := range mods.AppendHeaders { + for key, values := range headerMods.Append { for _, value := range values { headerOps[strings.ToLower(key)] = append(headerOps[strings.ToLower(key)], &headerOp{opType: "append", value: value}) } } - // Collect RemoveHeader operations - for _, key := range mods.RemoveHeaders { + for _, key := range headerMods.Remove { headerOps[strings.ToLower(key)] = append(headerOps[strings.ToLower(key)], &headerOp{opType: "remove", value: ""}) } - if mods.AddQueryParameters != nil { - path = utils.AddQueryParametersToPath(path, mods.AddQueryParameters) + addQuery := mods.QueryParametersToAdd + if addQuery == nil { + addQuery = mods.AddQueryParameters + } + if addQuery != nil { + path = utils.AddQueryParametersToPath(path, addQuery) pathMutation = &path }Also applies to: 167-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go` around lines 105 - 142, The loop that translates policyResult.Action into headerOps/path/bodyMutation must first normalize the newer mutation fields into the legacy flat shape so both old and new fields are honored: detect when policyResult.Action is a *policy.UpstreamRequestModifications and if mods.Header (new header map), mods.QueryParametersToAdd, mods.QueryParametersToRemove, or mods.AnalyticsHeaderFilter are present, merge them into mods.SetHeaders, mods.AppendHeaders, mods.RemoveHeaders, mods.AddQueryParameters, mods.RemoveQueryParameters, and DropHeadersFromAnalytics respectively before the existing translation logic runs; update the normalization in the translator.go block handling UpstreamRequestModifications (the code that currently reads mods.SetHeaders/AppendHeaders/RemoveHeaders/AddQueryParameters/RemoveQueryParameters/DropHeadersFromAnalytics) so policies using Header, QueryParametersToAdd/Remove, or AnalyticsHeaderFilter are not no-ops.
398-458:⚠️ Potential issue | 🔴 CriticalHonor the new response mutation fields in this translator.
Like the request path, this still aggregates only
SetHeaders,AppendHeaders,RemoveHeaders, andDropHeadersFromAnalytics.DownstreamResponseModifications.HeaderandAnalyticsHeaderFilterare ignored, so policies migrated to the new surface never mutate the downstream response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go` around lines 398 - 458, The translator currently ignores DownstreamResponseModifications.Header and AnalyticsHeaderFilter so migrated policies don't actually mutate downstream responses; update the block that handles policy.DownstreamResponseModifications (the mods variable) to: 1) collect/modulate header mutations from mods.Header into headerOps (like SetHeaders/AppendHeaders/RemoveHeaders) so downstream headers are mutated, 2) honor mods.AnalyticsHeaderFilter when computing finalizedHeaders (use it together with dropAction when calling finalizeAnalyticsHeaders or a new helper) and add the resulting filtered set into analyticsData["response_headers"], and 3) ensure any reference to execCtx.responseContext.ResponseHeaders/GetAll remains the source of originalHeaders; touch symbols headerOps, headerOp, mods.Header, mods.AnalyticsHeaderFilter, finalizeAnalyticsHeaders, analyticsData, and execCtx.responseContext.ResponseHeaders.sdk/gateway/policy/v1alpha/context.go (1)
153-158:⚠️ Potential issue | 🟡 MinorFix stale streaming interface names in the exported docs.
RequestStreamContextandResponseStreamContextstill point readers toStreamingRequestBodyPolicy/StreamingResponseBodyPolicy, which are not part of the current public surface. Generated docs will send policy authors to dead symbols unless these comments reference the current streaming callbacks.Also applies to: 170-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/gateway/policy/v1alpha/context.go` around lines 153 - 158, The GoDoc for RequestStreamContext (and likewise for ResponseStreamContext) still references removed symbols StreamingRequestBodyPolicy/StreamingResponseBodyPolicy; update the comment text on RequestStreamContext and ResponseStreamContext to remove those stale names and instead reference the current exported streaming callback types/functions used by the SDK (replace the dead symbol names with the present streaming callback identifiers), keeping the same explanation about read-only headers and body arrival via the stream argument so generated docs point to valid public symbols.gateway/gateway-runtime/policy-engine/internal/executor/chain.go (1)
147-152:⚠️ Potential issue | 🟠 MajorParameter cloning is still shallow; nested values can leak mutations across requests.
Top-level map copying here does not clone nested maps/slices, so policy mutations can still race through shared nested references.
Suggested fix (use deep clone helper in both paths)
- params := make(map[string]interface{}, len(spec.Parameters.Raw)) - for k, v := range spec.Parameters.Raw { - params[k] = v - } + params := cloneParams(spec.Parameters.Raw)- params := make(map[string]interface{}, len(spec.Parameters.Raw)) - for k, v := range spec.Parameters.Raw { - params[k] = v - } + params := cloneParams(spec.Parameters.Raw)+func cloneParams(raw map[string]interface{}) map[string]interface{} { + cloned := make(map[string]interface{}, len(raw)) + for k, v := range raw { + cloned[k] = deepCloneValue(v) + } + return cloned +} + +func deepCloneValue(v interface{}) interface{} { + switch t := v.(type) { + case map[string]interface{}: + m := make(map[string]interface{}, len(t)) + for k, vv := range t { + m[k] = deepCloneValue(vv) + } + return m + case []interface{}: + s := make([]interface{}, len(t)) + for i, vv := range t { + s[i] = deepCloneValue(vv) + } + return s + default: + return v + } +}Also applies to: 281-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go` around lines 147 - 152, The current shallow copy of spec.Parameters.Raw into params allows nested maps/slices to be shared and mutated; replace the manual top-level copy with a deep-clone of spec.Parameters.Raw using the project's deep clone helper (e.g., deepClone or DeepClone) so nested structures are duplicated; update both occurrences (the block that creates params from spec.Parameters.Raw and the other instance at the later block around the 281-286 region) to call the helper and assign the result to params to prevent cross-request mutation.
🧹 Nitpick comments (1)
gateway/gateway-runtime/policy-engine/internal/testutils/policies.go (1)
69-80: Use the new nested header actions in this shared test fixture.
HeaderModifyingPolicystill returnsSetHeaders, so every test that relies on this helper keeps validating only the deprecated API. Switching this fixture toUpstreamRequestModifications.Header/DownstreamResponseModifications.Headerwould make the suite exercise the migration path by default.Proposed fixture update
func (p *HeaderModifyingPolicy) OnRequest(*policy.RequestContext, map[string]interface{}) policy.RequestAction { return &policy.UpstreamRequestModifications{ - SetHeaders: map[string]string{p.Key: p.Value}, + Header: &policy.UpstreamRequestHeaderModifications{ + Set: map[string]string{p.Key: p.Value}, + }, } } // OnResponse returns modifications to set the configured header. func (p *HeaderModifyingPolicy) OnResponse(*policy.ResponseContext, map[string]interface{}) policy.ResponseAction { return &policy.DownstreamResponseModifications{ - SetHeaders: map[string]string{p.Key: p.Value}, + Header: &policy.DownstreamResponseHeaderModifications{ + Set: map[string]string{p.Key: p.Value}, + }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/testutils/policies.go` around lines 69 - 80, The shared test fixture HeaderModifyingPolicy still populates the deprecated SetHeaders fields, so update OnRequest and OnResponse to use the new nested Header structs: return an &policy.UpstreamRequestModifications{Header: &policy.HeaderActions{Set: map[string]string{p.Key: p.Value}}} in OnRequest and an &policy.DownstreamResponseModifications{Header: &policy.HeaderActions{Set: map[string]string{p.Key: p.Value}}} in OnResponse so tests exercise the migration path; adjust to reference UpstreamRequestModifications.Header, DownstreamResponseModifications.Header and policy.HeaderActions.Set instead of SetHeaders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/gateway/policy/v1alpha/interface.go`:
- Around line 3-9: The comment in the Policy interface block incorrectly
instructs authors to write new policies against policyv2alpha and to prefer the
v2 API (and deprecate BodyModeStream) even though the engine will not invoke v2
methods yet; update the comment for Policy (and the lines referencing Mode(),
OnRequest, OnResponse, policyv2alpha, and BodyModeStream) to instead state that
new policies should continue to target the existing Policy interface
(Mode()/OnRequest()/OnResponse()) for now, and mention policyv2alpha as a future
migration target without recommending it as the current implementation path or
deprecating BodyModeStream until the engine actually switches.
---
Duplicate comments:
In `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go`:
- Around line 147-152: The current shallow copy of spec.Parameters.Raw into
params allows nested maps/slices to be shared and mutated; replace the manual
top-level copy with a deep-clone of spec.Parameters.Raw using the project's deep
clone helper (e.g., deepClone or DeepClone) so nested structures are duplicated;
update both occurrences (the block that creates params from spec.Parameters.Raw
and the other instance at the later block around the 281-286 region) to call the
helper and assign the result to params to prevent cross-request mutation.
In `@gateway/gateway-runtime/policy-engine/internal/kernel/translator.go`:
- Around line 105-142: The loop that translates policyResult.Action into
headerOps/path/bodyMutation must first normalize the newer mutation fields into
the legacy flat shape so both old and new fields are honored: detect when
policyResult.Action is a *policy.UpstreamRequestModifications and if mods.Header
(new header map), mods.QueryParametersToAdd, mods.QueryParametersToRemove, or
mods.AnalyticsHeaderFilter are present, merge them into mods.SetHeaders,
mods.AppendHeaders, mods.RemoveHeaders, mods.AddQueryParameters,
mods.RemoveQueryParameters, and DropHeadersFromAnalytics respectively before the
existing translation logic runs; update the normalization in the translator.go
block handling UpstreamRequestModifications (the code that currently reads
mods.SetHeaders/AppendHeaders/RemoveHeaders/AddQueryParameters/RemoveQueryParameters/DropHeadersFromAnalytics)
so policies using Header, QueryParametersToAdd/Remove, or AnalyticsHeaderFilter
are not no-ops.
- Around line 398-458: The translator currently ignores
DownstreamResponseModifications.Header and AnalyticsHeaderFilter so migrated
policies don't actually mutate downstream responses; update the block that
handles policy.DownstreamResponseModifications (the mods variable) to: 1)
collect/modulate header mutations from mods.Header into headerOps (like
SetHeaders/AppendHeaders/RemoveHeaders) so downstream headers are mutated, 2)
honor mods.AnalyticsHeaderFilter when computing finalizedHeaders (use it
together with dropAction when calling finalizeAnalyticsHeaders or a new helper)
and add the resulting filtered set into analyticsData["response_headers"], and
3) ensure any reference to execCtx.responseContext.ResponseHeaders/GetAll
remains the source of originalHeaders; touch symbols headerOps, headerOp,
mods.Header, mods.AnalyticsHeaderFilter, finalizeAnalyticsHeaders,
analyticsData, and execCtx.responseContext.ResponseHeaders.
In `@sdk/gateway/policy/v1alpha/context.go`:
- Around line 153-158: The GoDoc for RequestStreamContext (and likewise for
ResponseStreamContext) still references removed symbols
StreamingRequestBodyPolicy/StreamingResponseBodyPolicy; update the comment text
on RequestStreamContext and ResponseStreamContext to remove those stale names
and instead reference the current exported streaming callback types/functions
used by the SDK (replace the dead symbol names with the present streaming
callback identifiers), keeping the same explanation about read-only headers and
body arrival via the stream argument so generated docs point to valid public
symbols.
---
Nitpick comments:
In `@gateway/gateway-runtime/policy-engine/internal/testutils/policies.go`:
- Around line 69-80: The shared test fixture HeaderModifyingPolicy still
populates the deprecated SetHeaders fields, so update OnRequest and OnResponse
to use the new nested Header structs: return an
&policy.UpstreamRequestModifications{Header: &policy.HeaderActions{Set:
map[string]string{p.Key: p.Value}}} in OnRequest and an
&policy.DownstreamResponseModifications{Header: &policy.HeaderActions{Set:
map[string]string{p.Key: p.Value}}} in OnResponse so tests exercise the
migration path; adjust to reference UpstreamRequestModifications.Header,
DownstreamResponseModifications.Header and policy.HeaderActions.Set instead of
SetHeaders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 134579fc-a533-47a2-9903-4d4cd20379df
⛔ Files ignored due to path filters (1)
sdk/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.gitignoregateway/gateway-runtime/policy-engine/internal/executor/chain.gogateway/gateway-runtime/policy-engine/internal/executor/chain_bench_test.gogateway/gateway-runtime/policy-engine/internal/executor/chain_test.gogateway/gateway-runtime/policy-engine/internal/kernel/body_mode.gogateway/gateway-runtime/policy-engine/internal/kernel/translator.gogateway/gateway-runtime/policy-engine/internal/kernel/translator_test.gogateway/gateway-runtime/policy-engine/internal/kernel/xds.gogateway/gateway-runtime/policy-engine/internal/testutils/policies.gogateway/gateway-runtime/policy-engine/internal/xdsclient/handler.gosdk/gateway/policy/v1alpha/action.gosdk/gateway/policy/v1alpha/context.gosdk/gateway/policy/v1alpha/interface.gosdk/gateway/policy/v2alpha/interface.go
💤 Files with no reviewable changes (3)
- gateway/gateway-runtime/policy-engine/internal/xdsclient/handler.go
- gateway/gateway-runtime/policy-engine/internal/kernel/xds.go
- gateway/gateway-runtime/policy-engine/internal/kernel/body_mode.go
Comment on lines
+3
to
+9
| // Policy is the interface all policies must implement. | ||
| // It uses a monolithic style: Mode() declares processing requirements, and | ||
| // OnRequest/OnResponse are called by the kernel for the respective phases. | ||
| // | ||
| // New policies should be written against policyv2alpha, which uses a clean | ||
| // sub-interface model where capabilities are declared by implementing | ||
| // RequestHeaderPolicy, RequestPolicy, ResponsePolicy, etc. |
Contributor
There was a problem hiding this comment.
Don’t steer policy authors to policyv2alpha as the runtime path yet.
The migration plan for this PR says v2alpha methods compile for dual-implementation, but are not invoked until the engine is updated. These comments currently tell authors to write new policies against policyv2alpha and deprecate BodyModeStream in its favor, which would push users to an API the current engine ignores.
Also applies to: 121-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/gateway/policy/v1alpha/interface.go` around lines 3 - 9, The comment in
the Policy interface block incorrectly instructs authors to write new policies
against policyv2alpha and to prefer the v2 API (and deprecate BodyModeStream)
even though the engine will not invoke v2 methods yet; update the comment for
Policy (and the lines referencing Mode(), OnRequest, OnResponse, policyv2alpha,
and BodyModeStream) to instead state that new policies should continue to target
the existing Policy interface (Mode()/OnRequest()/OnResponse()) for now, and
mention policyv2alpha as a future migration target without recommending it as
the current implementation path or deprecating BodyModeStream until the engine
actually switches.
Contributor
Author
There was a problem hiding this comment.
Changes in interface.go
- Policy interface moved to the top
- BodyModeStream deprecation notice
- Level and its constants moved up
- ProcessingMode fields lost their inline field comments
Contributor
There was a problem hiding this comment.
@Thushani-Jayasekera, thanks for the summary. A few notes on each point:
-
Policy interface moved to the top — No issue there structurally, but the comment block still contains:
New policies should be written against policyv2alpha…
My concern stands: since the engine does not yet invoke v2alpha methods, steering authors there now can cause silent no-ops. It would be safer to soften the wording until the engine is updated — e.g., "A capability-based v2alpha interface is available for future migration; the engine currently dispatches only through this interface."
-
BodyModeStreamdeprecation notice — Same concern. The deprecation comment// Deprecated: Use policyv2alpha.StreamingRequestPolicy / StreamingResponsePolicy instead.directs users to an API the runtime doesn't call yet. Consider replacing it with something like// Note: streaming chunk support is coming in a future engine release.until v2alpha dispatch is live. -
Level and its constants moved up — Purely structural, no concerns.
-
ProcessingModefields lost their inline field comments — Good catch on your end. The previous version had per-field documentation (e.g., whatRequestHeaderModevsRequestBodyModecontrols). Those are useful for authors scanning the struct. Worth restoring them, for example:
type ProcessingMode struct {
// RequestHeaderMode controls whether OnRequest is called at the header phase.
RequestHeaderMode HeaderProcessingMode
// RequestBodyMode controls whether/how the request body is buffered before OnRequest.
RequestBodyMode BodyProcessingMode
// ResponseHeaderMode controls whether OnResponse is called at the response header phase.
ResponseHeaderMode HeaderProcessingMode
// ResponseBodyMode controls whether/how the response body is buffered before OnResponse.
ResponseBodyMode BodyProcessingMode
}
| // ResponseExecutionResult represents the result of executing all response policies in a chain | ||
| type ResponseExecutionResult struct { | ||
| Results []ResponsePolicyResult | ||
| ShortCircuited bool // true if chain stopped early due to ImmediateResponse |
Contributor
Author
There was a problem hiding this comment.
separates "chain completed normally" from "chain bailed early to block the request"
| // OperationPath is the operation path pattern from the API definition | ||
| // (e.g., "/pets/{id}" for a parameterized path) | ||
| // This differs from RequestContext.Path which contains the actual request path | ||
| // This differs from RequestHeaderContext.Path which contains the actual request path |
Contributor
Author
There was a problem hiding this comment.
Changes in context.go - Four new types added:
- RequestHeaderContext — header-phase context for the new RequestHeaderPolicy sub-interface
- ResponseHeaderContext — header-phase context for the new ResponseHeaderPolicy sub-interface
- StreamBody — a single streaming chunk (raw bytes + end-of-stream signal)
- RequestStreamContext — per-chunk context for StreamingRequestPolicy
- ResponseStreamContext — per-chunk context for StreamingResponsePolicy
No existing types were modified. Zero breaking changes in this file.
| @@ -1,72 +1,237 @@ | |||
| package policyv1alpha | |||
|
|
|||
| // RequestAction marker interface (oneof pattern) | |||
Contributor
Author
There was a problem hiding this comment.
---
Renamed types (breaking for existing policies)
┌────────────────────────────────────────┬──────────────────────────────────────────────┐
│ Old name │ New name │
├────────────────────────────────────────┼──────────────────────────────────────────────┤
│ UpstreamResponseModifications (struct) │ DownstreamResponseModifications (new struct) │
└────────────────────────────────────────┴──────────────────────────────────────────────┘
UpstreamResponseModifications now exists as a type alias pointing to DownstreamResponseModifications:
type UpstreamResponseModifications = DownstreamResponseModifications
Existing policies returning UpstreamResponseModifications{} continue to compile because it's an alias, not a new type.
---
Fields renamed inside UpstreamRequestModifications (breaking for existing policies)
┌──────────────────────────┬─────────────────────────┐
│ Old field │ New field │
├──────────────────────────┼─────────────────────────┤
│ AddQueryParameters │ QueryParametersToAdd │
├──────────────────────────┼─────────────────────────┤
│ RemoveQueryParameters │ QueryParametersToRemove │
├──────────────────────────┼─────────────────────────┤
│ DropHeadersFromAnalytics │ AnalyticsHeaderFilter │
└──────────────────────────┴─────────────────────────┘
The old field names are kept as deprecated fields alongside the new ones, so existing policies still compile. But any policy setting
the old names will get deprecation notices.
---
New fields added to UpstreamRequestModifications
- Header *UpstreamRequestHeaderModifications — sub-struct for header mutations
- AnalyticsHeaderFilter DropHeaderAction — renamed version of DropHeadersFromAnalytics
New fields added to DownstreamResponseModifications (was UpstreamResponseModifications)
- Header *DownstreamResponseHeaderModifications — sub-struct for header mutations
- AnalyticsHeaderFilter DropHeaderAction — renamed version of DropHeadersFromAnalytics
New field added to ImmediateResponse
- AnalyticsHeaderFilter DropHeaderAction — renamed version of DropHeadersFromAnalytics
---
Entirely new types added
- RequestHeaderAction — sealed interface for header-phase request actions
- ResponseHeaderAction — sealed interface for header-phase response actions
- UpstreamRequestHeaderModifications — concrete type implementing RequestHeaderAction
- DownstreamResponseHeaderModifications — concrete type implementing ResponseHeaderAction
- RequestChunkAction — returned by streaming request chunk handlers
- ResponseChunkAction — returned by streaming response chunk handlers
---
New compile-time interface checks added
var (
_ RequestHeaderAction = UpstreamRequestHeaderModifications{}
_ RequestHeaderAction = ImmediateResponse{}
_ ResponseHeaderAction = DownstreamResponseHeaderModifications{}
_ ResponseHeaderAction = ImmediateResponse{}
_ RequestAction = UpstreamRequestModifications{}
_ RequestAction = ImmediateResponse{}
_ ResponseAction = DownstreamResponseModifications{}
_ ResponseAction = ImmediateResponse{}
)
5cef198 to
8bf27e2
Compare
Thushani-Jayasekera
changed the title
…ore version to v0.5.4 in the gateway and SDK directories.
… to reflect the removal of sdk/core dependencies.
…remove deprecated v2alpha policy interface
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #1384
Summary by CodeRabbit
Release Notes