fix(ocudu): classify every non-101 handshake answer, not just the 3xx/4xx band - #318
Merged
Conversation
thc1006
force-pushed
the
fix/handshake-status-classification
branch
from
July 31, 2026 02:32
aa12a83 to
27ed9e3
Compare
…/4xx band The classifier keyed on a status BAND — 300 <= code < 500 — so anything outside it fell through to "gNB unreachable" and the tight per-minute cadence. coder/websocket accepts only a valid 101 and returns a NON-NIL *http.Response for every other outcome. Verified against the vendored v1.8.15 by probing each case and inspecting resp directly: 200, 204, 302, 401, 500, 503 and a 101 whose handshake headers are invalid ALL come back with a non-nil response carrying that status. So the band silently missed three real misconfigurations, each of them deterministic and each hammered once a minute forever: - a plain 200 from an HTTP server on the wrong port - a 204 from a proxy that swallowed the Upgrade - a 101 whose Sec-WebSocket-Accept / Upgrade header / subprotocol is invalid Classification now keys on the response being PRESENT rather than on a band: a definitive answer means the endpoint is reachable and is either refusing us or is not a WebSocket server, which the next minute cannot change, so it takes the bounded self-heal poll. Exceptions stay transient: 5xx (a server-side blip) and 408/425 (RFC 9110 15.5.9, RFC 8470 5.2 — repeat the same request). 501 Not Implemented and 505 HTTP Version Not Supported are deliberately carved OUT of the 5xx exception. They say the server is INCAPABLE of the request rather than momentarily struggling, so treating them as transient would re-introduce the very mistake this commit fixes, just at a different boundary. That carve-out is mine, not the reviewer's — it fell out of asking whether the new rule was itself principled or merely a better-placed band. A malformed 101 carries the library's own reason, which names the offending header; flattening it to "dial failed" threw away the entire diagnostic. Not adopted: a distinct typed reason for 401/403. It would change no behaviour (both already take the same poll), the status is already in the condition message, and it cuts against #296's deliberate unification of credential-failure reasons. Mutation-verified, each reverting only itself: - restore the 3xx/4xx band -> 200, 204 and the malformed-101 cases redden - drop the 5xx exception -> only 500 and 503 redden - fold 501/505 back into 5xx -> only those two redden
thc1006
force-pushed
the
fix/handshake-classify-non101
branch
from
July 31, 2026 02:58
f2864fe to
301d30a
Compare
This was referenced Jul 31, 2026
thc1006
added a commit
that referenced
this pull request
Jul 31, 2026
test/e2e/ contained ZERO tests touching remoteControl, wss or mTLS. Everything under #206, #295, #297, #313, #318 and #322 was unit-tested against httptest, which cannot show the deployment shape works: OCUDU's remote_control server is plaintext, so the feature only exists behind a TLS-terminating proxy — and that proxy shipped as a sample nobody executed. Design choices, each load-bearing: - NO IMAGE BUILD. The gNB stand-in is stdlib-only Go (its own RFC 6455 handshake and framing), mounted from a ConfigMap and run with the stock golang image; the proxy is stock nginx. So the suite runs identically on Kind in CI and on a plain kubeadm cluster — no Docker, no registry, no `kind load`. Verified: the stub interops with the operator's real coder/websocket client, and starts in-cluster from the ConfigMap with no network at container start. - The nginx config is READ FROM the shipped sample, so the sample itself is what is under test. A copy inlined here would drift and let the sample rot green. - The assertion is WHAT CROSSED THE WIRE, read back out of the plaintext backend. A condition flipping to True proves the operator's opinion, not the chain. - The suite is SELF-CONTAINED: it patches the manager's --ephemeris-allowed-private-hosts if absent, because the GP mock sits on a private ClusterIP and the SSRF-safe fetcher would otherwise block it — a failure that reads as "the push is broken" but is not. Proven by stripping the flag and re-running: the suite re-adds it and still passes. - The GP fixture carries EPOCH=now. The checked-in testdata is months old and sourceEpochFresh (maxEpochAge 7d) would refuse it, which is why the runtime push has never actually been reachable from e2e. Four arms, the last three each also asserting that NOTHING reached the backend: happy path (frame validated for cmd/plmn/nci/future-epoch), wrong bearer -> RemoteEndpointRejected, token without ca.crt -> refused before any dial (#313), dropped client certificate -> refused by the proxy. Adversarial pass on the test itself: the arms first waited only for a False prefix, so an arm could assert while the PREVIOUS arm's reason was still on the object — intermittent failure, the worst kind. Each wait is now reason-specific, and every arm restores and waits for True/Pushed so the next starts from a known baseline. Mutation-checked: an arm that skips its own mutation fails. CI guards the same false-green classes the HA step does — a build-tag typo or a rename matching zero tests, a silently deleted arm, and (specific to this suite) a run where no ntn_config_update frame ever crossed the wire.
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.
The review is right, and it caught something I looked at and waved through
While adversarially reviewing #297 I explicitly considered the 2xx case and talked myself out of it — "a 200-to-Upgrade is vanishingly rare vs 401/429, and tight-retrying it is not harmful (it's the status quo)". That reasoning does not survive contact with the rest of #297: the same argument would have justified leaving
404on the tight cadence, and #297 moved404. Keeping a status band while claiming to classify by what fixes the failure was inconsistent, and the reviewer found the seam.The premise, verified rather than assumed
The whole review rests on "resp is still non-nil". I probed the vendored v1.8.15 directly and inspected
resp:err != nilSec-WebSocket-AcceptUpgradeheaderAll nine were being classified
wsUnreachable→ tight per-minute retry. Confirmed by a second probe that ran them throughpushNTNConfigUpdateitself.So the band silently missed three real, deterministic misconfigurations — a plain 200 from an HTTP server on the wrong port, a 204 from a proxy that swallowed the
Upgrade, and a malformed 101 — and hammered each of them once a minute, forever.Change
Classification keys on the response being present, not on a band. A definitive answer means the endpoint is reachable and is either refusing us or is not a WebSocket server, which the next minute cannot change → bounded self-heal poll. Transient exceptions:
5xx, and408/425(RFC 9110 §15.5.9, RFC 8470 §5.2).A malformed 101 now carries the library's own reason, which names the offending header — flattening that to
dial failedthrew away the entire diagnostic.One carve-out that is mine, not the reviewer's
501 Not Implementedand505 HTTP Version Not Supportedare excluded from the 5xx exception. They say the server is incapable of the request, not that it is momentarily struggling — so banding them as transient would re-introduce the exact mistake this PR fixes, one boundary to the right. This fell out of asking whether the new rule was actually principled or just a better-placed band.Not adopted
A distinct typed reason for 401/403. It changes no behaviour (both already take the same poll), the HTTP status is already in the condition message, and it cuts against #296's deliberate unification of credential-failure reasons. Declined as surface without effect.
Verification
go build,go vet,golangci-lint v2.12.2: 0 issues;gofmtclean.go test ./internal/controller/... ./pkg/...: 8 packages ok.3xx/4xxband →200,204and the malformed-101 cases fail500and503fail501/505back into 5xx → only those two failNew coverage:
200/204/501/505rows in the status table, plusTestPushNTNConfigUpdate_Malformed101IsRejectedNotUnreachabledriving three distinct invalid upgrades through a hijacking test server.Sources: coder/websocket #316 (non-101 handshake error shape), coder/websocket #461.