diff --git a/README.md b/README.md index a61b168..1b04bc1 100644 --- a/README.md +++ b/README.md @@ -483,9 +483,10 @@ cached bodies for the same request. - **Group override**: `GroupConfig.cache` (a `*bool`) overrides the top-level `cache.enabled` setting per group — `nil` inherits it, `false` opts the group out even when caching is globally on, `true` opts the - group in. Setting `true` on a group when the top-level `cache` block is - absent entirely is a construction error: there is nothing configured to - inherit `ttl`/`maxBodyBytes` from. + group in. Setting `true` on a group when the top-level `cache` is not + enabled — the block omitted entirely, or present with `enabled: false`, + both read the same way — is a construction error: there is nothing + configured to inherit `ttl`/`maxBodyBytes` from. - **Storage**: values are stored via the same hand-rolled RESP2 client (`resp.go`) the limiter's distributed counters use — the identical Redis/Valkey/Dragonfly connection, not a second one. A Redis error @@ -575,7 +576,8 @@ request to any of them falls through to the plugin's existing themselves. - **`GET /admin`** serves the dashboard's HTML/CSS/JS shell. This route is deliberately served **with no authentication at all** once - `admin.enabled` is true (disabled still 404s): a browser navigating + `admin.enabled` is true (disabled still falls through to the same + 404/`passthroughUnknown` handling described above): a browser navigating straight to the URL has no way to attach a custom `Authorization` or `x-api-key` header, so gating the page itself would make it unreachable from a browser in the first place. This is safe because the shell diff --git a/examples/kubernetes.yaml b/examples/kubernetes.yaml index 6b5ec60..d333498 100644 --- a/examples/kubernetes.yaml +++ b/examples/kubernetes.yaml @@ -87,11 +87,17 @@ spec: users: file: /secrets/llmgw-users/users.json inline: - # Grants the admin dashboard (see the "admin" block above), - # which stays unreachable until admin.enabled is true. In a - # real deployment this key still belongs in a Secret via - # env:/file: the same as every other apiKey here — inline only - # to keep this one example self-contained. + # Grants the admin dashboard (see the "admin" block below), + # which stays unreachable until admin.enabled is true. Declared + # inline here — rather than added to the users.json Secret + # above — only to keep this admin-granting example visible in + # one file; the key itself is NOT a literal, it uses the same + # env: form as every other apiKey in this Middleware, and its + # env var (LLMGW_ADMIN_KEY) must be set on the Traefik pod + # regardless of whether admin.enabled is true or false below — + # every inline user's apiKey resolves at construction time, not + # lazily on first admin request. See the Helm env snippet at + # the bottom of this file. - name: dashboard-admin group: engineering apiKey: "env:LLMGW_ADMIN_KEY" @@ -111,10 +117,12 @@ spec: cache: enabled: true ttl: 5m - # Disabled here deliberately: flip to `enabled: true`, then flag at - # least one user `admin: true` (see the inline "dashboard-admin" - # user under users.inline below) to try the read-only dashboard at - # https://llm.example.com/admin — see ../README.md#admin. + # Disabled here deliberately: flip to `enabled: true` to try the + # read-only dashboard at https://llm.example.com/admin — the + # "dashboard-admin" user above (users.inline) already grants access + # once this is on, and its LLMGW_ADMIN_KEY env var is already + # required regardless (see that user's own comment) — see + # ../README.md#admin. admin: enabled: false pricing: @@ -201,9 +209,13 @@ spec: # moduleName: "github.com/lukaszraczylo/traefik-llmgateway" # version: "v0.1.0" # -# 2. Provider keys as env vars, sourced from Secrets — never inlined into -# the Middleware CR above, which is why every apiKey field there reads -# "env:VAR_NAME" instead of a literal: +# 2. Every "env:VAR_NAME" secret this Middleware references as env vars, +# sourced from Secrets — never inlined into the Middleware CR above. +# This is NOT just the three provider keys and the Redis password: +# LLMGW_ADMIN_KEY (the "dashboard-admin" user above) resolves at +# construction time too, so it must be set here even while +# admin.enabled is false — an unset "env:" var is a hard construction +# error regardless of whether the feature it gates is on. # # env: # - name: OPENAI_API_KEY @@ -214,6 +226,8 @@ spec: # valueFrom: {secretKeyRef: {name: llmgw-provider-keys, key: gemini}} # - name: REDIS_PASSWORD # valueFrom: {secretKeyRef: {name: llmgw-redis, key: password}} +# - name: LLMGW_ADMIN_KEY +# valueFrom: {secretKeyRef: {name: llmgw-admin-key, key: apiKey}} # # additionalVolumes: # - name: llmgw-users diff --git a/integration/integration_test.go b/integration/integration_test.go index 423a512..c0a2ffc 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -339,6 +339,26 @@ func flushRedis(t *testing.T) { } } +// redisKeys returns every key in the shared Redis matching pattern (a +// redis-cli KEYS glob), via the same docker compose exec pattern +// flushRedis uses above. KEYS is O(n) and blocks Redis briefly — fine +// against this suite's tiny keyspace, and used only for a one-off test +// assertion, never on a hot path. +func redisKeys(t *testing.T, pattern string) []string { + t.Helper() + out, err := exec.Command("docker", "compose", "-f", composeFile(), "exec", "-T", "redis", "redis-cli", "KEYS", pattern).CombinedOutput() + if err != nil { + t.Fatalf("redis-cli KEYS %s: %v\n%s", pattern, err, out) + } + var keys []string + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if line != "" { + keys = append(keys, line) + } + } + return keys +} + // TestCrossReplicaLimits covers integration test 4: bob's limited group // (requestsPerMinute: 3) is enforced globally across both Traefik // "replicas" via the Redis they share, regardless of which one a given @@ -533,12 +553,19 @@ func TestRealUpstreamSmoke(t *testing.T) { // retry made exactly two upstream tries, not merely that the mock // happened to succeed on a fresh process. // -// This test is NOT safe to re-run against a `make integration-keep` -// stack without restarting mockopenai first: handleFlakyOpenAIChat's own -// hit counter is process-lifetime state with no reset endpoint (see its -// doc comment, integration/mock/main.go) — a second run against the same -// container never observes the forced-503-then-success sequence again. +// Resets the mock's hit counter first via POST /flaky/reset — reachable +// through the "flaky" provider's own native passthrough route +// (routes_passthrough.go), which reaches handleFlakyReset +// (integration/mock/main.go) — so this test reproduces the same +// forced-503-then-success sequence on every run, including a re-run +// against an already-up `make integration-keep` stack with no +// mockopenai restart. func TestRetryFlakyRecovers(t *testing.T) { + resetResp, resetBody := doJSON(t, http.MethodPost, traefik1URL+"/flaky/reset", aliceKey, nil) + if resetResp.StatusCode != http.StatusOK { + t.Fatalf("reset flaky counter: status = %d, body=%#v", resetResp.StatusCode, resetBody) + } + reqBody := map[string]any{ "model": "flaky/flaky-mock", "messages": []map[string]any{{"role": "user", "content": "hi"}}, @@ -605,6 +632,9 @@ func TestResponseCache(t *testing.T) { if content := firstChoiceContent(t, body1); content != "mock openai response" { t.Errorf("first request: content = %q, want %q", content, "mock openai response") } + if keys := redisKeys(t, "llmgw:cache:*"); len(keys) == 0 { + t.Error("redis holds no llmgw:cache:* key after a cache miss, want the entry the miss's SET (cache.go) should have written") + } tokensAfterMiss, _ := adminUsageEntry(t, "users", "alice")["tokensPerDay"].(float64) if tokensAfterMiss <= 0 { diff --git a/integration/mock/main.go b/integration/mock/main.go index db2cc87..fc617b3 100644 --- a/integration/mock/main.go +++ b/integration/mock/main.go @@ -76,7 +76,12 @@ func newServerMux(mode string) (*http.ServeMux, error) { // 5): the plugin's "flaky" provider points its baseUrl at this // mode's own address plus "/flaky", so a.baseURL+"/v1/chat/ // completions" (provider_openai.go) resolves to exactly this path. + // flaky/reset is reachable the same way, through the "flaky" + // provider's native passthrough route (adapter.base()+"/"+rest, + // routes_passthrough.go) — the test calls it directly, never this + // mock process's own network address. mux.HandleFunc("/flaky/v1/chat/completions", handleFlakyOpenAIChat) + mux.HandleFunc("/flaky/reset", handleFlakyReset) mux.HandleFunc("/v1/images/generations", handleOpenAIImages) mux.HandleFunc("/v1/audio/speech", handleOpenAIAudioSpeech) case "anthropic": @@ -208,24 +213,37 @@ func handleOpenAIChat(w http.ResponseWriter, r *http.Request) { sw.write("", "[DONE]") } -// flakyHits counts every request handleFlakyOpenAIChat has served across -// this process's lifetime. It is package-level, process-lifetime state -// deliberately, not reset per request: the retry integration test relies -// on exactly the FIRST request this mock process ever receives on -// /flaky/v1/chat/completions failing, and every one after succeeding, so -// a fresh mockopenai container (one per `make integration` run) always -// starts the scenario from a clean slate. Re-running the integration -// suite against an already-up stack (`make integration-keep`, without -// restarting mockopenai) will NOT reproduce the retry test a second time -// — the same non-idempotence flushRedis exists to paper over for the -// rate-limit counters has no equivalent reset here, since there is no -// admin endpoint on this fixture server to clear it. +// flakyHits counts every request handleFlakyOpenAIChat has served since +// the last reset (handleFlakyReset, below), or since process start if +// never reset. It is package-level state deliberately, not reset per +// request: the retry integration test relies on exactly the FIRST +// request counted this way failing, and every one after succeeding. +// TestRetryFlakyRecovers calls handleFlakyReset (via the gateway's own +// "flaky" passthrough provider) at its own start, so a re-run against an +// already-up stack (`make integration-keep`, no mockopenai restart) +// reproduces the same forced-503-then-success scenario every time — the +// same idempotence flushRedis gives the rate-limit counters via +// TestCrossReplicaLimits. var flakyHits int32 -// handleFlakyOpenAIChat answers the first request this process ever -// receives with HTTP 503 (a transient failure retryPolicy.isTransient -// classifies as retryable, retry.go) and every request after that with a -// normal chat completion response carrying an extra top-level +// handleFlakyReset resets flakyHits to 0, letting the retry integration +// test reproduce its forced-503-then-success scenario on demand rather +// than only on this process's true first request. POST only — a GET (or +// any other method) must never mutate state; it answers 405 instead. +func handleFlakyReset(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + atomic.StoreInt32(&flakyHits, 0) + writeJSON(w, map[string]any{"reset": true}) +} + +// handleFlakyOpenAIChat answers the first request since the last reset +// (handleFlakyReset, or process start if never reset) with HTTP 503 (a +// transient failure retryPolicy.isTransient classifies as retryable, +// retry.go) and every request after that with a normal chat completion +// response carrying an extra top-level // "mock_attempt" field set to this handler's own call count — the // unified route's openai-type adapter forwards a JSON response body // verbatim (forwardJSON, provider_openai.go), so this field survives diff --git a/integration/traefik/dynamic.yml.tmpl b/integration/traefik/dynamic.yml.tmpl index 5610897..40f01d1 100644 --- a/integration/traefik/dynamic.yml.tmpl +++ b/integration/traefik/dynamic.yml.tmpl @@ -44,14 +44,16 @@ http: baseUrl: "http://mockgemini:8080" apiKey: "mock-gemini-key" models: ["gemini-mock"] - # Backs test 8's retry coverage only: baseUrl points at the + # Backs task 5's retry coverage only: baseUrl points at the # SAME mockopenai container, under its dedicated /flaky prefix # (see integration/mock/main.go's handleFlakyOpenAIChat), so # a.baseURL+"/v1/chat/completions" (provider_openai.go) - # resolves to /flaky/v1/chat/completions. discovery is left - # off — GET /flaky/v1/models isn't served — so the explicit - # "flaky-mock" model id below is the only way this provider's - # model is known. + # resolves to /flaky/v1/chat/completions, and this provider's + # own native passthrough route (/flaky/reset) reaches + # handleFlakyReset the same way. discovery is left off — GET + # /flaky/v1/models isn't served — so the explicit "flaky-mock" + # model id below is the only way this provider's model is + # known. flaky: type: openai baseUrl: "http://mockopenai:8080/flaky"