diff --git a/CHANGELOG.md b/CHANGELOG.md index cb1358a6c..d199378ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed Financial-grade API (FAPI) policy not showing up in the admin portal [PR #1528](https://github.com/3scale/APIcast/pull/1528) [THREESCALE-11620](https://issues.redhat.com/browse/THREESCALE-11620) - Remove Conditional Policy from the UI [PR #1534](https://github.com/3scale/APIcast/pull/1534) [THREESCALE-6116](https://issues.redhat.com/browse/THREESCALE-6116) - Remove redis connection error message from response body in edge limiting policy [PR #1537](https://github.com/3scale/APIcast/pull/1537) [THREESCALE-11701](https://issues.redhat.com/browse/THREESCALE-11701) +- Fix `on_failed` policy doesn't work with `conditional policy` [THREESCALE-11738](https://issues.redhat.com/browse/THREESCALE-11738) [PR #1541](https://github.com/3scale/APIcast/pull/1541) ### Added diff --git a/gateway/src/apicast/policy/conditional/README.md b/gateway/src/apicast/policy/conditional/README.md index b5b3325f4..fd296fd18 100644 --- a/gateway/src/apicast/policy/conditional/README.md +++ b/gateway/src/apicast/policy/conditional/README.md @@ -46,6 +46,10 @@ When the request is not a POST, the order of execution for each phase is: 2) Caching 3) Upstream +NOTE: when one or more policies in conditional chain are invalid, APIcast will +skip the invalid policy and load the next policy in the chain, which may lead +to unexpected behavior. If you want to terminate the chain, add an `on_failed` +policy to the chain. ## Conditions @@ -138,3 +142,39 @@ the `Backend` header of the request is `staging`: } ``` + +With `on-failed` policy + +```json +{ + "name":"conditional", + "version":"builtin", + "configuration":{ + "condition":{ + "operations":[ + { + "left":"{{ headers['Backend'] }}", + "left_type":"liquid", + "op":"==", + "right":"staging" + } + ] + }, + "policy_chain":[ + { + "name": "example", + "version": "1.0", + "configuration": {} + }, + { + "name": "on_failed", + "version": "builtin", + "configuration": { + "error_status_code": 419 + } + } + ] + } +} + +``` diff --git a/gateway/src/apicast/policy/conditional/conditional.lua b/gateway/src/apicast/policy/conditional/conditional.lua index 731b88247..7d113dacd 100644 --- a/gateway/src/apicast/policy/conditional/conditional.lua +++ b/gateway/src/apicast/policy/conditional/conditional.lua @@ -1,54 +1,66 @@ local ipairs = ipairs local insert = table.insert -local policy = require('apicast.policy') +local Policy = require('apicast.policy') local policy_phases = require('apicast.policy').phases local PolicyChain = require('apicast.policy_chain') local Condition = require('apicast.conditions.condition') local Operation = require('apicast.conditions.operation') local ngx_variable = require('apicast.policy.ngx_variable') -local _M = policy.new('Conditional policy', 'builtin') +local _M = Policy.new('Conditional policy', 'builtin') local new = _M.new -local function build_policy_chain(chain) - if not chain then return {} end +do + local null = ngx.null - local policies = {} + local function policy_configuration(policy) + local config = policy.configuration - for i=1, #chain do - policies[i] = PolicyChain.load_policy( - chain[i].name, - chain[i].version, - chain[i].configuration - ) + if config and config ~= null then + return config + end end - return PolicyChain.new(policies) -end + local function build_policy_chain(policies) + if not policies then return {} end -local function build_operations(config_ops) - local res = {} + local chain = PolicyChain.new() - for _, op in ipairs(config_ops) do - insert(res, Operation.new(op.left, op.left_type, op.op, op.right, op.right_type)) + for i=1, #policies do + chain:add_policy( + policies[i].name, + policies[i].version, + policy_configuration(policies[i]) + ) + end + + return chain end - return res -end + local function build_operations(config_ops) + local res = {} -function _M.new(config) - local self = new(config) + for _, op in ipairs(config_ops) do + insert(res, Operation.new(op.left, op.left_type, op.op, op.right, op.right_type)) + end + + return res + end - config.condition = config.condition or {} - self.condition = Condition.new( - build_operations(config.condition.operations), - config.condition.combine_op - ) + function _M.new(config) + local self = new(config) - self.policy_chain = build_policy_chain(config.policy_chain) - return self + config.condition = config.condition or {} + self.condition = Condition.new( + build_operations(config.condition.operations), + config.condition.combine_op + ) + + self.policy_chain = build_policy_chain(config.policy_chain) + return self + end end function _M:export() diff --git a/t/apicast-policy-conditional.t b/t/apicast-policy-conditional.t index e9d97eca5..e1306bfad 100644 --- a/t/apicast-policy-conditional.t +++ b/t/apicast-policy-conditional.t @@ -225,3 +225,120 @@ yay, api backend --- error_code: 200 --- no_error_log [error] + + + +=== TEST 5: conditional policy combined with on-failed policy +This test shows that conditional policies can be used in conjunction with an +on-failed policy that will return a 419 when one or more policies in the chain +fail to load. In this test, we will attempt to load an example policy that +does not exist. +--- configuration +{ + "services": [ + { + "id": 42, + "proxy": { + "policy_chain": [ + { + "name": "apicast.policy.conditional", + "configuration": { + "condition": { + "operations": [ + { + "left": "{{ uri }}", + "left_type": "liquid", + "op": "==", + "right": "/", + "right_type": "plain" + } + ] + }, + "policy_chain": [ + { + "name": "example", + "version": "1.0", + "configuration": {} + }, + { + "name": "on_failed", + "version": "builtin", + "configuration": { + "error_status_code": 419 + } + } + ] + } + }, + { + "name": "apicast.policy.echo" + } + ] + } + } + ] +} +--- request +GET / +--- error_code: 419 +--- no_error_log +[error] + + + +=== TEST 6: conditional policy combined with on-failed policy +With this test, the on-failed policy only triggers if the condition is met +(request path match "/get"). +--- configuration +{ + "services": [ + { + "id": 42, + "proxy": { + "policy_chain": [ + { + "name": "apicast.policy.conditional", + "configuration": { + "condition": { + "operations": [ + { + "left": "{{ uri }}", + "left_type": "liquid", + "op": "==", + "right": "/get", + "right_type": "plain" + } + ] + }, + "policy_chain": [ + { + "name": "example", + "version": "1.0", + "configuration": {} + }, + { + "name": "on_failed", + "version": "builtin", + "configuration": { + "error_status_code": 419 + } + } + ] + } + }, + { + "name": "apicast.policy.echo" + } + ] + } + } + ] +} +--- pipelined_requests eval +["GET /","GET /get"] +--- response_body eval +["GET / HTTP/1.1\n",""] +--- error_code eval +[200, 419] +--- no_error_log +[error]