fullstorydev / grpcurl

Like cURL, but for gRPC: Command-line tool for interacting with gRPC servers
MIT License
10.35k stars 497 forks source link

add encoding flag to allow specifying compression as identity #444

Closed joprice closed 5 months ago

joprice commented 5 months ago

Gzip encoding was added in https://github.com/fullstorydev/grpcurl/pull/124. By default the go client will only use this encoder for all requests and not send the identity encoder value. I'm testing a dev server that doesn't have gzip encoding implemented yet, so would like to send requests using identity. This new optional flag encoding sends the expected grpc-encoding and grpc-accept-encoding headers. In the case of identity, the grpc-encoding header only includes identity, while the grpc-accept-encoding header includes identity and gzip, so the server can still respond with gzip if it chooses when identity is passed.

dragonsinth commented 5 months ago

Wait... are you saying that currently, grpcurl always uses gzip on requests?

joprice commented 5 months ago

I couldn't get it not to use gzip, but it could be my malfunctioning server. I'm ok using using my fork with this fix for now, so I don't need it merged in a rush. I can dig deeper to figure out if it's a encoding negotiation issue. I used wireshark and only saw gzip being sent and quickly did this workaround.

dragonsinth commented 5 months ago

I'd just like to get to the bottom of things before we commit something! I'm not sure ordinary grpcurl really needs a gzip option, so if the actual issue is "grpcurl unexpectedly sends gzip", we should identify that. AFAIK registering the gzip compressor is mostly to support servers which (for whatever reason) decide to send a gzip response back.

joprice commented 5 months ago

That makes sense. I'll fire up wireshark and grab the headers again to confirm the behavior. I think what I remember seeing was that the grpc-encoding header was always gzip without telling it to use identity. If that's the case, and the go client doesn't do automatic compression fallback for the request body, the flag is still useful. I also would find it useful to be able to ensure that the request is using the right encoding for reproducing issues locally that could be compression related.

jhump commented 5 months ago

In the linked PR (124), that does not cause grpcurl to send gzipped requests. It simply allows grpcurl to accept gzip'ed responses.

IIUC, in order to actually compress a request with the grpc-go module, the caller must use the grpc.UseCompressor call option (or install it as a default call option, to always compress).

When you say you are seeing grpcurl send compressed requests, are you sure you aren't just seeing the Grpc-Accept-Encoding: gzip header? I just tested (using v1.8.9 of grpcurl), and it did not send a compressed request (i.e. Grpc-Encoding request header was not set). But it did send the Grpc-Accept-Encoding: gzip header, to advertise that it supports gzip'ed responses.

joprice commented 5 months ago

I see it sending both headers with my change. Screenshot 2024-01-29 at 5 49 05 PM

joprice commented 5 months ago

And without the change, I see just grpc-accept-encoding Screenshot 2024-01-29 at 5 53 57 PM.

I misunderstood the original situation, which is that grpcurl by default asks for gzip, but doesn't send it. And in this case, I guess the server can fall back to using an uncompressed response and set the encoding header accordingly. So this change would alter the behavior by sending as gzip unconditionally by default.

The flag could instead be empty by default, using the current behavior (asymmetric encoding), and then when provided, the request body will be sent as gzip as well.

jhump commented 5 months ago

The flag could instead be empty by default, using the current behavior (asymmetric encoding), and then when provided, the request body will be sent as gzip as well.

Is a flag even needed though? You said you misunderstood the original situation. So, now that the original situation is understood, is it likely that no change is needed in grpcurl at all?

joprice commented 5 months ago

Maybe I'm missing something. Is there a way currently to send a gzipped request? It seems that the tool always sends an uncompressed request and receives a (potentially) compressed one. If not, my change is still useful to me for now to test support for compressed and uncompressed calls independently (as one would with curl's --compressed flag). If it's not something frequent users of grpc services end up needing, I can close the pr and use my fork for a few weeks while I'm debugging.

jhump commented 5 months ago

Is there a way currently to send a gzipped request?

There is not. I guess what's unclear is why that's needed. And from your last reply, it sounds like the answer is "to test server implementations' compression support"

as one would with curl's --compressed flag

FWIW, this flag does not compress requests. It only requests compressed responses, just like grpcurl does by default.

$ curl --compressed -X POST -d '{}' -H "content-type: application/json" -v -o /dev/null \
        https://demo.connectrpc.com/connectrpc.eliza.v1.ElizaService/Say
...
> POST /connectrpc.eliza.v1.ElizaService/Say HTTP/2
> Host: demo.connectrpc.com
> User-Agent: curl/8.4.0
> Accept: */*
> Accept-Encoding: deflate, gzip
> content-type: application/json
> Content-Length: 2
> 
} [2 bytes data]
...
joprice commented 5 months ago

Yea I wasn't trying to be pedantic about it, just pointing out a flag on curl that can be used in a similar fashion, even if it doesn't map exactly to what I'm looking for. There are other flags and headers that can be used to control request encoding. I'll accept this is a limitation of this cli that it doesn't intend to expose similar control.

dragonsinth commented 5 months ago

👍 thanks for hashing this out!