From fb784d6a9171e22084bd8492213339ccdaa540f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 23 Mar 2018 10:23:57 +0100 Subject: [PATCH 1/5] removing retry after when no necessary --- lib/cartodb/middleware/rate-limit.js | 4 +- test/acceptance/rate-limit.test.js | 72 ++++++++++++++++------------ 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/lib/cartodb/middleware/rate-limit.js b/lib/cartodb/middleware/rate-limit.js index 9a3daa9b..d7cc3598 100644 --- a/lib/cartodb/middleware/rate-limit.js +++ b/lib/cartodb/middleware/rate-limit.js @@ -39,11 +39,13 @@ function rateLimit(userLimitsApi, endpointGroup = null) { res.set({ 'Carto-Rate-Limit-Limit': limit, 'Carto-Rate-Limit-Remaining': remaining, - 'Retry-After': retry, 'Carto-Rate-Limit-Reset': reset }); if (isBlocked) { + // retry is floor rounded in seconds by redis-cell + res.set('Retry-After', retry + 1); + const rateLimitError = new Error('You are over the limits.'); rateLimitError.http_status = 429; rateLimitError.type = 'limit'; diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 339e187e..9ef5a813 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -87,8 +87,12 @@ function getReqAndRes() { req: {}, res: { headers: {}, - set(headers) { - this.headers = headers; + set(headers, value) { + if(typeof headers === 'object') { + this.headers = headers; + } else { + this.headers[headers] = value; + } }, locals: { user: 'localhost' @@ -97,18 +101,21 @@ function getReqAndRes() { }; } -function assertGetLayergroupRequest (status, limit, remaining, reset, retry, done = null) { - const response = { +function assertGetLayergroupRequest (status, limit, remaining, reset, retry, done) { + let response = { status, headers: { 'Content-Type': 'application/json; charset=utf-8', 'Carto-Rate-Limit-Limit': limit, 'Carto-Rate-Limit-Remaining': remaining, - 'Carto-Rate-Limit-Reset': reset, - 'Retry-After': retry + 'Carto-Rate-Limit-Reset': reset } }; + if(retry) { + response.headers['Retry-After'] = retry; + } + testClient.getLayergroup({ response }, err => { assert.ifError(err); if (done) { @@ -117,15 +124,20 @@ function assertGetLayergroupRequest (status, limit, remaining, reset, retry, don }); } -function assertRateLimitRequest (status, limit, remaining, reset, retry, done = null) { +function assertRateLimitRequest (status, limit, remaining, reset, retry, done) { const { req, res } = getReqAndRes(); rateLimit(req, res, function (err) { - assert.deepEqual(res.headers, { + let expectedHeaders = { "Carto-Rate-Limit-Limit": limit, "Carto-Rate-Limit-Remaining": remaining, - "Carto-Rate-Limit-Reset": reset, - "Retry-After": retry - }); + "Carto-Rate-Limit-Reset": reset + }; + + if(retry) { + expectedHeaders['Retry-After'] = retry; + } + + assert.deepEqual(res.headers, expectedHeaders); if(status === 200) { assert.ifError(err); @@ -178,7 +190,7 @@ describe('rate limit', function() { const burst = 1; setLimit(count, period, burst); - assertGetLayergroupRequest(200, '2', '1', '1', '-1', done); + assertGetLayergroupRequest(200, '2', '1', '1', null, done); }); it("1 req/sec: 2 req/seg should be limited", function(done) { @@ -187,12 +199,12 @@ describe('rate limit', function() { const burst = 1; setLimit(count, period, burst); - assertGetLayergroupRequest(200, '2', '1', '1', '-1'); - setTimeout( () => assertGetLayergroupRequest(200, '2', '0', '1', '-1'), 250); - setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '0'), 500); - setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '0'), 750); - setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '0'), 950); - setTimeout( () => assertGetLayergroupRequest(200, '2', '0', '1', '-1', done), 1050); + assertGetLayergroupRequest(200, '2', '1', '1'); + setTimeout( () => assertGetLayergroupRequest(200, '2', '0', '1'), 250); + setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '1'), 500); + setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '1'), 750); + setTimeout( () => assertGetLayergroupRequest(429, '2', '0', '1', '1'), 950); + setTimeout( () => assertGetLayergroupRequest(200, '2', '0', '1', null, done), 1050); }); }); @@ -234,12 +246,12 @@ describe('rate limit middleware', function () { }); it("1 req/sec: 2 req/seg should be limited", function (done) { - assertRateLimitRequest(200, 1, 0, 1, -1); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 250); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 500); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 750); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 950); - setTimeout( () => assertRateLimitRequest(200, 1, 0, 1, -1, done), 1050); + assertRateLimitRequest(200, 1, 0, 1); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 250); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 500); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 750); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 950); + setTimeout( () => assertRateLimitRequest(200, 1, 0, 1, null, done), 1050); }); it("1 req/sec: 2 req/seg should be limited, removing SHA script from Redis", function (done) { @@ -248,12 +260,12 @@ describe('rate limit middleware', function () { 'SCRIPT', ['FLUSH'], function () { - assertRateLimitRequest(200, 1, 0, 1, -1); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 500); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 500); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 750); - setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 0), 950); - setTimeout( () => assertRateLimitRequest(200, 1, 0, 1, -1, done), 1050); + assertRateLimitRequest(200, 1, 0, 1); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 500); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 500); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 750); + setTimeout( () => assertRateLimitRequest(429, 1, 0, 0, 1), 950); + setTimeout( () => assertRateLimitRequest(200, 1, 0, 1, null, done), 1050); } ); }); From 609bf13765346d66f936aed349c9d6fad46699c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 23 Mar 2018 11:42:53 +0100 Subject: [PATCH 2/5] correct error message in rate limit --- lib/cartodb/middleware/rate-limit.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/middleware/rate-limit.js b/lib/cartodb/middleware/rate-limit.js index d7cc3598..b3c6d7fe 100644 --- a/lib/cartodb/middleware/rate-limit.js +++ b/lib/cartodb/middleware/rate-limit.js @@ -46,7 +46,9 @@ function rateLimit(userLimitsApi, endpointGroup = null) { // retry is floor rounded in seconds by redis-cell res.set('Retry-After', retry + 1); - const rateLimitError = new Error('You are over the limits.'); + let rateLimitError = new Error( + 'You are over platform\'s limits. Please contact us to know more details' + ); rateLimitError.http_status = 429; rateLimitError.type = 'limit'; rateLimitError.subtype = 'rate-limit'; From 1ce908177ef56b92138c40eea007565593f5b4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 23 Mar 2018 11:47:28 +0100 Subject: [PATCH 3/5] correct error message in rate limit tests --- test/acceptance/rate-limit.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 9ef5a813..5a21ad0f 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -143,7 +143,7 @@ function assertRateLimitRequest (status, limit, remaining, reset, retry, done) { assert.ifError(err); } else { assert.ok(err); - assert.equal(err.message, 'You are over the limits.'); + assert.equal(err.message, 'You are over platform\'s limits. Please contact us to know more details'); assert.equal(err.http_status, 429); assert.equal(err.type, 'limit'); assert.equal(err.subtype, 'rate-limit'); From f19eeff8997e64e6a3de37b18f991df82d6ef25a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 23 Mar 2018 13:30:47 +0100 Subject: [PATCH 4/5] returning error mvt on rate limit --- lib/cartodb/middleware/vector-error.js | 6 +- test/acceptance/rate-limit.test.js | 87 +++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/middleware/vector-error.js b/lib/cartodb/middleware/vector-error.js index 75e93b0a..8268ec1f 100644 --- a/lib/cartodb/middleware/vector-error.js +++ b/lib/cartodb/middleware/vector-error.js @@ -5,7 +5,7 @@ module.exports = function vectorError() { return function vectorErrorMiddleware(err, req, res, next) { if(req.params.format === 'mvt') { - if (isTimeoutError(err)) { + if (isTimeoutError(err) || isRateLimitError(err)) { res.set('Content-Type', 'application/x-protobuf'); return res.status(429).send(timeoutErrorVectorTile); } @@ -27,3 +27,7 @@ function isDatasourceTimeoutError (err) { function isTimeoutError (err) { return isRenderTimeoutError(err) || isDatasourceTimeoutError(err); } + +function isRateLimitError (err) { + return err.type === 'limit' && err.subtype === 'rate-limit'; +} diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 5a21ad0f..0813023c 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -15,6 +15,7 @@ let redisClient; let testClient; let keysToDelete = ['user:localhost:mapviews:global']; const user = 'localhost'; +let layergroupid; const query = ` SELECT @@ -68,13 +69,13 @@ const createMapConfig = ({ }); -function setLimit(count, period, burst) { +function setLimit(count, period, burst, endpoint = RATE_LIMIT_ENDPOINTS_GROUPS.ANONYMOUS) { redisClient.SELECT(8, err => { if (err) { return; } - const key = `limits:rate:store:${user}:maps:${RATE_LIMIT_ENDPOINTS_GROUPS.ANONYMOUS}`; + const key = `limits:rate:store:${user}:maps:${endpoint}`; redisClient.rpush(key, burst); redisClient.rpush(key, count); redisClient.rpush(key, period); @@ -270,3 +271,85 @@ describe('rate limit middleware', function () { ); }); }); + +describe('rate limit and vector tiles', function () { + + before(function(done) { + global.environment.enabledFeatures.rateLimitsEnabled = true; + global.environment.enabledFeatures.rateLimitsByEndpoint.tile = true; + + redisClient = redis.createClient(global.environment.redis.port); + const count = 1; + const period = 1; + const burst = 0; + setLimit(count, period, burst, RATE_LIMIT_ENDPOINTS_GROUPS.TILE); + + testClient = new TestClient(createMapConfig(), 1234); + testClient.getLayergroup({status: 200}, (err, res) => { + assert.ifError(err); + + layergroupid = res.layergroupid; + + done(); + }); + }); + + after(function() { + global.environment.enabledFeatures.rateLimitsEnabled = false; + global.environment.enabledFeatures.rateLimitsByEndpoint.tile = false; + }); + + afterEach(function(done) { + keysToDelete.forEach( key => { + redisClient.del(key); + }); + + redisClient.SELECT(0, () => { + redisClient.del('user:localhost:mapviews:global'); + + redisClient.SELECT(5, () => { + redisClient.del('user:localhost:mapviews:global'); + done(); + }); + }); + }); + + it('mvt rate limited', function (done) { + const tileParams = (status, limit, remaining, reset, retry, contentType) => { + let headers = { + "Content-Type": contentType, + "Carto-Rate-Limit-Limit": limit, + "Carto-Rate-Limit-Remaining": remaining, + "Carto-Rate-Limit-Reset": reset + }; + + if (retry) { + headers['Retry-After'] = retry; + } + + return { + layergroupid: layergroupid, + format: 'mvt', + response: {status, headers} + }; + }; + + testClient.getTile(0, 0, 0, tileParams(204, '1', '0', '1'), (err) => { + assert.ifError(err); + + testClient.getTile(0, 0, 0, tileParams(429, '1', '0', '0', '1', 'application/x-protobuf'), (err, res, tile) => { + assert.ifError(err); + + var tileJSON = tile.toJSON(); + assert.equal(Array.isArray(tileJSON), true); + assert.equal(tileJSON.length, 2); + assert.equal(tileJSON[0].name, 'errorTileSquareLayer'); + assert.equal(tileJSON[1].name, 'errorTileStripesLayer'); + + done(); + }); + + }); + + }); +}); From d28c915635a2047f1d3e437ae4d0d3c6901ad4d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 23 Mar 2018 15:57:30 +0100 Subject: [PATCH 5/5] jshint happy --- test/acceptance/rate-limit.test.js | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 0813023c..eb9f8954 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -337,18 +337,23 @@ describe('rate limit and vector tiles', function () { testClient.getTile(0, 0, 0, tileParams(204, '1', '0', '1'), (err) => { assert.ifError(err); - testClient.getTile(0, 0, 0, tileParams(429, '1', '0', '0', '1', 'application/x-protobuf'), (err, res, tile) => { - assert.ifError(err); + testClient.getTile( + 0, + 0, + 0, + tileParams(429, '1', '0', '0', '1', 'application/x-protobuf'), + (err, res, tile) => { + assert.ifError(err); - var tileJSON = tile.toJSON(); - assert.equal(Array.isArray(tileJSON), true); - assert.equal(tileJSON.length, 2); - assert.equal(tileJSON[0].name, 'errorTileSquareLayer'); - assert.equal(tileJSON[1].name, 'errorTileStripesLayer'); - - done(); - }); + var tileJSON = tile.toJSON(); + assert.equal(Array.isArray(tileJSON), true); + assert.equal(tileJSON.length, 2); + assert.equal(tileJSON[0].name, 'errorTileSquareLayer'); + assert.equal(tileJSON[1].name, 'errorTileStripesLayer'); + done(); + } + ); }); });