Repository navigation
feat: VCL trace-context rewrite (tracing P2) - #108
Merged
Merged
Conversation
8 TDD tasks: generator HasTracing flag + uuid import + recv validation + backend_fetch minting, P2 fixtures recorded with the tracing VCL active, tracer adoption of VCL-minted ids (incl. self-root trace adoption and the unrewritten-forward guard), vinylprobe -body-capture and span-id filters, first real envtest admission assertions, E2E rewrite proof via the echo backend's reflected traceparent, spec corrections, verification battery. Assisted-by: Claude Fable 5
P1 added spec.tracing to the CRD and the tracer sidecar; this makes generated VCL participate. Gated by a new HasTracing flag (Spec.Tracing.Enabled): - main.vcl.tmpl imports vmod uuid when tracing is enabled. - vcl_recv validates an inbound traceparent against the W3C trace-context shape and drops all-zero trace/parent ids, before the PURGE/BAN/cluster-routing blocks — those return(pass)/return(synth) early and would skip validation placed any later. - vcl_backend_fetch mints a fresh span id into bereq.http.traceparent (rewriting the parent-id segment on an existing header, or minting a full self-rooted header when none arrived) so the tracer can adopt it as the "varnish fetch" span's id from the VSL BereqHeader record. TDD: five new tests in generator_test.go cover disabled (no tracing content emitted), uuid import, recv validation + its required ordering before PURGE, backend_fetch minting, and coexistence with operator-authored VCL snippets. Full ./internal/generator/ suite, whole-repo build/test, and make lint all green. Assisted-by: Claude Fable 5
Adds record-fixtures-tracing.sh, which runs varnish:8.0.2 with an
embedded VCL mirroring the tracing blocks of
internal/generator/templates/{vcl_recv,vcl_backend_fetch}.vcl.tmpl
(rendered by hand for a static backend instead of the k8s backend
list the generator needs). This is the first time that VCL meets a
real varnishd; it loaded and ran without incident.
rewrite_miss.txt: a request carrying a valid traceparent gets its
trace id preserved and its span id rewritten by vcl_backend_fetch.
rewrite_selfroot.txt: a request with no traceparent gets a full
VCL-minted one. Both confirmed lowercase by eye per testdata/README.md.
Assisted-by: Claude Fable 5
Build now adopts the fetch span id (and trace id, when the client sent none) that the P2 VCL snippet mints onto bereq's traceparent, so the backend's own spans land under the fetch span instead of as its sibling. A BereqHeader span id equal to the incoming parent id is an unrewritten P1-style forward, not a minted one, and is ignored. Also fix vsl.Tx.Header to return the last matching record instead of the first: VCL logs an unset-then-reset pair when it rewrites a header, so the first match is the pre-rewrite value. This only affects headers VCL rewrites (BereqHeader traceparent); the existing single-occurrence ReqHeader lookups are unaffected. Assisted-by: Claude Fable 5
The previous commit's fix to vsl.Tx.Header (return the last matching record so a VCL unset+re-add resolves to the rewritten value) was only proven indirectly, through spans_test.go's adoption tests. Add TestHeader_LastMatchWinsOnRewrittenHeader against the real rewrite_miss.txt fixture, whose BeReq child genuinely carries the forwarded "Traceparent", a BereqUnset, and the VCL-minted lowercase "traceparent" re-add. Verified this goes red on a first-match revert (confirmed by hand before committing) and green on the current implementation. Assisted-by: Claude Fable 5
Adds probe.BodyCapture (internal/probe/cache.go) for GET-once regex capture against a response body, used by vinylprobe -body-capture to print the first capture group (exit 0), FAIL on no match (exit 1), or error on bad regex/transport (exit 2) -- the same stdout contract as -seed's token, for E2E steps that need to read back backend-reflected state such as a VCL-minted traceparent. decideSpans gains traceID/spanID equality filters, exposed as -assert-spans's new -trace-id/-span-id flags, and -assert-spans's FAIL message now appends the last poll iteration's transport error when present, instead of hiding it behind a silent retry. Assisted-by: Claude Fable 5
Adds the first two envtest It blocks in the v1alpha1 webhook suite, proving the API-server round trip (webhook wired, failurePolicy effective) rather than just the pure ValidateVinylCache function (already unit-tested in internal/webhook/vinylcache_validator_test.go). minimalAdmissibleVC mirrors the unit suite's minimalValidVC fixture verbatim, since test helpers cannot be imported across packages. Assisted-by: Claude Fable 5
Adds the rewrite-proof step between assert-spans and cleanup: a fresh-path request captures the backend-reflected traceparent the VCL minted onto bereq (vinylprobe -body-capture), then asserts a "varnish fetch" span with exactly that trace/span id and a "varnish request" span in the same trace with no client traceparent sent, proving self-root trace adoption. Updates the suite description's NOT COVERED line now that P2 trace-context rewrite is covered. Assisted-by: Claude Fable 5
The step's worst case (30s -body-capture default timeout + 60s + 30s -assert-spans polls = 120s) exactly equalled the script timeout, leaving zero margin for the three kubectl exec spawns' overhead -- unlike the sibling assert-spans step, which documents and budgets a deliberate margin over its own worst case. Raise the script timeout to 180s and document the arithmetic in the step description, mirroring assert-spans. Assisted-by: Claude Fable 5
- Image name: vinyl-tracer → cloud-vinyl-tracer (shipped name) - VCL injection: clarify that snippet is generated via HasTracing flag - X-Cache headers: describe deployment concern for cache-status capture - Webhook responsibility: defaults applied controller-side, webhook validates - Testing section: remove reference to webhook defaulting (validation only) Assisted-by: Claude Fable 5
vcl_recv's tracing gate unset malformed and all-zero traceparents but
let version "ff" through (the shape regex allows any [0-9a-f]{2}
version byte). parseTraceparent rejects ff per W3C trace-context, so
an ff header passed the VCL gate, got a bereq id minted under it, and
the tracer adopted nothing: request and fetch spans landed in a
random self-rooted trace with the backend left disconnected from it.
Add an unset condition for "^ff-" alongside the existing all-zero
checks, and mirror it into record-fixtures-tracing.sh's embedded VCL
so the two stay logic-identical per the keep-in-sync comment there.
Recorded fixtures are unaffected (inputs don't touch this path).
TDD: extended TestGenerate_TracingEnabled_RecvValidatesTraceparent to
assert the rendered VCL contains the ^ff- unset condition; confirmed
RED before the template fix, GREEN after.
Assisted-by: Claude Fable 5
The committed plan doc's global constraints told agentic workers to end commits with the outdated attribution form, which is outdated: repo policy is Assisted-by: Claude Fable 5, never outdated-attribution, never a no-reply address. Assisted-by: Claude Fable 5
Two comment-only clarifications from the whole-branch review: - vsl.Tx.Header: note that *Unset records are not processed, so a header that ends unset in the real request still returns its last set value here. Callers relying on this for "what the backend saw" are only safe while their own parser is stricter than the VCL gate that would have unset it. - spans.Build's adoption comment: note that with multiple direct BeReq children under a self-root, only the first valid child's trace id is adopted (mintedTraceID is set once); later children's backends would land in orphaned traces of their own. Practically unreachable today (one BeReq per top-level Request); P3 revisits this with retry/ESI fixtures. Assisted-by: Claude Fable 5
CI's Unit Tests job runs bare `go test -race ./...` with no KUBEBUILDER_ASSETS. Before this branch, internal/webhook/v1alpha1's Ginkgo suite had zero It blocks, so BeforeSuite never actually ran there; the new admission specs (d0b0a58) start envtest on every plain `go test`, which fails in CI with "fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory". Tag webhook_suite_test.go and vinylcache_webhook_test.go with `//go:build integration`, matching internal/controller's existing suite_test.go/vinylcache_controller_test.go convention. The suite now only runs via `make test-int` (which passes -tags=integration); plain `go test ./...` and `make test` report "no test files" for the package, same as the controller suite already does. Assisted-by: Claude Fable 5
In CI, e2e/tests/tracing's traffic-miss-then-hit step got "connect: connection refused" (vinylprobe exit 2) a few seconds after the StatefulSet's readyReplicas assert had already gone green — a readiness flap right at startup, not a caching bug; the parallel untraced copy of this test passed a minute later. vinylprobe separates transport failure (exit 2) from a wrong verdict (exit 1) for exactly this: wrap the single vinylprobe call in a loop that retries only on exit 2 (up to 5 times, 5s apart) and lets exit 0 or exit 1 through immediately, so a real assertion failure still fails fast instead of being retried into a false pass. Documented in the step description why the added sleep is acceptable here (bounded transport-error backoff, not an assertion poll — vinylprobe still owns every assertion) and why the ~30s worst case still fits the step's existing spec-level exec:120s budget without raising it. Verified: python3 yaml.safe_load, hack/check-suite-labels.sh, and hack/check-e2e-boundary.sh all pass on the updated file. Assisted-by: Claude Fable 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.
Summary
P2 of OpenTelemetry tracing, per the (herein corrected) design spec: the backend becomes a proper child of the Varnish fetch span.
HasTracingflag, the xkey/esi pattern; user snippet fields untouched):import uuid;, a traceparent-validation block early invcl_recv(W3C shape, all-zero ids, and the W3C-forbiddenffversion all unset; placed before the PURGE/cluster-routingreturn(pass)paths), and a minting block invcl_backend_fetchthat rewrites the span id onbereq.http.traceparent(trace id and flags preserved; full self-root mint when the client sent nothing; every hex throughstd.tolower).spans.Buildadopts the VCL-minted fetch span id from the VSLBereqHeaderrecord — and, for self-rooted traffic, the minted trace id — with a guard so an unrewritten P1-style forward is never adopted.vsl.Header()now returns the last matching record (the VSL logs the forwarded header, an unset, and the re-add; last is what the backend received), pinned by a dedicated test against the recorded fixture.rewrite_miss,rewrite_selfroot) — this recording was also the first live varnishd validation of the VCL, which loaded first try.-body-capture(print a regex capture from a response body) and-trace-id/-span-idfilters on-assert-spans, plus transport errors surfaced in the FAIL line.varnish fetchspan with exactly that trace and span id, and avarnish requestspan in the same trace (also proving self-root trace adoption).Verification
make lint,make test,make test-int(incl. the new admission specs),helm unittest(59/59), both hack checks, zero generated-file drift — all green at HEAD. Every task passed spec+quality review; the final whole-branch review's one functional finding (theff-version gate divergence) is fixed in the last commits and re-reviewed.Note for reviewers
The rewrite-proof E2E step has never executed before this PR's CI run. Please do not merge before the fast suite is green. Its budget arithmetic is documented in the step (worst case 120s under a 180s ceiling).
Follow-ups (P3 scope)
Coalescing/grace/ESI truth semantics; retry/ESI bereqs are nested VSL grandchildren the builder does not yet visit (pre-existing P1 gap); multi-child self-root and duplicate-distinct-traceparent hardening.
Assisted-by: Claude Fable 5