Skip to content

Commit

Permalink
fix(balancer) update the Host header between balancer retries
Browse files Browse the repository at this point in the history
This reverts commit 0a71c0b.

The issue has been fixed at openresty/lua-nginx-module#1770

Co-authored-by: Datong Sun <datong.sun@konghq.com>
  • Loading branch information
Murillo Paula and dndx authored Mar 23, 2021
1 parent f5e1f38 commit 28437a8
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 28 deletions.
2 changes: 1 addition & 1 deletion .requirements
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ RESTY_OPENSSL_VERSION=1.1.1j
RESTY_PCRE_VERSION=8.44
LIBYAML_VERSION=0.2.5
KONG_GO_PLUGINSERVER_VERSION=v0.6.1
KONG_BUILD_TOOLS_VERSION=4.13.0
KONG_BUILD_TOOLS_VERSION=4.14.1
8 changes: 8 additions & 0 deletions kong/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ local certificate = require "kong.runloop.certificate"
local concurrency = require "kong.concurrency"
local cache_warmup = require "kong.cache.warmup"
local balancer_execute = require("kong.runloop.balancer").execute
local balancer_set_host_header = require("kong.runloop.balancer").set_host_header
local kong_error_handlers = require "kong.error_handlers"
local migrations_utils = require "kong.cmd.utils.migrations"
local plugin_servers = require "kong.runloop.plugin_servers"
Expand Down Expand Up @@ -968,6 +969,13 @@ function Kong.balancer()
return ngx.exit(errcode)
end

ok, err = balancer_set_host_header(balancer_data)
if not ok then
ngx_log(ngx_ERR, "failed to set balancer Host header: ", err)

return ngx.exit(500)
end

else
-- first try, so set the max number of retries
local retries = balancer_data.retries
Expand Down
40 changes: 38 additions & 2 deletions kong/runloop/balancer.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ local workspaces = require "kong.workspaces"
local utils = require "kong.tools.utils"
local hooks = require "kong.hooks"
local get_certificate = require("kong.runloop.certificate").get_certificate
local recreate_request = require("ngx.balancer").recreate_request


-- due to startup/require order, cannot use the ones from 'kong' here
Expand Down Expand Up @@ -31,6 +32,7 @@ local table_concat = table.concat
local table_remove = table.remove
local timer_at = ngx.timer.at
local run_hook = hooks.run_hook
local var = ngx.var
local get_phase = ngx.get_phase


Expand Down Expand Up @@ -795,7 +797,7 @@ local get_value_to_hash = function(upstream, ctx)
(ctx.authenticated_credential or EMPTY_T).id

elseif hash_on == "ip" then
identifier = ngx.var.remote_addr
identifier = var.remote_addr

elseif hash_on == "header" then
identifier = ngx.req.get_headers()[upstream[header_field_name]]
Expand All @@ -804,7 +806,7 @@ local get_value_to_hash = function(upstream, ctx)
end

elseif hash_on == "cookie" then
identifier = ngx.var["cookie_" .. upstream.hash_on_cookie]
identifier = var["cookie_" .. upstream.hash_on_cookie]

-- If the cookie doesn't exist, create one and store in `ctx`
-- to be added to the "Set-Cookie" header in the response
Expand Down Expand Up @@ -1224,6 +1226,39 @@ local function get_upstream_health(upstream_id)
end


local function set_host_header(balancer_data)
if balancer_data.preserve_host then
return true
end

-- set the upstream host header if not `preserve_host`
local upstream_host = var.upstream_host
local orig_upstream_host = upstream_host
local phase = get_phase()

upstream_host = balancer_data.hostname

local upstream_scheme = var.upstream_scheme
if upstream_scheme == "http" and balancer_data.port ~= 80 or
upstream_scheme == "https" and balancer_data.port ~= 443 or
upstream_scheme == "grpc" and balancer_data.port ~= 80 or
upstream_scheme == "grpcs" and balancer_data.port ~= 443
then
upstream_host = upstream_host .. ":" .. balancer_data.port
end

if upstream_host ~= orig_upstream_host then
var.upstream_host = upstream_host

if phase == "balancer" then
return recreate_request()
end
end

return true
end


--------------------------------------------------------------------------------
-- Get healthcheck information for a balancer.
-- @param upstream_id the id of the upstream.
Expand Down Expand Up @@ -1295,6 +1330,7 @@ return {
get_upstream_by_id = get_upstream_by_id,
get_balancer_health = get_balancer_health,
stop_healthcheckers = stop_healthcheckers,
set_host_header = set_host_header,

-- ones below are exported for test purposes only
_create_balancer = create_balancer,
Expand Down
48 changes: 23 additions & 25 deletions kong/runloop/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -1312,6 +1312,15 @@ return {
local balancer_data = ctx.balancer_data
balancer_data.scheme = var.upstream_scheme -- COMPAT: pdk

-- The content of var.upstream_host is only set by the router if
-- preserve_host is true
--
-- We can't rely on var.upstream_host for balancer retries inside
-- `set_host_header` because it would never be empty after the first -- balancer try
if var.upstream_host ~= nil and var.upstream_host ~= "" then
balancer_data.preserve_host = true
end

local ok, err, errcode = balancer_execute(ctx)
if not ok then
local body = utils.get_default_exit_body(errcode, err)
Expand All @@ -1320,33 +1329,22 @@ return {

var.upstream_scheme = balancer_data.scheme

do
-- set the upstream host header if not `preserve_host`
local upstream_host = var.upstream_host
local upstream_scheme = var.upstream_scheme

if not upstream_host or upstream_host == "" then
upstream_host = balancer_data.hostname

if upstream_scheme == "http" and balancer_data.port ~= 80 or
upstream_scheme == "https" and balancer_data.port ~= 443 or
upstream_scheme == "grpc" and balancer_data.port ~= 80 or
upstream_scheme == "grpcs" and balancer_data.port ~= 443
then
upstream_host = upstream_host .. ":" .. balancer_data.port
end
local ok, err = balancer.set_host_header(balancer_data)
if not ok then
ngx.log(ngx.ERR, "failed to set balancer Host header: ", err)

var.upstream_host = upstream_host
end
return ngx.exit(500)
end

-- the nginx grpc module does not offer a way to overrride
-- the :authority pseudo-header; use our internal API to
-- do so
if upstream_scheme == "grpc" or upstream_scheme == "grpcs" then
ok, err = kong.service.request.set_header(":authority", upstream_host)
if not ok then
log(ERR, "failed to set :authority header: ", err)
end
-- the nginx grpc module does not offer a way to overrride the
-- :authority pseudo-header; use our internal API to do so
local upstream_host = var.upstream_host
local upstream_scheme = var.upstream_scheme

if upstream_scheme == "grpc" or upstream_scheme == "grpcs" then
ok, err = kong.service.request.set_header(":authority", upstream_host)
if not ok then
log(ERR, "failed to set :authority header: ", err)
end
end

Expand Down
132 changes: 132 additions & 0 deletions spec/02-integration/05-proxy/10-balancer/05-recreate-request_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
local bu = require "spec.fixtures.balancer_utils"
local helpers = require "spec.helpers"

for _, strategy in helpers.each_strategy() do
describe("Balancing with round-robin #" .. strategy, function()
local bp, proxy_client

lazy_setup(function()
bp = bu.get_db_utils_for_dc_and_admin_api(strategy, {
"routes",
"services",
"plugins",
"upstreams",
"targets",
})

local fixtures = {
http_mock = {
least_connections = [[
server {
listen 10001;
location ~ "/recreate_test" {
content_by_lua_block {
ngx.sleep(700)
ngx.exit(ngx.OK)
}
}
}
server {
listen 10002;
location ~ "/recreate_test" {
content_by_lua_block {
ngx.say("host is: ", ngx.var.http_host)
ngx.exit(ngx.OK)
}
}
}
]]
},
dns_mock = helpers.dns_mock.new()
}

fixtures.dns_mock:A {
name = "upstream.example.com",
address = "127.0.0.1",
}

assert(helpers.start_kong({
database = strategy,
nginx_conf = "spec/fixtures/custom_nginx.template",
worker_state_update_frequency = bu.CONSISTENCY_FREQ,
}, nil, nil, fixtures))

end)

lazy_teardown(function()
helpers.stop_kong()
end)

before_each(function()
proxy_client = helpers.proxy_client()
end)

after_each(function()
if proxy_client then
proxy_client:close()
end
end)


it("balancer retry updates Host header in request buffer", function()
bu.begin_testcase_setup(strategy, bp)
local upstream_name, upstream_id = bu.add_upstream(bp)
bu.add_target(bp, upstream_id, "upstream.example.com", 10001) -- this will timeout
bu.add_target(bp, upstream_id, "upstream.example.com", 10002)

local service = assert(bp.services:insert({
url = "http://" .. upstream_name,
read_timeout = 500,
}))

bp.routes:insert({
service = { id = service.id },
paths = { "/", },
})
bu.end_testcase_setup(strategy, bp, "strict")

local res = assert(proxy_client:send {
method = "GET",
path = "/recreate_test",
})

local body = assert.response(res).has_status(200)
assert.equal("host is: upstream.example.com:10002", body)
end)

it("balancer retry doesn't update Host if preserve_host is true", function()
bu.begin_testcase_setup(strategy, bp)
local upstream_name, upstream_id = bu.add_upstream(bp)
bu.add_target(bp, upstream_id, "upstream.example.com", 10001) -- this will timeout
bu.add_target(bp, upstream_id, "upstream.example.com", 10002)

local service = assert(bp.services:insert({
url = "http://" .. upstream_name,
read_timeout = 500,
}))

bp.routes:insert({
service = { id = service.id },
preserve_host = true,
paths = { "/", },
hosts = { "test.com" }
})
bu.end_testcase_setup(strategy, bp, "strict")

local res = assert(proxy_client:send {
method = "GET",
path = "/recreate_test",
headers = { ["Host"] = "test.com" },
})

local body = assert.response(res).has_status(200)
assert.equal("host is: test.com", body)
end)
end)
end

0 comments on commit 28437a8

Please sign in to comment.