From 6e233c879f145088da9d225bbc7b2c91f6b63b2e Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Mon, 4 Dec 2023 21:35:20 +0000 Subject: [PATCH] fix(ratelimit): Round up when setting Retry-After header Previously this would truncate to the nearest integer, which could result in clients retrying before the quota has reset, and getting rate limited again. This also includes an update from go-rate that will round up when reporting the reset time for the RateLimit header. Note that in the case of a 503, the Retry-After header was already being set correctly. See: https://github.com/hashicorp/go-rate/pull/8 --- go.mod | 2 +- go.sum | 4 +-- .../controller_ratelimit_reload_test.go | 28 +++++++++---------- internal/ratelimit/handler.go | 2 +- internal/ratelimit/handler_test.go | 16 +++++------ 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/go.mod b/go.mod index 2306fad941..0dc95d8f47 100644 --- a/go.mod +++ b/go.mod @@ -94,7 +94,7 @@ require ( github.com/hashicorp/cap/ldap v0.0.0-20231012003312-273118a6e3b8 github.com/hashicorp/dbassert v0.0.0-20231012105025-1bc1bd88e22b github.com/hashicorp/go-kms-wrapping/extras/kms/v2 v2.0.0-20231124110655-4315424d22d0 - github.com/hashicorp/go-rate v0.0.0-20231130213406-dd41f530f095 + github.com/hashicorp/go-rate v0.0.0-20231204194614-cc8d401f70ab github.com/hashicorp/go-version v1.6.0 github.com/hashicorp/nodeenrollment v0.2.9 github.com/jackc/pgx/v5 v5.4.3 diff --git a/go.sum b/go.sum index a7fcad51d7..44c2385bb3 100644 --- a/go.sum +++ b/go.sum @@ -260,8 +260,8 @@ github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9 github.com/hashicorp/go-plugin v1.4.3/go.mod h1:5fGEH17QVwTTcR0zV7yhDPLLmFX9YSZ38b18Udy6vYQ= github.com/hashicorp/go-plugin v1.5.2 h1:aWv8eimFqWlsEiMrYZdPYl+FdHaBJSN4AWwGWfT1G2Y= github.com/hashicorp/go-plugin v1.5.2/go.mod h1:w1sAEES3g3PuV/RzUrgow20W2uErMly84hhD3um1WL4= -github.com/hashicorp/go-rate v0.0.0-20231130213406-dd41f530f095 h1:l/scdJ8Uy823fhxFEdwlDR5NkDRdoOOaH6g36Uqz1AM= -github.com/hashicorp/go-rate v0.0.0-20231130213406-dd41f530f095/go.mod h1:uTQC66EFZuSKunHYx0hHIfdH3nhRrv7UOOE8Ug0yI0A= +github.com/hashicorp/go-rate v0.0.0-20231204194614-cc8d401f70ab h1:XIeGyyCCS80b5MKfU4E6ioLocIYtT4FmfdNH0uNYQi8= +github.com/hashicorp/go-rate v0.0.0-20231204194614-cc8d401f70ab/go.mod h1:uTQC66EFZuSKunHYx0hHIfdH3nhRrv7UOOE8Ug0yI0A= github.com/hashicorp/go-retryablehttp v0.5.3/go.mod h1:9B5zBasrRhHXnJnui7y6sL7es7NDiJgTc6Er0maI1Xs= github.com/hashicorp/go-retryablehttp v0.7.4 h1:ZQgVdpTdAL7WpMIwLzCfbalOcSUdkDZnpUv3/+BxzFA= github.com/hashicorp/go-retryablehttp v0.7.4/go.mod h1:Jy/gPYAdjqffZ/yFGCFV2doI5wjtH1ewM9u8iYVjtX8= diff --git a/internal/cmd/commands/server/controller_ratelimit_reload_test.go b/internal/cmd/commands/server/controller_ratelimit_reload_test.go index 24fd44bdc3..154b17f0db 100644 --- a/internal/cmd/commands/server/controller_ratelimit_reload_test.go +++ b/internal/cmd/commands/server/controller_ratelimit_reload_test.go @@ -233,7 +233,7 @@ func TestRealodControllerRateLimits(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=1, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=1, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -244,7 +244,7 @@ func TestRealodControllerRateLimits(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -255,7 +255,7 @@ func TestRealodControllerRateLimits(t *testing.T) { require.NoError(t, err) // out of quota, so we expect a 429 assert.Equal(t, http.StatusTooManyRequests, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.SighupCh <- struct{}{} @@ -275,7 +275,7 @@ func TestRealodControllerRateLimits(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=5, remaining=4, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=5, remaining=4, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `5;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.ShutdownCh <- struct{}{} @@ -328,7 +328,7 @@ func TestRealodControllerRateLimitsSameConfig(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=1, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=1, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -339,7 +339,7 @@ func TestRealodControllerRateLimitsSameConfig(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -350,7 +350,7 @@ func TestRealodControllerRateLimitsSameConfig(t *testing.T) { require.NoError(t, err) // out of quota, so we expect a 429 assert.Equal(t, http.StatusTooManyRequests, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.SighupCh <- struct{}{} @@ -370,7 +370,7 @@ func TestRealodControllerRateLimitsSameConfig(t *testing.T) { require.NoError(t, err) // should still be rate limited, so 429 assert.Equal(t, http.StatusTooManyRequests, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.ShutdownCh <- struct{}{} @@ -426,7 +426,7 @@ func TestRealodControllerRateLimitsDisable(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=1, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=1, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -437,7 +437,7 @@ func TestRealodControllerRateLimitsDisable(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -448,7 +448,7 @@ func TestRealodControllerRateLimitsDisable(t *testing.T) { require.NoError(t, err) // out of quota, so we expect a 429 assert.Equal(t, http.StatusTooManyRequests, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.SighupCh <- struct{}{} @@ -545,7 +545,7 @@ func TestRealodControllerRateLimitsEnable(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=1, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=1, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -556,7 +556,7 @@ func TestRealodControllerRateLimitsEnable(t *testing.T) { require.NoError(t, err) // unauthed request, so we expect a 400 assert.Equal(t, http.StatusBadRequest, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) r, err = c.Do(func() *http.Request { @@ -567,7 +567,7 @@ func TestRealodControllerRateLimitsEnable(t *testing.T) { require.NoError(t, err) // out of quota, so we expect a 429 assert.Equal(t, http.StatusTooManyRequests, r.StatusCode) - assert.Equal(t, `limit=2, remaining=0, reset=59`, r.Header.Get("Ratelimit")) + assert.Equal(t, `limit=2, remaining=0, reset=60`, r.Header.Get("Ratelimit")) assert.Equal(t, `2;w=60;comment="total", 60;w=30;comment="ip-address", 60;w=30;comment="auth-token"`, r.Header.Get("Ratelimit-Policy")) cmd.ShutdownCh <- struct{}{} diff --git a/internal/ratelimit/handler.go b/internal/ratelimit/handler.go index 626daa2217..2244be7b29 100644 --- a/internal/ratelimit/handler.go +++ b/internal/ratelimit/handler.go @@ -172,7 +172,7 @@ func Handler(ctx context.Context, f LimiterFunc, next http.Handler) http.Handler } if !allowed { - rw.Header().Add("Retry-After", fmt.Sprintf("%.0f", quota.ResetsIn().Seconds())) + rw.Header().Add("Retry-After", fmt.Sprintf("%.0f", math.Ceil(quota.ResetsIn().Seconds()))) rw.WriteHeader(http.StatusTooManyRequests) return } diff --git a/internal/ratelimit/handler_test.go b/internal/ratelimit/handler_test.go index a4019b7ffb..34d9c1b718 100644 --- a/internal/ratelimit/handler_test.go +++ b/internal/ratelimit/handler_test.go @@ -72,7 +72,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -114,7 +114,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -156,7 +156,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -198,7 +198,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -240,7 +240,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -282,7 +282,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -324,7 +324,7 @@ func TestHandler(t *testing.T) { http.StatusOK, http.Header{ "RateLimit-Policy": []string{`10;w=60;comment="total", 10;w=60;comment="ip-address", 10;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=10, remaining=9, reset=59`}, + "RateLimit": []string{`limit=10, remaining=9, reset=60`}, }, }, { @@ -375,7 +375,7 @@ func TestHandler(t *testing.T) { http.Header{ "Retry-After": []string{"60"}, "RateLimit-Policy": []string{`2;w=60;comment="total", 2;w=60;comment="ip-address", 2;w=60;comment="auth-token"`}, - "RateLimit": []string{`limit=2, remaining=0, reset=59`}, + "RateLimit": []string{`limit=2, remaining=0, reset=60`}, }, }, {