Skip to content

Validate policy params for LLM operationPolicies and the policies list - #3383

Open
thivindu wants to merge 1 commit into
wso2:mainfrom
thivindu:bug-fixes
Open

Validate policy params for LLM operationPolicies and the policies list#3383
thivindu wants to merge 1 commit into
wso2:mainfrom
thivindu:bug-fixes

Conversation

@thivindu

@thivindu thivindu commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Purpose

PolicyValidator validates policy params against the policy definition's declared JSON Schema for most artifact types, but silently skips it for two of the five policy collections an LlmProvider/LlmProxy can declare. validateLLMPolicyRefs resolved the policy reference for the operation-level and deprecated lists and then discarded the resolved definition:

// Operation-level policies: validate name + version existence.
_, errs := pv.validatePolicyRef(policy.Name, policy.Version, ...)   // ← the *models.PolicyDefinition is dropped

Only the global (api-level) list routed through validatePolicy, which performs the schema check.

Coverage before this PR:

Artifact kind Policy collection Name + version Params vs. schema
RestApi spec.policies, spec.operations[].policies
Mcp spec.policies
LlmProvider / LlmProxy spec.globalPolicies
LlmProvider / LlmProxy spec.operationPolicies[].paths[].params
LlmProvider / LlmProxy spec.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:

Goals

Schema-validate params for 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 what RestApi, Mcp and LLM globalPolicies already do.

Approach

validateLLMPolicyRefs now uses the *models.PolicyDefinition that validatePolicyRef was already returning, and validates each path attachment's params:

policyDef, errs := pv.validatePolicyRef(policy.Name, policy.Version, fieldPath)
if len(errs) > 0 { errors = append(errors, errs...); continue }
for j := range policy.Paths {
    errors = append(errors, pv.validateAttachedPolicyParams(policyDef, policy.Paths[j].Params,
        fmt.Sprintf("%s.paths[%d]", fieldPath, j))...)
}

A new validateAttachedPolicyParams helper coerces then schema-checks one params map. It handles the two things that differ from the api-level path:

  • A nil params map is still validated, not skipped — otherwise omitting params: entirely would bypass a schema's required list.
  • Coercion runs first, since template rendering always yields strings ({{ env "LIMIT" }}"100" for an integer param), mirroring validatePolicy's handling of api-level params. Both call sites validate rendered config (RenderSpec at llm_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

  • Unit tests — 8 new tests in policy_validator_llm_test.go (+188 lines), covering: valid params; missing required / out-of-range / unknown-property; absent params map; string→int coercion; a definition with no parameter schema; unresolvable ref not re-reporting per path; the deprecated policies list; and the template-merge rationale. Full gateway-controller module passes (go build ./..., go vet, go test ./...).
  • Integration tests — none added. The change is confined to a pure validation function with no I/O; the behavior is fully covered at unit level.

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-controllers and validated every operation-level/deprecated params block in gateway/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/LlmProxy already 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.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: ee0e39eb-7ce4-421a-8fb2-260c2ccb2e8d

📥 Commits

Reviewing files that changed from the base of the PR and between 1be5bb7 and 765600d.

📒 Files selected for processing (2)
  • gateway/gateway-controller/pkg/config/policy_validator.go
  • gateway/gateway-controller/pkg/config/policy_validator_llm_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

LLM 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 valid, invalid, schema-less, unresolved, deprecated, and template-related cases.

Changes

LLM policy parameter validation

Layer / File(s) Summary
Validate attached policy parameters
gateway/gateway-controller/pkg/config/policy_validator.go
Resolved operation-level and deprecated policy references now validate each path’s parameters. Validation coerces schema-defined values and treats missing maps as empty objects.
Test policy parameter validation
gateway/gateway-controller/pkg/config/policy_validator_llm_test.go
Tests cover schema validation, required and unknown parameters, coercion, absent maps, schema-less definitions, unresolved references, deprecated policies, and template-extraction parameters.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 76560

This change validates LLM policy attachment parameters against their policy schemas, including required fields, coercion, deprecated policies, and unresolved references. The covered validation behavior is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: validating parameters for LLM operationPolicies and the deprecated policies list.
Description check ✅ Passed The description explains the purpose, goals, approach, user story, tests, regression checks, behavior change, and test environment. It omits the Documentation, Security checks, Samples, and Related PR…
Linked Issues check ✅ Passed The implementation satisfies issue [#3381] by validating per-path parameters for both LLM operationPolicies and the deprecated policies list. It also handles nil maps, coercion, unresolved references,…
Out of Scope Changes check ✅ Passed The changes are limited to policy validation logic and focused unit tests. The import reorder is incidental and remains within the modified validator file. No unrelated changes are present.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant