Runtime created middlewares

glerchundi commented 7 years ago

First of all thanks for this incredible piece of framework/library/tooling/whatever it is!

I would like to discuss about the idea of having a dynamic middleware/endpoints, one that can do whatever middleware action based on context/request.

For example, rate limit based on the request data, JWT email/identifier, or whatever context information you already know its there:

func NewFinishedMiddleware(response interface{}, err error) endpoint.Middleware {
    return func(_ endpoint.Endpoint) endpoint.Endpoint {
        return func(ctx context.Context, request interface{}) (interface{}, error) {
            return response, err
        }       
    }
}

func NewRuntimeMiddleware(mwFunc func (context.Context, interface{}) endpoint.Middleware) endpoint.Middleware {
    return func(next endpoint.Endpoint) endpoint.Endpoint {
        return func(ctx context.Context, request interface{}) (interface{}, error) {
            mw := mwFunc(ctx, request)
            return mw(next)(ctx, request)
        }
    }
}

And then rate limit based on token bucket retrieved from redis based on jwt identifier:

redis := ...
mw := NewRuntimeMiddleware(func(ctx context.Context, request interface{}) endpoint.Middleware {
    name := ctx.Value(jwt.JWTTokenContextKey).Header["name"]
    tbs, err := redis.Get(fmt.Sprintf("/token-buckets/%s", name)
    if err != nil {
        return NewFinishedMiddleware(nil, err)
    }       
    var tb ...
    if err := proto.Unmarshal(tbs, &tb); err != nil {
        return NewFinishedMiddleware(nil, err)
    }
    return NewTokenBucketLimiter(tb)
})

Probably there are a lot of errors in this pseudocode, this is just for having an idea how it would work.

WDYT?

peterbourgon commented 7 years ago

You can create these yourself, right? What support do you want to see in Go kit proper?

glerchundi commented 7 years ago

I would like to see this topic discussed here so that a community thought solution could benefit to everyone as I think this is a very common problem.

Probably what I have outlined before its not the best solution in terms of composability, performance, ...

How does people usually solve this problem? For example Signal Server (by (un)marshalling a leacky bucket in redis uses dynamic rate limiters per user/action.

In case you think this is something not applicable to this library, be free to close the issue.

On Fri, 31 Mar 2017 at 19:46, Peter Bourgon notifications@github.com wrote:

You can create these yourself, right? What support do you want to see in Go kit proper?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/go-kit/kit/issues/509#issuecomment-290780581, or mute the thread https://github.com/notifications/unsubscribe-auth/ACIPlnRaTTU0ej3zIx_QDv7QVTrLH8ngks5rrTvfgaJpZM4Mv6LV .

peterbourgon commented 7 years ago

I don't think this is a common problem. Or, I haven't fully understood what you're suggesting. From where I sit, every middleware is a dynamic middleware. You can write a RedisJWTRateLimitMiddleware just fine:

func MakeJWTViaRedisLimitingMiddleware(redis *redigo.Client) endpoint.Middleware {
    return func(next endpoint.Endpoint) endpoint.Endpoint {
        return func(ctx context.Context, request interface{}) (response interface{}, err error) {

            name := ctx.Value(jwt.JWTTokenContextKey).Header["name"]
            count, err := redis.Get(fmt.Sprintf("/req-count/%s", name))
            if err != nil {
                return nil, ConnectionError{InternalError: err}
            }
            if count > someLimit {
                return nil, ErrLimited
            }
            return next(ctx, request)

        }
    }
}

What you can't do, unless I am really misunderstanding something, is serialize a token bucket object into Redis and just fetch it out again, as you imply here:

if err := proto.Unmarshal(tbs, &tb); err != nil {
    return NewFinishedMiddleware(nil, err)
}
return NewTokenBucketLimiter(tb)

Am I missing the point? :)

glerchundi commented 7 years ago

You're not missing the point, its exactly that.

But, I would love to see this done in a generic way, at least give the possibility to everyone to do it in a composable and reusable manner.

I don't think this is a common problem.

In case of rate limiting, i think it is. Rate limiting APIs per client identifier cannot be done by reusing your rate limiter (go-kit/ratelimit) because token must be set before knowing the contextual data (request or context info).

What you can't do, unless I am really misunderstanding something, is serialize a token bucket object into Redis and just fetch it out again, as you imply here:

Take a look at these references, in case race (which would require atomic get/test/set changes in redis) is not that important for you, token bucket can be got, deserialized, modified, in case it applies rate limited based on this, serialized and then set again into redis:

Maybe I can put some examples on the table on how this can be achieved, take into account that although I'm talking at all times about rate limiting, this applies to every middleware that can be tweaked with request/context scoped data (circuit breakers for example):

The example I put when i opened the issue. Reusable, but not that performant. Allocating a middleware per each request seems like too much.
NewTokenBucketLimiter refactored/extended to NewTokenBucketLimiterWithResolver, obviously this is the easiest but at the same time the most cumbersome because it must be done per each middleware. Perhaps it can be adopted as a pattern.
```
func NewTokenBucketLimiter(tb *ratelimit.Bucket) endpoint.Middleware {
return NewTokenBucketLimiterWithResolver(func(_ context.Context, _ interface{}) { return tb })
}
```

func NewTokenBucketLimiterWithResolver(resolver func(context, interface{}) *ratelimit.Bucket) endpoint.Middleware { [...] }

* The same as before, but contaminating the context, i really dislike this option because it's not that explicit and it messes the context (but just in case):

func NewTokenBucketLimiter(tb *ratelimit.Bucket) endpoint.Middleware { v, ok := ctx.Value("RateLimiterResolver") resolver := v.(func(context.Context, interface{}) [...] }



With this comment I would like to convey the idea of the need for this type of patterns.

WDYT?

peterbourgon commented 7 years ago

But, I would love to see this done in a generic way, at least give the possibility to everyone to do it in a composable and reusable manner.

The existing endpoint.Middleware definition is already a generic, composable, and reusable pattern for creating these "dynamic" middlewares. As far as I can see, there is no advantage to adding another e.g. DynamicMiddleware type definition to Go kit.

Rate limiting APIs per client identifier cannot be done by reusing your rate limiter (go-kit/ratelimit) because token must be set before knowing the contextual data (request or context info).

You're right: the rate limiting that Go kit ships with is per-service-instance rate limiting, not per-client-identifier rate limiting. But, as I've demonstrated in my code sample, it's straightforward to construct a per-client-identifier rate limiter using the existing endpoint.Middleware definition and shared storage (e.g. Redis) without any difficulty.

Take a look at these references, in case race (which would require atomic get/test/set changes in redis) is not that important for you, token bucket can be got, deserialized, modified, in case it applies rate limited based on this, serialized and then set again into redis:

Those links appear to show a Java implementation of a token bucket rate limiter. I'm familiar with the concept; Go kit also provides one. The links don't appear to have anything to do with serializing/deserializing the object itself into Redis as a means to share state between processes. That's what I was objecting to — it's not a reasonable implementation of the pattern, as it's racy and inefficient. If you want to use distributed memory (e.g. Redis) as a means of sharing state between processes, you should interact with that state using natively-supported primitives (e.g. Redis counters) as my code sample above illustrates.

glerchundi commented 7 years ago

The existing endpoint.Middleware definition is already a generic, composable, and reusable pattern for creating these "dynamic" middlewares. As far as I can see, there is no advantage to adding another e.g. DynamicMiddleware type definition to Go kit.

Right, I agree with you. It was just a way to open the discussion :)

You're right: the rate limiting that Go kit ships with is per-service-instance rate limiting, not per-client-identifier rate limiting. But, as I've demonstrated in my code sample, it's straightforward to construct a per-client-identifier rate limiter using the existing endpoint.Middleware definition and shared storage (e.g. Redis) without any difficulty.

Yep, its straightforward but probably people is doing it in their own applications all the time. IMO, I think that being open to adapt this library to all the uses cases would be a big win. And would help identifying bugs/problems in the core library because everyone would be using it (as it solves most of the use cases!).

Those links appear to show a Java implementation of a token bucket rate limiter.

Its the leaky bucket indeed (https://en.wikipedia.org/wiki/Leaky_bucket).

The links don't appear to have anything to do with serializing/deserializing the object itself into Redis as a means to share state between processes.

https://github.com/WhisperSystems/Signal-Server/blob/master/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimiter.java#L59-L68

That's what I was objecting to — it's not a reasonable implementation of the pattern, as it's racy and inefficient. If you want to use distributed memory (e.g. Redis) as a means of sharing state between processes, you should interact with that state using natively-supported primitives (e.g. Redis counters) as my code sample above illustrates.

Don't take it as the way to solve it, it can be done in a atomic way in redis of course, but that's not the point. If go-kit opens an extension point to resolve, in this case the token bucket, its up to the api/library consumer how to implement it. I shown how does Signal people do, and in case race is not that important for you, like for the Signal guys its a easy&straghtforward way to implement it.

peterbourgon commented 7 years ago

OK, I think we're talking past each other. If your proposal is to add the NewFinishedMiddleware and NewRuntimeMiddleware definitions to Go kit, I veto it on the grounds that they don't provide any novel value. If your proposal is something different, please make it concrete :)

go-kit / kit

Runtime created middlewares #509