refactor(router) simplify URI stripping logic

kong/core/handler.lua

@@ -20,6 +20,8 @@ local balancer_execute = require("kong.core.balancer").execute
 
 local router, router_err
 local ngx_now = ngx.now
+local sub = string.sub
+
 local server_header = _KONG._NAME.."/".._KONG._VERSION
 
 
@@ -119,7 +121,6 @@ return {
       var.upstream_scheme = upstream.scheme
 
       ctx.api              = api
-      ctx.uri              = uri
       ctx.balancer_address = balancer_address
 
       local ok, err = balancer_execute(balancer_address)
@@ -129,7 +130,12 @@ return {
           "' with: "..tostring(err))
       end
 
-      -- if set `host_header` is the original header to be preserved
+      -- `uri` is the URI with which to call upstream, as returned by the
+      -- router, which might have truncated it (`strip_uri`).
+      --
+      -- If set `host_header` is the original header to be preserved.
+
+      var.upstream_uri  = uri
       var.upstream_host = host_header or
           balancer_address.hostname..":"..balancer_address.port
 
@@ -139,15 +145,19 @@ return {
       local ctx = ngx.ctx
       local var = ngx.var
 
-      local uri      = ctx.uri
-      local uri_args = var.args
+      do
+        -- Nginx's behavior when proxying a request with an empty querystring
+        -- `/foo?` is to keep `$is_args` an empty string, hence effectively
+        -- stripping the empty querystring.
+        -- We overcome this behavior with our own logic, to prevent user
+        -- desired semantics.
+        local upstream_uri = var.upstream_uri
 
-      if uri_args then
-        uri = uri .. "?" .. uri_args
+        if var.is_args == "?" or sub(var.request_uri, -1) == "?" then
+          var.upstream_uri = upstream_uri .. "?" .. (var.args or "")
+        end
       end
 
-      var.upstream_uri = uri
-
       local now = get_now()
 
       ctx.KONG_ACCESS_TIME = now - ctx.KONG_ACCESS_START -- time spent in Kong's access_by_lua

kong/core/router.lua

@@ -632,8 +632,9 @@ function _M.new(apis)
 
 
   function self.exec(ngx)
-    local method = ngx.req.get_method()
-    local uri    = ngx.var.request_uri
+    local method      = ngx.req.get_method()
+    local request_uri = ngx.var.request_uri
+    local uri         = request_uri
 
 
     do
@@ -659,16 +660,12 @@ function _M.new(apis)
       return nil
     end
 
-    local new_uri
-    local uri_root = uri == "/"
+    local uri_root = request_uri == "/"
 
-    if uri_root or not api_t.strip_uri_regex then
-      new_uri = uri
-
-    else
+    if not uri_root and api_t.strip_uri_regex then
       local _, err
-      new_uri, _, err = re_sub(uri, api_t.strip_uri_regex, "/$1", "ajo")
-      if not new_uri then
+      uri, _, err = re_sub(uri, api_t.strip_uri_regex, "/$1", "ajo")
+      if not uri then
         log(ERR, "could not strip URI: ", err)
         return
       end
@@ -677,35 +674,30 @@ function _M.new(apis)
 
     local upstream = api_t.upstream
     if upstream.path and upstream.path ~= "/" then
-      if new_uri ~= "/" then
-        new_uri = upstream.file .. new_uri
+      if uri ~= "/" then
+        uri = upstream.file .. uri
 
       else
         if upstream.path ~= upstream.file then
-          if uri_root or sub(uri, -1) == "/" then
-            new_uri = upstream.path
+          if uri_root or sub(request_uri, -1) == "/" then
+            uri = upstream.path
 
           else
-            new_uri = upstream.file
+            uri = upstream.file
           end
 
         else
-          if uri_root or sub(uri, -1) ~= "/" then
-            new_uri = upstream.file
+          if uri_root or sub(request_uri, -1) ~= "/" then
+            uri = upstream.file
 
           else
-            new_uri = upstream.file .. new_uri
+            uri = upstream.file .. uri
           end
         end
       end
     end
 
 
-    if new_uri ~= uri then
-      uri = new_uri
-    end
-
-
     local host_header
 
     if api_t.preserve_host then

spec/02-integration/05-proxy/01-router_spec.lua

@@ -124,22 +124,6 @@ describe("Router", function()
       assert.equal("api-1", res.headers["kong-api-name"])
     end)
 
-    it("preserves URI arguments", function()
-      local res = assert(client:send {
-        method = "GET",
-        path   = "/get",
-        query  = {
-          foo   = "bar",
-          hello = "world",
-        },
-      })
-
-      local body = assert.res_status(200, res)
-      local json = cjson.decode(body)
-      assert.equal("bar", json.args.foo)
-      assert.equal("world", json.args.hello)
-    end)
-
     describe("API with a path component in its upstream_url", function()
       it("with strip_uri = true", function()
         local res = assert(client:send {
@@ -165,6 +149,79 @@ describe("Router", function()
     end)
   end)
 
+  describe("URI arguments (querystring)", function()
+
+    setup(function()
+      insert_apis {
+        {
+          name = "api-1",
+          upstream_url = "http://httpbin.org",
+          hosts = { "example.com" },
+        },
+        {
+          name = "api-2",
+          upstream_url = "http://localhost:9999",
+          hosts = { "localhost" },
+        },
+      }
+
+      assert(helpers.start_kong {
+        nginx_conf = "spec/fixtures/custom_nginx.template",
+      })
+    end)
+
+    teardown(function()
+      helpers.stop_kong()
+    end)
+
+    it("preserves URI arguments", function()
+      local res = assert(client:send {
+        method = "GET",
+        path   = "/get",
+        query  = {
+          foo   = "bar",
+          hello = "world",
+        },
+        headers = {
+          ["Host"] = "example.com",
+        },
+      })
+
+      local body = assert.res_status(200, res)
+      local json = cjson.decode(body)
+      assert.equal("bar", json.args.foo)
+      assert.equal("world", json.args.hello)
+    end)
+
+    it("does proxy an empty querystring if URI does not contain arguments", function()
+      local res = assert(client:send {
+        method = "GET",
+        path   = "/get?",
+        headers = {
+          ["Host"] = "localhost",
+        },
+      })
+
+      local body = assert.res_status(200, res)
+      local json = cjson.decode(body)
+      assert.matches("/get%?$", json.vars.request_uri)
+    end)
+
+    it("does proxy a querystring with an empty value", function()
+      local res = assert(client:send {
+        method = "GET",
+        path   = "/get?hello",
+        headers = {
+          ["Host"] = "example.com",
+        },
+      })
+
+      local body = assert.res_status(200, res)
+      local json = cjson.decode(body)
+      assert.matches("/get%?hello$", json.url)
+    end)
+  end)
+
   describe("percent-encoded URIs", function()
 
     setup(function()