updated plugin following best practice from issue#10. #33

x-077 · 2019-04-06T23:41:03Z

The plugin has been reviewed, there is a example folder with an example of usage, I did not prepare the test file yet but the example show all the cases possible.

Let me know if that's suitable for you.

Here how it works, with fastify we register the plugin only once with the default parameter value that we would apply if this one is missing in the rules ( or just not set ).

The main options are :

max : set the max fetch that can do a user during a definite period of time;
timeWindow : to set the time frame
cache : not use I think
whitelist : is an array of ip which will not be restricted
redis : connection redis ( from pool for example )
skipOnError : skip errors;
keyGenerator : to generate a different key to identify the user in the cache. By default it's the ip, but It could also be the username. To adapt based on the infra, in case of reverse proxy for example.

  {
    max: 3000, // default 1000 
    //timeWindow: 1000*60, // default 1000 * 60
    //cache: 10000, // default 5000
    //whitelist: ['127.0.0.1'],
    //redis: redis, // default null
    skipOnError: false, // default false
    //keyGenerator: function(req) { /* ... */ }, // default (req) => req.raw.ip
  }

example of get with all the parameter and the 2 callback :

onExceeding: when the user is currently fetching/consuming his rate limit
onExceeded : when the user reached his rate limit.

fastify.get('/public/sub-rated-1', {
  config : {
    rateLimit : {
      timeWindow: '1 minute',
      whitelist : ['127.0.2.1'],
      onExceeding : function(req) {
        console.log('callback on exceededing ... executed before response to client. req is give as argument')
      },
      onExceeded : function(req) {
        console.log('callback on exceeded ... to black ip in security group for example, req is give as argument')
      }
    }
  }
},(req, reply) => {
  reply.send({ hello: 'from sub-rated-1 ... using default max value ... ' })
})

an other example. Here we can see that max was not provided, so it will automatically get the default value set in the plugin definition/registration with fastify. In this case it will be 3000

fastify.get('/private',{
  config : {
    rateLimit : {
      whitelist : ['127.0.2.1'],
      timeWindow: '1 minute'
    }
  }
},(req, reply) => {
  reply.send({ hello: 'from ... private' })
})

You will notice that whitelist is in the main config of the plugin and also in the rule. There is actually 2 different scope.

One scope global : define in the main conf. Will be use if you trust the source to access everything without limit;
One scope endpoint : define in the individual rule, to allow a source to access only specific endpoint.

Let me know what you think about this.

Thanks.

Eomm

The tests in CI are not running due to the linting.

Please, fix them so we can check easily this PR.
In most of the work can be done running standard --fix

index.js

…st correct

mcollina

This is very good work!

I would also like to retain the previous behavior. By default, the rate limit is applied to all routes. It can be disabled, and then it’s per-route. We should also support keeping it enabled for all routes, but off or with different settings for one.

mcollina · 2019-04-07T14:12:12Z

store/LocalStore.js

+    this.timers[keyName] = setTimeout(() => {
+      this.lru.delete(keyName)
+      this.timers[keyName] = null
+    }, timeWindow)


this would need an unref().

The previous approach was way more performant. This is going to create a significant higher amount of timers.. and those are expensive. Could this be refactored to avoid this and keep a global timer? I think keeping a plugin-wide timewindow make sense, otherwise things would become a bit more weird to debug.

I’m also ok on a per-route timewindow.

Hello @mcollina ,

Ok . I will change that.

Hello @mcollina ,

actually I'm not sure to understand, would you mind refactor this part. I added the unref

@mcollina

This is very good work!

I would also like to retain the previous behavior. By default, the rate limit is applied to all routes. It can be disabled, and then it’s per-route. We should also support keeping it enabled for all routes, but off or with different settings for one.

done. I also updated the doc with what i've done today.

please validate.

x-077 · 2019-04-08T00:23:09Z

Hello,

A new version, I corrected .unref() in the LocalStore. I also took the liberty to add a new functionality which is to store and check the whiteList in Redis.

There a 2 kind of whiteList :

global : will affect all the endpoint.
endpoint : will affect only the targeted endpoint.

The Global whitelist is set in the plugin when registering it with fastify.register(...).
The Endpoint whitelist is set on the endpoint directly with the { config : rateLimit : whitelist : [] }object.

To use it, you need to set the option whiteListInRedis to true when initialising the plugin.Set to false will use the normal way. ( in a arrray/object ).

The good thing about this feature is that it allow the admin to inject new IP/username ( what ever was define with the keyGenerator) dynamically in redis. For example if you want to whitelist a new IP whitout restarting your app. I hope it make sense.

I also added a new option in the definition of the plugin call appName. This value will be use as a prefix key in redis ( for clarity ). By default, the value is default... :D

I also refactor the plugin to add the support of the default behavior with the possibility to on / off globally as requested.

The doc was updated too with a example and the definition of each params.

@mcollina regarding Local cache and your comment, feel free to change it the way you want.
I also commented the last test on purpose... new one need to be prepared anyway ...

Thanks

…preHandler

… generate the preHandler

mcollina

I think timeWindow should just be a global settings. We are rate-limiting an ip, and the grace time should be shared by all routes, not by a single, specific route.

README.md

mcollina · 2019-04-08T07:04:01Z

index.js

+    cache: params.prefixCache ? params.prefixCache.replace(/[-,.;#*]/g, ':') : urlT
+  }
+  if (pluginComponent.isRedis && pluginComponent.whiteListInRedis) {
+    prefix.wl = urlT


What's urlT? What's wl? Please use long, descriptive variable names.

const urlT = (routeOptions.url === '/') ? 'root' : routeOptions.url.replace(/\//g, ':').slice(1)

urlT is the url transformed in case the user did not give a prefixCache. It will also make sure that if it's / ( root ), it will replace it with a string (root) as by default I replace the / by :.

wl is for the whiteList, it's a shortname for the variable.

When using redis the key look like this by default :

**For the whiteList define in the plugin (global) **

${pluginComponent.app}:wlg:${prefix.wl}

For the whiteList define in the route using { config : rateLimit : { whitelist : [...] } }

${pluginComponent.app}:wle:${prefix.wl}

pluginComponent.app is the name of the app define in the plugin with the option appName. It's useful for clarity in redis.

Let me know if further question.

Can you use log variables, and add the above explanation as a comment?

Sure, Let me comment the whole code if you want. it will be easier to review.

Hello @mcollina , commented all the code. Should be better to read.

mcollina · 2019-04-08T07:06:08Z

index.js

-      store.incr(key, onIncr)
+      if (pluginComponent.whitelist.global.indexOf(key) > -1 || pluginComponent.whitelist.endpoint[prefix.cache].indexOf(key) > -1) {
+        next()
+      }
    }


I'm not sure about this logic. This PR has already too much change, and it's getting hard to review. Would you mind removing the new whitelist mechanism and send it as a follow-up PR?

mcollina · 2019-04-08T07:08:21Z

index.js

+        res.header('X-RateLimit-Limit', params.max)
+        res.header('X-RateLimit-Remaining', params.max - current)
+
+        if (typeof params.onExceeding === 'function') {


I'd prefer to pass in a nop implementation and avoid the if check.

Sorry I don't know nop. I will check, you mean the if ... ?

function nop () {}, stick it at the end of the file.

ok, I will check :)

I'm sorry I look into it but I might be stupid I'm not sure how to do what you request with the nop function, feel free to modify as you wish it to be.

When you validate the settings:

params.onExceeding = params.onExceeding || nop

Then you can avoid the if-check completely. V8 will optimize the function call away.

mcollina · 2019-04-08T07:15:33Z

index.js

+      }
+    } else {
+      if (routeOptions.config && routeOptions.config.rateLimit && typeof routeOptions.config.rateLimit === 'object') {
+        buildRouteRate(pluginComponent, makeParams(routeOptions.config.rateLimit), routeOptions, next)


You should not pass next here. It's the outer next, and you have already called it when this happens.

Yes I had a doubt about that, I will change that.

Do you think it could work to return a new plugin from within the rate-limit-plugin, like a plugin generated on the fly ?

What I mean is that I could do something like :

const fp = require('fastify-plugin') ............................ .... some code .... ............................ return fp(buildRouteRate(fastify, { components : pluginComponent params : , makeParams(routeOptions.config.rateLimit), route : routeOptions }, next))

instead of :

buildRouteRate(pluginComponent, makeParams(routeOptions.config.rateLimit), routeOptions)

Would it be suitable, or it's not really in the logic of the fp package ?

It won't work. But I don't understand why you'll need this. Also, why are you passing next there.

the next in buildRouteRate is a mistake on my side, as you said earlier it's not needed .

I will commit the update at lunch, Sorry.

Co-Authored-By: matth-c3 <30625765+matth-c3@users.noreply.github.com>

mcollina · 2019-04-08T11:23:45Z

README.md

+      onExceeding: function (req) {
+        console.log('callback on exceededing ... executed before response to client')
+      },
+>>>>>>> 6c3daf23c35af4d1d62ff685699f2e466386424a


This should be fixed.

Yes sorry .. problem during the merge ...

mcollina · 2019-04-08T11:24:33Z

index.js

-  const max = opts.max || 1000
-  const whitelist = opts.whitelist || []
-  const after = ms(timeWindow, { long: true })
+  // const makeParams = (p) => { return { ...globalParams, ...p } }


Why is this ommented out?

It was crashing during the test on a older version of node.

mcollina · 2019-04-08T11:24:59Z

test.js

+//     t.strictEqual(res.headers['x-ratelimit-limit'], undefined)
+//     t.strictEqual(res.headers['x-ratelimit-remaining'], undefined)
+//   })
+// })


Why is this commented out?

This test was not going throw, I prefer us to review the code first if that's fine with you as all 99 previous test goes throw.
New test need to be created anyway, I will fix this one when the time will come.

mcollina · 2019-04-08T11:27:43Z

I'd still prefer to have the code for the new whitelist in its own PR. This is already big as a contribution.

Also, I think the timeWindow should be plugin-wise and not configurable in an individual route. I think It should be for each given ip, not per route/ip combination.

x-077 · 2019-04-08T12:13:43Z

I'd still prefer to have the code for the new whitelist in its own PR. This is already big as a contribution.

Well from my point of view it's more like a refresh / renew than a contribution.
No sorry I will not submit a different PR, I spend enough time this morning to comment everything in the clearest way possible. It's the 3rd PR that I do to fastify ( one on the initial plugin review which was not good I can admit it, one on the main core of fastify, and this).

I'm not greedy or lazy but the code is clear enough, same as the comment. If it's not just let me know and I will simply start a different plugin on my own.

Please be aware that the whitelist in redis is just a start, I already have a working version where params are adapting/updating dynamically from redis.

Also, I think the timeWindow should be plugin-wise and not configurable in an individual route. I think It should be for each given ip, not per route/ip combination.

Sure as mention already feel free to change it and I will pull your changes later on today.

Thanks.

jsumners · 2019-04-08T12:30:26Z

No sorry I will not submit a different PR, I spend enough time this morning to comment everything in the clearest way possible. It's the 3rd PR that I do to fastify ( one on the initial plugin review which was not good I can admit it, one on the main core of fastify, and this).

This is not the right attitude to take when submitting a PR. It is easiest to get changes accepted when 1. the PR is limited to one overall change and 2. you listen to the reviewers's feedback. This PR started out as one thing and then half-way through it you added in more:

#33 (comment)

A new version, I corrected .unref() in the LocalStore. I also took the liberty to add a new functionality which is to store and check the whiteList in Redis.

This is after your work on the original intent of the PR was commended:

#33 (review)

This is very good work!

The reviewers are simply trying to help you get the PR into a mergeable state. Big features can take a few days of review back-and-forth to get right. But that whole back-and-forth process is how good, maintainable, code gets included in an OSS project.

x-077 · 2019-04-08T12:47:58Z

@jsumners in that case take the time that you ( reviewers ) needs to take to do instead of dictating on what I should PR or not. If you need a week to valid those changes than take the week, even two or three but don't comment the way you (yes you) do because it's not helping.

So far I complied and did what I was asked to do/change/redo ( maybe even more ). If you ( reviewers ) want something to be change, you can do it and we (contributors) would be grateful too as it's how collaborative team work and things are moving.

Again, if you are not satisfy, you think that it's too much or you that's simply too complicated, just close the PR.

Like yours, my time is precious too. Think about that all of that next time before you answer to someone.

mcollina · 2019-04-08T12:58:39Z

You are probably saying this to one of most open teams in the land of Node.js: this is a friendly and open community. In fact, none of what we said was disrespectful in any form.

The truth is we care a lot about maintenance. Keeping some code around that has some relevant issues is going to cause problems in the future. Landing a change we are not convinced off would mean that we have to rework that PR, or fix a lot of bugs on it in the future. Once this is landed, you might walk away or stay around - I cannot rely on it. As such, I need to minimize that will take to future us to maintain this.

Note that this is an amazing feature and really good work done so far. However, I would like to land this with the minimum diff, as it's almost a rewrite.

I'll push the changes on the timewindow when I get the chance.

mcollina · 2019-04-10T08:12:21Z

@matth-c3 can you enable me to push to your repo? It's blocked and I can't push.

mcollina · 2019-04-10T08:30:02Z

I fix my review comments about LocalStore, and I removed the Redis whitelist feature.
I might have removed too much, as the code was complex with a lot of nested ifs.

x-077 · 2019-04-10T08:36:10Z

Ok, just let me know when you merge your change in your repo that I can delete mine.

Thanks.

x-077 · 2019-04-12T16:25:38Z

Hello @mcollina , is there any action remaining on my side ?

mcollina · 2019-04-13T08:51:10Z

I would like to use onRequest instead of preHandler here, so I did fastify/fastify#1594

x-077 · 2019-04-13T12:47:36Z

Hello @mcollina, on my side I will not use it, just change what you feel like :)

maybe it's better for you to create a new branch instead of working on the fork ?

x-077 · 2019-04-18T00:26:25Z

closing in favor of #34

matthieu ROLLA added 4 commits April 5, 2019 06:43

change to be able to set by route

7d8fa51

change to be able to set by route

df22d4d

change to be able to set by route

fb78cd6

plugin reviewed and modified following issue#10

6c2ccc8

x-077 mentioned this pull request Apr 6, 2019

change to be able to set rateLimit by route directly from fastify fastify/fastify#1576

Closed

4 tasks

Eomm requested changes Apr 7, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

change to be able to set rateLimit by route directly from fastify, te…

c5d9fea

…st correct

mcollina reviewed Apr 7, 2019

View reviewed changes

add capacity to store whitelist in redis

c1c8fb8

matthieu ROLLA added 4 commits April 8, 2019 04:41

add global mode on/off + refactor of the way the plugin generate the …

6535175

…preHandler

add global mode for default bahavior + refactor of the way the plugin…

be9c93f

… generate the preHandler

add global mode for default bahavior + refactor of the way the plugin…

42dc9a3

… generate the preHandler

add global mode for default bahavior + refactor of the way the plugin…

1c75cc5

… generate the preHandler

mcollina reviewed Apr 8, 2019

View reviewed changes

mcollina and others added 3 commits April 8, 2019 09:18

Update README.md

6c3daf2

Co-Authored-By: matth-c3 <30625765+matth-c3@users.noreply.github.com>

comment each step of the plugin

b34aa7f

comment each step of the plugin

13a52f7

mcollina reviewed Apr 8, 2019

View reviewed changes

Refactored and removed the Redis whitelist functionality.

48b02c0

mcollina added 2 commits April 10, 2019 10:22

Support to disable the rate limit.

267908f

Adjusted README

6f285cf

x-077 closed this Apr 18, 2019

updated plugin following best practice from issue#10. #33

updated plugin following best practice from issue#10. #33

Conversation

x-077 commented Apr 6, 2019 • edited

Eomm left a comment

Choose a reason for hiding this comment

mcollina left a 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

x-077 commented Apr 8, 2019 • edited

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x-077 Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x-077 Apr 8, 2019 • edited

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

mcollina Apr 8, 2019 • edited

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

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

mcollina commented Apr 8, 2019

x-077 commented Apr 8, 2019 • edited

jsumners commented Apr 8, 2019

x-077 commented Apr 8, 2019

mcollina commented Apr 8, 2019

mcollina commented Apr 10, 2019

mcollina commented Apr 10, 2019

x-077 commented Apr 10, 2019 • edited

x-077 commented Apr 12, 2019

mcollina commented Apr 13, 2019

x-077 commented Apr 13, 2019

x-077 commented Apr 18, 2019

x-077 commented Apr 6, 2019 •

edited

x-077 commented Apr 8, 2019 •

edited

x-077 Apr 8, 2019 •

edited

x-077 Apr 8, 2019 •

edited

mcollina Apr 8, 2019 •

edited

x-077 commented Apr 8, 2019 •

edited

x-077 commented Apr 10, 2019 •

edited