Skip to content

perf(policy-engine): drop per-request deep copy of policy params #2617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
renuka-fernando wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −119
Open

perf(policy-engine): drop per-request deep copy of policy params #2617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 18 additions & 63 deletions gateway/gateway-runtime/policy-engine/internal/executor/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package executor

import (
"context"
"encoding/json"
"fmt"
"log/slog"
"strings"
Expand Down Expand Up @@ -141,13 +140,9 @@ func (c *ChainExecutor) ExecuteRequestHeaderPolicies(
}
}

params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

action := headerPol.OnRequestHeaders(ctx, reqCtx, params)
// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
action := headerPol.OnRequestHeaders(ctx, reqCtx, spec.Parameters.Raw)
Comment on lines +143 to +145

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm the concrete type of Parameters.Raw and look for any existing immutability guard
rg -nP -g '*.go' '\btype\s+Parameters\b' -A 10
rg -nP -g '*.go' 'Raw\s+map\[string\]interface\{\}'

# Search for any policy implementation (outside tests) that writes into a params-like map argument
rg -nP -g '*.go' -g '!*_test.go' '\b(params|parameters)\s*\[[^\]]+\]\s*='

Repository: wso2/api-platform

Length of output: 155

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant files before reading them
git ls-files 'gateway/gateway-runtime/policy-engine/internal/executor/chain.go' \
             'gateway/gateway-runtime/policy-engine/internal/**' \
             | sed -n '1,200p'

# Inspect the chain implementation around the cited lines
wc -l gateway/gateway-runtime/policy-engine/internal/executor/chain.go
cat -n gateway/gateway-runtime/policy-engine/internal/executor/chain.go | sed -n '120,180p'

# Find the Parameters type and the Raw field definition
rg -n --hidden --glob '*.go' '\btype\s+Parameters\b|Raw\s+map\[string\]interface\{\}|type\s+.*Parameters' gateway/gateway-runtime/policy-engine/internal

# Find policy callback signatures to see whether the map is exposed directly everywhere
rg -n --hidden --glob '*.go' 'OnRequestHeaders|OnRequestBody|OnResponseHeaders|OnResponseBody|OnRequestBodyChunk|OnResponseBodyChunk' gateway/gateway-runtime/policy-engine/internal

# Look for any code in the policy engine that mutates the shared map or nested values
rg -n --hidden --glob '*.go' --glob '!**/*_test.go' 'spec\.Parameters\.Raw|Parameters\.Raw|\[[^]]+\]\s*=|append\(' gateway/gateway-runtime/policy-engine/internal

Repository: wso2/api-platform

Length of output: 11339

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the Parameters type and the Raw field
rg -n --hidden --glob '*.go' 'type\s+Parameters\b|Raw\s+map\[string\]interface\{\}' gateway/gateway-runtime/policy-engine/internal

# Read the file(s) that define Parameters in full enough to inspect the surrounding context
for f in $(rg -l --hidden --glob '*.go' 'type\s+Parameters\b|Raw\s+map\[string\]interface\{\}' gateway/gateway-runtime/policy-engine/internal); do
  echo "===== $f ====="
  wc -l "$f"
  cat -n "$f" | sed -n '1,260p'
done

# Inspect the policy callback interfaces and any wrappers around the params argument
rg -n --hidden --glob '*.go' 'type\s+.*interface\b|OnRequestHeaders|OnRequestBody|OnResponseHeaders|OnResponseBody|OnRequestBodyChunk|OnResponseBodyChunk' gateway/gateway-runtime/policy-engine/internal

Repository: wso2/api-platform

Length of output: 155

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find where spec.Parameters is defined and how Raw is typed
rg -n --hidden --glob '*.go' 'spec\.Parameters|Parameters\s+struct|Raw\s+map\[string\]interface\{\}|map\[string\]any|map\[string\]interface\{\}' gateway/gateway-runtime/policy-engine

# Find policy callback interface definitions and implementations
rg -n --hidden --glob '*.go' 'OnRequestHeaders\(|OnRequestBody\(|OnResponseHeaders\(|OnResponseBody\(|OnRequestBodyChunk\(|OnResponseBodyChunk\(' gateway/gateway-runtime/policy-engine

# Inspect likely definition files if found
for f in \
  $(rg -l --hidden --glob '*.go' 'spec\.Parameters|Raw\s+map\[string\]interface\{\}|map\[string\]any' gateway/gateway-runtime/policy-engine | head -n 20); do
  echo "===== $f ====="
  wc -l "$f"
  cat -n "$f" | sed -n '1,220p'
done

Repository: wso2/api-platform

Length of output: 50374

Shared spec.Parameters.Raw is still a mutable map[string]interface{}
chain.go passes the same map instance into every policy callback, but the policy API exposes it as a raw map with no defensive copy or read-only wrapper. Any mutation can leak state across requests or trigger concurrent map writes in the gateway. Replace it with a read-only accessor or clone/freeze params before dispatch.

🤖 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 `@gateway/gateway-runtime/policy-engine/internal/executor/chain.go` around
lines 143 - 145, Update the policy dispatch around OnRequestHeaders so policies
cannot mutate the shared spec.Parameters.Raw map: replace the raw-map argument
with the established read-only accessor, or create a defensive per-request clone
before invoking the callback. Preserve the existing parameter values while
ensuring concurrent requests never share a mutable map instance.

executionTime := time.Since(policyStartTime)

// Apply header mutations to reqCtx so subsequent policies and CEL conditions see the mutated state
Expand Down Expand Up @@ -308,16 +303,10 @@ func (c *ChainExecutor) ExecuteRequestPolicies(ctx context.Context, policyList [
}
}

// Deep-copy params to prevent a policy from mutating the shared spec map
// across concurrent requests (nested maps/slices require a full deep copy).
params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
slog.Debug("[body] calling OnRequestBody", "policy", spec.Name, "version", spec.Version, "route", route)
action := rp.OnRequestBody(ctx, reqCtx, params)
action := rp.OnRequestBody(ctx, reqCtx, spec.Parameters.Raw)
executionTime := time.Since(policyStartTime)

// Record policy execution metrics
Expand Down Expand Up @@ -467,13 +456,9 @@ func (c *ChainExecutor) ExecuteResponseHeaderPolicies(
}
}

params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

action := headerPol.OnResponseHeaders(ctx, respCtx, params)
// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
action := headerPol.OnResponseHeaders(ctx, respCtx, spec.Parameters.Raw)
executionTime := time.Since(policyStartTime)

// Apply header mutations to respCtx so subsequent policies and CEL conditions see the mutated state
Expand Down Expand Up @@ -629,16 +614,10 @@ func (c *ChainExecutor) ExecuteResponsePolicies(ctx context.Context, policyList
}
}

// Deep-copy params to prevent a policy from mutating the shared spec map
// across concurrent requests (nested maps/slices require a full deep copy).
params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
slog.Debug("[body] calling OnResponseBody", "policy", spec.Name, "version", spec.Version, "route", route)
action := rp.OnResponseBody(ctx, respCtx, params)
action := rp.OnResponseBody(ctx, respCtx, spec.Parameters.Raw)
executionTime := time.Since(policyStartTime)

// Record policy execution metrics
Expand Down Expand Up @@ -788,14 +767,10 @@ func (c *ChainExecutor) ExecuteStreamingRequestPolicies(
}
}

params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
slog.Debug("[streaming] calling OnRequestBodyChunk", "policy", spec.Name, "version", spec.Version, "route", route, "end_of_stream", currentChunk.EndOfStream)
action := streamingPol.OnRequestBodyChunk(ctx, reqCtx, currentChunk, params)
action := streamingPol.OnRequestBodyChunk(ctx, reqCtx, currentChunk, spec.Parameters.Raw)
executionTime := time.Since(policyStartTime)

metrics.PolicyExecutionsTotal.WithLabelValues(spec.Name, spec.Version, api, route, "executed").Inc()
Expand Down Expand Up @@ -936,14 +911,10 @@ func (c *ChainExecutor) ExecuteStreamingResponsePolicies(
}
}

params, err := deepCopyParams(spec.Parameters.Raw)
if err != nil {
span.End()
return nil, fmt.Errorf("failed to clone parameters for policy %s:%s: %w", spec.Name, spec.Version, err)
}

// spec.Parameters.Raw is an immutable snapshot published at chain-build time and
// shared read-only across concurrent requests; policies must not mutate it.
slog.Debug("[streaming] calling OnResponseBodyChunk", "policy", spec.Name, "version", spec.Version, "route", route, "end_of_stream", currentChunk.EndOfStream)
action := streamingPol.OnResponseBodyChunk(ctx, respCtx, currentChunk, params)
action := streamingPol.OnResponseBodyChunk(ctx, respCtx, currentChunk, spec.Parameters.Raw)
executionTime := time.Since(policyStartTime)

metrics.PolicyExecutionsTotal.WithLabelValues(spec.Name, spec.Version, api, route, "executed").Inc()
Expand Down Expand Up @@ -1066,22 +1037,6 @@ func applyResponseModifications(ctx *policy.ResponseContext, mods *policy.Downst
}
}

// deepCopyParams returns a deep copy of a map[string]interface{} via a JSON round-trip.
func deepCopyParams(src map[string]interface{}) (map[string]interface{}, error) {
if len(src) == 0 {
return make(map[string]interface{}), nil
}
b, err := json.Marshal(src)
if err != nil {
return nil, err
}
var dst map[string]interface{}
if err := json.Unmarshal(b, &dst); err != nil {
return nil, err
}
return dst, nil
}

// ─── ChainExecutor ────────────────────────────────────────────────────────────

// ChainExecutor represents the policy chain execution engine
Expand Down
56 changes: 0 additions & 56 deletions gateway/gateway-runtime/policy-engine/internal/executor/chain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,62 +744,6 @@ func TestResponseExecutionResult(t *testing.T) {
assert.Nil(t, result.FinalAction)
}

// =============================================================================
// Tests for deepCopyParams
// =============================================================================

// TestDeepCopyParams_MutationIsolation verifies that a policy mutating params
// during execution does not affect the original spec map or a concurrent request.
func TestDeepCopyParams_MutationIsolation(t *testing.T) {
original := map[string]interface{}{
"level1": map[string]interface{}{
"nested": "original",
},
}

copy1, err := deepCopyParams(original)
require.NoError(t, err)

// Simulate a policy mutating a nested value
copy1["level1"].(map[string]interface{})["nested"] = "mutated"

// A second copy (next request) should still see the original value
copy2, err := deepCopyParams(original)
require.NoError(t, err)

assert.Equal(t, "original", copy2["level1"].(map[string]interface{})["nested"],
"mutation of copy1 must not affect the source map used by copy2")
assert.Equal(t, "original", original["level1"].(map[string]interface{})["nested"],
"mutation of copy1 must not affect the original source map")
}

// TestDeepCopyParams_EmptyMap verifies that an empty map returns an empty copy.
func TestDeepCopyParams_EmptyMap(t *testing.T) {
result, err := deepCopyParams(map[string]interface{}{})
require.NoError(t, err)
assert.NotNil(t, result)
assert.Empty(t, result)
}

// TestDeepCopyParams_SliceIsolation verifies that slice values are also deep-copied.
func TestDeepCopyParams_SliceIsolation(t *testing.T) {
original := map[string]interface{}{
"items": []interface{}{"a", "b", "c"},
}

copy1, err := deepCopyParams(original)
require.NoError(t, err)

// Mutate the slice in copy1
copy1["items"] = []interface{}{"x", "y"}

copy2, err := deepCopyParams(original)
require.NoError(t, err)

assert.Equal(t, []interface{}{"a", "b", "c"}, copy2["items"],
"slice mutation in copy1 must not affect subsequent copies")
}

// TestExecuteResponsePolicies_ImmediateResponseShortCircuit verifies that a
// policy returning ImmediateResponse from OnResponse short-circuits the chain.
func TestExecuteResponsePolicies_ImmediateResponseShortCircuit(t *testing.T) {
Expand Down
Loading