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

Add conditions to rate-limit policy #839

Merged
merged 5 commits into from
Aug 20, 2018
Merged

Add conditions to rate-limit policy #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4fb4d6
policy/conditional: extract 'condition' in schema to 'definitions'
davidor Aug 3, 2018
e875a31
policy/rate_limit: add conditions to keys in config schema
davidor Aug 3, 2018
e60c459
policy/rate_limit: add conditions
davidor Aug 10, 2018
4c279db
t/apicast-policy-rate-limit: add test with conditions
davidor Aug 10, 2018
27312ff
CHANGELOG: add entry for conditions in rate-limit policy
davidor Aug 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Request headers are now exposed in the context available when evaluating Liquid [PR #819](https://github.com/3scale/apicast/pull/819)
- Rewrite URL captures policy. This policy captures arguments in a URL and rewrites the URL using them [PR #827](https://github.com/3scale/apicast/pull/827), [THREESCALE-1139](https://issues.jboss.org/browse/THREESCALE-1139)
- Support for HTTP Proxy [THREESCALE-221](https://issues.jboss.org/browse/THREESCALE-221), [PR #835](https://github.com/3scale/apicast/pull/835)
- Conditions for the limits of the rate-limit policy [PR #839](https://github.com/3scale/apicast/pull/839)

### Changed

Expand Down
12 changes: 8 additions & 4 deletions gateway/src/apicast/policy/conditional/apicast-policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,11 @@
}
},
"required": ["left", "op", "right"]
}
},
"properties": {
},
"condition": {
"description": "conditions to be evaluated",
"$id": "#/definitions/condition",
"type": "object",
"description": "condition to be evaluated",
"properties": {
"operations": {
"type": "array",
Expand All @@ -75,6 +74,11 @@
"default": "and"
}
}
}
},
"properties": {
"condition": {
"$ref": "#/definitions/condition"
},
"policy_chain": {
"description": "the policy chain to execute when the condition is true",
Expand Down
75 changes: 75 additions & 0 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 @@ -54,6 +54,72 @@
"enum": ["log"],
"description": "Let the request go through and only output logs"
}]
},
"operation": {
"type": "object",
"$id": "#/definitions/operation",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define those in the policy manifest spec? And then just refer to those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean manifest-schema.json ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It could make the validation harder, but sharing those definitions would be good in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would avoid duplication, but I'm not sure if the tooling we have for validation will work correctly. I'll try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think it's better to not include them in manifest-schema.json. I'd rather duplicate that for now in the 2 policies that need those definitions than opening them in the schema and having to support them in case someones starts depending on them. At least until the conditional policy is more mature.

"properties": {
"left": {
"type": "string"
},
"op": {
"type": "string",
"enum": ["==", "!="]
},
"right": {
"type": "string"
},
"left_type": {
"description": "How to evaluate 'left'",
"type": "string",
"default": "plain",
"oneOf": [
{
"enum": ["plain"],
"title": "Evaluate 'left' as plain text."
},
{
"enum": ["liquid"],
"title": "Evaluate 'left' as liquid."
}
]
},
"right_type": {
"description": "How to evaluate 'right'",
"type": "string",
"default": "plain",
"oneOf": [
{
"enum": ["plain"],
"title": "Evaluate 'right' as plain text."
},
{
"enum": ["liquid"],
"title": "Evaluate 'right' as liquid."
}
]
}
},
"required": ["left", "op", "right"]
},
"condition": {
"$id": "#/definitions/condition",
"type": "object",
"description": "condition to be evaluated",
"properties": {
"operations": {
"type": "array",
"items": {
"$ref": "#/definitions/operation"
},
"minItems": 1
},
"combine_op": {
"type": "string",
"enum": ["and", "or"],
"default": "and"
}
}
}
},
"properties": {
Expand All @@ -65,6 +131,9 @@
"key": {
"$ref": "#/definitions/key"
},
"condition": {
"$ref": "#/definitions/condition"
},
"conn": {
"type": "integer",
"description": "The maximum number of concurrent requests allowed",
Expand All @@ -91,6 +160,9 @@
"key": {
"$ref": "#/definitions/key"
},
"condition": {
"$ref": "#/definitions/condition"
},
"rate": {
"type": "integer",
"description": "The specified request rate (number per second) threshold",
Expand All @@ -112,6 +184,9 @@
"key": {
"$ref": "#/definitions/key"
},
"condition": {
"$ref": "#/definitions/condition"
},
"count": {
"type": "integer",
"description": "The specified number of requests threshold",
Expand Down
72 changes: 55 additions & 17 deletions gateway/src/apicast/policy/rate_limit/rate_limit.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ local limit_traffic = require "resty.limit.traffic"
local ngx_variable = require ('apicast.policy.ngx_variable')
local redis_shdict = require('redis_shdict')

local Condition = require('apicast.policy.conditional.condition')
local Operation = require('apicast.policy.conditional.operation')

local tonumber = tonumber
local next = next
local shdict_key = 'limiter'
Expand Down Expand Up @@ -79,6 +82,29 @@ local function init_error_settings(limits_exceeded_error, configuration_error)
return error_settings
end

local function build_operations(config_ops)
local res = {}

for _, op in ipairs(config_ops or {}) do
insert(res, Operation.new(op.left, op.left_type, op.op, op.right, op.right_type))
end

return res
end

local function build_condition(config_condition)
local cond = config_condition or {}

return Condition.new(
build_operations(cond.operations),
cond.combine_op
)
end

local function liquid_context(policy_context)
return ngx_variable.available_context(policy_context)
end

local function build_limiters_and_keys(type, limiters, redis, error_settings, context)
local res_limiters = {}
local res_keys = {}
Expand All @@ -94,6 +120,12 @@ local function build_limiters_and_keys(type, limiters, redis, error_settings, co

lim.dict = redis or lim.dict

lim.condition_is_true = function(policy_context)
local condition = build_condition(limiter.condition)
local liquid_ctx = liquid_context(policy_context)
return condition:evaluate(liquid_ctx)
end

insert(res_limiters, lim)

local key = limiter.template_string:render(
Expand Down Expand Up @@ -122,6 +154,28 @@ local function build_templates(limiters)
end
end

local function select_condition_is_true(groups_of_limiters, groups_of_keys, policies_ctx)
local res_limiters = {}
local res_keys = {}

for i = 1, #groups_of_limiters do
local limiters = groups_of_limiters[i]
local keys = groups_of_keys[i]

for j = 1, #limiters do
local limiter = limiters[j]
local key = keys[j]

if limiter.condition_is_true(policies_ctx) then
insert(res_limiters, limiter)
insert(res_keys, key)
end
end
end

return res_limiters, res_keys
end

function _M.new(config)
local self = new()
self.config = config or {}
Expand Down Expand Up @@ -160,25 +214,9 @@ function _M:access(context)
local fixed_window_limiters, fixed_window_keys = build_limiters_and_keys(
'fixed_window', self.fixed_window_limiters, red, self.error_settings, context)

local limiters = {}
local limiter_groups = { conn_limiters, leaky_bucket_limiters, fixed_window_limiters }
for _, limiter_group in ipairs(limiter_groups) do
if #limiter_group > 0 then
for _, limiter in ipairs(limiter_group) do
insert(limiters, limiter)
end
end
end

local keys = {}
local keys_groups = { conn_keys, leaky_bucket_keys, fixed_window_keys }
for _, keys_group in ipairs(keys_groups) do
if #keys_group > 0 then
for _, key in ipairs(keys_group) do
insert(keys, key)
end
end
end
local limiters, keys = select_condition_is_true(limiter_groups, keys_groups, context)

local states = {}
local connections_committed = {}
Expand Down
Loading