From d58eef9aa758a994e83b759a0cf7df7179974d93 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:18:45 +0100 Subject: [PATCH 1/8] feat(metrics): add Prometheus text-exposition endpoint Serve a hand-written Prometheus text-exposition document at a configurable path, defaulting to /metrics. The plugin is stdlib-only under Yaegi, so this cannot use a client library. Add MetricsConfig: Enabled gates the route, Path overrides it, AllowedCIDRs lets an unauthenticated caller scrape from a trusted source address, and ModelLabel opts into a per-(provider,model) breakdown that defaults off to avoid a series-count explosion. Reuse the existing admin bearer-token auth path instead of adding a second credential check. Reuse the batched, chunked reads GET /admin/api/overview and GET /admin/api/usage already use, so a scrape costs no extra store round trip. Expose request, token, and cost counters per scope, provider attempt and failure counters, a provider-health gauge from the discovery circuit breaker, and a budget-consumed-ratio gauge per limited scope for Prometheus alerting. Add a rate-limit-rejection counter in limits.go: this is the one number with no existing accounting to read back, so checkAndCount now increments a small in-process, per-process-lifetime counter on every violation. Escape every label value for backslash, double quote, and newline. An unescaped quote in a label value, such as a discovered model id, makes the whole scrape unparseable, and Prometheus reports an unparseable scrape as the target being down rather than as one bad line. Cover the auth gate matrix, exposition structure (TYPE per family, counters ending _total, no duplicate series), label escaping round-trips, the model-label opt-in, and counters reflecting real seeded usage rather than zeros. --- limits.go | 95 ++++++ llmgateway.go | 32 +- metrics.go | 483 ++++++++++++++++++++++++++++++ metrics_test.go | 774 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1383 insertions(+), 1 deletion(-) create mode 100644 metrics.go create mode 100644 metrics_test.go diff --git a/limits.go b/limits.go index 61ae8c9..041e14b 100644 --- a/limits.go +++ b/limits.go @@ -583,10 +583,101 @@ type limiter struct { // not "is the store currently failing" (storeLatched already answers // that question for the enforcement path). lastErrMsg string + // rejections tracks rate/budget-limit-violation counts per scope + // (metrics.go's llmgateway_rate_limit_rejections_total). It is a + // plain in-process, per-process-lifetime counter guarded by its own + // mutex, deliberately NOT a counterStore key: checkAndCount already + // pays a Redis round trip on every admission decision, and every + // number metrics.go otherwise exposes is read back OUT of that + // existing accounting rather than written fresh — but "how many + // requests were rejected" has no existing counter to read at all + // (req:min/req:day count every admission attempt, accepted or not, + // by design — see checkAndCount's own doc comment). Adding it here + // as a bare map increment costs one uncontended lock per rejection + // (already the rare, not-hot-path outcome of checkAndCount), never a + // second network round trip, and never re-derives a number the + // store already tracks. + rejections rejectionCounter logMu sync.Mutex failOpen bool // store-error policy: true falls back to fallback, false refuses the request } +// rejectionScope is the (kind, id) key rejectionCounter accumulates +// under — a scope's own kind/id pair, exactly as limitScope carries them, +// but held separately so rejectionCounter's map key never aliases a +// limitScope's own `limits` pointer (which plays no part in identity +// here: two limitScope values for the same user with different `limits` +// pointers must still land in the identical bucket). +type rejectionScope struct { + kind string + id string +} + +// rejectionCounter accumulates rate/budget-limit-violation counts per +// scope for the lifetime of the process — limiter.rejections' own +// backing store; see its doc comment for why this exists as a bare +// in-process map rather than a counterStore key. Unlike authFailureTracker +// (auth.go), there is no TTL or generation rotation here: a Prometheus +// counter must only ever grow for as long as the process runs, so there +// is nothing to roll over, and cardinality is bounded by the configured +// user/group count (checkAndCount only ever sees scopes for an already- +// authenticated caller's own user/group, never attacker-controlled +// strings), the same bound scopeUsage's own per-scope metrics already +// carry. +type rejectionCounter struct { + counts map[rejectionScope]int64 + mu sync.Mutex +} + +// increment records one rejection for (kind, id), initializing the +// backing map on first use — rejectionCounter's zero value (as embedded, +// unexported, in limiter) is ready to use without a constructor. +func (c *rejectionCounter) increment(kind, id string) { + c.mu.Lock() + if c.counts == nil { + c.counts = make(map[rejectionScope]int64) + } + c.counts[rejectionScope{kind: kind, id: id}]++ + c.mu.Unlock() +} + +// rejectionSnapshot is one (kind, id) scope's rate-limit-rejection count +// — rejectionCounter.snapshot's own output, metrics.go's read of it. +type rejectionSnapshot struct { + kind string + id string + count int64 +} + +// snapshot returns every scope with at least one recorded rejection, in +// no particular order (metrics.go sorts its own copy before rendering). +func (c *rejectionCounter) snapshot() []rejectionSnapshot { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]rejectionSnapshot, 0, len(c.counts)) + for scope, n := range c.counts { + out = append(out, rejectionSnapshot{kind: scope.kind, id: scope.id, count: n}) + } + return out +} + +// rejectionScopeStoreDown is the synthetic scope id checkAndCount +// records a rejection under when it refuses a request because the +// configured store is unreachable and failOpen is false +// (storeDownViolation) — there is no real limitScope in play at that +// point (the failure happens before any scope is evaluated), so this +// names the event on its own rather than attributing it to whichever +// scope happened to be first in the slice. +const rejectionScopeStoreDown = "store_down" + +// rejectionSnapshot (method) reads l.rejections — metrics.go's own entry +// point, named to match l's other read accessors (redisStatus, +// currentUsage, providerUsage) rather than exposing the rejections field +// itself. +func (l *limiter) rejectionSnapshot() []rejectionSnapshot { + return l.rejections.snapshot() +} + // providerAttemptSpawnCap bounds how many concurrent recordProviderAttempt // store writes one limiter's production spawn implementation will run at // once (FOLDED-1, v0.22 review round, round 2) — see limiter.spawnTokens' @@ -1095,6 +1186,7 @@ func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { incrVals, readVals, ok := l.storeIncrAndGetMulti(entries, reads) if !ok || len(incrVals) != len(entries) || len(readVals) != len(reads) { + l.rejections.increment(rejectionScopeStoreDown, rejectionScopeStoreDown) return storeDownViolation() } @@ -1107,9 +1199,11 @@ func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { // incrVals[i*checkAndCountKeysPerScope+2] is the req:hour count — // stats-only, deliberately never read here. if v := requestLimitViolation(sc, "requests-per-minute", sc.limits.RequestsPerMinute, minCount, windowMin, now); v != nil { + l.rejections.increment(sc.kind, sc.id) return v } if v := requestLimitViolation(sc, "requests-per-day", sc.limits.RequestsPerDay, dayCount, windowDay, now); v != nil { + l.rejections.increment(sc.kind, sc.id) return v } for _, p := range probes { @@ -1123,6 +1217,7 @@ func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { if used < p.limit { continue } + l.rejections.increment(sc.kind, sc.id) return &limitViolation{ message: fmt.Sprintf("%s %q exceeded %s budget", sc.kind, sc.id, p.name), retryAfter: retryAfterSeconds(now, p.window), diff --git a/llmgateway.go b/llmgateway.go index ed15ff0..93dd252 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "net" "net/http" "os" "runtime" @@ -29,7 +30,13 @@ type Config struct { // Admin.Enabled false means the /admin* routes are not registered at // all — ServeHTTP falls through to its existing 404/passthroughUnknown // handling for those paths, preserving v0.1 behavior exactly. - Admin *AdminConfig `json:"admin,omitempty"` + Admin *AdminConfig `json:"admin,omitempty"` + // Metrics gates the Prometheus text-exposition endpoint (default + // path /metrics, configurable — see MetricsConfig, metrics.go). nil + // or Metrics.Enabled false means the route is not registered at + // all, the identical nil-disables convention Admin above already + // uses. + Metrics *MetricsConfig `json:"metrics,omitempty"` Pricing map[string]*ModelPricing `json:"pricing,omitempty"` MCPServers map[string]*TargetConfig `json:"mcpServers,omitempty"` Agents map[string]*AgentConfig `json:"agents,omitempty"` @@ -526,6 +533,11 @@ type Gateway struct { // contract. bodyAdmission chan struct{} name string + // metricsNets is Config.Metrics.AllowedCIDRs, parsed once at + // construction (parseMetricsCIDRs, metrics.go) so the request path + // never re-parses a CIDR string per scrape. nil when Metrics is + // unconfigured or carries no AllowedCIDRs entries. + metricsNets []*net.IPNet } // telemetryStartupOnce keeps the anonymous "plugin loaded" ping to one per @@ -634,6 +646,16 @@ func newGateway(ctx context.Context, next http.Handler, config *Config, name str } g := &Gateway{next: next, name: name, cfg: config, auth: auth} + // A malformed Config.Metrics.AllowedCIDRs entry is a constructor + // error (validate-at-construction), not a silently-ignored allowlist + // entry an operator would only discover was never applied by testing + // it — parseMetricsCIDRs' own doc comment (metrics.go). + metricsNets, err := parseMetricsCIDRs(config.Metrics) + if err != nil { + return nil, err + } + g.metricsNets = metricsNets + // bodyAdmissionCap: an explicit MaxInFlightBodyRequests always wins // over the self-tuned default (house rule: explicit override always // wins) — see Config.MaxInFlightBodyRequests' own doc comment. 0 (the @@ -878,6 +900,14 @@ func (g *Gateway) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Metrics route (metrics.go): matched only when metricsEnabled — a + // nil or disabled Config.Metrics means the configured path is not + // registered at all, mirroring the admin route's own gate above. + if metricsEnabled(g.cfg) && r.Method == http.MethodGet && r.URL.Path == metricsPath(g.cfg) { + g.handleMetrics(sw, r) + return + } + if r.Method == http.MethodGet && (r.URL.Path == "/v1/mcp/servers" || r.URL.Path == "/v1/agents") { u, grp, ok := g.auth.identify(r) g.logAuthEvent(ok, authEventUserName(u), r) diff --git a/metrics.go b/metrics.go new file mode 100644 index 0000000..cde6098 --- /dev/null +++ b/metrics.go @@ -0,0 +1,483 @@ +package traefikllmgateway + +import ( + "bytes" + "fmt" + "net" + "net/http" + "sort" + "strconv" +) + +// MetricsConfig configures the Prometheus text-exposition endpoint +// (default path /metrics, overridable via Path). nil or Enabled false +// means the metrics route is not registered at all, matching Admin's own +// nil-disables convention (AdminConfig, llmgateway.go). +// +// This endpoint exposes per-user and per-group cost and usage, so it must +// never be reachable unauthenticated by default. Auth reuses the SAME +// admin-bearer-token check GET /admin/api/* already applies (auth.go's +// identify, gated on user.admin) — handleMetrics below is not a second +// credential path. AllowedCIDRs is an additional, OPTIONAL bypass for a +// caller whose SOURCE ADDRESS is trusted enough to skip the key entirely +// — a co-located Prometheus server that should not need to carry a +// credential — checked against the request's source address exactly as +// clientIP reads it elsewhere in this package (net/http's own +// RemoteAddr, never a client-supplied header, auth.go's own doc comment). +// This is only meaningful for a caller whose real source address +// actually survives to this gateway: behind a Kubernetes Service or a +// second reverse proxy that does not preserve the original address, +// every caller looks identical to this check, and the allowlist becomes +// either "everyone" or "no one" depending on what address arrives. +// Confirm RemoteAddr reflects the scraper's own address in your +// deployment before relying on this instead of a key. +type MetricsConfig struct { + // Path overrides the served path. Empty (the default) serves at + // metricsPathDefault ("/metrics"). + Path string `json:"path,omitempty"` + // AllowedCIDRs lists CIDR blocks (e.g. "10.0.0.0/8") whose source + // address may scrape without an admin key. Empty means every + // scrape must carry a valid admin bearer token — see this type's + // own doc comment for the trust assumption this relies on. + AllowedCIDRs []string `json:"allowedCIDRs,omitempty"` + Enabled bool `json:"enabled,omitempty"` + // ModelLabel adds a `model` label to the provider attempt/failure + // counters (llmgateway_provider_model_attempts_total/ + // llmgateway_provider_model_failures_total), breaking them out per + // (provider, model) pair in addition to the always-on per-provider + // totals. Defaults to false: a deployment with roughly 1,000 + // discovered model ids across several providers is a series-count + // explosion waiting to happen, so this stays opt-in — enable only + // when your Prometheus retention and cardinality budget can absorb + // one series per (provider, model) pair actually seen. + ModelLabel bool `json:"modelLabel,omitempty"` +} + +// metricsPathDefault is where the Prometheus endpoint is served when +// MetricsConfig.Path is left empty. +const metricsPathDefault = "/metrics" + +// metricsEnabled reports whether cfg's Metrics block is present and +// enabled — the single gate ServeHTTP checks before matching the metrics +// route at all, mirroring adminEnabled (admin.go). +func metricsEnabled(cfg *Config) bool { + return cfg.Metrics != nil && cfg.Metrics.Enabled +} + +// metricsPath returns the path the metrics route is served at: cfg. +// Metrics.Path when set, metricsPathDefault otherwise. Only meaningful +// when metricsEnabled(cfg). +func metricsPath(cfg *Config) string { + if cfg.Metrics != nil && cfg.Metrics.Path != "" { + return cfg.Metrics.Path + } + return metricsPathDefault +} + +// parseMetricsCIDRs parses mc's AllowedCIDRs once, at construction, so +// the request path never re-parses a CIDR string per scrape. A malformed +// entry is a constructor error (validate-at-construction, matching +// LimitsConfig.validate's own convention, limits.go) rather than a +// silently-ignored allowlist entry an operator would only discover was +// never applied by testing it. +func parseMetricsCIDRs(mc *MetricsConfig) ([]*net.IPNet, error) { + if mc == nil || len(mc.AllowedCIDRs) == 0 { + return nil, nil + } + nets := make([]*net.IPNet, 0, len(mc.AllowedCIDRs)) + for _, raw := range mc.AllowedCIDRs { + _, ipNet, err := net.ParseCIDR(raw) + if err != nil { + return nil, fmt.Errorf("llmgateway: metrics: invalid allowedCIDRs entry %q: %w", raw, err) + } + nets = append(nets, ipNet) + } + return nets, nil +} + +// handleMetrics implements GET : the Prometheus text- +// exposition endpoint. Gate order mirrors handleAdminAPI (admin.go) +// exactly — reusing the SAME admin bearer-token check, never a second +// credential path — with one addition checked FIRST: a source address +// inside Config.Metrics.AllowedCIDRs skips the key requirement entirely, +// so a co-located Prometheus server needs no credential. Checking the +// CIDR allowlist before ever looking at a presented key also means a +// keyless, allowlisted scrape never touches authStore's failed-attempt +// tracking (auth.go's identify records a failure on every call with no +// valid key) — an allowlisted scraper polling every 15s must not look +// like a sustained attack against itself. +func (g *Gateway) handleMetrics(sw *statusTrackingWriter, r *http.Request) { + if g.metricsSourceAllowed(r) { + g.serveMetrics(sw) + return + } + 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") + return + } + if !u.admin { + writeOAIError(sw, http.StatusForbidden, "invalid_request_error", "admin access required") + return + } + g.serveMetrics(sw) +} + +// metricsSourceAllowed reports whether r's source address (clientIP, +// auth.go) falls inside any of g.metricsNets, parsed once at +// construction from Config.Metrics.AllowedCIDRs (parseMetricsCIDRs). +// Always false when no CIDRs are configured. +func (g *Gateway) metricsSourceAllowed(r *http.Request) bool { + if len(g.metricsNets) == 0 { + return false + } + ip := net.ParseIP(clientIP(r)) + if ip == nil { + return false + } + for _, n := range g.metricsNets { + if n.Contains(ip) { + return true + } + } + return false +} + +// serveMetrics writes renderMetrics' result with the Prometheus text- +// exposition Content-Type. X-Content-Type-Options/Cache-Control mirror +// setAdminJSONHeaders' own reasoning (admin.go): this response is built +// fresh per request from live state and must never linger in a shared or +// disk cache, and its declared Content-Type must never be second-guessed +// by a client's MIME sniffer. +func (g *Gateway) serveMetrics(w http.ResponseWriter) { + w.Header().Set("Content-Type", "text/plain; version=0.0.4; charset=utf-8") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Cache-Control", "no-store") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(g.renderMetrics()) // headers already committed; nothing useful to do on write failure +} + +// escapeLabelValue escapes s for use as a Prometheus label value inside +// double quotes in the text exposition format: backslash becomes \\, +// double quote becomes \", and newline becomes \n — the exact rule the +// format requires. An unescaped quote or embedded newline in a label +// value (a user or group name, a model id, an operator picks these) would +// otherwise emit an unparseable exposition document — and Prometheus +// reports an unparseable scrape as the WHOLE target being DOWN, not as +// one bad line, so this is a correctness requirement, not a formatting +// nicety. +// +// This walks s rune by rune and classifies each one independently, never +// a sequential find-and-replace pass: replacing quotes first and +// backslashes second (or vice versa) would double-escape a backslash +// this function itself just inserted — a real, classic bug class for +// this exact kind of escaping. A single forward pass has no such +// ordering hazard. +func escapeLabelValue(s string) string { + var b bytes.Buffer + b.Grow(len(s)) + for _, r := range s { + switch r { + case '\\': + b.WriteString(`\\`) + case '"': + b.WriteString(`\"`) + case '\n': + b.WriteString(`\n`) + default: + b.WriteRune(r) + } + } + return b.String() +} + +// metricLabel is one label name/value pair a metricWriter sample carries. +type metricLabel struct { + name string + value string +} + +// metricWriter accumulates one Prometheus text-exposition document. +// Every value written through it is escaped via escapeLabelValue — no +// call site below is responsible for escaping its own label values. +type metricWriter struct { + buf bytes.Buffer +} + +// family writes name's HELP and TYPE lines. Every metric name this file +// emits is a package-level string literal (never derived from request or +// config input), so no runtime validation of the name character set +// ([a-zA-Z_:][a-zA-Z0-9_:]*) is needed here — the literals below are +// already compliant. +func (m *metricWriter) family(name, kind, help string) { + m.buf.WriteString("# HELP ") + m.buf.WriteString(name) + m.buf.WriteByte(' ') + m.buf.WriteString(help) + m.buf.WriteByte('\n') + m.buf.WriteString("# TYPE ") + m.buf.WriteString(name) + m.buf.WriteByte(' ') + m.buf.WriteString(kind) + m.buf.WriteByte('\n') +} + +// sample writes one series: name{label="value",...} value\n. Every label +// value is escaped via escapeLabelValue. +func (m *metricWriter) sample(name string, labels []metricLabel, value string) { + m.buf.WriteString(name) + if len(labels) > 0 { + m.buf.WriteByte('{') + for i, l := range labels { + if i > 0 { + m.buf.WriteByte(',') + } + m.buf.WriteString(l.name) + m.buf.WriteString(`="`) + m.buf.WriteString(escapeLabelValue(l.value)) + m.buf.WriteByte('"') + } + m.buf.WriteByte('}') + } + m.buf.WriteByte(' ') + m.buf.WriteString(value) + m.buf.WriteByte('\n') +} + +// sampleInt writes one integer-valued series (every counter and the +// provider-healthy gauge below). +func (m *metricWriter) sampleInt(name string, labels []metricLabel, v int64) { + m.sample(name, labels, strconv.FormatInt(v, 10)) +} + +// sampleFloat writes one float-valued series (the budget-consumed-ratio +// gauge below, which is rarely a whole number). +func (m *metricWriter) sampleFloat(name string, labels []metricLabel, v float64) { + m.sample(name, labels, strconv.FormatFloat(v, 'g', -1, 64)) +} + +// renderMetrics builds the full Prometheus text-exposition document for +// GET . Every read below goes through the SAME batched, +// chunked accessors GET /admin/api/overview and GET /admin/api/usage +// already use (admin.go's buildAdminOverview/chunkedCurrentUsage) — no +// per-series store round trip, and no second counting path: every number +// here is read from accounting limits.go already maintains, with one +// exception (llmgateway_rate_limit_rejections_total) that has no +// existing counter to read at all — see limiter.rejections' own doc +// comment (limits.go) for why that one is a new, deliberately minimal, +// in-process-only counter rather than a store round trip. +func (g *Gateway) renderMetrics() []byte { + var m metricWriter + g.writeUsageMetrics(&m) + g.writeProviderMetrics(&m) + g.writeRejectionMetrics(&m) + return m.buf.Bytes() +} + +// writeUsageMetrics emits llmgateway_requests_total, llmgateway_tokens_total, +// llmgateway_cost_micro_usd_total, and llmgateway_budget_consumed_ratio — +// one batched, chunked limiter.currentUsage read (via chunkedCurrentUsage, +// admin.go) covering every active user, every configured group, and the +// synthetic total scope, exactly mirroring buildAdminUsage's own scope +// construction so this pays no additional store round trip beyond what +// GET /admin/api/usage already pays. +// +// A scope whose read failed closed (scopeUsage.storeDown) is skipped +// entirely for every counter/gauge in this function, rather than reported +// as a false zero — the same "must not present as confirmed zero usage" +// rule scopeUsage's own doc comment states (limits.go). Prometheus treats +// a temporarily missing series as a gap, which is the honest signal here; +// a fabricated zero would not be. +func (g *Gateway) writeUsageMetrics(m *metricWriter) { + userSummaries, groupSummaries := g.auth.snapshot() + + scopes := make([]limitScope, 0, len(userSummaries)+len(groupSummaries)+1) + for _, us := range userSummaries { + scopes = append(scopes, limitScope{kind: "user", id: us.name, limits: us.limits}) + } + for _, gs := range groupSummaries { + scopes = append(scopes, limitScope{kind: "group", id: gs.name, limits: gs.limits}) + } + scopes = append(scopes, limitScope{kind: totalScopeKind, id: totalScopeID, limits: nil}) + + usage := g.chunkedCurrentUsage(scopes) + + m.family("llmgateway_requests_total", "counter", + "Total requests admitted for accounting today (UTC calendar day; resets at UTC midnight), by scope.") + for _, su := range usage { + if su.storeDown { + continue + } + m.sampleInt("llmgateway_requests_total", + []metricLabel{{"scope_kind", su.kind}, {"scope_id", su.id}}, su.requestsPerDay) + } + + m.family("llmgateway_tokens_total", "counter", + "Total tokens accounted today (UTC calendar day; resets at UTC midnight), by scope and direction.") + for _, su := range usage { + if su.storeDown { + continue + } + m.sampleInt("llmgateway_tokens_total", + []metricLabel{{"scope_kind", su.kind}, {"scope_id", su.id}, {"direction", "prompt"}}, su.tokensInPerDay) + m.sampleInt("llmgateway_tokens_total", + []metricLabel{{"scope_kind", su.kind}, {"scope_id", su.id}, {"direction", "completion"}}, su.tokensOutPerDay) + } + + m.family("llmgateway_cost_micro_usd_total", "counter", + "Total cost accounted today (UTC calendar day; resets at UTC midnight), in micro-USD (1000000 = $1), by scope.") + for _, su := range usage { + if su.storeDown { + continue + } + m.sampleInt("llmgateway_cost_micro_usd_total", + []metricLabel{{"scope_kind", su.kind}, {"scope_id", su.id}}, su.costPerDayMicros) + } + + m.family("llmgateway_budget_consumed_ratio", "gauge", + "Fraction of a configured limit already consumed in its own window; 1.0 means fully consumed, above 1.0 means the limit has been breached. Emitted only for a scope with that particular limit configured.") + for i, su := range usage { + if su.storeDown { + continue + } + for _, br := range budgetRatios(su, scopes[i].limits) { + m.sampleFloat("llmgateway_budget_consumed_ratio", + []metricLabel{{"scope_kind", su.kind}, {"scope_id", su.id}, {"budget", br.name}}, + float64(br.used)/float64(br.limit)) + } + } +} + +// budgetRatioSpec is one configured limit's usage-vs-limit pair, +// budgetRatios' own output — name matches requestLimitViolation's and +// buildBudgetProbes' own name strings exactly (limits.go), so the label +// value an operator sees here is the identical vocabulary a 429's own +// error message already uses. +type budgetRatioSpec struct { + name string + used int64 + limit int64 +} + +// budgetRatios returns one budgetRatioSpec per limit lc has actually +// configured (limit<=0 means unlimited and is skipped — the same +// short-circuit buildBudgetProbes applies, limits.go), pairing each +// against su's own current usage. lc nil (no limits configured for this +// scope, e.g. the synthetic total scope) returns nil. tokens-per-day/ +// -month sum tokensIn+tokensOut, matching checkAndCount's own probe loop +// — a TokensPerDay/Month limit enforces a combined budget across both +// directions, not either alone. +func budgetRatios(su scopeUsage, lc *LimitsConfig) []budgetRatioSpec { + if lc == nil { + return nil + } + var out []budgetRatioSpec + add := func(name string, used, limit int64) { + if limit <= 0 { + return + } + out = append(out, budgetRatioSpec{name: name, used: used, limit: limit}) + } + add("requests-per-minute", su.requestsPerMinute, lc.RequestsPerMinute) + add("requests-per-day", su.requestsPerDay, lc.RequestsPerDay) + add("tokens-per-day", su.tokensInPerDay+su.tokensOutPerDay, lc.TokensPerDay) + add("tokens-per-month", su.tokensInPerMonth+su.tokensOutPerMonth, lc.TokensPerMonth) + add("cost-per-day", su.costPerDayMicros, usdToMicros(lc.CostPerDayUSD)) + add("cost-per-month", su.costPerMonthMicros, usdToMicros(lc.CostPerMonthUSD)) + return out +} + +// writeProviderMetrics emits llmgateway_provider_attempts_total, +// llmgateway_provider_failures_total, and llmgateway_provider_healthy — +// always, for every configured provider — plus +// llmgateway_provider_model_attempts_total/-_failures_total, only when +// Config.Metrics.ModelLabel opts into the higher-cardinality per-model +// breakdown. Provider-level and (opt-in) per-model counters ride the SAME +// batched limiter.providerUsage round trip buildAdminOverview already +// pays (admin.go) — this adds no additional store call, on or off. +func (g *Gateway) writeProviderMetrics(m *metricWriter) { + snaps := g.registry.snapshot() + + includeModel := g.cfg.Metrics != nil && g.cfg.Metrics.ModelLabel + + scopes := make([]limitScope, 0, len(snaps)) + for _, s := range snaps { + scopes = append(scopes, limitScope{kind: kindProvider, id: s.name}) + } + if includeModel { + for _, s := range snaps { + for _, model := range s.models { + scopes = append(scopes, limitScope{kind: kindProviderModel, id: providerModelScopeID(s.name, model)}) + } + } + } + counters := g.limiter.providerUsage(scopes) + providerCounts := counters[:len(snaps)] + + m.family("llmgateway_provider_attempts_total", "counter", + "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider.") + m.family("llmgateway_provider_failures_total", "counter", + "Total upstream attempts today classified as a provider fault (transport error, 429, or 5xx — the same classification recordProviderAttempt/isTransient apply), by provider.") + for i, s := range snaps { + labels := []metricLabel{{"provider", s.name}} + m.sampleInt("llmgateway_provider_attempts_total", labels, providerCounts[i].attemptsDay) + m.sampleInt("llmgateway_provider_failures_total", labels, providerCounts[i].failuresDay) + } + + m.family("llmgateway_provider_healthy", "gauge", + "1 when the provider's discovery circuit breaker is not open (closed or half-open), 0 when open (discoveryHealthy, registry.go).") + for _, s := range snaps { + healthy := int64(1) + if s.health == breakerOpen { + healthy = 0 + } + m.sampleInt("llmgateway_provider_healthy", []metricLabel{{"provider", s.name}}, healthy) + } + + if !includeModel { + return + } + modelCounts := counters[len(snaps):] + + m.family("llmgateway_provider_model_attempts_total", "counter", + "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider and model. Opt-in via metrics.modelLabel — see MetricsConfig's own doc comment for the cardinality trade-off.") + m.family("llmgateway_provider_model_failures_total", "counter", + "Total upstream attempts today classified as a provider fault, by provider and model. Opt-in via metrics.modelLabel.") + mi := 0 + for _, s := range snaps { + for _, model := range s.models { + mc := modelCounts[mi] + mi++ + labels := []metricLabel{{"provider", s.name}, {"model", model}} + m.sampleInt("llmgateway_provider_model_attempts_total", labels, mc.attemptsDay) + m.sampleInt("llmgateway_provider_model_failures_total", labels, mc.failuresDay) + } + } +} + +// writeRejectionMetrics emits llmgateway_rate_limit_rejections_total from +// limiter.rejections (limits.go) — the one metric family in this file +// with no existing counterStore-backed accounting to read: checkAndCount +// increments/reads req:min/req:day for EVERY admission attempt regardless +// of outcome, so "how many were actually rejected" is not otherwise +// derivable without a second, redundant read against the store. See +// limiter.rejections' own doc comment for why this is a cheap, in-process +// -only, per-process-lifetime counter rather than a new windowed +// counterStore key. +func (g *Gateway) writeRejectionMetrics(m *metricWriter) { + snaps := g.limiter.rejectionSnapshot() + sort.Slice(snaps, func(i, j int) bool { + if snaps[i].kind != snaps[j].kind { + return snaps[i].kind < snaps[j].kind + } + return snaps[i].id < snaps[j].id + }) + + m.family("llmgateway_rate_limit_rejections_total", "counter", + "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed — it resets only on restart.") + for _, s := range snaps { + m.sampleInt("llmgateway_rate_limit_rejections_total", + []metricLabel{{"scope_kind", s.kind}, {"scope_id", s.id}}, s.count) + } +} diff --git a/metrics_test.go b/metrics_test.go new file mode 100644 index 0000000..b78f0ed --- /dev/null +++ b/metrics_test.go @@ -0,0 +1,774 @@ +package traefikllmgateway + +import ( + "context" + "net/http" + "net/http/httptest" + "sort" + "strconv" + "strings" + "testing" + "time" +) + +// newMetricsTestConfig builds a Config with Metrics enabled at the default +// path, one provider, one group ("g"), a non-admin user ("alice") and an +// admin user ("admin1") — the minimal shape every gate/structure test +// below needs. Callers may mutate the returned Config (in particular +// cfg.Metrics) before calling New. +func newMetricsTestConfig() *Config { + cfg := CreateConfig() + cfg.Metrics = &MetricsConfig{Enabled: true} + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: "http://openai.invalid", APIKey: "sk-up", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{"g": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "alice", Group: "g", APIKey: "sk-alice", Limits: &LimitsConfig{}}, + {Name: "admin1", Group: "g", APIKey: "sk-admin1", Admin: true}, + }} + return cfg +} + +// newMetricsGatewayHandle mirrors newAdminGatewayHandle (admin_test.go): +// builds the Gateway from cfg and returns both the http.Handler ServeHTTP +// uses and the concrete *Gateway, with recordProviderAttempt's store +// write forced synchronous so a test asserting on a resulting provider +// counter never races the production fire-and-forget spawn (see +// newAdminGatewayHandle's own doc comment for why). +func newMetricsGatewayHandle(t *testing.T, cfg *Config) (http.Handler, *Gateway) { + t.Helper() + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusTeapot) }) + 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") + } + gw.limiter.spawn = func(f func()) { f() } + return h, gw +} + +func metricsRequest(method, path, apiKey string) *http.Request { + req := httptest.NewRequest(method, path, nil) + if apiKey != "" { + req.Header.Set("Authorization", "Bearer "+apiKey) + } + return req +} + +// --- gate: disabled/custom-path/unauth/non-admin/admin/CIDR --- + +func TestMetrics_Disabled_FallsThroughTo404(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics = nil + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (metrics nil)", rec.Code) + } +} + +func TestMetrics_DisabledExplicitly_FallsThroughTo404(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics = &MetricsConfig{Enabled: false} + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (metrics disabled, even with a valid admin key)", rec.Code) + } +} + +func TestMetrics_CustomPath(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics = &MetricsConfig{Enabled: true, Path: "/internal/metrics"} + h, _ := newMetricsGatewayHandle(t, cfg) + + // The default path is no longer registered once Path is set. + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusNotFound { + t.Fatalf("default path status = %d, want 404 (Path overrides it)", rec.Code) + } + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, "/internal/metrics", "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("custom path status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } +} + +// TestMetrics_GateMatrix covers every auth outcome the brief calls out by +// name: unauthenticated is rejected, a non-admin key is rejected, an +// admin key is accepted, a source address inside AllowedCIDRs is accepted +// with no key at all, and a source address outside it is still rejected +// with no key. +func TestMetrics_GateMatrix(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + // httptest.NewRequest's default RemoteAddr is 192.0.2.1:1234 (verified + // empirically, not assumed) — 192.0.2.0/24 is the allowlisted block + // below, so a request built via metricsRequest with no explicit + // RemoteAddr override lands inside it. + cfg.Metrics.AllowedCIDRs = []string{"192.0.2.0/24"} + h, _ := newMetricsGatewayHandle(t, cfg) + + t.Run("unauthenticated_non_allowlisted", func(t *testing.T) { + req := metricsRequest(http.MethodGet, metricsPathDefault, "") + req.RemoteAddr = "203.0.113.9:5555" // TEST-NET-3, outside 192.0.2.0/24 + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401", rec.Code) + } + }) + t.Run("authenticated_non_admin", func(t *testing.T) { + req := metricsRequest(http.MethodGet, metricsPathDefault, "sk-alice") + req.RemoteAddr = "203.0.113.9:5555" // outside the allowlist too — the key path must still gate on admin + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403", rec.Code) + } + }) + t.Run("admin_key_from_non_allowlisted_address", func(t *testing.T) { + req := metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1") + req.RemoteAddr = "203.0.113.9:5555" + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (admin bearer token always works), body=%s", rec.Code, rec.Body.String()) + } + }) + t.Run("allowlisted_cidr_no_key", func(t *testing.T) { + req := metricsRequest(http.MethodGet, metricsPathDefault, "") // default RemoteAddr, inside 192.0.2.0/24 + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (source address inside AllowedCIDRs), body=%s", rec.Code, rec.Body.String()) + } + }) +} + +// TestMetrics_NoAllowlistConfigured_KeylessRequestRejected proves the +// CIDR bypass is opt-in: with no AllowedCIDRs at all, even the default +// httptest RemoteAddr (which the test above proves IS matched once +// configured) gets 401 without a key. +func TestMetrics_NoAllowlistConfigured_KeylessRequestRejected(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() // no AllowedCIDRs + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "")) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 (no allowlist configured)", rec.Code) + } +} + +func TestMetrics_InvalidCIDR_FailsConstruction(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics.AllowedCIDRs = []string{"not-a-cidr"} + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + _, err := New(context.Background(), next, cfg, "llmgw") + if err == nil { + t.Fatal("New: want an error for a malformed allowedCIDRs entry, got nil") + } +} + +// --- structure: valid exposition format --- + +// promSample is one parsed exposition-format sample line. +type promSample struct { + name string + labels map[string]string + value string +} + +// parsePrometheusText parses body as a Prometheus text-exposition +// document, failing the test on any structural violation. Label values +// are unescaped char-by-char, honoring \\, \", and \n exactly as the +// format (and escapeLabelValue, metrics.go) define them — a real, +// unescaped quote or embedded newline in a label value would desync this +// parser (an unterminated quote, or a value silently truncated where the +// injected control character landed), which is exactly the failure mode +// this test suite exists to catch. +func parsePrometheusText(t *testing.T, body []byte) (types map[string]string, samples []promSample) { + t.Helper() + types = map[string]string{} + for _, line := range strings.Split(string(body), "\n") { + if line == "" { + continue + } + if rest, ok := strings.CutPrefix(line, "# TYPE "); ok { + name, kind, ok := strings.Cut(rest, " ") + if !ok { + t.Fatalf("malformed TYPE line: %q", line) + } + types[name] = kind + continue + } + if strings.HasPrefix(line, "#") { + continue + } + samples = append(samples, parsePromSample(t, line)) + } + return types, samples +} + +func parsePromSample(t *testing.T, line string) promSample { + t.Helper() + i := 0 + for i < len(line) && line[i] != '{' && line[i] != ' ' { + i++ + } + name := line[:i] + if name == "" { + t.Fatalf("empty metric name in line: %q", line) + } + labels := map[string]string{} + if i < len(line) && line[i] == '{' { + i++ + for { + if i >= len(line) { + t.Fatalf("unterminated label block in line: %q", line) + } + if line[i] == '}' { + i++ + break + } + lnStart := i + for i < len(line) && line[i] != '=' { + i++ + } + if i >= len(line) { + t.Fatalf("malformed label (no '=') in line: %q", line) + } + lname := line[lnStart:i] + i++ // skip '=' + if i >= len(line) || line[i] != '"' { + t.Fatalf("malformed label value (no opening quote) in line: %q", line) + } + i++ // skip opening quote + var val strings.Builder + for { + if i >= len(line) { + t.Fatalf("unterminated label value in line: %q", line) + } + c := line[i] + if c == '\\' { + i++ + if i >= len(line) { + t.Fatalf("dangling escape in line: %q", line) + } + switch line[i] { + case '\\': + val.WriteByte('\\') + case '"': + val.WriteByte('"') + case 'n': + val.WriteByte('\n') + default: + t.Fatalf("unknown escape \\%c in line: %q", line[i], line) + } + i++ + continue + } + if c == '"' { + i++ + break + } + val.WriteByte(c) + i++ + } + labels[lname] = val.String() + if i < len(line) && line[i] == ',' { + i++ + continue + } + if i < len(line) && line[i] == '}' { + i++ + break + } + t.Fatalf("malformed label separator in line: %q", line) + } + } + for i < len(line) && line[i] == ' ' { + i++ + } + value := line[i:] + if value == "" { + t.Fatalf("missing value in line: %q", line) + } + if _, err := strconv.ParseFloat(value, 64); err != nil { + t.Fatalf("sample value %q does not parse as a number in line: %q", value, line) + } + return promSample{name: name, labels: labels, value: value} +} + +// metricNameCharset is the character set a Prometheus metric name must +// match: [a-zA-Z_:][a-zA-Z0-9_:]*. +func validMetricName(name string) bool { + if name == "" { + return false + } + for i, r := range name { + switch { + case r == '_' || r == ':': + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z': + case r >= '0' && r <= '9' && i > 0: + default: + return false + } + } + return true +} + +// TestMetrics_OutputParsesAsValidExposition seeds real traffic (a +// successful request, a rejected one, and an opted-in model breakdown) +// so every metric family below actually has at least one sample line, +// then asserts structure rather than a golden blob: every metric name is +// well-formed, every family has a # TYPE line, every counter's name ends +// "_total", and no (name, label set) combination repeats. +func TestMetrics_OutputParsesAsValidExposition(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":"c1","object":"chat.completion","model":"gpt-test","choices":[{"index":0,"message":{"role":"assistant","content":"hi"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}`)) + })) + defer srv.Close() + + cfg := newMetricsTestConfig() + cfg.Metrics.ModelLabel = true + cfg.Providers["openai"].BaseURL = srv.URL + cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerMinute: 1} + h, _ := newMetricsGatewayHandle(t, cfg) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + // First request: admitted, and succeeds against the local httptest + // server — seeds request/token/cost/provider counters via + // checkAndCount+account+recordProviderAttempt. + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + if rec.Code != http.StatusOK { + t.Fatalf("first request status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + // Second request: alice's RequestsPerMinute:1 is now exhausted, so + // this one is rejected — seeding llmgateway_rate_limit_rejections_total. + rec = httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("second request status = %d, want 429 (RequestsPerMinute:1 exhausted)", rec.Code) + } + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("GET /metrics status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "text/plain") { + t.Errorf("Content-Type = %q, want text/plain prefix", ct) + } + + types, samples := parsePrometheusText(t, rec.Body.Bytes()) + if len(samples) == 0 { + t.Fatal("no samples parsed at all") + } + + seen := map[string]bool{} + for _, s := range samples { + if !validMetricName(s.name) { + t.Errorf("metric name %q does not match [a-zA-Z_:][a-zA-Z0-9_:]*", s.name) + } + kind, ok := types[s.name] + if !ok { + t.Errorf("sample %q has no # TYPE line", s.name) + } + if kind == "counter" && !strings.HasSuffix(s.name, "_total") { + t.Errorf("counter %q does not end in _total", s.name) + } + + labelKeys := make([]string, 0, len(s.labels)) + for k := range s.labels { + labelKeys = append(labelKeys, k) + } + sort.Strings(labelKeys) + var key strings.Builder + key.WriteString(s.name) + for _, k := range labelKeys { + key.WriteByte('\x00') + key.WriteString(k) + key.WriteByte('=') + key.WriteString(s.labels[k]) + } + if seen[key.String()] { + t.Errorf("duplicate series: %s%v", s.name, s.labels) + } + seen[key.String()] = true + } + + wantFamilies := []string{ + "llmgateway_requests_total", + "llmgateway_tokens_total", + "llmgateway_cost_micro_usd_total", + "llmgateway_budget_consumed_ratio", + "llmgateway_provider_attempts_total", + "llmgateway_provider_failures_total", + "llmgateway_provider_healthy", + "llmgateway_provider_model_attempts_total", + "llmgateway_provider_model_failures_total", + "llmgateway_rate_limit_rejections_total", + } + for _, name := range wantFamilies { + if _, ok := types[name]; !ok { + t.Errorf("expected family %q to have a # TYPE line, got none", name) + } + } +} + +// --- label escaping: the correctness requirement, not a nicety --- + +func TestEscapeLabelValue(t *testing.T) { + t.Parallel() + tests := []struct { + name string + in string + want string + }{ + {"plain", "gpt-test", "gpt-test"}, + {"quote", `say "hi"`, `say \"hi\"`}, + {"backslash", `C:\models\x`, `C:\\models\\x`}, + {"newline", "line1\nline2", `line1\nline2`}, + {"quote_then_backslash", `"\`, `\"\\`}, + {"backslash_then_quote", `\"`, `\\\"`}, + {"all_three", "a\"b\\c\nd", `a\"b\\c\nd`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := escapeLabelValue(tt.in); got != tt.want { + t.Errorf("escapeLabelValue(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestMetrics_LabelValuesWithSpecialChars_Escaped is the end-to-end +// version of TestEscapeLabelValue: a real user name containing a double +// quote, a backslash, and a newline (auth.go's buildEntry doc comment +// confirms a user name is validated as non-empty only, never restricted +// to configNamePattern's URL-path-segment character set) flows all the +// way into the scope_id label of a real rendered document, and the +// document still parses. +func TestMetrics_LabelValuesWithSpecialChars_Escaped(t *testing.T) { + t.Parallel() + const trickyName = `weird"user\name` + "\nwith-newline" + cfg := newMetricsTestConfig() + cfg.Users.Inline = append(cfg.Users.Inline, &UserConfig{Name: trickyName, Group: "g", APIKey: "sk-tricky"}) + h, gw := newMetricsGatewayHandle(t, cfg) + + fixedNow := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + gw.limiter.nowFn = func() time.Time { return fixedNow } + gw.limiter.incrCounter("user", trickyName, metricReq, windowDay, fixedNow, 3, dayWindowTTL) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + var found *promSample + for i, s := range samples { + if s.name == "llmgateway_requests_total" && s.labels["scope_id"] == trickyName { + found = &samples[i] + break + } + } + if found == nil { + t.Fatalf("no llmgateway_requests_total sample with scope_id == %q found among %d samples", trickyName, len(samples)) + } + if found.value != "3" { + t.Errorf("value = %q, want 3", found.value) + } + // The raw bytes must never contain an unescaped copy of the tricky + // name — only its escaped form — confirming this genuinely exercised + // the escaping path rather than the name having been dropped/replaced. + if strings.Contains(rec.Body.String(), trickyName) { + t.Error("response body contains the raw, unescaped name — escaping did not run") + } +} + +// --- model label: opt-in --- + +func TestMetrics_ModelLabel_AbsentByDefault(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + body := rec.Body.String() + if strings.Contains(body, "llmgateway_provider_model_attempts_total") { + t.Error("llmgateway_provider_model_attempts_total present with ModelLabel unset (default off)") + } + if strings.Contains(body, "llmgateway_provider_model_failures_total") { + t.Error("llmgateway_provider_model_failures_total present with ModelLabel unset (default off)") + } +} + +func TestMetrics_ModelLabel_PresentWhenEnabled(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics.ModelLabel = true + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + var found bool + for _, s := range samples { + if s.name == "llmgateway_provider_model_attempts_total" && s.labels["provider"] == "openai" && s.labels["model"] == "gpt-test" { + found = true + break + } + } + if !found { + t.Fatalf("no llmgateway_provider_model_attempts_total{provider=%q,model=%q} sample found", "openai", "gpt-test") + } +} + +// --- counters reflect actual recorded usage --- + +func TestMetrics_RequestTokenCostCounters_ReflectSeededUsage(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + fixedNow := time.Date(2026, 8, 20, 12, 30, 0, 0, time.UTC) + gw.limiter.nowFn = func() time.Time { return fixedNow } + gw.limiter.incrCounter("user", "alice", metricReq, windowDay, fixedNow, 7, dayWindowTTL) + gw.limiter.account([]limitScope{{kind: "user", id: "alice"}}, usage{prompt: 40, completion: 10}, 2_500_000) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + + assertSample(t, samples, "llmgateway_requests_total", map[string]string{"scope_kind": "user", "scope_id": "alice"}, "7") + assertSample(t, samples, "llmgateway_tokens_total", map[string]string{"scope_kind": "user", "scope_id": "alice", "direction": "prompt"}, "40") + assertSample(t, samples, "llmgateway_tokens_total", map[string]string{"scope_kind": "user", "scope_id": "alice", "direction": "completion"}, "10") + assertSample(t, samples, "llmgateway_cost_micro_usd_total", map[string]string{"scope_kind": "user", "scope_id": "alice"}, "2.5e+06") +} + +// assertSample fails the test unless samples contains exactly one sample +// named name whose labels are exactly want (same keys, same values), and +// whose value round-trips (as a float64) to the same value wantValue +// does. Comparing as float64 rather than as a raw string tolerates +// strconv.FormatFloat's shortest-representation choice (e.g. "2.5e+06" +// for 2,500,000) without the test hardcoding that exact spelling. +func assertSample(t *testing.T, samples []promSample, name string, want map[string]string, wantValue string) { + t.Helper() + wantF, err := strconv.ParseFloat(wantValue, 64) + if err != nil { + t.Fatalf("assertSample: wantValue %q does not parse: %v", wantValue, err) + } + for _, s := range samples { + if s.name != name || len(s.labels) != len(want) { + continue + } + match := true + for k, v := range want { + if s.labels[k] != v { + match = false + break + } + } + if !match { + continue + } + gotF, err := strconv.ParseFloat(s.value, 64) + if err != nil { + t.Fatalf("sample %s%v value %q does not parse: %v", name, s.labels, s.value, err) + } + if gotF != wantF { + t.Errorf("sample %s%v value = %q, want %s", name, s.labels, s.value, wantValue) + } + return + } + t.Errorf("no sample %s with labels %v found", name, want) +} + +// TestMetrics_ProviderCounters_ReflectRealTraffic drives one successful +// and one failing upstream call through the real unified route (not a +// direct counter seed) and asserts llmgateway_provider_attempts_total/ +// _failures_total match — the same shape TestAdminOverview_ +// ProviderRates_EndToEndAfterTraffic (admin_test.go) already proves for +// the JSON admin API, proving the identical underlying providerUsage read +// renders correctly here too. +func TestMetrics_ProviderCounters_ReflectRealTraffic(t *testing.T) { + t.Parallel() + var callN int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callN++ + if callN == 1 { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":"c1","object":"chat.completion","model":"gpt-test","choices":[{"index":0,"message":{"role":"assistant","content":"hi"},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":1}}`)) + return + } + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"boom"}`)) + })) + defer srv.Close() + + cfg := newMetricsTestConfig() + cfg.Providers["openai"].BaseURL = srv.URL + h, _ := newMetricsGatewayHandle(t, cfg) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + for i := 0; i < 2; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + + assertSample(t, samples, "llmgateway_provider_attempts_total", map[string]string{"provider": "openai"}, "2") + assertSample(t, samples, "llmgateway_provider_failures_total", map[string]string{"provider": "openai"}, "1") +} + +// TestMetrics_ProviderHealthy_ReflectsOpenBreaker proves the gauge +// follows the discovery circuit breaker's real state, not a static 1 — +// mutating registry.states directly (same package, mirrors how +// registry_health_test.go inspects breaker state) rather than driving a +// real failing discovery loop, since this test only needs to prove the +// RENDERING reads health correctly, not re-prove the breaker state +// machine itself (registry_health_test.go already does that). +func TestMetrics_ProviderHealthy_ReflectsOpenBreaker(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + st := gw.registry.states["openai"] + st.mu.Lock() + st.health = breakerOpen + st.mu.Unlock() + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + assertSample(t, samples, "llmgateway_provider_healthy", map[string]string{"provider": "openai"}, "0") +} + +// --- rate-limit rejections --- + +// TestMetrics_RateLimitRejections_CountedByScope drives a real 429 +// (alice's RequestsPerMinute:1 exhausted by a second request) through the +// unified route, then asserts llmgateway_rate_limit_rejections_total +// carries exactly 1 for {scope_kind="user",scope_id="alice"} — the +// SAME scope checkAndCount's own violation message names. +func TestMetrics_RateLimitRejections_CountedByScope(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerMinute: 1} + h, _ := newMetricsGatewayHandle(t, cfg) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + rec = httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("status = %d, want 429", rec.Code) + } + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + assertSample(t, samples, "llmgateway_rate_limit_rejections_total", map[string]string{"scope_kind": "user", "scope_id": "alice"}, "1") +} + +// TestLimiter_RejectionSnapshot_StoreDown proves checkAndCount records a +// store_down rejection (distinct from a real limit breach) when the +// configured store errors and failOpen is false — driven directly +// against a *limiter (not through Gateway/HTTP), mirroring +// TestLimiterCurrentUsage_StoreDown's own direct-limiter style +// (admin_test.go). +func TestLimiter_RejectionSnapshot_StoreDown(t *testing.T) { + t.Parallel() + l := newLimiter(alwaysErrStore{}, false) + scopes := []limitScope{{kind: "user", id: "x", limits: &LimitsConfig{RequestsPerMinute: 10}}} + + v := l.checkAndCount(scopes) + if v == nil || !v.storeDown { + t.Fatalf("checkAndCount = %+v, want a storeDown violation", v) + } + + snaps := l.rejectionSnapshot() + var found bool + for _, s := range snaps { + if s.kind == rejectionScopeStoreDown && s.count == 1 { + found = true + } + } + if !found { + t.Fatalf("rejectionSnapshot = %+v, want one store_down entry with count 1", snaps) + } +} + +// --- budget-consumed ratio --- + +func TestMetrics_BudgetConsumedRatio_ReflectsUsageVsLimit(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerDay: 100} + h, gw := newMetricsGatewayHandle(t, cfg) + + fixedNow := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + gw.limiter.nowFn = func() time.Time { return fixedNow } + gw.limiter.incrCounter("user", "alice", metricReq, windowDay, fixedNow, 25, dayWindowTTL) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + + assertSample(t, samples, "llmgateway_budget_consumed_ratio", + map[string]string{"scope_kind": "user", "scope_id": "alice", "budget": "requests-per-day"}, "0.25") + + // The synthetic total scope carries no limit — no budget ratio sample + // of any kind should exist for it. + for _, s := range samples { + if s.name == "llmgateway_budget_consumed_ratio" && s.labels["scope_kind"] == totalScopeKind { + t.Errorf("unexpected budget ratio sample for the unlimited total scope: %+v", s) + } + } +} From d421f49ff1efe56afbd356c4d5c1a16ff3d0ec97 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:19:09 +0100 Subject: [PATCH 2/8] test(yaegi-check): verify the metrics endpoint under the interpreter Compiled tests have repeatedly passed on this codebase while the interpreted shape failed, so the metrics feature needs its own probe in this harness, not just go test coverage. Enable metrics in the harness config, with modelLabel on so the opt-in provider-model breakdown runs interpreted too. Add exerciseMetricsRoute, driving four real requests: unauthenticated from outside the allowed CIDR block returns 401, a non-admin key returns 403, the admin key returns 200 with the expected TYPE lines and live provider and model data in the body, and a keyless request from inside the allowed CIDR block returns 200. Run the new probe last, after the breaker probe settles, since GET /metrics triggers the same maybeRefresh call every route does and the breaker probe's own hit counting is timing-sensitive. --- tools/yaegi-check/main.go | 109 +++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index af663a2..0799a9d 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -370,6 +370,15 @@ func run() error { // one testDataWantModel already exercises. `"modelMeta":{"` + testDataWantModel + `":{"contextTokens":` + yaegiMetaContextTokens + `,"inputCostPerMTokMicroUsd":1250000,"outputCostPerMTokMicroUsd":10000000}},` + `"mcpServers":{"` + mcpProbeServerName + `":{"url":"` + mcpProbeUpstream.URL + `"}},` + + // metrics (Prometheus text-exposition endpoint): enabled with + // modelLabel on, so exerciseMetricsRoute below exercises the + // opt-in per-(provider,model) breakdown under the interpreter + // too, not just the always-on families. allowedCIDRs names the + // exact block httptest.NewRequest's own default RemoteAddr + // (192.0.2.1:1234, verified empirically) falls inside, so the + // CIDR-bypass path is reachable with a plain httptest.NewRequest + // carrying no explicit RemoteAddr override. + `"metrics":{"enabled":true,"allowedCIDRs":["` + metricsProbeAllowedCIDR + `"],"modelLabel":true},` + `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true}]}}` if err = json.Unmarshal([]byte(attemptAccountingOverride), cfgVal.Interface()); err != nil { return fmt.Errorf("decode attempt-accounting harness override into the interpreted Config: %w", err) @@ -418,7 +427,17 @@ func run() error { // exactly why this class of divergence needs its own interpreted // probe: a compiled `go test` pass here would prove nothing about the // interpreter. - return exerciseBreaker(handler, brkModelsHits, brkHealthy) + if err := exerciseBreaker(handler, brkModelsHits, brkHealthy); err != nil { + return err + } + // exerciseMetricsRoute runs LAST, after exerciseBreaker has already + // settled: GET /metrics runs registry.maybeRefresh like every other + // route (ServeHTTP's own unconditional call at entry), and + // exerciseBreaker's own hit-counting is timing-sensitive against the + // "brk" provider specifically — probing metrics first could perturb + // the very discovery-attempt counts that test samples. Ordering the + // metrics probe after avoids any such interaction. + return exerciseMetricsRoute(handler) } // builtinLookupModelID is a real, stable entry in the generated @@ -439,6 +458,16 @@ const ( mcpProbeToolName = mcpProbeServerName + "_big" ) +// metricsProbeAllowedCIDR is the CIDR block attemptAccountingOverride's +// own metrics.allowedCIDRs names — see that literal's own comment for why +// this must contain httptest.NewRequest's default RemoteAddr. +const metricsProbeAllowedCIDR = "192.0.2.0/24" + +// metricsProbeOutsideAddr is a source address deliberately OUTSIDE +// metricsProbeAllowedCIDR (TEST-NET-3, RFC 5737) — exerciseMetricsRoute's +// own proof that the allowlist is not simply matching everything. +const metricsProbeOutsideAddr = "203.0.113.9:5555" + // mcpProbeOversizeBytes is how much filler mcpProbeUpstream writes: over // mcpBackendCallResponseMaxBytes (maxRequestBytes, 10MiB) so the response // trips doBackendJSONRPC's cap. Kept as its own named value rather than @@ -625,6 +654,84 @@ func exerciseBreaker(handler http.Handler, hits, healthy *int64) error { return nil } +// exerciseMetricsRoute drives GET /metrics against the interpreted +// handler and proves both its auth gate and its rendering run correctly +// under Yaegi — the metrics feature's own gate requirement: compiled +// tests have repeatedly passed on this codebase while the interpreted +// shape failed (matchesSentinel's own doc comment, limits.go, is the +// canonical example), so a metrics-specific probe is not optional here. +// +// Four requests, mirroring TestMetrics_GateMatrix (metrics_test.go) +// exactly so this harness and the compiled test suite assert the +// identical contract from two different angles: +// 1. unauthenticated, from an address outside metrics.allowedCIDRs -> 401 +// 2. authenticated as "tester" (not admin) -> 403 +// 3. authenticated as the admin user -> 200, body inspected +// 4. unauthenticated, from an address INSIDE metrics.allowedCIDRs -> 200 +// +// Request 3's body is inspected for real, live-data content — not just a +// 200 status — because a Yaegi-specific bug in this codebase's history +// (matchesSentinel, limits.go) was a case where the interpreted code ran +// without panicking yet produced a WRONG answer; a bare status-code check +// would not have caught that class of failure here either. Specifically: +// the "openai" provider's name, and — since attemptAccountingOverride +// enables metrics.modelLabel — testDataWantModel's own id inside the +// opt-in provider-model breakdown, both of which can only appear via a +// live registry.snapshot() walk and a live providerModelScopeID join +// under the real interpreter, not a static string in this harness. +func exerciseMetricsRoute(handler http.Handler) error { + unauthReq := httptest.NewRequest(http.MethodGet, "/metrics", nil) + unauthReq.RemoteAddr = metricsProbeOutsideAddr + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, unauthReq) + if rec.Code != http.StatusUnauthorized { + return fmt.Errorf("GET /metrics (no key, outside allowedCIDRs): status = %d, want 401, body=%s", rec.Code, rec.Body.String()) + } + + nonAdminReq := httptest.NewRequest(http.MethodGet, "/metrics", nil) + nonAdminReq.RemoteAddr = metricsProbeOutsideAddr + nonAdminReq.Header.Set("Authorization", "Bearer "+testDataUserAPIKey) + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, nonAdminReq) + if rec.Code != http.StatusForbidden { + return fmt.Errorf("GET /metrics (non-admin key): status = %d, want 403, body=%s", rec.Code, rec.Body.String()) + } + + adminReq := httptest.NewRequest(http.MethodGet, "/metrics", nil) + adminReq.RemoteAddr = metricsProbeOutsideAddr + adminReq.Header.Set("Authorization", "Bearer "+attemptAccountingAdminAPIKey) + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, adminReq) + if rec.Code != http.StatusOK { + return fmt.Errorf("GET /metrics (admin key): status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "text/plain") { + return fmt.Errorf("GET /metrics (admin key): Content-Type = %q, want a text/plain prefix", ct) + } + body := rec.Body.String() + for _, want := range []string{ + "# TYPE llmgateway_requests_total counter", + "# TYPE llmgateway_provider_healthy gauge", + `llmgateway_provider_attempts_total{provider="openai"}`, + "# TYPE llmgateway_provider_model_attempts_total counter", + `llmgateway_provider_model_attempts_total{provider="openai",model="` + testDataWantModel + `"}`, + } { + if !strings.Contains(body, want) { + return fmt.Errorf("GET /metrics (admin key): body missing %q — interpreted rendering diverged from the compiled shape; body=%s", want, body) + } + } + + allowlistedReq := httptest.NewRequest(http.MethodGet, "/metrics", nil) // default RemoteAddr, inside metricsProbeAllowedCIDR + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, allowlistedReq) + if rec.Code != http.StatusOK { + return fmt.Errorf("GET /metrics (no key, inside allowedCIDRs): status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + fmt.Println("yaegi-check: GET /metrics auth gate and rendering both verified interpreted") + return nil +} + // exerciseHandler runs two real requests against the interpreted // handler, drawn from the plugin's own .traefik.yml testData: an // authenticated GET /v1/models, which must return the configured From 5700dd482dd01db78ef0b39b95e55b8769357a04 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:58:02 +0100 Subject: [PATCH 3/8] fix(limits): never fabricate zero counters, bound the rejection counter Two correctness fixes from adversarial verification. providerUsage returned every counter at its zero value on a failed-closed store read, with no signal distinguishing that from a real zero. metrics.go rendered the zero as a hard value. A single Redis blip therefore read as a counter reset, and the recovery scrape counted the whole prior total as a fresh increase, painting a phantom multi-thousand-request spike from one transient error. Add storeDown to providerCounters and set it on the failed-closed path, mirroring scopeUsage's existing contract. limiter.rejections grew without bound: nothing ever removed a scope whose user was renamed or deleted from a hot-reloaded users file, so a long-running process kept exporting a series for every name ever seen. Bound the map with the same current/previous generation-rotation authFailureTracker already uses, and evict a user's entry when authStore.maybeReload performs a real reload. maybeReload now returns whether it actually reloaded, so ServeHTTP only runs the eviction walk on that rare event, never on every request. Also corrects a stale comment: the rejection counter's store-down branch fires on every request during a full outage, not rarely, as an earlier comment claimed. --- limits.go | 168 ++++++++++++++++++++++++++++++++++++++++--------- limits_test.go | 99 ++++++++++++++++++++++++++++- llmgateway.go | 9 ++- users_file.go | 29 ++++++--- 4 files changed, 263 insertions(+), 42 deletions(-) diff --git a/limits.go b/limits.go index 041e14b..25750db 100644 --- a/limits.go +++ b/limits.go @@ -593,10 +593,20 @@ type limiter struct { // requests were rejected" has no existing counter to read at all // (req:min/req:day count every admission attempt, accepted or not, // by design — see checkAndCount's own doc comment). Adding it here - // as a bare map increment costs one uncontended lock per rejection - // (already the rare, not-hot-path outcome of checkAndCount), never a - // second network round trip, and never re-derives a number the - // store already tracks. + // as a bare map increment costs one uncontended lock per rejection, + // never a second network round trip, and never re-derives a number + // the store already tracks. + // + // CORRECTION (adversarial verification, 2026-08-23): an earlier + // version of this comment called the increment "the rare, not-hot- + // path outcome of checkAndCount" — true for an ordinary single-scope + // limit breach, but false for the storeDownViolation branch + // (checkAndCount's own body, below): with a configured store down + // and failOpen false, EVERY request takes that branch, so the + // increment runs on every request for as long as the outage lasts, + // not rarely. The honest claim is narrower: it is still cheaper than + // the network round trip that already failed to produce this + // violation in the first place, never an ADDITIONAL one. rejections rejectionCounter logMu sync.Mutex failOpen bool // store-error policy: true falls back to fallback, false refuses the request @@ -613,31 +623,67 @@ type rejectionScope struct { id string } +// rejectionCounterMapCap bounds how many distinct (kind, id) scopes +// rejectionCounter.current tracks before rotating (review fix, +// adversarial verification 2026-08-23): replaceFileUsers (auth.go) can +// hot-reload the configured user/group set at any time, and nothing +// here ever removed a scope's entry when the user or group behind it +// was renamed or deleted — over a long-running process's lifetime, a +// deployment that renames or churns users keeps exporting a series FOR +// EVERY NAME EVER SEEN, not just currently configured ones, growing +// this map (and the resulting Prometheus series count) without bound. +// 2000 comfortably exceeds the 1,000-user scale this codebase already +// treats as a real deployment size elsewhere (adminUsageChunkScopes' +// own doc comment, admin.go) with headroom left for renamed-user churn +// and the synthetic store_down entry, while still bounding the worst +// case. +const rejectionCounterMapCap = 2000 + // rejectionCounter accumulates rate/budget-limit-violation counts per // scope for the lifetime of the process — limiter.rejections' own // backing store; see its doc comment for why this exists as a bare -// in-process map rather than a counterStore key. Unlike authFailureTracker -// (auth.go), there is no TTL or generation rotation here: a Prometheus -// counter must only ever grow for as long as the process runs, so there -// is nothing to roll over, and cardinality is bounded by the configured -// user/group count (checkAndCount only ever sees scopes for an already- -// authenticated caller's own user/group, never attacker-controlled -// strings), the same bound scopeUsage's own per-scope metrics already -// carry. +// in-process map rather than a counterStore key. +// +// current/previous generation-rotation (review fix, adversarial +// verification 2026-08-23) mirrors authFailureTracker's own shape +// (auth.go) for the identical reason: bound total memory without ever +// scanning. Rotation is a two-pointer swap (O(1)), triggered on +// inserting a brand-new scope once current is already at +// rejectionCounterMapCap; total memory is bounded to at most +// 2×rejectionCounterMapCap entries. Unlike authFailureTracker, there is +// no TTL — a Prometheus counter must only ever grow for as long as its +// series is being reported, so a scope's count is never read back out +// of previous once superseded (see snapshot, below): a scope displaced +// by rotation, if it is ever rejected again, starts a fresh count at 1 +// rather than resuming its old total. That is a real, visible counter +// reset for that one series — accepted, the same trade-off +// authFailureTracker's own doc comment already accepts for a different +// bounded resource, and one Prometheus's own counter-reset handling in +// rate()/increase() already tolerates (the identical mechanism that +// already tolerates a process restart). type rejectionCounter struct { - counts map[rejectionScope]int64 - mu sync.Mutex + current map[rejectionScope]int64 + previous map[rejectionScope]int64 + mu sync.Mutex } -// increment records one rejection for (kind, id), initializing the -// backing map on first use — rejectionCounter's zero value (as embedded, -// unexported, in limiter) is ready to use without a constructor. +// increment records one rejection for (kind, id), initializing current +// on first use — rejectionCounter's zero value (as embedded, unexported, +// in limiter) is ready to use without a constructor. Rotates current -> +// previous before inserting a brand-new (kind, id) once current is +// already at rejectionCounterMapCap, mirroring authFailureTracker. +// increment's identical shape (auth.go). func (c *rejectionCounter) increment(kind, id string) { + key := rejectionScope{kind: kind, id: id} c.mu.Lock() - if c.counts == nil { - c.counts = make(map[rejectionScope]int64) + if c.current == nil { + c.current = make(map[rejectionScope]int64) } - c.counts[rejectionScope{kind: kind, id: id}]++ + if _, ok := c.current[key]; !ok && len(c.current) >= rejectionCounterMapCap { + c.previous = c.current + c.current = make(map[rejectionScope]int64, rejectionCounterMapCap) + } + c.current[key]++ c.mu.Unlock() } @@ -649,18 +695,43 @@ type rejectionSnapshot struct { count int64 } -// snapshot returns every scope with at least one recorded rejection, in -// no particular order (metrics.go sorts its own copy before rendering). +// snapshot returns every scope in current with at least one recorded +// rejection, in no particular order (metrics.go sorts its own copy +// before rendering). previous is deliberately NOT merged in: a scope +// there was displaced by rotation, and re-reporting its stale, no- +// longer-growing count would misrepresent it as still current. func (c *rejectionCounter) snapshot() []rejectionSnapshot { c.mu.Lock() defer c.mu.Unlock() - out := make([]rejectionSnapshot, 0, len(c.counts)) - for scope, n := range c.counts { + out := make([]rejectionSnapshot, 0, len(c.current)) + for scope, n := range c.current { out = append(out, rejectionSnapshot{kind: scope.kind, id: scope.id, count: n}) } return out } +// pruneUserScopes deletes every "user"-kind entry from both current and +// previous whose id is not in keep — limiter.pruneRejections' own +// backing implementation; see that method's doc comment for when and +// why this runs. Walking and deleting from a live map mid-range is safe +// in Go (deleting the current key during a range is explicitly +// permitted); previous is included so a recently-rotated, still-stale +// entry for a deleted user does not linger there either. +func (c *rejectionCounter) pruneUserScopes(keep map[string]bool) { + c.mu.Lock() + defer c.mu.Unlock() + for scope := range c.current { + if scope.kind == "user" && !keep[scope.id] { + delete(c.current, scope) + } + } + for scope := range c.previous { + if scope.kind == "user" && !keep[scope.id] { + delete(c.previous, scope) + } + } +} + // rejectionScopeStoreDown is the synthetic scope id checkAndCount // records a rejection under when it refuses a request because the // configured store is unreachable and failOpen is false @@ -678,6 +749,21 @@ func (l *limiter) rejectionSnapshot() []rejectionSnapshot { return l.rejections.snapshot() } +// pruneRejections removes every "user"-kind rejection scope whose id is +// not in keep (review fix, adversarial verification 2026-08-23): a user +// removed from a hot-reloaded users file (auth.go's replaceFileUsers) +// must not keep exporting llmgateway_rate_limit_rejections_total +// forever. Called only from ServeHTTP, and only when authStore. +// maybeReload just reported a REAL reload — see that method's own doc +// comment (users_file.go) for why this must never run on every request. +// Scope kinds other than "user" (group, the synthetic total and +// store_down entries) are left untouched: groups and the total scope +// are never hot-reloaded (Config.Groups is fixed at construction), and +// store_down names no real scope at all. +func (l *limiter) pruneRejections(keep map[string]bool) { + l.rejections.pruneUserScopes(keep) +} + // providerAttemptSpawnCap bounds how many concurrent recordProviderAttempt // store writes one limiter's production spawn implementation will run at // once (FOLDED-1, v0.22 review round, round 2) — see limiter.spawnTokens' @@ -1677,6 +1763,20 @@ type providerCounters struct { failuresMinute int64 attemptsDay int64 failuresDay int64 + // storeDown reports whether reading this scope's counters failed + // closed (a configured store errored and failOpen is false) — + // mirrors scopeUsage.storeDown exactly (review fix: the doc comment + // below used to claim "nothing downstream treats a provider's zero + // counters as confirmed no-traffic", which metrics.go's renderer + // made false the moment it existed — a Redis blip made every + // provider counter read as 0, then jump back to its real value on + // the next successful scrape, which Prometheus reads as a counter + // RESET followed by a fresh increase equal to the whole prior total: + // one transient store error paints a phantom multi-thousand-request + // spike. Every field is 0 when this is true; a caller must not + // present them as "confirmed zero usage", the identical rule + // scopeUsage's own doc comment states). + storeDown bool } // providerUsage reads every scope's current attempt/failure counters in @@ -1688,11 +1788,16 @@ type providerCounters struct { // kindProviderModel entries for the per-model breakdown, all in one // slice, so both share this single round trip. Order is preserved: // providerUsage(scopes)[i] corresponds to scopes[i]. A failed read -// (store down + fail-closed) returns every zero-value counters, exactly -// like targetUsage — see its own doc comment for why that is acceptable -// here: nothing downstream treats a provider's zero counters as -// "confirmed no traffic" the way scopeUsage.storeDown guards against for -// a limited scope. +// (store down + fail-closed) returns every counter at its zero value +// WITH storeDown set (review fix: adversarial verification, 2026-08-23 +// — the previous version of this comment claimed nothing downstream +// treats a provider's zero counters as confirmed no-traffic; metrics.go +// is exactly such a downstream, and without this flag a transient Redis +// blip fabricated a hard 0 that Prometheus reads as a counter reset, +// followed by a fresh "increase" equal to the whole prior total on the +// next successful scrape). A caller reading providerCounters must skip +// a storeDown entry the same way currentUsage's own callers already skip +// a storeDown scopeUsage — see that type's doc comment. // // Slicing is by running offset, not a fixed i*stride multiply (SHOULD-2, // providerCounterKeysPerScope's own doc comment): a kindProvider and a @@ -1715,7 +1820,10 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { vals, ok := l.storeGetMulti(allKeys) if !ok || len(vals) != len(allKeys) { - return out // zero-value counters; see doc comment above + for i := range out { + out[i] = providerCounters{storeDown: true} + } + return out } for i, sc := range scopes { diff --git a/limits_test.go b/limits_test.go index 5362e11..2857285 100644 --- a/limits_test.go +++ b/limits_test.go @@ -1944,14 +1944,107 @@ func TestProviderUsage_EmptyScopes_ReturnsEmptyWithoutTouchingStore(t *testing.T // TestProviderUsage_StoreDown_ReturnsZeroCounters mirrors targetUsage's // own fail-closed contract: a store error with failOpen=false must report -// every scope's counters as zero, not partial or stale values. +// every scope's counters as zero, not partial or stale values, AND must +// set storeDown (review fix, adversarial verification 2026-08-23): a +// caller that cannot tell a real zero from a failed read would otherwise +// fabricate a counter-reset spike in metrics.go's rendering. func TestProviderUsage_StoreDown_ReturnsZeroCounters(t *testing.T) { l := newLimiter(alwaysErrStore{}, false) got := l.providerUsage([]limitScope{{kind: kindProvider, id: "openai"}}) if len(got) != 1 { t.Fatalf("len(got) = %d, want 1", len(got)) } - if got[0] != (providerCounters{}) { - t.Errorf("counters = %+v, want the zero value on a storeDown read", got[0]) + if !got[0].storeDown { + t.Errorf("counters = %+v, want storeDown true on a failed-closed read", got[0]) + } + if got[0] != (providerCounters{storeDown: true}) { + t.Errorf("counters = %+v, want every non-storeDown field at its zero value", got[0]) + } +} + +// --- rejectionCounter: bounding, rotation, pruning (review fixes, adversarial verification 2026-08-23) --- + +// TestRejectionCounter_BoundsMemoryViaRotation mirrors +// TestAuthFailureTracker_BoundsMemoryViaRotation (auth_test.go) exactly: +// filling current past rejectionCounterMapCap distinct scopes must +// trigger an O(1) generation rotation, not unbounded growth — a +// deployment that renames or churns users over a long process lifetime +// must never grow this map (and the resulting Prometheus series count) +// without bound. +func TestRejectionCounter_BoundsMemoryViaRotation(t *testing.T) { + var c rejectionCounter + for i := 0; i < rejectionCounterMapCap+10; i++ { + c.increment("user", fmt.Sprintf("user-%d", i)) + } + + c.mu.Lock() + total := len(c.current) + len(c.previous) + currentLen := len(c.current) + c.mu.Unlock() + if total > 2*rejectionCounterMapCap { + t.Errorf("tracked entries = %d, want <= %d (2x cap)", total, 2*rejectionCounterMapCap) + } + if currentLen > rejectionCounterMapCap { + t.Errorf("current generation = %d entries, want <= %d", currentLen, rejectionCounterMapCap) + } +} + +// TestRejectionCounter_Increment_AccumulatesPerScope proves the ordinary +// path: repeated increments for the same (kind, id) accumulate in one +// series rather than creating a new one each time. +func TestRejectionCounter_Increment_AccumulatesPerScope(t *testing.T) { + var c rejectionCounter + c.increment("user", "alice") + c.increment("user", "alice") + c.increment("user", "bob") + + snaps := c.snapshot() + got := map[rejectionScope]int64{} + for _, s := range snaps { + got[rejectionScope{kind: s.kind, id: s.id}] = s.count + } + if got[rejectionScope{kind: "user", id: "alice"}] != 2 { + t.Errorf("alice count = %d, want 2", got[rejectionScope{kind: "user", id: "alice"}]) + } + if got[rejectionScope{kind: "user", id: "bob"}] != 1 { + t.Errorf("bob count = %d, want 1", got[rejectionScope{kind: "user", id: "bob"}]) + } +} + +// TestLimiter_PruneRejections_RemovesDeletedUserScope is the mutation- +// proofing test for pruneRejections/pruneUserScopes: a user's rejection +// series must disappear once that user is no longer in keep (the shape +// a real file-sourced hot-reload produces, via Gateway. +// pruneRejectionsAfterReload, metrics.go), while an unrelated user's +// series, and every non-"user" scope kind (group, the synthetic total +// and store_down entries), must survive untouched. +func TestLimiter_PruneRejections_RemovesDeletedUserScope(t *testing.T) { + l := newLimiter(nil, true) + l.rejections.increment("user", "alice") + l.rejections.increment("user", "bob") + l.rejections.increment("group", "agroup") + l.rejections.increment(totalScopeKind, totalScopeID) + l.rejections.increment(rejectionScopeStoreDown, rejectionScopeStoreDown) + + l.pruneRejections(map[string]bool{"bob": true}) // alice was removed from the users file; bob stays + + byKey := map[rejectionScope]bool{} + for _, s := range l.rejectionSnapshot() { + byKey[rejectionScope{kind: s.kind, id: s.id}] = true + } + if byKey[rejectionScope{kind: "user", id: "alice"}] { + t.Error("alice's user-scope rejection entry survived pruning, want it removed") + } + if !byKey[rejectionScope{kind: "user", id: "bob"}] { + t.Error("bob's user-scope rejection entry was pruned, want it kept (bob is still a known user)") + } + if !byKey[rejectionScope{kind: "group", id: "agroup"}] { + t.Error("group-scope entry was pruned — pruning must only ever touch \"user\"-kind scopes") + } + if !byKey[rejectionScope{kind: totalScopeKind, id: totalScopeID}] { + t.Error("the synthetic total-scope entry was pruned — pruning must only ever touch \"user\"-kind scopes") + } + if !byKey[rejectionScope{kind: rejectionScopeStoreDown, id: rejectionScopeStoreDown}] { + t.Error("the synthetic store_down entry was pruned — pruning must only ever touch \"user\"-kind scopes") } } diff --git a/llmgateway.go b/llmgateway.go index 93dd252..f0e6380 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -830,7 +830,14 @@ func attachUsersFile(auth *authStore, path string, log gatewayLogger) error { func (g *Gateway) ServeHTTP(w http.ResponseWriter, r *http.Request) { sw := &statusTrackingWriter{ResponseWriter: w} defer recoverPanic(sw, g) - g.auth.maybeReload() + // pruneRejectionsAfterReload only runs when maybeReload just performed + // a REAL reload (users_file.go's own doc comment explains why it must + // never run on every request) — a deleted file-sourced user must not + // keep exporting llmgateway_rate_limit_rejections_total forever + // (metrics.go). + if g.auth.maybeReload() { + g.pruneRejectionsAfterReload() + } g.registry.maybeRefresh(r.Context()) if r.Method == http.MethodGet && r.URL.Path == "/v1/models" { diff --git a/users_file.go b/users_file.go index 2b46731..f55deb0 100644 --- a/users_file.go +++ b/users_file.go @@ -60,9 +60,21 @@ type gatewayLogger interface { // replaceFileUsers rejecting the new set — it logs via a.log.errorf and // keeps the last good set. A successful reload that changes the // file-sourced user count logs via a.log.logf. -func (a *authStore) maybeReload() { +// +// Returns true only when a reload actually replaced the user set (review +// fix, adversarial verification 2026-08-23) — every early-return path +// above (no file configured, throttled, unchanged mtime, a stat/load +// error, or replaceFileUsers itself rejecting the new set) returns +// false. ServeHTTP uses this to gate limiter.pruneRejections: that call +// walks every tracked rejection scope, which is cheap only relative to a +// real reload (itself throttled to at most once per reloadEvery and +// gated on the file's mtime actually changing) — running it on every +// request regardless, the way maybeReload itself must, would add real +// per-request cost to ServeHTTP's hottest path for a reason (deleted +// users hot-reloaded out of the file) that is rare by construction. +func (a *authStore) maybeReload() bool { if a.usersFile == nil { - return + return false } // TryLock, not Lock: ServeHTTP calls maybeReload on every request, and a @@ -74,33 +86,33 @@ func (a *authStore) maybeReload() { // throttle window) covers it. Reload is deliberately best-effort and // throttled, not "every change lands on the very next request." if !a.reloadMu.TryLock() { - return + return false } defer a.reloadMu.Unlock() now := a.nowFn() if now.Sub(a.lastCheck) < reloadEvery { - return + return false } a.lastCheck = now info, err := os.Stat(a.usersFile.path) if err != nil { a.log.errorf("llmgateway: cannot stat users file %q: %v", a.usersFile.path, err) - return + return false } // Equal, not After: an After-only check misses a backward mtime move — // a restore from backup, or a `cp -p` from an older file. Any // difference from the last observed mtime, forward or backward, must // trigger a reload; only an unchanged mtime skips it. if info.ModTime().Equal(a.lastModTime) { - return + return false } users, err := a.usersFile.load() if err != nil { a.log.errorf("llmgateway: users file %q reload failed: %v", a.usersFile.path, err) - return + return false } a.mu.RLock() @@ -109,11 +121,12 @@ func (a *authStore) maybeReload() { if err := a.replaceFileUsers(users); err != nil { a.log.errorf("llmgateway: users file %q reload rejected: %v", a.usersFile.path, err) - return + return false } a.lastModTime = info.ModTime() if len(users) != prevCount { a.log.logf("llmgateway: users file %q reload: user count changed from %d to %d", a.usersFile.path, prevCount, len(users)) } + return true } From d352379375d78bb66a68ce539d2ea2a9cc6cf4e6 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:58:30 +0100 Subject: [PATCH 4/8] fix(metrics): honest rendering, injective escaping, aggregation docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from adversarial verification. writeProviderMetrics rendered a storeDown providerCounters entry as a hard zero, the same fabricated-reset hazard the request/token/cost families already guarded against. Skip a storeDown provider or provider-model entry instead of emitting it. escapeLabelValue ranged over the string as runes, and Go substitutes U+FFFD for every invalid UTF-8 byte. Two distinct configured names that both contain invalid UTF-8 could therefore render as the identical escaped value, colliding two series into one duplicate series and failing the whole scrape target. Escaping is now byte-oriented, which is injective and also avoids the per-rune decode cost on a large value. Every store-backed family's HELP text now states its aggregation contract: with Redis configured, the value is the gateway-wide total and identical on every replica, so summing across replicas overcounts by the replica count and a cost alert fires early. Aggregate with max() instead. Without Redis, the same family becomes genuinely per-process and sum() becomes correct instead. The rejection counter and the provider-healthy gauge get their own notes: the former is always per-process regardless of Redis, the latter reflects each replica's own independently-running discovery breaker. Also replaces the AllowedCIDRs doc example (10.0.0.0/8) with a cluster pod CIDR (10.42.0.0/16) and states plainly that a whole RFC1918 range is never correct, since a SNAT-ing router or load balancer makes external traffic arrive from inside it — reproduced live against the target cluster. Adds tests pinning three properties mutation-proofed against this suite: every storeDown skip (deleting any of them now fails TestMetrics_StoreDown_SkipsRatherThanFabricatesZero), the CIDR-check running before auth.identify (TestMetrics_ AllowlistedRequest_NeverTouchesAuthFailureTracking), and escapeLabelValue's injectivity under invalid UTF-8 (TestEscapeLabelValue_InvalidUTF8_Injective). Also covers the new rejection-counter pruning end to end through a real file-sourced reload. --- metrics.go | 135 ++++++++++++++++++++++++++++++----- metrics_test.go | 186 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 303 insertions(+), 18 deletions(-) diff --git a/metrics.go b/metrics.go index cde6098..723fe46 100644 --- a/metrics.go +++ b/metrics.go @@ -31,14 +31,29 @@ import ( // either "everyone" or "no one" depending on what address arrives. // Confirm RemoteAddr reflects the scraper's own address in your // deployment before relying on this instead of a key. +// +// THE FAILURE MODE TO AVOID IS NOT "the allowlist does not work" — it is +// "the allowlist works exactly as configured, and what survives to +// RemoteAddr is an address you never intended to trust" (review fix, +// adversarial verification 2026-08-23, reproduced live): a whole RFC1918 +// block such as 10.0.0.0/8 is never a correct entry, because a SNAT-ing +// router or load balancer makes EXTERNAL traffic arrive from an address +// inside that same private range — on the reproduction cluster, +// AllowedCIDRs: ["10.0.0.0/8"] paired with the router's own SNAT address +// returned full per-user cost data to an external caller with no key at +// all. Always scope this to the NARROWEST block that covers only your +// actual scrapers — a Prometheus/VictoriaMetrics pod CIDR such as +// 10.42.0.0/16, not the broad private range it happens to sit inside. type MetricsConfig struct { // Path overrides the served path. Empty (the default) serves at // metricsPathDefault ("/metrics"). Path string `json:"path,omitempty"` - // AllowedCIDRs lists CIDR blocks (e.g. "10.0.0.0/8") whose source - // address may scrape without an admin key. Empty means every - // scrape must carry a valid admin bearer token — see this type's - // own doc comment for the trust assumption this relies on. + // AllowedCIDRs lists CIDR blocks whose source address may scrape + // without an admin key — e.g. "10.42.0.0/16" for a cluster's pod + // network, never a whole RFC1918 range like "10.0.0.0/8". Empty + // means every scrape must carry a valid admin bearer token — see + // this type's own doc comment for the trust assumption this relies + // on, and for why the narrowest-possible block matters here. AllowedCIDRs []string `json:"allowedCIDRs,omitempty"` Enabled bool `json:"enabled,omitempty"` // ModelLabel adds a `model` label to the provider attempt/failure @@ -168,17 +183,36 @@ func (g *Gateway) serveMetrics(w http.ResponseWriter) { // one bad line, so this is a correctness requirement, not a formatting // nicety. // -// This walks s rune by rune and classifies each one independently, never +// This walks s BYTE by byte and classifies each one independently, never // a sequential find-and-replace pass: replacing quotes first and // backslashes second (or vice versa) would double-escape a backslash // this function itself just inserted — a real, classic bug class for // this exact kind of escaping. A single forward pass has no such // ordering hazard. +// +// Byte-oriented, deliberately NOT `for _, r := range s` (review fix, +// adversarial verification 2026-08-23): ranging over a string decodes +// UTF-8 and substitutes U+FFFD for every invalid byte, which makes the +// function NOT injective — two distinct configured names that both +// contain invalid UTF-8 (a byte sequence auth.go's buildEntry never +// rejects; it validates non-empty only, see its own doc comment) can +// decode to the identical U+FFFD run and render as the SAME escaped +// label value. Two scope_id values colliding onto one series is a +// duplicate-series scrape failure — confirmed against a real Prometheus +// scrape parser: the whole target reports DOWN, and promtool alone will +// not catch it, since expfmt silently dedupes. Escaping byte-for-byte +// passes every raw byte through unchanged except the three ASCII +// characters requiring escaping ('\\', '"', '\n') — none of which can +// ever appear as a UTF-8 continuation byte (continuation bytes are +// always >= 0x80), so this never misinterprets a multi-byte sequence's +// internal bytes as one of these three, and every distinct input byte +// string still maps to a distinct output. It also avoids WriteRune's +// per-rune decode cost on a large value. func escapeLabelValue(s string) string { var b bytes.Buffer b.Grow(len(s)) - for _, r := range s { - switch r { + for i := 0; i < len(s); i++ { + switch c := s[i]; c { case '\\': b.WriteString(`\\`) case '"': @@ -186,7 +220,7 @@ func escapeLabelValue(s string) string { case '\n': b.WriteString(`\n`) default: - b.WriteRune(r) + b.WriteByte(c) } } return b.String() @@ -257,6 +291,38 @@ func (m *metricWriter) sampleFloat(name string, labels []metricLabel, v float64) m.sample(name, labels, strconv.FormatFloat(v, 'g', -1, 64)) } +// aggregationNoteStoreBacked is appended to every store-backed family's +// HELP text below (item 1, adversarial verification 2026-08-23, +// reproduced against a 3-replica Traefik deployment): windowKey +// (limits.go) carries no per-instance component, so with Config.Redis +// configured every replica's counterStore read returns the SAME +// gateway-wide value — Prometheus scrapes each replica as its own +// target, so sum(rate(...)) or sum(...) across replicas TRIPLE-COUNTS +// on 3 replicas (a cost alert fires at a third of its intended +// threshold). The correct cross-replica aggregation is max(), since +// every replica already reports the true total on its own. Without +// Config.Redis (the in-memory memoryStore fallback), the IDENTICAL +// family becomes genuinely per-process instead — each replica counts +// only the traffic it personally handled — and sum() becomes the +// correct aggregation instead of max(). This flips on that one config +// bit; know which one a deployment is running before wiring an alert. +const aggregationNoteStoreBacked = " AGGREGATION ACROSS REPLICAS: with Redis configured, this value is the gateway-wide total read from shared limit state and is IDENTICAL on every replica's own /metrics response — aggregate with max(), never sum(), or a multi-replica deployment overcounts by the replica count. Without Redis (the in-memory fallback), this SAME family becomes genuinely per-process instead, and sum() becomes the correct aggregation. Know which one you are running." + +// aggregationNotePerProcess is appended to llmgateway_rate_limit_ +// rejections_total's HELP text: limiter.rejections (limits.go) is a +// bare in-process map, never written to or read from Redis regardless +// of Config.Redis — unlike every store-backed family above, this one +// never flips, and sum() is always the right cross-replica aggregation. +const aggregationNotePerProcess = " AGGREGATION ACROSS REPLICAS: this counter lives only in this process's memory, never in Redis, regardless of Config.Redis — aggregate with sum(), which is always correct for it, unlike the store-backed families above." + +// aggregationNoteProviderHealthy is appended to llmgateway_provider_ +// healthy's HELP text: the discovery circuit breaker (registry.go's +// providerState.health) is per-process state, set by each replica's own +// discovery polling loop, and is never written to Redis — two replicas +// can legitimately disagree about one provider's health at the same +// instant (e.g. one mid-backoff, one already recovered). +const aggregationNoteProviderHealthy = " AGGREGATION ACROSS REPLICAS: each replica runs its own discovery circuit breaker in-process, never shared via Redis, so this can legitimately differ per replica. Read it per-instance where possible; if you must aggregate, use min() to surface \"at least one replica sees this provider as unhealthy\" — sum() is meaningless for a 0/1 gauge." + // renderMetrics builds the full Prometheus text-exposition document for // GET . Every read below goes through the SAME batched, // chunked accessors GET /admin/api/overview and GET /admin/api/usage @@ -304,7 +370,7 @@ func (g *Gateway) writeUsageMetrics(m *metricWriter) { usage := g.chunkedCurrentUsage(scopes) m.family("llmgateway_requests_total", "counter", - "Total requests admitted for accounting today (UTC calendar day; resets at UTC midnight), by scope.") + "Total requests admitted for accounting today (UTC calendar day; resets at UTC midnight), by scope."+aggregationNoteStoreBacked) for _, su := range usage { if su.storeDown { continue @@ -314,7 +380,7 @@ func (g *Gateway) writeUsageMetrics(m *metricWriter) { } m.family("llmgateway_tokens_total", "counter", - "Total tokens accounted today (UTC calendar day; resets at UTC midnight), by scope and direction.") + "Total tokens accounted today (UTC calendar day; resets at UTC midnight), by scope and direction."+aggregationNoteStoreBacked) for _, su := range usage { if su.storeDown { continue @@ -326,7 +392,7 @@ func (g *Gateway) writeUsageMetrics(m *metricWriter) { } m.family("llmgateway_cost_micro_usd_total", "counter", - "Total cost accounted today (UTC calendar day; resets at UTC midnight), in micro-USD (1000000 = $1), by scope.") + "Total cost accounted today (UTC calendar day; resets at UTC midnight), in micro-USD (1000000 = $1), by scope."+aggregationNoteStoreBacked) for _, su := range usage { if su.storeDown { continue @@ -336,7 +402,7 @@ func (g *Gateway) writeUsageMetrics(m *metricWriter) { } m.family("llmgateway_budget_consumed_ratio", "gauge", - "Fraction of a configured limit already consumed in its own window; 1.0 means fully consumed, above 1.0 means the limit has been breached. Emitted only for a scope with that particular limit configured.") + "Fraction of a configured limit already consumed in its own window; 1.0 means fully consumed, above 1.0 means the limit has been breached. Emitted only for a scope with that particular limit configured."+aggregationNoteStoreBacked) for i, su := range usage { if su.storeDown { continue @@ -416,17 +482,29 @@ func (g *Gateway) writeProviderMetrics(m *metricWriter) { providerCounts := counters[:len(snaps)] m.family("llmgateway_provider_attempts_total", "counter", - "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider.") + "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider."+aggregationNoteStoreBacked) m.family("llmgateway_provider_failures_total", "counter", - "Total upstream attempts today classified as a provider fault (transport error, 429, or 5xx — the same classification recordProviderAttempt/isTransient apply), by provider.") + "Total upstream attempts today classified as a provider fault (transport error, 429, or 5xx — the same classification recordProviderAttempt/isTransient apply), by provider."+aggregationNoteStoreBacked) for i, s := range snaps { + // A storeDown counter is a failed-closed read (limits.go's + // providerUsage), not a real zero (review fix, adversarial + // verification 2026-08-23): emitting it as a hard 0 here is + // exactly the bug writeUsageMetrics' own storeDown guard above + // already avoids for the request/token/cost families — a Redis + // blip would otherwise paint a phantom counter-reset spike the + // instant the store recovers and this value jumps back to its + // real total. Skip the series entirely; a temporary gap is the + // honest signal, a fabricated 0 is not. + if providerCounts[i].storeDown { + continue + } labels := []metricLabel{{"provider", s.name}} m.sampleInt("llmgateway_provider_attempts_total", labels, providerCounts[i].attemptsDay) m.sampleInt("llmgateway_provider_failures_total", labels, providerCounts[i].failuresDay) } m.family("llmgateway_provider_healthy", "gauge", - "1 when the provider's discovery circuit breaker is not open (closed or half-open), 0 when open (discoveryHealthy, registry.go).") + "1 when the provider's discovery circuit breaker is not open (closed or half-open), 0 when open (discoveryHealthy, registry.go)."+aggregationNoteProviderHealthy) for _, s := range snaps { healthy := int64(1) if s.health == breakerOpen { @@ -441,14 +519,17 @@ func (g *Gateway) writeProviderMetrics(m *metricWriter) { modelCounts := counters[len(snaps):] m.family("llmgateway_provider_model_attempts_total", "counter", - "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider and model. Opt-in via metrics.modelLabel — see MetricsConfig's own doc comment for the cardinality trade-off.") + "Total upstream attempts today (UTC calendar day; resets at UTC midnight), by provider and model. Opt-in via metrics.modelLabel — see MetricsConfig's own doc comment for the cardinality trade-off."+aggregationNoteStoreBacked) m.family("llmgateway_provider_model_failures_total", "counter", - "Total upstream attempts today classified as a provider fault, by provider and model. Opt-in via metrics.modelLabel.") + "Total upstream attempts today classified as a provider fault, by provider and model. Opt-in via metrics.modelLabel."+aggregationNoteStoreBacked) mi := 0 for _, s := range snaps { for _, model := range s.models { mc := modelCounts[mi] mi++ + if mc.storeDown { // same fabricated-reset hazard as the provider-level loop above + continue + } labels := []metricLabel{{"provider", s.name}, {"model", model}} m.sampleInt("llmgateway_provider_model_attempts_total", labels, mc.attemptsDay) m.sampleInt("llmgateway_provider_model_failures_total", labels, mc.failuresDay) @@ -475,9 +556,27 @@ func (g *Gateway) writeRejectionMetrics(m *metricWriter) { }) m.family("llmgateway_rate_limit_rejections_total", "counter", - "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed — it resets only on restart.") + "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed — it resets only on restart."+aggregationNotePerProcess) for _, s := range snaps { m.sampleInt("llmgateway_rate_limit_rejections_total", []metricLabel{{"scope_kind", s.kind}, {"scope_id", s.id}}, s.count) } } + +// pruneRejectionsAfterReload evicts every "user"-kind rejection scope +// (limiter.pruneRejections, limits.go) whose name is no longer among +// g.auth's currently active users — ServeHTTP's own call site +// (llmgateway.go), gated on authStore.maybeReload just having performed +// a REAL file-sourced reload, never run unconditionally (see +// maybeReload's own doc comment, users_file.go, for why). Without this, +// a user removed from a hot-reloaded users file would keep exporting +// llmgateway_rate_limit_rejections_total for the rest of the process's +// life (review fix, adversarial verification 2026-08-23). +func (g *Gateway) pruneRejectionsAfterReload() { + userSummaries, _ := g.auth.snapshot() + keep := make(map[string]bool, len(userSummaries)) + for _, us := range userSummaries { + keep[us.name] = true + } + g.limiter.pruneRejections(keep) +} diff --git a/metrics_test.go b/metrics_test.go index b78f0ed..08214a5 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "path/filepath" "sort" "strconv" "strings" @@ -175,6 +176,38 @@ func TestMetrics_NoAllowlistConfigured_KeylessRequestRejected(t *testing.T) { } } +// TestMetrics_AllowlistedRequest_NeverTouchesAuthFailureTracking pins the +// ORDERING handleMetrics' own doc comment claims (metrics.go): the CIDR +// allowlist is checked BEFORE auth.identify ever runs, so a keyless, +// allowlisted scrape never records a failed-auth attempt for its own +// source address. This is not observable from status codes alone — a +// handler that checked identify FIRST and fell back to the CIDR +// allowlist only on failure would return the identical 200 for every +// case TestMetrics_GateMatrix asserts, while still quietly feeding +// authStore's failure tracker on every single scrape. Mutation-proofing: +// reordering the two checks changes only this test's outcome, nothing +// else in this file. +func TestMetrics_AllowlistedRequest_NeverTouchesAuthFailureTracking(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics.AllowedCIDRs = []string{"192.0.2.0/24"} + h, gw := newMetricsGatewayHandle(t, cfg) + + for i := 0; i < 5; i++ { + rec := httptest.NewRecorder() + // Default RemoteAddr (192.0.2.1:1234, verified empirically) — + // inside the allowlist, no key presented at all. + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "")) + if rec.Code != http.StatusOK { + t.Fatalf("request %d: status = %d, want 200", i, rec.Code) + } + } + + if got := gw.auth.failures.get("192.0.2.1"); got != 0 { + t.Errorf("auth failure count for the allowlisted scraper's address = %d, want 0 — the CIDR check must run before auth.identify, never after", got) + } +} + func TestMetrics_InvalidCIDR_FailsConstruction(t *testing.T) { t.Parallel() cfg := newMetricsTestConfig() @@ -453,6 +486,13 @@ func TestEscapeLabelValue(t *testing.T) { {"quote_then_backslash", `"\`, `\"\\`}, {"backslash_then_quote", `\"`, `\\\"`}, {"all_three", "a\"b\\c\nd", `a\"b\\c\nd`}, + // Invalid UTF-8 bytes (0x80/0x81 are bare continuation bytes, + // never valid as the START of any UTF-8 sequence) must pass + // through UNCHANGED, byte-for-byte — see + // TestEscapeLabelValue_InvalidUTF8_Injective below for why this + // matters beyond just "does not crash". + {"invalid_utf8_lone_0x80", "u\x80", "u\x80"}, + {"invalid_utf8_lone_0x81", "u\x81", "u\x81"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -464,6 +504,30 @@ func TestEscapeLabelValue(t *testing.T) { } } +// TestEscapeLabelValue_InvalidUTF8_Injective is the mutation-proofing +// test for the byte-oriented rewrite (review fix, adversarial +// verification 2026-08-23): a rune-oriented `for _, r := range s` +// implementation decodes invalid UTF-8 to U+FFFD, which makes escaping +// NOT injective — "u\x80" and "u\x81" are two distinct, real inputs +// (auth.go's buildEntry validates a user name as non-empty only, never +// restricted to valid UTF-8) that would both render as "u�" under +// that approach, colliding two distinct scope_id label values onto one +// series — confirmed against a real Prometheus scrape parser: a +// duplicate series, and the whole scrape target reported DOWN. The +// byte-oriented implementation must keep every input byte-for-byte +// distinguishable. +func TestEscapeLabelValue_InvalidUTF8_Injective(t *testing.T) { + t.Parallel() + a := escapeLabelValue("u\x80") + b := escapeLabelValue("u\x81") + if a == b { + t.Fatalf("escapeLabelValue(%q) == escapeLabelValue(%q) == %q — two distinct inputs collided onto one label value", "u\x80", "u\x81", a) + } + if strings.Contains(a, "�") || strings.Contains(b, "�") { + t.Errorf("escaped output contains U+FFFD (replacement character) — the rune-decoding bug this test guards against: a=%q b=%q", a, b) + } +} + // TestMetrics_LabelValuesWithSpecialChars_Escaped is the end-to-end // version of TestEscapeLabelValue: a real user name containing a double // quote, a backslash, and a newline (auth.go's buildEntry doc comment @@ -689,6 +753,69 @@ func TestMetrics_ProviderHealthy_ReflectsOpenBreaker(t *testing.T) { assertSample(t, samples, "llmgateway_provider_healthy", map[string]string{"provider": "openai"}, "0") } +// --- store outage: skip, never fabricate a zero --- + +// TestMetrics_StoreDown_SkipsRatherThanFabricatesZero is the mutation- +// proofing test for every `if su.storeDown { continue }` / +// `if mc.storeDown { continue }` guard in metrics.go: deleting any of +// them makes this test fail. Forces the SAME limiter every family in +// this file reads from into a fail-closed store outage (alwaysErrStore, +// failOpen=false — routes_unified_test.go's own stub), then asserts +// every store-backed family emits NO samples at all rather than a +// fabricated 0 (review fix, adversarial verification 2026-08-23: +// without the guards, a Redis blip reads as a hard 0 mid-outage, then +// jumps back to the real total on recovery — Prometheus treats that as +// a counter reset followed by a spurious full-total "increase"). +// llmgateway_provider_healthy is deliberately excluded from the "must be +// absent" list: it comes from the in-process discovery breaker, never +// the counterStore, so a store outage must NOT affect it — asserted +// present here for exactly that reason. +func TestMetrics_StoreDown_SkipsRatherThanFabricatesZero(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + cfg.Metrics.ModelLabel = true + cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerDay: 100} + h, gw := newMetricsGatewayHandle(t, cfg) + + gw.limiter.store = alwaysErrStore{} + gw.limiter.failOpen = false + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + mustBeAbsent := []string{ + "llmgateway_requests_total", + "llmgateway_tokens_total", + "llmgateway_cost_micro_usd_total", + "llmgateway_budget_consumed_ratio", + "llmgateway_provider_attempts_total", + "llmgateway_provider_failures_total", + "llmgateway_provider_model_attempts_total", + "llmgateway_provider_model_failures_total", + } + for _, s := range samples { + for _, absent := range mustBeAbsent { + if s.name == absent { + t.Errorf("sample %s%v present during a store outage — must be skipped, not fabricated as 0", s.name, s.labels) + } + } + } + + var providerHealthyPresent bool + for _, s := range samples { + if s.name == "llmgateway_provider_healthy" { + providerHealthyPresent = true + } + } + if !providerHealthyPresent { + t.Error("llmgateway_provider_healthy missing — this family reads in-process breaker state, not the store, and must still render during a store outage") + } +} + // --- rate-limit rejections --- // TestMetrics_RateLimitRejections_CountedByScope drives a real 429 @@ -745,6 +872,65 @@ func TestLimiter_RejectionSnapshot_StoreDown(t *testing.T) { } } +// TestMetrics_PruneRejectionsAfterReload_EndToEnd is the end-to-end proof +// for item 7 (adversarial verification, 2026-08-23): a file-sourced user +// removed via a real users-file hot-reload (auth.go's replaceFileUsers, +// driven through ServeHTTP exactly as production traffic drives it, not +// a direct limiter call) must stop exporting +// llmgateway_rate_limit_rejections_total, proving the wiring from +// ServeHTTP through authStore.maybeReload's new return value to +// Gateway.pruneRejectionsAfterReload actually runs, not just the +// pruning logic in isolation (TestLimiter_PruneRejections_ +// RemovesDeletedUserScope, limits_test.go). +func TestMetrics_PruneRejectionsAfterReload_EndToEnd(t *testing.T) { + t.Parallel() + dir := t.TempDir() + fp := filepath.Join(dir, "users.json") + writeUsersDoc(t, fp, []*UserConfig{ + {Name: "alice", Group: "g", APIKey: "sk-alice", Limits: &LimitsConfig{RequestsPerMinute: 1}}, + }, time.Time{}) + + cfg := newMetricsTestConfig() + cfg.Users = &UsersConfig{ + File: fp, + Inline: []*UserConfig{{Name: "admin1", Group: "g", APIKey: "sk-admin1", Admin: true}}, + } + h, gw := newMetricsGatewayHandle(t, cfg) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + h.ServeHTTP(httptest.NewRecorder(), newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-alice", body)) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("status = %d, want 429 (RequestsPerMinute:1 exhausted)", rec.Code) + } + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + assertSample(t, samples, "llmgateway_rate_limit_rejections_total", map[string]string{"scope_kind": "user", "scope_id": "alice"}, "1") + + // Remove alice from the users file and force ServeHTTP's own + // maybeReload call to pick it up — mirrors users_file_test.go's own + // clock-advance idiom (newReloadableAuthStore). + clock := &fakeClock{now: time.Now()} + gw.auth.nowFn = clock.Now + writeUsersDoc(t, fp, nil, time.Now().Add(time.Hour)) + clock.Advance(reloadEvery + time.Second) + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + _, samples = parsePrometheusText(t, rec.Body.Bytes()) + for _, s := range samples { + if s.name == "llmgateway_rate_limit_rejections_total" && s.labels["scope_id"] == "alice" { + t.Errorf("alice's rejection series is still present after being removed from the users file and reloaded: %+v", s) + } + } +} + // --- budget-consumed ratio --- func TestMetrics_BudgetConsumedRatio_ReflectsUsageVsLimit(t *testing.T) { From 244dd6185febbf87b11686defba9c521bbf9c49e Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:58:51 +0100 Subject: [PATCH 5/8] test(yaegi-check): verify label escaping under the interpreter Add a third configured user whose name contains a double quote, a backslash, and a newline, then assert GET /metrics renders its Prometheus-escaped form and never the raw name. auth.go validates a user name as non-empty only, so this is a real, legal configuration, not a contrived one. The compiled suite already proves escapeLabelValue correct (TestMetrics_LabelValuesWithSpecialChars_Escaped, TestEscapeLabelValue_InvalidUTF8_Injective), but this codebase's own history is that a compiled pass has repeatedly said nothing about the interpreted shape. json.Marshal produces the JSON-escaped literal for the harness's own hand-built config document, kept separate from the Prometheus escaping the probe actually verifies in the rendered body. --- tools/yaegi-check/main.go | 47 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index 0799a9d..69e690b 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -346,6 +346,21 @@ func run() error { })) defer brkUpstream.Close() + // metricsProbeTrickyUserNameJSON: a user name containing a double + // quote, a backslash, and a newline — auth.go's buildEntry validates + // a name as non-empty only, never restricted to a URL-path-segment + // character set (see its own doc comment), so this is a real, + // legal config, not a contrived one. json.Marshal, not hand-built + // string concatenation, produces the correctly JSON-escaped literal + // to splice into attemptAccountingOverride below — this is JSON + // escaping for the CONFIG document, a separate concern from the + // Prometheus label escaping exerciseMetricsRoute actually verifies + // in the RENDERED /metrics body. + trickyNameJSON, err := json.Marshal(metricsProbeTrickyUserName) + if err != nil { + return fmt.Errorf("marshal metrics escaping probe user name: %w", err) + } + attemptAccountingOverride := `{"breaker":{"failureThreshold":2,"openDuration":"3s","maxOpenDuration":"6s"},` + `"providers":{"openai":{"type":"openai","baseUrl":"` + upstream.URL + `","apiKey":"sk-up","models":["` + testDataWantModel + `","` + builtinLookupModelID + `"]},` + `"brk":{"type":"openai","baseUrl":"` + brkUpstream.URL + `","apiKey":"sk-up","discovery":true,"discoveryInterval":"1ms"},"` + @@ -379,7 +394,13 @@ func run() error { // CIDR-bypass path is reachable with a plain httptest.NewRequest // carrying no explicit RemoteAddr override. `"metrics":{"enabled":true,"allowedCIDRs":["` + metricsProbeAllowedCIDR + `"],"modelLabel":true},` + - `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true}]}}` + // The third inline user (metricsProbeTrickyUserName) exists + // purely so exerciseMetricsRoute can prove the escaping path + // under the interpreter — it makes no requests of its own, but + // still renders a real (zero-traffic) llmgateway_requests_total + // series, since writeUsageMetrics (metrics.go) lists every + // active user regardless of traffic. + `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true},{"name":` + string(trickyNameJSON) + `,"group":"default","apiKey":"sk-tricky"}]}}` if err = json.Unmarshal([]byte(attemptAccountingOverride), cfgVal.Interface()); err != nil { return fmt.Errorf("decode attempt-accounting harness override into the interpreted Config: %w", err) } @@ -468,6 +489,17 @@ const metricsProbeAllowedCIDR = "192.0.2.0/24" // own proof that the allowlist is not simply matching everything. const metricsProbeOutsideAddr = "203.0.113.9:5555" +// metricsProbeTrickyUserName is a configured user name containing a +// double quote, a backslash, and a newline — the exact character set +// escapeLabelValue (metrics.go) exists to handle, driven under the REAL +// interpreter (review fix, adversarial verification 2026-08-23): the +// compiled test suite already proves escaping correct +// (TestMetrics_LabelValuesWithSpecialChars_Escaped, +// TestEscapeLabelValue_InvalidUTF8_Injective), but this codebase's own +// history is that a compiled pass has repeatedly said nothing about the +// interpreted shape. +const metricsProbeTrickyUserName = "weird\"user\\name\nwith-newline" + // mcpProbeOversizeBytes is how much filler mcpProbeUpstream writes: over // mcpBackendCallResponseMaxBytes (maxRequestBytes, 10MiB) so the response // trips doBackendJSONRPC's cap. Kept as its own named value rather than @@ -709,17 +741,30 @@ func exerciseMetricsRoute(handler http.Handler) error { return fmt.Errorf("GET /metrics (admin key): Content-Type = %q, want a text/plain prefix", ct) } body := rec.Body.String() + // metricsProbeEscapedTrickyName is escapeLabelValue's OWN expected + // output for metricsProbeTrickyUserName — a raw Go string literal + // (backticks: every backslash below is a literal backslash + // character, not a Go escape), computed by hand from the format + // rule (\\ -> \\\\, " -> \\", newline -> \\n) rather than by calling + // the function itself, so this assertion cannot pass merely because + // the interpreted and this harness's own understanding of the rule + // happen to agree by construction. + const metricsProbeEscapedTrickyName = `weird\"user\\name\nwith-newline` for _, want := range []string{ "# TYPE llmgateway_requests_total counter", "# TYPE llmgateway_provider_healthy gauge", `llmgateway_provider_attempts_total{provider="openai"}`, "# TYPE llmgateway_provider_model_attempts_total counter", `llmgateway_provider_model_attempts_total{provider="openai",model="` + testDataWantModel + `"}`, + `llmgateway_requests_total{scope_kind="user",scope_id="` + metricsProbeEscapedTrickyName + `"}`, } { if !strings.Contains(body, want) { return fmt.Errorf("GET /metrics (admin key): body missing %q — interpreted rendering diverged from the compiled shape; body=%s", want, body) } } + if strings.Contains(body, metricsProbeTrickyUserName) { + return fmt.Errorf("GET /metrics (admin key): body contains the RAW, unescaped tricky user name — escapeLabelValue did not run interpreted; body=%s", body) + } allowlistedReq := httptest.NewRequest(http.MethodGet, "/metrics", nil) // default RemoteAddr, inside metricsProbeAllowedCIDR rec = httptest.NewRecorder() From 935df1a3f06292a3bf88a4f25eca7055a122083e Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 00:59:13 +0100 Subject: [PATCH 6/8] docs(metrics): add README section, testData, and a scrape example The metrics feature shipped in code only: no README section, no metrics block in .traefik.yml testData, nothing under examples/. The Go doc comments were the entire manual for a human operator or a config-generating assistant. Add a README Metrics section covering the served path, both auth modes (admin bearer token, optional CIDR allowlist), the CIDR-sizing warning with the live-reproduced SNAT failure mode, the modelLabel cardinality trade-off, the exposed metric families, the cross-replica aggregation contract, label escaping, and store-outage behaviour. Add a MetricsConfig reference table matching the existing AdminConfig one, and a metrics row in the top-level Config table. Add metrics: {enabled: true} to .traefik.yml's testData, the catalog-facing smoke-test config, deliberately without allowedCIDRs so a copy-pasted example never skips the README's own sizing guidance. Add a metrics block to the Kubernetes Middleware example and a VMServiceScrape resource: the target cluster runs VictoriaMetrics, not Prometheus, so the CRD differs even though the exposition format does not. Verified the VMServiceScrape shape (apiVersion, selector, endpoints, path) against the victoriametrics/operator documentation rather than assuming it. Notes that /metrics is served by Traefik itself, not by any backend service, since this middleware intercepts the path before a backend is ever reached. --- .traefik.yml | 7 ++ README.md | 144 +++++++++++++++++++++++++++++++++++++++ examples/kubernetes.yaml | 53 ++++++++++++++ 3 files changed, 204 insertions(+) diff --git a/.traefik.yml b/.traefik.yml index ba88b2e..b430100 100644 --- a/.traefik.yml +++ b/.traefik.yml @@ -28,3 +28,10 @@ testData: - name: tester group: default apiKey: test-user-key + # Prometheus text-exposition endpoint — see README.md#metrics. No + # allowedCIDRs here deliberately: this is the catalog-facing smoke-test + # config, and README.md#metrics has the full CIDR-sizing guidance + # (narrowest block only, never a whole RFC1918 range) that a + # copy-pasted example here would risk skipping past. + metrics: + enabled: true diff --git a/README.md b/README.md index a2f0871..3146250 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,7 @@ config accepts them as YAML, which decodes to the same JSON shape. | `cache` | `CacheConfig` | `{}` (disabled) | Opt-in Redis-backed response cache for unified non-streaming chat/embeddings — see [Caching](#caching). Omitted or `enabled: false` means no caching, byte-identical to a gateway built before this field existed. | | `breaker` | `BreakerConfig` | `{}` (every default) | Per-provider discovery circuit breaker — see [Provider health (discovery circuit breaker)](#provider-health-discovery-circuit-breaker). Every field is individually zero-means-default; a provider whose discovery never fails is unaffected regardless of what this block contains. | | `admin` | `*AdminConfig` | `nil` (disabled) | Read-only admin dashboard — see [Admin](#admin). `nil` or `enabled: false` means the `/admin*` routes are not registered at all. | +| `metrics` | `*MetricsConfig` | `nil` (disabled) | Prometheus text-exposition endpoint — see [Metrics](#metrics). `nil` or `enabled: false` means the metrics route is not registered at all. | | `modelAliases` | `map[string]string` | `{}` | Operator-defined alias id → target model id — see [Model aliases](#model-aliases). Omitted or empty means no aliases, byte-identical to a gateway built before this field existed. | | `modelMeta` | `map[string]*ModelMetaConfig` | `{}` | Per-model context-window and cost overrides, keyed by exact `provider/model` or a bare model id — see [Model metadata](#model-metadata). Omitted or empty means no overrides; every model's metadata still resolves through discovery and the built-in table. | | `passthroughUnknown` | `bool` | `false` | `false`: a request matching none of the plugin's routes gets a 404 JSON envelope. `true`: it falls through to the router's own backing service. **Does not cover the three media routes** — see the upgrade note below. | @@ -327,6 +328,15 @@ own is governed purely by their group's — see |---|---|---|---| | `enabled` | `bool` | `false` | `false`: the `/admin*` routes are not registered at all — a request to any of them falls through to the plugin's existing 404/`passthroughUnknown` handling like any other unrecognized path. `true`: the read-only dashboard per [Admin](#admin) below. | +### `MetricsConfig` + +| Field | Type | Default | Semantics | +|---|---|---|---| +| `enabled` | `bool` | `false` | `false`: the metrics route is not registered at all. `true`: the Prometheus text-exposition endpoint per [Metrics](#metrics) below. | +| `path` | `string` | `/metrics` | Overrides the served path. | +| `allowedCIDRs` | `[]string` | `[]` (no bypass) | CIDR blocks whose source address may scrape without an admin key — e.g. `10.42.0.0/16` for a cluster's pod network. **Never use a whole RFC1918 range like `10.0.0.0/8`** — see [Metrics](#metrics)'s own warning for why that specific mistake is live-reproducible, not theoretical. | +| `modelLabel` | `bool` | `false` | Adds a `model` label to the provider attempt/failure counters, breaking them out per `(provider, model)` pair. Off by default: a deployment with hundreds of discovered model ids is a series-count explosion waiting to happen. | + ### `ModelPricing` | Field | Type | Semantics | @@ -1312,6 +1322,140 @@ or in CI. for asset responses to be the exception. The dashboard loads no external asset of any kind and works in an air-gapped cluster. +## Metrics + +A hand-written Prometheus text-exposition endpoint — the plugin is +stdlib-only under Yaegi, so it cannot use `client_golang` or any other +metrics library. Off by default (`metrics.enabled: false`, or the +`metrics` block omitted entirely): the route is not registered at all, +the same nil-disables convention [Admin](#admin) uses. Served at +`/metrics` by default, overridable via `metrics.path`. Works identically +with any scraper that speaks the standard text exposition format — +Prometheus with a `ServiceMonitor`/`PodMonitor`, or +[VictoriaMetrics](https://victoriametrics.com) with a `VMServiceScrape`; +the format is the same either way, only the CRD kind that points a +scraper at this endpoint differs, and that is entirely outside this +plugin's config. + +- **This endpoint exposes per-user and per-group cost and usage data** + (everything [Admin](#admin)'s `GET /admin/api/usage` exposes, in + Prometheus form), so it is never reachable unauthenticated by default. + Two ways in, either is sufficient on its own: + - **Admin bearer token** (the default, and always works): the identical + gate `GET /admin/api/*` uses — present a key belonging to a user + flagged `admin: true` as `Authorization: Bearer ` or + `x-api-key: `. Unauthenticated → 401; authenticated non-admin → + 403; admin → served. There is no second credential path — this reuses + `authStore.identify` exactly. + - **`metrics.allowedCIDRs`** (optional, off by default): CIDR blocks + whose **source address** may scrape with no key at all — for a + Prometheus/VictoriaMetrics server that should not need to carry a + credential. Checked against the request's source address exactly as + Traefik's own `net/http` server sees it (`RemoteAddr`, never a + client-supplied header like `X-Forwarded-For`) — **only meaningful for + a caller whose real source address actually survives to this + gateway**. Behind a Kubernetes `Service` or a second reverse proxy + that does not preserve the original address, every caller looks + identical to this check, and the allowlist becomes either "everyone" + or "no one" depending on what address arrives; confirm `RemoteAddr` + reflects the scraper's own address in your deployment before relying + on this instead of a key. + + **On CIDR sizing — this is the mistake that actually breaks security, + live-reproduced, not a theoretical warning**: always scope + `allowedCIDRs` to the **narrowest block that covers only your actual + scrapers** — a cluster's Prometheus/VictoriaMetrics pod network, e.g. + `10.42.0.0/16`. **Never configure a whole RFC1918 range** such as + `10.0.0.0/8`, `172.16.0.0/12`, or `192.168.0.0/16`. The failure mode is + not "the allowlist does not work" — it is "the allowlist works exactly + as configured, and what survives to `RemoteAddr` is an address you never + intended to trust": a SNAT-ing router or load balancer makes + **external** traffic arrive from an address inside that same private + range. `allowedCIDRs: ["10.0.0.0/8"]` paired with a router's own SNAT + address has been reproduced returning full per-user cost data to an + external caller with no key at all. + +- **`metrics.modelLabel`** (`false` by default) adds a `model` label to + the provider attempt/failure counters + (`llmgateway_provider_model_attempts_total`/ + `llmgateway_provider_model_failures_total`), breaking them out per + `(provider, model)` pair in addition to the always-on per-provider + totals. Left off by default because a deployment with hundreds of + discovered model ids across several providers is a series-count + explosion waiting to happen — enable it only when your scraper's + retention and cardinality budget can absorb one series per + `(provider, model)` pair actually seen. + +- **What's exposed** (families; see each one's own `# HELP` line in the + live output for the exact contract): + - `llmgateway_requests_total{scope_kind,scope_id}`, + `llmgateway_tokens_total{scope_kind,scope_id,direction}`, + `llmgateway_cost_micro_usd_total{scope_kind,scope_id}` — the + identical per-user/per-group/total accounting `GET /admin/api/usage` + already exposes as JSON, today's UTC-calendar-day window (resets at + UTC midnight). + - `llmgateway_budget_consumed_ratio{scope_kind,scope_id,budget}` — + fraction of a configured limit already consumed in its own window + (`0.0`–`1.0`+; above `1.0` means the limit has been breached), for + Prometheus's own alerting to threshold on. Emitted only for a scope + with that particular limit configured. + - `llmgateway_provider_attempts_total{provider}` / + `llmgateway_provider_failures_total{provider}` (+ opt-in + `_model_` variants, above) — the identical `attemptsDay`/`failuresDay` + counters `GET /admin/api/overview` already exposes. + - `llmgateway_provider_healthy{provider}` — `1`/`0` gauge from the + discovery circuit breaker (`healthState` in the admin API); see + [Provider health](#provider-health-discovery-circuit-breaker). + - `llmgateway_rate_limit_rejections_total{scope_kind,scope_id}` — how + many requests a rate or budget limit actually refused, by scope. This + one has no equivalent in the admin JSON API: nothing else tracks it. + +- **Aggregation across replicas — read this before wiring an alert**: with + `redis` configured, **every replica reads the SAME shared counters** + (the underlying key carries no per-instance component), so + `llmgateway_requests_total`, `llmgateway_tokens_total`, + `llmgateway_cost_micro_usd_total`, `llmgateway_budget_consumed_ratio`, + and both `llmgateway_provider_*_total` families are the + **gateway-wide total, identical on every replica's own `/metrics` + response**. Prometheus scrapes each replica as its own target, so + `sum(rate(llmgateway_requests_total[5m]))` (or any other `sum()`) across + a 3-replica deployment **triple-counts** — a cost alert built on `sum()` + fires at a third of its intended threshold. **Aggregate these with + `max()`, never `sum()`.** Without `redis` (the in-process fallback), the + identical families become genuinely **per-replica** instead — each + replica counts only the traffic it personally handled — and `sum()` + becomes the correct aggregation instead. This flips on that one config + bit; know which one you are running before wiring an alert. Two + families do **not** follow this rule: + - `llmgateway_rate_limit_rejections_total` lives only in each replica's + own process memory, **never** in Redis, regardless of the `redis` + config — `sum()` is always correct for it. + - `llmgateway_provider_healthy` reflects each replica's own, + independently-running discovery circuit breaker — never shared via + Redis either — so two replicas can legitimately disagree about one + provider's health at the same instant. Read it per-instance where + possible; if you must aggregate, `min()` surfaces "at least one + replica sees this provider as unhealthy" — `sum()` is meaningless for + a 0/1 gauge. + +- **Labels are escaped**: a label value that is user- or + upstream-controlled (a user/group name, a model id) is escaped for + backslash, double quote, and newline, byte-for-byte — the exact rule + the text-exposition format requires. This matters more than it looks: + Prometheus reports an unparseable scrape as the **whole target being + down**, not as one bad line, so a single unescaped quote in a model id + would otherwise take the entire endpoint offline from Prometheus's point + of view, not just that one series. + +- **A store outage never fabricates a zero**: if the configured Redis is + unreachable and `redis.failOpen` is `false`, an affected scope's + request/token/cost/provider counters are **skipped entirely** for that + scrape rather than reported as `0` — a temporary gap in the series is + the honest signal; a fabricated `0` would read to Prometheus as a + counter reset followed by a phantom spike the instant the store + recovers. `llmgateway_provider_healthy` is unaffected either way — it + never reads from the store. + ## MCP and A2A - **Registry** (group-filtered, sorted by name): diff --git a/examples/kubernetes.yaml b/examples/kubernetes.yaml index 4f17733..780dbf0 100644 --- a/examples/kubernetes.yaml +++ b/examples/kubernetes.yaml @@ -125,6 +125,23 @@ spec: # ../README.md#admin. admin: enabled: false + # Prometheus text-exposition endpoint — see ../README.md#metrics. + # allowedCIDRs is the cluster's OWN pod network (a placeholder here; + # replace 10.42.0.0/16 with your cluster CIDR, e.g. `kubectl get + # nodes -o jsonpath='{.items[0].spec.podCIDR}'`), letting an + # in-cluster scraper (VMServiceScrape/ServiceMonitor below) hit + # /metrics with no credential. NEVER widen this to a whole RFC1918 + # range like 10.0.0.0/8 — see ../README.md#metrics for why that + # specific mistake is live-reproducible, not theoretical: a + # SNAT-ing router or load balancer makes EXTERNAL traffic arrive + # from an address inside that same private range too. The + # dashboard-admin key above (users.inline) works here regardless, + # for a scraper that lives outside allowedCIDRs. + metrics: + enabled: true + allowedCIDRs: + - 10.42.0.0/16 + modelLabel: false pricing: gpt-5-mini: {inputPerM: 0.25, outputPerM: 2.0} mcpServers: @@ -193,6 +210,42 @@ spec: - name: llmgateway-dummy-backend port: 80 +--- +# Scraping /metrics (../README.md#metrics): this cluster runs +# VictoriaMetrics, not Prometheus, so the CRD is VMServiceScrape +# (victoriametrics/operator), not a Prometheus-operator ServiceMonitor — +# the exposition FORMAT this plugin emits is identical either way (a +# ServiceMonitor with the same selector/port/path works unchanged if +# your cluster runs Prometheus instead); only the CRD kind differs, and +# that is entirely outside this plugin's own config. +# +# IMPORTANT: /metrics is served by TRAEFIK ITSELF, not by +# llmgateway-dummy-backend above — this middleware intercepts the path +# before any backend service is ever reached (same as every other route +# this plugin owns). Point this scrape at YOUR Traefik installation's own +# Service (commonly named "traefik" — verify against your own Helm +# release), never at a backend service this file defines. Adjust +# selector/namespaceSelector/port to match your Traefik Service's real +# labels and named port. +apiVersion: operator.victoriametrics.com/v1beta1 +kind: VMServiceScrape +metadata: + name: llmgateway-metrics + namespace: traefik +spec: + namespaceSelector: + matchNames: + - traefik + selector: + matchLabels: + app.kubernetes.io/name: traefik + endpoints: + - port: websecure # the Traefik Service's own named entryPoint port llmgateway is exposed on + path: /metrics # the default; matches metrics.path left unset above + scheme: https + tlsConfig: + insecureSkipVerify: true # adjust for your cluster's own CA/cert setup + --- # Helm values snippet (traefik/traefik-helm-chart) — illustrative, verify # field names against the chart version you deploy. Two things are From 47e22e627cd3537666c496d5b2317ca46db828c9 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 01:56:20 +0100 Subject: [PATCH 7/8] fix(metrics): close the default-config store-down gap, fix escaping Three blocking fixes from a second round of adversarial verification, plus two should-fix items bundled in since they touch the same code. storeDown detection covered only the fail-CLOSED path. failOpen defaults to true (newConfiguredLimiter), so on the shipped default a broken store made storeGetMulti return ok=true after silently reading the empty in-process fallback, bypassing every storeDown guard in metrics.go. Add limiter.configuredStoreDown, checked alongside ok in currentUsage and providerUsage, so a fail-open read served from the fallback is treated the same as a fail-closed refusal. The pinning test previously hand-set failOpen=false, which meant it only ever exercised the case that was already correct; it now leaves failOpen at its true default. The escaping rewrite from the previous round achieved injectivity by copying an invalid UTF-8 byte through raw, which can make the output itself invalid UTF-8. Measured against a real Prometheus scrape parser this round: invalid UTF-8 in a label value is fatal, the scrape target goes down, which is worse than the duplicate series the previous round was fixing (measured non-fatal, dropped and logged, target stays up). Replace an invalid byte with a deterministic \xHH marker instead, proven injective and always valid UTF-8 by an exhaustive test over every 1- and 2-byte input, not merely spot-checked. Also adds the fast path a benchmark showed the byte-oriented rewrite needed: unescaped, already-valid-UTF-8 input, the common case, returns unchanged with zero allocations instead of paying a full copy. llmgateway_rate_limit_rejections_total's HELP text claimed it "resets only on restart", which stopped being true once the previous round added generation-rotation bounding and reload-triggered eviction: a scope resets when its user is removed, and every tracked scope resets together in bulk the instant the tracked-scope cap is exceeded. The HELP text and README now say so. Adds llmgateway_limit_store_up, a per-replica gauge for this process's own connection health to the configured limit store: with the store flapping faster than its fail latch, every store-backed family can go silent scrape after scrape with nothing on the endpoint itself saying why. Also adds a test pinning that Gateway.pruneRejectionsAfterReload runs only when authStore.maybeReload reports a real reload, never on every request. --- README.md | 79 +++++++++++++---- limits.go | 38 +++++++- metrics.go | 181 +++++++++++++++++++++++++++++++------- metrics_test.go | 225 +++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 441 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 3146250..d98ee47 100644 --- a/README.md +++ b/README.md @@ -1406,9 +1406,26 @@ plugin's config. - `llmgateway_provider_healthy{provider}` — `1`/`0` gauge from the discovery circuit breaker (`healthState` in the admin API); see [Provider health](#provider-health-discovery-circuit-breaker). + - `llmgateway_limit_store_up` — `1`/`0` gauge for this replica's own + connection to the configured limit store (Redis); absent entirely + when no store is configured. Every store-backed family above simply + goes silent, scope by scope, for whatever it cannot currently read — + the honest choice for those families, but on its own that gives a + store outage no independent signal on this endpoint (with + `redis.failOpen: false` and a store flapping faster than its 5s fail + latch, every store-backed family can be absent scrape after scrape + with nothing else here saying why). This gauge is that signal. - `llmgateway_rate_limit_rejections_total{scope_kind,scope_id}` — how many requests a rate or budget limit actually refused, by scope. This one has no equivalent in the admin JSON API: nothing else tracks it. + It does **not** reset only on process restart: a scope's own count + also resets to `0` when that user is removed via a hot-reloaded users + file, and **every** tracked scope resets together, in one bulk cliff, + the instant more than 2000 distinct scopes have ever been seen in + this process's lifetime (a bounded-memory safeguard, not a graceful + per-scope decay). A value that unexpectedly drops is one of these + events, not a scrape anomaly — see the live `# HELP` line for the + exact current threshold. - **Aggregation across replicas — read this before wiring an alert**: with `redis` configured, **every replica reads the SAME shared counters** @@ -1425,7 +1442,7 @@ plugin's config. identical families become genuinely **per-replica** instead — each replica counts only the traffic it personally handled — and `sum()` becomes the correct aggregation instead. This flips on that one config - bit; know which one you are running before wiring an alert. Two + bit; know which one you are running before wiring an alert. Three families do **not** follow this rule: - `llmgateway_rate_limit_rejections_total` lives only in each replica's own process memory, **never** in Redis, regardless of the `redis` @@ -1437,24 +1454,41 @@ plugin's config. possible; if you must aggregate, `min()` surfaces "at least one replica sees this provider as unhealthy" — `sum()` is meaningless for a 0/1 gauge. + - `llmgateway_limit_store_up` reflects each replica's own, locally + -discovered connection health to the configured store — it cannot + itself be read from the store it describes, so it is inherently + per-replica too. Same guidance as `llmgateway_provider_healthy`: + prefer per-instance, `min()` if you must aggregate. - **Labels are escaped**: a label value that is user- or upstream-controlled (a user/group name, a model id) is escaped for - backslash, double quote, and newline, byte-for-byte — the exact rule - the text-exposition format requires. This matters more than it looks: - Prometheus reports an unparseable scrape as the **whole target being - down**, not as one bad line, so a single unescaped quote in a model id - would otherwise take the entire endpoint offline from Prometheus's point - of view, not just that one series. + backslash, double quote, and newline — the exact rule the + text-exposition format requires. This matters more than it looks: an + unescaped quote or newline breaks the exposition format's own lexical + structure (an unterminated quoted string), and Prometheus reports an + unparseable scrape as the **whole target being down**, not as one bad + line — so a single unescaped quote in a model id would otherwise take + the entire endpoint offline from Prometheus's point of view, not just + that one series. A raw, invalid UTF-8 byte is escaped too, as a + deterministic `\xHH` marker rather than passed through unchanged: an + invalid-UTF-8 label value is independently fatal to a real Prometheus + scrape (measured against `promparse.go`), so the escaping keeps every + distinct input distinguishable (no two different names collapse onto + one series) while guaranteeing the output stays valid UTF-8 either way. - **A store outage never fabricates a zero**: if the configured Redis is - unreachable and `redis.failOpen` is `false`, an affected scope's - request/token/cost/provider counters are **skipped entirely** for that - scrape rather than reported as `0` — a temporary gap in the series is - the honest signal; a fabricated `0` would read to Prometheus as a - counter reset followed by a phantom spike the instant the store - recovers. `llmgateway_provider_healthy` is unaffected either way — it - never reads from the store. + unreachable, an affected scope's request/token/cost/provider counters + are **skipped entirely** for that scrape rather than reported as `0` — + a temporary gap in the series is the honest signal; a fabricated `0` + would read to Prometheus as a counter reset followed by a phantom spike + the instant the store recovers. This holds regardless of + `redis.failOpen`: with `failOpen: false` (or unset) the request itself + is refused during the outage, and with `failOpen: true` (the default) + requests keep flowing against the in-process fallback, but `/metrics` + still treats the affected scopes as unreadable rather than presenting + the fallback's own, non-authoritative zero as real gateway-wide usage. + `llmgateway_provider_healthy` is unaffected either way — it never reads + from the store. ## MCP and A2A @@ -1586,6 +1620,11 @@ plugin's config. envelope. A successful attempt logs the resolved user's name and the route. Neither line ever includes the presented API key — only the identity it resolved to, never the key material itself. +- **`GET /metrics` (see [Metrics](#metrics)) exposes per-user and + per-group cost and usage data**, the identical sensitivity as the admin + dashboard's JSON API, so it is gated the same way: an admin bearer + token by default, with an optional, narrowly-scoped `allowedCIDRs` + bypass for a co-located scraper — never leave it reachable with neither. ## Anonymous usage reporting @@ -1741,8 +1780,11 @@ struct — every field in that example round-trips through `New()` cleanly), a dummy backing `Deployment`/`Service` (Traefik requires a real router backend even though this middleware never actually reaches it — see [Quickstart](#quickstart)), an `IngressRoute` wiring it all to a -host, and a Traefik Helm values snippet for loading the plugin and -wiring provider keys in from Secrets via environment variables. +host, a second, Host-free `IngressRoute` plus a `VMServiceScrape` for +scraping [`/metrics`](#metrics) (a `ServiceMonitor` works the same way on +a Prometheus-operator cluster — only the CRD kind differs), and a Traefik +Helm values snippet for loading the plugin and wiring provider keys in +from Secrets via environment variables. The short version: @@ -1758,6 +1800,11 @@ The short version: - Point `redis.address` at your cluster's existing Redis/Valkey/Dragonfly rather than standing up a dedicated instance for this plugin — the distributed limiter is a client, not a datastore. +- [`/metrics`](#metrics) is served by Traefik itself, not by any backend + Service, so scraping it needs its own Host-free route (see the example + manifest's own comment on this — a Host-restricted router alone 404s an + in-cluster scrape) and its own CIDR/admin-key auth — never point a + scraper at it without one or the other configured. ### Config hot-reload diff --git a/limits.go b/limits.go index 25750db..9f05ced 100644 --- a/limits.go +++ b/limits.go @@ -871,6 +871,29 @@ func (l *limiter) storeLatched() bool { return !l.lastStoreFailure.IsZero() && l.now().Sub(l.lastStoreFailure) < storeDownLatchFor } +// configuredStoreDown reports whether l has a configured store that is +// currently within its failure latch (storeLatched) — meaning any read +// made right now is served from the in-process fallback, never the +// real, shared store, REGARDLESS of what storeGetMulti's own ok return +// says. l.store == nil (no store configured at all — a fallback-only +// deployment) always reports false: there, the fallback IS the source +// of truth by design, nothing has degraded. +// +// This exists because storeGetMulti's ok=true does not distinguish "the +// store answered for real" from "the store is down, failOpen fell back +// to the empty in-process fallback, which answered with zeros" — and +// failOpen defaults to true (newConfiguredLimiter, llmgateway.go). A +// caller that only checks ok, as currentUsage/providerUsage's callers +// used to, reports a fail-OPEN store outage as confirmed real usage: +// exactly the fabricated-counter-reset bug metrics.go's storeDown skip +// was built to prevent, except on the DEFAULT config, where it was +// never actually reachable (review fix, adversarial verification +// 2026-08-23) — see currentUsage/providerUsage's own doc comments for +// where this is now checked. +func (l *limiter) configuredStoreDown() bool { + return l.store != nil && l.storeLatched() +} + // recordStoreFailure logs err (rate-limited, see logStoreError) and opens // the store-down latch: every storeIncrBy/storeGet call for the next // storeDownLatchFor skips the network call and applies the fail-open/ @@ -1819,7 +1842,10 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { } vals, ok := l.storeGetMulti(allKeys) - if !ok || len(vals) != len(allKeys) { + // configuredStoreDown catches the fail-open case ok alone misses + // (review fix, adversarial verification 2026-08-23) — see + // currentUsage's identical check and its own doc comment for why. + if !ok || len(vals) != len(allKeys) || l.configuredStoreDown() { for i := range out { out[i] = providerCounters{storeDown: true} } @@ -1906,6 +1932,14 @@ func usageWindowKeys(sc limitScope, now time.Time) []string { // every enforcement read already does, and it is read-only: unlike // checkAndCount, it never increments anything. // +// storeDown is also set when configuredStoreDown reports the store +// currently latched (review fix, adversarial verification 2026-08-23): +// with failOpen true (the default), storeGetMulti's own ok stays true +// on a store outage — it silently reads the in-process fallback instead +// — so ok alone is not enough to tell a real reading apart from a +// fail-open one served from an empty fallback. Checked in addition to, +// never instead of, storeGetMulti's own ok/length checks below. +// // ONE storeGetMulti call for the whole scopes slice (v0.2 final review // wave, 2026-08-20; supersedes the "one call per scope" amendment this // comment previously described, 2026-08-20 review): every scope's keys @@ -1947,7 +1981,7 @@ func (l *limiter) currentUsage(scopes []limitScope) []scopeUsage { // guarded defensively so a future or test-only store's short slice // reports every scope storeDown instead of panicking on an // out-of-range index below (review sweep, 2026-08-20). - if !ok || len(vals) != len(allKeys) { + if !ok || len(vals) != len(allKeys) || l.configuredStoreDown() { for i, sc := range scopes { out[i] = scopeUsage{kind: sc.kind, id: sc.id, storeDown: true} } diff --git a/metrics.go b/metrics.go index 723fe46..2f40a21 100644 --- a/metrics.go +++ b/metrics.go @@ -7,6 +7,8 @@ import ( "net/http" "sort" "strconv" + "strings" + "unicode/utf8" ) // MetricsConfig configures the Prometheus text-exposition endpoint @@ -178,41 +180,94 @@ func (g *Gateway) serveMetrics(w http.ResponseWriter) { // double quote becomes \", and newline becomes \n — the exact rule the // format requires. An unescaped quote or embedded newline in a label // value (a user or group name, a model id, an operator picks these) would -// otherwise emit an unparseable exposition document — and Prometheus -// reports an unparseable scrape as the WHOLE target being DOWN, not as -// one bad line, so this is a correctness requirement, not a formatting -// nicety. +// otherwise emit an unparseable exposition document, which is fatal to a +// real scrape — see the CORRECTNESS section below for exactly how, and +// for a claim this comment previously made that turned out to be wrong. // -// This walks s BYTE by byte and classifies each one independently, never -// a sequential find-and-replace pass: replacing quotes first and -// backslashes second (or vice versa) would double-escape a backslash -// this function itself just inserted — a real, classic bug class for -// this exact kind of escaping. A single forward pass has no such -// ordering hazard. +// Two independent correctness properties, both required, are in tension +// here and this function's whole shape exists to satisfy both at once: // -// Byte-oriented, deliberately NOT `for _, r := range s` (review fix, -// adversarial verification 2026-08-23): ranging over a string decodes -// UTF-8 and substitutes U+FFFD for every invalid byte, which makes the -// function NOT injective — two distinct configured names that both -// contain invalid UTF-8 (a byte sequence auth.go's buildEntry never -// rejects; it validates non-empty only, see its own doc comment) can -// decode to the identical U+FFFD run and render as the SAME escaped -// label value. Two scope_id values colliding onto one series is a -// duplicate-series scrape failure — confirmed against a real Prometheus -// scrape parser: the whole target reports DOWN, and promtool alone will -// not catch it, since expfmt silently dedupes. Escaping byte-for-byte -// passes every raw byte through unchanged except the three ASCII -// characters requiring escaping ('\\', '"', '\n') — none of which can -// ever appear as a UTF-8 continuation byte (continuation bytes are -// always >= 0x80), so this never misinterprets a multi-byte sequence's -// internal bytes as one of these three, and every distinct input byte -// string still maps to a distinct output. It also avoids WriteRune's -// per-rune decode cost on a large value. +// 1. INJECTIVE: two distinct inputs must never produce the same escaped +// output. A collision means two distinct scope_id/model/provider +// values render as the identical label value — a duplicate series. +// 2. VALID UTF-8 OUT: the output must always be valid UTF-8, regardless +// of whether s is. +// +// CORRECTNESS (review fixes, adversarial verification 2026-08-23, two +// rounds): round 1 walked s byte-by-byte and copied every non-special +// byte through verbatim, including an invalid one — injective (property +// 1 held), but an invalid byte copied through means the OUTPUT can itself +// be invalid UTF-8, which fails property 2. Measured this round against +// prometheus/prometheus's real scrape path (promparse.go): an invalid- +// UTF-8 label value is FATAL — the parser errors, the scrape loop breaks, +// and Prometheus marks the target up=0 (DOWN). That is a strictly worse +// failure than the one round 1 was fixing: the doc comment at the time +// claimed a duplicate series is ALSO fatal ("the whole target reports +// DOWN"); measured properly this round against scrape.go, that claim was +// wrong — checkAddError treats a duplicate sample as non-fatal (returns +// false, nil), bumps a counter, and the scrape still succeeds with +// up=1. So the ORIGINAL rune-based version (pre-round-1) was lossy but +// safe: colliding invalid-UTF-8 inputs onto one U+FFFD-bearing series +// dropped data but never took the target down. Round 1's byte-oriented +// version was lossless but unsafe: correct in the one dimension it +// measured, worse in the one it did not. +// +// This version keeps both: valid runes (including a genuine, already- +// valid 3-byte-encoded U+FFFD that was actually present in s) pass +// through unchanged via utf8.DecodeRuneInString, which reports both the +// rune and how many bytes it consumed; only a TRULY undecodable single +// byte (RuneError with a reported width of 1 — DecodeRuneInString's own +// documented signal that byte could not start any valid encoding, as +// opposed to width 3 for a real U+FFFD) is replaced with \xHH, HH being +// that byte's own hex value — recoverable, so distinct invalid bytes can +// never collide, and it can never appear from the normal escape path +// either: every literal backslash in s is ASCII and therefore always +// decodes as its own valid 1-byte rune, which the switch below always +// doubles to \\ — a lone, undoubled backslash in the output can +// therefore only ever be this marker, never user-typed text. Reachability +// caveat: no live config path is known to reach the invalid-UTF-8 branch +// today (encoding/json, which every user/group name currently decodes +// through, coerces invalid UTF-8 to U+FFFD before this function ever +// sees it) — this is closing a latent gap, not a currently-exploitable +// one, but the SAME property (injective, always-valid-UTF-8 output) is +// what a future caller — an upstream-reported model id read some other +// way, say — would need without re-deriving this reasoning. +// +// Fast path (item 5, adversarial verification round 2, benchmarked): +// nearly every real user/group/model name needs none of the above — no +// backslash/quote/newline, already valid UTF-8 (true of every Go string +// literal and every encoding/json-decoded value by construction) — so +// skip the walk and its allocation entirely when s is already its own +// answer. Benchmarked over 7 representative values: the byte-oriented +// slow path alone cost 318.8 ns/op, 560 B/op, 14 allocs/op (worse than +// the original rune-based version's 344.7 ns, 120 B, 7 allocs on +// allocations specifically, from bytes.Buffer.String() copying where +// strings.Builder.String() does not — fixed below by switching to +// strings.Builder too); the fast path measures 84.4 ns/op, 0 B, 0 +// allocs/op. At 1,000 configured users that is the difference between +// roughly 20,000 allocations and 800 KB of garbage per scrape, inside +// the shared Traefik process, and zero. func escapeLabelValue(s string) string { - var b bytes.Buffer + if !strings.ContainsAny(s, "\\\"\n") && utf8.ValidString(s) { + return s + } + + var b strings.Builder b.Grow(len(s)) - for i := 0; i < len(s); i++ { - switch c := s[i]; c { + for i := 0; i < len(s); { + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size == 1 { + // A genuinely undecodable byte — see this function's own + // CORRECTNESS section above for why \xHH, not � and not + // a raw copy, is the only choice that keeps both required + // properties. + b.WriteString(`\x`) + b.WriteByte(hexDigit(s[i] >> 4)) + b.WriteByte(hexDigit(s[i] & 0x0f)) + i++ + continue + } + switch r { case '\\': b.WriteString(`\\`) case '"': @@ -220,12 +275,27 @@ func escapeLabelValue(s string) string { case '\n': b.WriteString(`\n`) default: - b.WriteByte(c) + // A valid rune, 1-4 bytes: none of the three special ASCII + // characters above can appear as any byte of a valid + // multi-byte encoding (every byte of one is >= 0x80), so a + // verbatim copy of s[i:i+size] can never hide an unescaped + // backslash/quote/newline inside it. + b.WriteString(s[i : i+size]) } + i += size } return b.String() } +// hexDigit returns the lowercase hex digit for the low nibble of n +// (0x0-0xf) — escapeLabelValue's own \xHH marker for an undecodable +// byte, written without fmt.Sprintf to keep that (already-rare) branch +// allocation-free too. +func hexDigit(n byte) byte { + const digits = "0123456789abcdef" + return digits[n&0x0f] +} + // metricLabel is one label name/value pair a metricWriter sample carries. type metricLabel struct { name string @@ -323,6 +393,14 @@ const aggregationNotePerProcess = " AGGREGATION ACROSS REPLICAS: this counter li // instant (e.g. one mid-backoff, one already recovered). const aggregationNoteProviderHealthy = " AGGREGATION ACROSS REPLICAS: each replica runs its own discovery circuit breaker in-process, never shared via Redis, so this can legitimately differ per replica. Read it per-instance where possible; if you must aggregate, use min() to surface \"at least one replica sees this provider as unhealthy\" — sum() is meaningless for a 0/1 gauge." +// aggregationNoteStoreHealth is appended to llmgateway_limit_store_up's +// HELP text: whether THIS replica's own connection to the configured +// limit store is currently healthy is, by definition, discovered +// locally (limiter.storeLatched, limits.go) — it cannot itself be read +// from the store being described, so — like provider health above — +// it is inherently per-replica, never shared via Redis. +const aggregationNoteStoreHealth = " AGGREGATION ACROSS REPLICAS: each replica discovers its own connection health to the configured store independently — this cannot itself be read from the store being described — so it can legitimately differ per replica. Read it per-instance where possible; if you must aggregate, use min() to surface \"at least one replica currently sees the store as down\" — sum() is meaningless for a 0/1 gauge." + // renderMetrics builds the full Prometheus text-exposition document for // GET . Every read below goes through the SAME batched, // chunked accessors GET /admin/api/overview and GET /admin/api/usage @@ -338,6 +416,7 @@ func (g *Gateway) renderMetrics() []byte { g.writeUsageMetrics(&m) g.writeProviderMetrics(&m) g.writeRejectionMetrics(&m) + g.writeStoreHealthMetrics(&m) return m.buf.Bytes() } @@ -556,13 +635,49 @@ func (g *Gateway) writeRejectionMetrics(m *metricWriter) { }) m.family("llmgateway_rate_limit_rejections_total", "counter", - "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed — it resets only on restart."+aggregationNotePerProcess) + fmt.Sprintf( + "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed, but it does NOT reset only on restart: a scope's own count resets to 0 when that user is removed via a hot-reloaded users file, and every tracked scope resets together, in one bulk cliff (not a graceful per-scope decay), the instant more than %d distinct scopes have ever been seen — a value that drops is one of these events, not a scrape anomaly.", + rejectionCounterMapCap, + )+aggregationNotePerProcess) for _, s := range snaps { m.sampleInt("llmgateway_rate_limit_rejections_total", []metricLabel{{"scope_kind", s.kind}, {"scope_id", s.id}}, s.count) } } +// writeStoreHealthMetrics emits llmgateway_limit_store_up (item 7, +// adversarial verification 2026-08-23): every store-backed family above +// simply goes silent for the scopes it cannot read during an outage +// (writeUsageMetrics/writeProviderMetrics's own storeDown skip) — the +// honest choice for THOSE families (see their own doc comments), but it +// means a store outage has NO signal of its own on this endpoint: with +// failOpen false and a store flapping faster than the fail latch +// (storeDownLatchFor, limits.go), every store-backed family can simply +// be ABSENT, scrape after scrape, with nothing on /metrics itself +// saying why — an operator needs live traffic AND a fail-closed config +// to even notice. This gauge exists to be the "why are my other +// families missing" signal on its own, independent of traffic volume or +// failOpen. +// +// Absent entirely when no limit store is configured at all (a fallback- +// only deployment, limiter.redisStatus' own "configured" return) — +// there is no store health to report there; the fallback is always +// "up" by definition, a trivial, uninteresting reading not worth a +// series. +func (g *Gateway) writeStoreHealthMetrics(m *metricWriter) { + configured, _, _ := g.limiter.redisStatus() + if !configured { + return + } + up := int64(1) + if g.limiter.configuredStoreDown() { + up = 0 + } + m.family("llmgateway_limit_store_up", "gauge", + "1 when this replica's own connection to the configured limit store (Redis) is currently healthy, 0 when it is latched down after a recent failure (limiter.storeLatched, limits.go). A store-backed family (llmgateway_requests_total and the rest) going silent for one or more scrapes in a row, with this gauge at 0, is the store outage those families' own storeDown skip is deliberately quiet about."+aggregationNoteStoreHealth) + m.sampleInt("llmgateway_limit_store_up", nil, up) +} + // pruneRejectionsAfterReload evicts every "user"-kind rejection scope // (limiter.pruneRejections, limits.go) whose name is no longer among // g.auth's currently active users — ServeHTTP's own call site diff --git a/metrics_test.go b/metrics_test.go index 08214a5..efd9055 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" "time" + "unicode/utf8" ) // newMetricsTestConfig builds a Config with Metrics enabled at the default @@ -386,7 +387,12 @@ func TestMetrics_OutputParsesAsValidExposition(t *testing.T) { cfg.Metrics.ModelLabel = true cfg.Providers["openai"].BaseURL = srv.URL cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerMinute: 1} - h, _ := newMetricsGatewayHandle(t, cfg) + h, gw := newMetricsGatewayHandle(t, cfg) + // A configured (if in-process) store, so llmgateway_limit_store_up + // (item 7) also has a sample to check structure against below — + // newMemoryStore behaves identically to the fallback it stands in + // for, so this changes nothing else about this test's traffic. + gw.limiter.store = newMemoryStore() body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} // First request: admitted, and succeeds against the local httptest @@ -462,6 +468,7 @@ func TestMetrics_OutputParsesAsValidExposition(t *testing.T) { "llmgateway_provider_model_attempts_total", "llmgateway_provider_model_failures_total", "llmgateway_rate_limit_rejections_total", + "llmgateway_limit_store_up", } for _, name := range wantFamilies { if _, ok := types[name]; !ok { @@ -487,12 +494,20 @@ func TestEscapeLabelValue(t *testing.T) { {"backslash_then_quote", `\"`, `\\\"`}, {"all_three", "a\"b\\c\nd", `a\"b\\c\nd`}, // Invalid UTF-8 bytes (0x80/0x81 are bare continuation bytes, - // never valid as the START of any UTF-8 sequence) must pass - // through UNCHANGED, byte-for-byte — see - // TestEscapeLabelValue_InvalidUTF8_Injective below for why this - // matters beyond just "does not crash". - {"invalid_utf8_lone_0x80", "u\x80", "u\x80"}, - {"invalid_utf8_lone_0x81", "u\x81", "u\x81"}, + // never valid as the START of any UTF-8 sequence) must become a + // deterministic, recoverable \xHH marker — see + // TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8 below for + // why this is the only choice satisfying both required + // properties (injective, and always valid UTF-8 out). + {"invalid_utf8_lone_0x80", "u\x80", `u\x80`}, + {"invalid_utf8_lone_0x81", "u\x81", `u\x81`}, + // A literal, user-typed "\x80" (five ordinary ASCII characters, + // not a raw invalid byte) must render distinguishably from the + // marker above: its own backslash gets doubled like any other, + // producing \\x80 (six characters) rather than colliding with + // the four-character \x80 marker an actual invalid byte 0x80 + // produces. + {"literal_backslash_x80_text", `\x80`, `\\x80`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -504,27 +519,71 @@ func TestEscapeLabelValue(t *testing.T) { } } -// TestEscapeLabelValue_InvalidUTF8_Injective is the mutation-proofing -// test for the byte-oriented rewrite (review fix, adversarial -// verification 2026-08-23): a rune-oriented `for _, r := range s` -// implementation decodes invalid UTF-8 to U+FFFD, which makes escaping -// NOT injective — "u\x80" and "u\x81" are two distinct, real inputs -// (auth.go's buildEntry validates a user name as non-empty only, never -// restricted to valid UTF-8) that would both render as "u�" under -// that approach, colliding two distinct scope_id label values onto one -// series — confirmed against a real Prometheus scrape parser: a -// duplicate series, and the whole scrape target reported DOWN. The -// byte-oriented implementation must keep every input byte-for-byte -// distinguishable. -func TestEscapeLabelValue_InvalidUTF8_Injective(t *testing.T) { +// TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8 is the +// mutation-proofing test for the two-round rewrite (review fixes, +// adversarial verification 2026-08-23, round 2 correcting round 1): +// +// - Round 1 fixed a rune-oriented `for _, r := range s` implementation, +// which decodes invalid UTF-8 to U+FFFD and is therefore NOT +// injective: "u\x80" and "u\x81" both render as "u�", colliding two +// distinct scope_id values onto one series (a duplicate series). +// - Round 1's own fix (copying an invalid byte through raw) achieved +// injectivity but broke the OTHER required property: the OUTPUT is +// then itself invalid UTF-8. Measured against prometheus/prometheus's +// real scrape parser (promparse.go) this round: that is FATAL — the +// scrape errors out entirely (target reported DOWN), which is worse +// than a duplicate series (measured non-fatal against scrape.go: +// dropped, target stays up). Round 1's own doc comment asserted the +// opposite — that a duplicate series is what takes a target down — +// which was the wrong premise, corrected here. +// +// This test pins both properties at once, since a fix for either one +// alone (as both prior rounds separately proved) is not sufficient. +func TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8(t *testing.T) { t.Parallel() a := escapeLabelValue("u\x80") b := escapeLabelValue("u\x81") if a == b { - t.Fatalf("escapeLabelValue(%q) == escapeLabelValue(%q) == %q — two distinct inputs collided onto one label value", "u\x80", "u\x81", a) + t.Fatalf("escapeLabelValue(%q) == escapeLabelValue(%q) == %q — two distinct inputs collided onto one label value (not injective)", "u\x80", "u\x81", a) } - if strings.Contains(a, "�") || strings.Contains(b, "�") { - t.Errorf("escaped output contains U+FFFD (replacement character) — the rune-decoding bug this test guards against: a=%q b=%q", a, b) + if !utf8.ValidString(a) { + t.Errorf("escapeLabelValue(%q) = %q is not valid UTF-8 — fatal to a real Prometheus scrape (promparse.go), not merely a duplicate series", "u\x80", a) + } + if !utf8.ValidString(b) { + t.Errorf("escapeLabelValue(%q) = %q is not valid UTF-8 — fatal to a real Prometheus scrape (promparse.go), not merely a duplicate series", "u\x81", b) + } +} + +// TestEscapeLabelValue_Exhaustive1And2Byte_InjectiveAndValidUTF8 proves +// both required properties by construction rather than by spot-check, +// over every possible 1-byte string (256) and every possible 2-byte +// string (65,536) — valid UTF-8, invalid UTF-8, and every ASCII special +// character alike — all checked against ONE shared map, so a collision +// BETWEEN a 1-byte and a 2-byte input (the escaped output is not +// length-preserving, so this is not automatically ruled out just +// because the inputs differ in length) would be caught too, not only a +// same-length collision. Fast enough (well under a second) to run on +// every test invocation, not just as a one-off manual verification. +func TestEscapeLabelValue_Exhaustive1And2Byte_InjectiveAndValidUTF8(t *testing.T) { + t.Parallel() + seen := make(map[string]string, 256+65536) + check := func(in string) { + out := escapeLabelValue(in) + if !utf8.ValidString(out) { + t.Fatalf("escapeLabelValue(%q) = %q is not valid UTF-8", in, out) + } + if prior, ok := seen[out]; ok && prior != in { + t.Fatalf("collision: escapeLabelValue(%q) and escapeLabelValue(%q) both produced %q", prior, in, out) + } + seen[out] = in + } + for b := 0; b < 256; b++ { + check(string([]byte{byte(b)})) + } + for a := 0; a < 256; a++ { + for b := 0; b < 256; b++ { + check(string([]byte{byte(a), byte(b)})) + } } } @@ -759,13 +818,26 @@ func TestMetrics_ProviderHealthy_ReflectsOpenBreaker(t *testing.T) { // proofing test for every `if su.storeDown { continue }` / // `if mc.storeDown { continue }` guard in metrics.go: deleting any of // them makes this test fail. Forces the SAME limiter every family in -// this file reads from into a fail-closed store outage (alwaysErrStore, -// failOpen=false — routes_unified_test.go's own stub), then asserts -// every store-backed family emits NO samples at all rather than a -// fabricated 0 (review fix, adversarial verification 2026-08-23: -// without the guards, a Redis blip reads as a hard 0 mid-outage, then -// jumps back to the real total on recovery — Prometheus treats that as -// a counter reset followed by a spurious full-total "increase"). +// this file reads from into a store outage — a real, configured store +// (alwaysErrStore, routes_unified_test.go's own stub) that errors on +// every call — then asserts every store-backed family emits NO samples +// at all rather than a fabricated 0 (review fix, adversarial +// verification 2026-08-23: without the guards, a Redis blip reads as a +// hard 0 mid-outage, then jumps back to the real total on recovery — +// Prometheus treats that as a counter reset followed by a spurious +// full-total "increase"). +// +// Deliberately does NOT touch gw.limiter.failOpen (round 2, adversarial +// verification 2026-08-23): newConfiguredLimiter (llmgateway.go) +// defaults it to true, and a round-1 version of this test hand-set it +// to false, which meant the suite only ever exercised the fail-CLOSED +// path — the one case where storeGetMulti's own ok=false already made +// the bug impossible to miss. The actual production default (failOpen +// true) silently read zeros from the empty in-process fallback and +// reported ok=true, bypassing every storeDown guard in this file +// entirely; that is the path this test now pins, matching +// limiter.configuredStoreDown's own doc comment (limits.go). +// // llmgateway_provider_healthy is deliberately excluded from the "must be // absent" list: it comes from the in-process discovery breaker, never // the counterStore, so a store outage must NOT affect it — asserted @@ -777,8 +849,13 @@ func TestMetrics_StoreDown_SkipsRatherThanFabricatesZero(t *testing.T) { cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerDay: 100} h, gw := newMetricsGatewayHandle(t, cfg) + // Simulates "Redis was configured and reachable, then broke" without + // standing up a real broken Redis connection: gw.limiter.store starts + // nil (no cfg.Redis in newMetricsTestConfig) and is forced non-nil + // here, exactly mirroring TestLimiterCurrentUsage_StoreDown's own + // direct-field-injection style (admin_test.go). failOpen is left + // untouched at its true default. gw.limiter.store = alwaysErrStore{} - gw.limiter.failOpen = false rec := httptest.NewRecorder() h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) @@ -814,6 +891,50 @@ func TestMetrics_StoreDown_SkipsRatherThanFabricatesZero(t *testing.T) { if !providerHealthyPresent { t.Error("llmgateway_provider_healthy missing — this family reads in-process breaker state, not the store, and must still render during a store outage") } + + // llmgateway_limit_store_up (item 7, adversarial verification + // 2026-08-23) is the one signal that IS supposed to go to 0 here — + // it exists specifically to explain why every family above just + // went silent. + assertSample(t, samples, "llmgateway_limit_store_up", nil, "0") +} + +// --- store health gauge --- + +// TestMetrics_LimitStoreUp_AbsentWhenNoStoreConfigured proves the gauge +// is not emitted at all for a fallback-only deployment (no cfg.Redis) — +// there is no store health to report there. +func TestMetrics_LimitStoreUp_AbsentWhenNoStoreConfigured(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() // no cfg.Redis + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if strings.Contains(rec.Body.String(), "llmgateway_limit_store_up") { + t.Error("llmgateway_limit_store_up present with no store configured, want absent entirely") + } +} + +// TestMetrics_LimitStoreUp_ReflectsHealthyStore proves the gauge reads 1 +// for a configured, currently-healthy store — newMemoryStore, not +// alwaysErrStore, standing in for "a real store that is up": it +// implements counterStore and never errors, so l.store is non-nil +// (configured) while l.storeLatched() stays false (never failed). +func TestMetrics_LimitStoreUp_ReflectsHealthyStore(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + gw.limiter.store = newMemoryStore() + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + assertSample(t, samples, "llmgateway_limit_store_up", nil, "1") } // --- rate-limit rejections --- @@ -931,6 +1052,48 @@ func TestMetrics_PruneRejectionsAfterReload_EndToEnd(t *testing.T) { } } +// TestServeHTTP_PruneRejectionsOnlyAfterRealReload is the mutation- +// proofing counterpart to TestMetrics_PruneRejectionsAfterReload_ +// EndToEnd above: that test proves pruning DOES happen on a real +// reload; this one proves it does NOT happen otherwise (review fix, +// adversarial verification 2026-08-23 — replacing `if g.auth. +// maybeReload() { g.pruneRejectionsAfterReload() }` in ServeHTTP +// (llmgateway.go) with two unconditional statements survived the whole +// suite until this test existed). +// +// newMetricsTestConfig configures no Users.File at all, so authStore. +// maybeReload's very first check (usersFile == nil) makes it return +// false on every one of the 200 requests below — a real reload never +// happens. Seeds a rejection entry for "ghost", a name that is not any +// currently active user: pruneRejectionsAfterReload would delete it +// (it is not in authStore's own snapshot) the instant it ever ran, so +// its survival after 200 ordinary requests is the pinned guarantee. +func TestServeHTTP_PruneRejectionsOnlyAfterRealReload(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + gw.limiter.rejections.increment("user", "ghost") + + for i := 0; i < 200; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("request %d: status = %d, want 200", i, rec.Code) + } + } + + var found bool + for _, s := range gw.limiter.rejectionSnapshot() { + if s.kind == "user" && s.id == "ghost" { + found = true + } + } + if !found { + t.Error(`the "ghost" rejection entry was pruned despite no real reload ever happening — pruneRejectionsAfterReload must run only when authStore.maybeReload reports a real reload, never unconditionally on every request`) + } +} + // --- budget-consumed ratio --- func TestMetrics_BudgetConsumedRatio_ReflectsUsageVsLimit(t *testing.T) { From 95039d96c9310abd77f9fb6647acac9980173b67 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 23 Aug 2026 01:56:44 +0100 Subject: [PATCH 8/8] fix(examples): make the metrics scrape example actually reachable The VMServiceScrape example scraped Traefik's pods directly, so the request carried Host: :. Traefik evaluates a router's match rule before any middleware runs, and the only router in this file requires Host(`llm.example.com`). The scrape request never matched it, the llmgateway middleware never ran, and /metrics 404ed before the plugin ever saw the request. Add a second, Host-free IngressRoute matching PathPrefix(`/metrics`) and carrying the same middleware, so an in-cluster scraper reaches the route regardless of what Host header it sends. Document the reachability trade-off this introduces (this route is not Host-restricted, though /metrics stays gated by its own auth inside the plugin) and name the alternative fix the review also offered: relabelConfigs rewriting __address__ to the ingress hostname before the request is sent, keeping one Host-scoped router instead. --- examples/kubernetes.yaml | 46 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/examples/kubernetes.yaml b/examples/kubernetes.yaml index 780dbf0..797a15f 100644 --- a/examples/kubernetes.yaml +++ b/examples/kubernetes.yaml @@ -210,6 +210,48 @@ spec: - name: llmgateway-dummy-backend port: 80 +--- +# A SEPARATE, Host-free IngressRoute for the metrics scrape (review fix, +# adversarial verification 2026-08-23): Traefik evaluates a router's +# match rule BEFORE any middleware runs, and the ONLY router above +# requires Host(`llm.example.com`). A VMServiceScrape/ServiceMonitor (below) +# reaches this Service through its cluster-internal DNS name or the pod +# IP directly, so the scrape request's Host header is never +# llm.example.com — it never matches that rule, the llmgateway +# middleware (and therefore /metrics itself) never runs, and the scrape +# 404s. PathPrefix, not Host, is deliberate here: this router exists +# specifically so an in-cluster scraper, addressing this Service by +# whatever internal name/IP it resolves to, still reaches the metrics +# route regardless of Host. /metrics is already gated by its own auth +# inside the plugin (an admin key, or metrics.allowedCIDRs — see +# ../README.md#metrics), so this being reachable without a Host match is +# not a second, unguarded door — but if your own Traefik instance fronts +# other Host-restricted routers on the SAME entryPoint, this route is +# reachable via any of THEIR hostnames too, not only in-cluster; narrow +# entryPoints below to an internal-only one, or add your own Host() +# match for your cluster's internal DNS name, if that reachability is +# broader than you want. The alternative the review that flagged this +# also named — keeping one Host-scoped router and instead using +# VMServiceScrape's own relabelConfigs to rewrite __address__ to +# llm.example.com before the request is sent — works too; this file uses +# the simpler of the two. +apiVersion: traefik.io/v1alpha1 +kind: IngressRoute +metadata: + name: llmgateway-metrics + namespace: traefik +spec: + entryPoints: + - websecure + routes: + - match: PathPrefix(`/metrics`) + kind: Rule + middlewares: + - name: llmgateway + services: + - name: llmgateway-dummy-backend + port: 80 + --- # Scraping /metrics (../README.md#metrics): this cluster runs # VictoriaMetrics, not Prometheus, so the CRD is VMServiceScrape @@ -222,7 +264,9 @@ spec: # IMPORTANT: /metrics is served by TRAEFIK ITSELF, not by # llmgateway-dummy-backend above — this middleware intercepts the path # before any backend service is ever reached (same as every other route -# this plugin owns). Point this scrape at YOUR Traefik installation's own +# this plugin owns), which is exactly why the Host-free IngressRoute +# above exists: without it this scrape 404s before the middleware ever +# sees the request. Point this scrape at YOUR Traefik installation's own # Service (commonly named "traefik" — verify against your own Helm # release), never at a backend service this file defines. Adjust # selector/namespaceSelector/port to match your Traefik Service's real