Add rate limiting #411

Choraden · 2023-09-11T15:02:16Z

This adds global read and write rate limit.

what-the-diff · 2023-09-11T15:03:02Z

PR Summary

Inclusion of Read and Write Limit Controls
An update has been done to our HTTPProxyConfig settings. This now includes ReadLimit and WriteLimit controls that dictate the amount of data read or written per second via the proxy. This is an overarching control that applies to all proxy activities.
New Dependency Introduction
There's been an update to our module information file, go.mod. This now includes reference to another module: golang.org/x/time v0.0.0-20191024005414-555d28b269f0.
Inclusion of a Rate Limit Package
A new package named ratelimit has been added. This includes several files which achieve the following:
- conn.go: Incorporates rate limiting for data read or written via the network connection.
- listener.go: Incorporates rate limiting on connections accepted.
- ratelimit.go: This enables creation of a new rate limiter, based on the bandwidth requirements.
  Bear in mind, these changes have also culminated in a range of other updates across different packages and files, though the most crucial updates are covered above.

mmatczuk · 2023-09-12T10:01:06Z

http_proxy.go

@@ -86,6 +87,8 @@ type HTTPProxyConfig struct {
 	CloseAfterReply        bool
 	DenyDomains            []*regexp.Regexp
 	DenyDomainsExclude     []*regexp.Regexp
+	ReadLimit              uint


I think it would be beneficial to use a special type that supports kb mb etc, see in rclone for inspiration.

mmatczuk · 2023-09-12T10:01:49Z

bind/flag.go

@@ -127,6 +127,12 @@ func HTTPProxyConfig(fs *pflag.FlagSet, cfg *forwarder.HTTPProxyConfig, lcfg *lo
 			"Use this flag multiple times to specify multiple deny domain excludes. "+
 			"This flag takes precedence over deny-domain. "+
 			"Can be specified only if --deny-domain is also specified. ")
+
+	fs.UintVar(&cfg.ReadLimit, "read-limit", cfg.ReadLimit, "<bandwidth>"+
+		"Global read rate limit in bytes per second.")


Same here more sophisticated input would be nice.

mmatczuk · 2023-09-12T10:02:35Z

Looks good, some commits could be squashed.

mmatczuk · 2023-09-14T08:21:40Z

resource/sizesuffix.go

@@ -0,0 +1,247 @@
+package resource


How about putting it directly in forwarder. This is needed for config and CLI.

mmatczuk · 2023-09-14T08:23:18Z

ratelimit/ratelimit.go

+const defaultMaxBurstSize = 4 * 1024 * 1024 // Must be bigger than the biggest request.
+
+func newRateLimiter(bandwidth resource.SizeSuffix) *rate.Limiter {
+	// The rate limiter is configured to allow a burst of 1/64th of the bandwidth or defaultMaxBurstSize.


Let's leave the original discussion and a pointer to rclone issue.
The point is that it does not allow high bandwidth if defaultMaxBurstSize is used.

Choraden · 2023-09-14T09:17:36Z

@mmatczuk comments addressed

mmatczuk · 2023-09-14T09:34:23Z

e2e/tests/flag_test.go

@@ -94,3 +97,50 @@ func TestFlagDenyDomain(t *testing.T) {
 	newClient(t, "https://www.google.com").GET("/").ExpectStatus(http.StatusForbidden)
 	newClient(t, httpbin).GET("/status/200").ExpectStatus(http.StatusOK)
 }
+


Those tests do not make any sense to me.

There should be one setup with rx and tx

Test must use more than one connection

TestFlagWriteLimit is in fact read limit

TestFlagReadLimit is doing sth very strange

We need an httpbin endpoint that copies request body to io.Discard

mmatczuk · 2023-09-14T12:09:11Z

utils/httpbin/httpbin.go

@@ -24,6 +24,7 @@ func Handler() http.Handler {
 	m.HandleFunc("/delay/", delayHandler)
 	m.HandleFunc("/status/", statusHandler)
 	m.HandleFunc("/stream-bytes/", streamBytesHandler)
+	m.HandleFunc("/stream-discard/", streamDiscardHandler)


stream comes from Dynamic data Generates random and dynamic data

It's more like sink just read all body, it feels like any function should do it.
You could be more creative and have count-bytes that one would return body size as a header etc.

mmatczuk · 2023-09-14T12:10:10Z

e2e/setups.go

@@ -359,6 +360,45 @@ func SetupFlagDenyDomain(l *setupList) {
 	)
 }

+func SetupFlagRateLimit(l *setupList) {
+	l.Add(


How about a single setup?

mmatczuk · 2023-09-14T12:11:43Z

e2e/tests/flag_test.go

+		}()
+	}
+	wg.Wait()
+	if elapsed := time.Since(ts); elapsed.Round(time.Second) != expectedTime {


Rounding is not great, let's say it can be 5% off and compute.

mmatczuk · 2023-09-14T13:49:07Z

e2e/tests/flag_test.go

+		connections     = 2
+		size            = 5 * 1024 * 1024 // 5MiB
+		expectedTime    = 6 * time.Second
+		acceptableRange = expectedTime * 5 / 100 // 5%


how about epsilon.

Sounds good.

SizeSuffix code and tests were copied from rclone project https://github.com/rclone/rclone. golangci: skip sizesuffix files chore: go mod tidy

The package exports custom Listener with global rate limiters for every accepted connection. The burst setup is inspired by the one in rclone project [1]. More on that in this thread [2]. [1] https://github.com/rclone/rclone/blob/cda09704a8078fe5cecd1107503de5ab8b401ad4/fs/accounting/token_bucket.go#L59-L77 [2] rclone/rclone#5507 golangci: disable err check in Limiter.WaitN

Fixes #299

It reads the request body and sends back the number of bytes read in a header.

mmatczuk · 2023-09-14T13:50:24Z

e2e/tests/flag_test.go

@@ -94,3 +98,79 @@ func TestFlagDenyDomain(t *testing.T) {
 	newClient(t, "https://www.google.com").GET("/").ExpectStatus(http.StatusForbidden)
 	newClient(t, httpbin).GET("/status/200").ExpectStatus(http.StatusOK)
 }
+
+func TestFlagReadLimit(t *testing.T) {


I was thinking since the test init sequence is almost identical that maybe we should have a helper function?

Extracted it, looks better indeed.

e2e/tests/flag_test.go

Choraden force-pushed the hg/rate_limit branch from e09e425 to 98b71d2 Compare September 11, 2023 15:07

mmatczuk reviewed Sep 12, 2023

View reviewed changes

Choraden force-pushed the hg/rate_limit branch 3 times, most recently from d1478a6 to 9b88eb0 Compare September 13, 2023 10:33

Choraden marked this pull request as ready for review September 13, 2023 10:54

Choraden force-pushed the hg/rate_limit branch 2 times, most recently from 88ace92 to 2f340ee Compare September 13, 2023 14:37

Choraden requested a review from mmatczuk September 13, 2023 14:46

mmatczuk reviewed Sep 14, 2023

View reviewed changes

Choraden force-pushed the hg/rate_limit branch from 2f340ee to db3998a Compare September 14, 2023 09:07

mmatczuk reviewed Sep 14, 2023

View reviewed changes

Choraden force-pushed the hg/rate_limit branch 3 times, most recently from b3bf3a1 to 4780cea Compare September 14, 2023 11:57

mmatczuk reviewed Sep 14, 2023

View reviewed changes

Choraden force-pushed the hg/rate_limit branch 5 times, most recently from 41311a3 to 86fd15d Compare September 14, 2023 13:12

mmatczuk reviewed Sep 14, 2023

View reviewed changes

forwarder: add SizeSuffix type

352f8d5

SizeSuffix code and tests were copied from rclone project https://github.com/rclone/rclone. golangci: skip sizesuffix files chore: go mod tidy

Choraden added 5 commits September 14, 2023 15:50

http_proxy: support read and write limit

239875e

Fixes #299

bind: add read and write limit

8897061

golangci: disable gochecknoglobals linter

21491f9

utils/httpbin: add count-bytes endpoint

d31bf53

It reads the request body and sends back the number of bytes read in a header.

mmatczuk reviewed Sep 14, 2023

View reviewed changes

e2e/tests/flag_test.go Outdated Show resolved Hide resolved

Choraden force-pushed the hg/rate_limit branch 3 times, most recently from 0871cfa to 5a1b0bd Compare September 14, 2023 14:16

e2e: add rate limit tests

771213c

Choraden force-pushed the hg/rate_limit branch from 5a1b0bd to 771213c Compare September 15, 2023 06:38

mmatczuk merged commit 595f74c into main Sep 15, 2023

mmatczuk deleted the hg/rate_limit branch September 15, 2023 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rate limiting #411

Add rate limiting #411

Choraden commented Sep 11, 2023 •

edited

Loading

what-the-diff bot commented Sep 11, 2023

mmatczuk Sep 12, 2023

mmatczuk Sep 12, 2023

mmatczuk commented Sep 12, 2023

mmatczuk Sep 14, 2023

Choraden Sep 14, 2023

mmatczuk Sep 14, 2023

Choraden commented Sep 14, 2023

mmatczuk Sep 14, 2023

mmatczuk Sep 14, 2023

mmatczuk Sep 14, 2023

mmatczuk Sep 14, 2023

mmatczuk Sep 14, 2023

Choraden Sep 14, 2023

mmatczuk Sep 14, 2023

Choraden Sep 14, 2023

Add rate limiting #411

Add rate limiting #411

Conversation

Choraden commented Sep 11, 2023 • edited Loading

what-the-diff bot commented Sep 11, 2023

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choraden commented Sep 14, 2023

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

Choraden commented Sep 11, 2023 •

edited

Loading