fix(sdk/go): honor explicitly-set zero option values in oidc defaults - #3235
fix(sdk/go): honor explicitly-set zero option values in oidc defaults#3235rhuss wants to merge 1 commit into
Conversation
`loginConfig.applyDefaults()` keyed off the zero value rather than set-ness, so it could not distinguish an unset field from one a caller explicitly set to its zero value: - `WithTimeout(0)` was silently replaced by the 2m default. - `WithScopes()` (explicit empty) was replaced by the default scopes, even though `WithScopes` already records `scopesSet`. Switch `applyDefaults` to consult the `*Set` sentinels, add a `timeoutSet` sentinel set by `WithTimeout`, and guard the client-credentials exchange so a zero timeout means "no deadline" instead of creating an already-expired context (matching Login and DeviceFlow). `WithTimeout(0)` now means "no timeout". Unset fields still receive their defaults; non-zero explicit values are unaffected. Signed-off-by: Roland Huß <rhuss@redhat.com>
| // A zero timeout means "no deadline"; only bound the exchange when a | ||
| // positive timeout was configured (mirrors Login and DeviceFlow). | ||
| exchangeCtx := context.Background() | ||
| cancel := context.CancelFunc(func() {}) | ||
| if a.cfg.timeout > 0 { | ||
| exchangeCtx, cancel = context.WithTimeout(exchangeCtx, a.cfg.timeout) | ||
| } |
There was a problem hiding this comment.
Does context.WithTimeout() interpret a 0 value as no deadline, or does it explicitly need this handling?
There was a problem hiding this comment.
context.WithTimeout(parent, 0) doesn't mean "no deadline". It's defined as WithDeadline(parent, time.Now().Add(timeout)), so a 0 (or negative) timeout sets the deadline to now and the context is born already-expired; any call using it returns context.DeadlineExceeded immediately. So the explicit timeout > 0 guard is needed to actually get "no deadline" behavior, and it mirrors what Login and DeviceFlow already do. Without it, WithTimeout(0) would break the client-credentials exchange instead of disabling the timeout (covered by TestClientCredentialsAuthZeroTimeoutHasNoDeadline).
2 participants
Summary
oidc.loginConfig.applyDefaults()decided whether to apply a default by inspecting the field's value (len(c.scopes) == 0,c.timeout == 0) rather than whether the caller had set it. As a result, a caller who explicitly passed a zero value had it silently replaced:WithTimeout(0)became the 2-minute default.WithScopes()(explicit empty) became the default scopes, even thoughWithScopesalready recordsscopesSet.This makes
applyDefaultsconsult the*Setsentinels instead, so defaults fill only genuinely-unset fields. Unset behavior and non-zero explicit values are unchanged.Related Issue
Follow-up to a review comment on #3232 (raised by @elezar): #3232 (comment). This is a small, localized pre-existing bug fix, so no separate issue is filed.
Changes
oidc/options.go: add atimeoutSetsentinel (set byWithTimeout);applyDefaultsnow checks!scopesSet/!timeoutSetinstead of the zero value.oidc/credentials_auth.go: guard the client-credentials token exchange so a zero timeout means "no deadline" rather than an already-expired context (matching the existing guards in Login and DeviceFlow).WithTimeout(0)succeeds instead of failing on a born-expired context.Behavior change:
WithTimeout(0)now means "no timeout" instead of the 2-minute default.Testing
mise run go:cigreen (build,golangci-lint,gofmt, fullgo test, proto-check, docs-check).Checklist
WithTimeout(0)) documented above