Retry OIDC discovery with backoff instead of crashing on first failure - #752
CodeBuildder wants to merge 1 commit into
Conversation
NewKeycloakAuthenticator made exactly one attempt to reach Keycloak for OIDC discovery at startup. If Keycloak wasn't reachable yet, that error propagated straight up to log.Fatal and killed the process immediately. That's a real problem in docker-compose and Kubernetes, where container start order isn't guaranteed and Keycloak commonly isn't ready the instant Tornjak boots. Reproduced this against a real Keycloak container: starting Tornjak before Keycloak was up crashed it right away, confirming the mechanism described in the issue. I also checked whether Tornjak crashes once it's already running and Keycloak later goes down, since the issue title implies that too, and it doesn't: the request path only touches an in-memory JWKS cache, and the background JWKS refresh already degrades gracefully by design (logs and keeps running). So the actual gap is only at startup. Wrap the discovery call in exponential backoff, capped at 30 seconds, so a Keycloak that's still starting gets ridden out instead of killing the process on the first try. A permanently unreachable issuer still fails loud after the window, same as before, just not instantly. Fixes spiffe#410 Signed-off-by: Kaushik Kumaran <47471121+CodeBuildder@users.noreply.github.com>
|
@maia-iyer Another one from me this week - this retries OIDC discovery with backoff instead of crashing outright when the IAM provider briefly drops (#410). Take your time. |
mrsabath
left a comment
There was a problem hiding this comment.
Good fix, well scoped. Uses the existing cenkalti/backoff/v4 rather than hand-rolling, bounds startup via MaxElapsedTime = 30s, and still fails loudly — the error propagates up to log.Fatal at the caller when Keycloak never comes up. Tests cover immediate success, transient recovery, and a time-bounded give-up.
Two nits beyond the inline comment: the 30s bound is a fixed const with no config/env override (fine for now, but slower IAM startups cannot tune it), and the transient-recovery test re-binds a freed port with a small TOCTOU window that could flake in CI.
Assessment only — not an approval.
| var oidcClient *discovery.Client | ||
| err := backoff.Retry(func() error { | ||
| var discoverErr error | ||
| oidcClient, discoverErr = discovery.NewClient(context.Background(), issuerURL) |
There was a problem hiding this comment.
[suggestion] Two things on this retry:
- Silent retries — operators will see an up-to-30s stall with no log output before success or the fatal. Consider
backoff.RetryNotifywith a notify func logging each attempt at info/warn. - No per-attempt deadline —
context.Background()is uncancellable andbackoff.Retrywill not interrupt an in-flight call, so a discovery request that hangs (TCP connects but the server never responds) can block pastMaxElapsedTime. Considerbackoff.RetryContextwith a per-attemptcontext.WithTimeout. The give-up test only covers fast connection-refused, not the hang case.
Fixes #410
NewKeycloakAuthenticator made exactly one attempt to reach Keycloak for OIDC discovery at startup. If Keycloak wasn't reachable yet, that error went straight to log.Fatal and killed the process. This is a real problem in docker-compose and Kubernetes, where container start order isn't guaranteed, so Keycloak is very often not ready the instant Tornjak boots.
I reproduced this against a real Keycloak container rather than just reading the code: starting Tornjak before Keycloak was up crashed it immediately, matching the log the reporter posted in the issue thread.
I also checked the other half of what the issue title implies, whether Tornjak crashes once it's already running and Keycloak later goes down. It doesn't, and I confirmed that two ways. First, the request path (verificationMiddleware -> AuthenticateRequest) never calls out to Keycloak per request, it only reads an in-memory JWKS cache built once at startup. I killed Keycloak while Tornjak was actively serving requests and it kept responding fine the whole time. Second, the only other thing that talks to Keycloak after startup is the background JWKS refresh goroutine, so I built the same keyfunc client Tornjak's code builds, pointed it at a real Keycloak, killed Keycloak, and forced a refresh. The failure just goes to RefreshErrorHandler, which logs it, and the process keeps running, matching what the keyfunc library itself documents. So the actual gap really is only at startup, this PR is scoped to that.
The fix wraps the discovery call in exponential backoff (github.com/cenkalti/backoff/v4, same library already used for the DataStore plugin's setup in api/agent/config.go, so nothing new gets pulled in), capped at 30 seconds. A Keycloak that's still starting gets ridden out instead of killing the process on the first try. A genuinely unreachable issuer still fails loud after the window, same as before, just not instantly, which matches what a maintainer described wanting in the issue thread back in 2024 ("try a couple times and ultimately crash if it can't, to alert administrators").
I did consider a more thorough fix: have Tornjak start serving immediately with a not-ready authenticator and initialize the real one in a background goroutine with retry, so the server's own liveness isn't coupled to Keycloak's at all, closer to how a Kubernetes readiness probe should behave. I think that's a genuinely better long-term design, but it's a bigger change to startup sequencing than what was asked for here and I'd rather get a small, targeted fix in front of you first. Happy to explore that separately if there's interest.
Verification: go build, go vet (clean on the changed package), gofmt clean, go test ./api/... ./pkg/... all passing, plus five runs of the new tests back to back to check for flakiness around the port-timing in the transient-recovery test.
Added pkg/agent/authentication/authenticator/keycloak_test.go:
To keep the tests fast and deterministic without waiting out the real 30 second window, I split NewKeycloakAuthenticator into a thin public wrapper and an internal newKeycloakAuthenticator that takes an explicit backoff.BackOff, so tests can pass a much shorter one. Production behavior is unchanged, this is just a test seam.