Retry OIDC discovery with backoff instead of crashing on first failure - #752
Open
CodeBuildder wants to merge 1 commit into
Open
Retry OIDC discovery with backoff instead of crashing on first failure#752CodeBuildder wants to merge 1 commit into
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>
CodeBuildder
requested review from
lumjjb,
maia-iyer,
mamy-CS and
mrsabath
as code owners
August 2, 2026 05:12
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.
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.