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.

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

Routing Policy enhancements #1103

Merged
merged 8 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Option to load service configurations one by one lazily [PR #1099](https://github.com/3scale/APIcast/pull/1099), [THREESCALE-3168](https://issues.jboss.org/browse/THREESCALE-3168)
- New maintenance mode policy, useful for maintenance periods. [PR #1105](https://github.com/3scale/APIcast/pull/1105), [THREESCALE-3189](https://issues.jboss.org/browse/THREESCALE-3189)
- Remove dnsmasq process for APIcast [PR #1090](https://github.com/3scale/APIcast/pull/1090), [THREESCALE-1555](https://issues.jboss.org/browse/THREESCALE-1555)
- Enable liquid operations and original request variable on routing policy [PR #1103](https://github.com/3scale/APIcast/pull/1103) [THREESCALE-3239](https://issues.jboss.org/browse/THREESCALE-3239)
- Allow to use capture function in liquid templates. [PR #1107](https://github.com/3scale/APIcast/pull/1107), [THREESCALE-1911](https://issues.jboss.org/browse/THREESCALE-1911)
- OAuth 2.0 MTLS policy [PR #1101](https://github.com/3scale/APIcast/pull/1101) [Issue #1003](https://github.com/3scale/APIcast/issues/1003)
- Add an option to enable keepalive_timeout on gateway [THREESCALE-2886](https://issues.jboss.org/browse/THREESCALE-2886) [PR #1106](https://github.com/3scale/APIcast/pull/1106)
Expand All @@ -24,6 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Fix issues when OPENTRACING_FORWARD_HEADER was set [PR #1109](https://github.com/3scale/APIcast/pull/1109), [THREESCALE-1660](https://issues.jboss.org/browse/THREESCALE-1660)
- New TLS termination policy [PR #1108](https://github.com/3scale/APIcast/pull/1108), [THREESCALE-2898](https://issues.jboss.org/browse/THREESCALE-2898)


## [3.6.0-rc1] - 2019-07-04

### Added
Expand Down
28 changes: 27 additions & 1 deletion gateway/src/apicast/executor.lua
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,29 @@ local function build_context(executor)
return linked_list.readwrite({}, config)
end

local function store_original_request(context)
-- There are a few phases[0] that req and var are not set and API is
-- disabled. The reason to call this using a pcall function is to avoid to
-- define the phases manually that are error prone. Also in openresty there
-- is not a good method to check this. [1]
-- [0] invalid phases: init_worker, init, timer and ssl_cer
-- [1] https://github.com/openresty/lua-resty-core/blob/9937f5d83367e388da4fcc1d7de2141c9e38d7e2/lib/resty/core/request.lua#L96
--
if not context or context.original_request then
return
end

pcall(function()
context.original_request = linked_list.readonly({
headers = ngx.req.get_headers(),
host = ngx.var.host,
path = ngx.var.request_uri,
uri = ngx.var.uri,
server_addr = ngx.var.server_addr,
})
end)
end

local function shared_build_context(executor)
local ok, ctx = pcall(function() return ngx.ctx end)
if not ok then
Expand All @@ -47,10 +70,13 @@ local function shared_build_context(executor)

if not context then
context = build_context(executor)

ctx.context = context
end

if not ctx.original_request then
store_original_request(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this guarantee that the original request will be initialized on every request?

In other words, store_original_request only runs when the context has not been initialized, but store_original_request fails in some phases, so I wonder if the following case is possible:

  • The body of this if runs and initializes ctx.context
  • store_original_request is called but fails to assign the original request data
  • The body of this if is not executed again and the original request data is never initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

due to error happens in the same function call, I do not think that it's the case.

Copy link
Contributor

@davidor davidor Sep 5, 2019

Choose a reason for hiding this comment

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

@eloycoto I've reviewed the latest changes of the PR and everything looks good to me except this. Maybe I'm missing something.

I see 2 scenarios:

  • context is nil. store_original_request is called and it fails (there's a pcall so there are cases where it can fail). In that case, the original request would never be initialized in the context.
  • context is nil, but when store_original_request is called it never fails. In that case, the pcall would be unnecessary.

What am I missing?

end

return context
end

Expand Down
1 change: 1 addition & 0 deletions gateway/src/apicast/policy/ngx_variable.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ local _M = {}
local function context_values()
return {
uri = ngx.var.uri,
path = ngx.var.path,
host = ngx.var.host,
remote_addr = ngx.var.remote_addr,
remote_port = ngx.var.remote_port,
Expand Down
29 changes: 29 additions & 0 deletions gateway/src/apicast/policy/routing/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,35 @@ the path is not `/accounts`:
}
}
```
## Liquid templating on match

It is possible to use liquid templating in the match section. This allows to
define rules with dynamic information and provide a way to match with any
request info.
davidor marked this conversation as resolved.
Show resolved Hide resolved

```json
{
"name": "routing",
"version": "builtin",
"configuration": {
"rules": [
{
"url": "http://example.com",
"condition": {
"operations": [
{
"match": "liquid",
"liquid_value": "{{original_request.path}}",
"op": "matches",
"value": "/bridge-1"
}
]
}
}
]
}
}
```

## Liquid templating

Expand Down
18 changes: 17 additions & 1 deletion gateway/src/apicast/policy/routing/apicast-policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
"path",
"header",
"query_arg",
"jwt_claim"
"jwt_claim",
"liquid"
]
},
"op": {
Expand Down Expand Up @@ -114,6 +115,21 @@
"jwt_claim_name"
]
},
{
"properties": {
"match": {
"enum": [
"liquid"
]
},
"liquid_value" : {
"type": "string"
}
},
"required": [
"liquid_value"
]
},
{
"properties": {
"match": {
Expand Down
9 changes: 9 additions & 0 deletions gateway/src/apicast/policy/routing/routing_operation.lua
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

local setmetatable = setmetatable
local Operation = require('apicast.conditions.operation')
local TemplateString = require('apicast.template_string')

local _M = {}

Expand Down Expand Up @@ -53,6 +54,14 @@ function _M.new_op_with_jwt_claim(jwt_claim_name, op, value, value_type)
return new(eval_left_func, op, value, value_type)
end

function _M.new_op_with_liquid_templating(liquid_expression, op, value, value_type)
local eval_left_func = function()
return TemplateString.new(liquid_expression or "" , "liquid"):render(ngx.ctx)
end

return new(eval_left_func, op, value, value_type)
end

function _M:evaluate(context)
local left_operand_val = self.evaluate_left_side_func(context.request)

Expand Down
3 changes: 3 additions & 0 deletions gateway/src/apicast/policy/routing/rule.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ local function value_of_match(thing_to_be_matched, config_condition)
return (thing_to_be_matched == 'header' and config_condition.header_name) or
(thing_to_be_matched == 'query_arg' and config_condition.query_arg_name) or
(thing_to_be_matched == 'jwt_claim' and config_condition.jwt_claim_name) or
(thing_to_be_matched == 'liquid' and config_condition.liquid_value) or
nil
end

Expand All @@ -34,6 +35,8 @@ local function init_operation(config_operation)
return RoutingOperation.new_op_with_query_arg(match_val, op, value, value_type)
elseif match == 'jwt_claim' then
return RoutingOperation.new_op_with_jwt_claim(match_val, op, value, value_type)
elseif match == 'liquid' then
return RoutingOperation.new_op_with_liquid_templating(match_val, op, value, value_type)
else
error('Thing to be matched not supported: ' .. match)
end
Expand Down
21 changes: 21 additions & 0 deletions spec/policy/routing/routing_operation_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,27 @@ describe('RoutingOperation', function()
end)
end)


describe('when the operation involves a liquid expression', function()

before_each(function()
stub(ngx_variable, 'available_context', function() return {foo="bar"} end)
end)

it('evaluates true conditions correctly', function()
local operation = RoutingOperation.new_op_with_liquid_templating(
"{{foo}}", "==", "bar")
assert.is_true(operation:evaluate({}))
end)

it('evaluates false conditions correctly', function()
local operation = RoutingOperation.new_op_with_liquid_templating(
"{{foo}}", "==", "false")
assert.is_false(operation:evaluate({}))
end)

end)

it('can evaluate the right operand as liquid', function()
-- Stub the available context to avoid depending on ngx.var.*
stub(ngx_variable, 'available_context', function(context) return context end)
Expand Down
117 changes: 117 additions & 0 deletions t/apicast-policy-routing.t
Original file line number Diff line number Diff line change
Expand Up @@ -1439,3 +1439,120 @@ yay, api backend
--- error_code: 200
--- no_error_log
[error]

=== TEST 22: test with liquid expression
--- configuration
{
"services": [
{
"id": 42,
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.routing",
"configuration": {
"rules": [
{
"url": "http://test:$TEST_NGINX_SERVER_PORT",
"condition": {
"operations": [
{
"match": "liquid",
"liquid_value": "{{headers.foo}}",
"op": "==",
"value": "fooValue"
}
]
}
}
]
}
},
{
"name": "apicast.policy.echo"
}
]
}
}
]
}
--- upstream
location / {
content_by_lua_block {
ngx.say('yay, api backend');
}
}
--- request
GET /
--- more_headers
foo: fooValue
--- response_body
yay, api backend
--- error_code: 200
--- no_error_log
[error]

=== TEST 23: test with original request values expression
This is to validate the issue #1088, where a url_rewriting change the path in
the rewrite phase and user does not have a way to route the policy. This is to
validate that a user scenario is working correctly.
--- configuration
{
"services": [
{
"id": 42,
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.routing",
"configuration": {
"rules": [
{
"url": "http://test:$TEST_NGINX_SERVER_PORT",
"condition": {
"operations": [
{
"match": "liquid",
"liquid_value": "{{ original_request.path }}",
"op": "==",
"value": "/bridge-1"
}
]
}
}
]
}
},
{
"name": "url_rewriting",
"configuration": {
"commands": [
{
"op": "sub",
"regex": "^/bridge",
"replace": "/"
}
]
}
},
{
"name": "apicast.policy.echo"
}
]
}
}
]
}
--- upstream
location / {
content_by_lua_block {
ngx.say('yay, api backend');
}
}
--- request
GET /bridge-1
--- response_body
yay, api backend
--- error_code: 200
--- no_error_log
[error]