From 9fced97050b8554718bf4a38567f3955b76f06a3 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 21 Aug 2026 09:24:35 +0100 Subject: [PATCH] feat(admin): exclude admin traffic from usage statistics Operator directive (progress ledger, 2026-08-21): admin requests must not touch req/min-req/day statistics. handleAdminAPI no longer calls checkAndCount for GET /admin/api/overview or GET /admin/api/usage (it already skipped it for GET /admin/api/usage/history) - admin traffic must never appear in usage statistics, which measure real LLM traffic only. identify()'s group return is now discarded (u, _, ok :=), since buildLimitScopes is no longer built here. Direct consequence, documented in the new doc comment: an admin's own requestsPerMinute/requestsPerDay limit, if configured, is no longer enforced against admin-route traffic either - accepted trade-off (admin-gated, cheap reads; a self-inflicted, not shared, resource cost). admin_test.go: TestAdmin_RateLimitApplies (asserted a 2nd request 429s) inverted to TestAdmin_NeverRateLimitedAndCountersUntouched - 5x requests to both overview and usage against an admin key pinned at RequestsPerMinute:1, asserting every one is 200 and the admin's own req/min counter stays exactly 0. integration/integration_test.go: TestAdmin_Dashboard's admin-user assertion inverted from requestsPerDay > 0 ("admin API calls count too") to requestsPerDay == 0 ("admin routes never call checkAndCount") - the admin key is used throughout the whole suite exclusively to hit /admin/api/*, so it must show exactly 0 despite issuing many requests by the time this test runs. --- admin.go | 40 ++++++++++++++------------------- admin_test.go | 36 ++++++++++++++++++----------- integration/integration_test.go | 24 ++++++++++++++------ 3 files changed, 57 insertions(+), 43 deletions(-) diff --git a/admin.go b/admin.go index c1d8d29..596e92a 100644 --- a/admin.go +++ b/admin.go @@ -100,21 +100,23 @@ 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. GET /admin/api/overview and GET -// /admin/api/usage count request counters via checkAndCount before -// serving, like every other authenticated route: an admin over their own -// req/min limit gets a 429 here exactly as they would on any other route -// (spec §4's "Accounting: admin routes count request counters like any -// authed route"). +// non-admin → 403, admin → serve. // -// GET /admin/api/usage/history (v0.2 data-layer task) is the one -// exception: it skips checkAndCount entirely. The operator-directed -// removal of admin-route stat counting (progress ledger, 2026-08-20) -// lands for every /admin/api/* route in a later task; this new route is -// built consistent with that direction from the start, rather than -// counted now and un-counted later. +// 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. func (g *Gateway) handleAdminAPI(sw *statusTrackingWriter, r *http.Request) { - u, grp, ok := g.auth.identify(r) + u, _, ok := g.auth.identify(r) g.logAuthEvent(ok, authEventUserName(u), r) if !ok { writeOAIError(sw, http.StatusUnauthorized, "authentication_error", "invalid or missing API key") @@ -125,21 +127,13 @@ func (g *Gateway) handleAdminAPI(sw *statusTrackingWriter, r *http.Request) { return } - if r.URL.Path == adminUsageHistoryPath { - g.serveAdminUsageHistory(sw, r) - return - } - - if violation := g.limiter.checkAndCount(buildLimitScopes(u, grp)); violation != nil { - writeLimitViolation(sw, violation) - return - } - switch r.URL.Path { case adminOverviewPath: g.serveAdminOverview(sw) case adminUsagePath: g.serveAdminUsage(sw) + case adminUsageHistoryPath: + g.serveAdminUsageHistory(sw, r) } } diff --git a/admin_test.go b/admin_test.go index c188e8e..732ec56 100644 --- a/admin_test.go +++ b/admin_test.go @@ -504,9 +504,19 @@ func TestLimiterCurrentUsage_StoreDown(t *testing.T) { } } -// --- rate limit applies to admin routes --- +// --- admin routes never rate-limit and never move usage counters (operator directive, 2026-08-21) --- -func TestAdmin_RateLimitApplies(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. +func TestAdmin_NeverRateLimitedAndCountersUntouched(t *testing.T) { t.Parallel() cfg := newAdminTestConfig() cfg.Users.Inline[1].Limits = &LimitsConfig{RequestsPerMinute: 1} // admin1 @@ -515,19 +525,19 @@ func TestAdmin_RateLimitApplies(t *testing.T) { fixedNow := time.Date(2026, 8, 20, 9, 0, 0, 0, time.UTC) gw.limiter.nowFn = func() time.Time { return fixedNow } - rec1 := httptest.NewRecorder() - h.ServeHTTP(rec1, adminRequest(http.MethodGet, adminOverviewPath, "sk-admin1")) - if rec1.Code != http.StatusOK { - t.Fatalf("1st request status = %d, want 200, body=%s", rec1.Code, rec1.Body.String()) + for _, p := range []string{adminOverviewPath, adminUsagePath} { + for i := 0; i < 5; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, p, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("%s request %d: status = %d, want 200 (never 429), body=%s", p, i, rec.Code, rec.Body.String()) + } + } } - rec2 := httptest.NewRecorder() - h.ServeHTTP(rec2, adminRequest(http.MethodGet, adminOverviewPath, "sk-admin1")) - if rec2.Code != http.StatusTooManyRequests { - t.Fatalf("2nd request status = %d, want 429 (admin over req/min limit)", rec2.Code) - } - if rec2.Header().Get("Retry-After") == "" { - t.Error("429 response must carry a Retry-After header") + reqCount, ok := gw.limiter.getCounter("user", "admin1", metricReq, windowMin, fixedNow) + if !ok || reqCount != 0 { + t.Errorf("admin1 req/min counter = %d (ok=%v), want 0 (admin routes never call checkAndCount)", reqCount, ok) } } diff --git a/integration/integration_test.go b/integration/integration_test.go index 07cb236..d672801 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -949,10 +949,13 @@ func TestAudioTranscriptions(t *testing.T) { // TestAdminDashboard covers v0.2 integration coverage (spec §4): the // unauthenticated HTML shell carries no data, the two JSON routes gate // on admin vs. non-admin vs. unauthenticated, and GET /admin/api/usage -// reflects the traffic this suite has generated by the time this test -// runs — deliberately placed near the end of the file (Go runs tests in -// source order with no -shuffle in this suite's Makefile target) so -// alice's own request counters are already non-zero. +// reflects the REAL LLM traffic this suite has generated by the time this +// test runs — deliberately placed near the end of the file (Go runs +// tests in source order with no -shuffle in this suite's Makefile +// target) so alice's own request counters are already non-zero. It also +// asserts the stats-exclusion directive's other half: the admin key +// itself, used throughout this suite only to call /admin/api/* routes, +// must show exactly 0 requestsPerDay — admin traffic is never counted. func TestAdminDashboard(t *testing.T) { pageReq, err := http.NewRequest(http.MethodGet, traefik1URL+"/admin", nil) if err != nil { @@ -995,11 +998,18 @@ func TestAdminDashboard(t *testing.T) { aliceUsage := adminUsageEntry(t, "users", "alice") if reqDay, _ := aliceUsage["requestsPerDay"].(float64); reqDay <= 0 { - t.Errorf("alice requestsPerDay = %v, want > 0 (this suite's own earlier traffic)", reqDay) + t.Errorf("alice requestsPerDay = %v, want > 0 (this suite's own earlier real-LLM traffic)", reqDay) } + // Stats-exclusion directive (progress ledger, 2026-08-21): admin routes + // never call checkAndCount, so the admin key — used throughout this + // suite ONLY to call /admin/api/* (adminUsageEntry/adminUsageHistory, + // plus this test's own overview/usage calls above) — must show exactly + // 0 requestsPerDay despite having issued many requests by this point in + // the run. This inverts the pre-directive assertion here, which + // expected admin's own admin-API calls to count like any other route. adminUsage := adminUsageEntry(t, "users", "admin") - if reqDay, _ := adminUsage["requestsPerDay"].(float64); reqDay <= 0 { - t.Errorf("admin requestsPerDay = %v, want > 0 (this test's own admin API calls count too)", reqDay) + if reqDay, _ := adminUsage["requestsPerDay"].(float64); reqDay != 0 { + t.Errorf("admin requestsPerDay = %v, want 0 (admin routes never call checkAndCount)", reqDay) } // v0.2 data-layer task: GET /admin/api/usage/history's hour series for