Conversation
The proxy is a full Fetch codec: it decodes, merges and re-encodes broker Fetch responses. A proxy-to-broker Fetch serialization or connection-state mismatch therefore breaks consume while Metadata and ListOffsets keep answering normally. Readiness based on "a backend exists" reports ready in that state, Kubernetes keeps the pod in the Service, and every consumer silently receives zero records. /readyz now answers two questions: is a backend known, and does that backend actually serve Fetch. The deep gate sends a Fetch for one partition of a deliberately unknown topic ID on a fresh connection, so it needs no real topic, reads no data, and also detects a stale or half-open pooled connection. A correct broker answers UNKNOWN_TOPIC_ID. KAFSCALE_PROXY_READYZ_FETCH_PROBE=false falls back to the shallow check. It is an escape hatch for a false negative in the field, not a steady state. Framing is the part worth guarding. forwardToBackend prepends the frame length itself, and encodeFetchRequest already strips the size prefix that kmsg's AppendRequest emits, so the probe payload must be the bare request header plus body. Stripping a second time removes the API key and version: the request is then not a malformed Fetch but a different API, and a broker that cannot parse a request header closes the connection. The probe reports EOF and the failure reads like a broken broker rather than a broken probe. buildFetchProbePayload is split out so that encoding is unit-testable without a backend. Two tests state the invariant from both sides: the payload parses back as Fetch v13 with the probe's topic ID, and a payload that has lost its four header bytes must not parse as Fetch, so the first test cannot silently stop distinguishing the bug. Startup ordering is a consequence worth documenting: with the probe enabled the proxy stays NotReady until a broker serves Fetch, so an installer that waits for proxy readiness before creating the resource that produces the broker will deadlock. - cmd/proxy/main.go: split checkReady into haveBackend plus the deep gate, add fetchProbe, buildFetchProbePayload and envBoolDefault - cmd/proxy/main_test.go: TestBuildFetchProbePayloadIsSingleFramed, TestBuildFetchProbePayloadRejectsDoubleStrip, TestEnvBoolDefault - docs/operations.md: "Proxy readiness" section and the env var index entry
PaxMachinaOne
left a comment
Collaborator
There was a problem hiding this comment.
🤖 PaxMachina automated review (worker-code)
The Fetch probe’s timeout does not bound backend I/O.
- Blocking:
cmd/proxy/main.go:481—probeCtxis not applied as a connection deadline, whileforwardToBackendperforms context-unaware blocking reads/writes. A backend that accepts TCP but never returns a complete frame can therefore hang/readyzindefinitely, accumulating connections and handler goroutines as probes repeat. Set the connection deadline fromprobeCtxor make the forwarding I/O cancellation-aware.
This branch has not been deployed
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
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.
What
/readyzon the proxy answers two questions instead of one:haveBackend)KAFSCALE_PROXY_READYZ_FETCH_PROBE=falsefalls back to the shallow check.Why
The proxy is a full Fetch codec: it decodes, merges and re-encodes broker Fetch
responses. A proxy-to-broker Fetch serialization or connection-state mismatch
therefore breaks consume while Metadata and ListOffsets keep answering
normally.
In that state readiness based on question 1 alone reports
ready, Kuberneteskeeps the pod in the Service, and every consumer silently receives zero
records. The failure is invisible to
kubectl get podsand to any health checkthat only asks whether a process is up.
The probe asks for one partition of a deliberately unknown topic ID, so it
needs no real topic and reads no data; a correct broker answers
UNKNOWN_TOPIC_ID. It uses a fresh connection on purpose, which also detects astale or half-open pooled connection that cached state cannot.
The part worth reviewing: framing
forwardToBackendprepends the frame length itself, andencodeFetchRequestalready strips the size prefix that kmsg's
AppendRequestemits. The probepayload must therefore be the bare request header plus body.
Stripping a second time removes the API key and version, four bytes. The
request is then not a malformed Fetch, it is a different API: the broker reads
the correlation ID as the API key. A broker that cannot parse a request header
closes the connection, so the probe reports
EOFand the failure reads like abroken broker rather than a broken probe.
buildFetchProbePayloadis split out so the encoding is unit-testable without abackend.
Startup ordering
With the probe enabled the proxy stays NotReady until a broker serves Fetch. An
installer that waits for proxy readiness before creating the cluster resource
that produces the broker will deadlock. Create the broker first, or do not gate
the install on proxy readiness. Documented in
docs/operations.md.Tests
TestBuildFetchProbePayloadIsSingleFramedReplicaID -1TestBuildFetchProbePayloadRejectsDoubleStripTestEnvBoolDefaultBoth framing tests were checked against a deliberately broken encoder:
need 26227 have 98is what the broker sees before it closes the connection.Verification
gofmt -l .clean,go vet ./cmd/... ./pkg/...cleango test ./cmd/proxyokgo test ./...22 packages ok, 0 failuresVerified end to end on a single-node k3s edge deployment: with the probe
correctly framed the proxy reaches Ready and
helm --waitcompletes; produceand consume of 500 messages through the proxy round-trip with no gaps,
duplicates or checksum errors.
Scope
Three files, one concern. No behaviour change when
KAFSCALE_PROXY_READYZ_FETCH_PROBE=false.