feat(mcp): McpServer admin resource (registration + CRUD) - #664
Conversation
Adds `McpServer` as a first-class Admin resource, parallel to ProviderKey: an upstream MCP server registration (name, url, Streamable HTTP transport, gateway-held auth, timeout, enabled) that the MCP gateway endpoint will source upstreams from. - aisix-core: `McpServer` struct + `McpTransport`/`McpAuthType` enums + Resource impl (`kind = "mcp_servers"`); schema validator wired through the same `struct_root_schema` producer as the other resources (published == enforced), registered in dump-schema; new `mcp_servers` snapshot table. - aisix-etcd: loader decode arm + supervisor merge / present / delete / clone dispatch for the new kind. - aisix-admin: `ConfigStore` get/put/list/delete + InMemoryStore + EtcdConfigStore (subkey `mcp_servers`); `/admin/v1/mcp_servers[/:id]` handlers (validate, dup-name 409, reject the reserved `__` separator in display_name, uuid on POST, revision bump on PUT) + routes. - tests: resource unit tests, an etcd CRUD round-trip, and the new kind added to `loader_picks_up_every_admin_write` (asserts the Admin → EtcdConfigStore → loader path agrees on the subkey constant). The committed `schemas/resources/mcp_server.schema.json` is regenerated via `cargo run -p aisix-core --bin dump-schema`. OpenAPI reference for the new routes is deferred to #663 (no functionality depends on it; the routes work, only the generated OpenAPI omits them). Refs AISIX-Cloud#894
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesMcpServer Resource End-to-End
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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 |
Audit (CLAUDE.md §8) on #664 returned APPROVE with two LOW suggestions, both folded in: - LOW: `auth_type=bearer` with no/empty `secret` was accepted and would send an empty `Authorization: Bearer ` upstream. `decode()` now rejects it (400), alongside the existing reserved-`__` check. - LOW: the `__`-rejection (net-new logic) had no coverage. Added unit tests for the decode guards: rejects `__` in display_name, rejects bearer without secret, accepts a valid server. Refs AISIX-Cloud#894
Independent audit (CLAUDE.md §8): APPROVEA fresh audit agent checked out the branch, regenerated the schema, ran build/tests/clippy, and grepped every Verified complete & correct:
Two LOW findings, both folded in (
Note (not actioned): Verdict: no HIGH/MEDIUM. fmt + |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/aisix-admin/tests/etcd_integration.rs (1)
199-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the MCP-specific payload, not just the id.
This helper only checks status, id, and list size, so this new test still passes if
url,auth_type,secret, or MCP defaults serialize incorrectly. A small response-body/GET assertion here, plus one 400/409 case for the new validation rules, would give this path real coverage.🤖 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 199 - 218, The new mcp_servers_round_trip_through_real_etcd test only validates status, id, and list size, so it can miss MCP field serialization bugs. Update this test to assert the MCP-specific response/body fields using the mcp_servers endpoint and the returned resource, verifying url, auth_type, secret handling, and any MCP defaults. Also add one negative case covering the new validation rules with a 400 or 409 response so the MCP path is actually exercised beyond the shared admin_crud_round_trip helper.
🤖 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/mcp_servers_handlers.rs`:
- Around line 69-80: update_mcp_server currently does a read-modify-write using
existing.revision + 1 and an unconditional put_mcp_server, which allows
concurrent overwrites. Update the store API used by
mcp_servers_handlers::update_mcp_server and ResourceEntry::new to perform a
compare-and-swap on the persisted revision (using the revision read from
get_mcp_server), return a conflict/error when the revision no longer matches,
and have the successful write return the committed revision back to the handler.
- Around line 54-59: The `add_mcp_server` and related write path currently
enforce `display_name` uniqueness by scanning with `list_mcp_servers` and then
calling `assert_unique_display_name` before `put_mcp_server`, which is not safe
under concurrency. Move the uniqueness check into the backing store layer as an
atomic operation, ideally via a transaction or compare-and-swap style write in
the store implementation used by `state.store.put_mcp_server`, so both insert
paths cannot race and persist the same `display_name`. Update the handler to
rely on that atomic store guarantee rather than doing pre-checks in
`mcp_servers_handlers`.
In `@crates/aisix-core/src/models/mcp_server.rs`:
- Around line 41-46: The current MCP server validation in the `McpServer` model
only documents the `auth_type`/`secret` rule, so invalid combinations can still
pass through the shared validator. Add an explicit cross-field validation in the
`McpServer`/`Validator` path that enforces `secret` is required when `auth_type`
is `bearer` and must be absent when `auth_type` is `none`, so both admin writes
and loader ingestion reject bad states before they reach the live snapshot.
- Around line 18-23: The `McpServer::display_name` schema currently enforces
only `min_length`, but the contract also forbids the reserved `__` separator.
Update the canonical validation used by `validate_mcp_server`/loader paths in
`mcp_server.rs` to reject any `display_name` containing `__`, and make sure the
schema reflects that constraint so direct etcd writes cannot persist ambiguous
tool namespaces.
---
Nitpick comments:
In `@crates/aisix-admin/tests/etcd_integration.rs`:
- Around line 199-218: The new mcp_servers_round_trip_through_real_etcd test
only validates status, id, and list size, so it can miss MCP field serialization
bugs. Update this test to assert the MCP-specific response/body fields using the
mcp_servers endpoint and the returned resource, verifying url, auth_type, secret
handling, and any MCP defaults. Also add one negative case covering the new
validation rules with a 400 or 409 response so the MCP path is actually
exercised beyond the shared admin_crud_round_trip helper.
🪄 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: CHILL
Plan: Pro
Run ID: c66a94b2-7cec-408d-9ed1-555ee0ed7f43
📒 Files selected for processing (14)
crates/aisix-admin/src/etcd_store.rscrates/aisix-admin/src/lib.rscrates/aisix-admin/src/mcp_servers_handlers.rscrates/aisix-admin/src/store.rscrates/aisix-admin/tests/etcd_integration.rscrates/aisix-core/src/bin/dump-schema.rscrates/aisix-core/src/lib.rscrates/aisix-core/src/models/mcp_server.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/schema.rscrates/aisix-core/src/models/snapshot.rscrates/aisix-etcd/src/loader.rscrates/aisix-etcd/src/supervisor.rsschemas/resources/mcp_server.schema.json
| let all = state.store.list_mcp_servers().await?; | ||
| assert_unique_display_name(&all, &mcp_server.display_name, None)?; | ||
|
|
||
| let id = Uuid::new_v4().to_string(); | ||
| let entry = ResourceEntry::new(&id, mcp_server, STARTING_REVISION); | ||
| state.store.put_mcp_server(entry.clone()).await?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce display_name uniqueness atomically.
Both write paths do list_mcp_servers → assert_unique_display_name → put_mcp_server. With only blind writes underneath, two concurrent requests can both pass the scan and persist the same display_name, which breaks the downstream <display_name>__<tool> namespace contract. This needs to move to an atomic store operation / etcd transaction instead of staying in the handler.
Also applies to: 76-80
🤖 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/mcp_servers_handlers.rs` around lines 54 - 59, The
`add_mcp_server` and related write path currently enforce `display_name`
uniqueness by scanning with `list_mcp_servers` and then calling
`assert_unique_display_name` before `put_mcp_server`, which is not safe under
concurrency. Move the uniqueness check into the backing store layer as an atomic
operation, ideally via a transaction or compare-and-swap style write in the
store implementation used by `state.store.put_mcp_server`, so both insert paths
cannot race and persist the same `display_name`. Update the handler to rely on
that atomic store guarantee rather than doing pre-checks in
`mcp_servers_handlers`.
| let existing = state | ||
| .store | ||
| .get_mcp_server(&id) | ||
| .await? | ||
| .ok_or(AdminError::NotFound)?; | ||
| let mcp_server = decode(&raw)?; | ||
|
|
||
| let all = state.store.list_mcp_servers().await?; | ||
| assert_unique_display_name(&all, &mcp_server.display_name, Some(&id))?; | ||
|
|
||
| let entry = ResourceEntry::new(&id, mcp_server, existing.revision + 1); | ||
| state.store.put_mcp_server(entry.clone()).await?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Guard PUT with optimistic concurrency.
update_mcp_server reads the current row, bumps the revision locally, and then calls an unconditional put_mcp_server. Two concurrent updates to the same id will both succeed and one silently overwrites the other. The store layer needs a compare-and-swap on the persisted revision and should return the committed revision to the handler.
🤖 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/mcp_servers_handlers.rs` around lines 69 - 80,
update_mcp_server currently does a read-modify-write using existing.revision + 1
and an unconditional put_mcp_server, which allows concurrent overwrites. Update
the store API used by mcp_servers_handlers::update_mcp_server and
ResourceEntry::new to perform a compare-and-swap on the persisted revision
(using the revision read from get_mcp_server), return a conflict/error when the
revision no longer matches, and have the successful write return the committed
revision back to the handler.
| /// Operator-facing label, unique within the gateway. It is used as the | ||
| /// namespace prefix for this server's tools, which are exposed to clients as | ||
| /// `<display_name>__<tool>`, so it must not contain the reserved separator | ||
| /// `__`. | ||
| #[schemars(length(min = 1))] | ||
| pub display_name: String, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the reserved __ separator in the canonical schema.
This field documents a hard contract, but the model only enforces minLength. Because validate_mcp_server is also what the loader uses for etcd rows, a direct write can still persist display_name values containing __, which makes the <display_name>__<tool> namespace ambiguous at runtime.
Suggested fix
- #[schemars(length(min = 1))]
+ #[schemars(regex(pattern = "^(?!.*__).+$"), length(min = 1))]
pub display_name: String,📝 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.
| /// Operator-facing label, unique within the gateway. It is used as the | |
| /// namespace prefix for this server's tools, which are exposed to clients as | |
| /// `<display_name>__<tool>`, so it must not contain the reserved separator | |
| /// `__`. | |
| #[schemars(length(min = 1))] | |
| pub display_name: String, | |
| /// Operator-facing label, unique within the gateway. It is used as the | |
| /// namespace prefix for this server's tools, which are exposed to clients as | |
| /// `<display_name>__<tool>`, so it must not contain the reserved separator | |
| /// `__`. | |
| #[schemars(regex(pattern = "^(?!.*__).+$"), length(min = 1))] | |
| pub display_name: String, |
🤖 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-core/src/models/mcp_server.rs` around lines 18 - 23, The
`McpServer::display_name` schema currently enforces only `min_length`, but the
contract also forbids the reserved `__` separator. Update the canonical
validation used by `validate_mcp_server`/loader paths in `mcp_server.rs` to
reject any `display_name` containing `__`, and make sure the schema reflects
that constraint so direct etcd writes cannot persist ambiguous tool namespaces.
| /// Authentication credential for the upstream server. Required when | ||
| /// `auth_type` is `bearer`, where it is sent as `Authorization: Bearer | ||
| /// <secret>` on every upstream request. Leave unset when `auth_type` is | ||
| /// `none`. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub secret: Option<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Encode the auth_type/secret dependency instead of only documenting it.
Right now the shared validator still accepts both {"auth_type":"bearer"} and {"auth_type":"none","secret":"..."}. Since admin writes and loader ingestion both go through that validator, those invalid combinations can be stored and propagated into the live snapshot.
🤖 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-core/src/models/mcp_server.rs` around lines 41 - 46, The current
MCP server validation in the `McpServer` model only documents the
`auth_type`/`secret` rule, so invalid combinations can still pass through the
shared validator. Add an explicit cross-field validation in the
`McpServer`/`Validator` path that enforces `secret` is required when `auth_type`
is `bearer` and must be absent when `auth_type` is `none`, so both admin writes
and loader ingestion reject bad states before they reach the live snapshot.
1 participant
What
Adds
McpServeras a first-class Admin resource (parallel toProviderKey): an upstream MCP server registration that the MCP gateway endpoint will source its upstreams from, instead of the explicit list it takes today. This is the control-plane prerequisite for wiring the gateway behind the governance pipeline.Fields (customer-facing, rendered into the Admin API reference):
display_name(unique; namespace prefix for the server's tools, so__is rejected),url(Streamable HTTP endpoint),transport(enum,streamable_http),auth_type(none/bearer) +secret(gateway-held),timeout_ms,enabled.How (mirrors the existing resource scaffolding)
McpServerstruct +McpTransport/McpAuthTypeenums +Resourceimpl (kind = "mcp_servers"); schema validator built through the samestruct_root_schemaproducer the other resources use, so the published schema == the enforced schema; registered indump-schema; newmcp_serverssnapshot table.ConfigStoreget/put/list/delete on bothInMemoryStoreandEtcdConfigStore(etcd subkeymcp_servers);/admin/v1/mcp_servers[/:id]handlers (schema-validate, dup-name 409, reject reserved__, uuid on POST, revision bump on PUT) + routes.Test plan
mcp_servers_round_trip_through_real_etcd— full Admin CRUD over a real etcd.loader_picks_up_every_admin_writeextended to seed anmcp_serversrow and assert the loader accepts it — this catches subkey-constant drift betweenEtcdConfigStoreandaisix_etcd::loader(the most likely wiring bug).cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test -p aisix-core -p aisix-etcd -p aisix-adminall green;schemas/resources/mcp_server.schema.jsonregenerated viadump-schema.Scope / explicitly deferred
generate admin openapiCI step validates structure, not a committed snapshot). The routes are fully functional; only the generated OpenAPI omits them. Called out here per the merge gate so it's a tracked gap, not a silent one.Refs AISIX-Cloud#894
Summary by CodeRabbit
New Features
Bug Fixes