Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion crates/aisix-admin/src/etcd_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
//! deterministic behaviour continue to use [`crate::InMemoryStore`].

use aisix_core::resource::ResourceEntry;
use aisix_core::{ApiKey, CachePolicy, Guardrail, Model, ObservabilityExporter, ProviderKey};
use aisix_core::{
ApiKey, CachePolicy, Guardrail, McpServer, Model, ObservabilityExporter, ProviderKey,
};
use etcd_client::{Client, DeleteOptions, GetOptions};
use serde::de::DeserializeOwned;
use serde::Serialize;
Expand All @@ -35,6 +37,7 @@ pub const PROVIDER_KEYS_SUBKEY: &str = "provider_keys";
pub const GUARDRAILS_SUBKEY: &str = "guardrails";
pub const CACHE_POLICIES_SUBKEY: &str = "cache_policies";
pub const OBSERVABILITY_EXPORTERS_SUBKEY: &str = "observability_exporters";
pub const MCP_SERVERS_SUBKEY: &str = "mcp_servers";

pub struct EtcdConfigStore {
client: Mutex<Client>,
Expand Down Expand Up @@ -333,6 +336,35 @@ impl ConfigStore for EtcdConfigStore {
self.delete_one(&self.key_for(OBSERVABILITY_EXPORTERS_SUBKEY, id))
.await
}

async fn put_mcp_server(&self, entry: ResourceEntry<McpServer>) -> Result<(), StoreError> {
let key = self.key_for(MCP_SERVERS_SUBKEY, &entry.id);
self.put_json(&key, &entry.value).await
}

async fn get_mcp_server(
&self,
id: &str,
) -> Result<Option<ResourceEntry<McpServer>>, StoreError> {
let key = self.key_for(MCP_SERVERS_SUBKEY, id);
Ok(self
.get_one::<McpServer>(&key)
.await?
.map(|(v, rev)| ResourceEntry::new(id, v, rev)))
}

async fn list_mcp_servers(&self) -> Result<Vec<ResourceEntry<McpServer>>, StoreError> {
Ok(self
.list_range::<McpServer>(MCP_SERVERS_SUBKEY)
.await?
.into_iter()
.map(|(id, v, rev)| ResourceEntry::new(id, v, rev))
.collect())
}

async fn delete_mcp_server(&self, id: &str) -> Result<bool, StoreError> {
self.delete_one(&self.key_for(MCP_SERVERS_SUBKEY, id)).await
}
}

#[cfg(test)]
Expand Down
12 changes: 12 additions & 0 deletions crates/aisix-admin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ mod error;
pub mod etcd_store;
mod guardrails_handlers;
mod health_handler;
mod mcp_servers_handlers;
mod models_handlers;
mod models_status_handler;
mod observability_exporters_handlers;
Expand Down Expand Up @@ -119,6 +120,17 @@ pub fn build_router(state: AdminState) -> Router {
.put(provider_keys_handlers::update_provider_key)
.delete(provider_keys_handlers::delete_provider_key),
)
.route(
"/admin/v1/mcp_servers",
get(mcp_servers_handlers::list_mcp_servers)
.post(mcp_servers_handlers::create_mcp_server),
)
.route(
"/admin/v1/mcp_servers/:id",
get(mcp_servers_handlers::get_mcp_server)
.put(mcp_servers_handlers::update_mcp_server)
.delete(mcp_servers_handlers::delete_mcp_server),
)
.route(
"/admin/v1/guardrails",
get(guardrails_handlers::list_guardrails)
Expand Down
163 changes: 163 additions & 0 deletions crates/aisix-admin/src/mcp_servers_handlers.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
//! CRUD handlers for `/admin/v1/mcp_servers`.
//!
//! Same shape as the ProviderKeys handlers: validate against the JSON schema,
//! reject duplicate display_names (409), generate a uuid v4 on POST, bump
//! revision on PUT. Additionally rejects a display_name containing the reserved
//! tool-namespace separator `__`, since the name prefixes the server's tools.

use aisix_core::models::validate_mcp_server;
use aisix_core::resource::ResourceEntry;
use aisix_core::{McpAuthType, McpServer};
use axum::extract::{Path, State};
use axum::Json;
use serde_json::Value;
use uuid::Uuid;

use crate::auth::AdminAuth;
use crate::error::AdminError;
use crate::state::AdminState;

const STARTING_REVISION: i64 = 1;

/// Reserved separator between a server's name and a tool name in the gateway's
/// aggregated namespace (`<display_name>__<tool>`). A server name must not
/// contain it.
const TOOL_NAMESPACE_SEPARATOR: &str = "__";

pub async fn list_mcp_servers(
_auth: AdminAuth,
State(state): State<AdminState>,
) -> Result<Json<Vec<ResourceEntry<McpServer>>>, AdminError> {
let entries = state.store.list_mcp_servers().await?;
Ok(Json(entries))
}

pub async fn get_mcp_server(
_auth: AdminAuth,
Path(id): Path<String>,
State(state): State<AdminState>,
) -> Result<Json<ResourceEntry<McpServer>>, AdminError> {
let entry = state
.store
.get_mcp_server(&id)
.await?
.ok_or(AdminError::NotFound)?;
Ok(Json(entry))
}

pub async fn create_mcp_server(
_auth: AdminAuth,
State(state): State<AdminState>,
Json(raw): Json<Value>,
) -> Result<Json<ResourceEntry<McpServer>>, AdminError> {
let mcp_server = decode(&raw)?;
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?;
Comment on lines +54 to +59

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Enforce display_name uniqueness atomically.

Both write paths do list_mcp_serversassert_unique_display_nameput_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`.

Ok(Json(entry))
}

pub async fn update_mcp_server(
_auth: AdminAuth,
Path(id): Path<String>,
State(state): State<AdminState>,
Json(raw): Json<Value>,
) -> Result<Json<ResourceEntry<McpServer>>, AdminError> {
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?;
Comment on lines +69 to +80

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Ok(Json(entry))
}

pub async fn delete_mcp_server(
_auth: AdminAuth,
Path(id): Path<String>,
State(state): State<AdminState>,
) -> Result<Json<Value>, AdminError> {
let removed = state.store.delete_mcp_server(&id).await?;
if !removed {
return Err(AdminError::NotFound);
}
Ok(Json(serde_json::json!({"deleted": true, "id": id})))
}

fn decode(raw: &Value) -> Result<McpServer, AdminError> {
validate_mcp_server(raw)?;
let server: McpServer = serde_json::from_value(raw.clone())
.map_err(|e| AdminError::BadRequest(format!("malformed McpServer payload: {e}")))?;
if server.display_name.contains(TOOL_NAMESPACE_SEPARATOR) {
return Err(AdminError::BadRequest(format!(
"display_name must not contain the reserved separator `{TOOL_NAMESPACE_SEPARATOR}`"
)));
}
if matches!(server.auth_type, McpAuthType::Bearer)
&& server.secret.as_deref().unwrap_or_default().is_empty()
{
return Err(AdminError::BadRequest(
"secret is required and must be non-empty when auth_type is `bearer`".to_string(),
));
}
Ok(server)
}

fn assert_unique_display_name(
existing: &[ResourceEntry<McpServer>],
display_name: &str,
self_id: Option<&str>,
) -> Result<(), AdminError> {
for e in existing {
if e.value.display_name == display_name && self_id.is_none_or(|sid| sid != e.id) {
return Err(AdminError::Conflict(display_name.to_string()));
}
}
Ok(())
}

#[cfg(test)]
mod tests {
use super::*;
use serde_json::json;

#[test]
fn decode_rejects_separator_in_display_name() {
let err = decode(&json!({"display_name": "a__b", "url": "https://x/mcp"}))
.expect_err("`__` in display_name must be rejected");
assert!(matches!(err, AdminError::BadRequest(_)));
}

#[test]
fn decode_rejects_bearer_without_secret() {
let err = decode(&json!({
"display_name": "gh",
"url": "https://x/mcp",
"auth_type": "bearer"
}))
.expect_err("bearer auth without a secret must be rejected");
assert!(matches!(err, AdminError::BadRequest(_)));
}

#[test]
fn decode_accepts_valid_server() {
let server = decode(&json!({
"display_name": "github",
"url": "https://api.example.com/mcp",
"auth_type": "bearer",
"secret": "tok"
}))
.expect("valid server should decode");
assert_eq!(server.display_name, "github");
assert_eq!(server.secret.as_deref(), Some("tok"));
}
}
33 changes: 32 additions & 1 deletion crates/aisix-admin/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
//! in the handler layer so the store stays dumb and fast.

use aisix_core::resource::ResourceEntry;
use aisix_core::{ApiKey, CachePolicy, Guardrail, Model, ObservabilityExporter, ProviderKey};
use aisix_core::{
ApiKey, CachePolicy, Guardrail, McpServer, Model, ObservabilityExporter, ProviderKey,
};
use dashmap::DashMap;
use std::sync::Arc;

Expand Down Expand Up @@ -65,6 +67,14 @@ pub trait ConfigStore: Send + Sync + 'static {
&self,
) -> Result<Vec<ResourceEntry<ObservabilityExporter>>, StoreError>;
async fn delete_observability_exporter(&self, id: &str) -> Result<bool, StoreError>;

async fn put_mcp_server(&self, entry: ResourceEntry<McpServer>) -> Result<(), StoreError>;
async fn get_mcp_server(
&self,
id: &str,
) -> Result<Option<ResourceEntry<McpServer>>, StoreError>;
async fn list_mcp_servers(&self) -> Result<Vec<ResourceEntry<McpServer>>, StoreError>;
async fn delete_mcp_server(&self, id: &str) -> Result<bool, StoreError>;
}

/// In-memory store. Thread-safe via DashMap; mainly used by tests, but
Expand All @@ -77,6 +87,7 @@ pub struct InMemoryStore {
guardrails: DashMap<String, ResourceEntry<Guardrail>>,
cache_policies: DashMap<String, ResourceEntry<CachePolicy>>,
observability_exporters: DashMap<String, ResourceEntry<ObservabilityExporter>>,
mcp_servers: DashMap<String, ResourceEntry<McpServer>>,
}

impl InMemoryStore {
Expand Down Expand Up @@ -209,6 +220,26 @@ impl ConfigStore for InMemoryStore {
async fn delete_observability_exporter(&self, id: &str) -> Result<bool, StoreError> {
Ok(self.observability_exporters.remove(id).is_some())
}

async fn put_mcp_server(&self, entry: ResourceEntry<McpServer>) -> Result<(), StoreError> {
self.mcp_servers.insert(entry.id.clone(), entry);
Ok(())
}

async fn get_mcp_server(
&self,
id: &str,
) -> Result<Option<ResourceEntry<McpServer>>, StoreError> {
Ok(self.mcp_servers.get(id).map(|r| r.clone()))
}

async fn list_mcp_servers(&self) -> Result<Vec<ResourceEntry<McpServer>>, StoreError> {
Ok(self.mcp_servers.iter().map(|r| r.clone()).collect())
}

async fn delete_mcp_server(&self, id: &str) -> Result<bool, StoreError> {
Ok(self.mcp_servers.remove(id).is_some())
}
}

#[cfg(test)]
Expand Down
28 changes: 27 additions & 1 deletion crates/aisix-admin/tests/etcd_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,27 @@ async fn provider_keys_round_trip_through_real_etcd() {
.await;
}

#[tokio::test]
async fn mcp_servers_round_trip_through_real_etcd() {
let Some(url) = etcd_url() else {
eprintln!("skipping: ADMIN_TEST_ETCD_URL not set");
return;
};
let prefix = unique_prefix();
let state = build_state_with_real_etcd(&url, &prefix).await;
admin_crud_round_trip(
state,
"/admin/v1/mcp_servers",
json!({
"display_name": "github-it",
"url": "https://api.example.com/mcp",
"auth_type": "bearer",
"secret": "tok-it"
}),
)
.await;
}

#[tokio::test]
async fn guardrails_round_trip_through_real_etcd() {
let Some(url) = etcd_url() else {
Expand Down Expand Up @@ -317,6 +338,10 @@ async fn loader_picks_up_every_admin_write() {
"endpoint": "https://otel.example.com/v1/traces"
}),
),
(
"/admin/v1/mcp_servers",
json!({"display_name": "loader-mcp", "url": "https://api.example.com/mcp"}),
),
];
for (uri, body) in writes {
let app = build_router(state.clone());
Expand Down Expand Up @@ -363,7 +388,7 @@ async fn loader_picks_up_every_admin_write() {
likely a subkey-constant drift between EtcdConfigStore::*_SUBKEY \
and the match arms in aisix_etcd::loader: {stats:?}"
);
assert_eq!(stats.accepted, 6, "expected 6 entries; got {stats:?}");
assert_eq!(stats.accepted, 7, "expected 7 entries; got {stats:?}");

// Each resource table should now have exactly one entry.
assert_eq!(snap.models.len(), 1);
Expand All @@ -372,4 +397,5 @@ async fn loader_picks_up_every_admin_write() {
assert_eq!(snap.guardrails.len(), 1);
assert_eq!(snap.cache_policies.len(), 1);
assert_eq!(snap.observability_exporters.len(), 1);
assert_eq!(snap.mcp_servers.len(), 1);
}
1 change: 1 addition & 0 deletions crates/aisix-core/src/bin/dump-schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ fn main() {
"guardrail_attachment",
schema::guardrail_attachment_root_schema(),
);
dump_value(&out_dir, "mcp_server", schema::mcp_server_root_schema());

dump::<EnsembleConfig>(&out_dir, "ensemble");
dump::<RateLimit>(&out_dir, "rate_limit");
Expand Down
9 changes: 5 additions & 4 deletions crates/aisix-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ pub use error::{
AdminError, AdminErrorEnvelope, BootstrapError, ProxyError, ProxyErrorEnvelope, RateLimitScope,
};
pub use models::{
validate_apikey, validate_cache_policy, validate_guardrail, validate_model,
validate_observability_exporter, validate_provider_key, validate_rate_limit_policy, Adapter,
AisixSnapshot, ApiKey, AppliedGuardrail, CachePolicy, CooldownConfig, ExporterKind, Guardrail,
GuardrailHookPoint, GuardrailKind, KeywordConfig, KeywordPattern, Model, ObservabilityExporter,
validate_apikey, validate_cache_policy, validate_guardrail, validate_mcp_server,
validate_model, validate_observability_exporter, validate_provider_key,
validate_rate_limit_policy, Adapter, AisixSnapshot, ApiKey, AppliedGuardrail, CachePolicy,
CooldownConfig, ExporterKind, Guardrail, GuardrailHookPoint, GuardrailKind, KeywordConfig,
KeywordPattern, McpAuthType, McpServer, McpTransport, Model, ObservabilityExporter,
ParamConstraints, PolicyScope, PolicyWindow, ProviderKey, RateLimit, RateLimitPolicy,
RequestOverrides, ResponseOverrides, Routing, RoutingStrategy, RoutingTarget, SchemaError,
StreamDoneMarker, TelemetryKind, TelemetryTags, WhenAllUnavailablePolicy,
Expand Down
Loading
Loading