sethvargo / go-limiter Goto Github PK

Package memorystore is confusing to GCP users probably since https://cloud.google.com/memorystore. inmemorystore is probably a better name to consider early on?

It is impossible to compile a project with github.com/sethvargo/go-limiter dependency using recent Go 1.17rc1.

Sample code:

package main

import (
        "context"
        _ "github.com/sethvargo/go-limiter"
        "log"
        "time"
)

func main() {
        store, err := memorystore.New(&memorystore.Config{
                // Number of tokens allowed per interval.
                Tokens: 15,

                // Interval until tokens reset.
                Interval: time.Minute,
        })
        if err != nil {
                log.Fatal(err)
        }
        ctx := context.Background()
        store.Take(ctx, key)
}

go build .
# golimitfails
github.com/sethvargo/go-limiter/memorystore.(*store).purge: relocation target runtime.walltime not defined
github.com/sethvargo/go-limiter/memorystore.newBucket: relocation target runtime.walltime not defined
github.com/sethvargo/go-limiter/memorystore.(*bucket).take: relocation target runtime.walltime not defined

go version
go version go1.17rc1 linux/amd64

Hey, this comment in fasttime.go is a lie:

Now returns a monotonic clock value

In fact, you're reading both wall time and monotonic time, and only using the wall time. You likely want this instead:

// runtimeNano returns the current value of the runtime clock in nanoseconds.
//
//go:linkname runtimeNano runtime.nanotime
func runtimeNano() int64

See CL314277 and relation chain

$ gotip test ./... -v
?       github.com/sethvargo/go-limiter [no test files]
# github.com/sethvargo/go-limiter/memorystore.test
github.com/sethvargo/go-limiter/memorystore.(*store).purge: relocation target runtime.walltime1 not defined
github.com/sethvargo/go-limiter/memorystore.newBucket: relocation target runtime.walltime1 not defined
github.com/sethvargo/go-limiter/memorystore.(*bucket).take: relocation target runtime.walltime1 not defined
github.com/sethvargo/go-limiter/memorystore.TestStore_Take.func1.2: relocation target runtime.walltime1 not defined
# github.com/sethvargo/go-limiter/httplimit.test
github.com/sethvargo/go-limiter/memorystore.(*store).purge: relocation target runtime.walltime1 not defined
github.com/sethvargo/go-limiter/memorystore.newBucket: relocation target runtime.walltime1 not defined
github.com/sethvargo/go-limiter/memorystore.(*bucket).take: relocation target runtime.walltime1 not defined
FAIL    github.com/sethvargo/go-limiter/httplimit [build failed]
?       github.com/sethvargo/go-limiter/internal/fasttime       [no test files]
FAIL    github.com/sethvargo/go-limiter/memorystore [build failed]
testing: warning: no tests to run
PASS
ok      github.com/sethvargo/go-limiter/noopstore       (cached) [no tests to run]
FAIL

This package doesn't seem to build on windows:

# github.com/hashicorp/vault
runtime.walltime: relocation target runtime.walltime1 not defined

Presumably due to

//go:noescape
//go:linkname walltime runtime.walltime
func walltime() (int64, int32)

In time_nofake.go:

func walltime() (sec int64, nsec int32) {
	return walltime1()
}

And in os_windows.go:

// walltime1 isn't implemented on Windows, but will never be called.
func walltime1() (sec int64, nsec int32)

From the docs:

The middleware automatically set the following headers, conforming to the latest RFCs:
...
X-RateLimit-Reset - UTC time when the limit resets.

In what I think is the latest IETF draft on RateLimit Header Fields for HTTP the header RateLimit-Reset is set to delay-seconds because it doesn't rely on clock synchronization and mitigates thundering heard if too many clients are given the same timestamp.

Will you consider changing and use delay-seconds?

Or is this what is also mentioned in the docs what enables a resolution higher than one second and focus more on rate limiting other services than HTTP?

If the server time is adjusted to a previous time, the condition 'b. lastTick<currTick' will not occur, and 'b. availabilityTokens' will quickly drop to 0, and subsequent requests will be restricted from accessing

Thanks for the library. I tried to use it in a project and the API is too limited for my application.

In my app I have a setup where I can let clients make unlimited number of requests if they provide a correct challenge key. But, if they fail to provide the correct key I need them to be limited for some duration.

In practice this means I have something like:

// calling validKey below is expensive so check if the client is rate limited first
_, _, _, ok := store.State 
if !ok {
  return 429
}

if !validKey(key) {
  _, _, _, ok := store.Take()
  if !ok {
    return 429
  }
}

doAppWork()

So, I would love to have the Store interface be extended to have a function such as State below:

	// State returns the current status of the limiter for the given key:
	//
	// - the configured limit size
	// - the number of remaining tokens in the interval
	// - the server time when new tokens will be available
	// - whether the take was successful
	//
	// If "ok" is false, the take was unsuccessful and the caller should NOT
	// service the request.
	//
	// See the note about keys on the interface documentation.
	State(key string) (limit, remaining, reset uint64, ok bool)

I noticed this library uses the fasttime package which uses runtime.nanotime. I'm having difficulty interpreting the value of reset returned by the Take API. For instance, looking at the HTTP middleware, you'll see a value like Sun, 04 Jan 1970 01:55:37 UTC, whereas I'd expect to see a value that is roughly greater than the time the request was executed.

Should reset be interpreted as nanoseconds until replenishment from now? If so, https://github.com/sethvargo/go-limiter/blob/main/httplimit/middleware.go#L102 doesn't seem correct.

func TestNow(t *testing.T) {
	n1 := Now()
	n2 := time.Now().UnixNano()

	fmt.Printf("fasttime: %d\n", n1)
	fmt.Printf("time.Now(): %d\n", n2)

	t1 := time.Unix(0, int64(n1)).UTC().Format(time.RFC1123)
	t2 := time.Unix(0, n2).UTC().Format(time.RFC1123)
	require.Equal(t, t1, t2)
}

=== RUN   TestNow
fasttime: 268633347645075
time.Now(): 1595538425892185000
    TestNow: fasttime_test.go:20: 
        	Error Trace:	fasttime_test.go:20
        	Error:      	Not equal: 
        	            	expected: "Sun, 04 Jan 1970 02:37:13 UTC"
        	            	actual  : "Thu, 23 Jul 2020 21:07:05 UTC"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-Sun, 04 Jan 1970 02:37:13 UTC
        	            	+Thu, 23 Jul 2020 21:07:05 UTC
        	Test:       	TestNow
--- FAIL: TestNow (0.00s)

Snippet to reproduce

package main

import (
	"context"
	"fmt"
	"time"

	"github.com/sethvargo/go-limiter/memorystore"
)

func main()  {
	s, _ := memorystore.New(&memorystore.Config{
		Tokens:        65535,
		Interval:      time.Second,
	})

	for i := 0; i < 20; i++{
		limit, remaining, _, _, _ := s.Take(context.Background(), "asd")
		fmt.Println("Run =", i, " limit = ", limit, " remaining = ", remaining)
		time.Sleep(100 * time.Millisecond)
	}

}

With this setting this will cause fillrate https://github.com/sethvargo/go-limiter/blob/main/memorystore/store.go#L278 to went below token limit, which will trigger the logic here https://github.com/sethvargo/go-limiter/blob/main/memorystore/store.go#L327-L330 causing remaining count to be incorrect.

Run = 0  limit =  65535  remaining =  65534
Run = 1  limit =  65535  remaining =  65533
Run = 2  limit =  65535  remaining =  65532
Run = 3  limit =  65535  remaining =  65531
Run = 4  limit =  65535  remaining =  65530
Run = 5  limit =  65535  remaining =  65529
Run = 6  limit =  65535  remaining =  65528
Run = 7  limit =  65535  remaining =  65527
Run = 8  limit =  65535  remaining =  65526
Run = 9  limit =  65535  remaining =  65525
Run = 10  limit =  65535  remaining =  15258  <----
Run = 11  limit =  65535  remaining =  15257
Run = 12  limit =  65535  remaining =  15256
Run = 13  limit =  65535  remaining =  15255
Run = 14  limit =  65535  remaining =  15254
Run = 15  limit =  65535  remaining =  15253
Run = 16  limit =  65535  remaining =  15252
Run = 17  limit =  65535  remaining =  15251
Run = 18  limit =  65535  remaining =  15250
Run = 19  limit =  65535  remaining =  15249

this behaviour can be triggered with 31622.776601 token/s settings
https://www.wolframalpha.com/input/?i=10000+sqrt%2810%29

Is it possible to bypass the rate limiter by some custom header? or maybe a custom bypasser function?

Sorry if the question was asked before, i just couldn't find much,

Apologies if this is a naive question. New to this area and researching my options. It looks like the "leaky-bucket" algorithm, but I haven't seen that stated explicitly anywhere.

github.com/sethvargo/go-limiter/memorystore.(*store).purge: relocation target runtime.walltime not defined
github.com/sethvargo/go-limiter/memorystore.newBucket: relocation target runtime.walltime not defined
github.com/sethvargo/go-limiter/memorystore.(*bucket).take: relocation target runtime.walltime not defined

Probably due to golang/go#46777

https://datatracker.ietf.org/doc/html/rfc7807
Allow introducing custom response behavior for 429 status code, Problem Details as an example.
Should not bring breaking changes, just will introduce new config param

Consider changing the Stop method on the stores
i.e:

To implement the io.Closer interface instead, rename Stop() to Close()

In v0.7.0, the date and time calculated for X-RateLimit-Reset is incorrect. Rather than a value relative to the current time, it's a value a shortly after the Unix epoch:

0.6.0: X-RateLimit-Reset: Fri, 10 Sep 2021 15:23:06 UTC

0.7.0: X-RateLimit-Reset: Fri, 02 Jan 1970 13:24:25 UTC

This seems to be because the timestamp being returned from fasttime.Now() does not represent the current time, which seems to be due to the switch to runtime.nanotime instead of runtime.walltime. I'm seeing this behaviour on Go 1.17 darwin/amd64.

i need to create middleware from httplimit.NewMiddleware . but its func can't take store from redisstore.New because redisstore.New return store , not storeWithContext.

it happen in

github.com/sethvargo/go-limiter v0.4.1
github.com/sethvargo/go-redisstore v0.1.2

but i can use redisstore for httplimit.NewMiddleware in older version.

https://datatracker.ietf.org/doc/html/rfc6648

Can you introduce the possibility of customizing response header names (except for HeaderRetryAfter, since it's part of 429 response)?
This change can be done without any breaking changes to existing behavior. If you will approve - I can take to implement this one.

For generating monitoring metrics, say to export to prometheus, it would be nice to be able to enumerate the keys in the store. An interface as simple as this would be fine with me:

type Store interface {
    Keys() string[]
}

This interface with a Peek (#11) would make it possible to implement metrics, I think.

Hello, many thanks for the library with a great performance and nice API.

It's written in readme that:

While go-limit supports rate limiting at the HTTP layer, it can also be used to rate limit literally anything.

I am researching a rate limiting based on traffic read from the socket. In this case I need to take the number of tokens which is equal to the length of data read from the socket. It seems not possible with the current package API. Is it good idea to extend API in this way or it's out of scope?

Hey,
Thanks for publishing this! Currently reading through the code, and snagged on this comment:

While b.take() will return true, the comment is a bit confusing. I suspect it was written before that call was put in, I guess to avoid having an extra parameter to newBucket with "initial tokens"?

I understand the benefit of the various HTTP headers which are being added when using the middleware, but what if I'm not developing an API and just want to prevent a brute-force attack? Should I still be sending the rate-limiting HTTP headers?

If not, should we have an option to disable these headers? Potentially via a new parameter for NewMiddleware?

Looks like a false trap that context that Take takes as a parameter.