[THREESCALE-9320] Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed confusing log display when APIcast listens on HTTPS and path routing is enabled [PR #1486](https://github.com/3scale/APIcast/pull/1486/files) [THREESCALE #8486](https://issues.redhat.com/browse/THREESCALE-8486)
 
+- Fixed Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers [PR #1485](https://github.com/3scale/APIcast/pull/1485) [THREESCALE-9320](https://issues.redhat.com/browse/THREESCALE-9320)
+
 ### Added
 
 - Bump openresty to 1.21.4.3 [PR #1461](https://github.com/3scale/APIcast/pull/1461) [THREESCALE-10601](https://issues.redhat.com/browse/THREESCALE-10601)

gateway/src/apicast/policy/camel/camel.lua

@@ -45,17 +45,15 @@ function _M:access(context)
   upstream:set_skip_https_connect_on_proxy()
 end
 
-function _M:export()
+function _M:rewrite(context)
   -- This get_http_proxy function will be called in upstream just in case if a
   -- proxy is defined.
-  return  {
-    get_http_proxy = function(uri)
-      if not uri.scheme then
-        return nil
-      end
-      return find_proxy(self, uri.scheme)
+  context.get_http_proxy = function(uri)
+    if not uri.scheme then
+      return nil
     end
-  }
+    return find_proxy(self, uri.scheme)
+  end
 end
 
 return _M

gateway/src/apicast/policy/http_proxy/proxy.lua

@@ -34,15 +34,15 @@ local function find_proxy(self, scheme)
   return self.proxies[scheme]
 end
 
-function _M:export()
-  return  {
-    get_http_proxy = function(uri)
-      if not uri.scheme then
-        return nil
-      end
-      return find_proxy(self, uri.scheme)
+function _M:rewrite(context)
+  -- APIcast reads this flag in the access phase, that's why we need to set it
+  -- in rewrite phase.
+  context.get_http_proxy = function(uri)
+    if not uri.scheme then
+      return nil
     end
-  }
+    return find_proxy(self, uri.scheme)
+  end
 end
 
 return _M

gateway/src/apicast/policy/request_unbuffered/request_unbuffered.lua

@@ -13,10 +13,8 @@ function _M.new(config)
   return self
 end
 
-function _M:export()
-  return {
-    request_unbuffered = true,
-  }
+function _M:rewrite(context)
+  context.request_unbuffered = true
 end
 
 return _M

gateway/src/apicast/policy/retry/retry.lua

@@ -13,8 +13,8 @@ function _M.new(config)
   return self
 end
 
-function _M:export()
-  return { upstream_retries = self.retries }
+function _M:rewrite(context)
+  context.upstream_retries = self.retries
 end
 
 return _M

gateway/src/apicast/policy/upstream_connection/upstream_connection.lua

@@ -18,13 +18,11 @@ function _M.new(config)
   return self
 end
 
-function _M:export()
-  return {
-    upstream_connection_opts = {
-      connect_timeout = self.connect_timeout,
-      send_timeout = self.send_timeout,
-      read_timeout = self.read_timeout
-    }
+function _M:rewrite(context)
+  context.upstream_connection_opts = {
+    connect_timeout = self.connect_timeout,
+    send_timeout = self.send_timeout,
+    read_timeout = self.read_timeout
   }
 end
 

spec/policy/camel/camel_spec.lua

@@ -8,15 +8,20 @@ describe('Camel policy', function()
 
   local http_uri = {scheme="http"}
   local https_uri = {scheme="https"}
+  local context
+
+  before_each(function()
+    context = {}
+  end)
 
   it("http[s] proxies are defined if all_proxy is in there", function()
     local proxy = camel_policy.new({
       all_proxy = all_proxy_val
     })
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.same(callback.get_http_proxy(http_uri), resty_url.parse(all_proxy_val))
-    assert.same(callback.get_http_proxy(https_uri), resty_url.parse(all_proxy_val))
+    assert.same(context.get_http_proxy(http_uri), resty_url.parse(all_proxy_val))
+    assert.same(context.get_http_proxy(https_uri), resty_url.parse(all_proxy_val))
   end)
 
   it("all_proxy does not overwrite http/https proxies", function()
@@ -25,34 +30,35 @@ describe('Camel policy', function()
       http_proxy = http_proxy_val,
       https_proxy = https_proxy_val
     })
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.same(callback.get_http_proxy(http_uri), resty_url.parse(http_proxy_val))
-    assert.same(callback.get_http_proxy(https_uri), resty_url.parse(https_proxy_val))
+    assert.same(context.get_http_proxy(http_uri), resty_url.parse(http_proxy_val))
+    assert.same(context.get_http_proxy(https_uri), resty_url.parse(https_proxy_val))
   end)
 
   it("empty config return all nil", function()
     local proxy = camel_policy.new({})
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.is_nil(callback.get_http_proxy(https_uri))
-    assert.is_nil(callback.get_http_proxy(http_uri))
+    assert.is_nil(context.get_http_proxy(https_uri))
+    assert.is_nil(context.get_http_proxy(http_uri))
   end)
 
   describe("get_http_proxy callback", function()
-    local callback = camel_policy.new({
+    local proxy = camel_policy.new({
         all_proxy = all_proxy_val
-    }):export()
+    })
 
     it("Valid protocol", function()
-
-      local result = callback.get_http_proxy(
+      proxy:rewrite(context)
+      local result = context.get_http_proxy(
         resty_url.parse("http://google.com"))
       assert.same(result, resty_url.parse(all_proxy_val))
     end)
 
     it("invalid protocol", function()
-      local result = callback:get_http_proxy(
+      proxy:rewrite(context)
+      local result = context:get_http_proxy(
         {}, {scheme="invalid"})
       assert.is_nil(result)
     end)

spec/policy/http_proxy/http_proxy_spec.lua

@@ -8,54 +8,61 @@ describe('HTTP proxy  policy', function()
 
   local http_uri = {scheme="http"}
   local https_uri = {scheme="https"}
+  local context
+
+  before_each(function()
+    context = {}
+  end)
 
   it("http[s] proxies are defined if all_proxy is in there", function()
     local proxy = proxy_policy.new({
       all_proxy = all_proxy_val
     })
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.same(callback.get_http_proxy(http_uri), resty_url.parse(all_proxy_val))
-    assert.same(callback.get_http_proxy(https_uri), resty_url.parse(all_proxy_val))
+    assert.same(context.get_http_proxy(http_uri), resty_url.parse(all_proxy_val))
+    assert.same(context.get_http_proxy(https_uri), resty_url.parse(all_proxy_val))
   end)
 
   it("all_proxy does not overwrite http/https proxies", function()
+    local context = {}
     local proxy = proxy_policy.new({
       all_proxy = all_proxy_val,
       http_proxy = http_proxy_val,
       https_proxy = https_proxy_val
     })
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.same(callback.get_http_proxy(http_uri), resty_url.parse(http_proxy_val))
-    assert.same(callback.get_http_proxy(https_uri), resty_url.parse(https_proxy_val))
+    assert.same(context.get_http_proxy(http_uri), resty_url.parse(http_proxy_val))
+    assert.same(context.get_http_proxy(https_uri), resty_url.parse(https_proxy_val))
   end)
 
   it("empty config return all nil", function()
+    local context = {}
     local proxy = proxy_policy.new({})
-    local callback = proxy:export()
+    proxy:rewrite(context)
 
-    assert.is_nil(callback.get_http_proxy(https_uri))
-    assert.is_nil(callback.get_http_proxy(http_uri))
+    assert.is_nil(context.get_http_proxy(https_uri))
+    assert.is_nil(context.get_http_proxy(http_uri))
   end)
 
   describe("get_http_proxy callback", function()
-    local callback = proxy_policy.new({
-        all_proxy = all_proxy_val
-    }):export()
+    local proxy = proxy_policy.new({
+      all_proxy = all_proxy_val,
+    })
 
     it("Valid protocol", function()
-
-      local result = callback.get_http_proxy(
+      proxy:rewrite(context)
+      local result = context.get_http_proxy(
         resty_url.parse("http://google.com"))
       assert.same(result, resty_url.parse(all_proxy_val))
     end)
 
     it("invalid protocol", function()
-      local result = callback:get_http_proxy(
+      proxy:rewrite(context)
+      local result = context.get_http_proxy(
         {}, {scheme="invalid"})
       assert.is_nil(result)
     end)
-
   end)
 end)

spec/policy/retry/retry_spec.lua

@@ -5,10 +5,11 @@ describe('Retry policy', function()
     it('returns the the number of retries', function()
       local policy_config = { retries = 5, }
       local policy = RetryPolicy.new(policy_config)
+      local context = {}
 
-      local exported = policy:export()
+      policy:rewrite(context)
 
-      assert.same(policy_config.retries, exported.upstream_retries)
+      assert.same(policy_config.retries, context.upstream_retries)
     end)
   end)
 end)

spec/policy/upstream_connection/upstream_connection_spec.lua

@@ -8,20 +8,20 @@ describe('Upstream connection policy', function()
         send_timeout = 2,
         read_timeout = 3
       }
+      local context = {}
       local policy = UpstreamConnectionPolicy.new(config_timeouts)
+      policy:rewrite(context)
 
-      local exported = policy:export()
-
-      assert.same(config_timeouts, exported.upstream_connection_opts)
+      assert.same(config_timeouts, context.upstream_connection_opts)
     end)
 
     it('does not return timeout params that is not in the config', function()
       local config_timeouts = { connect_timeout = 1 } -- Missing send and read
+      local context = {}
       local policy = UpstreamConnectionPolicy.new(config_timeouts)
+      policy:rewrite(context)
 
-      local exported = policy:export()
-
-      assert.same(config_timeouts, exported.upstream_connection_opts)
+      assert.same(config_timeouts, context.upstream_connection_opts)
     end)
   end)
 end)