Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rate limit per JWT claim #17

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

brunobastosg
Copy link

@brunobastosg brunobastosg commented Apr 23, 2021

We have a use-case where different clients have a different number of requests they can make to the API.

We implemented this in our custom KrakenD build, but I'm opening this PR in case you're interested.

Basically, we added a tierConfiguration block under github.com/devopsfaith/krakend-ratelimit/juju/router's extra_config:

EDIT (18/05/2021): I changed the config file format to accept IP/header as client identifier (as the regular client max rate does) and use a header to identify tier as suggested in the discussion below.

"tierConfiguration": {
  "headerTier": "x-tier",
  "strategy": "header",
  "key": "x-user",
  "duration": "24h",
  "tiers": [
    {
      "name": "unlimited",
      "limit": 0
    },
    {
      "name": "gold",
      "limit": 50000
    },
    {
      "name": "silver",
      "limit": 20000
    },
    {
      "name": "bronze",
      "limit": 5000
    }
  ]
}

The original format I suggested was:

"tierConfiguration": {
  "jwtClaim": "tier",
  "duration": "24h",
  "tiers": [
    {
      "name": "unlimited",
      "limit": 0
    },
    {
      "name": "gold",
      "limit": 50000
    },
    {
      "name": "silver",
      "limit": 20000
    },
    {
      "name": "bronze",
      "limit": 5000
    }
  ]
}

In the example above, a client in the gold tier can make 50k requests per day, and so on.

@alombarte alombarte requested a review from kpacha April 26, 2021 18:15
@kpacha
Copy link
Member

kpacha commented Apr 26, 2021

Hi, @brunobastosg

this is a great idea! I think we could add it to the project with some little changes.

please, check out my comments on the code

juju/router/gin/gin.go Outdated Show resolved Hide resolved
juju/router/gin/gin.go Outdated Show resolved Hide resolved
@@ -78,6 +91,18 @@ func NewIpLimiterWithKeyMw(header string, maxRate float64, capacity int64) Endpo
return NewTokenLimiterMw(NewIPTokenExtractor(header), juju.NewMemoryStore(maxRate, capacity))
}

func NewJwtClaimLimiterMw(tierConfiguration *router.TierConfiguration, fillInterval time.Duration) EndpointMw {
var stores = map[string]krakendrate.LimiterStore{}
Copy link
Member

Choose a reason for hiding this comment

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

instead of using a simple map, why don't you go with a sharded store (https://github.com/devopsfaith/krakend-ratelimit/blob/master/krakendrate.go#L52) ?

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into it.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

hi, there is a map without a mutex. There is a good implementation on the link above.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, you commented on an outdated version. What do you mean? I'm using a ShardedMemoryBackend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants