-
Notifications
You must be signed in to change notification settings - Fork 84
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
httpVersion check for createRouterTransport
#1258
Comments
BTW, I am happy to contribute to implementing the change if we come up with a solution. Cheers! |
Can you provide some details on where you started to see the error with v1.5? Do you mean that the tests ran fine with |
Yes, it works in the unit test with |
Thanks! Can you give an example of where you are setting |
Sure, I forked the ![]() The root cause is that we are not properly validate the trailer when calling createRouterTransport. I found one relevant unit test covering What would be the best way to address this issue? Should we remove the |
The gRPC protocol doesn't work on HTTP/1.1, it only works where HTTP/2 trailers are supported. |
Yes. Considering that we will check trailers by default with the gRPC protocol, should we throw an error or simply not allow HTTP/1.1, instead of swallowing the issue during the build? |
If you want to use gRPC protocol you should only use HTTP/2 |
The fact is we didn’t use HTTP/2 before Connect 1.5, and it ended up breaking our application in production without any issues during the build or unit tests when we upgraded. I believe we need to review our strategy regarding the HTTP version. |
If the trailer check is only relevant for HTTP/2, we should limit it to HTTP/2 checks. Otherwise, we shouldn’t allow HTTP/1.1 for the gRPC protocol. I’m just wondering, what was the intention behind adding the trailer check in Connect 1.5? |
If there is no trailer check, errors will not be reported. The protocol uses trailers to send error codes and error details. |
I understand the point. However, should we limit error detail support to only HTTP/2? If developers are not using HTTP/2, they should be guided to switch to HTTP/2 to ensure there is no impact on HTTP/1.1 support. Any insights? |
Hey! In the example, the server actually isn't sending the trailers. We think it might be best to not support for |
Krishna is right, gRPC implementations typically require HTTP/2, but the protocol itself does not. connect-es and connect-go can serve and call gRPC over HTTP 1.1. For example, we can modify the connect-go example to serve only HTTP 1.1: // ...
func main() {
mux := http.NewServeMux()
mux.Handle(elizav1connect.NewElizaServiceHandler(
NewElizaServer(time.Millisecond * 100),
))
err := http.ListenAndServe(
"127.0.0.1:8080",
mux,
)
if err != nil {
panic(err.Error())
}
} This service can be called from a connect-go client with gRPC, or from a connect-es client with This works for streaming too - with the exception of full-duplex streaming, which isn't possible in HTTP 1.1. The status, which is typically sent as a HTTP/2 trailer, is sent as a HTTP 1.1 trailer. So why does the connect-es client raise an error when it calls the same connect-go example service, hosted on It turns out the demo is not deployed correctly, and HTTP 1.1 trailers are lost. If the service were to raise an error, the client would never know. The additional checks in connect-es v1.5 verify that trailers are present as a safe-guard against exactly this situation. |
Thanks @timostamm @srikrsna-buf. The plan is reasonable for me. I appreciate your time and help! 👏 |
In the upcoming v2, we're disabling support for gRPC over HTTP 1.1. |
Is your feature request related to a problem? Please describe.
When Connect upgrade to v1.5, it starts to check the gRPC code and gRPC message from trailer or header by default. We encountered an issue when using
createRouterTransport
withhttpVersion: 1.1
, the test didn't pick up the breaking change for us.Describe the solution you'd like
I am wondering if possible to check the transport option when using
createRouterTransport
, ensure it's similar to testing against with real service.Please specify whether the request is for Connect for Web or Connect for Node.js.
Connect for Node.js
Describe alternatives you've considered
I am not sure of the best way to handle this, but perhaps we could verify the transport option like enforce checking the
httpVersion
starting from v1.5.Additional context
https://github.com/connectrpc/connect-es/pull/1205/files#diff-92e5a379d00b073cbd57f841a9669260a8e9603c2015e09aeeb49daf6fe788b8R40
The text was updated successfully, but these errors were encountered: