movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
497 stars 55 forks source link

Allow compressed response from services to gateway #132

Closed labala-sravani closed 2 years ago

labala-sravani commented 2 years ago

Currently, bramble was not able to handle the compressed response (ie., gzip format response) from services. So, added the switch case in the code which handles the compressed response as well from the services

pkqk commented 2 years ago

Hi @labala-sravani thanks for the PR, from this discussion on stackoverflow and the net/http documentation it appears that it should handle gzipped responses already.

Do you have an example of it failing to? I ran your test without setting the header and the gzip reader removed and it passed, if you set the header then it fails to decompress.

labala-sravani commented 2 years ago

Hi @pkqk, yes if the header was set, it is giving error at decoding response saying error decoding response: invalid character '\x1f' looking for beginning of value at this line

pkqk commented 2 years ago

Yes, the docs seem to imply that if you set the header it turns off the automatic gzip handling in http.Transport but if not specified it will add Accept-Encoding: gzip and decompress it if the response comes back compressed.

I modified your commit here: https://github.com/movio/bramble/commit/6dac7949ab6933464f50641f94e6a3f1c772dff2 to see if that would happen, the request has the Accept-Encoding header, and decompresses the response without the gzip.Reader.

Did you have another case where it wouldn't handle a compressed response?

labala-sravani commented 2 years ago

When using bramble, I have introduced plugins to re-route the query and observed that the required headers being passed to the gateway are not passed down to services. So, I have written the following code for adding the outgoing request headers to context.

ctx := r.Context() for key := range r.Header { if key == "Accept-Encoding" { continue } ctx = bramble.AddOutgoingRequestsHeaderToContext(ctx, key, r.Header.Get(key)) }

When I tried making a request to gateway using curl, it was passing the Accept-Encoding header and because of that, bramble is unable to decode the response. So to avoid it, I added the above condition as shown.

The only use case for the PR is when the Accept-Encoding header being passed explicitly from the gateway to service. (You can also refer to the test case).

sodabrew commented 2 years ago

Drive-by opinion: I don't understand why it would make sense to pass encoding headers from the client to the backend. Why is this a thing you want?

There are other headers important to pass, anything related to authorization, for example, but encoding is about the transfer of data between the services.

Client -- compressed --> Bramble -- uncompressed --> Backend

Client -- compressed Alg A--> Bramble -- compressed Alg B --> Backend

Crucially, there is a decompression that must happen at the middle box because it is processing the data within.

pkqk commented 2 years ago

@labala-sravani instead of putting a custom decoding case into bramble can you use the custom client config option introduced in https://github.com/movio/bramble/pull/122 ?

You could use that to register your own client or transport from a plugin.

labala-sravani commented 2 years ago

@sodabrew Even I don’t want to pass the accept-encoding header explicitly.

But the thing is, I need to pass many headers to services from client and I don’t want to mention each of the header name in code as, it led into the problem that whenever a new header being added to any of the service that header need to be mentioned in the gateway. That’s why, irrespective of specific headers, any headers that come to gateway is passed to the service.

So, when doing the call to the gateway, for example using curl, the Accept-Encoding is getting passed along with other headers which is causing the issue. (The Accept-Encoding header was not passed by me when making the call, it was passed by the curl itself)

@pkqk Currently, I am using v1.3.2. Thanks for the suggestion, will try the custom client config option with the latest version. Until then closing the PR.

sodabrew commented 2 years ago

Ohhh, that makes sense! Thanks for explaining!

Well then would it make sense to have a list of headers to filter out?

Things like the content type, accept encoding, maybe cache control headers that shouldn't go through to the backend?