From 0ec8eeafe4645d0d68f2f384b3421b903f5a0147 Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Sun, 23 Feb 2025 14:17:27 +0800 Subject: [PATCH 1/6] fix(grpc-web): response contains two trailer chunks --- apisix/plugins/grpc-web.lua | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/apisix/plugins/grpc-web.lua b/apisix/plugins/grpc-web.lua index 260e84c4eea3..c22ea7f88e42 100644 --- a/apisix/plugins/grpc-web.lua +++ b/apisix/plugins/grpc-web.lua @@ -172,9 +172,15 @@ function _M.body_filter(conf, ctx) 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 + if ngx_arg[2] then -- if eof + local status = ctx.var.upstream_trailer_grpc_status + local message = ctx.var.upstream_trailer_grpc_message + + -- When the response body completes and still does not receive the grpc status + if status == nil or status == "" then + core.log.error("upstream grpc status not received") + end + 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" From 30e60f299af1eba4470f9c37307b0feb09d7e257 Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Sun, 23 Feb 2025 14:45:16 +0800 Subject: [PATCH 2/6] fix: return unknown error when status is empty --- apisix/plugins/grpc-web.lua | 61 +++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/apisix/plugins/grpc-web.lua b/apisix/plugins/grpc-web.lua index c22ea7f88e42..127953663562 100644 --- a/apisix/plugins/grpc-web.lua +++ b/apisix/plugins/grpc-web.lua @@ -176,37 +176,46 @@ function _M.body_filter(conf, ctx) local status = ctx.var.upstream_trailer_grpc_status local message = ctx.var.upstream_trailer_grpc_message + 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 + + local attach_trailer = function (trailer_buf) + 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) + end + + -- clear trailer + ctx.var.upstream_trailer_grpc_status = nil + ctx.var.upstream_trailer_grpc_message = nil + end + -- When the response body completes and still does not receive the grpc status if status == nil or status == "" then core.log.error("upstream grpc status not received") - end - - 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 ctx.grpc_web_encoding == CONTENT_ENCODING_BINARY then - ngx_arg[1] = ngx_arg[1] .. trailer_buf + attach_trailer(build_trailer(2, "upstream grpc status not received")) else - ngx_arg[1] = ngx_arg[1] .. encode_base64(trailer_buf) + attach_trailer(build_trailer(status, message)) end - - -- clear trailer - ctx.var.upstream_trailer_grpc_status = nil - ctx.var.upstream_trailer_grpc_message = nil end end From 26fc40b036e4ca11b2195e7c0c497942b9eeccd5 Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Mon, 24 Feb 2025 22:21:34 +0800 Subject: [PATCH 3/6] chore: format code --- apisix/plugins/grpc-web.lua | 98 ++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 46 deletions(-) diff --git a/apisix/plugins/grpc-web.lua b/apisix/plugins/grpc-web.lua index 127953663562..8b1ca64609ac 100644 --- a/apisix/plugins/grpc-web.lua +++ b/apisix/plugins/grpc-web.lua @@ -68,6 +68,32 @@ 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 + +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 @@ -76,19 +102,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 @@ -98,7 +124,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"] @@ -110,16 +136,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 @@ -139,11 +166,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 @@ -176,46 +211,17 @@ function _M.body_filter(conf, ctx) local status = ctx.var.upstream_trailer_grpc_status local message = ctx.var.upstream_trailer_grpc_message - 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 - - local attach_trailer = function (trailer_buf) - 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) - end - - -- clear trailer - ctx.var.upstream_trailer_grpc_status = nil - ctx.var.upstream_trailer_grpc_message = nil - end - -- When the response body completes and still does not receive the grpc status - if status == nil or status == "" then - core.log.error("upstream grpc status not received") - attach_trailer(build_trailer(2, "upstream grpc status not received")) - else - attach_trailer(build_trailer(status, message)) + 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 + + ngx_arg[1] = ngx_arg[1] .. trailer_buf end end From 1e945ca0c4a9ce8a45a216f93bf4cac32cb1ee7d Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Mon, 24 Feb 2025 22:22:54 +0800 Subject: [PATCH 4/6] test: fix --- t/plugin/grpc-web.t | 51 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/t/plugin/grpc-web.t b/t/plugin/grpc-web.t index 1d3a3647e705..ac8f689ee9af 100644 --- a/t/plugin/grpc-web.t +++ b/t/plugin/grpc-web.t @@ -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"} @@ -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: {} } @@ -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 @@ -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 @@ -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 @@ -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' \ From 08b163b1e55c91390646f2ef8665671a63e237c5 Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Tue, 25 Feb 2025 10:07:19 +0800 Subject: [PATCH 5/6] chore: move build trailer comment --- apisix/plugins/grpc-web.lua | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/apisix/plugins/grpc-web.lua b/apisix/plugins/grpc-web.lua index 8b1ca64609ac..1f4a617c306b 100644 --- a/apisix/plugins/grpc-web.lua +++ b/apisix/plugins/grpc-web.lua @@ -73,6 +73,20 @@ local function exit(ctx, status) 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 byte[] local build_trailer = function (grpc_status, grpc_message) local status_str = "grpc-status:" .. grpc_status local status_msg = "grpc-message:" .. ( grpc_message or "") @@ -194,19 +208,6 @@ 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 - - --]] if ngx_arg[2] then -- if eof local status = ctx.var.upstream_trailer_grpc_status local message = ctx.var.upstream_trailer_grpc_message From abb1f92359c8a3683408f1c82decb1bb9e00e5c2 Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Tue, 25 Feb 2025 10:11:47 +0800 Subject: [PATCH 6/6] chore: use raw string --- apisix/plugins/grpc-web.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apisix/plugins/grpc-web.lua b/apisix/plugins/grpc-web.lua index 1f4a617c306b..d69d3e6651ca 100644 --- a/apisix/plugins/grpc-web.lua +++ b/apisix/plugins/grpc-web.lua @@ -86,7 +86,7 @@ end -- -- @param grpc_status number grpc status code -- @param grpc_message string grpc message --- @return string grpc-web trailer chunk in byte[] +-- @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 "")