Skip to content

Commit a0d55a4

Browse files
ArshardhArshardh
committed
Fix policy attachment order for wildcard provider policies
1 parent 7d63507 commit a0d55a4

2 files changed

Lines changed: 202 additions & 175 deletions

File tree

‎gateway/gateway-controller/pkg/utils/llm_transformer.go‎

Lines changed: 138 additions & 131 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package utils
22

33
import (
44
"fmt"
5+
"sort"
56
"strings"
67

78
api "github.com/wso2/api-platform/gateway/gateway-controller/pkg/api/management"
@@ -25,6 +26,11 @@ type pathMethodKey struct {
2526
method string
2627
}
2728

29+
type llmPolicyAttachment struct {
30+
policy api.LLMPolicy
31+
pathEntry api.LLMPolicyPath
32+
}
33+
2834
func NewLLMProviderTransformer(store *storage.ConfigStore, db storage.Storage, routerConfig *config.RouterConfig, policyVersionResolver PolicyVersionResolver) *LLMProviderTransformer {
2935
if db == nil {
3036
panic("LLMProviderTransformer requires non-nil storage")
@@ -226,50 +232,41 @@ func (t *LLMProviderTransformer) transformProxy(proxy *api.LLMProxyConfiguration
226232
if proxy.Spec.Policies != nil {
227233
registerExplicitLLMPolicyOperations(operationRegistry, *proxy.Spec.Policies, nil)
228234

229-
for _, llmPol := range *proxy.Spec.Policies {
230-
for _, pathEntry := range llmPol.Paths {
231-
policyMethods := expandLLMPolicyMethods(pathEntry.Methods)
235+
for _, attachment := range orderedLLMPolicyAttachments(*proxy.Spec.Policies) {
236+
policyMethods := expandLLMPolicyMethods(attachment.pathEntry.Methods)
232237

233-
for _, policyMethod := range policyMethods {
234-
attachedPolicyPaths := make(map[string]bool)
235-
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
236-
237-
// Create operation if it doesn't exist (dynamic operation creation)
238-
key := pathMethodKey{path: pathEntry.Path, method: policyMethod}
239-
if _, exists := operationRegistry[key]; !exists {
240-
op := ensureOperation(operationRegistry, pathEntry.Path, policyMethod)
241-
methodOperations = append(methodOperations, op)
242-
}
238+
for _, policyMethod := range policyMethods {
239+
attachedPolicyPaths := make(map[string]bool)
240+
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
243241

244-
for _, op := range methodOperations {
245-
// Use pathsMatch to determine if policy applies to this operation
246-
if pathsMatch(op.Path, pathEntry.Path) {
247-
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
248-
if attachedPolicyPaths[targetPath] {
249-
continue
250-
}
251-
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
252-
targetOp, exists := operationRegistry[targetKey]
253-
if !exists {
254-
targetOp = &api.Operation{
255-
Path: targetPath,
256-
Method: api.OperationMethod(policyMethod),
257-
}
258-
operationRegistry[targetKey] = targetOp
242+
for _, op := range methodOperations {
243+
// Use pathsMatch to determine if policy applies to this operation
244+
if pathsMatch(op.Path, attachment.pathEntry.Path) {
245+
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
246+
if attachedPolicyPaths[targetPath] {
247+
continue
248+
}
249+
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
250+
targetOp, exists := operationRegistry[targetKey]
251+
if !exists {
252+
targetOp = &api.Operation{
253+
Path: targetPath,
254+
Method: api.OperationMethod(policyMethod),
259255
}
256+
operationRegistry[targetKey] = targetOp
257+
}
260258

261-
templateParams, err := buildTemplateParams(tmpl, targetPath)
262-
if err != nil {
263-
return nil, fmt.Errorf("failed to build template params: %w", err)
264-
}
265-
pol := api.Policy{
266-
Name: llmPol.Name,
267-
Version: llmPol.Version,
268-
Params: mergeParams(pathEntry.Params, templateParams),
269-
}
270-
appendOperationPolicy(targetOp, pol)
271-
attachedPolicyPaths[targetPath] = true
259+
templateParams, err := buildTemplateParams(tmpl, targetPath)
260+
if err != nil {
261+
return nil, fmt.Errorf("failed to build template params: %w", err)
262+
}
263+
pol := api.Policy{
264+
Name: attachment.policy.Name,
265+
Version: attachment.policy.Version,
266+
Params: mergeParams(attachment.pathEntry.Params, templateParams),
272267
}
268+
appendOperationPolicy(targetOp, pol)
269+
attachedPolicyPaths[targetPath] = true
273270
}
274271
}
275272
}
@@ -444,64 +441,54 @@ func (t *LLMProviderTransformer) transformProvider(provider *api.LLMProviderConf
444441
return !isDeniedByException(path, method, deniedPathMethods)
445442
})
446443

447-
for _, llmPol := range *provider.Spec.Policies {
448-
for _, pathEntry := range llmPol.Paths {
449-
policyMethods := expandLLMPolicyMethods(pathEntry.Methods)
444+
for _, attachment := range orderedLLMPolicyAttachments(*provider.Spec.Policies) {
445+
policyMethods := expandLLMPolicyMethods(attachment.pathEntry.Methods)
450446

451-
for _, policyMethod := range policyMethods {
452-
attachedPolicyPaths := make(map[string]bool)
453-
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
447+
for _, policyMethod := range policyMethods {
448+
attachedPolicyPaths := make(map[string]bool)
449+
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
454450

455-
// CRITICAL: Skip if this path+method is denied by exception
456-
if isDeniedByException(pathEntry.Path, policyMethod, deniedPathMethods) {
457-
continue // Exception deny policy takes precedence
458-
}
451+
// CRITICAL: Skip if this path+method is denied by exception
452+
if isDeniedByException(attachment.pathEntry.Path, policyMethod, deniedPathMethods) {
453+
continue // Exception deny policy takes precedence
454+
}
459455

460-
// Check if explicit operation exists for this policy path+method
461-
key := pathMethodKey{path: pathEntry.Path, method: policyMethod}
462-
if _, exists := operationRegistry[key]; !exists {
463-
// Create operation for user policy
464-
op := ensureOperation(operationRegistry, pathEntry.Path, policyMethod)
465-
methodOperations = append(methodOperations, op)
456+
for _, op := range methodOperations {
457+
// Skip if this operation has deny policy (from exceptions)
458+
if denyPolicyVersion != "" && hasDenyPolicy(op, denyPolicyVersion) {
459+
continue
466460
}
467461

468-
for _, op := range methodOperations {
469-
// Skip if this operation has deny policy (from exceptions)
470-
if denyPolicyVersion != "" && hasDenyPolicy(op, denyPolicyVersion) {
471-
continue
472-
}
473-
474-
if pathsMatch(op.Path, pathEntry.Path) {
475-
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
476-
if attachedPolicyPaths[targetPath] {
477-
continue
478-
}
479-
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
480-
targetOp, exists := operationRegistry[targetKey]
481-
if !exists {
482-
targetOp = &api.Operation{
483-
Path: targetPath,
484-
Method: api.OperationMethod(policyMethod),
485-
}
486-
operationRegistry[targetKey] = targetOp
462+
if pathsMatch(op.Path, attachment.pathEntry.Path) {
463+
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
464+
if attachedPolicyPaths[targetPath] {
465+
continue
466+
}
467+
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
468+
targetOp, exists := operationRegistry[targetKey]
469+
if !exists {
470+
targetOp = &api.Operation{
471+
Path: targetPath,
472+
Method: api.OperationMethod(policyMethod),
487473
}
474+
operationRegistry[targetKey] = targetOp
475+
}
488476

489-
if denyAppliesToTarget(targetPath, policyMethod, denyPolicyVersion, operationRegistry) {
490-
continue
491-
}
477+
if denyAppliesToTarget(targetPath, policyMethod, denyPolicyVersion, operationRegistry) {
478+
continue
479+
}
492480

493-
templateParams, err := buildTemplateParams(tmpl, targetPath)
494-
if err != nil {
495-
return nil, fmt.Errorf("failed to build template params: %w", err)
496-
}
497-
pol := api.Policy{
498-
Name: llmPol.Name,
499-
Version: llmPol.Version,
500-
Params: mergeParams(pathEntry.Params, templateParams),
501-
}
502-
appendOperationPolicy(targetOp, pol)
503-
attachedPolicyPaths[targetPath] = true
481+
templateParams, err := buildTemplateParams(tmpl, targetPath)
482+
if err != nil {
483+
return nil, fmt.Errorf("failed to build template params: %w", err)
484+
}
485+
pol := api.Policy{
486+
Name: attachment.policy.Name,
487+
Version: attachment.policy.Version,
488+
Params: mergeParams(attachment.pathEntry.Params, templateParams),
504489
}
490+
appendOperationPolicy(targetOp, pol)
491+
attachedPolicyPaths[targetPath] = true
505492
}
506493
}
507494
}
@@ -551,53 +538,40 @@ func (t *LLMProviderTransformer) transformProvider(provider *api.LLMProviderConf
551538
return isAllowedByAccessControl(path, method, normalizedExceptions)
552539
})
553540

554-
for _, llmPol := range *provider.Spec.Policies {
555-
for _, pathEntry := range llmPol.Paths {
556-
policyMethods := expandLLMPolicyMethods(pathEntry.Methods)
557-
558-
for _, policyMethod := range policyMethods {
559-
attachedPolicyPaths := make(map[string]bool)
560-
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
561-
562-
// Check if this policy path+method is allowed by access control
563-
if isAllowedByAccessControl(pathEntry.Path, policyMethod, normalizedExceptions) {
564-
// Check if explicit operation exists for this policy path+method
565-
key := pathMethodKey{path: pathEntry.Path, method: policyMethod}
566-
if _, exists := operationRegistry[key]; !exists {
567-
// Create operation for specific policy path covered by wildcard access control
568-
op := ensureOperation(operationRegistry, pathEntry.Path, policyMethod)
569-
methodOperations = append(methodOperations, op)
570-
}
571-
}
541+
for _, attachment := range orderedLLMPolicyAttachments(*provider.Spec.Policies) {
542+
policyMethods := expandLLMPolicyMethods(attachment.pathEntry.Methods)
572543

573-
for _, op := range methodOperations {
574-
if pathsMatch(op.Path, pathEntry.Path) {
575-
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
576-
if attachedPolicyPaths[targetPath] {
577-
continue
578-
}
579-
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
580-
targetOp, exists := operationRegistry[targetKey]
581-
if !exists {
582-
targetOp = &api.Operation{
583-
Path: targetPath,
584-
Method: api.OperationMethod(policyMethod),
585-
}
586-
operationRegistry[targetKey] = targetOp
587-
}
544+
for _, policyMethod := range policyMethods {
545+
attachedPolicyPaths := make(map[string]bool)
546+
methodOperations := getOperationsForMethod(operationRegistry, policyMethod)
588547

589-
templateParams, err := buildTemplateParams(tmpl, targetPath)
590-
if err != nil {
591-
return nil, fmt.Errorf("failed to build template params: %w", err)
592-
}
593-
pol := api.Policy{
594-
Name: llmPol.Name,
595-
Version: llmPol.Version,
596-
Params: mergeParams(pathEntry.Params, templateParams),
548+
for _, op := range methodOperations {
549+
if pathsMatch(op.Path, attachment.pathEntry.Path) {
550+
for _, targetPath := range expandPolicyTargetPaths(op.Path, &tmpl.Configuration.Spec) {
551+
if attachedPolicyPaths[targetPath] {
552+
continue
553+
}
554+
targetKey := pathMethodKey{path: targetPath, method: policyMethod}
555+
targetOp, exists := operationRegistry[targetKey]
556+
if !exists {
557+
targetOp = &api.Operation{
558+
Path: targetPath,
559+
Method: api.OperationMethod(policyMethod),
597560
}
598-
appendOperationPolicy(targetOp, pol)
599-
attachedPolicyPaths[targetPath] = true
561+
operationRegistry[targetKey] = targetOp
562+
}
563+
564+
templateParams, err := buildTemplateParams(tmpl, targetPath)
565+
if err != nil {
566+
return nil, fmt.Errorf("failed to build template params: %w", err)
600567
}
568+
pol := api.Policy{
569+
Name: attachment.policy.Name,
570+
Version: attachment.policy.Version,
571+
Params: mergeParams(attachment.pathEntry.Params, templateParams),
572+
}
573+
appendOperationPolicy(targetOp, pol)
574+
attachedPolicyPaths[targetPath] = true
601575
}
602576
}
603577
}
@@ -774,6 +748,39 @@ func appendOperationPolicy(op *api.Operation, pol api.Policy) {
774748
op.Policies = &existing
775749
}
776750

751+
func orderedLLMPolicyAttachments(policies []api.LLMPolicy) []llmPolicyAttachment {
752+
attachments := make([]llmPolicyAttachment, 0)
753+
for _, llmPol := range policies {
754+
for _, pathEntry := range llmPol.Paths {
755+
attachments = append(attachments, llmPolicyAttachment{
756+
policy: llmPol,
757+
pathEntry: pathEntry,
758+
})
759+
}
760+
}
761+
762+
sort.SliceStable(attachments, func(i, j int) bool {
763+
return shouldAttachPathBefore(attachments[i].pathEntry.Path, attachments[j].pathEntry.Path)
764+
})
765+
766+
return attachments
767+
}
768+
769+
func shouldAttachPathBefore(leftPath, rightPath string) bool {
770+
leftHasWildcard := strings.Contains(leftPath, constants.WILD_CARD)
771+
rightHasWildcard := strings.Contains(rightPath, constants.WILD_CARD)
772+
773+
if leftHasWildcard != rightHasWildcard {
774+
return leftHasWildcard
775+
}
776+
777+
if len(leftPath) != len(rightPath) {
778+
return len(leftPath) < len(rightPath)
779+
}
780+
781+
return leftPath < rightPath
782+
}
783+
777784
// ensureOperation checks if an operation for the given path+method exists in the registry, and creates it if not
778785
func ensureOperation(operationRegistry map[pathMethodKey]*api.Operation, path, method string) *api.Operation {
779786
key := pathMethodKey{path: path, method: method}

0 commit comments

Comments
 (0)