express-gateway icon indicating copy to clipboard operation
express-gateway copied to clipboard

Bump express-rate-limit to 5.0.0

Open ioggstream opened this issue 5 years ago • 9 comments

I expect

express-gateway to work with express-rate-limit to 5.0.0

Instead

If I upgrade express-rate-limit, it does not work.

@sanyamdogra

ioggstream avatar Feb 07 '20 17:02 ioggstream

Yup, the schema needs to be updated by removing two deprecated functions delayMs and delayAfter.

sanyamdogra avatar Feb 09 '20 10:02 sanyamdogra

I would have made the PR with updated dependency and removing deprecated functions but just waiting for this PR to merge so that the PR makes sense.

sanyamdogra avatar Feb 09 '20 10:02 sanyamdogra

@sanyamdogra Not too fast my friend — that would be a breaking change. We are going to need to keep such parameters and write a layer that would still use them somehow.

XVincentX avatar Feb 12 '20 10:02 XVincentX

@XVincentX Yes, you are right but how to should we go about it? If we update express-rate-limit to the new version, we'll lose the functions, if we don't we lose the headers.

sanyamdogra avatar Feb 12 '20 14:02 sanyamdogra

I believe that part of features has just been moved to another package; you'd need to install that into express gateway as well and use it.

XVincentX avatar Feb 12 '20 14:02 XVincentX

Yes, it's been moved to express-slow-down.

sanyamdogra avatar Feb 12 '20 14:02 sanyamdogra

All right, let’s melt them together.

XVincentX avatar Feb 12 '20 14:02 XVincentX

Yes sure, let's wait for this to merge

sanyamdogra avatar Feb 12 '20 14:02 sanyamdogra

Can I suggest 2 things:

  1. Support more parameters from the underlying module (e.g. skipSuccessfulRequests)
  2. move slow-down to a new policy (while supporting rate-limit -> delayAfter / delayMs parameters for backward compatibility)

The reason of both suggestions is because for rate limit, the main objective is to prevent unfairness / service plan limitation, hence it should counted per person / IP without skipping successful nor failed requests.

For slow down, my usage is to prevent sudden burst of traffic causing underlying services not able to responding and result in 502 bad gateway. In such case, I would want to count concurrently processing calls instead of total call per API path. I can achieve this by using req.baseUrl + req.path as the key and enabling skipSuccessfulRequests and skipFailedRequests to decrement count when a request is completed.

With above suggestion, I could achieve my goals by setting:

- rate-limit:
        -
          action:                         # allow
            max: 100                       # max 100 request
            windowMs: 60000              # per 60 seconds
            rateLimitBy: "${req.ip}" # per request IP
- slow-down:
        -
          action:                         # slow down
            delayAfter: 4                       # after 4 concurrently processing call are in place (assume underlying service could handle 5 requests concurrently, give some buffer)
            delayMs: 400                       # by 400 ms per call queued in gateway (assume underlying service typically response in 300ms, give some buffer)
            rateLimitBy: "${req.baseUrl + req.path}" # per API Path
            windowMs: 300000              # resetting after 300 seconds (5 mins)
            skipSuccessfulRequests: true # (decrease count when request is successful)
            skipFailedRequests: true # (decrease count when request is failed)

Comments are welcomed as I maybe wrong in concept.

kenime avatar Apr 23 '20 10:04 kenime