You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "bzp2010 (via GitHub)" <gi...@apache.org> on 2025/02/23 06:38:14 UTC
[PR] fix(grpc-web): response contains two trailer chunks [apisix]
bzp2010 opened a new pull request, #11988:
URL: https://github.com/apache/apisix/pull/11988
### Description
This PR is intended to address the following issues:
- Sometimes grpc-web requests incorrectly contain two trailer blocks, which according to grpc's protocol requirements must exist and exist only once. (https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2 Message framing)
The reason for this is easy to explain, the old code used `status` to assert whether the response body had finished.
But this is unreliable, and it can go wrong in the following cases:
Since the response is chunked, the trailer chunk from upstream may not be fully included in the first response block, so the upstream_trailer_grpc_message variable is not available in the first body_filter, which causes APISIX to trigger the body_filter call a second time and incorrectly add a duplicate trailer block.
```
RESP HEADER: balabala
--- first chunk, eof = false
RESP BODY:
balabala
grpc-status: 0
--- second chunk, eof = true
RESP BODY:
grpc-message: "demo"
--- done
```
Based on the pseudo-code above, you'll see that when this happens, the `upstream_trailer_grpc_status` value is already available to us at the first `body_filter` call, which causes the plugin code to send a trailer chunk.
When the next upstream chunk is received, `body_filter` is called a second time, and `upstream_trailer_grpc_status` still has the value it had before, so the plugin code sends the trailer block again.
This results in multiple duplicate trailers and accidentally breaks the grpc-web js client in the browser.
This PR changes the end of the response asserted using the `status` value to use the `ngx.arg[2]` value, which will be the most reliable and trigger the code to send the trailer only on the last chunk. It does not add or modify any existing behavior, so it does not result in test case modifications either.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(grpc-web): response contains two trailer chunks [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11988:
URL: https://github.com/apache/apisix/pull/11988#discussion_r1968781193
##########
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
Review Comment:
```suggestion
return trailer_buf
```
##########
apisix/plugins/grpc-web.lua:
##########
@@ -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)
Review Comment:
1. why do we need warn log here?
2. we can use core.log.warn instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(grpc-web): response contains two trailer chunks [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #11988:
URL: https://github.com/apache/apisix/pull/11988#discussion_r1968784269
##########
apisix/plugins/grpc-web.lua:
##########
@@ -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)
Review Comment:
This is probably code for testing and debugging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(grpc-web): response contains two trailer chunks [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #11988:
URL: https://github.com/apache/apisix/pull/11988#discussion_r1968783946
##########
apisix/plugins/grpc-web.lua:
##########
@@ -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)
Review Comment:
Obviously here leaves out some unneeded code, which I will remove in another PR. 🫡
##########
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
Review Comment:
Good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(grpc-web): response contains two trailer chunks [apisix]
Posted by "juzhiyuan (via GitHub)" <gi...@apache.org>.
juzhiyuan merged PR #11988:
URL: https://github.com/apache/apisix/pull/11988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(grpc-web): response contains two trailer chunks [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11988:
URL: https://github.com/apache/apisix/pull/11988#discussion_r1968801292
##########
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
Review Comment:
this is a good first issue, hopefully someone from the community can fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org