Skip to content

fix(egress-prober): fail loudly when the server rejects the byJwt - #4

Open
Ryanmello07 wants to merge 1 commit into
mainfrom
fix/prober-credential-self-check
Open

fix(egress-prober): fail loudly when the server rejects the byJwt#4
Ryanmello07 wants to merge 1 commit into
mainfrom
fix/prober-credential-self-check

Conversation

@Ryanmello07

Copy link
Copy Markdown
Collaborator

What happened

A beta deployment produced zero successful probes for ~8 hours — 870 consecutive failures, every one classified no_consensus with ok=0/131. Every provider looked dead.

No provider was dead. The prober's UR_PROBER_BY_JWT had been minted before the api began enforcing the audience claim, so it carried no aud and came back 401. It had not expired — it parsed perfectly, and parseByJwtClientId was happy with it.

Why it was invisible

The prober holds two credentials that fail asymmetrically:

credential used by on failure
UR_OPERATOR_SECRET due queue, attempt reporting, pin fetch already surfaces as ErrUnauthorized
UR_PROBER_BY_JWT the provider tunnel only nothing

So 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.expiryDuration is 24h, so every operator's prober credential dies daily and any deployment can land here.

The change

checkCredential makes 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:

200            accepted     -> log, continue
401/403        rejected     -> exit non-zero, naming the remedy
404/5xx/error  inconclusive -> WARN, continue

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.ErrNoEvidence already exists to avoid.

That posture is not invented here. ingest deliberately keeps ErrUnauthorized separate from ErrDueUnsupported so a bad operator secret cannot hide behind "old server" — its comment calls it "the quiet degradation ErrUnauthorized exists to prevent." This applies the same principle to the other credential, which never got it.

Also: -interval documentation

-interval is a sleep after a pass, not a fixed period, so the cycle is pass-duration + interval and throughput is due-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 1h gave ~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

  • 4 new tests covering accepted / rejected / 404 / transport error.
  • One asserts 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 exact bug. Verified by simulating that regression (returning errCredentialUnverified for 401): TestCheckCredentialRejects401 fails.
  • go test -race -count=1 -timeout 20m ./... (the CI command): all 8 packages ok.
  • gofmt -l clean.

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 — is WrapWithInputNoAuth server-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.

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 ./...).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant