Validate policy params for LLM operationPolicies and the policies list

gateway/gateway-controller/pkg/config/policy_validator.go

@@ -20,8 +20,8 @@ package config
 
 import (
 	"fmt"
-	"regexp"
 	"math"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -119,36 +119,72 @@ func (pv *PolicyValidator) ValidateLLMProxyPolicies(cfg *api.LLMProxyConfigurati
 
 // validateLLMPolicyRefs validates the three policy collections shared by LLM providers and
 // proxies: api-level (global) policies, operation-level policies, and the deprecated policies
-// list. An empty version resolves to the latest available version (handled by ResolvePolicyVersion).
+// list. Every collection gets its name/version reference resolved and its params validated
+// against the definition's declared parameter schema. An empty version resolves to the latest
+// available version (handled by ResolvePolicyVersion).
 func (pv *PolicyValidator) validateLLMPolicyRefs(globalPolicies *[]api.Policy, operationPolicies *[]api.OperationPolicy, legacyPolicies *[]api.LLMPolicy) []ValidationError {
 	var errors []ValidationError
 
-	// Global (api-level) policies carry params, so reuse validatePolicy to also validate them.
+	// Global (api-level) policies carry params on the policy itself, so reuse validatePolicy.
 	if globalPolicies != nil {
 		for i, policy := range *globalPolicies {
 			errors = append(errors, pv.validatePolicy(policy, fmt.Sprintf("spec.globalPolicies[%d]", i))...)
 		}
 	}
 
-	// Operation-level policies: validate name + version existence.
+	// Operation-level policies: name + version existence, then each path's params.
 	if operationPolicies != nil {
 		for i, policy := range *operationPolicies {
-			_, errs := pv.validatePolicyRef(policy.Name, policy.Version, fmt.Sprintf("spec.operationPolicies[%d]", i))
-			errors = append(errors, errs...)
+			fieldPath := fmt.Sprintf("spec.operationPolicies[%d]", i)
+			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))...)
+			}
 		}
 	}
 
-	// Deprecated policies list (still honoured): validate name + version existence.
+	// Deprecated policies list (still honoured): same as operation-level policies.
 	if legacyPolicies != nil {
 		for i, policy := range *legacyPolicies {
-			_, errs := pv.validatePolicyRef(policy.Name, policy.Version, fmt.Sprintf("spec.policies[%d]", i))
-			errors = append(errors, errs...)
+			fieldPath := fmt.Sprintf("spec.policies[%d]", i)
+			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))...)
+			}
 		}
 	}
 
 	return errors
 }
 
+// validateAttachedPolicyParams validates one per-path params map from an LLM operation-level
+// or deprecated policy attachment against the resolved definition's parameter schema. Params
+// are coerced first, since template rendering always yields strings ({{ env "X" }} -> "100"
+// even for an integer param) — mirroring validatePolicy's handling of api-level params.
+func (pv *PolicyValidator) validateAttachedPolicyParams(policyDef *models.PolicyDefinition, params map[string]interface{}, fieldPath string) []ValidationError {
+	if policyDef == nil || policyDef.Parameters == nil {
+		return nil
+	}
+	if params == nil {
+		// A missing params map still has to be validated: the schema may declare
+		// required properties, and an empty object must fail the same way.
+		params = map[string]interface{}{}
+	} else {
+		coerceParamsBySchema(params, *policyDef.Parameters)
+	}
+	return pv.validatePolicyParams(params, *policyDef.Parameters, fieldPath+".params")
+}
+
 // validatePolicy validates a single policy reference (name + version existence) and, when the
 // definition declares a parameter schema, the policy's params against that schema.
 func (pv *PolicyValidator) validatePolicy(policy api.Policy, fieldPath string) []ValidationError {

gateway/gateway-controller/pkg/config/policy_validator_llm_test.go

@@ -156,3 +156,191 @@ func TestPolicyValidator_ValidateLLMProxyPolicies_NonExistentMajorVersion(t *tes
 	assert.Len(t, errors, 1, "expected one error for a non-existent major version")
 	assert.Contains(t, errors[0].Message, "major version 'v999' not found")
 }
+
+// paramDefs returns definitions whose "token-based-ratelimit" policy declares a parameter
+// schema, so per-path params on operation-level and deprecated policies can be exercised.
+// additionalProperties:false mirrors the shipped policy definitions.
+func paramDefs() map[string]models.PolicyDefinition {
+	schema := map[string]interface{}{
+		"type":                 "object",
+		"additionalProperties": false,
+		"required":             []interface{}{"limit"},
+		"properties": map[string]interface{}{
+			"limit":    map[string]interface{}{"type": "integer", "minimum": float64(1)},
+			"duration": map[string]interface{}{"type": "string"},
+		},
+	}
+	return map[string]models.PolicyDefinition{
+		"token-based-ratelimit|v1.0.0": {Name: "token-based-ratelimit", Version: "v1.0.0", Parameters: &schema},
+		"no-schema-policy|v1.0.0":      {Name: "no-schema-policy", Version: "v1.0.0"},
+	}
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_OperationPolicyParamsValid(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"limit": 100, "duration": "1m"}},
+				}},
+			},
+		},
+	}
+
+	assert.Empty(t, validator.ValidateLLMProviderPolicies(cfg))
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_OperationPolicyParamsInvalid(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"limit": 100}},
+					{Path: "/embeddings", Params: map[string]interface{}{"duration": "1m"}},
+					{Path: "/responses", Params: map[string]interface{}{"limit": 0}},
+					{Path: "/models", Params: map[string]interface{}{"limit": 1, "bogus": "x"}},
+				}},
+			},
+		},
+	}
+
+	errors := validator.ValidateLLMProviderPolicies(cfg)
+	assert.Len(t, errors, 3, "expected one error each for the missing, out-of-range and unknown param")
+
+	fields := make([]string, 0, len(errors))
+	for _, e := range errors {
+		fields = append(fields, e.Field)
+	}
+	assert.NotContains(t, fields, "spec.operationPolicies[0].paths[0].params",
+		"paths[0] is valid and must not be reported")
+	assert.Contains(t, fields, "spec.operationPolicies[0].paths[1].params")
+	assert.Contains(t, errors[0].Message, "limit is required")
+	assert.Equal(t, "spec.operationPolicies[0].paths[2].params.limit", errors[1].Field)
+	assert.Contains(t, errors[2].Message, "Additional property bogus is not allowed")
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_OperationPolicyMissingParamsFailsRequired(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions"}, // no params at all
+				}},
+			},
+		},
+	}
+
+	errors := validator.ValidateLLMProviderPolicies(cfg)
+	assert.Len(t, errors, 1)
+	assert.Equal(t, "spec.operationPolicies[0].paths[0].params", errors[0].Field)
+	assert.Contains(t, errors[0].Message, "limit is required")
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_OperationPolicyParamsCoerced(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	// A rendered template ({{ env "LIMIT" }}) always produces a string; coercion must run
+	// before schema validation so "100" satisfies the integer param.
+	params := map[string]interface{}{"limit": "100", "duration": "1m"}
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: params},
+				}},
+			},
+		},
+	}
+
+	assert.Empty(t, validator.ValidateLLMProviderPolicies(cfg))
+	assert.Equal(t, float64(100), params["limit"], "params must be coerced in place")
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_OperationPolicyNoSchemaSkipsParams(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "no-schema-policy", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"anything": "goes"}},
+				}},
+			},
+		},
+	}
+
+	assert.Empty(t, validator.ValidateLLMProviderPolicies(cfg),
+		"a definition without a parameter schema must not reject params")
+}
+
+func TestPolicyValidator_ValidateLLMProviderPolicies_BadRefSkipsParamValidation(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v999", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"bogus": "x"}},
+				}},
+			},
+		},
+	}
+
+	errors := validator.ValidateLLMProviderPolicies(cfg)
+	assert.Len(t, errors, 1, "an unresolvable reference must report once, not also per path")
+	assert.Contains(t, errors[0].Message, "major version 'v999' not found")
+}
+
+func TestPolicyValidator_ValidateLLMProxyPolicies_LegacyPolicyParamsInvalid(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProxyConfiguration{
+		Spec: api.LLMProxyConfigData{
+			Policies: &[]api.LLMPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.LLMPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"limit": 100}},
+					{Path: "/embeddings", Params: map[string]interface{}{"limit": "not-a-number"}},
+				}},
+			},
+		},
+	}
+
+	errors := validator.ValidateLLMProxyPolicies(cfg)
+	assert.Len(t, errors, 1)
+	assert.Equal(t, "spec.policies[0].paths[1].params.limit", errors[0].Field)
+}
+
+// The LLM->RestAPI transform merges the provider template's extraction params
+// (requestModel, promptTokens, ...) into every operation-level policy attachment. Those keys
+// are declared by no policy schema, and most schemas set additionalProperties:false — so
+// validation must run against the user-authored params, never the post-merge result.
+func TestPolicyValidator_ValidateLLMProviderPolicies_TemplateExtractionParamsNotRequired(t *testing.T) {
+	validator := NewPolicyValidator(paramDefs())
+
+	cfg := &api.LLMProviderConfiguration{
+		Spec: api.LLMProviderConfigData{
+			OperationPolicies: &[]api.OperationPolicy{
+				{Name: "token-based-ratelimit", Version: "v1", Paths: []api.OperationPolicyPath{
+					{Path: "/chat/completions", Params: map[string]interface{}{"limit": 100}},
+				}},
+			},
+		},
+	}
+	assert.Empty(t, validator.ValidateLLMProviderPolicies(cfg),
+		"user-authored params alone must validate; template params are merged later")
+
+	// Sanity check that the merged shape would indeed be rejected, which is why the
+	// derived RestAPI is deliberately not the validation input.
+	merged := map[string]interface{}{
+		"limit":        100,
+		"requestModel": map[string]interface{}{"location": "payload", "identifier": "$.model"},
+	}
+	def := paramDefs()["token-based-ratelimit|v1.0.0"]
+	assert.NotEmpty(t, validator.validatePolicyParams(merged, *def.Parameters, "p"))
+}