Skip to content

[FIX] Address further review comments on multi-provider routing#2590

Merged
Arshardh merged 1 commit into
wso2:multi-provider-routingfrom
Aakashwije:fix/mpr-review-comments
Jul 11, 2026
Merged

[FIX] Address further review comments on multi-provider routing#2590
Arshardh merged 1 commit into
wso2:multi-provider-routingfrom
Aakashwije:fix/mpr-review-comments

Conversation

@Aakashwije

Copy link
Copy Markdown
Contributor

Purpose

This is a follow-up to #2586 (multi-provider routing for LLM proxies), addressing
review feedback raised on that PR. Three gaps were flagged:

  1. Transformer version accepted invalid formats. LLMProxyTransformer.version
    is documented as major-only (e.g. v1), but the schema only enforced
    minLength: 1, so values like v1.0 or latest were accepted at the API
    boundary and only failed later at gateway transform time, where the value is
    used as api.Policy.Version (which requires ^v\d+$).
  2. Operator CRD could not carry the transformer. The Kubernetes operator type
    LLMProxyAdditionalProvider had no Transformer field, so the generated CRDs
    silently pruned spec.additionalProviders[*].transformer — blocking
    conditional request/response translation for operator-managed proxies.
  3. No early validation of additional providers. LLM proxy Create/Update
    validated the primary provider.id but never additionalProviders[].id, so a
    reference to a nonexistent provider or a duplicate upstream name passed the
    API and only failed later at gateway transform time with a confusing
    deployment-time error.

Related to: #2586

Goals

  • Reject invalid transformer versions at the API boundary (schema + service-layer
    validator), matching the gateway's ^v\d+$ policy-version contract.
  • Let Kubernetes/operator users configure additionalProviders[*].transformer by
    adding the field to the operator API and regenerating the CRDs.
  • Surface an immediate, actionable API error for bad additionalProviders at
    Create/Update instead of a deferred gateway failure.

Approach

  • Transformer version format
    • Added pattern: '^v\d+$' to LLMProxyTransformer.version in
      gateway/gateway-controller/api/management-openapi.yaml.
    • Added a major-only check in LLMValidator.validateLLMProxyTransformer
      (gateway/gateway-controller/pkg/config/llm_validator.go), reusing the
      existing package-level majorVersionPattern so file-loaded configs (which
      bypass HTTP request-schema validation) are also guarded.
  • Operator transformer support
    • Added an LLMProxyTransformer type and a Transformer *LLMProxyTransformer
      field on LLMProxyAdditionalProvider in
      kubernetes/gateway-operator/api/v1alpha1/llmproxy_types.go (version marked
      +kubebuilder:validation:Pattern=^v\d+$``, params modeled as a schemaless
      runtime.RawExtension, mirroring existing policy params).
    • Regenerated zz_generated.deepcopy.go and both CRD artifacts (operator
      config/crd/bases/…_llmproxies.yaml and the Helm chart copy) with the
      project-pinned controller-gen v0.16.5. No controller wiring was needed —
      the controller serializes the whole spec to JSON, so the new field flows
      through automatically.
  • Eager additional-provider validation
    • Added a shared validateAdditionalProviders helper in
      platform-api/internal/service/llm.go, called from both Create and
      Update before the model is built. It mirrors the gateway's transform-time
      checks: every referenced provider must exist, and each upstream name (the
      as alias, or id when unset) must be unique within the proxy and must not
      collide with the primary provider id.

User stories

  • As an API developer, when I configure an LLM proxy transformer with an invalid
    version, I get an immediate validation error instead of a deferred
    deployment-time failure.
  • As a platform engineer using the Kubernetes operator, I can declare a
    per-provider request/response transformer on an additional provider and have it
    applied.
  • As an API developer, when I reference a nonexistent additional provider or
    reuse an upstream name, Create/Update fails fast with a clear error.

Documentation

N/A — no user-facing doc impact. Changes are schema-level constraints, an operator
CRD field addition, and stricter server-side validation; the existing
multi-provider routing documentation from #2586 already describes the transformer
and additionalProviders shape.

Automation tests

  • Unit tests
    • Added 4 unit tests in platform-api/internal/service/llm_test.go covering the
      new additional-provider validation on both handlers:
      • TestLLMProxyServiceCreateFailsWhenAdditionalProviderNotFound
      • TestLLMProxyServiceCreateFailsWhenAdditionalProviderNameCollides
      • TestLLMProxyServiceUpdateFailsWhenAdditionalProviderNotFound
      • TestLLMProxyServiceUpdateFailsWhenAdditionalProviderNameCollides
        They assert ErrLLMProviderNotFound / ErrInvalidInput and that Update is
        rejected before persisting.
    • go test ./internal/service/ (platform-api) and go test ./pkg/config/
      (gateway-controller) pass.
    • The operator type/CRD/deepcopy changes are verified via go build ./... and
      go vet ./api/...; the transformer version validator and OpenAPI pattern
      are enforced by existing schema/validation paths (no dedicated new unit test
      added for those two — they reuse the already-tested major-only convention).
  • Integration tests
    • None added; the change set is schema/validation and generated CRDs.

Security checks

  • Followed secure coding standards in WSO2 secure engineering guidelines? yes
  • Ran FindSecurityBugs plugin and verified report? N/A - FindSecurityBugs is a
    Java/SpotBugs plugin; the modified code is Go (gateway-controller, platform-api,
    operator).
  • Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or
    other secrets? yes — the commit contains exactly 8 source/generated files;
    the locally generated platform-api/config/data/secret-encryption.key was
    explicitly excluded and never staged.

Samples

gateway/examples/openai-multi-provider-proxy.yaml (from #2586) exercises
additionalProviders[*].transformer with major-only versions (v1), which the
new schema pattern, validator, and eager validation all accept.

Related PRs

Test environment

  • Language/runtime: Go 1.26.5
  • OS: macOS (darwin 24.6.0)
  • Database: SQLite (platform-api); service-layer unit tests use in-memory
    repository mocks, no live DB required
  • Browser: N/A (no UI changes)

- 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.
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8cdffec7-9a35-42dd-80cc-77f77aaee9b4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@Arshardh
Arshardh merged commit f7ce040 into wso2:multi-provider-routing Jul 11, 2026
2 checks passed
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.

2 participants