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

Add option to disable gzip grpc-accept-encoding #349

Merged
merged 8 commits into from Aug 17, 2022

Conversation

alexandrem
Copy link
Contributor

@alexandrem alexandrem commented Aug 16, 2022

It's currently impossible to use connect-go with Dapr gRPC integration.

This PR adds the ability to unset the default Grpc-Accept-Encoding: gzip and gzip compressors/decompressors in the client with a new client option named WithGzipAcceptEncoding(bool), which when set to false makes the communication through the proxy work as expected. It keeps the default to true to keep backward compatibility.

Context

Dapr comes with a gRPC proxy which enables service discovery, tracing, and mTLS among other things between gRPC applications. It seems that the proxy lacks the ability to decompress gzip content in the server responses, either generally, or perhaps only when speaking to a connect-go server (which would lead to believe that the connect-go implementation is doing something a bit differently which the proxy doesn't understand).

Currently, a connect-go client will implicitly set the Grpc-Accept-Encoding: gzip header to all requests by default. Combining a connect-go server with a connect-go client using dapr results in the following response on the client side coming from the proxy:

unimplemented: grpc: Decompressor is not installed for grpc-encoding "gzip"

By reproducing the grpc-go greet example, we can observe the following:

  • grpc-go client => grpc-go server = OK
  • dapr grpc-go client => dapr grpc-go server = OK
  • connect-go client => grpc-go server = OK
  • dapr connect-go client => dapr grpc-go server = OK
  • connect-go client => connect-go server = OK
  • dapr connect-go client => dapr connect-go server = ERROR

Note that we have to create a client with the gRPC protocol to make it talk to the proxy when using the connect-go client app as such:

	client := greetv1connect.NewGreetServiceClient(
		&http.Client{
			Transport: &http2.Transport{
				AllowHTTP: true,
				DialTLS: func(network, addr string, _ *tls.Config) (net.Conn, error) {
					// If you're also using this client for non-h2c traffic, you may want to
					// delegate to tls.Dial if the network isn't TCP or the addr isn't in an
					// allowlist.
					return net.Dial(network, addr)
				},
			},
		},
		"http://localhost:50007",
		connect.WithGRPC(),
	)

This PR fixes the problem by enabling a new option to not send the gzip accept encoding header:

	client := greetv1connect.NewGreetServiceClient(
		&http.Client{
			Transport: &http2.Transport{
				AllowHTTP: true,
				DialTLS: func(network, addr string, _ *tls.Config) (net.Conn, error) {
					// If you're also using this client for non-h2c traffic, you may want to
					// delegate to tls.Dial if the network isn't TCP or the addr isn't in an
					// allowlist.
					return net.Dial(network, addr)
				},
			},
		},
		"http://localhost:50007",
		connect.WithGRPC(),
		connect.WithGzipAcceptEncoding(false),
	)

The downside is that it's probably more expensive for large response payloads to do so.

I thought about fixing this in the connect-go handler, but I didn't want to break existing behaviour or introduce regressions for other http protocols. I suppose it's better to control this on the client side for communication between connect-go apps.

It's probably worth checking with the Dapr developers to see if there's something to do on their side to avoid doing this in the first place.

That being said, I observed that only connect-go was setting the Grpc-Accept-Encoding: gzip header by default, which differs from the grpc-go behaviour.

Therefore, I think that this PR allows to reproduce this behaviour on the client side if desired.

Let me know if you need the full working code example to test this with Dapr.

Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed error report and the PR, @alexandrem! Appreciate how much effort you've already put into debugging this 😍

This is a situation where connect-go implements more of the gRPC specification than many of the Google implementations. For clients, it's convenient to send uncompressed requests but ask for gzipped responses - everything works well even if the server doesn't support gzip, and in the best case you still end up compressing most of the data (since responses are usually much larger than requests). grpc/grpc-go#2786 tracks this missing feature, but it's been open for years. (Edit: it looks like there's finally some movement on this issue! grpc/grpc-go#5541 is under active review.)

I don't think we should work around Dapr by special-casing grpc-accept-encoding: gzip, because the same issue will arise with every other compressor (e.g., https://github.com/mattrobenolt/connect-brotli). I think we're better off with a way to un-register compressors. I'd prefer to do this by making WithAcceptCompression("gzip", nil, nil) remove gzip support from the client. Are you open to making this change? If not, I can do it.

For my own curiosity, can you also share your example code? This whole issue is very strange to me - I'm not sure why a sidecar gRPC proxy is decompressing the payload at all. I'd have expected this to work more like Envoy, where the bulk of the body is forwarded as-is.

@alexandrem
Copy link
Contributor Author

Hi @akshayjshah,
I will proceed with the proposed change with WithAcceptCompression, I think it makes more sense to unregister compressors indeed.

Please look at https://github.com/alexandrem/dapr-connect-go-example for a working example of the issue.

akshayjshah and others added 5 commits August 17, 2022 12:06
* Lint on Go 1.19

Unless new syntax is backward-incompatible, we should lint on the most
recent Go supported Go version. This PR changes lint to run on Go 1.19
and mechanically reformats comments with `gofmt -s -w .`.

* Use new Godoc links

Where possible, use Go 1.19's new Godoc links to refer to types,
packages, and external websites.

* Update deps, including golangci-lint

Update to latest golangci-lint (and other deps). Update to fix lint from
checks that now support generics.
Restructure so that the compression pool constructor doesn't return
nils. Unless there's an accompanying non-nil error, this makes nil
pointer dereferences too likely.
Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround, @alexandrem! I pushed a few commits to update the Godoc and add tests, but the core of your changes was great. I'll merge this as soon as tests pass.

Thanks again for contributing!

@akshayjshah akshayjshah merged commit b2db5b9 into connectrpc:main Aug 17, 2022
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
Rather than operating as a gRPC-aware HTTP proxy, Microsoft's `dapr` sidecar is apparently compressing and decompressing each proxied gRPC message. It's using grpc-go and doesn't appear to handle asymmetric compression, so this PR introduces a mechanism to disable gzip support in connect-go clients.
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

Successfully merging this pull request may close these issues.

None yet

2 participants