Uh oh!
There was an error while loading. Please reload this page.
feat(model): split provider_config inline into ProviderKey reference - #102
Merged
Conversation
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the Model domain struct to externalize provider credentials and configuration from the Model into ProviderKey resources; Model fields change from (name, model, provider_config) to (display_name, provider, model_name, provider_key_id). ProviderConfig is removed. A new proxy dispatch module centralizes ProviderKey resolution, secret validation, and base URL override/fallback. BridgeContext is extended to carry both Model and ProviderKey; bridges and proxy endpoints are updated to derive upstream config from ProviderKey. Tests and fixtures across admin, proxy, gateway, providers, etcd, and e2e are migrated to the new shape and helpers. ChangesModel Domain Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the Model resource to stop embedding provider credentials inline and instead reference a standalone ProviderKey via provider_key_id, aligning DP schemas and proxy dispatch with the normalized control-plane shape.
Changes:
- Updated
aisix-coreModelstruct + JSON Schema to the new{display_name, provider, model_name, provider_key_id}shape with a routing-vs-directoneOfXOR. - Added proxy-side dispatch helpers to resolve
ProviderKey+ compute base URLs, and updated all proxy endpoints/bridges to useBridgeContext { model, provider_key }. - Updated admin/API handlers, etcd loader tests, provider bridges, and e2e fixtures to seed
ProviderKeyresources and reference them from models.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/src/harness/admin.ts | Adds e2e helper for creating ProviderKey via admin API. |
| tests/e2e/src/cases/smoke.test.ts | Updates smoke test to create a ProviderKey and reference it from Model. |
| crates/aisix-proxy/src/responses.rs | Switches /v1/responses dispatch to resolve ProviderKey and new model fields. |
| crates/aisix-proxy/src/rerank.rs | Switches rerank dispatch to use ProviderKey.secret and api_base. |
| crates/aisix-proxy/src/passthrough.rs | Uses provider-based model selection to find a ProviderKey for passthrough calls. |
| crates/aisix-proxy/src/models.rs | Lists models using display_name and new provider field access. |
| crates/aisix-proxy/src/messages.rs | Switches /v1/messages dispatch to resolve ProviderKey and new model fields. |
| crates/aisix-proxy/src/lib.rs | Wires new dispatch module and updates proxy routing tests to seed provider keys. |
| crates/aisix-proxy/src/images.rs | Builds BridgeContext with provider_key for images endpoint. |
| crates/aisix-proxy/src/embeddings.rs | Builds BridgeContext with provider_key for embeddings endpoint. |
| crates/aisix-proxy/src/dispatch.rs | New shared helpers: resolve ProviderKey, require provider/model_name, resolve base URL, require secret. |
| crates/aisix-proxy/src/completions.rs | Builds BridgeContext with provider_key for completions endpoint. |
| crates/aisix-proxy/src/chat.rs | Updates streaming + routing paths to resolve ProviderKey and use display_name. |
| crates/aisix-proxy/src/audio.rs | Updates audio endpoints to resolve ProviderKey and compute base URL from it. |
| crates/aisix-provider-openai/src/bridge.rs | Reads secret/api_base from ctx.provider_key and upstream model from ctx.model.model_name. |
| crates/aisix-provider-gemini/src/lib.rs | Updates provider tests to construct BridgeContext with a ProviderKey. |
| crates/aisix-provider-deepseek/src/lib.rs | Updates provider tests to construct BridgeContext with a ProviderKey. |
| crates/aisix-provider-anthropic/src/bridge.rs | Reads secret/api_base from ctx.provider_key and upstream model from ctx.model.model_name. |
| crates/aisix-gateway/src/hub.rs | Updates hub test context to include ProviderKey. |
| crates/aisix-gateway/src/bridge.rs | Extends BridgeContext to include provider_key and updates tests accordingly. |
| crates/aisix-etcd/src/supervisor.rs | Updates etcd supervisor tests’ model fixtures to the new schema. |
| crates/aisix-etcd/src/loader.rs | Updates loader tests for new model schema and provider enum validation. |
| crates/aisix-core/src/models/snapshot.rs | Updates snapshot tests’ model fixture to the new schema. |
| crates/aisix-core/src/models/schema.rs | Replaces legacy model schema with new fields + direct-vs-routing oneOf XOR and cost block. |
| crates/aisix-core/src/models/model.rs | Refactors Model struct, removes ProviderConfig, updates docs/tests and Resource::name(). |
| crates/aisix-core/src/models/mod.rs | Updates exports to drop ProviderConfig. |
| crates/aisix-core/src/lib.rs | Updates public re-exports to drop ProviderConfig. |
| crates/aisix-admin/tests/etcd_integration.rs | Updates admin etcd integration tests’ model payloads to new fields. |
| crates/aisix-admin/src/store.rs | Updates store tests and model field references to display_name. |
| crates/aisix-admin/src/playground_handler.rs | Updates playground handler tests to seed ProviderKey and reference it from Model. |
| crates/aisix-admin/src/models_handlers.rs | Enforces uniqueness on display_name instead of legacy name. |
| crates/aisix-admin/src/lib.rs | Updates admin API tests to the new model payload and assertions. |
| crates/aisix-admin/src/health_handler.rs | Uses display_name for health lookup and response output. |
| crates/aisix-admin/src/etcd_store.rs | Updates etcd store tests and assertions to display_name. |
Comments suppressed due to low confidence (1)
crates/aisix-proxy/src/rerank.rs:131
- Base URL resolution logic is now duplicated here instead of reusing
crate::dispatch::resolve_base_url(...). This increases the risk of base URL normalization drifting across endpoints (e.g., trimming rules, blank handling). Prefer centralizing the behavior by requiring a provider early (like other endpoints) and calling the shared helper, keeping the Cohere fallback as an explicit/single-purpose override if needed.
let base = match pk_entry.value.api_base.as_deref() {
Some(b) if !b.trim().is_empty() => b.trim_end_matches('/').to_string(),
_ => {
// Derive a sensible default base from the provider.
model
.provider
.and_then(default_base_for_provider)
.unwrap_or_else(|| "https://api.cohere.ai".to_string())
}
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
130
| let base = crate::dispatch::resolve_base_url(Provider::Openai, &pk_entry.value); | ||
| let url = format!("{base}/v1/responses"); |
Comment on lines
+32
to
+48
| pub(crate) fn resolve_provider_key( | ||
| snapshot: &AisixSnapshot, | ||
| model: &Model, | ||
| ) -> Result<Arc<ResourceEntry<ProviderKey>>, ProxyError> { | ||
| let pk_id = model.provider_key_id.as_deref().ok_or_else(|| { | ||
| ProxyError::InvalidRequest(format!( | ||
| "model {:?} has no provider_key_id (routing models can't be dispatched directly)", | ||
| model.display_name | ||
| )) | ||
| })?; | ||
| snapshot.provider_keys.get_by_id(pk_id).ok_or_else(|| { | ||
| ProxyError::InvalidRequest(format!( | ||
| "model {:?} references unknown provider_key_id {pk_id:?}", | ||
| model.display_name | ||
| )) | ||
| }) | ||
| } |
| let model_arc = Arc::new(model.clone()); | ||
| let ctx = BridgeContext::new(request_id, model_arc); | ||
| let pk_arc = Arc::new(pk_entry.value.clone()); |
Comment on lines
+121
to
134
| // Find a model for this provider so we can borrow its provider_key. | ||
| let provider_lower = provider.to_lowercase(); | ||
| let all_models = snapshot.models.entries(); | ||
| let model_entry = all_models | ||
| .into_iter() | ||
| .find(|e| { | ||
| e.value | ||
| .model | ||
| .to_lowercase() | ||
| .starts_with(&format!("{provider_lower}/")) | ||
| .provider | ||
| .map(|p| p.as_str().eq_ignore_ascii_case(&provider_lower)) | ||
| .unwrap_or(false) | ||
| }) | ||
| .ok_or_else(|| { | ||
| ProxyError::ModelNotFound(format!("no model found for provider `{provider}`")) | ||
| })?; |
Realigns the standalone Model schema with the AISIX-Cloud control
plane's normalised shape — the projection cp-api has been waiting
for since PRD-09b §6 (the comment in mustMarshalModelKV calls this
out as "Phase 2 swaps to {model, provider_key_id} with DP-side
join, but requires Model.provider_config refactor across 26 DP
files which is a separate PR" — that's this PR).
Old shape (pre-#95 + this PR):
{ name, model: "<provider>/<id>", provider_config: { api_key, api_base } }
New shape:
{ display_name, provider, model_name, provider_key_id }
Where provider_key_id references a ProviderKey row (introduced as a
top-level resource in #95) carrying secret + api_base. Routing
models keep the same `routing` block but drop the upstream-config
triple — the router resolves a target Model and dispatches against
THAT model's provider_key_id.
Why
- One ProviderKey, many Models. Rotating the upstream secret used
to require rewriting every Model row that embedded it; now it's
a single PUT against the ProviderKey.
- AISIX-Cloud parity. cp-api already has a `ProviderKey` table;
managed-mode DPs need this shape to consume what cp-api projects
into kine.
- Snapshot-table integrity. The DP can validate at load time that
every Model.provider_key_id resolves to a ProviderKey in the same
snapshot, instead of carrying inline secrets it can't cross-check.
Changes by area
aisix-core
- Model: replaced { name, model, provider_config } with
{ display_name, provider: Option<Provider>, model_name:
Option<String>, provider_key_id: Option<String> }. Routing models
set `routing` and leave the upstream triple as None.
- Removed ProviderConfig struct entirely.
- JSON Schema: oneOf encodes the direct-vs-routing XOR
(direct ⇒ all three of provider/model_name/provider_key_id
required; routing ⇒ all three forbidden).
- Resource::name() now returns &display_name; ApiKey.allowed_models
matches against the same field (already did, just renamed).
aisix-gateway
- BridgeContext gains `provider_key: Arc<ProviderKey>`. Constructor
signature is now `new(request_id, model, provider_key)`.
aisix-provider-{openai,anthropic,gemini,deepseek}
- Bridge helpers (resolve_base / api_key / upstream_model) take
`&BridgeContext` and read from ctx.provider_key + ctx.model
rather than the now-gone provider_config.
aisix-proxy
- New `dispatch.rs` resolves both Model and ProviderKey from the
snapshot before each per-endpoint handler builds BridgeContext.
- Every endpoint (chat / completions / embeddings / messages /
responses / rerank / images / audio / passthrough) updated to
use the new resolver — no more inline `model.provider_config.api_key`.
- 422 with a clear error envelope when a Model references a
provider_key_id that isn't in the snapshot.
Tests + fixtures
- Every fixture across the workspace updated to the new JSON shape
(~30 files: aisix-admin, aisix-cache, aisix-ratelimit,
aisix-proxy, aisix-gateway, aisix-server, aisix-guardrails,
aisix-etcd).
Verified
- `cargo fmt --all --check` clean
- `cargo clippy --workspace --tests -- -D warnings` clean
- `cargo test --workspace` green (520+ tests, 0 failures)
Cross-repo follow-up
- AISIX-Cloud's `mustMarshalModelKV` (internal/cpapi/resources/handlers.go)
needs to switch from writing the inline `provider_config` shape to
the new `{display_name, provider, model_name, provider_key_id}`
shape. That's tracked separately and lands in AISIX-Cloud.The Phase B Model restructure commit landed the lib changes but the
test fixtures in crates/aisix-admin/tests/etcd_integration.rs and
tests/e2e/src/cases/smoke.test.ts still posted the old
{name, model:"openai/...", provider_config:{...}} shape. Both surfaces
fail in CI with the schema's
"Additional properties are not allowed" rejection.
- etcd_integration.rs: models_round_trip_through_real_etcd and
loader_picks_up_every_admin_write switched to {display_name,
provider, model_name, provider_key_id}
- smoke.test.ts: now posts a ProviderKey first, then references its
id from the Model — matches the production flow the dashboard
drives. Adds AdminClient.createProviderKey for the test harness.moonmingforce-pushed
the
feat/model-provider-key-ref
branch
from
May 7, 2026 06:09
7d1412d to
fa795d5Compare
Comment on lines
+79
to
+90
| /// The upstream API key — `provider_key.secret`. Empty string is | ||
| /// treated as a config error (ProviderKey rows shouldn't be empty, | ||
| /// but a hand-edited kine row could surface one). | ||
| pub(crate) fn require_secret<'a>( | ||
| provider_key: &'a ProviderKey, | ||
| model: &Model, | ||
| ) -> Result<&'a str, ProxyError> { | ||
| if provider_key.secret.is_empty() { | ||
| return Err(ProxyError::InvalidRequest(format!( | ||
| "model {:?} provider_key has empty secret", | ||
| model.display_name | ||
| ))); |
Comment on lines
+129
to
130
| let base = crate::dispatch::resolve_base_url(Provider::Openai, &pk_entry.value); | ||
| let url = format!("{base}/v1/responses"); |
Comment on lines
+81
to
85
| fn resolve_base(ctx: &BridgeContext) -> String { | ||
| match ctx.provider_key.api_base.as_deref() { | ||
| Some(b) if !b.trim().is_empty() => b.trim_end_matches('/').to_string(), | ||
| _ => OPENAI_DEFAULT_BASE.to_string(), | ||
| } |
Comment on lines
+550
to
+552
| Err(_) => { | ||
| last_err = Some(BridgeError::Config( | ||
| "model references unknown provider_key_id".into(), |
PR #100 (cross-provider /v1/messages — Anthropic protocol over non-Anthropic upstreams) landed on main with the pre-Phase-B Model API: model.provider() (method call), gemini_model(name, api_base) helpers, etc. After rebasing Phase B on top of #100, the Anthropic matrix tests + cross_provider_dispatch all stop compiling. This commit ports the survivors: - cross_provider_dispatch: switched to model.provider field access, picks up provider_key via dispatch::resolve_provider_key, threads it through BridgeContext::new(req_id, model, pk). - gemini_model / deepseek_model / anthropic_model_entry test helpers drop their api_base parameter — Phase B moves api_base onto ProviderKey, and the matrix harness now builds a fresh PK with the wiremock URI on every test. - Three test sites that still passed an extra api_base argument updated to the single-arg helper signature.
The supervisor's `apply_put`, `apply_delete`, and `clone_snapshot` helpers only handled `models` + `api_keys` — Phase B's ProviderKey and #97's Guardrail / CachePolicy / ObservabilityExporter were silently no-ops. Admin writes for those four resources landed in etcd fine, but the watch event got dropped and the proxy snapshot never updated, so dispatch saw a Model whose `provider_key_id` pointed at thin air. Smoke test #102 hit this: chat returned 500: bridge is misconfigured: model references unknown provider_key_id Fix is mechanical: extend the for-loops in apply_put + clone_snapshot and the match arms in apply_delete to cover every ResourceTable. Add `apply_put_propagates_every_resource_kind` + the matching delete test as forcing functions — any future resource type added to AisixSnapshot fails this test until the supervisor is updated. Verified - cargo fmt --all --check clean - cargo clippy --workspace --tests -- -D warnings clean - cargo test --workspace — 548 passed, 0 failed (was 546 + 2 new)
The smoke test's `chat completion forwards to mock upstream` case intermittently fails on CI with `unknown provider_key_id` even though `a Model + ApiKey written via Admin API are visible to /v1/models` passes immediately before. The fixed-time `waitConfigPropagation()` times out in 500ms; on slower CI runners only the Model row makes it into the snapshot inside that window, while the ProviderKey row the Model references arrives a beat later — long enough for the chat call to look up `provider_key_id` and miss. waitConfigPropagation now accepts an optional `condition` callback that polls a positive readiness probe on a 50ms cadence with a 5s deadline. The smoke test uses two such probes: - After the Admin writes, poll /v1/models for the Model id (covers the Model row's propagation as before). - Before the chat assertion, poll the chat path itself, retrying as long as the response carries the `unknown provider_key_id` config error. That's the only signal that captures the *complete* snapshot state (Model + ProviderKey + ApiKey), since the proxy doesn't expose ProviderKey directly. The upstream-was-hit assertion still passes because both probe and the real call land on `/v1/chat/completions`. Local repro stays green; CI now has 5s of headroom for the second- event race instead of the old 0ms past the fixed sleep.
Comment on lines
+32
to
+47
| pub(crate) fn resolve_provider_key( | ||
| snapshot: &AisixSnapshot, | ||
| model: &Model, | ||
| ) -> Result<Arc<ResourceEntry<ProviderKey>>, ProxyError> { | ||
| let pk_id = model.provider_key_id.as_deref().ok_or_else(|| { | ||
| ProxyError::InvalidRequest(format!( | ||
| "model {:?} has no provider_key_id (routing models can't be dispatched directly)", | ||
| model.display_name | ||
| )) | ||
| })?; | ||
| snapshot.provider_keys.get_by_id(pk_id).ok_or_else(|| { | ||
| ProxyError::InvalidRequest(format!( | ||
| "model {:?} references unknown provider_key_id {pk_id:?}", | ||
| model.display_name | ||
| )) | ||
| }) |
| let provider = model.provider().ok_or_else(|| { | ||
| let provider = model.provider.ok_or_else(|| { | ||
| ProxyError::InvalidRequest(format!("model `{model_name}` has no provider prefix")) |
Comment on lines
+129
to
131
| let base = crate::dispatch::resolve_base_url(Provider::Openai, &pk_entry.value); | ||
| let url = format!("{base}/v1/responses"); | ||
Comment on lines
+292
to
293
| let base = crate::dispatch::resolve_base_url(provider, &pk_entry.value); | ||
| let url = format!("{base}{upstream_path}"); |
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Realigns the standalone Model schema with the AISIX-Cloud control plane's normalised shape — the projection cp-api has been waiting for since PRD-09b §6.
cp-api's
mustMarshalModelKVliterally has this comment:This is that PR.
Schema change
Old (pre-#95 + this PR):
{ "name": "...", "model": "<provider>/<id>", "provider_config": { "api_key": "...", "api_base": "..." } }New:
{ "display_name": "...", "provider": "openai|anthropic|gemini|deepseek", "model_name": "...", "provider_key_id": "<uuid>" }provider_key_idreferences aProviderKeyrow (top-level resource introduced in #95) carryingsecret+api_base. Routing models keep the sameroutingblock but drop the upstream triple — the router resolves a target Model and dispatches against THAT model'sprovider_key_id.JSON Schema enforces the direct-vs-routing XOR via
oneOf:provider/model_name/provider_key_idrequiredroutingrequiredWhy
provider_keystable; managed-mode DPs need this shape to consume what cp-api projects into kine.Model.provider_key_idresolves to aProviderKeyin the same snapshot, instead of carrying inline secrets it can't cross-check.Changes by area
aisix-core
Model: new{display_name, provider: Option<Provider>, model_name: Option<String>, provider_key_id: Option<String>}. RemovedProviderConfigstruct.oneOffor direct/routing XOR.Resource::name()returns&display_name.aisix-gateway
BridgeContextgainsprovider_key: Arc<ProviderKey>. Constructor sig:new(request_id, model, provider_key).aisix-provider-{openai,anthropic,gemini,deepseek}
resolve_base/api_key/upstream_model) take&BridgeContextand read fromctx.provider_key+ctx.model.aisix-proxy
dispatch.rsresolves both Model and ProviderKey from the snapshot before each per-endpoint handler buildsBridgeContext.Model.provider_key_iddoesn't resolve.Tests + fixtures — ~30 files across the workspace updated to the new JSON shape.
Test plan
cargo fmt --all --checkcleancargo clippy --workspace --tests -- -D warningscleancargo test --workspace— 520+ tests green, 0 failuresaisix-etcd::loader::build_snapshotround-trip)Cross-repo follow-up
AISIX-Cloud's
mustMarshalModelKV(internal/cpapi/resources/handlers.go) needs to switch from writing the inlineprovider_configshape to the new shape. Will track separately.Summary by CodeRabbit
New Features
Bug Fixes
Refactor