From c253a7d59140c89f0642f09e9db2d89bdca4fc44 Mon Sep 17 00:00:00 2001 From: Aakash Wijesekara Date: Sat, 11 Jul 2026 10:41:00 +0530 Subject: [PATCH] address further review comments on multi-provider routing - Constrain LLMProxyTransformer.version to major-only in the management OpenAPI schema and the gateway-controller LLM validator. - Add operator-side LLMProxyTransformer type + additionalProviders[].transformer field; regenerate deepcopy and both CRD artifacts (operator + Helm). - Eagerly validate additionalProviders (existence + unique upstream names) at LLM proxy Create/Update, covered by Create/Update tests. --- .../api/management-openapi.yaml | 1 + .../pkg/config/llm_validator.go | 5 + .../api/v1alpha1/llmproxy_types.go | 29 ++++ .../api/v1alpha1/zz_generated.deepcopy.go | 25 ++++ ...eway.api-platform.wso2.com_llmproxies.yaml | 25 ++++ ...eway.api-platform.wso2.com_llmproxies.yaml | 25 ++++ platform-api/internal/service/llm.go | 42 ++++++ platform-api/internal/service/llm_test.go | 132 ++++++++++++++++-- 8 files changed, 276 insertions(+), 8 deletions(-) diff --git a/gateway/gateway-controller/api/management-openapi.yaml b/gateway/gateway-controller/api/management-openapi.yaml index 2cce8204dc..9a3b00cd7a 100644 --- a/gateway/gateway-controller/api/management-openapi.yaml +++ b/gateway/gateway-controller/api/management-openapi.yaml @@ -5910,6 +5910,7 @@ components: description: > Major-only translator policy version (for example v1). The Gateway Controller resolves it to the installed full version. + pattern: '^v\d+$' minLength: 1 example: v1 params: diff --git a/gateway/gateway-controller/pkg/config/llm_validator.go b/gateway/gateway-controller/pkg/config/llm_validator.go index 2eb2a1cd70..f521c57a24 100644 --- a/gateway/gateway-controller/pkg/config/llm_validator.go +++ b/gateway/gateway-controller/pkg/config/llm_validator.go @@ -659,6 +659,11 @@ func (v *LLMValidator) validateLLMProxyTransformer(fieldPrefix string, transform Field: fieldPrefix + ".version", Message: "Transformer version is required", }) + } else if !majorVersionPattern.MatchString(transformer.Version) { + errors = append(errors, ValidationError{ + Field: fieldPrefix + ".version", + Message: "Transformer version must be major-only (e.g. v1)", + }) } return errors } diff --git a/kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go b/kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go index b4b29e2ee4..f25baeef08 100644 --- a/kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go +++ b/kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go @@ -18,6 +18,7 @@ package v1alpha1 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) // LLMProxyProvider references a deployed LlmProvider that this proxy fronts. @@ -47,6 +48,34 @@ type LLMProxyAdditionalProvider struct { // calls when the referenced provider is protected by an auth policy. // +optional Auth *LLMUpstreamAuth `json:"auth,omitempty"` + + // Transformer optionally applies a request/response translator when this + // provider is the selected upstream. The proxy injects it as a conditional + // policy that runs only when this provider is selected. + // +optional + Transformer *LLMProxyTransformer `json:"transformer,omitempty"` +} + +// LLMProxyTransformer is a request/response translator applied when its owning +// provider is the selected upstream; mirrors the management-API +// LLMProxyTransformer payload. +type LLMProxyTransformer struct { + // Type is the translator policy name (for example openai-to-anthropic). + // +kubebuilder:validation:Required + Type string `json:"type"` + + // Version is the major-only translator policy version (for example v1). + // The Gateway Controller resolves it to the installed full version. + // +kubebuilder:validation:Required + // +kubebuilder:validation:Pattern=`^v\d+$` + Version string `json:"version"` + + // Params carries translator-specific parameters (for example model, + // apiVersion). + // +optional + // +kubebuilder:pruning:PreserveUnknownFields + // +kubebuilder:validation:Schemaless + Params *runtime.RawExtension `json:"params,omitempty"` } // LLMProxyConfigData mirrors the management-API LLMProxyConfigData payload. diff --git a/kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go b/kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go index 49b649ecc0..348c7ff78b 100644 --- a/kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go +++ b/kubernetes/gateway-operator/api/v1alpha1/zz_generated.deepcopy.go @@ -974,6 +974,11 @@ func (in *LLMProxyAdditionalProvider) DeepCopyInto(out *LLMProxyAdditionalProvid *out = new(LLMUpstreamAuth) (*in).DeepCopyInto(*out) } + if in.Transformer != nil { + in, out := &in.Transformer, &out.Transformer + *out = new(LLMProxyTransformer) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LLMProxyAdditionalProvider. @@ -1051,6 +1056,26 @@ func (in *LLMProxyProvider) DeepCopy() *LLMProxyProvider { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LLMProxyTransformer) DeepCopyInto(out *LLMProxyTransformer) { + *out = *in + if in.Params != nil { + in, out := &in.Params, &out.Params + *out = new(runtime.RawExtension) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LLMProxyTransformer. +func (in *LLMProxyTransformer) DeepCopy() *LLMProxyTransformer { + if in == nil { + return nil + } + out := new(LLMProxyTransformer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LLMUpstreamAuth) DeepCopyInto(out *LLMUpstreamAuth) { *out = *in diff --git a/kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml b/kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml index b5c56018f1..fb71af5fe6 100644 --- a/kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml +++ b/kubernetes/gateway-operator/config/crd/bases/gateway.api-platform.wso2.com_llmproxies.yaml @@ -117,6 +117,31 @@ spec: id: description: Id is the LlmProvider handle (metadata.name). type: string + transformer: + description: |- + Transformer optionally applies a request/response translator when this + provider is the selected upstream. The proxy injects it as a conditional + policy that runs only when this provider is selected. + properties: + params: + description: |- + Params carries translator-specific parameters (for example model, + apiVersion). + x-kubernetes-preserve-unknown-fields: true + type: + description: Type is the translator policy name (for example + openai-to-anthropic). + type: string + version: + description: |- + Version is the major-only translator policy version (for example v1). + The Gateway Controller resolves it to the installed full version. + pattern: ^v\d+$ + type: string + required: + - type + - version + type: object required: - id type: object diff --git a/kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml b/kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml index b5c56018f1..fb71af5fe6 100644 --- a/kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml +++ b/kubernetes/helm/operator-helm-chart/crds/gateway.api-platform.wso2.com_llmproxies.yaml @@ -117,6 +117,31 @@ spec: id: description: Id is the LlmProvider handle (metadata.name). type: string + transformer: + description: |- + Transformer optionally applies a request/response translator when this + provider is the selected upstream. The proxy injects it as a conditional + policy that runs only when this provider is selected. + properties: + params: + description: |- + Params carries translator-specific parameters (for example model, + apiVersion). + x-kubernetes-preserve-unknown-fields: true + type: + description: Type is the translator policy name (for example + openai-to-anthropic). + type: string + version: + description: |- + Version is the major-only translator policy version (for example v1). + The Gateway Controller resolves it to the installed full version. + pattern: ^v\d+$ + type: string + required: + - type + - version + type: object required: - id type: object diff --git a/platform-api/internal/service/llm.go b/platform-api/internal/service/llm.go index f073382340..80119a8d31 100644 --- a/platform-api/internal/service/llm.go +++ b/platform-api/internal/service/llm.go @@ -1248,6 +1248,38 @@ func (s *LLMProviderService) Delete(orgUUID, handle, deletedBy string) error { return nil } +// validateAdditionalProviders eagerly validates a proxy's additional providers +// so Create/Update surface an immediate, actionable API error instead of a +// confusing deployment-time failure. It mirrors the checks the gateway performs +// at transform time (see llm_transformer.go): every referenced provider must +// exist, and each upstream name (the `as` alias, or the provider id when no +// alias is set) must be unique within the proxy and must not collide with the +// primary provider id. +func (s *LLMProxyService) validateAdditionalProviders(orgUUID, primaryProviderID string, additionalProviders *[]api.LLMProxyAdditionalProvider) error { + if additionalProviders == nil { + return nil + } + seen := map[string]bool{primaryProviderID: true} + for _, ap := range *additionalProviders { + prov, err := s.providerRepo.GetByID(ap.Id, orgUUID) + if err != nil { + return fmt.Errorf("failed to validate additional provider %q: %w", ap.Id, err) + } + if prov == nil { + return constants.ErrLLMProviderNotFound + } + name := ap.Id + if ap.As != nil && *ap.As != "" { + name = *ap.As + } + if seen[name] { + return constants.ErrInvalidInput + } + seen[name] = true + } + return nil +} + func (s *LLMProxyService) Create(orgUUID, createdBy string, req *api.LLMProxy) (*api.LLMProxy, error) { if req == nil { return nil, constants.ErrInvalidInput @@ -1287,6 +1319,11 @@ func (s *LLMProxyService) Create(orgUUID, createdBy string, req *api.LLMProxy) ( return nil, constants.ErrLLMProviderNotFound } + // Validate additional providers exist and have unique upstream names + if err := s.validateAdditionalProviders(orgUUID, req.Provider.Id, req.AdditionalProviders); err != nil { + return nil, err + } + // Determine handle: use provided id or auto-generate from displayName var handle string if req.Id != nil && *req.Id != "" { @@ -1590,6 +1627,11 @@ func (s *LLMProxyService) Update(orgUUID, handle, updatedBy string, req *api.LLM } } + // Validate additional providers exist and have unique upstream names + if err := s.validateAdditionalProviders(orgUUID, req.Provider.Id, req.AdditionalProviders); err != nil { + return nil, err + } + contextValue := utils.DefaultStringPtr(req.Context, "/") m := &model.LLMProxy{ OrganizationUUID: orgUUID, diff --git a/platform-api/internal/service/llm_test.go b/platform-api/internal/service/llm_test.go index c16de920ec..0d2349af9a 100644 --- a/platform-api/internal/service/llm_test.go +++ b/platform-api/internal/service/llm_test.go @@ -1746,10 +1746,10 @@ func TestLLMProxyServiceUpdate_CleansUpRotatedSecret(t *testing.T) { func validProviderRequest(template string) *api.LLMProvider { return &api.LLMProvider{ - Id: strPointer("provider-1"), - DisplayName: "Test Provider", - Version: "v1.0", - Template: template, + Id: strPointer("provider-1"), + DisplayName: "Test Provider", + Version: "v1.0", + Template: template, Upstream: api.Upstream{ Main: api.UpstreamDefinition{Url: stringPtr("https://example.com/openai/v1")}, }, @@ -1759,10 +1759,10 @@ func validProviderRequest(template string) *api.LLMProvider { func validProxyRequest(providerID, projectID string) *api.LLMProxy { return &api.LLMProxy{ - Id: strPointer("proxy-1"), - DisplayName: "Test Proxy", - Version: "v1.0", - ProjectId: projectID, + Id: strPointer("proxy-1"), + DisplayName: "Test Proxy", + Version: "v1.0", + ProjectId: projectID, Provider: api.LLMProxyProvider{ Id: providerID, }, @@ -1777,3 +1777,119 @@ func upstreamAuthTypePtr(v string) *api.UpstreamAuthType { t := api.UpstreamAuthType(v) return &t } + +func TestLLMProxyServiceCreateFailsWhenAdditionalProviderNotFound(t *testing.T) { + proxyRepo := &mockLLMProxyRepo{} + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + if providerID == "provider-1" { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + } + return nil, nil + }, + } + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + + req := validProxyRequest("provider-1", "project-1") + req.AdditionalProviders = &[]api.LLMProxyAdditionalProvider{{Id: "missing-provider"}} + + if _, err := service.Create("org-1", "alice", req); err != constants.ErrLLMProviderNotFound { + t.Fatalf("expected ErrLLMProviderNotFound, got: %v", err) + } +} + +func TestLLMProxyServiceCreateFailsWhenAdditionalProviderNameCollides(t *testing.T) { + proxyRepo := &mockLLMProxyRepo{} + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + }, + } + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + + req := validProxyRequest("provider-1", "project-1") + // The additional provider exists, but its upstream `as` name collides with + // the primary provider id, which the gateway rejects at transform time. + req.AdditionalProviders = &[]api.LLMProxyAdditionalProvider{ + {Id: "provider-2", As: stringPtr("provider-1")}, + } + + if _, err := service.Create("org-1", "alice", req); err != constants.ErrInvalidInput { + t.Fatalf("expected ErrInvalidInput, got: %v", err) + } +} + +func TestLLMProxyServiceUpdateFailsWhenAdditionalProviderNotFound(t *testing.T) { + now := time.Now() + proxyRepo := &mockLLMProxyRepo{} + proxyRepo.getByIDFunc = func(proxyID, orgUUID string) (*model.LLMProxy, error) { + return &model.LLMProxy{ + UUID: "proxy-uuid", + ID: proxyID, + Name: "Old Proxy", + Version: "v1.0", + ProjectUUID: "project-1", + ProviderUUID: "provider-uuid", + CreatedAt: now, + UpdatedAt: now, + Configuration: model.LLMProxyConfig{Provider: "provider-1"}, + }, nil + } + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + if providerID == "provider-1" { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + } + return nil, nil + }, + } + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + + req := validProxyRequest("provider-1", "project-1") + req.AdditionalProviders = &[]api.LLMProxyAdditionalProvider{{Id: "missing-provider"}} + + if _, err := service.Update("org-1", "proxy-1", "test-user", req); err != constants.ErrLLMProviderNotFound { + t.Fatalf("expected ErrLLMProviderNotFound, got: %v", err) + } + if proxyRepo.updated != nil { + t.Fatalf("expected update to be rejected before persisting, but proxy was updated") + } +} + +func TestLLMProxyServiceUpdateFailsWhenAdditionalProviderNameCollides(t *testing.T) { + now := time.Now() + proxyRepo := &mockLLMProxyRepo{} + proxyRepo.getByIDFunc = func(proxyID, orgUUID string) (*model.LLMProxy, error) { + return &model.LLMProxy{ + UUID: "proxy-uuid", + ID: proxyID, + Name: "Old Proxy", + Version: "v1.0", + ProjectUUID: "project-1", + ProviderUUID: "provider-uuid", + CreatedAt: now, + UpdatedAt: now, + Configuration: model.LLMProxyConfig{Provider: "provider-1"}, + }, nil + } + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + }, + } + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + + req := validProxyRequest("provider-1", "project-1") + // Two additional providers resolve to the same upstream `as` name. + req.AdditionalProviders = &[]api.LLMProxyAdditionalProvider{ + {Id: "provider-2", As: stringPtr("shared")}, + {Id: "provider-3", As: stringPtr("shared")}, + } + + if _, err := service.Update("org-1", "proxy-1", "test-user", req); err != constants.ErrInvalidInput { + t.Fatalf("expected ErrInvalidInput, got: %v", err) + } + if proxyRepo.updated != nil { + t.Fatalf("expected update to be rejected before persisting, but proxy was updated") + } +}