spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.87k stars 2.44k forks source link

Cannot upload files with Zuul (gzip is handled incorrectly) #2890

Closed stheves closed 1 year ago

stheves commented 6 years ago

My setup is as follows:

client -> zuul -> downstream service and back. When sending a file from client to the downstream service, the client compresses the request body and sets the "Content-Encoding: gzip" header correctly. Now Zuul just removes the "Content-Encoding" header and forwards the request to the downstream service. The downstream service is now unable to read the body from the request because it is gzipped but the "Content-Encoding" header is missing.

We are using Edgware.SR3 release of Spring Cloud Netflix and this behaviour can easily reproduced with any spring boot sample app and setup.

ryanjbaxter commented 6 years ago

I know @brenuart submitted some fixes to the 2.0.x stream around gzip compression. Could you try Finchley.RC1 and see if you have the same issue?

stheves commented 6 years ago

I tried Finchley.RC1 release but the behaviour is still the same. The Content-Encoding header gets removed and the content still remains compressed making it unreadable for the downstream service.

I think the issue comes from the fact that the ProxyRequestHelper#isIncludeHeader is used in ProxyRequestHelper#buildZuulRequestHeaders AND ProxyRequestHelper#setResponse methods. I think it should only be used in the ProxyRequestHelper#setResponse method so that the "Content-Encoding" header remains in the request forwarded to the downstream service. What do you think?

ryanjbaxter commented 6 years ago

I think we dont want to remove the isIncludeHeader check from buildZuulRequestHeaders completely because we still want to check for any headers the user wanted to ignore. Maybe we can separate that out into a separate method though. I dont see any reason why Content-Encoding shouldnt be proxied downstream. @spencergibb what do you think?

spencergibb commented 6 years ago

Unsure why it is removed. Here are the official headers that should be removed: https://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-14#section-7.1.3

I think the content-length header was removed because one of the http clients puked if it was there. Maybe something similar with content-encoding?

stheves commented 6 years ago

@spencergibb You are right when removing the encoding header but keeping the length header the Apache HTTP client complains about it because the client wants to compute the content length by its own. But you can disable that if you want.

I´ve implemented my workaround as @ryanjbaxter suggested and just kept the content encoding header. I don`t like my workaround because I had to copy most of the code from ProxyRequestHelper#buildZuulRequestHeaders because it is not easy for a client to override.

Here is my workaround:

    private ProxyRequestHelper proxyRequestHelper(ZuulProperties zuulProperties) {
        TraceProxyRequestHelper helper = new TraceProxyRequestHelper() {
            @Override
            public MultiValueMap<String, String> buildZuulRequestHeaders(HttpServletRequest request) {
                RequestContext context = RequestContext.getCurrentContext();
                MultiValueMap<String, String> headers = new HttpHeaders();
                Enumeration<String> headerNames = request.getHeaderNames();
                if (headerNames != null) {
                    while (headerNames.hasMoreElements()) {
                        String name = headerNames.nextElement();
                        // this line is the bug fix
                        if (isIncludedRequestHeader(name)) {
                            Enumeration<String> values = request.getHeaders(name);
                            while (values.hasMoreElements()) {
                                String value = values.nextElement();
                                headers.add(name, value);
                            }
                        }
                    }
                }
                Map<String, String> zuulRequestHeaders = context.getZuulRequestHeaders();
                for (String header : zuulRequestHeaders.keySet()) {
                    headers.set(header, zuulRequestHeaders.get(header));
                }
                headers.set(HttpHeaders.ACCEPT_ENCODING, "gzip");
                return headers;
            }

            public boolean isIncludedRequestHeader(String headerName) {
                // forward the header to downstream services
                String name = headerName.toLowerCase();
                if ("content-encoding".equals(name)) {
                    return true;
                }
                return super.isIncludedHeader(headerName);
            }
        };
        if (this.traces != null) {
            helper.setTraces(this.traces);
        }
        helper.setIgnoredHeaders(zuulProperties.getIgnoredHeaders());
        helper.setTraceRequestBody(zuulProperties.isTraceRequestBody());
        return helper;
    }
ryanjbaxter commented 6 years ago

@stheves do you want to submit a PR with a change then?

OlgaMaciaszek commented 1 year ago

Closing the issue as Spring Cloud Netflix Zuul is no longer supported.