From 4245b9cce376141f2685b36bf173b1220dfc8222 Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Fri, 21 Aug 2015 14:51:23 -0700 Subject: [PATCH] fix(path resolver) not matching if querystring Use $uri instead of $request_uri to allow path matching even if querystring parameters are present. Fix #495 --- kong/resolver/access.lua | 40 ++++++++++---------- spec/integration/proxy/api_resolver_spec.lua | 17 ++------- 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/kong/resolver/access.lua b/kong/resolver/access.lua index 3cb27ac475b..c50094b872a 100644 --- a/kong/resolver/access.lua +++ b/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,8 +149,8 @@ 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 @@ -158,19 +158,19 @@ function _M.strip_path(request_uri, strip_path_pattern) 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 diff --git a/spec/integration/proxy/api_resolver_spec.lua b/spec/integration/proxy/api_resolver_spec.lua index ab70946803c..ddaf088b280 100644 --- a/spec/integration/proxy/api_resolver_spec.lua +++ b/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,7 +151,6 @@ 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) @@ -162,26 +158,26 @@ describe("Resolver", function() 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) @@ -189,7 +185,6 @@ describe("Resolver", function() 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)