THREESCALE-11738 - on_failed policy doesn't work with conditional policy

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
 

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
+           }
+         }
+      ]
+   }
+}
+
+```

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()

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]