Uh oh!
There was an error while loading. Please reload this page.
fix(admin): drop standalone max_budget_usd contract - #259
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR removes ChangesStandalone API budget enforcement model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
This PR removes max_budget_usd from the standalone admin API surface (write + read), while preserving the internal aisix_core::ApiKey shape so managed/control-plane budget plumbing remains decoupled from the standalone contract.
Changes:
- Reject
max_budget_usdin standalone admin ApiKey POST/PUT, and stop returning it from list/get/rotate responses via newPublicApiKey{,Entry}response shapes. - Update standalone admin OpenAPI schema to use
PublicApiKey{,Entry}(excludingmax_budget_usd) and add regression tests to pin this. - Update docs/README and adjust e2e coverage to reflect that budget policy is control-plane owned.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/src/cases/apikey-budget-e2e.test.ts | Updates e2e coverage to expect standalone admin rejection of max_budget_usd. |
| README.md | Clarifies that budgets are managed-mode/control-plane owned and not locally authored in standalone. |
| docs/api-admin.md | Removes max_budget_usd from standalone admin examples and documents rejection behavior. |
| crates/aisix-admin/src/openapi.rs | Switches ApiKey request/response schema refs to PublicApiKey{,Entry} without max_budget_usd. |
| crates/aisix-admin/src/lib.rs | Adds tests asserting max_budget_usd is excluded from responses and rejected on create; validates OpenAPI schema. |
| crates/aisix-admin/src/apikeys_handlers.rs | Implements PublicApiKey{,Entry} response types and rejects max_budget_usd in payload decoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let caught: unknown; | ||
| try { | ||
| await admin.createApiKey({ | ||
| key_hash: createHash("sha256").update("sk-neg-budget").digest("hex"), | ||
| key_hash: KEY_HASH, | ||
| allowed_models: ["*"], | ||
| max_budget_usd: -1, | ||
| max_budget_usd: 500.0, | ||
| }); | ||
| } catch (e) { | ||
| caught = e; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/aisix-core/src/models/apikey.rs:36
- This PR’s description says the internal
ApiKeyshape should remain intact to avoid coupling managed/control-plane budget paths to the standalone admin cleanup, butmax_budget_usdhas been removed from the coreApiKeystruct. Besides diverging from the stated intent, removing the field (while keeping#[serde(deny_unknown_fields)]) makes deserialization reject any stored ApiKey objects that still containmax_budget_usd. If managed mode or existing etcd data can still include this field, reintroduce it in the internal model (and hide it via standalone request/response types) or otherwise ensure backward-compatible parsing/migration.
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ApiKey {
/// SHA-256 hex of the plaintext bearer. Secondary-indexed for
/// O(1) auth — the proxy hashes incoming bearers before lookup.
pub key_hash: String,
/// Whitelisted Model identifiers. cp-api stores them as model
/// UUIDs; self-hosted dev fixtures may still use names — the DP
/// does string equality and doesn't care which. An **empty
/// array** denies every model (spec §3 authz rule).
pub allowed_models: Vec<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub rate_limit: Option<RateLimit>,
/// etcd-key uuid; filled by the loader, never in the JSON payload.
#[serde(skip)]
pub(crate) runtime_id: String,
}
| .as_str() | ||
| .unwrap() | ||
| .contains("schema validation")); | ||
| .contains("unknown field")); |
| }, | ||
| "rate_limit": { "$ref": "#/$defs/rate_limit" }, | ||
| "max_budget_usd": { "type": "number", "minimum": 0 } | ||
| "rate_limit": { "$ref": "#/$defs/rate_limit" } |
| @@ -30,12 +30,6 @@ pub struct ApiKey { | |||
| #[serde(default, skip_serializing_if = "Option::is_none")] | |||
| pub rate_limit: Option<RateLimit>, | |||
| }, | ||
| "rate_limit": { "$ref": "#/$defs/rate_limit" }, | ||
| "max_budget_usd": { "type": "number", "minimum": 0 } | ||
| "rate_limit": { "$ref": "#/$defs/rate_limit" } |
Summary
max_budget_usdon standalone admin ApiKey writes and stop exposing it from the standalone admin read contractApiKeyshape intact so managed/control-plane budget paths are not coupled to this standalone admin surface cleanupSummary by CodeRabbit
Bug Fixes
Documentation
Tests