Skip to content

feat(balancer) recreate request to update Host header #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat(balancer) recreate request to update Host header #13

wants to merge 1 commit into from

Conversation

locao
Copy link

@locao locao commented May 13, 2020

No description provided.

@locao locao requested review from dndx and fffonion May 13, 2020 17:59
@hutchic
Copy link

hutchic commented May 14, 2020

will these changes be compatible with past versions of Kong?

@locao locao force-pushed the feat/update_host_header_on_retry branch 2 times, most recently from 989d0a7 to 4bc3951 Compare May 14, 2020 23:06
fffonion
fffonion reviewed May 15, 2020
View reviewed changes
bungle
bungle reviewed May 15, 2020
View reviewed changes
lualib/resty/kong/balancer.lua Outdated
Comment on lines 18 to 37
local get_request
do
local ok, exdata = pcall(require, "thread.exdata")
if ok and exdata then
function get_request()
local r = exdata()
if r ~= nil then
return r
end
end

else
local getfenv = getfenv

function get_request()
return getfenv(0).__ngx_req
end
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just:

local get_request = require "resty.core.base".get_request

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, we don't need to support the 1.13.6 method anymore. 1.15.8 always comes with the get_request method.

dndx
dndx requested changes May 15, 2020
View reviewed changes
lualib/resty/kong/balancer.lua Outdated
Comment on lines 18 to 37
local get_request
do
local ok, exdata = pcall(require, "thread.exdata")
if ok and exdata then
function get_request()
local r = exdata()
if r ~= nil then
return r
end
end

else
local getfenv = getfenv

function get_request()
return getfenv(0).__ngx_req
end
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, we don't need to support the 1.13.6 method anymore. 1.15.8 always comes with the get_request method.

lualib/resty/kong/balancer.lua


function _M.update_host_header()
if ngx.config.subsystem == "http" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to make this function disappear in stream module instead of a no-op to make sure the caller errors out. Basically move the if one line higher.

src/ngx_http_lua_kong_module.c Outdated
@@ -63,6 +63,95 @@ ngx_module_t ngx_http_lua_kong_module = {
};


// private data structures from ngx_http_proxy_module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for Cisco PoC, but we should patch ngx_http_proxy_module instead of copying all the data structure from it. This could break easily with an Nginx upgrade.

src/ngx_http_lua_kong_module.c Outdated
ngx_int_t
ngx_http_lua_kong_ffi_recreate_request(ngx_http_request_t *r)
{
ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "entering here");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This need to be removed.

src/ngx_http_lua_kong_module.c Outdated
return NGX_ERROR;
}

cl = ngx_alloc_chain_link(r->pool);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we do not have to reallocate a new chain link it seems? Simply overriding the old chain link's buf pointer should be enough.

src/ngx_http_lua_kong_module.c Outdated

*e.pos++ = ':'; *e.pos++ = ' ';

if (ngx_strstr(e.pos - ngx_strlen("Host: "), "Host: ") != NULL &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ngx_strlen("Host: ") should be changed to sizeof("Host: ") -1 to avoid runtime penalties.

Also, should we check for lowercased version as well? Maybe use ngx_strcasestrn instead.

src/ngx_http_lua_kong_module.c Outdated
// run the script
while (*(uintptr_t *) e.ip) {
code = *(ngx_http_script_code_pt *) e.ip;
code((ngx_http_script_engine_t *) &e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we really need this logic? If proxy_set_header Host $upstream_host; is set inside the nginx.conf, can we simply set ngx.var.upstream_host = "new.host" inside balancer phase for the script engine to take care of the rest?

If it is caused by weird caching script engine does, we can always clear the cache before running the length and content codes below. That way we can guarantee the length calculated is always the same as the string written and avoid the buffer mangling below.

@dndx
Copy link
Member

dndx commented May 15, 2020

My recommendations:

Overall the change is going the right direction. However, the duplication of code from ngx_http_proxy_module.c is not ideal. Since the functions are almost identical to those of the proxy_module, maybe we should:

  1. Introduce a patch file to Nginx to make ngx_http_proxy_create_request public so this module can call it.
  2. Make any necessary change to ngx_http_proxy_create_request to make sure it can be safely called multiple times. This could includes adding additional parameters to the function. Since it is a private method in Nginx, nobody except the code inside ngx_http_proxy_module.c will be affected by it.
  3. We should aim to make minimum changes to the ngx_http_proxy_create_request itself. If the Host header can be re-rendered from a variable, it should not be manipulated manually with string manipulation functions.

This way we should be able to get rid of majority of the copied code inside this PR, which makes the PR much more reviewable and maintainable, but most importantly, remain safe across Nginx updates.

@locao locao force-pushed the feat/update_host_header_on_retry branch from 4bc3951 to 23d2541 Compare May 18, 2020 15:56
@dndx dndx mentioned this pull request Jun 23, 2020
Merged
@dndx
Copy link
Member

dndx commented Jul 21, 2020

Superseded by openresty/lua-resty-core#302.

@dndx dndx closed this Jul 21, 2020
@dndx dndx deleted the feat/update_host_header_on_retry branch July 21, 2020 15:35
@dndx dndx restored the feat/update_host_header_on_retry branch July 21, 2020 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bungle bungle bungle left review comments

@fffonion fffonion fffonion left review comments

@dndx dndx dndx requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@locao @hutchic @dndx @bungle @fffonion