Add gateway implementations for custom LLM provider feature#2456
Add gateway implementations for custom LLM provider feature#2456Tharanidk wants to merge 2 commits into
Conversation
Tharanidk
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesLLM provider template metadata fields
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
gateway/gateway-controller/api/management-openapi.yaml (1)
5569-5588: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding a
patternconstraint onversion.Other version fields in this spec (
APIConfigData.version,LLMProviderConfigData.version,LLMProxyConfigData.version) enforcepattern: '^v\d+\.\d+$', andPolicy.versionenforces'^v\d+$'. The newLLMProviderTemplateData.versionfield has no format constraint, so malformed values (e.g.,1.0,latest) could be accepted and persisted as template metadata.📝 Suggested diff
version: type: string description: | Template content version (e.g. v1.0). Multiple versions of the same groupId can coexist; defaults to v1.0 when omitted. + pattern: '^v\d+\.\d+$' example: v1.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/api/management-openapi.yaml` around lines 5569 - 5588, The new LLMProviderTemplateData.version field is missing the same format validation used by other version fields in the OpenAPI spec. Update the version property in management-openapi.yaml to add a pattern constraint consistent with the existing version schema (matching the other template/config version fields), so only valid version strings are accepted and persisted; keep the change localized to the LLMProviderTemplateData.version definition.gateway/gateway-controller/pkg/models/llm_provider_template.go (1)
46-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fallback behavior; consider extracting shared helper.
GetGroupID,GetVersion, andGetManagedByeach repeat the same nil-check/trim/fallback pattern. A shared private helper would reduce duplication and keep the trimming semantics consistent if this logic changes later.♻️ Suggested refactor
+func stringOrDefault(v *string, fallback string) string { + if v != nil { + if trimmed := strings.TrimSpace(*v); trimmed != "" { + return trimmed + } + } + return fallback +} + func (t *StoredLLMProviderTemplate) GetGroupID() string { - if t.Configuration.Spec.GroupId != nil { - if v := strings.TrimSpace(*t.Configuration.Spec.GroupId); v != "" { - return v - } - } - return t.Configuration.Metadata.Name + return stringOrDefault(t.Configuration.Spec.GroupId, t.Configuration.Metadata.Name) } func (t *StoredLLMProviderTemplate) GetVersion() string { - if t.Configuration.Spec.Version != nil { - if v := strings.TrimSpace(*t.Configuration.Spec.Version); v != "" { - return v - } - } - return DefaultTemplateVersion + return stringOrDefault(t.Configuration.Spec.Version, DefaultTemplateVersion) } func (t *StoredLLMProviderTemplate) GetManagedBy() string { - if t.Configuration.Spec.ManagedBy != nil { - if p := strings.TrimSpace(*t.Configuration.Spec.ManagedBy); p != "" { - return p - } - } - return DefaultTemplateManagedBy + return stringOrDefault(t.Configuration.Spec.ManagedBy, DefaultTemplateManagedBy) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/models/llm_provider_template.go` around lines 46 - 72, GetGroupID, GetVersion, and GetManagedBy all duplicate the same nil-check, trim, and fallback logic in StoredLLMProviderTemplate. Extract that shared behavior into a private helper on StoredLLMProviderTemplate that accepts the optional string pointer and fallback value, then have all three methods delegate to it so the trimming semantics stay consistent and future changes only need to be made in one place.gateway/gateway-controller/pkg/utils/llm_deployment.go (1)
595-616: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect defaulting logic and placement.
normalizeTemplateDefaultsis invoked after parsing and before render/validation, so validation and returned config both reflect the defaults — matches the documented OpenAPI fallback contract (groupId → metadata.name, version → v1.0, managedBy → customer) and is covered by tests.Note: the nil-check/trim/fallback logic here duplicates the same pattern in
StoredLLMProviderTemplate.GetGroupID/GetVersion/GetManagedBy(gateway/gateway-controller/pkg/models/llm_provider_template.go). Consider exposing a small shared helper frommodels(e.g., aNormalizeOptionalString(v *string, fallback string) *string) that both this function and the getters can use, to avoid the fallback semantics drifting between the two packages over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/utils/llm_deployment.go` around lines 595 - 616, The defaulting logic in normalizeTemplateDefaults is duplicated in StoredLLMProviderTemplate.GetGroupID/GetVersion/GetManagedBy, which risks the fallback behavior drifting over time. Extract the nil/trim/fallback handling into a shared helper in models, such as a NormalizeOptionalString-style function, and have both normalizeTemplateDefaults and the StoredLLMProviderTemplate getters use that shared helper so groupId, version, and managedBy stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/gateway-controller/api/management-openapi.yaml`:
- Around line 5569-5588: The new LLMProviderTemplateData.version field is
missing the same format validation used by other version fields in the OpenAPI
spec. Update the version property in management-openapi.yaml to add a pattern
constraint consistent with the existing version schema (matching the other
template/config version fields), so only valid version strings are accepted and
persisted; keep the change localized to the LLMProviderTemplateData.version
definition.
In `@gateway/gateway-controller/pkg/models/llm_provider_template.go`:
- Around line 46-72: GetGroupID, GetVersion, and GetManagedBy all duplicate the
same nil-check, trim, and fallback logic in StoredLLMProviderTemplate. Extract
that shared behavior into a private helper on StoredLLMProviderTemplate that
accepts the optional string pointer and fallback value, then have all three
methods delegate to it so the trimming semantics stay consistent and future
changes only need to be made in one place.
In `@gateway/gateway-controller/pkg/utils/llm_deployment.go`:
- Around line 595-616: The defaulting logic in normalizeTemplateDefaults is
duplicated in StoredLLMProviderTemplate.GetGroupID/GetVersion/GetManagedBy,
which risks the fallback behavior drifting over time. Extract the
nil/trim/fallback handling into a shared helper in models, such as a
NormalizeOptionalString-style function, and have both normalizeTemplateDefaults
and the StoredLLMProviderTemplate getters use that shared helper so groupId,
version, and managedBy stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38d5cf89-042b-4b5e-bbf7-216ed87092df
📒 Files selected for processing (13)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/default-llm-provider-templates/anthropic-template.yamlgateway/gateway-controller/default-llm-provider-templates/awsbedrock-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureaifoundry-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureopenai-template.yamlgateway/gateway-controller/default-llm-provider-templates/gemini-template.yamlgateway/gateway-controller/default-llm-provider-templates/mistral-template.yamlgateway/gateway-controller/default-llm-provider-templates/openai-template.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/models/llm_provider_template.gogateway/gateway-controller/pkg/models/llm_provider_template_test.gogateway/gateway-controller/pkg/utils/llm_deployment.gogateway/gateway-controller/pkg/utils/llm_deployment_test.go
| const DefaultTemplateVersion = "v1.0" | ||
|
|
||
| // DefaultTemplateManagedBy is used when a template does not declare managedBy. | ||
| const DefaultTemplateManagedBy = "customer" |
There was a problem hiding this comment.
For Custom Policies, we are using.
managedBy: customerShould we use the same for the LLM Provider template?
Or something like other?
managedBy: other
There was a problem hiding this comment.
We should use one approach ideally. Do we see any issue in managedBy: wso2/customer pair?
There was a problem hiding this comment.
Mm, yeah +1 for a consistent approach.
Ok. In the last code review meeting, we discussed changing this.
| name: my-llm-provider-template | ||
| spec: | ||
| displayName: OpenAI | ||
| groupId: anthropic |
There was a problem hiding this comment.
shall we change this to openai for consistency?
4 participants
Purpose
Extends the LLM provider template model with three new spec fields: groupId, managedBy, and version, and wires them through the gateway-controller management API.