Repository navigation
Conversation
GetCredentialConfig ran config.New on every Connect (i.e. every reconcile) for every CF resource, and config.New performs an eager password-grant login against the CF UAA. A single reconcile could trigger several logins (reference resolvers build a client again), and concurrent per-identity logins are the documented lockout trigger for the shared technical user. Cache one authenticated go-cfclient config per credential (keyed by url+email+password) and reuse it across all reconciles and all CF resource types. The login is held under a mutex so a concurrent stampede collapses to a single login; failed logins are not cached. The oauth2 token source refreshes internally on expiry, so no TTL is needed. Refs #1036.
SatabdiG
reviewed
Sep 28, 2026
SatabdiG
left a comment
SatabdiG
left a comment
Contributor
There was a problem hiding this comment.
🔥 Few comments from my side
Caching the whole *config.Config meant every controller shared one transport. On a 401 that transport re-ran the password grant and reassigned oauth2.Transport.Source without synchronization: a data race plus a burst of concurrent password logins (the lockout this PR fixes). Cache a per-credential oauth2.TokenSource instead. Each reconcile builds its own short-lived config from the shared token, so each has its own transport and expiry refreshes via the refresh_token grant, never the password grant. singleflight coalesces the one-time bootstrap login per credential, so no mutex is held across the network. A dead refresh token re-bootstraps once and drops the entry instead of wedging it. Tests now emit a valid JWT and count logins by grant type, with new -race cases for expired-token refresh and dead-refresh re-bootstrap.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
SatabdiG
reviewed
Sep 29, 2026
SatabdiG
left a comment
SatabdiG
left a comment
Contributor
There was a problem hiding this comment.
Nice redesign, caching the ReuseTokenSource instead of the config fixes the race (clean under -race) and the cross-credential blocking. 🔥
Address golangci-lint findings on the login-reuse rework: - noctx: use http.NewRequestWithContext for endpoint discovery - contextcheck: thread ctx through cachedCFConfig and derive the long-lived token-source context with context.WithoutCancel (survives reconcile cancellation while keeping request values) - errchkjson: build the test JWT without json.Marshal TLS verification stays skipped (//nolint:gosec); making it configurable is tracked in #348.
When the shared token source dies, every queued reconcile fails Token() at once. The unconditional Delete + singleflight Forget let each failing caller wipe the entry an earlier caller had just re-bootstrapped and start another password login, defeating the coalescing exactly when a burst of logins is most likely to trigger UAA lockout (thanks @SatabdiG). Use CompareAndDelete so a caller only evicts the specific bad entry it observed (never a freshly recovered one), and drop Forget so concurrent re-bootstraps collapse into a single password login. Add a race test asserting a 50-goroutine dead-token burst recovers with exactly one re-login.
This branch has not been deployed
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
3 participants
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.
Summary
GetCredentialConfigruns on everyConnect(every reconcile) for every CF resource, andconfig.New(UserPassword(...))performs an eager password-grant login against CF UAA. Reference resolvers build a client again within the same reconcile, so a single reconcile can trigger several logins. Concurrent per-identity password logins are the documented lockout trigger for the shared technical user.This PR caches the login — a goroutine-safe, refreshing
oauth2.TokenSource— not theconfig.Config. Each reconcile builds its own short-livedconfig.Configfrom the shared token source.Why not cache the
config.ConfigAn earlier version of this PR cached one
*config.Configper credential under a mutex. That was flawed (thanks @SatabdiG for the review):config.Configcarries a mutable, shared transport. On a 401, go-cfclient'sretryableAuthTransportre-runs the password grant and reassignsoauth2.Transport.Sourcewithout synchronization — sharing one config across controllers is a data race and can stampede password logins (the very lockout we're avoiding).config.New(network, up to 30s) blocksConnectfor all ProviderConfigs on one slow endpoint.How it works now
url+email+passwordin async.Map(lock-free reads on the hot path), with asingleflight.Groupso a cold-cache stampede collapses to a single bootstrap login per credential.login/uaaendpoints, then obtain a refreshing token source via one password grant. The token source is built on a cancellation-independent context (context.WithoutCancel) so it survives the triggering reconcile and can refresh later.refresh_tokengrant on expiry — never the password grant), then build a freshconfig.New(url, config.Token(...), config.AuthTokenURL(login, uaa), config.SkipTLSValidation()).config.Tokenuses the refresh grant (no eager login) andconfig.AuthTokenURLpre-seeds discovery, so the per-reconcile build does no network I/O..Sourceto race on.Passcodeis intentionally excluded from the cache key — it's a one-time code that must not be cached.Notes
config.SkipTLSValidation()), suppressed with//nolint:gosec. Making it configurable with a secure default is tracked in Make CF TLS certificate verification configurable (default: verify) #348.Test plan
go build ./...go test -race -count=1 ./internal/clients/— fake UAA counts token POSTs by grant type:refresh_tokengrant, race-free under-race(no password burst)