Support multiple independently-credentialed instances of the same provider type#351
Support multiple independently-credentialed instances of the same provider type#351gr3enarr0w wants to merge 22 commits into
Conversation
…ype providers Adds Config.ProviderInstances ([]ProviderInstanceConfig) so operators can declare additional, independently-credentialed instances of an existing canonical provider type (e.g. two separate Ollama Cloud accounts), each addressable under its own routing alias via targets[].virtual_key with its own concurrency/retry/circuit-breaker settings. Credentials use the same key names as providers.CfgKey* and are left unresolved here, per the internal/envref convention (resolution happens at provider construction). This is the config-schema piece only; registration, capability resolution, and cost routing are separate work-streams. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds ValidateConfig checks for the new ProviderInstances field: each alias must be non-empty, unique, and not shadow a canonical provider name, and each instance's type must resolve to a known provider. Bedrock-typed instances pass validation but are noted as unsupported by bootstrap registration in v1.
…nct aliases Registry.RegisterAs and Gateway.RegisterProviderAs key by routing alias instead of a provider's own Name(), so two independently-credentialed instances of the same provider type (e.g. two Ollama Cloud accounts) can be registered as separate routing targets without one overwriting the other. CanonicalType/canonicalProviderType resolve an alias back to its real provider type for catalog-derived model listing. Register/ RegisterProvider remain pure wrappers with identical existing behavior.
Build and register a providers.Provider for each configured
ProviderInstances entry under its own routing alias, resolving ${VAR}
credential references at construction time via internal/envref rather
than at config load. An unknown Type or resolution failure warns and
skips the instance instead of failing gateway startup. BuildGateway now
carries each provider's canonical type through to the Gateway via
RegisterProviderAs instead of collapsing every registry entry to
RegisterProvider.
Inherited from local main's history via the earlier upstream/main merge (commit dce1e81, a local dev-tooling daemon artifact) - not part of this repo, not part of this feature.
…ranking Routing aggregators like OpenRouter route each request to a dynamically chosen upstream provider/model, so their catalog-listed price does not reliably represent what a given request will actually cost. Cost-optimized routing now treats aggregator-tagged candidates as unrankable on price (regardless of the configured unpriced-candidate strategy) while still allowing them as a last-resort target when no other candidate is viable. Also fixes cost-optimized routing to key catalog/pricing lookups by the provider's canonical type rather than its routing alias, so multi-instance providers (e.g. two independently-credentialed accounts of the same provider type) price correctly regardless of which alias handled the request.
Cost calculation on the success path and in the streaming cost meter built catalog lookup keys directly from the routing alias, which fails silently for multi-instance providers registered under an alias distinct from their real provider type (e.g. two independently-credentialed accounts of the same provider). recordSuccess now resolves the alias to its canonical type before the catalog lookup while leaving the metrics/log-facing provider label unchanged. streamwrap.MeterMeta gains an optional CatalogProvider field carrying the canonical type for cost lookups; when unset, behavior is unchanged. The gateway now populates it from the resolved streaming provider.
…ce responses Both the streaming and non-streaming request paths were losing the routing alias for multi-instance same-type providers (e.g. two Ollama Cloud accounts registered under distinct aliases via provider_instances). Non-streaming: responseWithProvider only stamped its "provider" argument (the routing alias) onto resp.Provider when resp.Provider was already empty. Every real provider's Complete() sets Response.Provider to its own hardcoded canonical name before the strategy layer ever sees it, so that guard never fired for real traffic and the alias was silently dropped. Fixed to unconditionally stamp the alias — verified every provider always sets its own name there, so there is no legitimate case where preserving the provider-reported value was ever correct; the one existing test asserting the old "preserve if non-empty" behavior encoded the bug itself using a synthetic mock and has been replaced. Streaming: RouteStream used sp.Name() for the metered Provider label, but sp is resolved through limitedProvider/cbProvider wrappers that delegate Name() straight to the underlying provider, never the alias used to look it up. resolveStreamOrError, resolveStreamProviderFromKeysLocked, and streamingProviderForTargetLocked now thread the routing key back to the caller so MeterMeta.Provider (and the AttrGenAISystem span attribute) get the alias. sp.Name() is no longer consulted for the metered label — it is only used, via canonicalProviderType, for the separate CatalogProvider field used in cost lookups. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
registry.List() returns routing alias keys, but the nil-gateway fallback path called RegisterProvider(p), which re-derives the registration key from the provider's own canonical name and discards the alias. Two aliased instances of the same provider type would collide, silently clobbering the first. Mirror BuildGateway's existing pattern: resolve each alias's canonical type and call RegisterProviderAs.
AllModels()'s catalog-precedence tier correctly stamps ModelInfo.OwnedBy with the routing alias (matching resp.Provider and MeterMeta.Provider elsewhere), but the /v1/models enrichment handler then looked up the catalog by that same alias. The catalog is keyed by canonical provider type only, so enrichment silently failed (zeroed pricing, context window, capabilities) for any aliased provider instance. Add an exported Gateway.CanonicalProviderType and thread it through so the catalog lookup key resolves to the canonical type while OwnedBy in the response still reports the alias.
ValidateConfig already rejects an instance alias that matches a canonical provider ID before RegisterProviderInstances runs, but the function itself trusted that invariant unconditionally. Add a cheap guard so a future call site that skips validation can't silently overwrite a canonical provider's registry entry; failures follow the same warn-and-skip contract as every other failure case in this function.
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@config_load.go`:
- Around line 205-209: Replace the providers.AllProviders() iteration in the
alias validation with a direct providers.GetProviderEntry(inst.Alias) lookup.
Return the existing collision error when the lookup finds a canonical provider
entry, preserving the current alias-shadowing behavior without allocating the
full provider list.
In `@gateway_test.go`:
- Around line 4796-4806: Protect the catalog fixture assignment in the test with
gw.mu, acquiring the appropriate mutex before replacing gw.catalog and releasing
it afterward. Keep the existing catalog contents unchanged and follow the
gateway’s sync.RWMutex locking conventions.
In `@gateway.go`:
- Around line 496-499: Update the model aggregation branch around catalog and
p.Models() so every model is copied and stamped with the routing alias name in
OwnedBy before appending. Apply this normalization consistently to
catalog-derived and provider-derived models, preserving the existing source
selection and model contents otherwise.
In `@internal/strategies/costoptimized_test.go`:
- Around line 382-456: Add streaming-capable mock providers and extend the
aggregator-selection tests to exercise SelectTargets independently of Execute.
Mirror the exclusion, single-aggregator last-resort behavior for each mode, and
non-aggregator predicate cases, ensuring assertions validate the selected target
and canonical catalog lookup for the streaming path.
In `@internal/strategies/costoptimized.go`:
- Around line 265-279: Keep aggregators as the last fallback in both affected
paths: in internal/strategies/costoptimized.go lines 265-279, select the first
model-supporting non-aggregator before calling firstAggregatorCandidate, while
preserving the existing error and final fallback behavior; in lines 191-195,
update the streaming order to place viable non-aggregators first, aggregators
second, and all remaining targets last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fce9814f-5f52-446d-98c6-8dade7bb65db
📒 Files selected for processing (28)
.gitignoreAGENTS.mdconfig.example.jsonconfig.example.yamlconfig.goconfig_load.goconfig_load_test.gogateway.gogateway_route.gogateway_strategy.gogateway_stream.gogateway_test.gointernal/bootstrap/bootstrap.gointernal/bootstrap/provider_instances_test.gointernal/handler/models.gointernal/handler/models_test.gointernal/httpserver/ensuregateway_test.gointernal/httpserver/router.gointernal/strategies/costoptimized.gointernal/strategies/costoptimized_test.gointernal/strategies/strategy.gointernal/strategies/strategy_test.gointernal/streamwrap/wrap.gointernal/streamwrap/wrap_test.goproviders/factory.goproviders/providers_list.goproviders/registry.goproviders/registry_test.go
| for _, entry := range providers.AllProviders() { | ||
| if inst.Alias == entry.ID { | ||
| return fmt.Errorf("provider_instances: alias %q collides with the canonical provider name %q; instance aliases must not shadow a built-in provider type", inst.Alias, entry.ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use providers.GetProviderEntry directly for alias collision checks.
You can replace the providers.AllProviders() loop with a direct lookup using providers.GetProviderEntry(inst.Alias). This is more concise, consistent with the collision check in bootstrap.go, and avoids allocating a new slice of all providers for every configured instance.
♻️ Proposed refactor
- for _, entry := range providers.AllProviders() {
- if inst.Alias == entry.ID {
- return fmt.Errorf("provider_instances: alias %q collides with the canonical provider name %q; instance aliases must not shadow a built-in provider type", inst.Alias, entry.ID)
- }
- }
+ if _, ok := providers.GetProviderEntry(inst.Alias); ok {
+ return fmt.Errorf("provider_instances: alias %q collides with the canonical provider name %q; instance aliases must not shadow a built-in provider type", inst.Alias, inst.Alias)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, entry := range providers.AllProviders() { | |
| if inst.Alias == entry.ID { | |
| return fmt.Errorf("provider_instances: alias %q collides with the canonical provider name %q; instance aliases must not shadow a built-in provider type", inst.Alias, entry.ID) | |
| } | |
| } | |
| if _, ok := providers.GetProviderEntry(inst.Alias); ok { | |
| return fmt.Errorf("provider_instances: alias %q collides with the canonical provider name %q; instance aliases must not shadow a built-in provider type", inst.Alias, inst.Alias) | |
| } |
🤖 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 `@config_load.go` around lines 205 - 209, Replace the providers.AllProviders()
iteration in the alias validation with a direct
providers.GetProviderEntry(inst.Alias) lookup. Return the existing collision
error when the lookup finds a canonical provider entry, preserving the current
alias-shadowing behavior without allocating the full provider list.
| gw.catalog = models.Catalog{ | ||
| "ollama-cloud/model-a": { | ||
| Provider: "ollama-cloud", | ||
| ModelID: "model-a", | ||
| Mode: models.ModeChat, | ||
| Pricing: models.Pricing{ | ||
| InputPerMTokens: ptrFloat64(1.0), | ||
| OutputPerMTokens: ptrFloat64(2.0), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the catalog fixture assignment with gw.mu.
New starts background catalog refresh, so directly replacing gw.catalog can race with that worker under -race.
Proposed fix
+ gw.mu.Lock()
gw.catalog = models.Catalog{
"ollama-cloud/model-a": {
Provider: "ollama-cloud",
ModelID: "model-a",
Mode: models.ModeChat,
Pricing: models.Pricing{
InputPerMTokens: ptrFloat64(1.0),
OutputPerMTokens: ptrFloat64(2.0),
},
},
}
+ gw.mu.Unlock()As per coding guidelines, “Protect concurrent gateway state with sync.RWMutex.” <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gw.catalog = models.Catalog{ | |
| "ollama-cloud/model-a": { | |
| Provider: "ollama-cloud", | |
| ModelID: "model-a", | |
| Mode: models.ModeChat, | |
| Pricing: models.Pricing{ | |
| InputPerMTokens: ptrFloat64(1.0), | |
| OutputPerMTokens: ptrFloat64(2.0), | |
| }, | |
| }, | |
| } | |
| gw.mu.Lock() | |
| gw.catalog = models.Catalog{ | |
| "ollama-cloud/model-a": { | |
| Provider: "ollama-cloud", | |
| ModelID: "model-a", | |
| Mode: models.ModeChat, | |
| Pricing: models.Pricing{ | |
| InputPerMTokens: ptrFloat64(1.0), | |
| OutputPerMTokens: ptrFloat64(2.0), | |
| }, | |
| }, | |
| } | |
| gw.mu.Unlock() |
🤖 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_test.go` around lines 4796 - 4806, Protect the catalog fixture
assignment in the test with gw.mu, acquiring the appropriate mutex before
replacing gw.catalog and releasing it afterward. Keep the existing catalog
contents unchanged and follow the gateway’s sync.RWMutex locking conventions.
Source: Coding guidelines
| } else if catModels := g.catalog.ModelsForProvider(g.canonicalProviderTypeLocked(name)); len(catModels) > 0 { | ||
| models = append(models, core.ModelsFromList(name, catModels)...) | ||
| } else { | ||
| models = append(models, p.Models()...) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize OwnedBy to the routing alias in every model-source branch.
Only the catalog branch creates models with OwnedBy = name; p.Models() can retain the canonical provider name, making same-type instances indistinguishable in /v1/models. Copy and stamp models from every branch with the routing alias before appending.
🤖 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.go` around lines 496 - 499, Update the model aggregation branch
around catalog and p.Models() so every model is copied and stamped with the
routing alias name in OwnedBy before appending. Apply this normalization
consistently to catalog-derived and provider-derived models, preserving the
existing source selection and model contents otherwise.
| func TestCostOptimized_AggregatorExcludedFromRankingEvenWhenCheapest(t *testing.T) { | ||
| for _, mode := range []string{"fallback", "skip", "allow"} { | ||
| t.Run(mode, func(t *testing.T) { | ||
| catalog, aggregator, normal := aggregatorCatalogAndTargets() | ||
| targets := []Target{{VirtualKey: "aggregator"}, {VirtualKey: "normal"}} | ||
| s := NewCostOptimized(targets, newLookup(aggregator, normal), catalog, mode). | ||
| WithAggregatorPredicate(aggregatorPredicate) | ||
|
|
||
| resp, err := s.Execute(context.Background(), providers.Request{ | ||
| Model: "gpt-4o", | ||
| Messages: []providers.Message{{Role: "user", Content: "hello world"}}, | ||
| }) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if resp.ID != "normal" { | ||
| t.Errorf("mode %s: expected non-aggregator to win despite aggregator looking cheaper, got %q", mode, resp.ID) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCostOptimized_AggregatorUsedAsLastResortWhenOnlyCandidate(t *testing.T) { | ||
| for _, mode := range []string{"fallback", "skip", "allow"} { | ||
| t.Run(mode, func(t *testing.T) { | ||
| catalog, aggregator, _ := aggregatorCatalogAndTargets() | ||
| targets := []Target{{VirtualKey: "aggregator"}} | ||
| s := NewCostOptimized(targets, newLookup(aggregator), catalog, mode). | ||
| WithAggregatorPredicate(aggregatorPredicate) | ||
|
|
||
| resp, err := s.Execute(context.Background(), providers.Request{ | ||
| Model: "gpt-4o", | ||
| Messages: []providers.Message{{Role: "user", Content: "hello world"}}, | ||
| }) | ||
| if mode == "skip" { | ||
| // skip mode requires a priced candidate, but the aggregator | ||
| // should still be usable as a last resort since it's the | ||
| // only candidate at all. | ||
| if err != nil { | ||
| t.Fatalf("expected aggregator last-resort fallback in skip mode, got error: %v", err) | ||
| } | ||
| } else if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if resp.ID != "aggregator" { | ||
| t.Errorf("mode %s: expected aggregator as last-resort fallback, got %q", mode, resp.ID) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCostOptimized_NonAggregatorCandidateUnaffectedByAggregatorPredicate(t *testing.T) { | ||
| cheap := &mockProvider{name: "cheap", models: []string{"gpt-4o"}, resp: &providers.Response{ID: "cheap"}} | ||
| expensive := &mockProvider{name: "expensive", models: []string{"gpt-4o"}, resp: &providers.Response{ID: "expensive"}} | ||
|
|
||
| catalog := buildCatalog() | ||
| targets := []Target{{VirtualKey: "cheap"}, {VirtualKey: "expensive"}} | ||
| // Predicate never matches, so neither candidate is treated as an | ||
| // aggregator — behavior should be identical to the non-aggregator-aware | ||
| // baseline test (TestCostOptimized_PicksCheapest). | ||
| s := NewCostOptimized(targets, newLookup(cheap, expensive), catalog). | ||
| WithAggregatorPredicate(func(string) bool { return false }) | ||
|
|
||
| resp, err := s.Execute(context.Background(), providers.Request{ | ||
| Model: "gpt-4o", | ||
| Messages: []providers.Message{{Role: "user", Content: "hello world"}}, | ||
| }) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if resp.ID != "cheap" { | ||
| t.Errorf("expected cheap provider, got %q", resp.ID) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the separate streaming target-selection path.
These tests only exercise Execute, while SelectTargets independently implements aggregator exclusion and canonical catalog lookup. Add stream-capable providers and mirror these assertions through SelectTargets to prevent streaming-only regressions.
Also applies to: 484-550
🤖 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 `@internal/strategies/costoptimized_test.go` around lines 382 - 456, Add
streaming-capable mock providers and extend the aggregator-selection tests to
exercise SelectTargets independently of Execute. Mirror the exclusion,
single-aggregator last-resort behavior for each mode, and non-aggregator
predicate cases, ensuring assertions validate the selected target and canonical
catalog lookup for the streaming path.
| // Before giving up entirely, allow an aggregator to serve as a | ||
| // last-resort fallback if it's literally the only candidate that | ||
| // supports the model — better to route somewhere than nowhere. | ||
| if agg := firstAggregatorCandidate(candidates); agg != nil { | ||
| return agg, nil | ||
| } | ||
| // No cataloged/priced candidate is selectable; return an error. | ||
| return nil, fmt.Errorf("no priced provider supports model %s", model) | ||
| } | ||
| // Preserve historical fallback behavior: when no cataloged/priced candidate | ||
| // is selectable, fallback and allow route to the first compatible target. | ||
| // If the first candidate happens to be an aggregator excluded above, an | ||
| // aggregator is still an acceptable fallback here — it's the same | ||
| // last-resort case as any other unpriced candidate in these modes. | ||
| return &candidates[0], nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep aggregators last in every fallback path.
The fallback branches use original candidate order rather than preferring compatible non-aggregators. This can route through an aggregator while a direct provider remains available; streaming skip mode can also put an unpriced non-aggregator before the aggregator exception.
internal/strategies/costoptimized.go#L265-L279: return the first model-supporting non-aggregator before consideringfirstAggregatorCandidate.internal/strategies/costoptimized.go#L191-L195: build streaming order as viable non-aggregators first, aggregators second, then remaining targets.
📍 Affects 1 file
internal/strategies/costoptimized.go#L265-L279(this comment)internal/strategies/costoptimized.go#L191-L195
🤖 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 `@internal/strategies/costoptimized.go` around lines 265 - 279, Keep
aggregators as the last fallback in both affected paths: in
internal/strategies/costoptimized.go lines 265-279, select the first
model-supporting non-aggregator before calling firstAggregatorCandidate, while
preserving the existing error and final fallback behavior; in lines 191-195,
update the streaming order to place viable non-aggregators first, aggregators
second, and all remaining targets last.
- Simplify the alias/canonical-provider collision check to a direct lookup instead of scanning all providers. - Ensure AllModels() stamps the routing alias on every model-source tier (discovery and hardcoded fallback), not only the catalog tier, so aliased provider instances are distinguishable in /v1/models. - Guard a direct test assignment to gateway state with the gateway mutex to avoid a race with the background catalog refresh. - Fix two cost-optimized strategy fallback paths so a non-aggregator candidate is preferred over an aggregator purely by config order; aggregators are still used as a genuine last resort. - Add regression tests covering the fallback ordering fixes.
…-providers # Conflicts: # gateway_test.go # internal/strategies/strategy_test.go
Correct the import path for the metrics package (internal/metrics) introduced when resolving the merge conflict with upstream's test suite refactor.
Summary
Adds
provider_instances, a config block that lets an operator register multiple independently-credentialed instances of the same provider type (e.g. two separate Ollama Cloud accounts), each addressable as its own routing target with its own retry/circuit-breaker/concurrency settings.Type of change
Related issues
N/A
Changes
provider_instancesconfig block: each entry declares analias, a canonicaltype(aproviders.ProviderEntry.ID, e.g.ollama-cloud), andcredentials(same key names asproviders.CfgKey*, with${VAR}env interpolation resolved at provider-construction time, not config load).provider_instancesaliases and types (config.go,config_load.go), with a defense-in-depth alias-collision check inRegisterProviderInstances.internal/bootstrap/bootstrap.go) registers each instance under its alias so it's addressable intargets[].virtual_keyexactly like a built-in provider name./v1/modelscatalog enrichment, cost lookups), while multi-instance responses are stamped with the routing alias rather than the provider's own name.CapabilityAggregatorand excluded aggregators from cost-optimized ranking.AGENTS.mdandconfig.example.yaml/config.example.jsondocument the new block with a worked two-account Ollama Cloud example (each capped atmax_concurrency: 3).Testing
go test ./...)Config validation and provider-instance registration were exercised with unit tests and a mock/fake-credential smoke test of the
ferrogwbinary; no live calls were made to Ollama Cloud or any other provider.Provider checklist (fill in only for new/updated providers)
N/A — no new provider package; this adds a mechanism for reusing existing provider types under multiple credential sets.
Breaking change notes
None.
provider_instancesis opt-in and additive; existing configs are unaffected.Notes for reviewers
The branch's commit history includes a few small housekeeping commits (
drop .superset/ and CODEX.md tooling scaffold, gitignore it,finish staging .gitignore update from previous commit) that remove an unrelated local dev-tooling scaffold that had ended up in the branch's ancestry. Not part of the feature itself.Summary by CodeRabbit