Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve rate limit policy #665

Merged
merged 7 commits into from
Apr 23, 2018
Merged

Improve rate limit policy #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
592c756
have whitespace between the test blocks
y-tabata Apr 8, 2018
dbe4ede
change ">= 0.001" to ">0"
y-tabata Apr 8, 2018
12c1914
change description of the json schema
y-tabata Apr 8, 2018
3ffff7a
change to table.insert
y-tabata Apr 9, 2018
d5c2d13
remove try function
y-tabata Apr 10, 2018
e66bb69
change to define redis just once
y-tabata Apr 10, 2018
4ab2ac8
add "scope" of the key and "error_settings" and define "minimum"
y-tabata Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions examples/policies/rate_limit_configuration.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@ local rate_limit_policy = require('apicast.policy.rate_limit').new({
limiters = {
{
name = "connections",
key = "limit1",
key = {name = "limit1"},
conn = 20,
burst = 10,
delay = 0.5
},
{
name = "leaky_bucket",
key = "limit2",
key = {name = "limit2"},
rate = 18,
burst = 9
},
{
name = "fixed_window",
key = "limit3",
key = {name = "limit3"},
count = 10,
window = 10
}},
Expand Down
172 changes: 148 additions & 24 deletions gateway/src/apicast/policy/rate_limit/apicast-policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,54 @@
"description": "List of limiters to be applied",
"type": "array",
"items": {
"oneOf": [{
"anyOf": [{
"type": "object",
"properties": {
"name": {
"type": "string",
"enum": ["connections"],
"description": "limiting request concurrency (or concurrent connections)"
"description": "Limiting request concurrency (or concurrent connections)"
},
"key": {
"description": "Key of limiter",
"type": "string"
"description": "The key corresponding to the limiter object",
"type": "object",
"properties": {
"name": {
"type": "string",
"description": "The name of the key, must be unique in the scope"
},
"scope": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mikz mikz Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikz Thank you!

"type": "string",
"description": "Scope of the key",
"default": "global",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default should be service.

"oneOf": [{
"enum": ["global"],
"description": "Global scope, affecting to all services"
}, {
"enum": ["service"],
"description": "Service scope, affecting to one service"
}]
},
"service_name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"type": "string",
"description": "Name of service, necessary for service scope"
}
}
},
"conn": {
"type": "integer",
"description": "the maximum number of concurrent requests allowed"
"description": "The maximum number of concurrent requests allowed",
"exclusiveminimum": 0
},
"burst": {
"type": "integer",
"description": "the number of excessive concurrent requests (or connections) allowed to be delayed"
"description": "The number of excessive concurrent requests (or connections) allowed to be delayed",
"minimum": 0
},
"delay": {
"type": "number",
"description": "the default processing latency of a typical connection (or request)"
"description": "The default processing latency of a typical connection (or request)",
"exclusiveMinimum": 0
}
}
}, {
Expand All @@ -42,19 +67,43 @@
"name": {
"type": "string",
"enum": ["leaky_bucket"],
"description": "limiting request rate"
"description": "Limiting request rate"
},
"key": {
"description": "Key of limiter",
"type": "string"
"description": "The key corresponding to the limiter object",
"type": "object",
"properties": {
"name": {
"type": "string",
"description": "The name of the key, must be unique in the scope"
},
"scope": {
"type": "string",
"description": "Scope of the key",
"default": "global",
"oneOf": [{
"enum": ["global"],
"description": "Global scope, affecting to all services"
}, {
"enum": ["service"],
"description": "Service scope, affecting to one service"
}]
},
"service_name": {
"type": "string",
"description": "Name of service, necessary for service scope"
}
}
},
"rate": {
"type": "integer",
"description": "the specified request rate (number per second) threshold"
"description": "The specified request rate (number per second) threshold",
"exclusiveMinimum": 0
},
"burst": {
"type": "integer",
"description": "the number of excessive requests per second allowed to be delayed"
"description": "The number of excessive requests per second allowed to be delayed",
"minimum": 0
}
}
}, {
Expand All @@ -63,19 +112,43 @@
"name": {
"type": "string",
"enum": ["fixed_window"],
"description": "limiting request counts"
"description": "Limiting request counts"
},
"key": {
"description": "Key of limiter",
"type": "string"
"description": "The key corresponding to the limiter object",
"type": "object",
"properties": {
"name": {
"type": "string",
"description": "The name of the key, must be unique in the scope"
},
"scope": {
"type": "string",
"description": "Scope of the key",
"default": "global",
"oneOf": [{
"enum": ["global"],
"description": "Global scope, affecting to all services"
}, {
"enum": ["service"],
"description": "Service scope, affecting to one service"
}]
},
"service_name": {
"type": "string",
"description": "Name of service, necessary for service scope"
}
}
},
"count": {
"type": "integer",
"description": "the specified number of requests threshold"
"description": "The specified number of requests threshold",
"exclusiveMinimum": 0
},
"window": {
"type": "integer",
"description": "the time window in seconds before the request count is reset"
"description": "The time window in seconds before the request count is reset",
"exclusiveMinimum": 0
}
}
}]
Expand All @@ -85,13 +158,64 @@
"description": "URL of Redis",
"type": "string"
},
"status_code_rejected": {
"type": "integer",
"description": "the status code when requests over the limit, default 429"
},
"logging_only": {
"type": "boolean",
"description": "if true, the request goes through when there is some issue with rate limiting, default false"
"error_settings": {
"description": "List of error settings",
"type": "array",
"items": {
"anyOf": [{
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": ["limits_exceeded"],
"description": "The error setting when requests over the limit"
},
"status_code": {
"type": "integer",
"description": "The status code when requests over the limit",
"default": 429
},
"error_handling": {
"type": "string",
"description": "How to handle an error",
"default": "exit",
"oneOf": [{
"enum": ["exit"],
"description": "Respond with an error"
}, {
"enum": ["log"],
"description": "Let the request go through and only output logs"
}]
}
}
}, {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": ["configuration_issue"],
"description": "The error setting when there is some configuration issue"
},
"status_code": {
"type": "integer",
"description": "The status code when there is some configuration issue",
"default": 500
},
"error_handling": {
"type": "string",
"description": "How to handle an error",
"default": "exit",
"oneOf": [{
"enum": ["exit"],
"description": "Respond with an error"
}, {
"enum": ["log"],
"description": "Let the request go through and only output logs"
}]
}
}
}]
}
}
}
}
Expand Down
Loading