Skip to content

Retry OIDC discovery with backoff instead of crashing on first failure - #752

Open
CodeBuildder wants to merge 1 commit into
spiffe:mainfrom
CodeBuildder:fix-410-oidc-retry
Open

Retry OIDC discovery with backoff instead of crashing on first failure#752
CodeBuildder wants to merge 1 commit into
spiffe:mainfrom
CodeBuildder:fix-410-oidc-retry

Conversation

@CodeBuildder

Copy link
Copy Markdown

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:

  • TestNewKeycloakAuthenticator_SucceedsImmediately, the control case, a reachable issuer still succeeds on the first try exactly like before
  • TestNewKeycloakAuthenticator_RecoversFromTransientOIDCFailure, starts against a refused port, then binds a real HTTP server to that same port mid-retry and confirms it recovers
  • TestNewKeycloakAuthenticator_GivesUpAfterMaxElapsedTime, confirms a permanently unreachable issuer still errors out within roughly the configured window instead of hanging

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.

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>
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.

Backend crashes upon disconnect with IAM server

1 participant