Validate policy params for LLM operationPolicies and the policies list - #3383
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughLLM operation-level and deprecated policy attachments now validate per-path parameters against resolved policy schemas. Validation coerces rendered values, checks absent parameter maps, and reports schema errors. Tests cover the validation cases, and token-based rate-limit scenarios now use policy defaults. ChangesLLM policy parameter validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The updated validation and integration scenarios retain their intended behavior, with no concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR removes explicit ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dependency Validation ResultsDependency name: github.com/klauspost/compress Dependency name: github.com/a2aproject/a2a-go/v2 Dependency name: github.com/pb33f/libopenapi Dependency name: github.com/pb33f/libopenapi-validator Dependency name: github.com/go-playground/validator/v10 Dependency name: github.com/gorilla/websocket Dependency name: github.com/jackc/pgx/v5 Dependency name: github.com/knadh/koanf/parsers/toml/v2 Dependency name: github.com/knadh/koanf/providers/confmap Dependency name: github.com/knadh/koanf/v2 Dependency name: github.com/mattn/go-sqlite3 Dependency name: github.com/microsoft/go-mssqldb Dependency name: github.com/oapi-codegen/runtime Dependency name: github.com/stretchr/testify Dependency name: golang.org/x/crypto Dependency name: github.com/cucumber/godog Dependency name: github.com/golang-jwt/jwt/v5 Dependency name: github.com/jackc/pgx/v5 Dependency name: github.com/knadh/koanf/parsers/toml/v2 Dependency name: github.com/mattn/go-sqlite3 Dependency name: github.com/microsoft/go-mssqldb Dependency name: github.com/moby/moby/api Dependency name: github.com/moby/moby/client Dependency name: github.com/modelcontextprotocol/go-sdk Dependency name: github.com/mxschmitt/playwright-go Dependency name: github.com/stretchr/testify Dependency name: github.com/testcontainers/testcontainers-go Dependency name: github.com/testcontainers/testcontainers-go/modules/compose Dependency name: golang.org/x/crypto Dependency name: gopkg.in/yaml.v3 Next Steps
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3383 +/- ##
==========================================
- Coverage 51.81% 51.80% -0.01%
==========================================
Files 955 957 +2
Lines 136747 137561 +814
Branches 4447 4447
==========================================
+ Hits 70849 71270 +421
- Misses 59004 59389 +385
- Partials 6894 6902 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
3 participants
Purpose
PolicyValidatorvalidates policyparamsagainst the policy definition's declared JSON Schema for most artifact types, but silently skips it for two of the five policy collections anLlmProvider/LlmProxycan declare.validateLLMPolicyRefsresolved the policy reference for the operation-level and deprecated lists and then discarded the resolved definition:Only the global (api-level) list routed through
validatePolicy, which performs the schema check.Coverage before this PR:
RestApispec.policies,spec.operations[].policiesMcpspec.policiesLlmProvider/LlmProxyspec.globalPoliciesLlmProvider/LlmProxyspec.operationPolicies[].paths[].paramsLlmProvider/LlmProxyspec.policies[].paths[].params(deprecated)A misconfigured operation-level LLM policy therefore deployed successfully instead of being rejected: missing required params, out-of-range values and unknown properties all passed. The failure surfaced later at runtime, where the policy is dropped or misbehaves with no deploy-time signal — and the same params on the same policy were correctly rejected when attached as a
globalPolicy, which made the behavior look arbitrary.Resolves:
operationPoliciesand the deprecatedpolicieslist #3381Goals
Schema-validate
paramsfor the two remaining LLM policy collections, so an invalid operation-level policy fails at deploy time with a field path naming the offending param — matching whatRestApi,Mcpand LLMglobalPoliciesalready do.Approach
validateLLMPolicyRefsnow uses the*models.PolicyDefinitionthatvalidatePolicyRefwas already returning, and validates each path attachment's params:A new
validateAttachedPolicyParamshelper coerces then schema-checks one params map. It handles the two things that differ from the api-level path:params:entirely would bypass a schema'srequiredlist.{{ env "LIMIT" }}→"100"for an integer param), mirroringvalidatePolicy's handling of api-level params. Both call sites validate rendered config (RenderSpecatllm_deployment.go:270/:457, validation at:312/:498), so this is the correct order.An unresolvable name/version reports once and skips param validation, rather than repeating the same error per path.
User stories
As an API platform user deploying an
LlmProvider/LlmProxy, when I attach an operation-level policy with invalid params, the deploy is rejected with an error naming the param — instead of succeeding and silently misbehaving at runtime.Automation tests
policy_validator_llm_test.go(+188 lines), covering: valid params; missing required / out-of-range / unknown-property; absentparamsmap; string→int coercion; a definition with no parameter schema; unresolvable ref not re-reporting per path; the deprecatedpolicieslist; and the template-merge rationale. Fullgateway-controllermodule passes (go build ./...,go vet,go test ./...).Regression check against real policy definitions
Because this turns previously-accepted config into rejected config, verified it doesn't reject anything valid: loaded all 36 real policy definitions from
wso2/gateway-controllersand validated every operation-level/deprecatedparamsblock ingateway/examples/*.yaml(api-key-auth,content-length-guardrail,llm-header-router,llm-cost-based-ratelimit,openai-to-bedrock-transformer, …) — all clean, no false positives.Behavior change to be aware of: an
LlmProvider/LlmProxyalready deployed with invalid operation-level policy params will now fail validation on its next deploy/update. That is the intended fix, but it can surface as a new failure on config that previously "worked".Test environment
Go 1.26.2, macOS (darwin 24.6.0). Validation logic is platform- and DB-independent; no browser or database involvement.