feat(admin)!: remove the Admin API resource write path - #915
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe admin listener now exposes read-only resource routes. Resource writes use ChangesRead-only admin resource surface
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EtcdClient
participant AdminAPI
participant ConfigLoader
EtcdClient->>EtcdClient: write canonical resource document
AdminAPI->>EtcdClient: list/get resource
EtcdClient-->>AdminAPI: resource document
ConfigLoader->>EtcdClient: load canonical resource documents
EtcdClient-->>ConfigLoader: configuration data
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-admin/tests/etcd_integration.rs (1)
383-489: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd the missing
a2a_agentscanonical document.The PR retains eight resource kinds, but
writescontains seven entries. It omitsa2a_agents. Add a valid A2A agent document, assertstats.accepted == 8, and assertsnap.a2a_agents.len() == 1.🤖 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 `@crates/aisix-admin/tests/etcd_integration.rs` around lines 383 - 489, Extend the writes array with a valid a2a_agents canonical document using the existing seed flow, then update the accepted-entry assertion from 7 to 8. Add a corresponding snap.a2a_agents length assertion expecting exactly one loaded agent, leaving the other resource assertions unchanged.
🧹 Nitpick comments (1)
crates/aisix-admin/src/lib.rs (1)
1131-1198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead coverage now spans only two of eight resource kinds. Deleting the write handlers also deleted each handler module's test module. The replacement read tests in
lib.rsseed onlymodelsandapi_keys. The other six kinds —provider_keys,guardrails,cache_policies,observability_exporters,mcp_servers,a2a_agents— have 405 write-refusal coverage but no test proving that GET serves a seeded entry. These handler bodies are hand-written per module, not macro-generated, so a wrong store call or a wrong response shape in one of the six would pass CI.
crates/aisix-admin/src/lib.rs#L1131-L1198: extendbuild_seedable_statewith seed helpers for the remaining six kinds, then add list and get-by-id assertions for each, mirroringlist_models_returns_seeded_entriesandget_model_serves_seeded_entry.crates/aisix-admin/src/a2a_agents_handlers.rs#L12: add a test thatlist_a2a_agentsandget_a2a_agentreturn a seededA2aAgent, or confirm the newlib.rstests cover this module.crates/aisix-admin/src/mcp_servers_handlers.rs#L9: add the same list/get coverage forMcpServer, or confirm the newlib.rstests cover this module.Note that
InMemoryStorecurrently exposes onlyput_modelandput_apikeyas#[cfg(test)]helpers, so seeding the other six kinds requires adding matching helpers incrates/aisix-admin/src/store.rs.🤖 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 `@crates/aisix-admin/src/lib.rs` around lines 1131 - 1198, Extend crates/aisix-admin/src/store.rs with cfg(test) put helpers for provider_keys, guardrails, cache_policies, observability_exporters, mcp_servers, and a2a_agents, then update build_seedable_state and the tests in crates/aisix-admin/src/lib.rs#L1131-L1198 to seed each kind and assert both list and get-by-id responses, mirroring the model tests. Ensure crates/aisix-admin/src/a2a_agents_handlers.rs#L12 and crates/aisix-admin/src/mcp_servers_handlers.rs#L9 are covered by these lib.rs tests; no direct handler changes are required unless coverage cannot exercise their list/get functions.
🤖 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 `@crates/aisix-admin/src/lib.rs`:
- Around line 1200-1230: Remove the stale rotation-coverage comment immediately
before openapi_apikey_schema_excludes_max_budget_usd, and delete the empty
CachePolicy CRUD and Health endpoint section comments. Preserve the
guardrail_payload function, the ObservabilityExporter CRUD comment, and all
surrounding tests and formatting.
In `@crates/aisix-admin/tests/etcd_integration.rs`:
- Around line 317-354: Extend the resource-route assertions in the integration
test to issue both PUT and DELETE requests to /admin/v1/models, asserting each
returns METHOD_NOT_ALLOWED and includes GET in the Allow header. Keep the
existing POST check and final etcd emptiness assertion so all refused write
methods verify that no data is written.
In `@crates/aisix-server/src/main.rs`:
- Around line 898-902: Update the file-source match arm in the admin_store
initialization to bind the second tuple element as Some(_) instead of
Some(path), preserving the existing condition and FileManagedStore construction.
In `@tests/e2e/src/cases/file-resource-source-e2e.test.ts`:
- Around line 179-213: Update the rejected-write assertions in the e2e test to
require the exact Allow header value "GET" instead of merely containing GET,
covering both the authenticated POST/DELETE responses and the unauthenticated
POST response. Preserve the existing status and response-body assertions.
In `@tests/e2e/src/cases/openai-sdk-compat.test.ts`:
- Around line 54-84: After seeding the API key in the test setup, add an
independent readiness poll using the seeded caller credentials against
authenticated GET /v1/models, continuing until it returns 200. Remove the
client.chat.completions.create-based propagation gate and keep that call
exclusively for the SDK chat behavior under test.
---
Outside diff comments:
In `@crates/aisix-admin/tests/etcd_integration.rs`:
- Around line 383-489: Extend the writes array with a valid a2a_agents canonical
document using the existing seed flow, then update the accepted-entry assertion
from 7 to 8. Add a corresponding snap.a2a_agents length assertion expecting
exactly one loaded agent, leaving the other resource assertions unchanged.
---
Nitpick comments:
In `@crates/aisix-admin/src/lib.rs`:
- Around line 1131-1198: Extend crates/aisix-admin/src/store.rs with cfg(test)
put helpers for provider_keys, guardrails, cache_policies,
observability_exporters, mcp_servers, and a2a_agents, then update
build_seedable_state and the tests in crates/aisix-admin/src/lib.rs#L1131-L1198
to seed each kind and assert both list and get-by-id responses, mirroring the
model tests. Ensure crates/aisix-admin/src/a2a_agents_handlers.rs#L12 and
crates/aisix-admin/src/mcp_servers_handlers.rs#L9 are covered by these lib.rs
tests; no direct handler changes are required unless coverage cannot exercise
their list/get functions.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: f3cea19a-3b55-4ad2-b83e-bc29037270c5
📒 Files selected for processing (26)
README.mdconfig.example.yamlcrates/aisix-admin/src/a2a_agents_handlers.rscrates/aisix-admin/src/apikeys_handlers.rscrates/aisix-admin/src/cache_policies_handlers.rscrates/aisix-admin/src/error.rscrates/aisix-admin/src/etcd_store.rscrates/aisix-admin/src/file_store.rscrates/aisix-admin/src/guardrails_handlers.rscrates/aisix-admin/src/lib.rscrates/aisix-admin/src/mcp_servers_handlers.rscrates/aisix-admin/src/models_handlers.rscrates/aisix-admin/src/observability_exporters_handlers.rscrates/aisix-admin/src/openapi.rscrates/aisix-admin/src/provider_keys_handlers.rscrates/aisix-admin/src/state.rscrates/aisix-admin/src/store.rscrates/aisix-admin/tests/etcd_integration.rscrates/aisix-server/src/main.rstests/e2e/src/cases/apikey-budget-e2e.test.tstests/e2e/src/cases/apikey-lifecycle-e2e.test.tstests/e2e/src/cases/config-forward-compat-e2e.test.tstests/e2e/src/cases/file-resource-source-e2e.test.tstests/e2e/src/cases/openai-sdk-compat.test.tstests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.tstests/e2e/src/harness/admin.ts
💤 Files with no reviewable changes (8)
- tests/e2e/src/cases/apikey-budget-e2e.test.ts
- tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.ts
- crates/aisix-admin/src/state.rs
- crates/aisix-admin/src/observability_exporters_handlers.rs
- crates/aisix-admin/src/cache_policies_handlers.rs
- crates/aisix-admin/src/models_handlers.rs
- crates/aisix-admin/src/guardrails_handlers.rs
- crates/aisix-admin/src/provider_keys_handlers.rs
Comment on lines
+179
to
+213
| expect(postRes.status).toBe(405); | ||
| expect(postRes.headers.get("allow")).toContain("GET"); | ||
| await postRes.text(); | ||
|
|
||
| // The refused write did not change the resource set. | ||
| const relist = await fetch(`${app.adminUrl}/admin/v1/models`, { headers: auth }); | ||
| expect(((await relist.json()) as unknown[]).length).toBe(2); | ||
|
|
||
| // DELETE and rotate are covered by the same guard. | ||
| const delRes = await fetch(`${app.adminUrl}/admin/v1/models/any-id`, { | ||
| method: "DELETE", | ||
| headers: auth, | ||
| }); | ||
| expect(delRes.status).toBe(409); | ||
| expect(delRes.status).toBe(405); | ||
| expect(delRes.headers.get("allow")).toContain("GET"); | ||
| await delRes.text(); | ||
|
|
||
| // The rotate route was removed outright (it was POST-only), so the | ||
| // path no longer exists at all. | ||
| const rotateRes = await fetch(`${app.adminUrl}/admin/v1/api_keys/any-id/rotate`, { | ||
| method: "POST", | ||
| headers: auth, | ||
| }); | ||
| expect(rotateRes.status).toBe(409); | ||
| expect(rotateRes.status).toBe(404); | ||
| await rotateRes.text(); | ||
|
|
||
| // Auth ordering: an UNAUTHENTICATED write still gets 401, and the | ||
| // 401 body must not leak the resources-file path (that detail is | ||
| // only for authenticated admins). | ||
| // Method routing answers before auth: an unauthenticated write gets | ||
| // the same 405 — there is no write endpoint left to protect, and | ||
| // the 405 body carries no resources-file detail to leak. | ||
| const unauthed = await fetch(`${app.adminUrl}/admin/v1/models`, { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ display_name: "nope" }), | ||
| }); | ||
| expect(unauthed.status).toBe(401); | ||
| const unauthedBody = (await unauthed.json()) as { error_msg: string }; | ||
| expect(unauthedBody.error_msg).not.toContain(app.resourcesPath!); | ||
| expect(unauthed.status).toBe(405); | ||
| expect((await unauthed.text())).not.toContain(app.resourcesPath!); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact Allow header value.
Use toBe("GET") for each rejected write. Add the same assertion for the unauthenticated request. toContain("GET") also accepts an invalid value such as GET, POST.
Based on PR objectives, rejected resource writes must return 405 with Allow: GET.
Proposed test update
- expect(postRes.headers.get("allow")).toContain("GET");
+ expect(postRes.headers.get("allow")).toBe("GET");
...
- expect(delRes.headers.get("allow")).toContain("GET");
+ expect(delRes.headers.get("allow")).toBe("GET");
...
expect(unauthed.status).toBe(405);
+ expect(unauthed.headers.get("allow")).toBe("GET");📝 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.
Suggested change
| expect(postRes.status).toBe(405); | |
| expect(postRes.headers.get("allow")).toContain("GET"); | |
| await postRes.text(); | |
| // The refused write did not change the resource set. | |
| const relist = await fetch(`${app.adminUrl}/admin/v1/models`, { headers: auth }); | |
| expect(((await relist.json()) as unknown[]).length).toBe(2); | |
| // DELETE and rotate are covered by the same guard. | |
| const delRes = await fetch(`${app.adminUrl}/admin/v1/models/any-id`, { | |
| method: "DELETE", | |
| headers: auth, | |
| }); | |
| expect(delRes.status).toBe(409); | |
| expect(delRes.status).toBe(405); | |
| expect(delRes.headers.get("allow")).toContain("GET"); | |
| await delRes.text(); | |
| // The rotate route was removed outright (it was POST-only), so the | |
| // path no longer exists at all. | |
| const rotateRes = await fetch(`${app.adminUrl}/admin/v1/api_keys/any-id/rotate`, { | |
| method: "POST", | |
| headers: auth, | |
| }); | |
| expect(rotateRes.status).toBe(409); | |
| expect(rotateRes.status).toBe(404); | |
| await rotateRes.text(); | |
| // Auth ordering: an UNAUTHENTICATED write still gets 401, and the | |
| // 401 body must not leak the resources-file path (that detail is | |
| // only for authenticated admins). | |
| // Method routing answers before auth: an unauthenticated write gets | |
| // the same 405 — there is no write endpoint left to protect, and | |
| // the 405 body carries no resources-file detail to leak. | |
| const unauthed = await fetch(`${app.adminUrl}/admin/v1/models`, { | |
| method: "POST", | |
| headers: { "content-type": "application/json" }, | |
| body: JSON.stringify({ display_name: "nope" }), | |
| }); | |
| expect(unauthed.status).toBe(401); | |
| const unauthedBody = (await unauthed.json()) as { error_msg: string }; | |
| expect(unauthedBody.error_msg).not.toContain(app.resourcesPath!); | |
| expect(unauthed.status).toBe(405); | |
| expect((await unauthed.text())).not.toContain(app.resourcesPath!); | |
| expect(postRes.status).toBe(405); | |
| expect(postRes.headers.get("allow")).toBe("GET"); | |
| await postRes.text(); | |
| // The refused write did not change the resource set. | |
| const relist = await fetch(`${app.adminUrl}/admin/v1/models`, { headers: auth }); | |
| expect(((await relist.json()) as unknown[]).length).toBe(2); | |
| const delRes = await fetch(`${app.adminUrl}/admin/v1/models/any-id`, { | |
| method: "DELETE", | |
| headers: auth, | |
| }); | |
| expect(delRes.status).toBe(405); | |
| expect(delRes.headers.get("allow")).toBe("GET"); | |
| await delRes.text(); | |
| // The rotate route was removed outright (it was POST-only), so the | |
| // path no longer exists at all. | |
| const rotateRes = await fetch(`${app.adminUrl}/admin/v1/api_keys/any-id/rotate`, { | |
| method: "POST", | |
| headers: auth, | |
| }); | |
| expect(rotateRes.status).toBe(404); | |
| await rotateRes.text(); | |
| // Method routing answers before auth: an unauthenticated write gets | |
| // the same 405 — there is no write endpoint left to protect, and | |
| // the 405 body carries no resources-file detail to leak. | |
| const unauthed = await fetch(`${app.adminUrl}/admin/v1/models`, { | |
| method: "POST", | |
| headers: { "content-type": "application/json" }, | |
| body: JSON.stringify({ display_name: "nope" }), | |
| }); | |
| expect(unauthed.status).toBe(405); | |
| expect(unauthed.headers.get("allow")).toBe("GET"); | |
| expect((await unauthed.text())).not.toContain(app.resourcesPath!); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@tests/e2e/src/cases/file-resource-source-e2e.test.ts` around lines 179 - 213,
Update the rejected-write assertions in the e2e test to require the exact Allow
header value "GET" instead of merely containing GET, covering both the
authenticated POST/DELETE responses and the unauthenticated POST response.
Preserve the existing status and response-body assertions.
There was a problem hiding this comment.
Pull request overview
Removes Admin API resource writes, leaving read-only resource endpoints and moving management to declarative file or etcd paths.
Changes:
- Removes write handlers, routes, store operations, rotation, and deprecation middleware.
- Updates OpenAPI, documentation, and tests for the read-only contract.
- Migrates test setup to direct etcd seeding.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents the read-only Admin API. |
config.example.yaml |
Clarifies declarative resource management. |
crates/aisix-server/src/main.rs |
Wires read-only admin stores. |
crates/aisix-admin/src/lib.rs |
Removes write routes and middleware. |
crates/aisix-admin/src/openapi.rs |
Removes write operations and schemas. |
crates/aisix-admin/src/store.rs |
Makes ConfigStore read-only. |
crates/aisix-admin/src/state.rs |
Removes file-write guard state. |
crates/aisix-admin/src/error.rs |
Removes write-related errors. |
crates/aisix-admin/src/file_store.rs |
Retains snapshot reads only. |
crates/aisix-admin/src/etcd_store.rs |
Retains etcd reads only. |
crates/aisix-admin/src/models_handlers.rs |
Removes model writes. |
crates/aisix-admin/src/apikeys_handlers.rs |
Removes API-key writes and rotation. |
crates/aisix-admin/src/provider_keys_handlers.rs |
Removes provider-key writes. |
crates/aisix-admin/src/guardrails_handlers.rs |
Removes guardrail writes. |
crates/aisix-admin/src/cache_policies_handlers.rs |
Removes cache-policy writes. |
crates/aisix-admin/src/observability_exporters_handlers.rs |
Removes exporter writes. |
crates/aisix-admin/src/mcp_servers_handlers.rs |
Removes MCP-server writes. |
crates/aisix-admin/src/a2a_agents_handlers.rs |
Removes A2A-agent writes. |
crates/aisix-admin/tests/etcd_integration.rs |
Tests direct-etcd writes and admin reads. |
tests/e2e/src/harness/admin.ts |
Removes Admin API write helpers. |
tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.ts |
Removes obsolete path comparison tests. |
tests/e2e/src/cases/openai-sdk-compat.test.ts |
Migrates setup to etcd seeding. |
tests/e2e/src/cases/file-resource-source-e2e.test.ts |
Tests file-mode read-only behavior. |
tests/e2e/src/cases/config-forward-compat-e2e.test.ts |
Removes Admin write validation coverage. |
tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts |
Replaces rotation with declarative secret swapping. |
tests/e2e/src/cases/apikey-budget-e2e.test.ts |
Removes obsolete write-path validation test. |
Suppressed comments (1)
crates/aisix-admin/src/mcp_servers_handlers.rs:9
- This removal also eliminates the only production call to
aisix_mcp::validate_spec. The file and etcd loaders only run the core JSON Schema, which does not reject specs with zero generatable operations or colliding sanitized tool names, so the newly exclusive declarative paths accept configurations the former write API rejected. Add the semantic validation to both loaders before removing this path.
use aisix_core::McpServer;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::Value; | ||
| use uuid::Uuid; | ||
| use serde::Serialize; |
Comment on lines
9
to
10
| //! replaces the `key` field with a freshly-generated `sk-*` value and | ||
| //! bumps the revision, invalidating the old credential. |
Comment on lines
8
to
9
| //! | ||
| //! ids are UUID v4s generated on POST; PUT preserves the existing id. |
| @@ -38,7 +38,7 @@ const OPENAPI_JSON_BASE: &str = r##"{ | |||
| "info": { | |||
| "title": "AISIX Admin API", | |||
| "version": "dev", | |||
| "description": "The AISIX Admin API configures an open-source AISIX gateway at runtime. Use it when you operate the gateway directly and need to create or update models, caller API keys, provider credentials, guardrails, cache policies, and observability exporters.\n\nThe write endpoints (POST, PUT, DELETE) are deprecated in favor of declarative configuration: load resources from a `resources_file` (`resources.yaml`) or write them to etcd directly. Write endpoints remain functional, and every mutating response carries a `Deprecation` header (RFC 9745) plus a `Link` header with `rel=\"deprecation\"` pointing at the migration documentation. Read endpoints are not deprecated.\n\nGateways connected to AISIX Cloud do not expose this listener. Configure them through AISIX Cloud." | |||
| "description": "The AISIX Admin API is the read-only operational surface of an open-source AISIX gateway: list and inspect the loaded models, caller API keys, provider credentials, guardrails, MCP servers, A2A agents, cache policies, and observability exporters, check per-model upstream health, and drive the playground.\n\nResource write endpoints were removed in favor of declarative configuration: declare resources in a `resources_file` (`resources.yaml`) and reload with SIGHUP, or write them to etcd directly. See the resources file reference at https://docs.api7.ai/ai-gateway/reference/resources-file.\n\nGateways connected to AISIX Cloud do not expose this listener. Configure them through AISIX Cloud." | |||
Comment on lines
4
to
5
| //! validate against the JSON schema, reject duplicate names (409), | ||
| //! generate a uuid v4 on POST, bump revision on PUT. |
Comment on lines
4
to
5
| //! validate against the JSON schema, reject duplicate names (409), | ||
| //! generate a uuid v4 on POST, bump revision on PUT. |
Comment on lines
5
to
6
| //! PUT. Additionally rejects a name containing the reserved tool-namespace | ||
| //! separator `__`, since the name prefixes the server's tools. |
Comment on lines
8
to
9
| //! every configuration path rejects an incomplete credential set; the checks | ||
| //! below are defense in depth. |
Collaborator
Author
|
Review triage — every inline comment dispositioned; fixes landed in f9c10cc and e34694b. Fixed
Not adopted, with reasons
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts (1)
276-301: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse an independent readiness check for key setup.
seedKeygates propagation with aPOST /v1/chat/completionsrequest. The rotation and deletion flows also use the chat authorization path for their assertions. A chat failure can stop the test before it checks the key transition.Seed the caller keys, then verify readiness with
GET /v1/modelsand require200. Keep chat requests for the actual rotation and revocation assertions.As per coding guidelines, E2E readiness gates must use an independent condition, and caller API keys must be checked with
GET /v1/modelsreturning200.Also applies to: 311-312
🤖 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 `@tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts` around lines 276 - 301, Update the key setup readiness flow around seedKey and the related rotation/deletion cases to use an independent GET /v1/models request requiring HTTP 200, rather than POST /v1/chat/completions. Keep chat requests in the secret-swap and revocation assertions only, so readiness failures cannot mask key-transition checks.Source: Coding guidelines
🤖 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 `@crates/aisix-core/src/bin/dump-schema.rs`:
- Around line 44-47: Update the comment near the STRICT shape documentation to
qualify unknown-field rejection as applying only where the resource schema is
closed. Preserve the existing distinction between declarative write contracts
and lenient etcd reads, while acknowledging resource-specific exceptions such as
open fields and custom guardrail validation documented in the schema guidance.
In `@tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts`:
- Around line 304-307: Update the combined setup guard in the affected E2E test
to also check that otlp is available, skipping and returning when any shared
setup value—including etcdReachable, app, seed, or otlp—is missing.
---
Outside diff comments:
In `@tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts`:
- Around line 276-301: Update the key setup readiness flow around seedKey and
the related rotation/deletion cases to use an independent GET /v1/models request
requiring HTTP 200, rather than POST /v1/chat/completions. Keep chat requests in
the secret-swap and revocation assertions only, so readiness failures cannot
mask key-transition checks.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: aa61ea23-b8c7-4503-b1c7-e50f6378ee7d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
README.mdconfig.example.yamlcrates/aisix-admin/Cargo.tomlcrates/aisix-admin/src/apikeys_handlers.rscrates/aisix-admin/src/error.rscrates/aisix-admin/src/lib.rscrates/aisix-admin/src/openapi.rscrates/aisix-admin/tests/etcd_integration.rscrates/aisix-core/src/bin/dump-schema.rscrates/aisix-etcd/src/provider.rsschemas/README.mdtests/e2e/src/cases/apikey-lifecycle-e2e.test.ts
💤 Files with no reviewable changes (2)
- crates/aisix-admin/Cargo.toml
- crates/aisix-admin/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- config.example.yaml
- README.md
- crates/aisix-admin/src/apikeys_handlers.rs
- crates/aisix-admin/src/lib.rs
Comment on lines
+304
to
307
| if (!etcdReachable || !app || !seed) { | ||
| ctx.skip(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include otlp in the setup guard.
The guard omits otlp. Add it to the combined setup check so the test does not run with incomplete shared setup.
Based on learnings, E2E cases must preserve if (!etcdReachable || !app || !seed || !otlp) { ctx.skip(); return; }.
Proposed fix
- if (!etcdReachable || !app || !seed) {
+ if (!etcdReachable || !app || !seed || !otlp) {📝 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.
Suggested change
| if (!etcdReachable || !app || !seed) { | |
| ctx.skip(); | |
| return; | |
| } | |
| if (!etcdReachable || !app || !seed || !otlp) { | |
| ctx.skip(); | |
| return; | |
| } |
🤖 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 `@tests/e2e/src/cases/apikey-lifecycle-e2e.test.ts` around lines 304 - 307,
Update the combined setup guard in the affected E2E test to also check that otlp
is available, skipping and returning when any shared setup value—including
etcdReachable, app, seed, or otlp—is missing.
Source: Learnings
The admin listener keeps its read surface (lists/gets for all 8 resource kinds incl. the former apikeys spelling, models/status, health, OpenAPI + Scalar, playground, livez/readyz); resources are managed exclusively through the declarative paths — resources_file (SIGHUP reload) or direct etcd writes. BREAKING CHANGE: POST/PUT/DELETE on /admin/v1/* answer 405 with Allow: GET (409 file-managed rejection included); the api-key rotate route is gone (404) — rotate declaratively by writing the same resource id with a new key_hash. The published OpenAPI documents no write operations; write-only component schemas (ApiKeyRequest, ApiKeyEntry, ApiKeyRotateResponse, DeleteResponse) are removed. - router: every /admin/v1/* resource route serves get() only; rotate routes, file-managed write guard, and RFC 9745 deprecation-header middleware deleted; 43 write/rotate/uniqueness handler fns removed - store: ConfigStore is read-only (16 put_*/delete_* methods and StoreError::ReadOnly removed); EtcdConfigStore keeps reads only; FileManagedStore::new(snapshot) drops the path param; InMemoryStore keeps #[cfg(test)] inherent writes for unit-test seeding - openapi: 24 write ops + rotate path removed from the base document; no-op deprecation-marker pass deleted; unreachable component schemas pruned; new gate pins GET-only /admin/v1/* and zero deprecated marks - tests: write-path suites deleted; read tests seed via InMemoryStore; new 405/Allow + rotate-404 contract tests; etcd integration tests seed via direct etcd writes (the declarative front door) and pin that refused writes never touch etcd - e2e: AdminClient write helpers removed (SeedClient is the write front door); file-resource-source pins the new read-only contract; apikey-lifecycle rotate coverage became a declarative secret-swap test; obsolete write-path cases deleted - docs: README, config.example.yaml, crate module docs updated
Second-auditor findings on the removal PR, all test/doc-level (no
runtime changes):
- rotate-404 tests now GET the rotate URIs too — POST-only 404 could
not distinguish a deleted route from the old handler's unknown-id
404; GET answers 405 on a surviving POST-only route
- e2e: deleting a key's etcd entry revokes an in-use bearer
(fail-closed, unknown-token 401, other keys unaffected) — the
deletion branch had lost its only end-to-end proof
- etcd integration: a2a_agents round-trip + loader coverage (7 -> 8
kinds)
- apikeys read test pins the full PublicApiKey projection
(allowed_tools/disabled/expires_at), not just the id
- OpenAPI descriptions stop claiming ids are generated by the Admin
API and revisions increment on update/rotate
- stale write-path narrative removed: config.example.yaml (dead
docs/api-admin.md link), schemas/README, dump-schema comment,
apikeys_handlers module doc, aisix-etcd provider doc, README RBAC
row + e2e counts
- dead code: AdminError::{BadRequest,Conflict,Schema} variants and
the aisix-mcp/uuid dependencies left over from the write path
Per-comment review triage: - 7 handler module docs rewritten from CRUD-era text to the surviving read-only contract; stale rotate/CRUD section comments in the aisix-admin test module removed - OpenAPI 'Caller API Keys' tag no longer advertises key rotation - main.rs: unused match binding -> Some(_); canonical product name; dropped a comment referencing the removed write-rejection path - etcd integration: refused-writes test now covers PUT and DELETE (405 + Allow), not just POST - sdk-compat e2e: readiness gate switched from the SDK chat path (the behavior under test) to an independent authenticated GET /v1/models probe, per the harness gate rules
Cold-audit closeout:
- removed_resource_writes_answer_405_with_allow_get now generates the
FULL matrix (9 route spellings x POST/PUT/DELETE) instead of a
sampled subset — a partial revert (e.g. PUT re-added on one {id}
route) previously passed the whole suite
- last stale write-path narrative: store.rs module doc (read-only
trait), aisix-core schema.rs/models docs (declarative writers, not
'Admin API ... 400'), e2e smoke/seed/app/forward-compat headers no
longer cite deleted characterization or held-back write cases,
openapi.rs base-doc comment names a surviving schema
Round-two auditor findings: - OpenAPI: the two remaining Entry revision descriptions (McpServer, A2aAgent) stop describing create/update lifecycle; a regression assertion now rejects write-lifecycle prose on any documented revision field - schemas/README + schema.rs + dump-schema: scope the strict-contract claim to the in-repo writers (aisix validate, file source) — the control plane validates its own API schema, a raw direct etcd put gets no synchronous validation (lenient read only), and unknown-field rejection applies only where a resource closes fields; the previous rewrite overclaimed all three - deletion-revocation e2e: propagation barrier is now a fresh key seeded after the delete (later etcd revision), so the revocation assertion is a real assertion instead of a gate poll; a regression fails the assert, not a 30s timeout - apikeys projection test seeds rate_limit + allowed_agents and pins the list entry's projection too; module doc describes PublicApiKey as an explicit allowlist (not 'minus nothing') - lifecycle prose: secret-swap invalidates 'as soon as the write propagates' (not 'immediately'); README e2e counts scoped to scenario files (183/496)
moonming
force-pushed
the
feat/remove-admin-write-path
branch
from
August 11, 2026 01:59
74b0db5 to
f43eab2
Compare
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 free
to 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.
Removes the Admin API resource write path. The admin listener (
:3001) keeps its read surface — lists/gets for all 8 resource kinds (including the formerapikeysspelling),/admin/v1/models/status,/admin/v1/health, OpenAPI + Scalar UI, playground,/livez,/readyz— but resources are now managed exclusively through the declarative paths: aresources_file(resources.yaml, reloaded on SIGHUP) or direct etcd writes.This is the final step of the deprecation announced in v0.4.0 (RFC 9745
Deprecationheaders) and executes the removal scheduled for v0.5.0+ after #848 lifted the write-path-exclusive validations into the canonical schemas so the declarative paths enforce them.POST /admin/v1/<kind>,PUT/DELETE /admin/v1/<kind>/{id}(deprecated, functional)Allow: GETPOST /admin/v1/api_keys/{id}/rotate(andapikeysspelling)409naming the resources filekey_hash— the old plaintext stops authenticating as soon as the write propagates (pinned inapikey-lifecycle-e2e)What to update:
:3001→ writeresources.yaml(validate offline withaisix validate --resources <file>, reload with SIGHUP) or write entity-value JSON to etcd at{prefix}/{kind}/{id}./rotate→ hash the new secret client-side and update the resource'skey_hash./admin/openapi.json) no longer documents write operations; the write-only component schemas (ApiKeyRequest,ApiKeyEntry,ApiKeyRotateResponse,DeleteResponse) are gone fromcomponents.schemas.What changed
Router / handlers — every
/admin/v1/*resource route servesget(...)only; both rotate routes deleted; the file-managed write guard and the RFC 9745 deprecation-header middleware deleted (nothing left to mark). 43 write/rotate/uniqueness handler fns removed across the 8 handler modules.Store layer —
ConfigStoreis now a read-only trait (16put_*/delete_*methods removed,StoreError::ReadOnlygone).EtcdConfigStorekeeps only reads (module doc rewritten: resources reach etcd through the declarative paths).FileManagedStore::new(snapshot)drops the path parameter — read-only by construction.InMemoryStorekeeps#[cfg(test)]inherent write methods used by unit-test seeding.OpenAPI — 24 write operations and the rotate path removed from the base document; the no-op write-deprecation marker pass deleted; unreachable component schemas pruned via a reachability walk from
paths. New gate test pins that the published reference documents zero non-GET operations under/admin/v1/and zerodeprecatedmarks anywhere.Tests — write-path unit tests (CRUD flows, rotate atomicity, write-auth, write-validation) deleted; read tests re-seeded through
InMemoryStore; new contract tests pin 405 +Allow: GETon every collection/:idroute (auth'd and unauthenticated) and 404 on both rotate spellings. The etcd integration test now seeds via directetcd_clientputs — the path operators actually use.e2e —
AdminClientwrite helpers removed (reads stay;SeedClientis the write front door).file-resource-source-e2epins the new file-mode contract (reads serve the file, writes 405, rotate 404, unauthenticated write 405 with no file-path leak).apikey-lifecycle-e2e's rotate coverage became a declarative secret-swap test (old plaintext dies immediately, id unchanged). Deleted:seed-vs-admin-characterization-e2e(its own comment scheduled retirement once the seed migration completed),apikey-budget-e2e(tested write-path 400s), the sdk-compat deprecation-header test, and forward-compat's admin strict-write test.Docs — README,
config.example.yaml, crate module docs updated to the read-only story.Non-goals / follow-ups (filed internally)
validate_spec, HeaderName typing, duplicate tool names) still lack declarative-path enforcement — blocked on a dependency-direction extraction, tracked internally.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace— all green.resources.yaml, GET list serves the file,POST /admin/v1/models→ 405 +Allow: GET, rotate → 404,/admin/openapi.jsondocuments no admin writes.Summary by CodeRabbit
405responses, while rotation routes return404.resources_filereloads or direct etcd writes.