diff --git a/coderd/promoauth/github.go b/coderd/promoauth/github.go index 3f2a97d241..17449ef70f 100644 --- a/coderd/promoauth/github.go +++ b/coderd/promoauth/github.go @@ -16,12 +16,24 @@ type rateLimits struct { Resource string } -// githubRateLimits checks the returned response headers and +// githubRateLimits returns rate limit information from a GitHub response. +// GitHub rate limits are on a per-user basis, and tracking each user as +// a prometheus label might be too much. So only track rate limits for +// unauthorized responses. +// +// Unauthorized responses have a much stricter rate limit of 60 per hour. +// Tracking this is vital to ensure we do not hit the limit. func githubRateLimits(resp *http.Response, err error) (rateLimits, bool) { if err != nil || resp == nil { return rateLimits{}, false } + // Only track 401 responses which indicates we are using the 60 per hour + // rate limit. + if resp.StatusCode != http.StatusUnauthorized { + return rateLimits{}, false + } + p := headerParser{header: resp.Header} // See // https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#checking-the-status-of-your-rate-limit @@ -29,7 +41,7 @@ func githubRateLimits(resp *http.Response, err error) (rateLimits, bool) { Limit: p.int("x-ratelimit-limit"), Remaining: p.int("x-ratelimit-remaining"), Used: p.int("x-ratelimit-used"), - Resource: p.string("x-ratelimit-resource"), + Resource: p.string("x-ratelimit-resource") + "-unauthorized", } if limits.Limit == 0 && @@ -50,18 +62,6 @@ func githubRateLimits(resp *http.Response, err error) (rateLimits, bool) { } limits.Reset = resetAt - // Unauthorized requests have their own rate limit, so we should - // track them separately. - if resp.StatusCode == http.StatusUnauthorized { - limits.Resource += "-unauthorized" - } - - // A 401 or 429 means too many requests. This might mess up the - // "resource" string because we could hit the unauthorized limit, - // and we do not want that to override the authorized one. - // However, in testing, it seems a 401 is always a 401, even if - // the limit is hit. - if len(p.errors) > 0 { // If we are missing any headers, then do not try and guess // what the rate limits are. diff --git a/coderd/promoauth/oauth2_test.go b/coderd/promoauth/oauth2_test.go index 845e2dbcb5..e54608385c 100644 --- a/coderd/promoauth/oauth2_test.go +++ b/coderd/promoauth/oauth2_test.go @@ -179,6 +179,15 @@ func TestGithubRateLimits(t *testing.T) { } } + // Ignore the well known, required for setup + if !strings.Contains(r.URL.String(), ".well-known") { + // GitHub rate limits only are instrumented for unauthenticated calls. So emulate + // that here. We cannot actually use invalid credentials because the fake IDP + // will throw a test error, as it only expects things to work. And we want to use + // a real idp to emulate the right http calls to be instrumented. + rw.WriteHeader(http.StatusUnauthorized) + return + } next.ServeHTTP(rw, r) }) })) @@ -195,13 +204,13 @@ func TestGithubRateLimits(t *testing.T) { // Do a single oauth2 call ctx := testutil.Context(t, testutil.WaitShort) ctx = context.WithValue(ctx, oauth2.HTTPClient, idp.HTTPClient(nil)) - _, err := cfg.Exchange(ctx, idp.CreateAuthCode(t, "foo")) - require.NoError(t, err) + _, err := cfg.Exchange(ctx, "invalid") + require.Error(t, err, "expected unauthorized exchange") // Verify labels := prometheus.Labels{ "name": "test", - "resource": "core", + "resource": "core-unauthorized", } pass := true if !c.ExpectNoMetrics { diff --git a/scripts/metricsdocgen/metrics b/scripts/metricsdocgen/metrics index 7b6dff2ad9..f51b0b1856 100644 --- a/scripts/metricsdocgen/metrics +++ b/scripts/metricsdocgen/metrics @@ -12,8 +12,8 @@ coderd_oauth2_external_requests_rate_limit_reset_in_seconds{name="primary-github coderd_oauth2_external_requests_rate_limit_reset_in_seconds{name="secondary-github",resource="core"} 121.82186601 # HELP coderd_oauth2_external_requests_rate_limit_total The total number of allowed requests per interval. # TYPE coderd_oauth2_external_requests_rate_limit_total gauge -coderd_oauth2_external_requests_rate_limit_total{name="primary-github",resource="core"} 5000 -coderd_oauth2_external_requests_rate_limit_total{name="secondary-github",resource="core"} 5000 +coderd_oauth2_external_requests_rate_limit_total{name="primary-github",resource="core-unauthorized"} 5000 +coderd_oauth2_external_requests_rate_limit_total{name="secondary-github",resource="core-unauthorized"} 5000 # HELP coderd_oauth2_external_requests_rate_limit_used The number of requests made in this interval. # TYPE coderd_oauth2_external_requests_rate_limit_used gauge coderd_oauth2_external_requests_rate_limit_used{name="primary-github",resource="core"} 148