ExpressGateway / express-gateway

A microservices API Gateway built on top of Express.js
https://www.express-gateway.io
Apache License 2.0
2.97k stars 344 forks source link

Bump express-rate-limit to 5.0.0 #979

Open ioggstream opened 4 years ago

ioggstream commented 4 years ago

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

sanyamdogra commented 4 years ago

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

sanyamdogra commented 4 years ago

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.

XVincentX commented 4 years ago

@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.

sanyamdogra commented 4 years ago

@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.

XVincentX commented 4 years ago

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.

sanyamdogra commented 4 years ago

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

XVincentX commented 4 years ago

All right, let’s melt them together.

sanyamdogra commented 4 years ago

Yes sure, let's wait for this to merge

kenime commented 4 years ago

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.