Standardized Logging for Policy Developers #754
Conversation
WalkthroughIntroduces standardized policy logging by injecting a *slog.Logger into policy factories and adding helpers for request-scoped loggers; updates many policies to accept and use the logger. Removes request_header_mode configuration and hardcodes ext_proc processing modes. Multiple module dependency bumps and a VSCode launch argument were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Registry as Registry
participant LoggerSvc as Logger Creator
participant Factory as PolicyFactory
participant Policy as Policy Instance
participant Request as Request Handler
Registry->>LoggerSvc: create logger with policy name<br/>slog.Default().With("policy", name)
LoggerSvc-->>Registry: *slog.Logger (policy-scoped)
Registry->>Factory: invoke Factory(metadata, params, logger)
Factory->>Policy: initialize policy with logger field
Policy-->>Factory: return policy instance
Factory-->>Registry: policy ready
Registry-->>Request: return configured policy
Request->>Policy: OnRequest(ctx, params)
Policy->>Policy: derive scoped logger<br/>WithRequestID(p.logger, ctx.RequestID)
Policy-->>Request: RequestAction (logs via scoped logger)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
renuka-fernando
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
gateway/policies/advanced-ratelimit/v0.1.0/ratelimit.go (1)
264-279: Critical:loggerfield not assigned in struct initialization.The
loggerparameter is accepted inGetPolicybut never assigned to the returnedRateLimitPolicystruct. This will cause a nil pointer dereference panic whenOnRequestorOnResponsecallspolicy.WithRequestID(p.logger, ctx.RequestID).🐛 Proposed fix
// Return configured policy instance return &RateLimitPolicy{ + logger: logger, quotas: quotas, routeName: routeName, apiId: apiId,gateway/policies/model-weighted-round-robin/v0.1.0/weightedroundrobin.go (1)
528-556: Consider whether exposing internal error details in responses is acceptable.The error responses include raw error messages (e.g.,
err.Error()at lines 533, 543, 554) which could expose internal implementation details to clients. While helpful for debugging during development, this may not be desired in production.If this is intentional for this policy's use case, consider documenting that decision. Otherwise, consider using generic error messages for client-facing responses while logging the detailed errors.
gateway/system-policies/analytics/v0.1.0/analytics.go (1)
60-107: Logger injection is currently a no-op (and singleton blocks request-scoped logging).Line 65 adds a logger parameter, but it’s ignored and
slog.*is still used. Returning the sharedinsalso prevents request-scoped logger usage and would be racy if you later set the logger on it. Please instantiate a policy per GetPolicy call and route logs through the injected logger (with a safe fallback).🔧 Proposed fix
-type AnalyticsPolicy struct{} +type AnalyticsPolicy struct { + logger *slog.Logger +} -var ins = &AnalyticsPolicy{} - func GetPolicy( metadata policy.PolicyMetadata, params map[string]interface{}, logger *slog.Logger, ) (policy.Policy, error) { - return ins, nil + if logger == nil { + logger = slog.Default() + } + return &AnalyticsPolicy{logger: logger}, nil } @@ func (a *AnalyticsPolicy) OnRequest(ctx *policy.RequestContext, params map[string]interface{}) policy.RequestAction { - slog.Info("Analytics system policy: OnRequest called") + l := a.logger + if l == nil { + l = slog.Default() + } + l.Info("Analytics system policy: OnRequest called") @@ default: - slog.Error("Invalid API kind") + l.Error("Invalid API kind") } return nil } @@ func (p *AnalyticsPolicy) OnResponse(ctx *policy.ResponseContext, params map[string]interface{}) policy.ResponseAction { - slog.Info("Analytics system policy: OnResponse called") + l := p.logger + if l == nil { + l = slog.Default() + } + l.Info("Analytics system policy: OnResponse called") @@ - slog.Debug("API kind: ", "apiKind", apiKind) + l.Debug("API kind: ", "apiKind", apiKind) @@ - slog.Debug("Provider template param from policy: ", "providerTemplate", providerTemplate) + l.Debug("Provider template param from policy: ", "providerTemplate", providerTemplate) @@ - slog.Warn("Failed to extract LLM token info", "error", err) + l.Warn("Failed to extract LLM token info", "error", err) @@ - slog.Debug("Extracted LLM token info", + l.Debug("Extracted LLM token info", "promptTokens", tokenInfo.PromptTokens, "completionTokens", tokenInfo.CompletionTokens, "totalTokens", tokenInfo.TotalTokens, "remainingTokens", tokenInfo.RemainingTokens, "requestModel", tokenInfo.RequestModel, "responseModel", tokenInfo.ResponseModel, "providerName", tokenInfo.ProviderName, "providerDisplayName", tokenInfo.ProviderDisplayName, ) @@ default: - slog.Error("Invalid API kind") + l.Error("Invalid API kind") }gateway/policies/prompt-decorator/v0.1.0/promptdecorator.go (1)
169-172: Use a client-error status for empty request bodies.An empty body is a client input issue; returning 500 misclassifies it as a server fault. Consider a 4xx response for this branch.
🛠️ Suggested fix (example with 400)
- if ctx.Body == nil || len(content) == 0 { - return p.buildErrorResponse("Empty request body", nil) - } + if ctx.Body == nil || len(content) == 0 { + return policy.ImmediateResponse{ + StatusCode: 400, + Headers: map[string]string{ + "Content-Type": "application/json", + }, + Body: []byte(`{"type":"PROMPT_DECORATOR_ERROR","message":"Empty request body"}`), + } + }gateway/policies/semantic-cache/v0.1.0/semanticcache.go (1)
55-63: Guard against nil logger to prevent panic during initialization and request logging.
GetPolicystores the logger directly without validation and immediately callsp.logger.Debugat line 89. If a nil logger is passed, this causes an immediate panic. Additionally,OnRequest(line 318) andOnResponse(line 403) callpolicy.WithRequestID(p.logger, ...), which also panics if the logger is nil sinceWithRequestIDdoes not guard against nil receivers. Default toslog.Default()to ensure a safe fallback.🛠️ Suggested fix
- p := &SemanticCachePolicy{ - logger: logger, - } + if logger == nil { + logger = slog.Default() + } + p := &SemanticCachePolicy{ + logger: logger, + }Also applies to: 89-89
gateway/policies/mcp-auth/v0.1.0/mcp-auth.go (1)
41-58: Incomplete logger integration: logger parameter is received but not stored or used consistently.The
GetPolicyfunction accepts aloggerparameter but:
McpAuthPolicystruct has nologgerfield to store it- Only line 56 uses the injected logger; all other logging throughout the file uses the global
slogpackage directly (lines 80, 90, 94, 121, 162, 166, 192, 223, 232, 235, etc.)This defeats the PR's objective of standardized logging with automatic policy identification in logs.
Proposed fix: Add logger field and use it throughout
-type McpAuthPolicy struct{} +type McpAuthPolicy struct { + logger *slog.Logger +} -var ins = &McpAuthPolicy{} +var ins *McpAuthPolicy func GetPolicy( metadata policy.PolicyMetadata, params map[string]any, logger *slog.Logger, ) (policy.Policy, error) { + if ins == nil { + ins = &McpAuthPolicy{logger: logger} + } logger.Debug("GetPolicy called") return ins, nil }Then replace all
slog.Debug(...)calls withp.logger.Debug(...)throughout the file.
🤖 Fix all issues with AI agents
In `@gateway/policies/advanced-ratelimit/v0.1.0/go.mod`:
- Line 8: Update the SDK dependency versions across all policy modules so they
are consistent with advanced-ratelimit's go.mod
(github.com/wso2/api-platform/sdk v0.3.3); locate each policy's go.mod (e.g.,
basic-ratelimit, jwt-auth, mcp-auth, sentence-count-guardrail, url-guardrail,
word-count-guardrail, mcp-authz) and change any occurrences of v0.3.0 or v0.3.2
to v0.3.3, ensuring any local replace directives remain correct and running go
mod tidy in each module to verify dependency resolution.
In `@gateway/policies/aws-bedrock-guardrail/v0.1.0/awsbedrockguardrail.go`:
- Around line 61-73: GetPolicy stores the provided logger into
AWSBedrockGuardrailPolicy without nil-checking which can cause panics when later
calling p.logger.Debug or policy.WithRequestID; update GetPolicy to test if
logger == nil and if so set logger = slog.Default() (or return an error), then
assign it to the AWSBedrockGuardrailPolicy instance; also apply the same
nil-guard/default to other constructors/places that create
AWSBedrockGuardrailPolicy or call policy.WithRequestID so all uses (e.g., places
referenced by AWSBedrockGuardrailPolicy and policy.WithRequestID) never hold a
nil logger.
In `@gateway/policies/jwt-auth/v0.1.0/jwtauth.go`:
- Around line 98-105: GetPolicy currently ignores the provided logger and
returns the singleton ins that uses package-level slog calls in
OnRequest/OnResponse and helpers; modify the policy to carry a scoped logger by
adding a logger field to the policy instance returned by GetPolicy (e.g., set
ins.logger = logger or better: construct a new policy instance that embeds the
existing JWKS cache but has its own logger), replace all slog.* usages in
OnRequest, OnResponse and helper functions to use that instance logger, and if
ins must remain shared for the JWKS cache, extract the cache into a separate
struct (e.g., jwksCache) so you can instantiate per-policy wrappers with their
own logger while reusing the shared cache.
In `@gateway/policies/mcp-auth/v0.1.0/mcp-auth.go`:
- Around line 162-163: The policy forwards authentication to jwtauth.GetPolicy
but passes slog.Default() instead of the policy's provided logger, losing
context; update the MCP auth policy to store the incoming logger on the policy
struct (e.g., add a logger field on the struct used by GetPolicy creation) and
pass that stored logger to jwtauth.GetPolicy (replace slog.Default() with the
struct logger reference), ensuring the call that creates jwtPolicy (jwtPolicy, _
:= jwtauth.GetPolicy(...)) uses the forwarded logger so delegated logs retain
the policy-specific context.
In `@gateway/policies/mcp-authz/v0.1.0/mcp-authz.go`:
- Around line 77-83: GetPolicy receives a request-scoped logger but never stores
it on the McpAuthzPolicy, causing downstream methods to use global slog; modify
GetPolicy to set the provided logger on the McpAuthzPolicy instance and update
the policy struct to include a logger field (e.g., logger *slog.Logger), then
replace calls to slog.Debug()/global logger in OnRequest and all helper methods
with p.logger.Debug()/p.logger.<level> so they use the stored request-scoped
logger throughout the policy lifecycle.
In `@gateway/policies/url-guardrail/v0.1.0/go.mod`:
- Line 5: go.mod declares dependency github.com/wso2/api-platform/sdk at v0.3.2
but go.sum still has hashes for v0.3.1; update the module metadata by running a
module fetch/repair (for example run go get
github.com/wso2/api-platform/sdk@v0.3.2 and then go mod tidy) to regenerate
go.sum so the sdk version in go.mod and go.sum match; verify the updated entries
for github.com/wso2/api-platform/sdk now reference v0.3.2.
In `@gateway/system-policies/analytics/v0.1.0/go.mod`:
- Line 5: The go.mod was bumped to require github.com/wso2/api-platform/sdk
v0.3.2 but go.sum still only has v0.3.1 checksums; update go.sum by fetching the
new module checksums (e.g., run go get github.com/wso2/api-platform/sdk@v0.3.2
or go mod tidy / go mod download) so the v0.3.2 entries are added, then commit
the updated go.sum alongside the change to go.mod.
🧹 Nitpick comments (9)
gateway/policies/model-weighted-round-robin/v0.1.0/weightedroundrobin.go (2)
495-513: Consider deferring logger creation to reduce allocations.The logger created at line 496 is only used in the
defaultcase, while all other branches call helper methods that create their own request-scoped loggers. This results in an unnecessary allocation for the common paths.♻️ Suggested optimization
func (p *ModelWeightedRoundRobinPolicy) modifyRequestModel(ctx *policy.RequestContext, newModel string, requestID string) policy.RequestAction { - log := policy.WithRequestID(p.logger, requestID) location := p.params.RequestModel.Location identifier := p.params.RequestModel.Identifier switch location { case "payload": return p.modifyModelInPayload(ctx, newModel, identifier, requestID) case "header": return p.modifyModelInHeader(ctx, newModel, identifier, requestID) case "queryParam": return p.modifyModelInQueryParam(ctx, newModel, identifier, requestID) case "pathParam": return p.modifyModelInPathParam(ctx, newModel, identifier, requestID) default: + log := policy.WithRequestID(p.logger, requestID) log.Debug("Unsupported location", "location", location) return policy.UpstreamRequestModifications{} } }
650-655: Optional: Pre-compile regex pattern during policy initialization.The regex pattern is compiled on every request, but since it comes from configuration and doesn't change, it could be pre-compiled once in
GetPolicyand stored in the policy struct. This would avoid repeated compilation overhead for high-traffic scenarios.gateway/gateway-controller/pkg/xds/translator.go (1)
1849-1854: The hardcoded processing modes are appropriate for the target Envoy version.The chosen modes align well with the project's target Envoy version (1.36.0+).
FULL_DUPLEX_STREAMEDfor body processing was introduced in Envoy 1.34.0, so there are no version compatibility concerns.Consider adding a brief inline comment explaining why these specific modes were chosen (e.g., "streaming body support enables policy inspection and modification").
sdk/gateway/policy/v1alpha/logger.go (1)
5-16: Consider nil-safety forWithRequestIDto match defensive patterns used elsewhere in the codebase.The
slog.Logger.With()method is not nil-safe and will panic if called on a nil logger. While current usage passes initialized loggers fromPolicyFactory, adding a defensive guard aligns with nil-check patterns found in other parts of the codebase (e.g.,gateway/gateway-controller/pkg/utils/api_key.go,combined_cache.go).♻️ Suggested defensive guard
func WithRequestID(logger *slog.Logger, requestID string) *slog.Logger { + if logger == nil { + logger = slog.Default() + } return logger.With("requestId", requestID) }gateway/policies/basic-auth/v0.1.0/basicauth.go (1)
20-33: Consider emitting auth outcome via the injected logger (without credentials).The policy now receives a standardized logger but doesn’t use it. A small debug log on success/failure (without usernames/passwords) would align with the PR’s logging objective and help trace auth behavior.
💡 Example (no credential logging)
func (p *BasicAuthPolicy) handleAuthSuccess(ctx *policy.RequestContext, username string) policy.RequestAction { + if p.logger != nil { + log := policy.WithRequestID(p.logger, ctx.RequestID) + log.Debug("Basic auth success") + } // Set metadata indicating successful authentication ctx.Metadata[MetadataKeyAuthSuccess] = true ctx.Metadata[MetadataKeyAuthUser] = username ctx.Metadata[MetadataKeyAuthMethod] = "basic" @@ func (p *BasicAuthPolicy) handleAuthFailure(ctx *policy.RequestContext, allowUnauthenticated bool, realm string, reason string) policy.RequestAction { + if p.logger != nil { + log := policy.WithRequestID(p.logger, ctx.RequestID) + log.Debug("Basic auth failed", "reason", reason, "allowUnauthenticated", allowUnauthenticated) + } // Set metadata indicating failed authentication ctx.Metadata[MetadataKeyAuthSuccess] = false ctx.Metadata[MetadataKeyAuthMethod] = "basic"gateway/policies/json-schema-guardrail/v0.1.0/jsonschemaguardrail.go (1)
146-146: Consider type-safe alternatives to interface{} return with type assertions.The current pattern returns
interface{}fromvalidatePayloadand relies on type assertions in callers. While currently safe (theisResponseflag ensures correct types), this is fragile for future maintenance.Consider splitting into
validateRequestPayloadandvalidateResponsePayloadmethods that return the correct types directly, or using generics.Also applies to: 159-159
gateway/policies/modify-headers/v0.1.0/modifyheaders.go (1)
28-41: Logger field is added but never used.The
loggerfield is injected intoModifyHeadersPolicybut there are no logging statements in this policy. For consistency with other policies (e.g.,json-schema-guardrail,cors), consider adding debug logs for:
- Policy initialization in
GetPolicy- Header modifications in
OnRequest/OnResponseIf logging is intentionally omitted for this policy, consider documenting why or removing the unused field.
Example addition for GetPolicy
func GetPolicy( metadata policy.PolicyMetadata, params map[string]interface{}, logger *slog.Logger, ) (policy.Policy, error) { + logger.Debug("GetPolicy called") p := &ModifyHeadersPolicy{ logger: logger, } return p, nil }gateway/policies/cors/v0.1.0/cors.go (1)
151-152: Minor: Request-scoped logger created redundantly in helper methods.Both
handlePreflightandhandleNonPreflightcreate their own request-scoped logger, even though they're called fromOnRequestwhich already has one. Consider passing the logger as a parameter to avoid creating redundant logger instances.This is a minor optimization and the current approach keeps helper methods self-contained.
gateway/policy-engine/internal/registry/registry.go (1)
23-23: Consider adding policy version to logger context for disambiguation.Using only the policy name can make logs ambiguous when multiple versions are deployed simultaneously; adding version to the context keeps logs traceable.
Also applies to: 129-133
| func GetPolicy( | ||
| metadata policy.PolicyMetadata, | ||
| params map[string]interface{}, | ||
| logger *slog.Logger, | ||
| ) (policy.Policy, error) { | ||
| slog.Debug("JWT Auth Policy: GetPolicy called") | ||
| logger.Debug("GetPolicy called") | ||
| return ins, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the overall structure of the file and the GetPolicy function
cat -n gateway/policies/jwt-auth/v0.1.0/jwtauth.go | head -120Repository: wso2/api-platform
Length of output: 4404
🏁 Script executed:
# Track remaining global slog usage in this policy
rg -n '\bslog\.' gateway/policies/jwt-auth/v0.1.0/jwtauth.goRepository: wso2/api-platform
Length of output: 10792
🏁 Script executed:
# Check the OnRequest function to see what logger it's using
rg -A 20 'func.*OnRequest' gateway/policies/jwt-auth/v0.1.0/jwtauth.go | head -40Repository: wso2/api-platform
Length of output: 1239
🏁 Script executed:
# Check the policy struct definition to see if logger is stored
rg -B 5 -A 10 'type.*Policy struct' gateway/policies/jwt-auth/v0.1.0/jwtauth.goRepository: wso2/api-platform
Length of output: 466
🏁 Script executed:
# Verify the logger is never used after GetPolicy returns
rg -n 'logger\.' gateway/policies/jwt-auth/v0.1.0/jwtauth.goRepository: wso2/api-platform
Length of output: 98
Injected logger isn't propagated to request logs (objective gap).
GetPolicy accepts a logger but the policy continues to use global slog in OnRequest and helpers (90+ occurrences), so standardized/request-scoped logging is not achieved. The singleton ins also lacks a logger field, making it risky to store per-policy loggers without risk of cross-policy contamination.
Consider storing the logger on the policy instance and propagating it through OnRequest/OnResponse, replacing slog.* calls with the scoped logger. If ins must remain for shared JWKS cache, extract the cache into a separate struct to keep the logger instance-scoped.
🤖 Prompt for AI Agents
In `@gateway/policies/jwt-auth/v0.1.0/jwtauth.go` around lines 98 - 105, GetPolicy
currently ignores the provided logger and returns the singleton ins that uses
package-level slog calls in OnRequest/OnResponse and helpers; modify the policy
to carry a scoped logger by adding a logger field to the policy instance
returned by GetPolicy (e.g., set ins.logger = logger or better: construct a new
policy instance that embeds the existing JWKS cache but has its own logger),
replace all slog.* usages in OnRequest, OnResponse and helper functions to use
that instance logger, and if ins must remain shared for the JWKS cache, extract
the cache into a separate struct (e.g., jwksCache) so you can instantiate
per-policy wrappers with their own logger while reusing the shared cache.
| slog.Debug("MCP Auth Policy: Delegating authentication to JWT Auth Policy") | ||
| jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params) | ||
| jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, slog.Default()) |
There was a problem hiding this comment.
Logger not forwarded to delegated JWT auth policy.
slog.Default() is passed to jwtauth.GetPolicy instead of forwarding the logger that was provided to this policy. This breaks the standardized logging chain and loses any policy-specific context that would be attached to the logger.
Proposed fix
Store the logger in the struct (as suggested above), then:
- jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, slog.Default())
+ jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, p.logger)📝 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.
| slog.Debug("MCP Auth Policy: Delegating authentication to JWT Auth Policy") | |
| jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params) | |
| jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, slog.Default()) | |
| slog.Debug("MCP Auth Policy: Delegating authentication to JWT Auth Policy") | |
| jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, p.logger) |
🤖 Prompt for AI Agents
In `@gateway/policies/mcp-auth/v0.1.0/mcp-auth.go` around lines 162 - 163, The
policy forwards authentication to jwtauth.GetPolicy but passes slog.Default()
instead of the policy's provided logger, losing context; update the MCP auth
policy to store the incoming logger on the policy struct (e.g., add a logger
field on the struct used by GetPolicy creation) and pass that stored logger to
jwtauth.GetPolicy (replace slog.Default() with the struct logger reference),
ensuring the call that creates jwtPolicy (jwtPolicy, _ :=
jwtauth.GetPolicy(...)) uses the forwarded logger so delegated logs retain the
policy-specific context.
| func GetPolicy( | ||
| metadata policy.PolicyMetadata, | ||
| params map[string]any, | ||
| logger *slog.Logger, | ||
| ) (policy.Policy, error) { | ||
| slog.Debug("MCP Authorization Policy: GetPolicy called") | ||
| logger.Debug("GetPolicy called") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "mcp-authz.go" | head -20Repository: wso2/api-platform
Length of output: 109
🏁 Script executed:
rg -l "mcp-authz" --type go | head -20Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
fd "mcp-authz\.go$"Repository: wso2/api-platform
Length of output: 107
🏁 Script executed:
cat -n "gateway/policies/mcp-authz/v0.1.0/mcp-authz.go"Repository: wso2/api-platform
Length of output: 23584
Store logger in McpAuthzPolicy and use it throughout the policy lifecycle.
The logger parameter in GetPolicy is never stored in the McpAuthzPolicy struct, so logging in OnRequest and all helper methods falls back to global slog.Debug() calls. This loses the request-scoped logging context passed at initialization. Store the logger on the struct and propagate it through all methods for consistent, context-aware logging.
🔧 Suggested fix (store logger in policy)
type McpAuthzPolicy struct {
- Rules []Rule
+ Rules []Rule
+ logger *slog.Logger
}
func GetPolicy(
metadata policy.PolicyMetadata,
params map[string]any,
logger *slog.Logger,
) (policy.Policy, error) {
logger.Debug("GetPolicy called")
- p := &McpAuthzPolicy{}
+ p := &McpAuthzPolicy{
+ logger: logger,
+ }🤖 Prompt for AI Agents
In `@gateway/policies/mcp-authz/v0.1.0/mcp-authz.go` around lines 77 - 83,
GetPolicy receives a request-scoped logger but never stores it on the
McpAuthzPolicy, causing downstream methods to use global slog; modify GetPolicy
to set the provided logger on the McpAuthzPolicy instance and update the policy
struct to include a logger field (e.g., logger *slog.Logger), then replace calls
to slog.Debug()/global logger in OnRequest and all helper methods with
p.logger.Debug()/p.logger.<level> so they use the stored request-scoped logger
throughout the policy lifecycle.
| go 1.25.1 | ||
|
|
||
| require github.com/wso2/api-platform/sdk v0.3.1 | ||
| require github.com/wso2/api-platform/sdk v0.3.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/wso2/api-platform/sdk releases versions changelog
💡 Result:
I found the WSO2 API Platform repository and its Releases (which include release notes/changelogs) on GitHub:
- Repository (includes the sdk/ folder): https://github.com/wso2/api-platform. [1]
- Releases / changelogs page (release notes per tag): https://github.com/wso2/api-platform/releases. [2]
Open the Releases page to see available versions, tags, and each release’s changelog/notes. References: [1], [2].
🏁 Script executed:
# Search for PR `#754` references in the codebase
rg "754" --type-list | head -20
cd gateway/policies/url-guardrail/v0.1.0 && cat go.modRepository: wso2/api-platform
Length of output: 582
🏁 Script executed:
# Search for logging-related changes in the policy module
rg "logging|log\." gateway/policies/url-guardrail/ -A 2 -B 2Repository: wso2/api-platform
Length of output: 2968
🏁 Script executed:
# Check if there are other SDK version references in the repo
rg "wso2/api-platform/sdk" -A 1 -B 1Repository: wso2/api-platform
Length of output: 31626
Resolve go.mod/go.sum version mismatch for SDK dependency.
The go.mod specifies github.com/wso2/api-platform/sdk v0.3.2, but go.sum contains a hash for v0.3.1. These versions must match. Run go get -u or explicitly update go.sum to align with the intended v0.3.2 version declared in go.mod.
🤖 Prompt for AI Agents
In `@gateway/policies/url-guardrail/v0.1.0/go.mod` at line 5, go.mod declares
dependency github.com/wso2/api-platform/sdk at v0.3.2 but go.sum still has
hashes for v0.3.1; update the module metadata by running a module fetch/repair
(for example run go get github.com/wso2/api-platform/sdk@v0.3.2 and then go mod
tidy) to regenerate go.sum so the sdk version in go.mod and go.sum match; verify
the updated entries for github.com/wso2/api-platform/sdk now reference v0.3.2.
| go 1.25.1 | ||
|
|
||
| require github.com/wso2/api-platform/sdk v0.3.1 | ||
| require github.com/wso2/api-platform/sdk v0.3.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if go.mod file exists and contains the new SDK version
if [ -f "gateway/system-policies/analytics/v0.1.0/go.mod" ]; then
echo "=== go.mod content ==="
cat -n "gateway/system-policies/analytics/v0.1.0/go.mod"
echo ""
fi
# Check for corresponding go.sum file
if [ -f "gateway/system-policies/analytics/v0.1.0/go.sum" ]; then
echo "=== go.sum entries for SDK v0.3.2 ==="
rg -n "github.com/wso2/api-platform/sdk v0.3.2" "gateway/system-policies/analytics/v0.1.0/go.sum"
echo ""
echo "=== All SDK-related entries in go.sum ==="
rg -n "github.com/wso2/api-platform/sdk" "gateway/system-policies/analytics/v0.1.0/go.sum"
else
echo "go.sum file not found at gateway/system-policies/analytics/v0.1.0/go.sum"
fiRepository: wso2/api-platform
Length of output: 505
Update go.sum with v0.3.2 checksums.
The go.mod specifies github.com/wso2/api-platform/sdk v0.3.2, but go.sum only contains checksums for v0.3.1. This mismatch will cause builds with -mod=readonly to fail. The v0.3.2 checksums must be added to go.sum.
🤖 Prompt for AI Agents
In `@gateway/system-policies/analytics/v0.1.0/go.mod` at line 5, The go.mod was
bumped to require github.com/wso2/api-platform/sdk v0.3.2 but go.sum still only
has v0.3.1 checksums; update go.sum by fetching the new module checksums (e.g.,
run go get github.com/wso2/api-platform/sdk@v0.3.2 or go mod tidy / go mod
download) so the v0.3.2 entries are added, then commit the updated go.sum
alongside the change to go.mod.
7fd751f to
5b70c6e
Compare
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 (3)
gateway/policies/advanced-ratelimit/v0.1.0/ratelimit.go (1)
71-279: Initialize and persist the injected logger to avoid nil deref.
GetPolicynow acceptsloggerand uses it before the policy is created, but the returnedRateLimitPolicydoes not store it. That leavesp.loggernil and causes a panic inpolicy.WithRequestID(p.logger, ...)(and inlogger.Warn/Debugif caller passes nil). Please default and persist the logger.🛠️ Proposed fix
func GetPolicy( metadata policy.PolicyMetadata, params map[string]interface{}, logger *slog.Logger, ) (policy.Policy, error) { + if logger == nil { + logger = slog.Default() + } // Store route name for default key routeName := metadata.RouteName if routeName == "" { routeName = "unknown-route" } @@ // Return configured policy instance return &RateLimitPolicy{ + logger: logger, quotas: quotas, routeName: routeName, apiId: apiId, apiName: apiName, apiVersion: apiVersion,gateway/policies/model-round-robin/v0.1.0/roundrobin.go (1)
230-261: Remove racing field from log statement.
p.currentIndexis read at line 258 without mutex protection, while it can be modified concurrently at line 303 (inselectNextAvailableModel) under the mutex. This creates a data race condition under concurrent requests. The simplest fix is to remove the index from the log.- log.Debug("Selected model", "model", selectedModel.Model, "index", p.currentIndex) + log.Debug("Selected model", "model", selectedModel.Model)gateway/policies/model-weighted-round-robin/v0.1.0/weightedroundrobin.go (1)
260-291: Remove racycurrentIndexfrom log statement.
p.currentIndexis incremented inside the mutex withinselectNextAvailableWeightedModel(), but read outside the lock at line 288. With concurrent requests, this causes a data race. Remove the field from the debug log.Fix
- log.Debug("Selected model", "model", selectedModel.Model, "weight", selectedModel.Weight, "index", p.currentIndex) + log.Debug("Selected model", "model", selectedModel.Model, "weight", selectedModel.Weight)
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 1849-1856: The ProcessingMode block in translator.go currently
uses extproc.ProcessingMode_FULL_DUPLEX_STREAMED for RequestBodyMode and
ResponseBodyMode which prevents ModeOverride messages; change those two to
extproc.ProcessingMode_BUFFERED (or SKIP for headers-only flows) so Envoy can
accept per-request ModeOverride from the policy engine, remove the hardcoded
FULL_DUPLEX_STREAMED constraint, and keep AllowModeOverride=true so
getModeOverride() can dynamically switch to BUFFERED or NONE as needed.
In `@gateway/policies/prompt-template/v0.1.0/prompttemplate.go`:
- Around line 23-26: GetPolicy can receive a nil logger which causes parseParams
(calls logger.Debug) and WithRequestID (calls logger.With) to panic; update
GetPolicy to guard the logger field by defaulting nil to slog.Default (or
equivalent) before constructing PromptTemplatePolicy so all uses in parseParams
and WithRequestID are safe—ensure PromptTemplatePolicy.logger is set to the
non-nil logger, then continue to set params via parseParams and use
WithRequestID without additional nil checks.
In `@gateway/policies/sentence-count-guardrail/v0.1.0/sentencecountguardrail.go`:
- Line 28: The struct's logger field may be nil which will cause panics when
calling p.logger.Debug and policy.WithRequestID(p.logger, ...) (which calls
.With()) — fix by normalizing the logger on construction/assignment: if the
provided logger is nil, replace it with a safe no-op or default slog.Logger
(e.g., slog.Default() or a new logger) before storing in the struct so all uses
(p.logger.Debug, policy.WithRequestID, any .With calls) are safe; update the
constructor/initializer that sets logger and any code paths that assign to the
logger field to perform this nil-check and substitution.
In `@gateway/policies/word-count-guardrail/v0.1.0/wordcountguardrail.go`:
- Around line 27-29: The policy currently stores a potentially nil *slog.Logger
which can cause a panic when later used (e.g., policy.WithRequestID(p.logger,
...)); update the code that constructs/initializes WordCountGuardrailPolicy (and
any places assigning to the logger field) to check for nil and set policy.logger
= slog.Default() when nil (or return an error if you prefer), so every use of
p.logger (including WithRequestID) is safe; apply the same nil-check/defaulting
pattern around assignments to WordCountGuardrailPolicy.logger in the
constructor/initializers referenced (the struct, its constructor/initializer
functions and any setter locations).
♻️ Duplicate comments (5)
gateway/policies/url-guardrail/v0.1.0/go.mod (1)
5-5: Ensure go.sum matches the updated SDK version.If not already done, update
go.sumso it aligns withgithub.com/wso2/api-platform/sdk v0.3.2.#!/bin/bash # Verify that go.sum entries match the required sdk version. rg -n 'github.com/wso2/api-platform/sdk' -C2 go.sumgateway/policies/mcp-authz/v0.1.0/mcp-authz.go (1)
73-95: Store the injected logger on the policy and use it for request-scoped logs.
GetPolicyreceives a logger but it is not retained, so downstream logging still uses globalslog. This defeats the standardized, request-scoped logging goal. Please store the logger onMcpAuthzPolicyand route logs through it (ideally viapolicy.WithRequestIDin request paths).🔧 Minimal fix for logger retention
type McpAuthzPolicy struct { - Rules []Rule + Rules []Rule + logger *slog.Logger } func GetPolicy( metadata policy.PolicyMetadata, params map[string]any, logger *slog.Logger, ) (policy.Policy, error) { logger.Debug("GetPolicy called") - p := &McpAuthzPolicy{} + p := &McpAuthzPolicy{ + logger: logger, + }gateway/policies/jwt-auth/v0.1.0/jwtauth.go (1)
98-104: Injected logger still not used for request logs (standardized logging gap).Line 103 logs once, but the policy returns the singleton
insthat continues to use package-levelslogthroughoutOnRequestand helpers, so request-scoped logging isn’t achieved. Consider storing a logger per policy instance (or a wrapper with shared cache) and replacingslog.*calls.gateway/policies/mcp-auth/v0.1.0/mcp-auth.go (1)
51-57: Forward injected logger to JWT policy (and store it).Line 163 still uses
slog.Default(), so delegated logs lose the policy context; the logger parameter inGetPolicyis not preserved.🛠️ Possible fix
-type McpAuthPolicy struct{} +type McpAuthPolicy struct { + logger *slog.Logger +} -func GetPolicy( - metadata policy.PolicyMetadata, - params map[string]any, - logger *slog.Logger, -) (policy.Policy, error) { - logger.Debug("GetPolicy called") - return ins, nil -} +func GetPolicy( + metadata policy.PolicyMetadata, + params map[string]any, + logger *slog.Logger, +) (policy.Policy, error) { + if logger == nil { + logger = slog.Default() + } + logger.Debug("GetPolicy called") + return &McpAuthPolicy{logger: logger}, nil +}- jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, slog.Default()) + jwtPolicy, _ := jwtauth.GetPolicy(policy.PolicyMetadata{}, params, p.logger)Also applies to: 162-164
gateway/policies/aws-bedrock-guardrail/v0.1.0/awsbedrockguardrail.go (1)
31-35: Guard against nil logger to avoid panic.If the factory passes
nil,p.logger.Debug(...)(Line 138) andpolicy.WithRequestID(p.logger, ...)(Line 334) will panic. Defaulting toslog.Default()(or returning an error) avoids this.🛠️ Suggested fix
func GetPolicy( metadata policy.PolicyMetadata, params map[string]interface{}, logger *slog.Logger, ) (policy.Policy, error) { + if logger == nil { + logger = slog.Default() + } // Validate and extract static configuration from params if err := validateAWSConfigParams(params); err != nil { return nil, fmt.Errorf("invalid params: %w", err) } p := &AWSBedrockGuardrailPolicy{ logger: logger, region: getStringParam(params, "region"), guardrailID: getStringParam(params, "guardrailID"), guardrailVersion: getStringParam(params, "guardrailVersion"), }Also applies to: 61-75, 138-138, 316-329, 333-441
🧹 Nitpick comments (3)
gateway/policies/pii-masking-regex/v0.1.0/piimaskingregex.go (2)
96-96: Consider precompiling regex patterns for better performance.The pattern
^[A-Z_]+$is compiled on every iteration of the validation loop. Since this is a constant pattern, precompile it as a package-level variable similar totextCleanRegexCompiledon line 22.♻️ Suggested improvement
Add at package level:
var piiEntityNamePattern = regexp.MustCompile(`^[A-Z_]+$`)Then use:
- if !regexp.MustCompile(`^[A-Z_]+$`).MatchString(piiEntity) { + if !piiEntityNamePattern.MatchString(piiEntity) {
251-251: Precompile placeholder pattern at package level.This pattern is compiled on every call to
maskPIIFromContent. Since it's constant, move it to a package-level variable for efficiency.♻️ Suggested improvement
Add at package level:
var placeholderPattern = regexp.MustCompile(`^\[[A-Z_]+_[0-9a-f]{4}\]$`)Then remove the local declaration at line 251.
gateway/policies/mcp-auth/v0.1.0/go.mod (1)
5-5: Confirm SDK version alignment with the rest of the policy modules.
Most policies in this PR appear to be onv0.3.3; this one remainsv0.3.2. Please verify this is intentional to avoid SDK interface drift across policy modules; otherwise, align tov0.3.3.♻️ Possible alignment update
-require github.com/wso2/api-platform/sdk v0.3.2 +require github.com/wso2/api-platform/sdk v0.3.3
| ProcessingMode: &extproc.ProcessingMode{ | ||
| RequestHeaderMode: requestHeaderMode, | ||
| RequestHeaderMode: extproc.ProcessingMode_SEND, | ||
| RequestBodyMode: extproc.ProcessingMode_FULL_DUPLEX_STREAMED, | ||
| RequestTrailerMode: extproc.ProcessingMode_SEND, | ||
| ResponseHeaderMode: extproc.ProcessingMode_SEND, | ||
| ResponseBodyMode: extproc.ProcessingMode_FULL_DUPLEX_STREAMED, | ||
| ResponseTrailerMode: extproc.ProcessingMode_SEND, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "translator.go" -path "*/xds/*" 2>/dev/null | head -5Repository: wso2/api-platform
Length of output: 178
🏁 Script executed:
# First, let's locate the file
git ls-files | grep -E "gateway.*translator\.go$"Repository: wso2/api-platform
Length of output: 161
🏁 Script executed:
# Let's get the file structure to understand the context better
fd translator.go | grep -E "xds|gateway"Repository: wso2/api-platform
Length of output: 161
🏁 Script executed:
# Let's examine the specific file and lines
if [ -f "gateway/gateway-controller/pkg/xds/translator.go" ]; then
wc -l gateway/gateway-controller/pkg/xds/translator.go
fiRepository: wso2/api-platform
Length of output: 114
🏁 Script executed:
# Read the relevant section around lines 1849-1856
if [ -f "gateway/gateway-controller/pkg/xds/translator.go" ]; then
sed -n '1820,1880p' gateway/gateway-controller/pkg/xds/translator.go | cat -n
fiRepository: wso2/api-platform
Length of output: 2913
🏁 Script executed:
# Check function definition to understand context
if [ -f "gateway/gateway-controller/pkg/xds/translator.go" ]; then
sed -n '1800,1870p' gateway/gateway-controller/pkg/xds/translator.go | cat -n
fiRepository: wso2/api-platform
Length of output: 3252
🏁 Script executed:
# Search for FULL_DUPLEX_STREAMED usage in the codebase
rg "FULL_DUPLEX_STREAMED" --type goRepository: wso2/api-platform
Length of output: 296
🏁 Script executed:
# Search for ProcessingMode configuration patterns
rg "ProcessingMode" --type go -A 3 -B 1Repository: wso2/api-platform
Length of output: 28131
🏁 Script executed:
# Check for any policy-related configuration or mode_override patterns
rg "mode_override|request_header_mode|body.*mode" --type go -iRepository: wso2/api-platform
Length of output: 14250
🏁 Script executed:
# Check if AllowModeOverride is actually configured/used
rg "AllowModeOverride" --type go -B 2 -A 2Repository: wso2/api-platform
Length of output: 1579
🏁 Script executed:
# Look for ModeOverride usage in the policy engine
rg "ModeOverride" --type go -B 3 -A 3Repository: wso2/api-platform
Length of output: 5122
🏁 Script executed:
# Check the config structure to understand AllowModeOverride
rg "type.*PolicyEngine|type Config" --type go gateway/gateway-controller/pkg/config/ | head -20Repository: wso2/api-platform
Length of output: 288
🏁 Script executed:
# Look for where the mode override is actually sent to Envoy
rg "extproc.*ProcessingMode|ModeOverride" --type go -A 5 | head -50Repository: wso2/api-platform
Length of output: 4446
🌐 Web query:
Envoy ext_proc ProcessingMode FULL_DUPLEX_STREAMED vs BUFFERED modes documentation
💡 Result:
Short summary — main differences
-
BUFFERED: Envoy buffers the entire request/response body (up to configured limits) and sends it to the external processor as a single message. If the buffer limit is exceeded the body is truncated or an error is produced. Use when the processor needs the whole body at once (e.g., full-body inspection/mutation). [1][2]
-
FULL_DUPLEX_STREAMED: Body data is sent as a bidirectional stream of chunks (full‑duplex). Envoy and the external processor can concurrently stream body chunks and the processor may send streamed responses (including streamed immediate responses). This mode disables some per‑message semantics (different response rules, different timeout behavior) and is intended for low‑latency, streaming interactions where processor can act on chunks as they arrive. Mode overrides are not supported for FULL_DUPLEX_STREAMED. [1][3]
Notable operational details
- BUFFERED requires correct handling of content-length for mutated bodies (Envoy may allow content-length only in certain buffered+send combinations; mismatches cause rejection). FULL_DUPLEX_STREAMED (and several other streamed modes) will commonly cause Envoy to remove Content-Length and use chunked transfer encoding. [2]
- Message timeouts / response sequencing differ: when not FULL_DUPLEX_STREAMED, Envoy expects one ProcessingResponse per ProcessingRequest; FULL_DUPLEX_STREAMED follows its own streaming API and has different timeout/response rules. Observability mode and flow-control warnings also apply. [1][3]
Sources
- Envoy ext_proc filter docs / ext_proc.proto (ProcessingMode and behavior). [1]
- processing_mode.proto (BodySendMode enum descriptions — BUFFERED, FULL_DUPLEX_STREAMED, and content-length notes). [2]
- external_processor.proto (ProtocolConfiguration and ProcessingResponse rules for FULL_DUPLEX_STREAMED). [3]
Change initial ProcessingMode to support dynamic mode overrides.
The hardcoded ProcessingMode_FULL_DUPLEX_STREAMED for body modes prevents Envoy from accepting ModeOverride messages, which breaks the policy-engine's dynamic optimization mechanism.
Per Envoy ext_proc documentation: mode overrides are not supported when initial body mode is FULL_DUPLEX_STREAMED. Since your config sets AllowModeOverride: true and the policy-engine implements intelligent getModeOverride() (returning BUFFERED only when policies need body access, NONE otherwise), the initial mode should be:
ProcessingMode_BUFFERED(orSKIPfor headers-only) — allows per-requestModeOverridemessages- Remove the hardcoded
FULL_DUPLEX_STREAMEDconstraint
This way, header-only policies (authentication, rate-limiting) will skip body buffering, and only policies requiring body inspection will trigger buffering. With current config, all requests incur full-duplex streaming overhead regardless of actual policy requirements.
Code location
ProcessingMode: &extproc.ProcessingMode{
RequestHeaderMode: extproc.ProcessingMode_SEND,
RequestBodyMode: extproc.ProcessingMode_FULL_DUPLEX_STREAMED, // ← change to BUFFERED
RequestTrailerMode: extproc.ProcessingMode_SEND,
ResponseHeaderMode: extproc.ProcessingMode_SEND,
ResponseBodyMode: extproc.ProcessingMode_FULL_DUPLEX_STREAMED, // ← change to BUFFERED
ResponseTrailerMode: extproc.ProcessingMode_SEND,
},
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/xds/translator.go` around lines 1849 - 1856,
The ProcessingMode block in translator.go currently uses
extproc.ProcessingMode_FULL_DUPLEX_STREAMED for RequestBodyMode and
ResponseBodyMode which prevents ModeOverride messages; change those two to
extproc.ProcessingMode_BUFFERED (or SKIP for headers-only flows) so Envoy can
accept per-request ModeOverride from the policy engine, remove the hardcoded
FULL_DUPLEX_STREAMED constraint, and keep AllowModeOverride=true so
getModeOverride() can dynamically switch to BUFFERED or NONE as needed.
gateway/policies/sentence-count-guardrail/v0.1.0/sentencecountguardrail.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@gateway/policies/advanced-ratelimit/v0.1.0/ratelimit.go`:
- Around line 72-76: The logger may be nil when GetPolicy is entered and
logger.Warn(...) is called before it's ensured; call policy.EnsureLogger(logger)
at the start of GetPolicy (immediately after the function params) to guarantee a
non-nil logger, then replace subsequent calls that re-run EnsureLogger (the
later EnsureLogger call near policy creation) by using the already-initialized
logger directly; update references to logger.Warn(...) and other logger uses to
assume the ensured logger.
- Around line 233-237: The logger is used before it's guaranteed non-nil in
GetPolicy (e.g., in the q.Limiter cached branch where logger.Debug is called and
later at the other debug calls); call EnsureLogger at the start of GetPolicy and
use the returned/ensured logger value for all subsequent logging instead of the
original logger variable (update usages around q.Limiter assignment and the
debug calls in the cached/else branches to reference the ensured logger).
In `@gateway/policies/model-weighted-round-robin/v0.1.0/go.mod`:
- Around line 3-5: The go.mod currently requires a non-existent tag
"github.com/wso2/api-platform/sdk v0.3.5"; update the require line in go.mod to
point to a published tag for that module (for example replace v0.3.5 with an
available release such as v0.3.4 or the repository's latest stable tag) so that
`require github.com/wso2/api-platform/sdk` references a valid version; run `go
mod tidy` afterwards to verify and update go.sum.
In `@gateway/policies/semantic-cache/v0.1.0/go.mod`:
- Around line 7-8: Remove the invalid duplicate YAML module entry in go.mod:
delete the line declaring "go.yaml.in/yaml/v2 v2.4.2" so only the canonical
"gopkg.in/yaml.v2 v2.4.0" remains; keep the prometheus entries and the
intentional SDK bump "github.com/wso2/api-platform/sdk v0.3.5" as-is.
♻️ Duplicate comments (5)
gateway/policies/mcp-authz/v0.1.0/mcp-authz.go (2)
80-95: Logger injection is correctly implemented inGetPolicy, but the storage issue remains.The addition of the
loggerparameter and the use ofpolicy.EnsureLogger(logger)correctly follows the SDK pattern. The logger is properly used for the two debug calls withinGetPolicy.However, as noted in the existing review comment, the logger is not stored in the
McpAuthzPolicystruct, so all calls inOnRequestand helper methods (lines 218-271, 322-329, 338-341, 362-380, 424-441, 455-477, 510-570, 595-597) still use the globalslog.Debug(). This defeats the purpose of the standardized logging initiative.
216-273:OnRequestand downstream methods use globalsloginstead of the policy logger.Once the logger is stored in the
McpAuthzPolicystruct as suggested in the existing review, allslog.Debug()calls inOnRequestand helper methods (checkAuthorization,findMatchingRules,checkClaims,checkScopes,generateResourcePath,generateWwwAuthenticateHeader,handleAuthFailure) should be updated to usep.logger.Debug()for consistent, context-aware logging.Additionally, consider using
policy.WithRequestID(logger, ctx.RequestID)at the start ofOnRequestto include the request ID in all subsequent log entries, as other policies in this PR appear to do.gateway/system-policies/analytics/v0.1.0/go.mod (1)
5-5: Ensure go.sum is updated with v0.3.5 checksums before merging.The go.mod file requires SDK v0.3.5, but go.sum contains only v0.3.4 checksums. This mismatch will cause build failures. Run
go mod tidyorgo mod downloadto sync go.sum with the required v0.3.5 version, which exists as a valid release (tagsdk/v0.3.5).gateway/policies/jwt-auth/v0.1.0/jwtauth.go (1)
98-104: Injected logger still isn’t propagated to policy/request logs.Same issue as earlier: GetPolicy accepts a logger but the singleton policy continues to rely on global
slogelsewhere, so standardized/request-scoped logging isn’t achieved.gateway/policies/prompt-template/v0.1.0/prompttemplate.go (1)
44-49: Passp.loggerinstead of rawloggertoparseParamsto avoid potential nil dereference.At line 45,
policy.EnsureLogger(logger)guaranteesp.loggeris non-nil. However, line 49 passes the originalloggerparameter (which may be nil) toparseParams, which then callslogger.Debug()at line 117 without a nil guard. This will panic ifGetPolicyis called with a nil logger.Proposed fix
p := &PromptTemplatePolicy{ logger: policy.EnsureLogger(logger), } // Parse parameters - policyParams, err := parseParams(params, logger) + policyParams, err := parseParams(params, p.logger)
🧹 Nitpick comments (4)
gateway/policies/prompt-decorator/v0.1.0/promptdecorator.go (2)
329-331: Consider passing the per-request logger instead of requestID to avoid redundant logger creation.Both
updateStringAtPathandupdateArrayAtPathcreate new per-request loggers viapolicy.WithRequestID(p.logger, requestID), even though the caller (OnRequest) already has a per-request logger. Passing the logger directly would be more efficient and cleaner.♻️ Suggested refactor
-func (p *PromptDecoratorPolicy) updateStringAtPath(payloadData map[string]interface{}, jsonPath string, value string, requestID string) policy.RequestAction { - log := policy.WithRequestID(p.logger, requestID) +func (p *PromptDecoratorPolicy) updateStringAtPath(payloadData map[string]interface{}, jsonPath string, value string, log *slog.Logger) policy.RequestAction { -func (p *PromptDecoratorPolicy) updateArrayAtPath(payloadData map[string]interface{}, jsonPath string, value []map[string]interface{}, requestID string) policy.RequestAction { - log := policy.WithRequestID(p.logger, requestID) +func (p *PromptDecoratorPolicy) updateArrayAtPath(payloadData map[string]interface{}, jsonPath string, value []map[string]interface{}, log *slog.Logger) policy.RequestAction {Then update the call sites in
OnRequestto passlogdirectly.Also applies to: 371-373
169-172: Early return for empty body returns a 500 error without logging.When the request body is nil or empty,
buildErrorResponseis called but the error is not logged. Consider adding a debug log for consistency with other error paths.♻️ Suggested addition
// Check for empty or nil content before unmarshaling if ctx.Body == nil || len(content) == 0 { + log.Debug("Empty request body") return p.buildErrorResponse("Empty request body", nil) }gateway/policies/cors/v0.1.0/cors.go (1)
133-148: Consider passing the per-request logger to helper methods.
OnRequestcreates a per-request logger at line 134, buthandlePreflight(line 153) andhandleNonPreflight(line 281) each create their own per-request logger. Similarly,OnResponse(line 267) creates one. Since these helpers are always called from methods that already have a per-request logger, consider passing it as a parameter to avoid redundant logger creation.Also applies to: 151-153, 266-276, 279-281
gateway/policies/api-key-auth/v0.1.0/apikey.go (1)
205-227: Consider accepting per-request logger as parameter in helper methods.Both
handleAuthSuccessandhandleAuthFailurecreate their own per-request loggers viapolicy.WithRequestID(p.logger, ctx.RequestID), but they are always called fromOnRequestwhich already has a per-request logger. Passing the logger as a parameter would be cleaner.Also applies to: 234-278
| func GetPolicy( | ||
| metadata policy.PolicyMetadata, | ||
| params map[string]interface{}, | ||
| logger *slog.Logger, | ||
| ) (policy.Policy, error) { |
There was a problem hiding this comment.
Logger used before being ensured — potential nil pointer dereference.
At line 180, logger.Warn(...) is called, but policy.EnsureLogger(logger) isn't called until line 265 when the policy is created. If a nil logger is passed to GetPolicy, this will panic.
Move the EnsureLogger call to the beginning of the function.
🛠️ Suggested fix
func GetPolicy(
metadata policy.PolicyMetadata,
params map[string]interface{},
logger *slog.Logger,
) (policy.Policy, error) {
+ logger = policy.EnsureLogger(logger)
+
// Store route name for default key
routeName := metadata.RouteNameThen at line 265, simply use logger directly:
return &RateLimitPolicy{
- logger: policy.EnsureLogger(logger),
+ logger: logger,Also applies to: 180-181
🤖 Prompt for AI Agents
In `@gateway/policies/advanced-ratelimit/v0.1.0/ratelimit.go` around lines 72 -
76, The logger may be nil when GetPolicy is entered and logger.Warn(...) is
called before it's ensured; call policy.EnsureLogger(logger) at the start of
GetPolicy (immediately after the function params) to guarantee a non-nil logger,
then replace subsequent calls that re-run EnsureLogger (the later EnsureLogger
call near policy creation) by using the already-initialized logger directly;
update references to logger.Warn(...) and other logger uses to assume the
ensured logger.
| q.Limiter = cached.(limiter.Limiter) | ||
| slog.Debug("Reusing cached memory limiter", | ||
| logger.Debug("Reusing cached memory limiter", | ||
| "route", routeName, "apiName", apiName, | ||
| "quota", q.Name, "cacheKey", cacheKey[:16]) | ||
| } else { |
There was a problem hiding this comment.
Same nil logger issue affects these log calls.
Lines 234 and 256 also use logger directly before EnsureLogger is called. These will also panic if a nil logger is passed. Fixing the issue at the start of GetPolicy (as suggested above) will address all these cases.
Also applies to: 255-259
🤖 Prompt for AI Agents
In `@gateway/policies/advanced-ratelimit/v0.1.0/ratelimit.go` around lines 233 -
237, The logger is used before it's guaranteed non-nil in GetPolicy (e.g., in
the q.Limiter cached branch where logger.Debug is called and later at the other
debug calls); call EnsureLogger at the start of GetPolicy and use the
returned/ensured logger value for all subsequent logging instead of the original
logger variable (update usages around q.Limiter assignment and the debug calls
in the cached/else branches to reference the ensured logger).
| go 1.25.1 | ||
|
|
||
| require github.com/wso2/api-platform/sdk v0.3.1 | ||
| require github.com/wso2/api-platform/sdk v0.3.5 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go 1.25.1 release
💡 Result:
Go 1.25.1 — released September 3, 2025. It’s a minor/security patch for 1.25 that includes security fixes to net/http and bug fixes to the go command, net, os, os/exec, and testing/synctest packages (see Go 1.25.1 milestone/release notes). [1][2]
If you need links to the full release notes, changelog, or downloads, tell me which (notes, security advisories/CVEs, or binaries/source). [1] Go release history. [2] SUSE changelog summarizing 1.25.1 fixes.
🌐 Web query:
github.com/wso2/api-platform/sdk v0.3.5 release tag
💡 Result:
I couldn't find a git tag/release named v0.3.5 for github.com/wso2/api-platform/sdk. The SDK lives under the wso2/api-platform monorepo (see pkg.go.dev showing the sdk path and noting no tagged version), and WSO2's GitHub org repositories list but no sdk v0.3.5 release. [1][2]
Sources:
- pkg.go.dev listing for github.com/wso2/api-platform/sdk (shows no tagged version). [1]
- WSO2 GitHub / repositories overview. [2]
Update SDK dependency to an available version.
Go 1.25.1 is valid. However, github.com/wso2/api-platform/sdk v0.3.5 does not exist as a published release tag. Update the require statement to use an available SDK version that exists in the wso2/api-platform repository.
🤖 Prompt for AI Agents
In `@gateway/policies/model-weighted-round-robin/v0.1.0/go.mod` around lines 3 -
5, The go.mod currently requires a non-existent tag
"github.com/wso2/api-platform/sdk v0.3.5"; update the require line in go.mod to
point to a published tag for that module (for example replace v0.3.5 with an
available release such as v0.3.4 or the repository's latest stable tag) so that
`require github.com/wso2/api-platform/sdk` references a valid version; run `go
mod tidy` afterwards to verify and update go.sum.
| github.com/wso2/api-platform/sdk v0.3.5 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n gateway/policies/semantic-cache/v0.1.0/go.modRepository: wso2/api-platform
Length of output: 7221
🌐 Web query:
github.com/wso2/api-platform/sdk v0.3.5 Go module release
💡 Result:
Do you want me to (A) confirm whether the Go module version v0.3.5 exists and provide links, or (B) show how to fetch/use that version (go get / go.mod example)?
🌐 Web query:
prometheus/client_golang v1.23.2 release Go module
💡 Result:
Yes — prometheus/client_golang v1.23.2 is released (tag: v1.23.2, 2025-09-05). It’s the module github.com/prometheus/client_golang and the release upgrades prometheus/common to v0.66.1 with no functional changes. [1][2]
Install via Go modules:
- go get github.com/prometheus/client_golang@v1.23.2
Or add to go.mod: - require github.com/prometheus/client_golang v1.23.2
Sources:
[1] GitHub release (v1.23.2).
[2] pkg.go.dev module page.
🏁 Script executed:
# Check other gateway policy go.mod files to see if they have replace directives
find gateway/policies -name "go.mod" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -E "^replace" {} || echo "No replace directives"'Repository: wso2/api-platform
Length of output: 508
🌐 Web query:
github.com/wso2/api-platform SDK v0.3.5 release Go
💡 Result:
Do you mean the GitHub repository wso2/api-platform (releases) and whether an SDK v0.3.5 for Go exists — or do you want the download/link/notes for a specific Go SDK v0.3.5?
🌐 Web query:
prometheus/procfs v0.16.1 Go module release
💡 Result:
Yes — prometheus/procfs v0.16.1 is a released Go module. Release notes (changelog highlights: sync common files, Go toolchain update, several dependency bumps and bugfixes) are available. [1][2]
Sources:
- Release summary (v0.16.1). [1]
- Example downstream usages / go.sum entries showing module v0.16.1. [2]
References:
[1] NewReleases — prometheus/procfs v0.16.1 (changelog/links).
[2] grafana go.sum / other downstream manifests referencing github.com/prometheus/procfs v0.16.1.
🌐 Web query:
go.yaml.in/yaml/v2 v2.4.2 Go module
💡 Result:
Short answer: There is no official module path "go.yaml.in/yaml/v2" with a v2.4.2 release. The canonical v2 import path is gopkg.in/yaml.v2 (also hosted as gopkg.in/go-yaml/yaml.v2), and the latest v2 tag shown by gopkg.in is v2.4.0 (not v2.4.2). [1][2]
Sources:
- gopkg.in YAML v2 documentation (shows v2 → v2.4.0). [1]
- pkg.go.dev listing for the YAML v2 package (import path gopkg.in/yaml.v2 / docs). [2]
🏁 Script executed:
# Check if semantic-cache had replace directives in a previous commit or if this is expected
git log --oneline -n 10 -- gateway/policies/semantic-cache/v0.1.0/go.modRepository: wso2/api-platform
Length of output: 104
🏁 Script executed:
# Check if there are other go.mod files with the same invalid yaml import
grep -r "go.yaml.in/yaml/v2" gateway/policies/ --include="go.mod"Repository: wso2/api-platform
Length of output: 145
🏁 Script executed:
# Check the git history for when this invalid dependency was introduced
git log -p --all -- "*go.mod" | grep -B5 -A5 "go.yaml.in/yaml/v2" | head -50Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check if wso2/api-platform SDK v0.3.5 exists by looking at the SDK directory
ls -la sdk/ 2>/dev/null && cat sdk/go.mod | grep "^module"Repository: wso2/api-platform
Length of output: 448
Fix invalid YAML dependency and remove duplicate.
The entry at line 108 (go.yaml.in/yaml/v2 v2.4.2) is invalid—this module path and version do not exist in the public Go module registry. The canonical YAML v2 path is gopkg.in/yaml.v2 (correctly declared at line 123 with version v2.4.0). Remove the invalid go.yaml.in/yaml/v2 entry.
The prometheus dependencies (client_golang v1.23.2, common v0.66.1, procfs v0.16.1) and SDK v0.3.5 upgrade are all valid and available. The SDK bump is intentional based on the repository's policy version update.
🤖 Prompt for AI Agents
In `@gateway/policies/semantic-cache/v0.1.0/go.mod` around lines 7 - 8, Remove the
invalid duplicate YAML module entry in go.mod: delete the line declaring
"go.yaml.in/yaml/v2 v2.4.2" so only the canonical "gopkg.in/yaml.v2 v2.4.0"
remains; keep the prometheus entries and the intentional SDK bump
"github.com/wso2/api-platform/sdk v0.3.5" as-is.
1 participant
Purpose
Fix #753
Discussion: #748
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.