bug: grpc-web plugin causing missing trailers issue

[Affanmir]

Current Behavior

When i use the grpc-web plugin and cors plugin to communicate from my fronted to my backend, the response I receive an error missing trailers. I've tried the same thing using envoy and it works. There seems to be some misconfiguration is the response body which does not allow the response to

Expected Behavior

Envoy Response: (CORRECT AND DOES NOT CAUSE ISSUE)

Hello! Mohsin Javed� grpc-status:0
grpc-message:OK

Headers:

HTTP/1.1 200 OK
content-type: application/grpc-web+proto
grpc-accept-encoding: identity,deflate,gzip
grpc-encoding: identity
date: Fri, 24 Nov 2023 06:04:45 GMT
x-envoy-upstream-service-time: 12
access-control-allow-origin: http://localhost:58653/
access-control-expose-headers: custom-header-1,grpc-status,grpc-message
server: envoy
transfer-encoding: chunked

API SIX Response:: (INCORRECT

AND CAUSES ISSUE MISSING TRAILERS)

Hello! World

Headers:
HTTP/1.1 200 OK
Date: Fri, 24 Nov 2023 06:13:48 GMT
Content-Type: application/grpc-web+proto
Transfer-Encoding: chunked
Connection: keep-alive
grpc-accept-encoding: identity,deflate,gzip
grpc-encoding: identity
Server: APISIX/3.6.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: *
Access-Control-Max-Age: 5
Access-Control-Expose-Headers: *
Access-Control-Allow-Headers: *

Error Logs

Steps to Reproduce

enable grpc-web and cors plugin on a route that communicates from any frontend / backend service that implemented a grpc service using a proto definition.

Environment

Please take a clone of this m-jayy/grpc_with_envoy_and_apisix#1 All steps to reproduce are mentioned in the README.md

This is a small local setup with node as the back end and flutter as the frontend

[shreemaan-abhishek]

could you please provide me with reproduction steps and reproduction environment identical to yours? This will save us a lot of back and forth.

[Affanmir]

@shreemaan-abhishek Please take a clone of this PR All steps to reproduce are mentioned in the README.md

This is a small local setup with node as the back end and flutter as the frontend

[kayx23]

same issue: #10254

[kayx23]

See projectcontour/contour#3233

[shreemaan-abhishek]

@Affanmir I'm taking a look at this, mean while could you please share your route and upstream configuration? I want to confirm if you have used the grpc scheme correctly:

[Affanmir]

@shreemaan-abhishek You can find the schema in the PR I've linked, rest of the configuration I've mentioned below

[alifahadk]

@shreemaan-abhishek We are trying to use this plugin for proxying between a gRPC-web client (more specifically, the gRPC-dart web client) and a gRPC-java server.

Our gRPC-java server does not return a gRPC-web-compliant HTTP/1.1 response on its own (with the trailers present in the response body). Is this plugin meant to cater to such a scenario (similar to the support provided by the gRPC-web filter by Envoy)? Our initial assumption was that it does that, but latest explorations suggest otherwise.

tldr; we are looking for a way to transform the HTTP/2 based gRPC response from upstream into a HTTP/1.1 response for downstream with in-body gRPC trailers as required by the gRPC-web specification

[shreemaan-abhishek]

I am no expert in this field but

with in-body gRPC trailers as required by the gRPC-web specification

I remember reading this somewhere. Does it mean that grpc-web plugin not supporting trailers (as part of headers) is by design?

EDIT:

looks like trailers as part of response body is also not being supported.

[Affanmir]

@shreemaan-abhishek
looks like trailers as part of response body is also not being supported.
Yes exactly on point. Is this by design or a bug?

[shreemaan-abhishek]

Not a bug, it's just not supported. You may take this up if you want to fix this.

[Affanmir]

@shreemaan-abhishek Is it possible for you to provide us some resources on where this functionality needs to be added? are there any conventions or guidelines we should be following? Essentially what I'm thinking is to add this logic in the plugin itself. But it will require some changes to response body and exposing some additional headers as well.

[sheharyaar]

@Affanmir , is the associated PR ready or can I take up this issue and work on a PR ?
Your PR implements the various CORS requirements as mentioned in the grp-c doc : https://github.com/grpc/grpc-web/blob/master/doc/browser-features.md#cors-support

We will need to check and implement the entire requirement doc.

[Affanmir]

@sheharyaar it is missing, absolute pathing some additional DEFAULT_CORS_ALLOW_HEADERS, the missing trailers issue is fixed in the associated PR but these extra functionality will need to be incorporated

Current behavior of grpc is following
IMPORTANT
While using the grpc-web Plugin, always use a prefix matching pattern (/, /grpc/example/) for matching Routes. This is because the gRPC Web client passes the package name, the service interface name, the method name and other information in the proto in the URI. For example, /path/a6.RouteService/Insert.

Ive made it work locally and added parameters to enable/disable will be updating the PR soon this week

[sheharyaar]

Cool, we also need to have HTTP to GRPC mapping : https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md

This table is to be used only for clients that received a response that did not include grpc-status. If grpc-status was provided, it must be used. Servers must not use this table to determine an HTTP status code to use; the mappings are neither symmetric nor 1-to-1.

[Affanmir]

@sheharyaar maybe you can create a separate PR for that mapping

[sheharyaar]

Ok, once your PR is merged I can do that in another PR.

[sheharyaar]

@Affanmir , I will be taking up this PR, I will fix the CI and do the next steps.

[sheharyaar]

@shreemaan-abhishek can you please assign me this ?

[Affanmir]

@sheharyaar You can complete it but I've discovered another bug in this plugin. The CORS plugin does not work with this plugin essentially the issue will remain unresolved without this fix. I've tried changing the plugin execution order aswell but to no avail. Since this plugin has some partial cors code I'll be building upon that and adding parametrized way of adding cor headers. If you want to take that up aswell let me know

[sheharyaar]

Sure, just please list out the tasks that are pending. I will take up the rest and once the PR is ready, I will ping you.

[sheharyaar]

@Affanmir , I noticed that when you use grpc-web-text, the reponse has the trailers (grpc-status and grpc-message)

curl -i 'http://127.0.0.1:9080/grpc/web/a6.RouteService/GetRoute' \
                      -H 'Accept: application/grpc-web-text' \
                      -H 'Accept-Language: en-IN,en-US;q=0.9,en-GB;q=0.8,en;q=0.7' \
                      -H 'Connection: keep-alive' \
                      -H 'Content-Type: application/grpc-web-text' \
                      --data-raw 'AAAAAAcKBWhlbGxv' \
                      
HTTP/1.1 200 OK
Date: Wed, 17 Jan 2024 11:30:05 GMT
Content-Type: application/grpc-web-text
Transfer-Encoding: chunked
Connection: keep-alive
Server: APISIX/3.8.0
Vary: Origin
Access-Control-Allow-Origin: *

AAAAAA8KBWhlbGxvEgYvaGVsbG8=grpc-status: 0
grpc-message: 

[sheharyaar]

Also I noticed that, when using grpc-web as the Content-type, the trailer is received as a Header as shown (only when the status is not 0) :

This is in conformance to the spec :
https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md#protocol-differences-vs-grpc-over-http2