{{ message }}
feat: log and meter rate-limited external auth token validation#26754
Draft
jscottmiller wants to merge 3 commits into
Draft
feat: log and meter rate-limited external auth token validation#26754jscottmiller wants to merge 3 commits into
jscottmiller wants to merge 3 commits into
Conversation
When ValidateToken keeps a token because the validation endpoint
returned a rate-limited response (403 with rate-limit headers or 429),
emit a Warn log and increment a new counter
coderd_oauth2_external_requests_token_validation_total{name,result} so
operators can distinguish a provider-confirmed token from one kept
optimistically during a rate limit. The valid/invalid decision is
unchanged.
Replace the RecordTokenValidation method on InstrumentedOAuth2Config with a Factory.TokenValidationRecorder that returns a per-provider recorder function. externalauth.Config holds the recorder as a private field set by ConvertConfig, so the metric stays in promoauth without bolting an externalauth-specific concern onto the generic oauth2 abstraction. The test fake no longer needs to implement an unrelated method.
Remove the dedicated token validation counter and all of its wiring
(the promoauth counter, the Factory recorder, and the Config field).
The existing coderd_oauth2_external_requests_total{source="ValidateToken",
status_code} metric already distinguishes confirmed (200), rate-limited
(429), and invalid (401) outcomes, and the new Warn log captures the
ambiguous rate-limited 403 case. This keeps externalauth concerns out of
the generic oauth2 instrumentation.
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
1 participant
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
When
ValidateTokenkeeps a token because the validation endpoint returned a rate-limited response (a403with rate-limit headers or a429), it previously returnedvalid=truesilently. Operators had no way to tell a provider-confirmed token apart from one kept optimistically during a rate limit.This change is observability-only. The valid/invalid decision is unchanged.
Approach
The existing
coderd_oauth2_external_requests_total{name, source="ValidateToken", status_code}metric already distinguishes the unambiguous outcomes:200(confirmed),429(rate-limited), and401(invalid). The only gap is that a403is ambiguous (genuine revocation vs. rate limit). This PR closes that gap with a structuredWarnlog on the optimistic paths, rather than adding a new metric, keeping externalauth concerns out of the generic OAuth2 instrumentation.Changes
coderd/externalauth: Add aLoggertoConfig(zero value is a no-op). On both rate-limit branches (403with rate-limit headers,429), emit aWarnwithprovider_id,provider_type,status_code, andreason.ConvertConfignow takes a logger.cli/server.go: Pass the existing logger intoConvertConfig.Tests
TestValidateTokenasserts the structuredWarnlog fields across the rate-limited (403+ headers,403+Retry-After,429) cases, and asserts no warning is logged for genuine revocations,401, and confirmed (200) responses.Verified: affected packages build,
go test ./coderd/externalauth/...passes,gofmtclean,golangci-lintclean.Design notes
Earlier revisions added a dedicated
coderd_oauth2_external_requests_token_validation_total{name, result}counter. Reaching it fromValidateTokenrequired either bolting aRecordTokenValidationmethod onto the genericInstrumentedOAuth2Configinterface or threading a recorder callback throughConvertConfig, both of which leaked an externalauth-specific concern into the OAuth2 abstraction. Since the existing request metric plus the new log already cover the operator need, the dedicated counter was dropped.🤖 Generated with the help of Coder Agents on behalf of @jscottmiller.