fix(grpc-web): response contains two trailer chunks (#11988)

Author: bzp2010

apisix/plugins/grpc-web.lua

@@ -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
@@ -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
@@ -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"]
@@ -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
 
@@ -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
@@ -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
 

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' \