Fix/routing without Host header #3216
Conversation
If clients do not send a Host header (valid in HTTP/1.0), ngx.var.http_host is nil. Kong rejects this when routing to an API. However, only one of uris, hosts, or methods is required when adding an API. This change allows routing any HTTP/1.0 request with an appropriate method or URI to the corresponding API. Fix #3171
Tests that a request without a host header can route to an API if it matches other criteria. Fix #3171
Tests that an HTTP/1.0 request without the Host header is accepted and routes to an API matching its method. Fix #3171
There was a problem hiding this comment.
Thank you for the patch!
Would you mind addressing the few concerns I noted? Mostly: adding a test for NGINX's behavior with HTTP/1.1 and reworking a little bit the internals of the provided test to better follow Lua idioms (already followed by this codebase)
Thanks!
spec/01-unit/010-router_spec.lua
Outdated
| @@ -100,6 +100,13 @@ describe("Router", function() | |||
| assert.same(use_case[3], match_t.api) | |||
| end) | |||
|
|
|||
| it("[uri]", function() | |||
There was a problem hiding this comment.
could we rename this test to: [uri + empty host]?
| @@ -134,6 +134,26 @@ describe("Router", function() | |||
| assert.equal("api-1", res.headers["kong-api-name"]) | |||
| end) | |||
|
|
|||
| it("routes by method-only if no Host header present", function() | |||
There was a problem hiding this comment.
Would it be possible to create a new describe block for empty/no Host header tests? Like so:
describe("requests with no Host header", function()
it("HTTP/1.0 routes normally", function()
end)
it("HTTP/1.1 is rejected by NGINX", function()
-- new test: assert NGINX's behavior with HTTP/1.1 (missing from this PR)
end)
end)| -- based on the source of https://github.com/pintsized/lua-resty-http | ||
| local sock = ngx.socket.tcp() | ||
| finally(function() sock:close() end) | ||
| local ok, err = sock:connect(helpers.test_conf.proxy_ip, |
There was a problem hiding this comment.
style: it is nice to see the effort to stay under 80 chars. As per CONTRIBUTING.md, the preferred way of doing so is to align subsequent arguments right under the first one :)
| finally(function() sock:close() end) | ||
| local ok, err = sock:connect(helpers.test_conf.proxy_ip, | ||
| helpers.test_conf.proxy_port) | ||
| assert(ok, err) |
There was a problem hiding this comment.
The Lua idiom is to directly do:
assert(sock:connect(...))| helpers.test_conf.proxy_port) | ||
| assert(ok, err) | ||
| local req = "GET /get HTTP/1.0\r\nKong-Debug: 1\r\n\r\n" | ||
| sock:send(req) |
There was a problem hiding this comment.
We should also check for errors here
| local req = "GET /get HTTP/1.0\r\nKong-Debug: 1\r\n\r\n" | ||
| sock:send(req) | ||
| local line, err = sock:receive("*l") | ||
| assert(line, err) |
There was a problem hiding this comment.
In this case, the idiom becomes:
local line = assert(sock:receive("*l"))| local line, err = sock:receive("*l") | ||
| assert(line, err) | ||
| local status = tonumber(string.sub(line, 10, 12)) | ||
| assert.equal(status, 200) |
There was a problem hiding this comment.
The arguments need to be reversed here: it will produce a confusing error message if this fails (expected values come first in luassert, which is the library providing those extended assertions)
| local status = tonumber(string.sub(line, 10, 12)) | ||
| assert.equal(status, 200) | ||
| local remainder, err = sock:receive("*a") | ||
| assert(remainder, err) |
There was a problem hiding this comment.
ditto: better use the proper lua idioms
| local remainder, err = sock:receive("*a") | ||
| assert(remainder, err) | ||
| assert(string.find(string.lower(remainder), | ||
| "kong-api-name: api-1", 1, true)) |
There was a problem hiding this comment.
Here, we can use luassert's extended assertions too:
assert.matches("kong-api-name: api-1", string.lower(remainder), nil, true)
| @@ -831,7 +831,7 @@ function _M.new(apis) | |||
| end | |||
| end | |||
|
|
|||
| local req_host = ngx.var.http_host | |||
| local req_host = ngx.var.http_host or "" | |||
|
|
|||
There was a problem hiding this comment.
Should we allow this bevahior only when the request protocol is HTTP/1.0? The Host header is required for 1.1 requests; i would proffer that Kong should require 1.1 clients to follow the spec and not accept invalid requests.
There was a problem hiding this comment.
Yes, the goal of this patch is to support this behavior only for HTTP/1.0 (as discussed with @rainest). There is probably nothing else to add in the router since NGINX should disallow HTTP/1.1 requests without the Host head already (hence why the ask for the test so we avoid a future regression). We could also add a check in the router but it seems unnecessary to me...
HTTP/1.1 requires the Host header per RFC 2616. NGINX is expected to return status 4400 for requests lacking it.
|
Thanks! Merged to master with some modifications (including asserting that the NGINX error is caught by Kong itself). Good job! |
Summary
Allows Kong to accept HTTP/1.0 requests that do not contain a Host header
Full changelog
Issues resolved
Fix #3171