feat: email, sms rate limiting new config and refactored rate limiters #1746

hf · 2024-08-28T13:05:46Z

Adds a new config syntax that allows more flexible rate limiting configuration for the GOTRUE_RATE_LIMIT_EMAIL_SENT, GOTRUE_RATE_LIMIT_SMS_SENT configs. It can be added in follow up PRs to the other configs.

It's fully backward compatible.

The syntax is:

123 or 123.0 expresses 123 events with an unspecified time, this is the legacy case and is on a per-case basis (see DivideAndSetDuration)
123/24h expresses 123 emails sent per day (token bucket resets every 24 hours)

Additionally the rate limiters for SMS and email are replaced with simple counters, massively simplifying the code paths and decreasing reliance on the tollbooth library which has memory leaks.

You can use any time.ParseDuration() value in the second part of the rate config.

coveralls · 2024-08-28T13:18:35Z

Pull Request Test Coverage Report for Build 10597475519

Details

40 of 49 (81.63%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 57.948%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/conf/rate.go	39	48	81.25%

Totals
Change from base Build 10588384860:	0.06%
Covered Lines:	9176
Relevant Lines:	15835

💛 - Coveralls

internal/api/middleware.go

hf · 2024-09-10T17:40:02Z

internal/conf/rate.go

+	return *r
+}
+
+func (r *Rate) DivideIfDefaultDuration(div float64) *Rate {


hf · 2024-09-10T17:40:12Z

internal/conf/rate.go

+	return r
+}
+
+func (r *Rate) CreateLimiter() *limiter.Limiter {


hf · 2024-09-10T17:45:01Z

internal/conf/rate.go

+		return fmt.Sprintf("%f", r.Events)
+	}
+
+	return fmt.Sprintf("%f/%s", r.Events, r.OverTime.String())


Suggested change

return fmt.Sprintf("%f/%s", r.Events, r.OverTime.String())

return fmt.Sprintf("%d/%s", r.Events, r.OverTime.String())

kangmingtay

lgtm - left some comments to improve readability

kangmingtay · 2024-09-10T17:49:29Z

internal/conf/rate.go

+	Events   float64       `json:"events,omitempty"`
+	OverTime time.Duration `json:"over_time,omitempty"`


for clarification: Events keeps track of the number of requests allowed per second while OverTime is when the bucket gets refilled right? if so, i would just prefer to call OverTime the ExpirationTTL

/nit As a follow on suggestion, perhaps we can consider renaming events to AllowedEvents or similar

I'd prefer to not use "TTL" or "Expire" because it's not about expiring, it's describing allowing number of "Events" occurring "OverTime".

How that's implemented in rate limiting can vary.

kangmingtay · 2024-09-10T17:50:22Z

internal/conf/rate.go

+func (r *Rate) CreateLimiter() *limiter.Limiter {
+	overTime := r.OverTime
+	if int64(overTime) == 0 {
+		// if r.OverTime is not specified, i.e. the configuration specified just a single float64 number, the


incomplete comment but i'm guessing you meant we default the overTime to 1 hour

kangmingtay · 2024-09-10T17:56:22Z

internal/api/ratelimits.go

+	return atomic.AddUint64(&r.counter, events) < uint64(r.Rate.Events)
+}
+
+func (r *SimpleRateLimiter) fillBucket(initialWait time.Duration) {


hmm this function is basically counting down to the bucket expiry and resetting the counter right? should we name this something else instead? naming it fillBucket confused me a little and i thought this was where we incremented the counter

kangmingtay · 2024-09-10T17:57:25Z

internal/conf/rate.go

+	})
+}
+
+func (r *Rate) Decode(value string) error {


can we add a comment to describe that decode is necessary for envconfig to unmarshal the value correctly?

kangmingtay · 2024-09-10T17:58:13Z

internal/conf/configuration.go

+	RateLimitEmailSent      Rate    `split_words:"true" default:"30"`
+	RateLimitSmsSent        Rate    `split_words:"true" default:"30"`


do we want to keep the default setting here to 30?

Yup, no breaking changes

J0 · 2024-09-10T18:57:03Z

internal/conf/rate.go

+		return fmt.Errorf("rate: value does not match rate syntax %q", value)
+	}
+
+	e, err := strconv.ParseUint(parts[0], 10, 52)


Would this be set to 52 to account for the mantissa? Consider adding a comment as it might not be immediately obvious

J0 · 2024-09-10T19:00:20Z

internal/api/ratelimits.go

+
+	go r.fillBucket(counterResetsAt.Sub(now))
+
+	return nil


should this return the rate limiter?

Suggested change

return nil

return r

J0 · 2024-09-10T19:09:08Z

Nice work, don't forget to remove withLimiter and getLimiter as the Github warnings suggest

cstockton · 2024-09-10T17:52:04Z

internal/api/ratelimits.go

+
+	atomic.StoreUint64(&r.counter, 0)
+
+	ticker := time.NewTicker(r.Rate.OverTime)


defer ticker.Stop()

cstockton · 2024-09-10T17:55:02Z

internal/api/ratelimits.go

+	return atomic.AddUint64(&r.counter, events) < uint64(r.Rate.Events)
+}
+
+func (r *SimpleRateLimiter) fillBucket(initialWait time.Duration) {


Since this runs in a goroutine, is it possible we accept a context here by refactoring the responsibility of token filling? Alternatively this computation can be done at the time of each call to Increment. A reference implementation (an potential candidate for this, which includes context) could be:

https://pkg.go.dev/golang.org/x/time/rate

cstockton · 2024-09-10T17:58:56Z

internal/api/ratelimits.go

+
+	r.counter = uint64(rate.Events * proRate)
+
+	go r.fillBucket(counterResetsAt.Sub(now))


We solve a few potential caveats by moving the responsibility of starting the goroutine into the caller, providing a context to select on along with ticker to prevent goroutine leaks over time. Alternatively as suggested in the other comment we could remove the real-time bucket filling into a task to be done during incrementing.

cstockton · 2024-09-10T18:04:40Z

internal/api/api.go

@@ -70,6 +73,11 @@ func (a *API) deprecationNotices() {
 func NewAPIWithVersion(globalConfig *conf.GlobalConfiguration, db *storage.Connection, version string) *API {
 	api := &API{config: globalConfig, db: db, version: version}

+	now := time.Now()
+
+	api.emailRateLimiter = NewSimpleRateLimiter(now, globalConfig.RateLimitEmailSent.DivideAndSetDuration(6*60, time.Hour))


These could be parameterized in NewAPIWithVersion so it doesn't need to be responsible for starting goroutines, alternatively we provide a context.Context to signal when shutdown has occurred. This would allow us to signal cancelation and synchronize exiting.

cstockton · 2024-09-11T17:58:33Z

internal/api/ratelimits.go

+	atomic.StoreUint64(&r.counter, 0)
+
+	// then keep resetting at regular OverTime intervals
+	for range r.ticker.C {


You will need another mechanism to signal this goroutine should exit, because r.ticker.C will not be closed when r.ticker.Stop is called, leaving this blocked forever.

One option is to add ctx, another is to have a close chan so you can select between the ticker and r.closeCh here which could be close(r.closeCh) in r.Stop(), though you will want to add a mutex at that point to defend against double closes to allow flexibility for callers.

cstockton · 2024-09-11T17:58:37Z

internal/api/ratelimits.go

+	}
+}
+
+func (r *SimpleRateLimiter) Close() {


Is the intention here to have callers of New() (serve_cmd, Reloader, ...) call Close? Workable solution but we may need to change the fillBucket some to ensure it exits, see other comment.

hf requested a review from a team as a code owner August 28, 2024 13:05

hf force-pushed the hf/email-rate-limiting-new-config branch 2 times, most recently from 4bc81f3 to f498771 Compare August 28, 2024 13:15

J0 reviewed Sep 2, 2024

View reviewed changes

internal/api/middleware.go Outdated Show resolved Hide resolved

fix: add new config option for email rate limiting

9ddd38c

hf force-pushed the hf/email-rate-limiting-new-config branch 2 times, most recently from 158f705 to 9d89e34 Compare September 10, 2024 17:33

hf changed the title ~~fix: add new config syntax for email rate limiting~~ feat: email, sms rate limiting new config and refactored rate limiters Sep 10, 2024

hf commented Sep 10, 2024

View reviewed changes

internal/conf/rate.go Outdated

return *r

}

func (r *Rate) DivideIfDefaultDuration(div float64) *Rate {

Copy link

Contributor Author

hf Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

hf commented Sep 10, 2024

View reviewed changes

internal/conf/rate.go Outdated

return r

}

func (r *Rate) CreateLimiter() *limiter.Limiter {

Copy link

Contributor Author

hf Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

hf commented Sep 10, 2024

View reviewed changes

kangmingtay approved these changes Sep 10, 2024

View reviewed changes

J0 reviewed Sep 10, 2024

View reviewed changes

J0 approved these changes Sep 10, 2024

View reviewed changes

feat: simplified, pro-rated email and sms rate limiter

f05a4b7

hf force-pushed the hf/email-rate-limiting-new-config branch from 9d89e34 to f05a4b7 Compare September 11, 2024 14:11

cstockton reviewed Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: email, sms rate limiting new config and refactored rate limiters #1746

feat: email, sms rate limiting new config and refactored rate limiters #1746

hf commented Aug 28, 2024 •

edited

Loading

coveralls commented Aug 28, 2024 •

edited

Loading

hf Sep 10, 2024

hf Sep 10, 2024

hf Sep 10, 2024

kangmingtay left a comment

kangmingtay Sep 10, 2024

J0 Sep 10, 2024 •

edited

Loading

hf Sep 11, 2024

kangmingtay Sep 10, 2024

kangmingtay Sep 10, 2024

kangmingtay Sep 10, 2024

kangmingtay Sep 10, 2024

hf Sep 11, 2024

J0 Sep 10, 2024

J0 Sep 10, 2024

J0 commented Sep 10, 2024

cstockton Sep 10, 2024

cstockton Sep 10, 2024

cstockton Sep 10, 2024

cstockton Sep 10, 2024

cstockton Sep 11, 2024

cstockton Sep 11, 2024

	return fmt.Sprintf("%f/%s", r.Events, r.OverTime.String())
	return fmt.Sprintf("%d/%s", r.Events, r.OverTime.String())

		Events float64 `json:"events,omitempty"`
		OverTime time.Duration `json:"over_time,omitempty"`

		RateLimitEmailSent Rate `split_words:"true" default:"30"`
		RateLimitSmsSent Rate `split_words:"true" default:"30"`


		atomic.StoreUint64(&r.counter, 0)

		ticker := time.NewTicker(r.Rate.OverTime)


		r.counter = uint64(rate.Events * proRate)

		go r.fillBucket(counterResetsAt.Sub(now))

feat: email, sms rate limiting new config and refactored rate limiters #1746

Are you sure you want to change the base?

feat: email, sms rate limiting new config and refactored rate limiters #1746

Conversation

hf commented Aug 28, 2024 • edited Loading

coveralls commented Aug 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10597475519

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangmingtay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J0 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J0 commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hf commented Aug 28, 2024 •

edited

Loading

coveralls commented Aug 28, 2024 •

edited

Loading

J0 Sep 10, 2024 •

edited

Loading