fix(egress-prober): fail loudly when the server rejects the byJwt - #4
Open
Ryanmello07 wants to merge 1 commit into
Open
fix(egress-prober): fail loudly when the server rejects the byJwt#4Ryanmello07 wants to merge 1 commit into
Ryanmello07 wants to merge 1 commit into
Conversation
parseByJwtClientId proves the token decodes and carries a client_id. It does not prove the server accepts it, and the gap between those two is an outage mode the prober cannot currently detect at all. The byJwt authenticates only the provider tunnel. The due queue, attempt reporting and pin fetch all authenticate with the operator secret. So when the jwt stops being accepted -- it expires (jwt.expiryDuration is 24h), or it predates a claim the server has since begun enforcing -- every operator-secret path keeps working. The prober fetches its batch, opens tunnels that carry nothing, and dutifully reports each provider as no_consensus with ok=0/N. Nothing in that output says "credential". It says "the entire fleet is bad", which is convincing enough that on one deployment it ran 8 hours and 870 consecutive failures before anyone suspected the token. The single-shot path (-interval 0) already treats "submitted nothing, recorded failures" as fatal; the long-running loop has no equivalent alarm, which is exactly why it was the loop that stayed silent. checkCredential makes one authenticated request at startup, beside the confinement self-check and for the same reason: a fault invisible at runtime has to be caught here or not at all. It separates three outcomes, and the separation is the point: 200 accepted, log and continue 401/403 rejected -- exit non-zero, naming the remedy 404/5xx/error inconclusive -- WARN and continue The third case is deliberate. A 404 from a server predating the endpoint, or an unreachable host, says nothing about the credential, and stopping on it would convert "we could not ask" into a new outage. This mirrors ingest, which keeps ErrUnauthorized distinct from ErrDueUnsupported so a bad secret cannot hide behind "old server" -- the same principle, applied to the other credential. Also documents -interval honestly: it is a sleep AFTER a pass, so the cycle is pass-duration + interval and throughput is due-limit/(pass-duration+interval). Read as "a pass every interval" it overstates throughput by the pass duration -- at 500 providers per ~30m pass with -interval 1h that is ~390/hour, two thirds of it idle. Tests cover all four outcomes, including that a rejection does NOT also satisfy errCredentialUnverified -- if it did, main's switch would downgrade it to a warning and the prober would start on a dead credential, reintroducing the bug. Verified by simulation: making 401 return errCredentialUnverified fails TestCheckCredentialRejects401. Full suite green under the CI command (go test -race -count=1 -timeout 20m ./...).
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 happened
A beta deployment produced zero successful probes for ~8 hours — 870 consecutive failures, every one classified
no_consensuswithok=0/131. Every provider looked dead.No provider was dead. The prober's
UR_PROBER_BY_JWThad been minted before the api began enforcing the audience claim, so it carried noaudand came back 401. It had not expired — it parsed perfectly, andparseByJwtClientIdwas happy with it.Why it was invisible
The prober holds two credentials that fail asymmetrically:
UR_OPERATOR_SECRETErrUnauthorizedUR_PROBER_BY_JWTSo when the jwt died, every operator-secret path kept working. The prober fetched its batch, opened tunnels that carried no traffic, and reported each provider as
no_consensus— indistinguishable from a genuinely unreachable provider. It looked healthy the whole time. The output didn't say "credential", it said "the entire fleet is bad," and that was convincing enough to send the investigation at the fleet for most of a day.One more detail that explains the silence: the single-shot path (
-interval 0) already treats "submitted nothing and recorded failures" as fatal and exits non-zero. The long-running loop has no equivalent alarm — which is precisely why it was the loop that ran silent.This is not a one-off.
jwt.expiryDurationis 24h, so every operator's prober credential dies daily and any deployment can land here.The change
checkCredentialmakes one authenticated request at startup, placed beside the confinement self-check and justified the same way: a fault the prober cannot detect at runtime has to be caught at startup or not at all.It separates three outcomes, and that separation is the whole point:
The third case is deliberate. A 404 from a server predating the endpoint, or an unreachable host, says nothing about the credential. Stopping there would turn "we could not ask" into a new outage of its own — the mistake
confinement.ErrNoEvidencealready exists to avoid.That posture is not invented here.
ingestdeliberately keepsErrUnauthorizedseparate fromErrDueUnsupportedso a bad operator secret cannot hide behind "old server" — its comment calls it "the quiet degradationErrUnauthorizedexists to prevent." This applies the same principle to the other credential, which never got it.Also:
-intervaldocumentation-intervalis a sleep after a pass, not a fixed period, so the cycle ispass-duration + intervaland throughput isdue-limit / (pass-duration + interval). Read as "a pass every interval" — which the old help text invited — it overstates throughput by the pass duration. Measured on beta: 500 providers per ~30m pass at-interval 1hgave ~390/hour with the prober idle two thirds of every cycle. Help text only; no behavior change. Worth knowing for the 100k-provider sizing discussion, where the gap compounds.Verification
errCredentialUnverified. If it did,main's switch would downgrade it to a warning and the prober would start on a dead credential — reintroducing the exact bug. Verified by simulating that regression (returningerrCredentialUnverifiedfor 401):TestCheckCredentialRejects401fails.go test -race -count=1 -timeout 20m ./...(the CI command): all 8 packages ok.gofmt -lclean.Note on the endpoint
The check calls
GET /network/clients, chosen because it requires auth.find-providers2— the only endpoint the prober already sends the byJwt to — isWrapWithInputNoAuthserver-side and would answer 200 for any token, so it cannot validate anything. If you'd prefer a different endpoint, or one the prober already depends on, say so and I'll switch it; the 404 path means an older server degrades to a warning either way.A natural follow-up, deliberately not in this PR: warn when the token is within a few hours of
exp. That catches the ordinary 24h lapse before it bites, where this catches it at the next restart. Happy to add it if wanted.