From 432f8a75b9b5bfb55dfc7af2009722f878b70768 Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Sat, 18 Apr 2026 15:36:58 +0200 Subject: [PATCH 1/2] feat(generator): emit BAN ACL + handler when spec.invalidation.ban.enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously spec.invalidation.ban was persisted on the CR and its AllowedSources validated by the webhook, but none of it reached the generated VCL: no vinyl_ban_allowed ACL, no BAN handler in vcl_recv. Users who set ban.enabled: true saw the field accepted but BAN requests fell through to the regular cache-miss path. Generator changes: - HasBAN flag on TemplateData (from spec.invalidation.ban.enabled). - vinyl_ban_allowed ACL emitted when HasBAN, populated with: 127.0.0.1 (always) (when set — invalidation proxy source) (from spec.invalidation.ban.allowedSources) - BAN handler added to vcl_recv: - 403 when client.ip not in vinyl_ban_allowed - 400 when X-Vinyl-Ban header missing - ban(req.http.X-Vinyl-Ban) + synth(200, "Banned") otherwise Rate limiting (spec.invalidation.ban.rateLimitPerMinute) is still inert — tracked as future work in #39 but deferred for the MVP. Closes #39 --- internal/generator/generator.go | 4 ++ internal/generator/generator_test.go | 70 +++++++++++++++++++ internal/generator/templates/acls.vcl.tmpl | 12 ++++ .../generator/templates/vcl_recv.vcl.tmpl | 14 +++- 4 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/generator/generator.go b/internal/generator/generator.go index f830efd..9de4d36 100644 --- a/internal/generator/generator.go +++ b/internal/generator/generator.go @@ -61,6 +61,7 @@ type TemplateData struct { HasESI bool HasXkey bool HasSoftPurge bool + HasBAN bool HasProxyProtocol bool HasFullOverride bool VCLName string // sanitized name for vcl declaration @@ -199,6 +200,9 @@ func buildTemplateData(input Input) TemplateData { if input.Spec.Invalidation.Purge != nil { data.HasSoftPurge = input.Spec.Invalidation.Purge.Soft } + if input.Spec.Invalidation.BAN != nil { + data.HasBAN = input.Spec.Invalidation.BAN.Enabled + } // ESI: check VarnishParams for explicit feature flag. _, hasESI := input.Spec.VarnishParams["feature +esi"] diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go index 21dc355..86d8dec 100644 --- a/internal/generator/generator_test.go +++ b/internal/generator/generator_test.go @@ -835,3 +835,73 @@ func TestGenerate_OperatorIP_CoexistsWithAllowedSources(t *testing.T) { assert.Contains(t, r.VCL, `"192.168.1.0/24";`) assert.Contains(t, r.VCL, `"127.0.0.1";`) } + +func TestGenerate_BAN_DisabledByDefault(t *testing.T) { + g := newGenerator(t) + r, err := g.Generate(makeMinimalInput()) + require.NoError(t, err) + assert.NotContains(t, r.VCL, "acl vinyl_ban_allowed", + "BAN ACL must NOT be emitted when spec.invalidation.ban is unset") + assert.NotContains(t, r.VCL, `req.method == "BAN"`, + "BAN handler must NOT be emitted when ban is disabled") +} + +func TestGenerate_BAN_EnabledEmitsACLAndHandler(t *testing.T) { + g := newGenerator(t) + input := makeMinimalInput() + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{Enabled: true} + r, err := g.Generate(input) + require.NoError(t, err) + assert.Contains(t, r.VCL, "acl vinyl_ban_allowed {", + "BAN ACL must be emitted when ban.enabled=true") + assert.Contains(t, r.VCL, `req.method == "BAN"`, + "BAN handler must be emitted in vcl_recv") + assert.Contains(t, r.VCL, `client.ip ~ vinyl_ban_allowed`, + "BAN handler must check the BAN ACL") + assert.Contains(t, r.VCL, `ban(req.http.X-Vinyl-Ban)`, + "BAN handler must call ban() with the X-Vinyl-Ban header value") + assert.Contains(t, r.VCL, `return(synth(200, "Banned"))`, + "BAN handler must return synth 200 on success") +} + +func TestGenerate_BAN_OperatorIPInACL(t *testing.T) { + g := newGenerator(t) + input := makeMinimalInput() + input.OperatorIP = "10.244.1.7" + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{Enabled: true} + r, err := g.Generate(input) + require.NoError(t, err) + banACL := strings.SplitAfter(r.VCL, "acl vinyl_ban_allowed {")[1] + banACL = strings.SplitN(banACL, "}", 2)[0] + assert.Contains(t, banACL, `"10.244.1.7";`, + "operator IP must appear in vinyl_ban_allowed so operator-proxied BAN requests are accepted") + assert.Contains(t, banACL, `"127.0.0.1";`, + "localhost entry must remain") +} + +func TestGenerate_BAN_AllowedSourcesHonored(t *testing.T) { + g := newGenerator(t) + input := makeMinimalInput() + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{ + Enabled: true, + AllowedSources: []string{"10.0.0.0/8", "192.168.1.0/24"}, + } + r, err := g.Generate(input) + require.NoError(t, err) + banACL := strings.SplitAfter(r.VCL, "acl vinyl_ban_allowed {")[1] + banACL = strings.SplitN(banACL, "}", 2)[0] + assert.Contains(t, banACL, `"10.0.0.0/8";`) + assert.Contains(t, banACL, `"192.168.1.0/24";`) +} + +func TestGenerate_BAN_EnabledButNotConfigured_OnlyLocalhost(t *testing.T) { + g := newGenerator(t) + input := makeMinimalInput() + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{Enabled: true} + r, err := g.Generate(input) + require.NoError(t, err) + banACL := strings.SplitAfter(r.VCL, "acl vinyl_ban_allowed {")[1] + banACL = strings.SplitN(banACL, "}", 2)[0] + assert.Equal(t, 1, strings.Count(banACL, `"127.0.0.1";`), + "BAN ACL with no AllowedSources and no OperatorIP must contain exactly one entry (localhost)") +} diff --git a/internal/generator/templates/acls.vcl.tmpl b/internal/generator/templates/acls.vcl.tmpl index e94213a..a2c17f6 100644 --- a/internal/generator/templates/acls.vcl.tmpl +++ b/internal/generator/templates/acls.vcl.tmpl @@ -7,6 +7,18 @@ acl vinyl_purge_allowed { "{{ . }}"; {{- end }}{{ end }} } +{{- if .HasBAN }} + +acl vinyl_ban_allowed { + "127.0.0.1"; // localhost always allowed +{{- if .OperatorIP }} + "{{ .OperatorIP }}"; // operator pod IP (invalidation proxy source) +{{- end }} +{{- if .Spec.Invalidation.BAN }}{{ range .Spec.Invalidation.BAN.AllowedSources }} + "{{ . }}"; +{{- end }}{{ end }} +} +{{- end }} {{- if .HasCluster }} acl vinyl_cluster_peers { diff --git a/internal/generator/templates/vcl_recv.vcl.tmpl b/internal/generator/templates/vcl_recv.vcl.tmpl index 37179b0..640f0c3 100644 --- a/internal/generator/templates/vcl_recv.vcl.tmpl +++ b/internal/generator/templates/vcl_recv.vcl.tmpl @@ -47,7 +47,19 @@ sub vcl_recv { {{- end }} return(purge); } - +{{ if .HasBAN }} + # BAN handling + if (req.method == "BAN") { + if (!client.ip ~ vinyl_ban_allowed) { + return(synth(403, "Forbidden")); + } + if (!req.http.X-Vinyl-Ban) { + return(synth(400, "Missing X-Vinyl-Ban header")); + } + ban(req.http.X-Vinyl-Ban); + return(synth(200, "Banned")); + } +{{- end }} # Only handle GET and HEAD for caching (pass everything else) if (req.method != "GET" && req.method != "HEAD") { return(pass); From ed5b06ffb03084ac4b46fc48a2bf65358ced7552 Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Sat, 18 Apr 2026 16:08:54 +0200 Subject: [PATCH 2/2] fix: address v0.4.2 review findings on BAN + OperatorIP + cluster routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical, blocking-for-v0.4.2 issues raised by the end-to-end code review of the combined #40 + #41 + #42 changeset: - C1: cmd/operator/main.go never read POD_IP, so the OperatorIP field that #40 plumbed through the generator was always empty in production. Added os.Getenv("POD_IP") at reconciler construction so the PURGE ACL actually contains the operator pod IP and invalidation-proxy requests pass the ACL. - C2: the BAN handler emitted the deprecated bare ban() form — the project's own architecture docs explicitly require std.ban(). Switched to std.ban() and added a std.ban_error() check that returns synth(400) with the compile error when the X-Vinyl-Ban expression is malformed. Previously a bad expression silently returned 200 with no invalidation. Regression-guard assertion locks in the shape. - I2: ban-lurker-friendly x-url / x-host headers were only emitted when HasXkey. With BAN enabled but xkey disabled, users writing ban("obj.http.x-url ~ ...") would find the header missing and fall back to req.* bans that accumulate O(n*m). Widened the gate to {{- if or .HasXkey .HasBAN }} in both vcl_backend_response and vcl_deliver. - I3: trim on the BAN block (cosmetic — no stray blank line). - I4: docs/sources/reference/vinylcache-spec.md updated: * shard.by/healthy note clarifies cluster-peer is auto, per-backend is user-snippet. * New invalidation rows for ban.enabled, ban.allowedSources, ban.rateLimitPerMinute (latter explicitly marked inert). * Security note + usage example for the X-Vinyl-Ban header. - M1/M5: BAN-handler test now extracts the handler block and asserts on that substring (was: flat-file substring matches that could match noise elsewhere). Added TestGenerate_BAN_BanLurkerHeadersEmitted and TestGenerate_BAN_RateLimit_CurrentlyInert to lock in the deferred rate-limit state. - E2E: vcl-validation cleanup timeout bumped from default (30s) to 60s — the flake that reappeared in #42's CI run was the same symptom this test hit before the v0.4.0 release. Refs #18 #36 #39 --- cmd/operator/main.go | 5 ++ docs/sources/reference/vinylcache-spec.md | 26 +++++++- e2e/tests/vcl-validation/chainsaw-test.yaml | 1 + internal/generator/generator_test.go | 63 +++++++++++++++++-- .../templates/vcl_backend_response.vcl.tmpl | 2 +- .../generator/templates/vcl_deliver.vcl.tmpl | 2 +- .../generator/templates/vcl_recv.vcl.tmpl | 11 +++- 7 files changed, 96 insertions(+), 14 deletions(-) diff --git a/cmd/operator/main.go b/cmd/operator/main.go index a943596..7cf00e7 100644 --- a/cmd/operator/main.go +++ b/cmd/operator/main.go @@ -212,6 +212,11 @@ func main() { HTTPClient: &http.Client{Timeout: agentClientTimeout}, K8sClient: mgr.GetClient(), }, + // POD_IP is injected by the Helm chart via the downward API. + // Empty is safe (the ACL template and EndpointSlice reconciler both + // no-op on empty), but it means invalidation-proxy PURGE/BAN requests + // are rejected by Varnish's ACL — so in production this must be set. + OperatorIP: os.Getenv("POD_IP"), ProxyRouter: proxyRouter, ProxyPodMap: proxyPodMap, }).SetupWithManager(mgr); err != nil { diff --git a/docs/sources/reference/vinylcache-spec.md b/docs/sources/reference/vinylcache-spec.md index 797a30f..cb63729 100644 --- a/docs/sources/reference/vinylcache-spec.md +++ b/docs/sources/reference/vinylcache-spec.md @@ -70,9 +70,12 @@ See the [per-backend directors how-to](../how-to/per-backend-directors.md) for w | `shard.by` | string | `HASH` | Shard key source (`HASH`, `URL`). | | `shard.healthy` | string | `CHOSEN` | Health evaluation strategy (`CHOSEN`, `ALL`). | -> Note: `shard.by` and `shard.healthy` are accepted and persisted on the -> resource, but are not yet consumed by the generator for per-backend -> directors (see follow-up `shard-params-not-plumbed`). +> Note on `shard.by` and `shard.healthy`: +> - **Cluster-peer director** (when `cluster.enabled: true`): honored automatically — +> the operator emits `.backend(by=, healthy=)` in `vcl_recv`. +> - **Per-backend directors**: still request-time arguments. The CRD accepts +> the fields but you must use them in your own VCL snippet, e.g. +> `set req.backend_hint = plone.backend(by=HASH, healthy=CHOSEN);`. ### cluster @@ -86,8 +89,25 @@ See the [per-backend directors how-to](../how-to/per-backend-directors.md) for w | Field | Type | Default | Description | |-------|------|---------|-------------| | `purge.soft` | boolean | `true` | Use soft purge (stale-while-revalidate). | +| `purge.allowedSources` | list | `[]` | CIDRs permitted to send `PURGE`. `127.0.0.1` and the operator pod IP are always included. | | `xkey` | object | nil | Xkey (surrogate key) configuration. When set, `vmod_xkey` is loaded. | | `xkey.softPurge` | boolean | `true` | Use soft purge for xkey invalidation. | +| `ban.enabled` | boolean | `false` | When `true`, emit a `vinyl_ban_allowed` ACL and a `BAN` handler in `vcl_recv` that dispatches `std.ban(req.http.X-Vinyl-Ban)`. Also emits ban-lurker-friendly `x-url`/`x-host` headers on stored objects. | +| `ban.allowedSources` | list | `[]` | CIDRs permitted to send `BAN` (in addition to `127.0.0.1` and the operator pod IP). | +| `ban.rateLimitPerMinute` | integer | `0` | Inert in v0.4.2 — field is accepted but not enforced; rate-limiting is tracked as follow-up work. | + +> Note on BAN security: any client whose source IP is in `vinyl_ban_allowed` +> can invalidate the entire cache with an arbitrary ban expression. Scope +> `ban.allowedSources` tightly to trusted callers only. Ban expressions +> should prefer `obj.http.x-url` / `obj.http.x-host` over `req.*` fields — +> only the former can be processed by the ban lurker, so `req.*` bans +> accumulate without being compacted. +> +> Clients send BAN requests like: +> +> ```bash +> curl -X BAN -H 'X-Vinyl-Ban: obj.http.x-url ~ "^/news/"' http://varnish.svc/ +> ``` ## Status fields diff --git a/e2e/tests/vcl-validation/chainsaw-test.yaml b/e2e/tests/vcl-validation/chainsaw-test.yaml index 0926ef0..dc7d298 100644 --- a/e2e/tests/vcl-validation/chainsaw-test.yaml +++ b/e2e/tests/vcl-validation/chainsaw-test.yaml @@ -11,6 +11,7 @@ spec: apply: 10s assert: 60s delete: 60s + cleanup: 60s exec: 30s steps: - name: deploy-backend diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go index 86d8dec..2b589b0 100644 --- a/internal/generator/generator_test.go +++ b/internal/generator/generator_test.go @@ -852,16 +852,67 @@ func TestGenerate_BAN_EnabledEmitsACLAndHandler(t *testing.T) { input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{Enabled: true} r, err := g.Generate(input) require.NoError(t, err) + + // Extract just the BAN block so assertions don't accidentally match + // substrings emitted elsewhere in the VCL. + banBlock := strings.SplitAfter(r.VCL, `if (req.method == "BAN")`)[1] + banBlock = strings.SplitN(banBlock, "# Only handle GET", 2)[0] + assert.Contains(t, r.VCL, "acl vinyl_ban_allowed {", "BAN ACL must be emitted when ban.enabled=true") - assert.Contains(t, r.VCL, `req.method == "BAN"`, - "BAN handler must be emitted in vcl_recv") - assert.Contains(t, r.VCL, `client.ip ~ vinyl_ban_allowed`, + assert.Contains(t, banBlock, `client.ip ~ vinyl_ban_allowed`, "BAN handler must check the BAN ACL") - assert.Contains(t, r.VCL, `ban(req.http.X-Vinyl-Ban)`, - "BAN handler must call ban() with the X-Vinyl-Ban header value") - assert.Contains(t, r.VCL, `return(synth(200, "Banned"))`, + assert.Contains(t, banBlock, `synth(403, "Forbidden")`, + "BAN handler must 403 outside the ACL") + assert.Contains(t, banBlock, `synth(400, "Missing X-Vinyl-Ban header")`, + "BAN handler must 400 when X-Vinyl-Ban header is absent") + assert.Contains(t, banBlock, `std.ban(req.http.X-Vinyl-Ban)`, + "BAN handler must use std.ban(), not deprecated ban()") + assert.Contains(t, banBlock, `std.ban_error()`, + "BAN handler must surface std.ban_error() on malformed expressions") + assert.Contains(t, banBlock, `return(synth(200, "Banned"))`, "BAN handler must return synth 200 on success") + + // Hard regression guard: the bare ban() call form must never be emitted. + // (Matches the original architecture checklist: always use std.ban().) + // std.ban(...) is fine; only the bare form (preceded by whitespace, not + // by `.`) is forbidden. + assert.NotContains(t, r.VCL, " ban(req.http.X-Vinyl-Ban)", + "deprecated bare ban() call must not appear; use std.ban()") +} + +func TestGenerate_BAN_BanLurkerHeadersEmitted(t *testing.T) { + g := newGenerator(t) + input := makeMinimalInput() + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{Enabled: true} + r, err := g.Generate(input) + require.NoError(t, err) + assert.Contains(t, r.VCL, "beresp.http.x-url = bereq.url", + "BAN requires x-url ban-lurker-friendly header (otherwise bans accumulate O(n*m))") + assert.Contains(t, r.VCL, "beresp.http.x-host = bereq.http.host", + "BAN requires x-host ban-lurker-friendly header") + assert.Contains(t, r.VCL, "unset resp.http.x-url", + "BAN-support internal headers must be stripped before delivery") + assert.Contains(t, r.VCL, "unset resp.http.x-host", + "BAN-support internal headers must be stripped before delivery") +} + +func TestGenerate_BAN_RateLimit_CurrentlyInert(t *testing.T) { + // Regression guard: rate-limit field is accepted on the CR but not + // plumbed into VCL (tracked as a BAN follow-up). This test locks in + // the deferred state so nobody half-implements it. + g := newGenerator(t) + input := makeMinimalInput() + input.Spec.Invalidation.BAN = &vinylv1alpha1.BANSpec{ + Enabled: true, + RateLimitPerMinute: 60, + } + r, err := g.Generate(input) + require.NoError(t, err) + assert.NotContains(t, r.VCL, "vsthrottle", + "vsthrottle VMOD must not be imported until rate-limiting is wired") + assert.NotContains(t, r.VCL, "Rate limited", + "no rate-limit synth message until the feature is implemented") } func TestGenerate_BAN_OperatorIPInACL(t *testing.T) { diff --git a/internal/generator/templates/vcl_backend_response.vcl.tmpl b/internal/generator/templates/vcl_backend_response.vcl.tmpl index c0cb216..f1ea413 100644 --- a/internal/generator/templates/vcl_backend_response.vcl.tmpl +++ b/internal/generator/templates/vcl_backend_response.vcl.tmpl @@ -10,7 +10,7 @@ sub vcl_backend_response { if (beresp.grace < 24h) { set beresp.grace = 24h; } -{{- if .HasXkey }} +{{- if or .HasXkey .HasBAN }} # Copy URL and Host for BAN lurker friendliness (ban-lurker cannot access req.*) set beresp.http.x-url = bereq.url; diff --git a/internal/generator/templates/vcl_deliver.vcl.tmpl b/internal/generator/templates/vcl_deliver.vcl.tmpl index 032e0e3..f078564 100644 --- a/internal/generator/templates/vcl_deliver.vcl.tmpl +++ b/internal/generator/templates/vcl_deliver.vcl.tmpl @@ -1,6 +1,6 @@ sub vcl_deliver { -{{- if .HasXkey }} +{{- if or .HasXkey .HasBAN }} # Strip internal BAN-support headers before sending to client unset resp.http.x-url; unset resp.http.x-host; diff --git a/internal/generator/templates/vcl_recv.vcl.tmpl b/internal/generator/templates/vcl_recv.vcl.tmpl index 640f0c3..d7c3f04 100644 --- a/internal/generator/templates/vcl_recv.vcl.tmpl +++ b/internal/generator/templates/vcl_recv.vcl.tmpl @@ -47,8 +47,11 @@ sub vcl_recv { {{- end }} return(purge); } -{{ if .HasBAN }} - # BAN handling +{{- if .HasBAN }} + + # BAN handling — std.ban() preferred over deprecated ban() (Varnish >= 6.6). + # std.ban() returns true on success, false on malformed expression; + # std.ban_error() returns the last compile error for this session. if (req.method == "BAN") { if (!client.ip ~ vinyl_ban_allowed) { return(synth(403, "Forbidden")); @@ -56,7 +59,9 @@ sub vcl_recv { if (!req.http.X-Vinyl-Ban) { return(synth(400, "Missing X-Vinyl-Ban header")); } - ban(req.http.X-Vinyl-Ban); + if (!std.ban(req.http.X-Vinyl-Ban)) { + return(synth(400, "Invalid ban expression: " + std.ban_error())); + } return(synth(200, "Banned")); } {{- end }}