mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-14 05:18:54 +00:00
fix(docs,integration): example env completeness, flaky reset, cache key assertion
Task 5 re-review fixes: 1 Important + 3 folded. 1. (Important) examples/kubernetes.yaml was construction-broken on copy-paste: the inline dashboard-admin user resolves env:LLMGW_ADMIN_KEY at construction time regardless of admin.enabled, but the Helm env snippet never listed it. Added it there, alongside a comment on the admin user explaining the env var is required either way. Fixed the two reversed above/below cross-references (kubernetes.yaml:90/:115) and reworded the misleading "self-contained" comment on the inline admin user. Verified with a throwaway round-trip test covering: no env vars set fails, every documented env var set succeeds (admin.enabled staying false as committed), and — the actual bug — every var set EXCEPT LLMGW_ADMIN_KEY still fails even with admin disabled. 2. README: Admin section's "(disabled still 404s)" now points at the same 404/passthroughUnknown wording the section already uses above it. Caching section's "block absent entirely is a construction error" now says "not enabled" — auth.go's actual check is !cfg.Cache.Enabled, which is identically true whether the block is omitted or present with enabled: false. 3. integration/mock/main.go: added POST /flaky/reset (handleFlakyReset), reachable through the "flaky" provider's own native passthrough route, resetting the atomic hit counter on demand. TestRetryFlakyRecovers now calls it at the start of the test, removing the make integration-keep re-run trap the old doc comment warned about instead of fixing. dynamic.yml.tmpl's stale "test 8" comment now reads "task 5". 4. integration_test.go: added redisKeys (docker compose exec redis-cli KEYS, mirroring flushRedis's own pattern) and a TestResponseCache assertion that redis holds at least one llmgw:cache:* key immediately after the miss request. go test ./... -count=1, gofmt, vet, make yaegi-check all pass; make integration exits 0 across two fresh full runs.
This commit is contained in:
@@ -483,9 +483,10 @@ cached bodies for the same request.
|
|||||||
- **Group override**: `GroupConfig.cache` (a `*bool`) overrides the
|
- **Group override**: `GroupConfig.cache` (a `*bool`) overrides the
|
||||||
top-level `cache.enabled` setting per group — `nil` inherits it, `false`
|
top-level `cache.enabled` setting per group — `nil` inherits it, `false`
|
||||||
opts the group out even when caching is globally on, `true` opts the
|
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
|
group in. Setting `true` on a group when the top-level `cache` is not
|
||||||
absent entirely is a construction error: there is nothing configured to
|
enabled — the block omitted entirely, or present with `enabled: false`,
|
||||||
inherit `ttl`/`maxBodyBytes` from.
|
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
|
- **Storage**: values are stored via the same hand-rolled RESP2 client
|
||||||
(`resp.go`) the limiter's distributed counters use — the identical
|
(`resp.go`) the limiter's distributed counters use — the identical
|
||||||
Redis/Valkey/Dragonfly connection, not a second one. A Redis error
|
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.
|
themselves.
|
||||||
- **`GET /admin`** serves the dashboard's HTML/CSS/JS shell. This route is
|
- **`GET /admin`** serves the dashboard's HTML/CSS/JS shell. This route is
|
||||||
deliberately served **with no authentication at all** once
|
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
|
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
|
`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
|
from a browser in the first place. This is safe because the shell
|
||||||
|
|||||||
+26
-12
@@ -87,11 +87,17 @@ spec:
|
|||||||
users:
|
users:
|
||||||
file: /secrets/llmgw-users/users.json
|
file: /secrets/llmgw-users/users.json
|
||||||
inline:
|
inline:
|
||||||
# Grants the admin dashboard (see the "admin" block above),
|
# Grants the admin dashboard (see the "admin" block below),
|
||||||
# which stays unreachable until admin.enabled is true. In a
|
# which stays unreachable until admin.enabled is true. Declared
|
||||||
# real deployment this key still belongs in a Secret via
|
# inline here — rather than added to the users.json Secret
|
||||||
# env:/file: the same as every other apiKey here — inline only
|
# above — only to keep this admin-granting example visible in
|
||||||
# to keep this one example self-contained.
|
# 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
|
- name: dashboard-admin
|
||||||
group: engineering
|
group: engineering
|
||||||
apiKey: "env:LLMGW_ADMIN_KEY"
|
apiKey: "env:LLMGW_ADMIN_KEY"
|
||||||
@@ -111,10 +117,12 @@ spec:
|
|||||||
cache:
|
cache:
|
||||||
enabled: true
|
enabled: true
|
||||||
ttl: 5m
|
ttl: 5m
|
||||||
# Disabled here deliberately: flip to `enabled: true`, then flag at
|
# Disabled here deliberately: flip to `enabled: true` to try the
|
||||||
# least one user `admin: true` (see the inline "dashboard-admin"
|
# read-only dashboard at https://llm.example.com/admin — the
|
||||||
# user under users.inline below) to try the read-only dashboard at
|
# "dashboard-admin" user above (users.inline) already grants access
|
||||||
# https://llm.example.com/admin — see ../README.md#admin.
|
# 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:
|
admin:
|
||||||
enabled: false
|
enabled: false
|
||||||
pricing:
|
pricing:
|
||||||
@@ -201,9 +209,13 @@ spec:
|
|||||||
# moduleName: "github.com/lukaszraczylo/traefik-llmgateway"
|
# moduleName: "github.com/lukaszraczylo/traefik-llmgateway"
|
||||||
# version: "v0.1.0"
|
# version: "v0.1.0"
|
||||||
#
|
#
|
||||||
# 2. Provider keys as env vars, sourced from Secrets — never inlined into
|
# 2. Every "env:VAR_NAME" secret this Middleware references as env vars,
|
||||||
# the Middleware CR above, which is why every apiKey field there reads
|
# sourced from Secrets — never inlined into the Middleware CR above.
|
||||||
# "env:VAR_NAME" instead of a literal:
|
# 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:
|
# env:
|
||||||
# - name: OPENAI_API_KEY
|
# - name: OPENAI_API_KEY
|
||||||
@@ -214,6 +226,8 @@ spec:
|
|||||||
# valueFrom: {secretKeyRef: {name: llmgw-provider-keys, key: gemini}}
|
# valueFrom: {secretKeyRef: {name: llmgw-provider-keys, key: gemini}}
|
||||||
# - name: REDIS_PASSWORD
|
# - name: REDIS_PASSWORD
|
||||||
# valueFrom: {secretKeyRef: {name: llmgw-redis, key: password}}
|
# valueFrom: {secretKeyRef: {name: llmgw-redis, key: password}}
|
||||||
|
# - name: LLMGW_ADMIN_KEY
|
||||||
|
# valueFrom: {secretKeyRef: {name: llmgw-admin-key, key: apiKey}}
|
||||||
#
|
#
|
||||||
# additionalVolumes:
|
# additionalVolumes:
|
||||||
# - name: llmgw-users
|
# - name: llmgw-users
|
||||||
|
|||||||
@@ -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
|
// TestCrossReplicaLimits covers integration test 4: bob's limited group
|
||||||
// (requestsPerMinute: 3) is enforced globally across both Traefik
|
// (requestsPerMinute: 3) is enforced globally across both Traefik
|
||||||
// "replicas" via the Redis they share, regardless of which one a given
|
// "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
|
// retry made exactly two upstream tries, not merely that the mock
|
||||||
// happened to succeed on a fresh process.
|
// happened to succeed on a fresh process.
|
||||||
//
|
//
|
||||||
// This test is NOT safe to re-run against a `make integration-keep`
|
// Resets the mock's hit counter first via POST /flaky/reset — reachable
|
||||||
// stack without restarting mockopenai first: handleFlakyOpenAIChat's own
|
// through the "flaky" provider's own native passthrough route
|
||||||
// hit counter is process-lifetime state with no reset endpoint (see its
|
// (routes_passthrough.go), which reaches handleFlakyReset
|
||||||
// doc comment, integration/mock/main.go) — a second run against the same
|
// (integration/mock/main.go) — so this test reproduces the same
|
||||||
// container never observes the forced-503-then-success sequence again.
|
// 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) {
|
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{
|
reqBody := map[string]any{
|
||||||
"model": "flaky/flaky-mock",
|
"model": "flaky/flaky-mock",
|
||||||
"messages": []map[string]any{{"role": "user", "content": "hi"}},
|
"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" {
|
if content := firstChoiceContent(t, body1); content != "mock openai response" {
|
||||||
t.Errorf("first request: content = %q, want %q", 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)
|
tokensAfterMiss, _ := adminUsageEntry(t, "users", "alice")["tokensPerDay"].(float64)
|
||||||
if tokensAfterMiss <= 0 {
|
if tokensAfterMiss <= 0 {
|
||||||
|
|||||||
+34
-16
@@ -76,7 +76,12 @@ func newServerMux(mode string) (*http.ServeMux, error) {
|
|||||||
// 5): the plugin's "flaky" provider points its baseUrl at this
|
// 5): the plugin's "flaky" provider points its baseUrl at this
|
||||||
// mode's own address plus "/flaky", so a.baseURL+"/v1/chat/
|
// mode's own address plus "/flaky", so a.baseURL+"/v1/chat/
|
||||||
// completions" (provider_openai.go) resolves to exactly this path.
|
// 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/v1/chat/completions", handleFlakyOpenAIChat)
|
||||||
|
mux.HandleFunc("/flaky/reset", handleFlakyReset)
|
||||||
mux.HandleFunc("/v1/images/generations", handleOpenAIImages)
|
mux.HandleFunc("/v1/images/generations", handleOpenAIImages)
|
||||||
mux.HandleFunc("/v1/audio/speech", handleOpenAIAudioSpeech)
|
mux.HandleFunc("/v1/audio/speech", handleOpenAIAudioSpeech)
|
||||||
case "anthropic":
|
case "anthropic":
|
||||||
@@ -208,24 +213,37 @@ func handleOpenAIChat(w http.ResponseWriter, r *http.Request) {
|
|||||||
sw.write("", "[DONE]")
|
sw.write("", "[DONE]")
|
||||||
}
|
}
|
||||||
|
|
||||||
// flakyHits counts every request handleFlakyOpenAIChat has served across
|
// flakyHits counts every request handleFlakyOpenAIChat has served since
|
||||||
// this process's lifetime. It is package-level, process-lifetime state
|
// the last reset (handleFlakyReset, below), or since process start if
|
||||||
// deliberately, not reset per request: the retry integration test relies
|
// never reset. It is package-level state deliberately, not reset per
|
||||||
// on exactly the FIRST request this mock process ever receives on
|
// request: the retry integration test relies on exactly the FIRST
|
||||||
// /flaky/v1/chat/completions failing, and every one after succeeding, so
|
// request counted this way failing, and every one after succeeding.
|
||||||
// a fresh mockopenai container (one per `make integration` run) always
|
// TestRetryFlakyRecovers calls handleFlakyReset (via the gateway's own
|
||||||
// starts the scenario from a clean slate. Re-running the integration
|
// "flaky" passthrough provider) at its own start, so a re-run against an
|
||||||
// suite against an already-up stack (`make integration-keep`, without
|
// already-up stack (`make integration-keep`, no mockopenai restart)
|
||||||
// restarting mockopenai) will NOT reproduce the retry test a second time
|
// reproduces the same forced-503-then-success scenario every time — the
|
||||||
// — the same non-idempotence flushRedis exists to paper over for the
|
// same idempotence flushRedis gives the rate-limit counters via
|
||||||
// rate-limit counters has no equivalent reset here, since there is no
|
// TestCrossReplicaLimits.
|
||||||
// admin endpoint on this fixture server to clear it.
|
|
||||||
var flakyHits int32
|
var flakyHits int32
|
||||||
|
|
||||||
// handleFlakyOpenAIChat answers the first request this process ever
|
// handleFlakyReset resets flakyHits to 0, letting the retry integration
|
||||||
// receives with HTTP 503 (a transient failure retryPolicy.isTransient
|
// test reproduce its forced-503-then-success scenario on demand rather
|
||||||
// classifies as retryable, retry.go) and every request after that with a
|
// than only on this process's true first request. POST only — a GET (or
|
||||||
// normal chat completion response carrying an extra top-level
|
// 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
|
// "mock_attempt" field set to this handler's own call count — the
|
||||||
// unified route's openai-type adapter forwards a JSON response body
|
// unified route's openai-type adapter forwards a JSON response body
|
||||||
// verbatim (forwardJSON, provider_openai.go), so this field survives
|
// verbatim (forwardJSON, provider_openai.go), so this field survives
|
||||||
|
|||||||
@@ -44,14 +44,16 @@ http:
|
|||||||
baseUrl: "http://mockgemini:8080"
|
baseUrl: "http://mockgemini:8080"
|
||||||
apiKey: "mock-gemini-key"
|
apiKey: "mock-gemini-key"
|
||||||
models: ["gemini-mock"]
|
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
|
# SAME mockopenai container, under its dedicated /flaky prefix
|
||||||
# (see integration/mock/main.go's handleFlakyOpenAIChat), so
|
# (see integration/mock/main.go's handleFlakyOpenAIChat), so
|
||||||
# a.baseURL+"/v1/chat/completions" (provider_openai.go)
|
# a.baseURL+"/v1/chat/completions" (provider_openai.go)
|
||||||
# resolves to /flaky/v1/chat/completions. discovery is left
|
# resolves to /flaky/v1/chat/completions, and this provider's
|
||||||
# off — GET /flaky/v1/models isn't served — so the explicit
|
# own native passthrough route (/flaky/reset) reaches
|
||||||
# "flaky-mock" model id below is the only way this provider's
|
# handleFlakyReset the same way. discovery is left off — GET
|
||||||
# model is known.
|
# /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:
|
flaky:
|
||||||
type: openai
|
type: openai
|
||||||
baseUrl: "http://mockopenai:8080/flaky"
|
baseUrl: "http://mockopenai:8080/flaky"
|
||||||
|
|||||||
Reference in New Issue
Block a user