Merge pull request Kong#496 from Mashape/hotfix/path-resolver-queryst…

…ring

[hotfix/path resolver] not matching if querystring

Former-commit-id: 36a6092dbd95b8f9db0ae194b76ff3143dba15a5

kong/resolver/access.lua

@@ -29,7 +29,7 @@ local function get_backend_url(api)
   local len = string.len(result)
   if string.sub(result, len, len) == "/" then
     -- Remove one slash to avoid having a double slash
-    -- Because ngx.var.request_uri always starts with a slash
+    -- Because ngx.var.uri always starts with a slash
     result = string.sub(result, 0, len - 1)
   end
 
@@ -120,25 +120,25 @@ end
 
 -- To do so, we have to compare entire URI segments (delimited by "/").
 -- Comparing by entire segment allows us to avoid edge-cases such as:
--- request_uri = /mockbin-with-pattern/xyz
+-- uri = /mockbin-with-pattern/xyz
 -- api.path regex = ^/mockbin
 -- ^ This would wrongfully match. Wether:
 -- api.path regex = ^/mockbin/
 -- ^ This does not match.
 
 -- Because we need to compare by entire URI segments, all URIs need to have a trailing slash, otherwise:
--- request_uri = /mockbin
+-- uri = /mockbin
 -- api.path regex = ^/mockbin/
 -- ^ This would not match.
--- @param  `request_uri` The URI for this request.
+-- @param  `uri` The URI for this request.
 -- @param  `path_arr`    An array of all APIs that have a path property.
-function _M.find_api_by_path(request_uri, path_arr)
-  if not stringy.endswith(request_uri, "/") then
-    request_uri = request_uri.."/"
+function _M.find_api_by_path(uri, path_arr)
+  if not stringy.endswith(uri, "/") then
+    uri = uri.."/"
   end
 
   for _, item in ipairs(path_arr) do
-    local m, err = ngx.re.match(request_uri, "^"..item.path.."/")
+    local m, err = ngx.re.match(uri, "^"..item.path.."/")
     if err then
       ngx.log(ngx.ERR, "[resolver] error matching requested path: "..err)
     elseif m then
@@ -149,28 +149,28 @@ end
 
 -- Replace `/path` with `path`, and then prefix with a `/`
 -- or replace `/path/foo` with `/foo`, and then do not prefix with `/`.
-function _M.strip_path(request_uri, strip_path_pattern)
-  local uri = string.gsub(request_uri, strip_path_pattern, "", 1)
+function _M.strip_path(uri, strip_path_pattern)
+  local uri = string.gsub(uri, strip_path_pattern, "", 1)
   if string.sub(uri, 0, 1) ~= "/" then
     uri = "/"..uri
   end
   return uri
 end
 
 -- Find an API from a request made to nginx. Either from one of the Host or X-Host-Override headers
--- matching the API's `public_dns`, either from the `request_uri` matching the API's `path`.
+-- matching the API's `public_dns`, either from the `uri` matching the API's `path`.
 --
--- To perform this, we need to query _ALL_ APIs in memory. It is the only way to compare the `request_uri`
+-- To perform this, we need to query _ALL_ APIs in memory. It is the only way to compare the `uri`
 -- as a regex to the values set in DB, as well as matching wildcard dns.
 -- We keep APIs in the database cache for a longer time than usual.
 -- @see https://github.com/Mashape/kong/issues/15 for an improvement on this.
 --
--- @param  `request_uri` The URI for this request.
+-- @param  `uri` The URI for this request.
 -- @return `err`         Any error encountered during the retrieval.
 -- @return `api`         The retrieved API, if any.
 -- @return `hosts`       The list of headers values found in Host and X-Host-Override.
 -- @return `strip_path_pattern` If the API was retrieved by path, contain the pattern to strip it from the URI.
-local function find_api(request_uri)
+local function find_api(uri)
   local api, all_hosts, strip_path_pattern
 
   -- Retrieve all APIs
@@ -188,31 +188,31 @@ local function find_api(request_uri)
   end
 
   -- Otherwise, we look for it by path. We have to loop over all APIs and compare the requested URI.
-  api, strip_path_pattern = _M.find_api_by_path(request_uri, apis_dics.path_arr)
+  api, strip_path_pattern = _M.find_api_by_path(uri, apis_dics.path_arr)
 
   return nil, api, all_hosts, strip_path_pattern
 end
 
 function _M.execute(conf)
-  local request_uri = ngx.var.request_uri
-  local err, api, hosts, strip_path_pattern = find_api(request_uri)
+  local uri = ngx.var.uri
+  local err, api, hosts, strip_path_pattern = find_api(uri)
   if err then
     return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
   elseif not api then
     return responses.send_HTTP_NOT_FOUND {
       message = "API not found with these values",
       public_dns = hosts,
-      path = request_uri
+      path = uri
     }
   end
 
   -- If API was retrieved by path and the path needs to be stripped
   if strip_path_pattern and api.strip_path then
-    request_uri = _M.strip_path(request_uri, strip_path_pattern)
+    uri = _M.strip_path(uri, strip_path_pattern)
   end
 
   -- Setting the backend URL for the proxy_pass directive
-  ngx.var.backend_url = get_backend_url(api)..request_uri
+  ngx.var.backend_url = get_backend_url(api)..uri
   if api.preserve_host then
     ngx.var.backend_host = ngx.req.get_headers()["host"]
   else

spec/integration/proxy/api_resolver_spec.lua

@@ -108,7 +108,6 @@ describe("Resolver", function()
 
   describe("Existing API", function()
     describe("By Host", function()
-
       it("should proxy when the API is in Kong", function()
         local _, status = http_client.get(STUB_GET_URL, nil, {host = "mockbin.com"})
         assert.equal(200, status)
@@ -138,12 +137,10 @@ describe("Resolver", function()
           _, status = http_client.get(STUB_GET_URL, nil, {host = "wildcard.org"})
           assert.equal(201, status)
         end)
-
       end)
     end)
 
     describe("By Path", function()
-
       it("should proxy when no Host is present but the request_uri matches the API's path", function()
         local _, status = http_client.get(spec_helper.PROXY_URL.."/status/200")
         assert.equal(200, status)
@@ -154,42 +151,40 @@ describe("Resolver", function()
         local _, status = http_client.get(spec_helper.PROXY_URL.."/mockbin")
         assert.equal(200, status)
       end)
-
       it("should not proxy when the path does not match the start of the request_uri", function()
         local response, status = http_client.get(spec_helper.PROXY_URL.."/somepath/status/200")
         local body = cjson.decode(response)
         assert.equal("API not found with these values", body.message)
         assert.equal("/somepath/status/200", body.path)
         assert.equal(404, status)
       end)
-
       it("should proxy and strip the path if `strip_path` is true", function()
         local response, status = http_client.get(spec_helper.PROXY_URL.."/mockbin/request")
         assert.equal(200, status)
         local body = cjson.decode(response)
         assert.equal("http://mockbin.com/request", body.url)
       end)
-
       it("should proxy and strip the path if `strip_path` is true if path has pattern characters", function()
         local response, status = http_client.get(spec_helper.PROXY_URL.."/mockbin-with-pattern/request")
         assert.equal(200, status)
         local body = cjson.decode(response)
         assert.equal("http://mockbin.com/request", body.url)
       end)
-
       it("should proxy when the path has a deep level", function()
         local _, status = http_client.get(spec_helper.PROXY_URL.."/deep/path/status/200")
         assert.equal(200, status)
       end)
-
+      it("should not care about querystring parameters", function()
+        local _, status = http_client.get(spec_helper.PROXY_URL.."/mockbin?foo=bar")
+        assert.equal(200, status)
+      end)
       it("should not strip if the `path` pattern is repeated in the request_uri", function()
         local response, status = http_client.get(spec_helper.PROXY_URL.."/har/har/of/request")
         assert.equal(200, status)
         local body = cjson.decode(response)
         local upstream_url = body.log.entries[1].request.url
         assert.equal("http://mockbin.com/har/of/request", upstream_url)
       end)
-
     end)
 
     it("should return the correct Server and Via headers when the request was proxied", function()
@@ -198,18 +193,15 @@ describe("Resolver", function()
       assert.equal("cloudflare-nginx", headers.server)
       assert.equal(constants.NAME.."/"..constants.VERSION, headers.via)
     end)
-
     it("should return the correct Server and no Via header when the request was NOT proxied", function()
       local _, status, headers = http_client.get(STUB_GET_URL, nil, { host = "mockbin-auth.com"})
       assert.equal(401, status)
       assert.equal(constants.NAME.."/"..constants.VERSION, headers.server)
       assert.falsy(headers.via)
     end)
-
   end)
 
   describe("Preseve Host", function()
-
     it("should not preserve the host (default behavior)", function()
       local response, status = http_client.get(PROXY_URL.."/get", nil, { host = "httpbin-nopreserve.com"})
       assert.equal(200, status)
@@ -223,6 +215,5 @@ describe("Resolver", function()
       local parsed_response = cjson.decode(response)
       assert.equal("httpbin-preserve.com", parsed_response.headers["Host"])
     end)
-
   end)
 end)