Improve rate limit policy #665

y-tabata · 2018-04-10T04:38:55Z

This PR improves the rate limit policy according to the comments of PR #648.

davidor · 2018-04-10T09:59:43Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

@@ -104,7 +83,7 @@ function _M:access()
  local keys = {}

  for _, limiter in ipairs(self.limiters) do
-    local lim, initerr = init_limiter(limiter)
+    local lim, initerr = traffic_limiters[limiter.name](limiter)


I'm not sure about this change.

resty.limit calls assert when instantiating some limiters. See this for example:
https://github.com/openresty/lua-resty-limit-traffic/blob/master/lib/resty/limit/req.lua#L56

I'd expect that the library returned that kind of errors in the second var of the response, but that's not the case. For that reason, I think that we should catch the error like we did before.

What do you think @mikz ?

I think we should just define those requirements fro those parameters in the JSON schema.

Catching errors in LuaJIT is going to trash performance and it is really not necessary as we can just ensure we pass the right parameters.

Errors in OpenResty are raised only on super critical unexpected issues and those are not really expected to be recovered.
Errors that are supposed to be handled are returned as second return value and those we should handle as well as nil values etc.

Defining those constraints in the schema sounds better 👍

davidor · 2018-04-10T10:01:34Z

gateway/src/apicast/policy/rate_limit/apicast-policy.json

@@ -20,7 +20,7 @@
                "description": "Limiting request concurrency (or concurrent connections)"
              },
              "key": {
-                "description": "Key of limiter",
+                "description": "The user key corresponding to the limiter object. This must be unique in the scope.",


This is just a detail, but I think we should change the user key part. The reason is that one of the credentials that 3scale backend accepts is also called user key, and it's unrelated to this. In order to avoid confusion, I'd just call it key.

davidor · 2018-04-10T10:19:18Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

-        return
-      end
-    end
+    lim.dict = red and red or lim.dict


One of the red is redundant. This should be red or lim.dict.

davidor · 2018-04-10T10:27:17Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

+  end
+
+  for i, lim in ipairs(limiters) do
+    lim.dict = red and red or lim.dict


This should be red or lim.dict.

davidor · 2018-04-13T10:41:03Z

gateway/src/apicast/policy/rate_limit/apicast-policy.json

+              },
+              "status_code": {
+                "type": "integer",
+                "description": "The status code when requests over the limit, default 429"


You can use the default field.

davidor · 2018-04-13T12:08:39Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

+  local error_settings = default_error_settings
+  if config_error_settings then
+    for _, error_setting in pairs(config_error_settings) do
+      if error_setting.type then


Do we need this check? I think the type should be required according to the schema.

davidor · 2018-04-13T12:20:36Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

+  if config_error_settings then
+    for _, error_setting in pairs(config_error_settings) do
+      if error_setting.type then
+        if error_setting.status_code then


status_code is not required according to the schema. This means that here the code could skip this if, and leave the status_code unassigned. I'm not sure if this could cause problems.

I think we should either we make status_code required, or should have an else branch here that assigns the default value defined (429).

Same for error_handling below.

What do you think?

@davidor At L82, I set default_error_settings to error_settings, so the code does not leave the status_code and the error_handling unassigned.
When a user defines type and status_code, then status_code is changed.
When a user defines type and error_handling, then error_handling is changed.

You're right 👍
I was confused, sorry.

davidor · 2018-04-13T13:23:54Z

Good job @y-tabata 👍

This PR introduces some nice improvements. I've added comments about minor issues but in general looks good 👍

The circle s2i-builder check is failing, but it has nothing to do with your PR. I fixed the issue in PR #666

Can you please rebase on master so the fix applies in this branch? Thanks.

davidor · 2018-04-16T09:25:29Z

Tests are failing because there are errors in the schema. The error is in the s2i-builder check:

gateway/src/apicast/policy/rate_limit/apicast-policy.json invalid
[ { keyword: 'type',
    dataPath: '.configuration.properties[\'limiters\'].items.oneOf[0].properties[\'$ref\']',
    schemaPath: '#/type',
    params: { type: 'object,boolean' },
    message: 'should be object,boolean' },
  { keyword: 'type',
    dataPath: '.configuration.properties[\'limiters\'].items',
    schemaPath: '#/type',
    params: { type: 'array' },
    message: 'should be array' },
  { keyword: 'anyOf',
    dataPath: '.configuration.properties[\'limiters\'].items',
    schemaPath: '#/properties/items/anyOf',
    params: {},
    message: 'should match some schema in anyOf' } ]

y-tabata · 2018-04-18T03:16:17Z

@davidor The deploy error has nothing to do with this PR, isn't it?

mikz · 2018-04-18T06:21:02Z

@y-tabata yep, ci/circleci: deploy will always fail for external contributors.

davidor · 2018-04-19T17:07:08Z

@y-tabata This looks good 👍
Great work 👍

Could you squash some of your commits before merging, please? We try to keep a clean git history, although we are not strict about it. In your PR, there are many commits that fix issues introduced in previous commits. For example, the remove redundant commit fixes a problem introduced in change to define redis just once commit. So would be nice to squash those 2 before merging.

Just try to squash those that seem obvious, like the ones I mentioned, to clean the PR a bit. Do not spend too much time on this. Please let me know if you need any help with this.

Also, @mikz do you want to do a quick review before merging?

ytabata · 2018-04-20T04:00:55Z

@davidor I squashed some of my commits. Is there any unnecessary commits?

davidor · 2018-04-20T08:27:20Z

Thanks @y-tabata 👍

mikz · 2018-04-23T09:21:05Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

+
+    local key
+    if limiter.key.scope == "service" then
+      key = limiter.key.service_name.."_"..limiter.name.."_"..limiter.key.name


Better to use string.format. Concatenating strings allocates new one on every concatenation.

And this is done in access phase which is evaluated on every request.

And to even further help JIT, cache the string.format in the top of the file to avoid global table lookup by local format = string.format and then using just local function string.

mikz · 2018-04-23T09:22:02Z

gateway/src/apicast/policy/rate_limit/rate_limit.lua

    end

-    limiters[#limiters + 1] = lim
-    keys[#keys + 1] = limiter.key
+    table.insert(keys, key)


table is global object and global table lookup trashes performance.

Cache any functions on global objects locally on top of the file like: local insert = table.insert and then just use their local reference insert.

mikz · 2018-04-23T09:24:50Z

gateway/src/apicast/policy/rate_limit/apicast-policy.json

+                      "description": "Service scope, affecting to one service"
+                    }]
+                  },
+                  "service_name": {


I thought this won't be supplied by users but rather just automatically using the service_id available in the context. So all service scope limiters are automatically scoped and can't be shared between services. This could still be shared by multiple services when they fill the same identifier.

mikz · 2018-04-23T09:25:09Z

gateway/src/apicast/policy/rate_limit/apicast-policy.json

+                  "scope": {
+                    "type": "string",
+                    "description": "Scope of the key",
+                    "default": "global",


I think the default should be service.

mikz · 2018-04-23T09:27:27Z

gateway/src/apicast/policy/rate_limit/apicast-policy.json

+                    "type": "string",
+                    "description": "The name of the key, must be unique in the scope"
+                  },
+                  "scope": {


I think we could extract this into shared definition in this schema an reuse it so it is not duplicated 3 times.

See an example:
https://github.com/3scale/apicast/blob/4ab2ac80d1bd6f952de4a0068932b5a992619e0b/gateway/src/apicast/policy/manifest-schema.json#L11-L21

Possibly even the whole key property could be shared between all limiters.

@mikz If I change schema like this:

"definitions": { "key": { "$id": "#/definitions/key", "description": "The key corresponding to the limiter object", ... } }, "properties": { "limiters": { ... "items": { "anyOf": [{ "type": "object", "properties": { "name": { ... }, "$ref": "#/definitions/key", "conn": { ... },

the following errors appear in ajv:

gateway/src/apicast/policy/rate_limit/apicast-policy.json invalid [ { keyword: 'type', dataPath: '.configuration.properties[\'limiters\'].items.anyOf[0].properties[\'$ref\']', schemaPath: '#/type', params: { type: 'object,boolean' }, message: 'should be object,boolean' }, { keyword: 'type', dataPath: '.configuration.properties[\'limiters\'].items', schemaPath: '#/type', params: { type: 'array' }, message: 'should be array' }, { keyword: 'anyOf', dataPath: '.configuration.properties[\'limiters\'].items', schemaPath: '#/properties/items/anyOf', params: {}, message: 'should match some schema in anyOf' } ]

Do you have any idea to avoid them?

@y-tabata it should be:

{ "properties": { "key": { "$ref": "#/definitions/key" } }

You have "$ref" as a property of "properties", not "key" property.

See the example I linked:
https://github.com/3scale/apicast/blob/4ab2ac80d1bd6f952de4a0068932b5a992619e0b/gateway/src/apicast/policy/manifest-schema.json#L44-L62

@mikz Thank you!

mikz

👍 good job!

My comments are not critical enough, so we can merge this and continue later.
We are trying to merge PRs that will go into the next release, so better to merge this now and make improvements later.

y-tabata · 2018-04-23T23:27:35Z

@mikz @davidor Thank you for your review! I continue to improve the policy.

davidor reviewed Apr 10, 2018

View reviewed changes

davidor added the skip-changelog label Apr 10, 2018

davidor reviewed Apr 13, 2018

View reviewed changes

davidor mentioned this pull request Apr 13, 2018

Fix image tag in s2i-builder circle CI job #666

Merged

y-tabata added 2 commits April 20, 2018 11:41

have whitespace between the test blocks

592c756

change ">= 0.001" to ">0"

dbe4ede

y-tabata force-pushed the rate_limiting_to_service_policy branch 5 times, most recently from 8588974 to 4f5d327 Compare April 20, 2018 03:09

y-tabata added 4 commits April 20, 2018 12:10

change description of the json schema

12c1914

change to table.insert

3ffff7a

remove try function

d5c2d13

change to define redis just once

e66bb69

y-tabata force-pushed the rate_limiting_to_service_policy branch from 4f5d327 to c31ed90 Compare April 20, 2018 03:11

add "scope" of the key and "error_settings" and define "minimum"

4ab2ac8

y-tabata force-pushed the rate_limiting_to_service_policy branch from c31ed90 to 4ab2ac8 Compare April 20, 2018 03:51

davidor approved these changes Apr 20, 2018

View reviewed changes

mikz reviewed Apr 23, 2018

View reviewed changes

mikz approved these changes Apr 23, 2018

View reviewed changes

mikz merged commit 21fa6e9 into 3scale:master Apr 23, 2018

y-tabata deleted the rate_limiting_to_service_policy branch April 23, 2018 23:24

y-tabata mentioned this pull request Apr 24, 2018

Improve rate limit policy #679

Closed

This was referenced May 14, 2018

Rate Limit policy to take into account changes in the config #703

Merged

Improve rate limit policy #704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rate limit policy #665

Improve rate limit policy #665

y-tabata commented Apr 10, 2018

davidor Apr 10, 2018

mikz Apr 10, 2018

davidor Apr 10, 2018

davidor Apr 10, 2018

davidor Apr 10, 2018

davidor Apr 10, 2018

davidor Apr 13, 2018

davidor Apr 13, 2018

davidor Apr 13, 2018

y-tabata Apr 16, 2018

davidor Apr 16, 2018

davidor commented Apr 13, 2018

davidor commented Apr 16, 2018

y-tabata commented Apr 18, 2018

mikz commented Apr 18, 2018

davidor commented Apr 19, 2018 •

edited

Loading

ytabata commented Apr 20, 2018

davidor commented Apr 20, 2018

mikz Apr 23, 2018

mikz Apr 23, 2018

mikz Apr 23, 2018

mikz Apr 23, 2018

mikz Apr 23, 2018

mikz Apr 23, 2018

y-tabata Apr 24, 2018

mikz Apr 24, 2018 •

edited

Loading

y-tabata Apr 24, 2018

mikz left a comment

y-tabata commented Apr 23, 2018

Improve rate limit policy #665

Improve rate limit policy #665

Conversation

y-tabata commented Apr 10, 2018

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

davidor commented Apr 13, 2018

davidor commented Apr 16, 2018

y-tabata commented Apr 18, 2018

mikz commented Apr 18, 2018

davidor commented Apr 19, 2018 • edited Loading

ytabata commented Apr 20, 2018

davidor commented Apr 20, 2018

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

mikz Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

y-tabata commented Apr 23, 2018

davidor commented Apr 19, 2018 •

edited

Loading

mikz Apr 24, 2018 •

edited

Loading