[THREESCALE-852] Split upstream policy phases so it can match the original path

CHANGELOG.md

@@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed set of valid values for the exit param of the Echo policy [PR #684](https://github.com/3scale/apicast/pull/684/)
 
+### Changed
+
+- The upstream policy now performs the rule matching in the rewrite phase. This allows combining it with the URL rewriting policy – upstream policy regex will be matched against the original path if upstream policy is placed before URL rewriting in the policy chain, and against the rewritten path otherwise [PR #690](https://github.com/3scale/apicast/pull/690), [THREESCALE-852](https://issues.jboss.org/browse/THREESCALE-852)
+
 ## [3.2.0-rc1] - 2018-04-24
 
 ### Added

doc/policies.md

@@ -67,12 +67,6 @@ mapping rules of APIcast will be applied against the rewritten path.
 However, if the URL policy appears after the APIcast one, the mapping rules
 will be applied against the original path.
 
-Another example, suppose that we combine the upstream policy with the URL
-rewriting one. The upstream policy acts on the content phase, whereas the URL
-rewriting one acts on the rewrite phase. This means that the upstream policy
-will always take into account the rewritten path instead of the original one,
-regardless of the position of the policies in the chain.
-
 ### Types
 
 There are two types of policy chains in APIcast: per-service chains and a

gateway/src/apicast/policy/upstream/apicast-policy.json

@@ -4,10 +4,10 @@
   "summary": "Allows to modify the upstream URL of the request based on its path.",
   "description":
     ["This policy allows to modify the upstream URL (scheme, host and port) of the request based on its path. ",
-     "It accepts regular expressions that, when matched against the request path, ",
-     "replaces it with a given string. \n",
+     "It accepts regular expressions and, when matched against the request path, ",
+     "replaces the upstream URL with a given string. \n",
      "When combined with the APIcast policy, the upstream policy should be ",
-     "placed before it in the policy chain"],
+     "placed before it in the policy chain."],
   "version": "builtin",
   "configuration": {
     "type": "object",

gateway/src/apicast/policy/upstream/upstream.lua

@@ -65,20 +65,29 @@ function _M.new(config)
   return self
 end
 
-function _M:content(context)
+function _M:rewrite(context)
   local req_uri = ngx.var.uri
 
   for _, rule in ipairs(self.rules) do
     if match(req_uri, rule.regex) then
       ngx.log(ngx.DEBUG, 'upstream policy uri: ', req_uri, ' regex: ', rule.regex, ' match: true')
-      context.upstream_changed = true
-      return change_upstream(rule.url)
-    elseif ngx.config.debug then
+      context.new_upstream = rule.url
+      break
+    else
       ngx.log(ngx.DEBUG, 'upstream policy uri: ', req_uri, ' regex: ', rule.regex, ' match: false')
     end
   end
 end
 
+function _M.content(_, context)
+  local new_upstream = context.new_upstream
+
+  if new_upstream then
+    context.upstream_changed = true
+    return change_upstream(new_upstream)
+  end
+end
+
 function _M.balancer(_, context)
   if context.upstream_changed then
     balancer.call()

spec/policy/upstream/upstream_spec.lua

@@ -2,29 +2,28 @@ local balancer = require('apicast.balancer')
 local UpstreamPolicy = require('apicast.policy.upstream')
 
 describe('Upstream policy', function()
-  describe('.content', function()
-    -- Set request URI and matched/non-matched upstream used in all the tests
-    local test_req_uri = 'http://example.com/'
-    local test_upstream_matched_host = 'localhost'
-    local test_upstream_matched = string.format("http://%s/a_path:8080",
-      test_upstream_matched_host)
-    local test_upstream_matched_proxy_pass = 'http://upstream/a_path:8080'
-    local test_upstream_not_matched = 'http://localhost/a_path:80'
-
-    local context = {} -- Context shared between policies
-
-    before_each(function()
-      context = {}
-
-      -- Set headers_sent to false, otherwise, the upstream is not changed
-      ngx.headers_sent = false
-
-      -- ngx functions and vars
-      ngx.var = { uri = test_req_uri }
-      stub(ngx.req, 'set_header')
-      stub(ngx, 'exec')
-    end)
+  -- Set request URI and matched/non-matched upstream used in all the tests
+  local test_req_uri = 'http://example.com/'
+  local test_upstream_matched_host = 'localhost'
+  local test_upstream_matched = string.format("http://%s/a_path:8080",
+    test_upstream_matched_host)
+  local test_upstream_matched_proxy_pass = 'http://upstream/a_path:8080'
+  local test_upstream_not_matched = 'http://localhost/a_path:80'
+
+  local test_upstream_matched_url = {
+    host = 'localhost',
+    path = '/a_path:8080',
+    scheme = 'http'
+  }
+
+  local context -- Context shared between policies
+
+  before_each(function()
+    context = {}
+    ngx.var = { uri = test_req_uri }
+  end)
 
+  describe('.rewrite', function()
     describe('when there is a rule that matches the request URI', function()
       local config_with_a_matching_rule = {
         rules = {
@@ -35,18 +34,9 @@ describe('Upstream policy', function()
 
       local upstream = UpstreamPolicy.new(config_with_a_matching_rule)
 
-      it('changes the upstream according to that rule', function()
-        upstream:content(context)
-
-        assert.equals(test_upstream_matched_proxy_pass, ngx.var.proxy_pass)
-        assert.stub(ngx.req.set_header).was_called_with('Host',
-          test_upstream_matched_host)
-        assert.stub(ngx.exec).was_called_with('@upstream')
-      end)
-
-      it('marks in the context that the upstream has changed', function()
-        upstream:content(context)
-        assert.is_truthy(context.upstream_changed)
+      it('stores in the context the URL of that rule', function()
+        upstream:rewrite(context)
+        assert.same(test_upstream_matched_url, context.new_upstream)
       end)
     end)
 
@@ -61,7 +51,45 @@ describe('Upstream policy', function()
 
       local upstream = UpstreamPolicy.new(config_with_several_matching_rules)
 
-      it('changes the upstream according to the first rule that matches', function()
+      it('stores in the context the URL of the 1st rule that matches', function()
+        upstream:rewrite(context)
+        assert.same(test_upstream_matched_url, context.new_upstream)
+      end)
+    end)
+
+    describe('when there are no rules that match the request URI', function()
+      local config_without_matching_rules = {
+        rules = {
+          { regex = '/i_dont_match', url = test_upstream_not_matched }
+        }
+      }
+
+      local upstream = UpstreamPolicy.new(config_without_matching_rules)
+
+      it('does not store a URL in the context', function()
+        upstream:rewrite(context)
+        assert.is_nil(context.new_upstream)
+      end)
+    end)
+  end)
+
+  describe('.content', function()
+    before_each(function()
+      -- Set headers_sent to false, otherwise, the upstream is not changed
+      ngx.headers_sent = false
+
+      stub(ngx.req, 'set_header')
+      stub(ngx, 'exec')
+    end)
+
+    describe('when there is a new upstream in the context', function()
+      before_each(function()
+        context.new_upstream = test_upstream_matched_url
+      end)
+
+      it('changes the upstream', function()
+        local upstream = UpstreamPolicy.new({})
+
         upstream:content(context)
 
         assert.equals(test_upstream_matched_proxy_pass, ngx.var.proxy_pass)
@@ -71,21 +99,19 @@ describe('Upstream policy', function()
       end)
 
       it('marks in the context that the upstream has changed', function()
+        local upstream = UpstreamPolicy.new({})
         upstream:content(context)
         assert.is_truthy(context.upstream_changed)
       end)
     end)
 
-    describe('when there are no rules that match the request URI', function()
-      local config_without_matching_rules = {
-        rules = {
-          { regex = '/i_dont_match', url = test_upstream_not_matched }
-        }
-      }
-
-      local upstream = UpstreamPolicy.new(config_without_matching_rules)
+    describe('when there is not a new upstream in the context', function()
+      before_each(function()
+        context.new_upstream = nil
+      end)
 
       it('does not change the upstream', function()
+        local upstream = UpstreamPolicy.new({})
         upstream:content(context)
 
         assert.is_nil(ngx.var.proxy_pass)
@@ -94,6 +120,7 @@ describe('Upstream policy', function()
       end)
 
       it('does not mark in the context that the upstream has changed', function()
+        local upstream = UpstreamPolicy.new({})
         upstream:content(context)
         assert.is_falsy(context.upstream_changed)
       end)

t/apicast-policy-upstream.t

@@ -418,3 +418,123 @@ MHcCAQEEIFCV3VwLEFKz9+yTR5vzonmLPYO/fUvZiMVU1Hb11nN8oAoGCCqGSM49
 AwEHoUQDQgAEhkmo6Xp/9W9cGaoGFU7TaBFXOUkZxYbGXQfxyZZucIQPt89+4r1c
 bx0wVEzbYK5wRb7UiWhvvvYDltIzsD75vg==
 -----END EC PRIVATE KEY-----
+
+=== TEST 9: before the URL rewriting policy in the chain
+Check that the upstream policy matches the original path of the request
+instead of the rewritten one.
+--- backend
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      local expected = "service_token=token-value&service_id=42&usage%5Bhits%5D=2&user_key=uk"
+      require('luassert').same(ngx.decode_args(expected), ngx.req.get_uri_args(0))
+    }
+  }
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version":  1,
+      "backend_authentication_type": "service_token",
+      "backend_authentication_value": "token-value",
+      "proxy": {
+        "api_backend": "http://example.com:80/",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ],
+        "policy_chain": [
+          {
+            "name": "apicast.policy.upstream",
+            "configuration": {
+              "rules": [ { "regex": "/original", "url": "http://test:$TEST_NGINX_SERVER_PORT" } ]
+            }
+          },
+          {
+            "name": "apicast.policy.url_rewriting",
+            "configuration": {
+              "commands": [
+                { "op": "gsub", "regex": "/original", "replace": "/rewritten" }
+              ]
+            }
+          },
+          { "name": "apicast.policy.apicast" }
+        ]
+      }
+    }
+  ]
+}
+--- upstream
+  location /rewritten {
+     content_by_lua_block {
+       require('luassert').are.equal('GET /rewritten?user_key=uk&a_param=a_value HTTP/1.1',
+                                     ngx.var.request)
+       ngx.say('yay, api backend');
+     }
+  }
+--- request
+GET /original?user_key=uk&a_param=a_value
+--- response_body
+yay, api backend
+--- error_code: 200
+--- no_error_log
+[error]
+
+=== TEST 10: after the URL rewriting policy in the chain
+Check that the upstream policy matches the rewritten path of the request
+instead of the original one.
+--- backend
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      local expected = "service_token=token-value&service_id=42&usage%5Bhits%5D=2&user_key=uk"
+      require('luassert').same(ngx.decode_args(expected), ngx.req.get_uri_args(0))
+    }
+  }
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version":  1,
+      "backend_authentication_type": "service_token",
+      "backend_authentication_value": "token-value",
+      "proxy": {
+        "api_backend": "http://example.com:80/",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ],
+        "policy_chain": [
+          {
+            "name": "apicast.policy.url_rewriting",
+            "configuration": {
+              "commands": [
+                { "op": "gsub", "regex": "/original", "replace": "/rewritten" }
+              ]
+            }
+          },
+          {
+            "name": "apicast.policy.upstream",
+            "configuration": {
+              "rules": [ { "regex": "/rewritten", "url": "http://test:$TEST_NGINX_SERVER_PORT" } ]
+            }
+          },
+          { "name": "apicast.policy.apicast" }
+        ]
+      }
+    }
+  ]
+}
+--- upstream
+  location /rewritten {
+     content_by_lua_block {
+       require('luassert').are.equal('GET /rewritten?user_key=uk&a_param=a_value HTTP/1.1',
+                                     ngx.var.request)
+       ngx.say('yay, api backend');
+     }
+  }
+--- request
+GET /original?user_key=uk&a_param=a_value
+--- response_body
+yay, api backend
+--- error_code: 200
+--- no_error_log
+[error]