Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing trailers override? #1115

Open
gryphonmyers opened this issue Jun 21, 2024 · 5 comments
Open

Missing trailers override? #1115

gryphonmyers opened this issue Jun 21, 2024 · 5 comments

Comments

@gryphonmyers
Copy link

I am trying to use connectrpc-es with an nginx grpc-web proxy https://nginx.org/en/docs/http/ngx_http_grpc_module.html

I have the proxy working, with the exception that connectrpc-es throws an error over "missing trailers" despite having received the message intact. I am aware that nginx should send trailers, but it would seem that they're not planning to do that grpc/grpc-go#892.

Is there any way I can work around this so that connectrpc-es will forego throwing an error when the trailers are missing? I was hoping for something like shouldThrowOnMissingTrailers: false but I checked the docs and see no such configuration options.

@timostamm
Copy link
Member

Hey Gryphon, the error message is not about actual HTTP trailers. Since browsers do not expose headers to JavaScript, gRPC-web encodes the trailers that gRPC uses into the response body. Payload messages and trailers are delimited with a binary framing.

For some reason, it appears that the response body does not contain the gRPC-web trailer. We cannot ignore this case, since gRPC and gRPC-web encode errors in the trailer. Without trailers, you could call RPCs all day, and will never know that they failed 😅

I suspect that something else is happening. Can you:

  • Make sure that you have set up CORS correctly, if you are calling RPCs across origins. See here for thorough documentation: https://connectrpc.com/docs/cors
  • Check request and response:
    • Request field Content-Type should be application/grpc-web (+proto or +json is fine).
    • Same for response field Content-Type.
    • For successful calls, response field Grpc-Status should be 0 or omitted, and response body should be non-empty.
    • For failed calls, response field Grpc-Status may be > 0 - in this case, the body should be empty.

If this does not help you resolve the issue, please feel free to share request and response details from the browsers network explorer here, and we might be able to help.

@gryphonmyers
Copy link
Author

Hey Gryphon, the error message is not about actual HTTP trailers. Since browsers do not expose headers to JavaScript, gRPC-web encodes the trailers that gRPC uses into the response body. Payload messages and trailers are delimited with a binary framing.

For some reason, it appears that the response body does not contain the gRPC-web trailer. We cannot ignore this case, since gRPC and gRPC-web encode errors in the trailer. Without trailers, you could call RPCs all day, and will never know that they failed 😅

I suspect that something else is happening. Can you:

  • Make sure that you have set up CORS correctly, if you are calling RPCs across origins. See here for thorough documentation: https://connectrpc.com/docs/cors
  • Check request and response:
    • Request field Content-Type should be application/grpc-web (+proto or +json is fine).
    • Same for response field Content-Type.
    • For successful calls, response field Grpc-Status should be 0 or omitted, and response body should be non-empty.
    • For failed calls, response field Grpc-Status may be > 0 - in this case, the body should be empty.

If this does not help you resolve the issue, please feel free to share request and response details from the browsers network explorer here, and we might be able to help.

Thanks for the quick response. CORS is setup, content-type is set correctly, the response has no gRPC errors, the only evidence of anything going wrong is the "missing trailers" error being thrown by connectrpc-es. In fact, if I set a breakpoint on the thrown error, I can even see the data payload was received, but then it throws the error anyway over the lack of trailers in the response.

In searching this error, I've found evidence online indicating that this is a quirk of the nginx implementation of grpc-web grpc/grpc-web#1322 (comment) (also see link in my previous comment indicating that nginx does not intend to address this).

I understand that nginx really should stick to the spec, but from what I can see, the lack of trailers is not actually interfering with the receipt of a well-formed payload. This is why I ask if we might have a way of putting connectrpc-es into a more forgiving mode where it will not throw when the trailers are missing.

Also, "just use Envoy" is not an option. I can confirm that it does make this missing trailers issue go away, but for other reasons, it is not viable for our use case.

@timostamm
Copy link
Member

evidence online indicating that this is a quirk of the nginx implementation of grpc-web

Okay, I'm starting to see what's going on, thanks for the link to grpc-web discussion. To clarify: nginx does not implement grpc-web with the ngx_http_grpc_module. The purpose of the module is to pass gRPC requests to gRPC servers. This is not a quirk of it's grpc-web support - it doesn't support grpc-web.

While it's possible to add CORS headers and tweak the Content-Type header with nginx, the crucial part for gRPC-web - encoding trailers in the body - is missing.

If all you need is support for the happy path, the best way forward is to copy grpc-web-transport.ts into your project, and to modify it not to throw on line 227.

If we were to remove this check, users could easily get into a situation where everything seems to work, but the server is broken in a way that prevents the client from reporting an error. I hope you understand that this is not a good option.

@gryphonmyers
Copy link
Author

gryphonmyers commented Jun 29, 2024

evidence online indicating that this is a quirk of the nginx implementation of grpc-web

Okay, I'm starting to see what's going on, thanks for the link to grpc-web discussion. To clarify: nginx does not implement grpc-web with the ngx_http_grpc_module. The purpose of the module is to pass gRPC requests to gRPC servers. This is not a quirk of it's grpc-web support - it doesn't support grpc-web.

While it's possible to add CORS headers and tweak the Content-Type header with nginx, the crucial part for gRPC-web - encoding trailers in the body - is missing.

If all you need is support for the happy path, the best way forward is to copy grpc-web-transport.ts into your project, and to modify it not to throw on line 227.

If we were to remove this check, users could easily get into a situation where everything seems to work, but the server is broken in a way that prevents the client from reporting an error. I hope you understand that this is not a good option.

Of course not. The check should absolutely stay. My suggestion was opt-in configuration to bypass it. I would understand hesitance to do this because it seems a specific implementation (nginx) is the problem, even if it is one of only a few options serving this need.

@tahadostifam
Copy link

I had the same issue but i did not using Nginx. But i was getting missing trailer with no reason! That was interesting to me why is this happening. And after hours of researching i found that createGrpcWebTransport raises the error. And i migrated to createConnectTransport and problem fixed ^-^.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants