Skip to content

Commit

Permalink
fix(grpc-web): response contains two trailer chunks (#11988)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bzp2010
bzp2010 authored Feb 25, 2025
1 parent 2d524fe commit 2a5425f
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 73 deletions.
116 changes: 69 additions & 47 deletions apisix/plugins/grpc-web.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,46 @@ function _M.check_schema(conf)
return core.schema.check(schema, conf)
end

local function exit(ctx, status)
ctx.grpc_web_skip_body_filter = true
return status
end

--- Build gRPC-Web trailer chunk
-- grpc-web trailer format reference:
-- envoyproxy/envoy/source/extensions/filters/http/grpc_web/grpc_web_filter.cc
--
-- Format for grpc-web trailer
-- 1 byte: 0x80
-- 4 bytes: length of the trailer
-- n bytes: trailer
-- It using upstream_trailer_* variables from nginx, it is available since NGINX version 1.13.10
-- https://nginx.org/en/docs/http/ngx_http_upstream_module.html#var_upstream_trailer_
--
-- @param grpc_status number grpc status code
-- @param grpc_message string grpc message
-- @return string grpc-web trailer chunk in raw string
local build_trailer = function (grpc_status, grpc_message)
local status_str = "grpc-status:" .. grpc_status
local status_msg = "grpc-message:" .. ( grpc_message or "")
local grpc_web_trailer = status_str .. "\r\n" .. status_msg .. "\r\n"
local len = #grpc_web_trailer

-- 1 byte: 0x80
local trailer_buf = string.char(0x80)
-- 4 bytes: length of the trailer
trailer_buf = trailer_buf .. string.char(
bit.band(bit.rshift(len, 24), 0xff),
bit.band(bit.rshift(len, 16), 0xff),
bit.band(bit.rshift(len, 8), 0xff),
bit.band(len, 0xff)
)
-- n bytes: trailer
trailer_buf = trailer_buf .. grpc_web_trailer

return trailer_buf
end

function _M.access(conf, ctx)
-- set context variable mime
-- When processing non gRPC Web requests, `mime` can be obtained in the context
Expand All @@ -76,19 +116,19 @@ function _M.access(conf, ctx)

local method = core.request.get_method()
if method == ALLOW_METHOD_OPTIONS then
return 204
return exit(ctx, 204)
end

if method ~= ALLOW_METHOD_POST then
-- https://github.com/grpc/grpc-web/blob/master/doc/browser-features.md#cors-support
core.log.error("request method: `", method, "` invalid")
return 400
return exit(ctx, 405)
end

local encoding = grpc_web_content_encoding[ctx.grpc_web_mime]
if not encoding then
core.log.error("request Content-Type: `", ctx.grpc_web_mime, "` invalid")
return 400
return exit(ctx, 400)
end

-- set context variable encoding method
Expand All @@ -98,7 +138,7 @@ function _M.access(conf, ctx)
if not (ctx.curr_req_matched and ctx.curr_req_matched[":ext"]) then
core.log.error("routing configuration error, grpc-web plugin only supports ",
"`prefix matching` pattern routing")
return 400
return exit(ctx, 400)
end

local path = ctx.curr_req_matched[":ext"]
Expand All @@ -110,16 +150,17 @@ function _M.access(conf, ctx)

-- set grpc body
local body, err = core.request.get_body()
if err then
if err or not body then
core.log.error("failed to read request body, err: ", err)
return 400
return exit(ctx, 400)
end

if encoding == CONTENT_ENCODING_BASE64 then
body = decode_base64(body)
ngx.log(ngx.WARN, "DECODE BODY: ", body)
if not body then
core.log.error("failed to decode request body")
return 400
return exit(ctx, 400)
end
end

Expand All @@ -139,11 +180,19 @@ function _M.header_filter(conf, ctx)
if not ctx.cors_allow_origins then
core.response.set_header("Access-Control-Allow-Origin", DEFAULT_CORS_ALLOW_ORIGIN)
end
core.response.set_header("Content-Type", ctx.grpc_web_mime)
core.response.set_header("Access-Control-Expose-Headers", DEFAULT_CORS_EXPOSE_HEADERS)

if not ctx.grpc_web_skip_body_filter then
core.response.set_header("Content-Type", ctx.grpc_web_mime)
core.response.set_header("Content-Length", nil)
end
end

function _M.body_filter(conf, ctx)
if ctx.grpc_web_skip_body_filter then
return
end

-- If the MIME extension type description of the gRPC-Web standard is not obtained,
-- indicating that the request is not based on the gRPC Web specification,
-- the processing of the request body will be ignored
Expand All @@ -159,48 +208,21 @@ function _M.body_filter(conf, ctx)
ngx_arg[1] = chunk
end

--[[
upstream_trailer_* available since NGINX version 1.13.10 :
https://nginx.org/en/docs/http/ngx_http_upstream_module.html#var_upstream_trailer_
grpc-web trailer format reference:
envoyproxy/envoy/source/extensions/filters/http/grpc_web/grpc_web_filter.cc
Format for grpc-web trailer
1 byte: 0x80
4 bytes: length of the trailer
n bytes: trailer
--]]
local status = ctx.var.upstream_trailer_grpc_status
local message = ctx.var.upstream_trailer_grpc_message
if status ~= "" and status ~= nil then
local status_str = "grpc-status:" .. status
local status_msg = "grpc-message:" .. ( message or "")
local grpc_web_trailer = status_str .. "\r\n" .. status_msg .. "\r\n"
local len = #grpc_web_trailer

-- 1 byte: 0x80
local trailer_buf = string.char(0x80)
-- 4 bytes: length of the trailer
trailer_buf = trailer_buf .. string.char(
bit.band(bit.rshift(len, 24), 0xff),
bit.band(bit.rshift(len, 16), 0xff),
bit.band(bit.rshift(len, 8), 0xff),
bit.band(len, 0xff)
)
-- n bytes: trailer
trailer_buf = trailer_buf .. grpc_web_trailer
if ngx_arg[2] then -- if eof
local status = ctx.var.upstream_trailer_grpc_status
local message = ctx.var.upstream_trailer_grpc_message

if ctx.grpc_web_encoding == CONTENT_ENCODING_BINARY then
ngx_arg[1] = ngx_arg[1] .. trailer_buf
else
ngx_arg[1] = ngx_arg[1] .. encode_base64(trailer_buf)
-- When the response body completes and still does not receive the grpc status
local resp_ok = status ~= nil and status ~= ""
local trailer_buf = build_trailer(
resp_ok and status or 2,
resp_ok and message or "upstream grpc status not received"
)
if ctx.grpc_web_encoding == CONTENT_ENCODING_BASE64 then
trailer_buf = encode_base64(trailer_buf)
end

-- clear trailer
ctx.var.upstream_trailer_grpc_status = nil
ctx.var.upstream_trailer_grpc_message = nil
ngx_arg[1] = ngx_arg[1] .. trailer_buf
end
end

Expand Down
51 changes: 25 additions & 26 deletions t/plugin/grpc-web.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,14 @@ passed
=== TEST 2: Proxy unary request using APISIX with trailers gRPC-Web plugin
Status should be printed at most once per request, otherwise this would be out of specification.
--- exec
node ./t/plugin/grpc-web/client.js BIN UNARY
node ./t/plugin/grpc-web/client.js TEXT UNARY
--- response_body
Status: { code: 0, details: '', metadata: {} }
Status: { code: 0, details: '', metadata: {} }
{"name":"hello","path":"/hello"}
Status: { code: 0, details: '', metadata: {} }
Status: { code: 0, details: '', metadata: {} }
{"name":"hello","path":"/hello"}
Expand All @@ -90,11 +89,9 @@ node ./t/plugin/grpc-web/client.js TEXT STREAM
{"name":"hello","path":"/hello"}
{"name":"world","path":"/world"}
Status: { code: 0, details: '', metadata: {} }
Status: { code: 0, details: '', metadata: {} }
{"name":"hello","path":"/hello"}
{"name":"world","path":"/world"}
Status: { code: 0, details: '', metadata: {} }
Status: { code: 0, details: '', metadata: {} }
Expand All @@ -112,7 +109,7 @@ Access-Control-Allow-Origin: *
=== TEST 5: test non-options request
--- request
GET /grpc/web/a6.RouteService/GetRoute
--- error_code: 400
--- error_code: 405
--- response_headers
Access-Control-Allow-Origin: *
--- error_log
Expand All @@ -128,7 +125,7 @@ Content-Type: application/json
--- error_code: 400
--- response_headers
Access-Control-Allow-Origin: *
Content-Type: application/json
Content-Type: text/html
--- error_log
request Content-Type: `application/json` invalid
Expand Down Expand Up @@ -177,7 +174,7 @@ Content-Type: application/grpc-web
--- error_code: 400
--- response_headers
Access-Control-Allow-Origin: *
Content-Type: application/grpc-web
Content-Type: text/html
--- error_log
routing configuration error, grpc-web plugin only supports `prefix matching` pattern routing
Expand Down Expand Up @@ -226,33 +223,35 @@ passed
=== TEST 10: don't override Access-Control-Allow-Origin header in response
--- request
POST /grpc/web/a6.RouteService/GetRoute
{}
--- more_headers
Origin: http://test.com
Content-Type: application/grpc-web
--- response_headers
Access-Control-Allow-Origin: http://test.com
Content-Type: application/grpc-web
--- exec
curl -iv --location 'http://127.0.0.1:1984/grpc/web/a6.RouteService/GetRoute' \
--header 'Origin: http://test.com' \
--header 'Content-Type: application/grpc-web-text' \
--data-raw 'AAAAAAcKBXdvcmxkCgo='
--- response_body eval
qr/HTTP\/1.1 200 OK/ and qr/Access-Control-Allow-Origin: http:\/\/test.com/
=== TEST 11: check for Access-Control-Expose-Headers header in response
--- request
POST /grpc/web/a6.RouteService/GetRoute
{}
--- more_headers
Origin: http://test.com
Content-Type: application/grpc-web
--- response_headers
Access-Control-Allow-Origin: http://test.com
Access-Control-Expose-Headers: grpc-message,grpc-status
Content-Type: application/grpc-web
--- exec
curl -iv --location 'http://127.0.0.1:1984/grpc/web/a6.RouteService/GetRoute' \
--header 'Origin: http://test.com' \
--header 'Content-Type: application/grpc-web-text' \
--data-raw 'AAAAAAcKBXdvcmxkCgo='
--- response_body eval
qr/Access-Control-Expose-Headers: grpc-message,grpc-status/ and qr/Access-Control-Allow-Origin: http:\/\/test.com/
=== TEST 12: verify trailers in response
According to the gRPC documentation, the grpc-web proxy should not retain trailers received from upstream when
forwarding them, as the reference implementation envoy does, so the current test case is status quo rather
than "correct", which is not expected to have an impact since browsers ignore trailers.
Currently there is no API or hook point available in nginx/lua-nginx-module to remove specified trailers
on demand (grpc_hide_header can do it but it affects the grpc proxy), and some nginx patches may be needed
to allow for code-controlled removal of the trailer at runtime.
When we implement that, this use case will be removed.
--- exec
curl -iv --location 'http://127.0.0.1:1984/grpc/web/a6.RouteService/GetRoute' \
--header 'Content-Type: application/grpc-web+proto' \
Expand Down

0 comments on commit 2a5425f

Please sign in to comment.