Repository navigation
fix: make varnishParameters reach varnishd, and give ESI a real switch (#109) - #112
Merged
Merged
Conversation
spec.varnishParameters was documented as "runtime parameters passed to varnishd via -p flags" and carried a webhook blocklist, but nothing ever emitted -p args: the field was inert. Add varnishParamArgs, appended after the fixed -j/-T/-S and -s storage args in the varnish container's Args. Entries are sorted by key before being rendered: Go map iteration order is randomized per process, so an unsorted args slice would make the pod template differ on every reconcile for unchanged params, and controllerutil.CreateOrUpdate would roll the StatefulSet forever. Covers both the pure helper (empty/nil, single, and a 7-key sorted case that would almost certainly fail on an unsorted implementation) and the reconcile path (args present, absent when unset, ordering relative to the fixed and storage args). Assisted-by: Claude Opus 5
The generator gated beresp.do_esi on VarnishParams["feature +esi"] — a key that was never the intended convention, just an artifact, and impossible to set correctly: varnishd's actual "feature" parameter takes one comma-separated list of tokens (e.g. "+esi,+esi_disable_xml_check"), not one map entry per flag. Switch to parsing VarnishParams["feature"] as that comma-separated list and matching the exact "+esi" token. A substring check (strings.Contains(v, "+esi")) would be wrong: it also matches "+esi_disable_xml_check", which would silently turn on ESI processing for a param that only meant to disable XML sniffing. Updates the two existing tests that used the old "feature +esi": "on" shape to the real "feature": "+esi" convention, and adds two cases: "+esi_disable_xml_check" alone must not enable do_esi (the substring trap), and "+esi,+esi_disable_xml_check" together must. Assisted-by: Claude Opus 5
…vention Now that spec.varnishParameters actually reaches varnishd (-p args) and ESI detection uses the real "feature" parameter, spell out the convention where it was previously either wrong or entirely absent: - CRD doc comment: bare parameter names as keys (e.g. thread_pool_min: "100"), the comma-separated "feature" list for ESI (feature: "+esi", or "+esi,+esi_disable_xml_check" when response bodies don't start with '<'), and the webhook blocklist. - Regenerated config/crd/bases via `make manifests generate` and synced the manually-maintained chart copy at charts/cloud-vinyl/crds/vinylcache.yaml (plain cp, per this repo's established practice — see 78a5a3a). - docs/sources/reference/vinylcache-spec.md had no varnishParameters section at all; added one with the same convention and examples. Assisted-by: Claude Opus 5
…listed ones forbiddenVarnishParams[k] was a bare, unnormalized map lookup. Before this branch that was harmless because nothing consumed VarnishParams; now varnishParamArgs (controller-side) renders the raw map key straight into the varnish container's argv, which turns the blocklist into a live security boundary — and an exact-string lookup is bypassable with padding or case: " cc_command", "cc_command ", or "CC_COMMAND" all slip past forbiddenVarnishParams while still reaching varnishd as (or close enough to) the blocked parameter. Add isValidVarnishParamKey, an allowlist shape check (^[a-z][a-z0-9_]*$, varnishd's own parameter-name shape) run unconditionally in ValidateVinylCache alongside the existing blocklist check, with its own error message. This closes the whole class of normalization bypasses in one rule instead of trying to enumerate them. Tests (internal/webhook/vinylcache_validator_test.go): leading- and trailing-space padded keys rejected, an uppercase key rejected, a legitimate lowercase key accepted, a cleanly-blocked key still rejected by the blocklist, and a blocked key with padding rejected — specifically by the shape rule, since the blocklist's exact lookup does not and should not catch it. Documents the now-enforced key shape in the CRD field comment and docs/sources/reference/vinylcache-spec.md, regenerates config/crd/bases via `make manifests generate`, and syncs the manually-maintained chart copy at charts/cloud-vinyl/crds/vinylcache.yaml (plain cp, per this repo's established practice). Assisted-by: Claude Opus 5
The varnishParameters example in vinylcache-spec.md listed thread_pool_min, thread_pool_max, feature in YAML-insertion order and then claimed a rendered -p line in that same order — but varnishParamArgs sorts by key, so the real order is feature, thread_pool_max, thread_pool_min (`f` < `t`, then `thread_pool_max` < `thread_pool_min` since 'a' < 'i'). Fixed the rendered line to match and kept the sorted-args sentence, since that's the point the example is making. Also pins an already-correct but previously untested behavior: varnishd's feature list uses a leading '-' to explicitly disable a feature, so "feature": "-esi" must not be mistaken for "+esi" by hasVarnishFeature's exact-token match (TestGenerate_ESI_ExplicitDisable_NoDoESI). Assisted-by: Claude Opus 5
The webhook-only fix from the previous commit (isValidVarnishParamKey in internal/webhook) has a gap: failurePolicy=fail on the ValidatingWebhookConfiguration only protects against the webhook being down at request time, not against it never having been installed at all — a cluster that applies these CRDs without also applying the operator's webhook configuration has no gate whatsoever on what varnishParamArgs renders into varnishd's argv. Add a CEL x-kubernetes-validations rule directly on spec.varnishParameters (api/v1alpha1/vinylcache_types.go) via +kubebuilder:validation:XValidation, enforcing the same ^[a-z][a-z0-9_]*$ shape at the API server's CRD schema layer — independent of, and prior to, any admission webhook. Regenerated config/crd/bases via `make manifests generate` (confirmed the x-kubernetes-validations entry is present on the varnishParameters property with the expected rule and message) and re-synced charts/cloud-vinyl/crds/vinylcache.yaml (verified byte-identical). Added two envtest specs to internal/webhook/v1alpha1's Ginkgo suite (the layer that runs a real API server plus the real webhook manager). Confirmed empirically, not just from Kubernetes docs, that CRD schema/CEL validation executes before the validating admission webhook chain is invoked: a shape-invalid-only key (THREAD_POOL_MIN) is rejected with the CEL rule's own message and no "admission webhook ... denied the request" wrapper, proving ValidateVinylCache is never reached for it; a shape-valid-but-blocklisted key (cc_command) is rejected with that wrapper instead, since only the webhook's blocklist catches it. Verified the first assertion isn't vacuous by temporarily stripping the x-kubernetes-validations block from the generated CRD and re-running: the same key then fell through to the webhook and failed the test's "no admission webhook wrapper" assertion exactly as expected, then restored the CRD and confirmed green again. self.all(k, ...) does iterate a map's keys in CEL as documented; this was exercised directly rather than assumed (the THREAD_POOL_MIN and cc_command specs above only pass because the rule evaluates per-key). Assisted-by: Claude Opus 5
…s exist CLAUDE.md's envtest bullet said the webhook suite "boots envtest but exercises no admission decision yet." That stopped being true with P3's tracing-validation specs, and this branch added two more (CRD-level CEL rejection vs. webhook rejection, in internal/webhook/v1alpha1's Ginkgo suite) — so the note was actively misleading about the layer this branch itself just exercised. Updated the sentence to say what's actually covered (still keeping the file's "say what a layer is for, not what it currently covers" framing, since that principle holds regardless of today's spec count). Assisted-by: Claude Opus 5
Unblocks the second half of #109: P3 proved ESI child spans at the unit level against recorded VSL fixtures (NOTES.md's esi.txt section), but no E2E was possible until spec.varnishParameters actually reached varnishd (fixed earlier on this branch). This adds that E2E. - e2e/fixtures/backends/esi-backend.yaml: a purpose-built ConfigMap+ python:3.12-slim backend (ealen/echo-server cannot emit ESI markup). Serves /esi/* with Surrogate-Control: content="ESI/1.0" and a body that deliberately does NOT start with '<' (mirrors internal/tracer/vsl/testdata/truthbackend.py and NOTES.md's "Rig deviation" scenario exactly), and /frag -> "FRAGMENT". - e2e/fixtures/vinylcaches/esi-cache.yaml: own CR name to avoid colliding with tracing/tracing-truth's caches. varnishParameters carries feature=+esi_disable_xml_check, the real varnishd token the non-'<' body needs. DELIBERATELY omits "+esi": the documented convention ({"feature": "+esi"} to gate generator.go's HasESI) turns out to be unusable against real varnishd — that map renders verbatim into varnishd's actual -p feature=... argument, and varnishd 8.0.2 has no "esi" feature bit at all. Verified by hand against a varnish:8.0.2 container: `-p feature=+esi` refuses to start ("Unknown feature bit (+esi)"), and rejects the whole value even combined with a real token. Worked around via the pre-existing vcl.snippets. vclBackendResponse mechanism to set beresp.do_esi directly instead (also verified by hand: produces correct assembly, "before FRAGMENT after"). This is a real bug in code already on this branch (generator.go's HasESI gate); out of scope for a fixtures-only change, flagged here and in the fixture's own comment for a follow-up issue. - e2e/tests/tracing-esi/chainsaw-test.yaml: suite: full. Reuses the sink+probe fixtures, transport-only retry on first traffic. Asserts assembly happened (-body-capture '(FRAGMENT)', proving inclusion rather than proxying — the raw backend body has no literal "FRAGMENT" text), the ESI child span (-span-name "varnish request" -attr varnish.esi=true), and a same-page parent span (url.path=/esi/page) as a sanity check. Trace-id linking between parent and child is NOT asserted: -assert-spans's stdout contract only ever prints a match count, never a matched span's trace/span id, so there is no non-fragile way to pull it back out for comparison; that claim is already pinned at the unit level. Validated: python3 yaml.safe_load, hack/check-suite-labels.sh, hack/check-e2e-boundary.sh, and `chainsaw lint test` all pass. First execution is CI, under the e2e-full label — no local cluster run was available here. Assisted-by: Claude Opus 5
…vention
The previously documented way to enable ESI — {"feature": "+esi"} in
spec.varnishParameters, matched by the generator's HasESI gate — is
unusable against real varnishd: that map renders verbatim into
varnishd's actual "-p feature=..." argument, and real varnishd 8.0.2
has no "esi" feature bit at all. Verified by hand against a
varnish:8.0.2 container: `-p feature=+esi` refuses to start at all
("Unknown feature bit (+esi)"), rejecting the whole feature value even
combined with a real token like esi_disable_xml_check. Any CR using
the documented convention would crash-loop its StatefulSet before any
ESI traffic could ever run.
Add spec.esi.enabled as a first-class boolean field instead. It never
reaches varnishd's argv, so it carries none of the above risk.
spec.varnishParameters' "feature" key remains relevant alongside it
for the one real varnishd feature bit ESI sometimes needs
(esi_disable_xml_check, for response bodies that don't start with
'<') — that part of the old guidance was correct and is kept.
Regenerates config/crd/bases and the chart's CRD copy (make manifests
generate) to match.
Assisted-by: Claude Opus 5
Companion to the previous commit's api change: HasESI now reads input.Spec.ESI.Enabled directly instead of exact-token-parsing spec.varnishParameters["feature"] for "+esi". Deletes the now-dead hasVarnishFeature helper. Test changes: - TestGenerate_ESI_NoVmodImport, TestGenerate_ESI_ EnablesDoESIOnSurrogateControl: rewritten to set spec.esi.enabled instead of the old VarnishParams["feature"] map; same assertions. - TestGenerate_NoESI_NoDoESI: unchanged, still passes (makeMinimalInput leaves esi.enabled unset/false). - TestGenerate_ESI_DisableXMLCheckAlone_NoDoESI, TestGenerate_ESI_MultiValueFeatureList_EnablesDoESI, TestGenerate_ESI_ExplicitDisable_NoDoESI: DELETED. These existed solely to pin hasVarnishFeature's comma-separated-list/substring-trap/ explicit-disable token parsing, a mechanism that no longer exists — there is nothing left for them to test. - TestGenerate_ESI_VarnishParamsFeatureIgnored: ADDED. Pins that varnishParameters["feature"] — including the exact "+esi" token the removed convention asked for — now has no effect on do_esi at all; only spec.esi.enabled does. This is the concrete regression guard for the fix itself. Assisted-by: Claude Opus 5
{"feature": "+esi"} was documented as how to turn ESI on; it crashes
real varnishd (see the api/generator commits earlier in this branch).
Removes that guidance, adds an "esi" section for the new
spec.esi.enabled field, and keeps varnishParameters' own example
limited to what's genuinely valid: thread_pool_min/max plus
feature=+esi_disable_xml_check (the one real varnishd feature bit ESI
sometimes still needs).
Assisted-by: Claude Opus 5
Follows up the earlier commit in this test's own history that flagged
{"feature": "+esi"} as broken against real varnishd and worked around
it via a vcl.snippets.vclBackendResponse injection. That workaround is
no longer needed: spec.esi.enabled (added earlier on this branch) is
now the real, supported way to turn ESI on.
esi-cache.yaml: esi.enabled: true replaces the vclBackendResponse
snippet; varnishParameters keeps feature=+esi_disable_xml_check, the
real token the backend's non-'<' body still needs. This combination is
the first fixture in the repo exercising both the new field and the
-p plumbing reaching varnishd for real, in one scenario.
chainsaw-test.yaml: description updated to match — drops the
BLOCKED-AND-WORKED-AROUND framing (the bug it described is fixed) and
explains the new field instead. Assertions, retry idiom, and budget
arithmetic are unchanged.
Re-validated: python3 yaml.safe_load, hack/check-suite-labels.sh,
hack/check-e2e-boundary.sh, and `chainsaw lint test` all pass.
Assisted-by: Claude Opus 5
…comment TestVarnishParamArgs_SortedByKey used "feature": "+esi" as arbitrary sort-order test data -- the exact value this branch declares crash-inducing for varnishd. Swap it for a neutral real varnish param (default_ttl) that preserves the pinned alphabetical order. The do_esi comment in vcl_backend_response.vcl.tmpl called esi_disable_xml_check "the esi feature flag enabled"; there is no such bit. do_esi is meaningful on its own, and feature=+esi_disable_xml_check only matters when a response body doesn't start with '<'. Reword the comment to say so. Assisted-by: Claude Opus 5
heading_anchors=0 means MyST never generates heading anchors for this project, so [varnishParameters](#varnishparameters) and [esi](#esi) (added alongside the varnishParameters/esi docs) resolved to nothing and failed the `-W` docs build. Replace them with plain prose pointers, matching the rest of this file which has no internal links. Assisted-by: Claude Opus 5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #109.
What was broken
Two layers, the second hidden behind the first:
spec.varnishParameterswent nowhere. The CRD field existed, was documented as "passed to varnishd via -p flags", and had a security blocklist — but no code ever emitted a-parg. Nothing a user set could reach varnishd.varnishParameters["feature"]containing+esi. There is no such varnishd feature bit:varnishd -p feature=+esianswersUnknown feature bit (+esi)and refuses to start. Verified by hand against varnish:8.0.2. So the documented path to ESI would have crash-looped the cache, and the real esi_* bits only modify ESI behaviour — ESI itself is enabled solely byberesp.do_esi.What this changes
-p key=value, sorted by key. The sort is load-bearing rather than cosmetic: Go map order is random, and an unsorted arg slice would make the pod template differ on every reconcile and roll the StatefulSet forever.spec.esi.enabled. The impossible feature-token gate is gone, along with the tests that pinned its parsing. Thedo_esiblock still fires only on the backend's standardSurrogate-Control: content="ESI/1.0"; bodies not starting with<additionally need the real bitfeature: "+esi_disable_xml_check", which is now settable for the first time.vcc_allow_inline_candcc_command" are blocked to prevent arbitrary C compilation from VCL, but the check was a bare map lookup:" cc_command"with a leading space walked straight past it. Harmless while parameters went nowhere; consequential the moment they reach a real process's argv. Keys must now match^[a-z][a-z0-9_]*$(the actual shape of varnishd parameter names), enforced **twice**: in the webhook, and by a CEL rule on the field itself so the gate also holds on a cluster that installed the CRDs without the admission webhook. The webhook'sfailurePolicy=fail` covers a webhook that is down, not one that was never installed.suite: full, hence this PR'se2e-fulllabel): a backend serving<esi:include>markup plus Surrogate-Control, a cache withesi.enabledand the real xml-check bit, and assertions that the fragment text actually appears in the assembled response and that an ESI child span carryingvarnish.esi=truereaches the collector. The page body contains no copy of the fragment token, so the assertion cannot pass without varnish performing the include.Verification
lint, unit, envtest (now including CEL-vs-webhook layer-distinguishing specs), helm unittest, both boundary/label checks, and zero CRD drift — all green. Each change was reviewed; reviewers independently reproduced the load-bearing proofs rather than trusting reports: the CEL backstop's non-vacuity (by stripping the validation and watching the test fall through to the webhook), the generator rewire (by reinstating the old token gate and watching the new tests fail), and the CRD codegen (by re-running controller-gen).
The ESI E2E has never executed — this PR's labelled run is its first.
Note
A stale
bin/controller-gen-v0.20.1in a local worktree actually contained v0.20.0, and the Makefile'sgo-install-toolreuses a cached binary by filename without checking its version, producing phantom CRD annotation drift. Local-only (bin/ is gitignored), but the cache check is a footgun worth knowing about.Assisted-by: Claude Opus 5