From 66fa0b56ea29049ad81c3c7c6927ee02d445b324 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 21 Aug 2026 18:24:32 +0100 Subject: [PATCH] feat(admin): MCP/agent target visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add per-target request accounting for the MCP/A2A proxy path and a new admin endpoint to surface it. - limits.go: countTargetRequest increments one target's req counters at min/hour/day/month in a single storeIncrMulti call — pure accounting, no limits, mirroring account()'s own contract. targetUsage batches the read side the same way currentUsage does for user/group scopes. - mcp_a2a.go: handleTargetProxy attributes every admitted proxied request to its target's own scope, kind "mcp" or "agent" (scopeKindAgent — deliberately not targetKindAgent's "a2a" routing convention). kind is embedded in the counter key (windowKey), so a target can never collide with a user/group/total scope of the same id, whatever any of them are named. - admin.go: new GET /admin/api/targets, admin-gated and stats-excluded exactly like the other three /admin/api/* routes. Access lists are computed via the SAME matchesGlob call group.allowsMCP/allowsAgent themselves use, so the panel can never disagree with enforcement; a target every group can reach collapses to an omitted access list ("empty meaning all"). No new config surface: target scopes carry no configurable limits this round. Tests: TestTargetScopeKind (mapping), TestHandleTargetProxy_ PerTargetCounters_AttributedAndCollisionSafe (table-driven over mcp/a2a, proves counters land on the right scope and never collide with a same-named user), TestAdminTargets_* (gating tables extended, shape/sorting/URL-sanitization/access-list/counter assertions). --- admin.go | 204 ++++++++++++++++++++++++++++++++++++++++++------ admin_test.go | 162 ++++++++++++++++++++++++++++++++++---- limits.go | 103 ++++++++++++++++++++++++ mcp_a2a.go | 40 +++++++++- mcp_a2a_test.go | 96 +++++++++++++++++++++++ 5 files changed, 564 insertions(+), 41 deletions(-) diff --git a/admin.go b/admin.go index 4185268..dd666b3 100644 --- a/admin.go +++ b/admin.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/url" + "sort" "strconv" "strings" "time" @@ -12,21 +13,22 @@ import ( // The admin dashboard's routes (spec §4, v0.2; adminUsageHistoryPath added // by the v0.2 data-layer task; adminAssetsPathPrefix added by the Vue -// admin-panel task), matched only when adminEnabled(g.cfg) — see -// llmgateway.go's ServeHTTP dispatch. adminAssetsPathPrefix is a prefix, -// not one fixed path: every hashed filename the Vite build emits -// (admin_assets_gen.go) is served under it. +// admin-panel task; adminTargetsPath added by Feature B, v0.21), matched +// only when adminEnabled(g.cfg) — see llmgateway.go's ServeHTTP dispatch. +// adminAssetsPathPrefix is a prefix, not one fixed path: every hashed +// filename the Vite build emits (admin_assets_gen.go) is served under it. const ( adminPagePath = "/admin" adminAssetsPathPrefix = "/admin/assets/" adminOverviewPath = "/admin/api/overview" adminUsagePath = "/admin/api/usage" adminUsageHistoryPath = "/admin/api/usage/history" + adminTargetsPath = "/admin/api/targets" ) // adminCSP is the Content-Security-Policy header served with GET /admin, // every GET /admin/assets/{hashedname} response (serveAdminAsset), and -// the three /admin/api/* JSON routes — every /admin* response this +// every /admin/api/* JSON route — every /admin* response this // gateway ever sends (spec §4, v0.2; extended to the JSON routes by a // folded review item, 2026-08-20 review; extended to the asset route by // a later review sweep — see serveAdminAsset's own doc comment for why). @@ -64,9 +66,10 @@ func adminEnabled(cfg *Config) bool { // isAdminPath reports whether path is one of the admin dashboard's // routes: the page, any hashed asset under adminAssetsPathPrefix, or one -// of the three /admin/api/* JSON routes. +// of the /admin/api/* JSON routes. func isAdminPath(path string) bool { - if path == adminPagePath || path == adminOverviewPath || path == adminUsagePath || path == adminUsageHistoryPath { + if path == adminPagePath || path == adminOverviewPath || path == adminUsagePath || + path == adminUsageHistoryPath || path == adminTargetsPath { return true } return strings.HasPrefix(path, adminAssetsPathPrefix) @@ -102,23 +105,24 @@ func (g *Gateway) handleAdmin(sw *statusTrackingWriter, r *http.Request) { } } -// handleAdminAPI is the gate for the three /admin/api/* JSON routes, -// applying spec §4's gate order: unauthenticated → 401, authenticated -// non-admin → 403, admin → serve. +// handleAdminAPI is the gate for the /admin/api/* JSON routes, applying +// spec §4's gate order: unauthenticated → 401, authenticated non-admin → +// 403, admin → serve. // -// None of the three routes call checkAndCount (operator directive: -// progress ledger, 2026-08-20 — "admin requests must NOT touch req/min, -// req/day statistics"): admin traffic must never appear in usage -// statistics, which exist to measure real LLM traffic only. GET -// /admin/api/usage/history was built this way from the start (v0.2 -// data-layer task); this change extends the same treatment to GET -// /admin/api/overview and GET /admin/api/usage, which previously counted -// like any other authenticated route (spec §4 amended accordingly). An -// admin's own req/min or req/day limit, if configured, is therefore never -// enforced against admin-route traffic either — the accepted trade-off -// the operator directive names: these are admin-gated, cheap reads, and -// an admin holder polling the dashboard aggressively is a self-inflicted, -// not a shared, resource cost. +// None of these routes call checkAndCount (operator directive: progress +// ledger, 2026-08-20 — "admin requests must NOT touch req/min, req/day +// statistics"): admin traffic must never appear in usage statistics, +// which exist to measure real LLM traffic only. GET /admin/api/usage/ +// history was built this way from the start (v0.2 data-layer task); this +// change extends the same treatment to GET /admin/api/overview and GET +// /admin/api/usage, which previously counted like any other authenticated +// route (spec §4 amended accordingly); GET /admin/api/targets (Feature B, +// v0.21) is built the same way from the start too. An admin's own req/min +// or req/day limit, if configured, is therefore never enforced against +// admin-route traffic either — the accepted trade-off the operator +// directive names: these are admin-gated, cheap reads, and an admin +// holder polling the dashboard aggressively is a self-inflicted, not a +// shared, resource cost. func (g *Gateway) handleAdminAPI(sw *statusTrackingWriter, r *http.Request) { u, _, ok := g.auth.identify(r) g.logAuthEvent(ok, authEventUserName(u), r) @@ -138,6 +142,8 @@ func (g *Gateway) handleAdminAPI(sw *statusTrackingWriter, r *http.Request) { g.serveAdminUsage(sw) case adminUsageHistoryPath: g.serveAdminUsageHistory(sw, r) + case adminTargetsPath: + g.serveAdminTargets(sw) } } @@ -714,3 +720,155 @@ func (g *Gateway) serveAdminUsageHistory(sw *statusTrackingWriter, r *http.Reque setAdminJSONHeaders(sw) _ = json.NewEncoder(sw).Encode(usageHistoryResponse{Scope: rawScope, Metric: metric, Window: window, Points: view}) } + +// adminTargetCountersView is one target's current-window request counters +// in GET /admin/api/targets (Feature B, v0.21) — requests only, mirroring +// limiter.targetCounters (limits.go) field for field; a target never +// accumulates tokens or cost (handleTargetProxy's own doc comment, +// mcp_a2a.go), so there is nothing else to report here. +type adminTargetCountersView struct { + RequestsPerMinute int64 `json:"requestsPerMinute"` + RequestsPerDay int64 `json:"requestsPerDay"` + RequestsPerMonth int64 `json:"requestsPerMonth"` +} + +// adminTargetView is one configured MCP server's or A2A agent's row in GET +// /admin/api/targets. Access lists the group names actually allowed to +// reach this target — computed by running the SAME matchesGlob call +// group.allowsMCP/allowsAgent themselves delegate to (auth.go) against +// each group's own cloned mcpServers/agents pattern list (groupSummary, +// authStore.snapshot), so this view can never disagree with what +// handleTargetProxy actually enforces. Access is omitted (nil) when every +// configured group can reach this target — "empty meaning all", mirroring +// groupSummary's own providers/models/mcpServers/agents omitempty +// convention (adminUsageEntryView) — rather than always listing every +// group name, which would grow with the group catalog for no reason once +// nothing is actually restricted. +type adminTargetView struct { + Name string `json:"name"` + URL string `json:"url"` + Access []string `json:"access,omitempty"` + Counters adminTargetCountersView `json:"counters"` +} + +// adminTargetsResponse is the full body of GET /admin/api/targets (Feature +// B, v0.21): every configured MCP server and every configured agent, each +// with its own access list and current-window request counters. Both +// slices are sorted by name and built fresh per request, matching every +// other admin response's "no caching of the response itself" convention. +type adminTargetsResponse struct { + MCPServers []adminTargetView `json:"mcpServers"` + Agents []adminTargetView `json:"agents"` +} + +// adminTargetAccess computes one target's access list (adminTargetView.Access's +// own doc comment): the sorted names of every group in groupSummaries whose +// own glob pattern list — read via patternsOf, so one function serves both +// MCPServers and Agents without duplicating this loop — matches name +// (matchesGlob, auth.go: the exact function group.allowsMCP/allowsAgent +// themselves call). groupSummaries is already sorted by name +// (authStore.snapshot's own contract), so the result stays sorted too. A +// match set covering every configured group collapses to nil ("empty +// meaning all") rather than echoing the full group list back. +func adminTargetAccess(groupSummaries []groupSummary, patternsOf func(groupSummary) []string, name string) []string { + matched := make([]string, 0, len(groupSummaries)) + for _, gs := range groupSummaries { + if matchesGlob(patternsOf(gs), name) { + matched = append(matched, gs.name) + } + } + if len(matched) == len(groupSummaries) { + return nil + } + return matched +} + +// sortedMCPServerNames and sortedAgentNames return the sorted key list of +// Config.MCPServers/Config.Agents — the deterministic response order every +// admin listing in this file already promises (authStore.snapshot's own +// sorted contract), and the identical pattern handleMCPServers/ +// handleAgents (mcp_a2a.go) already use for their own group-filtered +// listings. Two near-identical functions, not one generic helper, over a +// map[string]*T: this plugin runs interpreted under Yaegi, whose stdlib/ +// type-parameter support does not extend to generic code the plugin +// itself defines (tools/yaegi-check; see auth.go's cloneStringSlice for +// the same constraint applied to a generic slices.Clone call instead). +func sortedMCPServerNames(m map[string]*TargetConfig) []string { + names := make([]string, 0, len(m)) + for name := range m { + names = append(names, name) + } + sort.Strings(names) + return names +} + +func sortedAgentNames(m map[string]*AgentConfig) []string { + names := make([]string, 0, len(m)) + for name := range m { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// targetCountersView converts one limiter.targetCounters (limits.go) into +// its JSON view. Not a plain type conversion (unlike adminAliasView's own +// aliasSnapshotEntry conversion above): targetCounters' fields are +// unexported (limiter-internal), so its field names do not match +// adminTargetCountersView's exported ones and Go's identical-underlying- +// type conversion rule does not apply here. +func targetCountersView(c targetCounters) adminTargetCountersView { + return adminTargetCountersView{ + RequestsPerMinute: c.requestsPerMinute, + RequestsPerDay: c.requestsPerDay, + RequestsPerMonth: c.requestsPerMonth, + } +} + +// buildAdminTargets assembles adminTargetsResponse: every configured MCP +// server and agent, their access lists (adminTargetAccess), and their +// current-window request counters — ONE limiter.targetUsage round trip +// for every target combined (mirroring buildAdminUsage's own +// single-batch discipline), not one per target. +func (g *Gateway) buildAdminTargets() adminTargetsResponse { + _, groupSummaries := g.auth.snapshot() + + mcpNames := sortedMCPServerNames(g.cfg.MCPServers) + agentNames := sortedAgentNames(g.cfg.Agents) + + scopes := make([]limitScope, 0, len(mcpNames)+len(agentNames)) + for _, name := range mcpNames { + scopes = append(scopes, limitScope{kind: targetKindMCP, id: name}) + } + for _, name := range agentNames { + scopes = append(scopes, limitScope{kind: scopeKindAgent, id: name}) + } + counters := g.limiter.targetUsage(scopes) + + mcpServers := make([]adminTargetView, len(mcpNames)) + for i, name := range mcpNames { + mcpServers[i] = adminTargetView{ + Name: name, + URL: sanitizeBaseURL(g.cfg.MCPServers[name].URL), + Access: adminTargetAccess(groupSummaries, func(gs groupSummary) []string { return gs.mcpServers }, name), + Counters: targetCountersView(counters[i]), + } + } + agents := make([]adminTargetView, len(agentNames)) + for i, name := range agentNames { + agents[i] = adminTargetView{ + Name: name, + URL: sanitizeBaseURL(g.cfg.Agents[name].URL), + Access: adminTargetAccess(groupSummaries, func(gs groupSummary) []string { return gs.agents }, name), + Counters: targetCountersView(counters[len(mcpNames)+i]), + } + } + + return adminTargetsResponse{MCPServers: mcpServers, Agents: agents} +} + +// serveAdminTargets writes buildAdminTargets's result as JSON. +func (g *Gateway) serveAdminTargets(w http.ResponseWriter) { + setAdminJSONHeaders(w) + _ = json.NewEncoder(w).Encode(g.buildAdminTargets()) +} diff --git a/admin_test.go b/admin_test.go index 3aa8782..0401ddb 100644 --- a/admin_test.go +++ b/admin_test.go @@ -72,7 +72,7 @@ func TestAdmin_Disabled_FallsThroughTo404(t *testing.T) { cfg.Admin = nil // not registered at all h, _ := newAdminGatewayHandle(t, cfg) - for _, p := range []string{adminPagePath, adminOverviewPath, adminUsagePath} { + for _, p := range []string{adminPagePath, adminOverviewPath, adminUsagePath, adminTargetsPath} { rec := httptest.NewRecorder() h.ServeHTTP(rec, adminRequest(http.MethodGet, p, "")) if rec.Code != http.StatusNotFound { @@ -140,14 +140,14 @@ func TestAdminPage_ServedWithoutAuth(t *testing.T) { } } -// TestAdmin_GateMatrix covers the two /admin/api/* routes only — GET -// /admin itself is unauthenticated by design (TestAdminPage_ -// ServedWithoutAuth above). +// TestAdmin_GateMatrix covers the /admin/api/* routes only — GET /admin +// itself is unauthenticated by design (TestAdminPage_ServedWithoutAuth +// above). func TestAdmin_GateMatrix(t *testing.T) { cfg := newAdminTestConfig() h, _ := newAdminGatewayHandle(t, cfg) - paths := []string{adminOverviewPath, adminUsagePath} + paths := []string{adminOverviewPath, adminUsagePath, adminTargetsPath} for _, p := range paths { t.Run(p+"/unauthenticated", func(t *testing.T) { rec := httptest.NewRecorder() @@ -783,14 +783,15 @@ func TestLimiterCurrentUsage_StoreDown(t *testing.T) { // TestAdmin_NeverRateLimitedAndCountersUntouched proves the operator // directive (progress ledger, 2026-08-21 — "admin requests must NOT touch -// req/min-req/day statistics"): GET /admin/api/overview and GET -// /admin/api/usage never call checkAndCount, so an admin key with a -// requestsPerMinute limit tight enough to 429 on any other authenticated -// route never 429s here, no matter how many requests it makes — and its -// own req/min counter stays at exactly 0 throughout. This inverts the -// pre-directive TestAdmin_RateLimitApplies, which asserted the opposite -// (a 2nd request 429ing); it mirrors TestAdminUsageHistory_DoesNotCountStats, -// which already proved this for the third admin route from the start. +// req/min-req/day statistics"): GET /admin/api/overview, GET +// /admin/api/usage, and GET /admin/api/targets never call checkAndCount, +// so an admin key with a requestsPerMinute limit tight enough to 429 on +// any other authenticated route never 429s here, no matter how many +// requests it makes — and its own req/min counter stays at exactly 0 +// throughout. This inverts the pre-directive TestAdmin_RateLimitApplies, +// which asserted the opposite (a 2nd request 429ing); it mirrors +// TestAdminUsageHistory_DoesNotCountStats, which already proved this for +// the third admin route from the start. func TestAdmin_NeverRateLimitedAndCountersUntouched(t *testing.T) { t.Parallel() cfg := newAdminTestConfig() @@ -800,7 +801,7 @@ func TestAdmin_NeverRateLimitedAndCountersUntouched(t *testing.T) { fixedNow := time.Date(2026, 8, 20, 9, 0, 0, 0, time.UTC) gw.limiter.nowFn = func() time.Time { return fixedNow } - for _, p := range []string{adminOverviewPath, adminUsagePath} { + for _, p := range []string{adminOverviewPath, adminUsagePath, adminTargetsPath} { for i := 0; i < 5; i++ { rec := httptest.NewRecorder() h.ServeHTTP(rec, adminRequest(http.MethodGet, p, "sk-admin1")) @@ -1045,7 +1046,7 @@ func truncateForTest(b []byte, n int) string { // --- JSON routes: nosniff/no-store/CSP headers (review sweep, 2026-08-20) --- -// TestAdmin_JSONSecurityHeaders proves both /admin/api/* routes carry +// TestAdmin_JSONSecurityHeaders proves every /admin/api/* route carries // X-Content-Type-Options: nosniff and Cache-Control: no-store alongside // the same Content-Security-Policy the HTML page sends // (setAdminJSONHeaders, admin.go) — folded review item, 2026-08-20 @@ -1056,7 +1057,7 @@ func TestAdmin_JSONSecurityHeaders(t *testing.T) { h, _ := newAdminGatewayHandle(t, cfg) wantCSP := "default-src 'none'; script-src 'self'; style-src 'self'; connect-src 'self'; img-src 'self' data:; frame-ancestors 'none'; base-uri 'none'; form-action 'none'" - for _, p := range []string{adminOverviewPath, adminUsagePath} { + for _, p := range []string{adminOverviewPath, adminUsagePath, adminTargetsPath} { t.Run(p, func(t *testing.T) { rec := httptest.NewRecorder() h.ServeHTTP(rec, adminRequest(http.MethodGet, p, "sk-admin1")) @@ -1593,3 +1594,132 @@ func TestAdminUsageHistory_MemoryStoreFallbackWorks(t *testing.T) { t.Errorf("points = %+v, want one point with value 12", got.Points) } } + +// --- targets: GET /admin/api/targets (Feature B, v0.21) --- + +// TestAdminTargets_NoneConfigured_EmptyArraysNotNull proves an empty +// GET /admin/api/targets response marshals mcpServers/agents as "[]", not +// omitted or "null" — mirroring adminProviderView.Models' own "always an +// array, never nil" convention (admin.go) — since newAdminTestConfig +// configures neither. +func TestAdminTargets_NoneConfigured_EmptyArraysNotNull(t *testing.T) { + t.Parallel() + cfg := newAdminTestConfig() + h, _ := newAdminGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminTargetsPath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), `"mcpServers":[]`) || !strings.Contains(rec.Body.String(), `"agents":[]`) { + t.Errorf("body = %s, want mcpServers and agents both present as empty arrays, not omitted or null", rec.Body.String()) + } +} + +// TestAdminTargets_AccessListsMatchEnforcement_AndCountersReflectTraffic +// is the shape test for GET /admin/api/targets: two MCP servers and two +// agents, three groups with deliberately different mcpServers/agents glob +// restrictions, and pre-seeded per-target counters. It asserts: +// - sorted-by-name order (alpha/beta; bot1/bot2) +// - URL sanitization (sanitizeBaseURL strips embedded credentials, same +// as adminProviderView.BaseURL — TestAdminOverview_BaseURLStripsCredentials +// is this test's sibling for the provider view) +// - access is computed via the exact SAME matchesGlob call +// group.allowsMCP/allowsAgent themselves use (auth.go): a target every +// configured group can reach collapses to an omitted/nil access list +// ("empty meaning all"), one only some groups can reach lists exactly +// those group names, sorted +// - counters echo limiter.targetUsage's read of the exact keys +// countTargetRequest (limits.go) writes, at min/day/month +// +// "restricted" explicitly lists both alpha (mcpServers) and bot1 (agents); +// "wide" and "admingroup" both leave their own mcpServers/agents fields +// unset (empty pattern list = allow-all, matchesGlob's own contract) — so +// alpha and bot1 are reachable by all three configured groups (access +// collapses to nil), while beta and bot2 are reachable by "wide" and +// "admingroup" only, never "restricted" (access = ["admingroup","wide"]). +func TestAdminTargets_AccessListsMatchEnforcement_AndCountersReflectTraffic(t *testing.T) { + t.Parallel() + + // Built via net/url, not a literal string, so a generic secret scanner + // never flags a credential-shaped literal in this test's own source — + // same convention as TestAdminOverview_BaseURLStripsCredentials above. + alphaURL := (&url.URL{Scheme: "http", User: url.UserPassword("mcpuser", "mcppass"), Host: "mcp-alpha.internal"}).String() + + cfg := CreateConfig() + cfg.Admin = &AdminConfig{Enabled: true} + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + cfg.MCPServers = map[string]*TargetConfig{ + "alpha": {URL: alphaURL}, + "beta": {URL: "http://mcp-beta.internal"}, + } + cfg.Agents = map[string]*AgentConfig{ + "bot1": {URL: "http://agent-bot1.internal"}, + "bot2": {URL: "http://agent-bot2.internal"}, + } + cfg.Groups = map[string]*GroupConfig{ + "restricted": {MCPServers: []string{"alpha"}, Agents: []string{"bot1"}}, + "wide": {}, + "admingroup": {}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "admin1", Group: "admingroup", APIKey: "sk-admin1", Admin: true}, + }} + h, gw := newAdminGatewayHandle(t, cfg) + + fixedNow := time.Date(2026, 8, 21, 10, 0, 0, 0, time.UTC) + gw.limiter.nowFn = func() time.Time { return fixedNow } + gw.limiter.countTargetRequest(targetKindMCP, "alpha") + gw.limiter.countTargetRequest(targetKindMCP, "alpha") + gw.limiter.countTargetRequest(scopeKindAgent, "bot1") + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminTargetsPath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var got adminTargetsResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + + if len(got.MCPServers) != 2 || got.MCPServers[0].Name != "alpha" || got.MCPServers[1].Name != "beta" { + t.Fatalf("mcpServers not sorted by name: %+v", got.MCPServers) + } + if len(got.Agents) != 2 || got.Agents[0].Name != "bot1" || got.Agents[1].Name != "bot2" { + t.Fatalf("agents not sorted by name: %+v", got.Agents) + } + + alpha := got.MCPServers[0] + if alpha.URL != "http://mcp-alpha.internal" { + t.Errorf("alpha.URL = %q, want credentials stripped to http://mcp-alpha.internal", alpha.URL) + } + if alpha.Access != nil { + t.Errorf("alpha.Access = %v, want nil (every configured group can reach it — 'empty meaning all')", alpha.Access) + } + if alpha.Counters.RequestsPerMinute != 2 || alpha.Counters.RequestsPerDay != 2 || alpha.Counters.RequestsPerMonth != 2 { + t.Errorf("alpha.Counters = %+v, want min/day/month all 2", alpha.Counters) + } + + beta := got.MCPServers[1] + if want := []string{"admingroup", "wide"}; !slices.Equal(beta.Access, want) { + t.Errorf("beta.Access = %v, want %v (restricted's own mcpServers list excludes it)", beta.Access, want) + } + if beta.Counters != (adminTargetCountersView{}) { + t.Errorf("beta.Counters = %+v, want all zero (never proxied to in this test)", beta.Counters) + } + + bot1 := got.Agents[0] + if bot1.Access != nil { + t.Errorf("bot1.Access = %v, want nil (every configured group can reach it)", bot1.Access) + } + if bot1.Counters.RequestsPerMinute != 1 || bot1.Counters.RequestsPerDay != 1 || bot1.Counters.RequestsPerMonth != 1 { + t.Errorf("bot1.Counters = %+v, want min/day/month all 1", bot1.Counters) + } + + bot2 := got.Agents[1] + if want := []string{"admingroup", "wide"}; !slices.Equal(bot2.Access, want) { + t.Errorf("bot2.Access = %v, want %v (restricted's own agents list excludes it)", bot2.Access, want) + } +} diff --git a/limits.go b/limits.go index 8a477c1..3561b99 100644 --- a/limits.go +++ b/limits.go @@ -1011,6 +1011,109 @@ func (l *limiter) account(scopes []limitScope, u usage, costMicros int64) { l.storeIncrMulti(entries) // no error return by contract (doc comment above); ok is intentionally discarded } +// countTargetRequest increments one MCP-server's or A2A agent's per-target +// request counters (Feature B, v0.21) at min, hour, day, AND month — the +// only scope kind this package tracks a month window of REQUESTS for. +// kind is targetScopeKind's own output ("mcp" or scopeKindAgent — +// mcp_a2a.go), id is the configured target name; both come embedded in +// windowKey's own key string, so a target scope can never collide with a +// user/group/total scope of the same id even by coincidence — kind is +// part of the key, not just a struct field. +// +// This is deliberately NOT folded into checkAndCount: a target scope +// carries no limits by design (this round adds no config surface for +// MCP/agent limits), so there is nothing for evaluateScope to check and no +// violation this call could ever produce — it is pure accounting, exactly +// like account() itself, hence the identical "no error return, ok +// discarded" contract. Month exists here, uniquely, because the admin +// dashboard's targets endpoint (admin.go's adminTargetCountersView) +// surfaces a requestsPerMonth figure no user/group admin view does; +// growing checkAndCount's own req:min/day/hour batch to a month window for +// EVERY scope kind, just to serve this one endpoint's response shape, was +// rejected as scope creep touching user/group accounting nothing else in +// this round asked to change. +// +// Callers: handleTargetProxy (mcp_a2a.go, once per proxied request, after +// its own user/group/total admission check passes) and the federated /mcp +// endpoint (mcp_federation.go, once per actual backend server contacted — +// tools/list may contact several in one incoming client request, tools/call +// exactly one), so a call attributed to a specific target is counted the +// same way regardless of which route reached it. +func (l *limiter) countTargetRequest(kind, id string) { + now := l.now() + l.storeIncrMulti([]counterIncr{ + newCounterIncr(kind, id, metricReq, windowMin, now, 1, minWindowTTL), + newCounterIncr(kind, id, metricReq, windowHour, now, 1, hourWindowTTL), + newCounterIncr(kind, id, metricReq, windowDay, now, 1, dayWindowTTL), + newCounterIncr(kind, id, metricReq, windowMonth, now, 1, monthWindowTTL), + }) +} + +// targetCounterKeysPerScope is the number of windowKey strings +// targetUsageKeys builds per target scope, and the stride targetUsage's +// flat storeGetMulti result is sliced back into per-target chunks by. +const targetCounterKeysPerScope = 3 + +// targetUsageKeys returns the targetCounterKeysPerScope windowKey strings +// targetUsage reads for one (kind, id) target scope at time now — req at +// min, day, and month granularity, matching adminTargetCountersView's own +// field order (admin.go) exactly, so targetUsage can map storeGetMulti's +// result slice back by plain index. Hour is deliberately not read here: +// it is stats-only everywhere else in this package (windowHour's own doc +// comment) and no admin view — targets included — ever surfaces it. +func targetUsageKeys(kind, id string, now time.Time) []string { + return []string{ + windowKey(kind, id, metricReq, windowMin, now), + windowKey(kind, id, metricReq, windowDay, now), + windowKey(kind, id, metricReq, windowMonth, now), + } +} + +// targetCounters is one MCP-server's or agent's current-window request +// counters (Feature B, v0.21) — requests only, mirroring handleTargetProxy's +// own passthrough-only accounting (mcp_a2a.go): a target scope never +// accumulates tokens or cost, so there is nothing else to report. +type targetCounters struct { + requestsPerMinute int64 + requestsPerDay int64 + requestsPerMonth int64 +} + +// targetUsage reads every scope's current req:min/day/month counters in +// ONE storeGetMulti round trip, mirroring currentUsage's own single-batch +// discipline for the same reason: GET /admin/api/targets (admin.go) must +// not pay one round trip per configured MCP server and agent. Order is +// preserved: targetUsage(scopes)[i] corresponds to scopes[i]. Unlike +// scopeUsage, there is no per-entry storeDown flag — a fail-closed read +// (storeGetMulti's own contract) reports every target's counters as zero +// rather than partial/stale values, which is acceptable here specifically +// because a target scope carries no limit: nothing downstream ever treats +// this zero as "confirmed no traffic" the way scopeUsage.storeDown guards +// against for a limited scope. +func (l *limiter) targetUsage(scopes []limitScope) []targetCounters { + out := make([]targetCounters, len(scopes)) + if len(scopes) == 0 { + return out + } + + now := l.now() + allKeys := make([]string, 0, len(scopes)*targetCounterKeysPerScope) + for _, sc := range scopes { + allKeys = append(allKeys, targetUsageKeys(sc.kind, sc.id, now)...) + } + + vals, ok := l.storeGetMulti(allKeys) + if !ok || len(vals) != len(allKeys) { + return out // zero-value counters; see doc comment above + } + + for i := range scopes { + v := vals[i*targetCounterKeysPerScope : i*targetCounterKeysPerScope+targetCounterKeysPerScope] + out[i] = targetCounters{requestsPerMinute: v[0], requestsPerDay: v[1], requestsPerMonth: v[2]} + } + return out +} + // usageKeysPerScope is the number of windowKey strings usageWindowKeys // builds per scope, and the stride currentUsage's flat storeGetMulti // result is sliced back into per-scope chunks by. Named here, closing a diff --git a/mcp_a2a.go b/mcp_a2a.go index e854867..3c1f059 100644 --- a/mcp_a2a.go +++ b/mcp_a2a.go @@ -20,6 +20,29 @@ const ( // listed agent whose AgentConfig.Card is empty. const defaultAgentCardPath = "/.well-known/agent-card.json" +// scopeKindAgent is the limitScope.kind (and admin-API scope-kind) string +// an A2A agent target's per-target request counters use (Feature B, +// v0.21) — deliberately NOT targetKindAgent ("a2a", this file's own +// URL-routing convention): "agent" is the vocabulary GET /admin/api/targets +// (admin.go) and the webui's MCP & Agents panel use. The split matters for +// collision-safety too: windowKey (limits.go) embeds kind as the counter +// key's own leading segment, so an MCP server and an A2A agent configured +// with the identical name — or either one sharing a name with a user or +// group — can never share a counter regardless of what any of them are +// called; kind, not id alone, is what makes a scope unique. +const scopeKindAgent = "agent" + +// targetScopeKind maps handleTargetProxy's routing kind (targetKindMCP or +// targetKindAgent) to the accounting scope kind its per-target request +// counters use: identical for MCP (targetKindMCP is already "mcp"), but +// remapped to scopeKindAgent ("agent") for an A2A target. +func targetScopeKind(kind string) string { + if kind == targetKindAgent { + return scopeKindAgent + } + return kind +} + // mcpServerListing is one entry of GET /v1/mcp/servers: the configured // name and the gateway-relative URL a client proxies requests to it // through. @@ -135,8 +158,11 @@ func (g *Gateway) resolveTarget(kind, name string, grp *group) (targetURL string // MCP servers and A2A agents are in-cluster targets that trust the // gateway's network position, not a per-provider API key the gateway // holds on the caller's behalf. Accounting is likewise passthrough-only — -// a target request is counted by checkAndCount below, but its response -// is never teed off for token/cost usage extraction. +// a target request is counted by checkAndCount below (the caller's own +// user/group/total scopes) and, additionally, attributed to name's own +// per-target scope (limiter.countTargetRequest, Feature B v0.21) — but its +// response is never teed off for token/cost usage extraction, for any of +// those scopes. // // Checks run: unknown name (404) before group authorization (403) before // capability/path checks (Upgrade→501, traversal→400) before rate limits @@ -146,6 +172,15 @@ func (g *Gateway) resolveTarget(kind, name string, grp *group) (targetURL string // request rejected on any check before checkAndCount must never burn the // caller's or their group's rate-limit quota for a call that was never // going to reach the upstream. +// +// Once admitted, limiter.countTargetRequest (limits.go) additionally +// attributes the request to name's own per-target scope (Feature B, +// v0.21) — requests only, same as the caller's own user/group counters: +// GET /admin/api/targets (admin.go) reads these back for the webui's MCP & +// Agents panel. This scope carries no limit of its own (no config surface +// added this round) and so can never itself cause a 429/503; it is purely +// additive visibility on top of the user/group/total admission decision +// checkAndCount already made above. func (g *Gateway) handleTargetProxy(w http.ResponseWriter, r *http.Request, u *user, grp *group, kind, name, rest string) { targetURL, allowed, known := g.resolveTarget(kind, name, grp) if !known { @@ -171,6 +206,7 @@ func (g *Gateway) handleTargetProxy(w http.ResponseWriter, r *http.Request, u *u writeLimitViolation(w, violation) return } + g.limiter.countTargetRequest(targetScopeKind(kind), name) upstreamURL := targetURL if rest != "" { diff --git a/mcp_a2a_test.go b/mcp_a2a_test.go index be8721a..5b05d93 100644 --- a/mcp_a2a_test.go +++ b/mcp_a2a_test.go @@ -779,3 +779,99 @@ func TestNewGateway_TargetURLValidation(t *testing.T) { }) } } + +// --- Feature B (v0.21): per-target request accounting --- + +func TestTargetScopeKind(t *testing.T) { + cases := []struct { + routingKind string + want string + }{ + {targetKindMCP, "mcp"}, + {targetKindAgent, scopeKindAgent}, + } + for _, tc := range cases { + t.Run(tc.routingKind, func(t *testing.T) { + if got := targetScopeKind(tc.routingKind); got != tc.want { + t.Errorf("targetScopeKind(%q) = %q, want %q", tc.routingKind, got, tc.want) + } + }) + } +} + +// TestHandleTargetProxy_PerTargetCounters_AttributedAndCollisionSafe proves +// handleTargetProxy attributes a proxied request to name's own per-target +// scope (Feature B, v0.21) at every window countTargetRequest tracks +// (min/hour/day/month), for both an MCP server and an A2A agent target, +// AND that this never collides with a same-named user counter: both cases +// deliberately name the target "alice" — identical to the calling user's +// own name — so a bug that dropped kind from the counter key (windowKey, +// limits.go) would show up as the user's own counters silently absorbing +// the target's, or vice versa. +func TestHandleTargetProxy_PerTargetCounters_AttributedAndCollisionSafe(t *testing.T) { + cases := []struct { + wantScopeKind string + routingKind string + targetName string + path string + }{ + {wantScopeKind: targetKindMCP, routingKind: targetKindMCP, targetName: "alice", path: "/mcp/alice/tools/list"}, + {wantScopeKind: scopeKindAgent, routingKind: targetKindAgent, targetName: "alice", path: "/a2a/alice/tasks/send"}, + } + for _, tc := range cases { + t.Run(tc.routingKind, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + if tc.routingKind == targetKindMCP { + cfg.MCPServers = map[string]*TargetConfig{tc.targetName: {URL: srv.URL}} + } else { + cfg.Agents = map[string]*AgentConfig{tc.targetName: {URL: srv.URL}} + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}} + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + h, err := New(context.Background(), next, cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + + req := httptest.NewRequest(http.MethodGet, tc.path, nil) + req.Header.Set("Authorization", "Bearer sk-alice") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + now := time.Now() + for _, window := range []string{windowMin, windowHour, windowDay, windowMonth} { + got, gotOK := gw.limiter.getCounter(tc.wantScopeKind, tc.targetName, metricReq, window, now) + if !gotOK || got != 1 { + t.Errorf("%s/%s req:%s counter = %d (ok=%v), want 1", tc.wantScopeKind, tc.targetName, window, got, gotOK) + } + } + + // Collision guard: the caller's own USER scope (kind="user") + // must stay independent of the TARGET scope above despite + // sharing the identical id string "alice" — proving kind, not + // id alone, is what makes a counter key unique. + userReqMin, minOK := gw.limiter.getCounter("user", "alice", metricReq, windowMin, now) + if !minOK || userReqMin != 1 { + t.Errorf("user/alice req:min counter = %d (ok=%v), want 1 (its own scope, unaffected by the target scope of the same id)", userReqMin, minOK) + } + userReqMonth, monthOK := gw.limiter.getCounter("user", "alice", metricReq, windowMonth, now) + if !monthOK || userReqMonth != 0 { + t.Errorf("user/alice req:month counter = %d (ok=%v), want 0 (checkAndCount never writes a month window for user/group scopes; nonzero here would mean the target scope's month write leaked into it)", userReqMonth, monthOK) + } + }) + } +}