-
Notifications
You must be signed in to change notification settings - Fork 170
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.
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
[THREESCALE-9542] Part 2: Add support to proxy request with Transfer-Encoding: chunked #1403
Changes from 2 commits
3efa69a
b02a888
e3f6db5
f5af618
5e05612
2bfc227
f19b9e5
b494ca8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,30 @@ | ||
local format = string.format | ||
local tostring = tostring | ||
local ngx_flush = ngx.flush | ||
local ngx_get_method = ngx.req.get_method | ||
local ngx_http_version = ngx.req.http_version | ||
local ngx_send_headers = ngx.send_headers | ||
|
||
local resty_url = require "resty.url" | ||
local resty_resolver = require 'resty.resolver' | ||
local round_robin = require 'resty.balancer.round_robin' | ||
local http_proxy = require 'resty.http.proxy' | ||
local file_reader = require("resty.file").file_reader | ||
local file_size = require("resty.file").file_size | ||
local client_body_reader = require("resty.http.request_reader").get_client_body_reader | ||
local send_response = require("resty.http.response_writer").send_response | ||
local concat = table.concat | ||
|
||
local _M = { } | ||
|
||
local http_methods_with_body = { | ||
POST = true, | ||
PUT = true, | ||
PATCH = true | ||
} | ||
|
||
local DEFAULT_CHUNKSIZE = 32 * 1024 | ||
|
||
function _M.reset() | ||
_M.balancer = round_robin.new() | ||
_M.resolver = resty_resolver | ||
|
@@ -82,15 +98,61 @@ | |
) | ||
end | ||
|
||
local function forward_https_request(proxy_uri, proxy_auth, uri, skip_https_connect) | ||
-- This is needed to call ngx.req.get_body_data() below. | ||
ngx.req.read_body() | ||
local function handle_expect() | ||
local expect = ngx.req.get_headers()["Expect"] | ||
if type(expect) == "table" then | ||
expect = expect[1] | ||
end | ||
|
||
local request = { | ||
uri = uri, | ||
method = ngx.req.get_method(), | ||
headers = ngx.req.get_headers(0, true), | ||
path = format('%s%s%s', ngx.var.uri, ngx.var.is_args, ngx.var.query_string or ''), | ||
if expect and expect:lower() == "100-continue" then | ||
ngx.status = 100 | ||
local ok, err = ngx_send_headers() | ||
|
||
if not ok then | ||
return nil, "failed to send response header: " .. (err or "unknown") | ||
end | ||
|
||
ok, err = ngx_flush(true) | ||
if not ok then | ||
return nil, "failed to flush response header: " .. (err or "unknown") | ||
end | ||
end | ||
end | ||
|
||
local function forward_https_request(proxy_uri, uri, proxy_opts) | ||
local body, err | ||
local sock | ||
local opts = proxy_opts or {} | ||
local req_method = ngx_get_method() | ||
local encoding = ngx.req.get_headers()["Transfer-Encoding"] | ||
local is_chunked = encoding and encoding:lower() == "chunked" | ||
|
||
if http_methods_with_body[req_method] then | ||
if opts.request_unbuffered and ngx_http_version() == 1.1 then | ||
local _, err = handle_expect() | ||
if err then | ||
ngx.log(ngx.ERR, "failed to handle expect header, err: ", err) | ||
return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
end | ||
|
||
if is_chunked then | ||
-- The default ngx reader does not support chunked request | ||
-- so we will need to get the raw request socket and manually | ||
-- decode the chunked request | ||
sock, err = ngx.req.socket(true) | ||
else | ||
sock, err = ngx.req.socket() | ||
end | ||
|
||
if not sock then | ||
ngx.log(ngx.ERR, "unable to obtain request socket: ", err) | ||
return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
end | ||
|
||
body = client_body_reader(sock, DEFAULT_CHUNKSIZE, is_chunked) | ||
else | ||
-- This is needed to call ngx.req.get_body_data() below. | ||
ngx.req.read_body() | ||
|
||
-- We cannot use resty.http's .get_client_body_reader(). | ||
-- In POST requests with HTTPS, the result of that call is nil, and it | ||
|
@@ -101,26 +163,53 @@ | |
-- read and need to be cached in a local file. This request will return | ||
-- nil, so after this we need to read the temp file. | ||
-- https://github.com/openresty/lua-nginx-module#ngxreqget_body_data | ||
body = ngx.req.get_body_data(), | ||
proxy_uri = proxy_uri, | ||
proxy_auth = proxy_auth | ||
} | ||
body = ngx.req.get_body_data() | ||
|
||
if not body then | ||
local temp_file_path = ngx.req.get_body_file() | ||
ngx.log(ngx.INFO, "HTTPS Proxy: Request body is bigger than client_body_buffer_size, read the content from path='", temp_file_path, "'") | ||
|
||
if temp_file_path then | ||
body, err = file_reader(temp_file_path) | ||
if err then | ||
ngx.log(ngx.ERR, "HTTPS proxy: Failed to read temp body file, err: ", err) | ||
ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
end | ||
|
||
if is_chunked then | ||
-- If the body is smaller than "client_boby_buffer_size" the Content-Length header is | ||
-- set based on the size of the buffer. However, when the body is rendered to a file, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Who is doing that? In other words, when all the conditions meet:
Who sets the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The lua-resty-http will set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see, It's because it is a string and the resty-http gets the length out of it. It happens here. I would make it explicit, but good enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree because this is something that will come up again in future when troubleshooting but it doesn't need to be done in this PR, can be added at a later date that |
||
-- we will need to calculate and manually set the Content-Length header based on the | ||
-- file size | ||
local contentLength, err = file_size(temp_file_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this safe to do for ALL requests which meet these conditions? I see that the calls in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it enough to wrap that functionality with a coroutine? I don't know how useful that would be since it would yield on the first call anyway. Also the file_reader also call But I totally agree with you that it is a I/O blocking function and should be avoided. Checking the module
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No sure how expensive is function fsize (filename)
local handle, err = open(filename)
local current = handle:seek() -- get current position
local size = handle:seek("end") -- get file size
handle:seek("set", current) -- restore position
return size
end Theoretically any IO operation could block the thread. We could try coroutines or any other mean to make it non blocking. Reading lua-nginx-module introduction it says:
Not sure if we can follow that recommendation, though. @tkan145 we can try to discuss about this in a short call.. Anyway, AFAIK, we have never measured the capacity of APICast to handle traffic with request body big enough to be persisted in disk. All the tests performed where "simple" GET requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed and also updated README file. @kevprice83 can you help review the README file and let me know if I need to add anything else? |
||
if err then | ||
ngx.log(ngx.ERR, "HTTPS proxy: Failed to set content length, err: ", err) | ||
ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
end | ||
|
||
ngx.req.set_header("Content-Length", tostring(contentLength)) | ||
end | ||
end | ||
end | ||
|
||
if not request.body then | ||
local temp_file_path = ngx.req.get_body_file() | ||
ngx.log(ngx.INFO, "HTTPS Proxy: Request body is bigger than client_body_buffer_size, read the content from path='", temp_file_path, "'") | ||
|
||
if temp_file_path then | ||
local body, err = file_reader(temp_file_path) | ||
if err then | ||
ngx.log(ngx.ERR, "HTTPS proxy: Failed to read temp body file, err: ", err) | ||
ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) | ||
end | ||
request.body = body | ||
-- The whole request is buffered in the memory so remove the Transfer-Encoding: chunked | ||
if is_chunked then | ||
ngx.req.set_header("Transfer-Encoding", nil) | ||
end | ||
end | ||
end | ||
|
||
local httpc, err = http_proxy.new(request, skip_https_connect) | ||
local request = { | ||
uri = uri, | ||
method = ngx.req.get_method(), | ||
headers = ngx.req.get_headers(0, true), | ||
path = format('%s%s%s', ngx.var.uri, ngx.var.is_args, ngx.var.query_string or ''), | ||
body = body, | ||
proxy_uri = proxy_uri, | ||
proxy_auth = opts.proxy_auth | ||
} | ||
|
||
local httpc, err = http_proxy.new(request, opts.skip_https_connect) | ||
|
||
if not httpc then | ||
ngx.log(ngx.ERR, 'could not connect to proxy: ', proxy_uri, ' err: ', err) | ||
|
@@ -132,8 +221,16 @@ | |
res, err = httpc:request(request) | ||
|
||
if res then | ||
httpc:proxy_response(res) | ||
httpc:set_keepalive() | ||
if opts.request_unbuffered and is_chunked then | ||
eguzki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
local bytes, err = send_response(sock, res, DEFAULT_CHUNKSIZE) | ||
if not bytes then | ||
ngx.log(ngx.ERR, "failed to send response: ", err) | ||
return sock:send("HTTP/1.1 502 Bad Gateway") | ||
end | ||
else | ||
httpc:proxy_response(res) | ||
httpc:set_keepalive() | ||
end | ||
else | ||
ngx.log(ngx.ERR, 'failed to proxy request to: ', proxy_uri, ' err : ', err) | ||
return ngx.exit(ngx.HTTP_BAD_GATEWAY) | ||
|
@@ -186,7 +283,13 @@ | |
return | ||
elseif uri.scheme == 'https' then | ||
upstream:rewrite_request() | ||
forward_https_request(proxy_uri, proxy_auth, uri, upstream.skip_https_connect) | ||
local proxy_opts = { | ||
proxy_auth = proxy_auth, | ||
skip_https_connect = upstream.skip_https_connect, | ||
request_unbuffered = upstream.request_unbuffered | ||
} | ||
|
||
forward_https_request(proxy_uri, uri, proxy_opts) | ||
return ngx.exit(ngx.OK) -- terminate phase | ||
else | ||
ngx.log(ngx.ERR, 'could not connect to proxy: ', proxy_uri, ' err: ', 'invalid request scheme') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
local httpc = require "resty.resolver.http" | ||
|
||
local _M = { | ||
} | ||
|
||
local cr_lf = "\r\n" | ||
|
||
-- chunked_reader return a body reader that translates the data read from | ||
-- lua-resty-http client_body_reader to HTTP "chunked" format before returning it | ||
-- | ||
-- The chunked reader return nil when the final 0-length chunk is read | ||
local function chunked_reader(sock, chunksize) | ||
chunksize = chunksize or 65536 | ||
local eof = false | ||
local reader = httpc:get_client_body_reader(chunksize, sock) | ||
eguzki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if not reader then | ||
return nil | ||
end | ||
|
||
return function() | ||
if eof then | ||
return nil | ||
end | ||
|
||
local buffer, err = reader() | ||
if err then | ||
return nil, err | ||
end | ||
if buffer then | ||
local chunk = string.format("%x\r\n", #buffer) .. buffer .. cr_lf | ||
return chunk | ||
else | ||
eof = true | ||
return "0\r\n\r\n" | ||
end | ||
end | ||
end | ||
|
||
function _M.get_client_body_reader(sock, chunksize, is_chunked) | ||
if is_chunked then | ||
return chunked_reader(sock, chunksize) | ||
else | ||
return httpc:get_client_body_reader(chunksize, sock) | ||
end | ||
end | ||
|
||
return _M |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
local _M = { | ||
} | ||
|
||
local cr_lf = "\r\n" | ||
|
||
local function send(socket, data) | ||
if not data or data == '' then | ||
ngx.log(ngx.DEBUG, 'skipping sending nil') | ||
return | ||
end | ||
|
||
return socket:send(data) | ||
end | ||
|
||
-- write_response writes response body reader to sock in the HTTP/1.x server response format, | ||
-- The connection is closed if send() fails or when returning a non-zero | ||
function _M.send_response(sock, response, chunksize) | ||
eguzki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
local bytes, err | ||
chunksize = chunksize or 65536 | ||
|
||
if not response then | ||
ngx.log(ngx.ERR, "no response provided") | ||
return | ||
end | ||
|
||
if not sock then | ||
return nil, "socket not initialized yet" | ||
end | ||
|
||
-- Status line | ||
local status = "HTTP/1.1 " .. response.status .. " " .. response.reason .. cr_lf | ||
bytes, err = send(sock, status) | ||
if not bytes then | ||
return nil, "failed to send status line, err: " .. (err or "unknown") | ||
end | ||
|
||
-- Write body | ||
local reader = response.body_reader | ||
repeat | ||
local chunk, read_err | ||
|
||
chunk, read_err = reader(chunksize) | ||
if read_err then | ||
return nil, "failed to read response body, err: " .. (err or "unknown") | ||
end | ||
|
||
if chunk then | ||
bytes, err = send(sock, chunk) | ||
if not bytes then | ||
return nil, "failed to send response body, err: " .. (err or "unknown") | ||
end | ||
end | ||
until not chunk | ||
|
||
return true, nil | ||
end | ||
|
||
return _M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if
Expect
needs to be handled for when buffering is enabledThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I do not think we should be doing this. The lua-resty-http lib is doing that for us. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib lua-resty-http is a client library and it handles the Expect returned from the server, while we are acting as a server here and need to process the Expect header from the client.
When I sent a large payload using cURL, the request hung, I later found out it was due to the Expect header.
I will run some more tests to see whether we really need it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think I understand now.
I think that when
buffered
is on, APIcast should protect upstream and should handle theExpect: 100-Continue
. That is, it is the apicast who returns HTTP Response100 Continue
and then consumes the body before opening the connection to upstream. I think this is how it works right now inmaster
. The requestExpect: 100-Continue
and response100 Continue
happens twice. First time between downstream and then between apicast and upstream (done by lua resty http lib because the Expect header is still there).We might consider removing the expect header on "buffered" mode. Unless we want to keep the Expect protocol with upstream to avoid sending the body if upstream does not want to. Which also makes sense to me.It is actually a requirement from rfc2616#section-8.2.3 to be like this. CheckRequirements for HTTP/1.1 proxies:
section.When unbuffered is on, APIcast does not read the body with
ngx.req.read_body()
, thus, it does not send100 Continue
to downstream. I think that is the reason you saw the request hung. Ideally, I think that we should let upstream to decide if it wants to continue or not, and propagate the response to downstream. Downstream would start sending the body only when upstream tells to do that. I think it is quite hard to implement that. Basically because the lua resty http lib consumes the 100 Continue response of the upstream and then tries to send the body. I do not see a way to do this, other than sending manually the 100 Continue response to the downstream and create a body reader that will be consumed by the lua resty http library. But I can see some issues in there as well. What if upstream says 302 redirect or 400 bad request instead of 100 Continue? The downstream client would have already write the body in the downstream socket and that socket would be unusable for following up HTTP sessions. I do not know how to proceed regarding this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I heave re-written the message above. In case you have read it previosly, please re-read it again 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused here. I haven't read the openresty code but do you mean
ngx.req.read_body()
will send100 Continue
downstream? Doesn't that also mean that APIcast returns100 Continue
to the downstream application before establishing the upstream connection?Regarding the 400, please correct me if I'm wrong, but I think the only case where the upstream server returns this error is if there is data in the request body. In my head the flow will be as follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Exactly (when buffered mode is on)
400 Bad Request is just an example. It could be 5XX error as well. In unbuffered mode, the workflow would be as follows (in my head)
So let's say that upstream does not want it to start upload:
My issue with this is that the client has sent the body and nobody has consumed it. I need to try this scenario to see what we can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this nginx thread https://mailman.nginx.org/pipermail/nginx/2021-May/060643.html. I think nginx does not handle this well either
How about we send back error response, discard the body and close the socket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's aggressive, but can be a way out.