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

httpVersion check for createRouterTransport #1258

Closed
nicole0707 opened this issue Oct 7, 2024 · 16 comments
Closed

httpVersion check for createRouterTransport #1258

nicole0707 opened this issue Oct 7, 2024 · 16 comments
Labels
enhancement New feature or request

Comments

@nicole0707
Copy link

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 usingcreateRouterTransport with httpVersion: 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

@nicole0707 nicole0707 added the enhancement New feature or request label Oct 7, 2024
@nicole0707
Copy link
Author

BTW, I am happy to contribute to implementing the change if we come up with a solution. Cheers!

@timostamm
Copy link
Member

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 createRouterTransport, but running against a real server, you started seeing the error?

@nicole0707
Copy link
Author

nicole0707 commented Oct 7, 2024

Yes, it works in the unit test with "httpVersion":1.1, but it throws an error protocol error: missing status when running against the real service.

@timostamm
Copy link
Member

Thanks! Can you give an example of where you are setting "httpVersion":1.1? Can you share what server software the client was running against?

@nicole0707
Copy link
Author

nicole0707 commented Oct 13, 2024

Sure, I forked the examples-es repo and updated createConnectTransport with createGrpcTransport, the httpVersion is also set to 1.1. All tests pass, and there are no issues on npm build connectrpc/examples-es@main...nicole0707:examples-es:main. However, when I run npm start, I encounter this issue.

image

The root cause is that we are not properly validate the trailer when calling createRouterTransport. I found one relevant unit test covering HTTP/1.1. However, it doesn't account for the case where we are using the gRPC protocol.

What would be the best way to address this issue? Should we remove the HTTP/1.1 option from theNodeTransportOptions? Cheers!

@srikrsna-buf
Copy link
Member

The gRPC protocol doesn't work on HTTP/1.1, it only works where HTTP/2 trailers are supported.

@nicole0707
Copy link
Author

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?

@srikrsna-buf
Copy link
Member

srikrsna-buf commented Oct 14, 2024

If you want to use gRPC protocol you should only use HTTP/2

@nicole0707
Copy link
Author

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.

@nicole0707
Copy link
Author

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?

@srikrsna-buf
Copy link
Member

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.

@nicole0707
Copy link
Author

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?

@srikrsna-buf
Copy link
Member

Hey! In the example, the server actually isn't sending the trailers. We think it might be best to not support for 1.1 for gRPC as most implementations don't support it. This will be breaking change so will do this in v2

@timostamm
Copy link
Member

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 createGrpcTransport({ httpVersion: "1.1", baseUrl: "http://127.0.0.1:8080" }).

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 demo.connectrpc.com? (Thanks for the repro, @nicole0707!)

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.

@nicole0707
Copy link
Author

Thanks @timostamm @srikrsna-buf. The plan is reasonable for me. I appreciate your time and help! 👏

@timostamm
Copy link
Member

In the upcoming v2, we're disabling support for gRPC over HTTP 1.1.

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

No branches or pull requests

3 participants