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

reverseproxy: Set the headers in the replacer before handle_response #4165

Merged
merged 1 commit into from May 12, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented May 12, 2021

Turns out this was an oversight, we assumed we could use {http.response.header.*} but that doesn't work because those are grabbed from the response writer, and we haven't copied any headers into the response writer yet.

So the fix is to set all the response headers into the replacer at a new namespace before running the handlers.

This adds the {http.reverse_proxy.header.*} replacer.

See https://caddy.community/t/empty-http-response-header-x-accel-redirect/12447

Turns out this was an oversight, we assumed we could use `{http.response.header.*}` but that doesn't work because those are grabbed from the response writer, and we haven't copied any headers into the response writer yet.

So the fix is to set all the response headers into the replacer at a new namespace before running the handlers.

This adds the `{http.reverse_proxy.header.*}` replacer.

See https://caddy.community/t/empty-http-response-header-x-accel-redirect/12447
@francislavoie francislavoie added the bug 🐞 Something isn't working label May 12, 2021
@francislavoie francislavoie added this to the v2.4.1 milestone May 12, 2021
@francislavoie francislavoie requested a review from mholt May 12, 2021 04:27
@francislavoie francislavoie changed the title ci: Run CI on PRs targeting minor version branches (#4164) reverseproxy: Set the headers in the replacer before handle_response May 12, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks, interesting situation... I guess this'll work, assuming we never need to differentiate request headers and response headers in the future.

@mholt mholt merged commit aef8d4d into 2.4 May 12, 2021
@mholt mholt deleted the handle-response-headers branch May 12, 2021 20:19
@francislavoie
Copy link
Member Author

francislavoie commented May 12, 2021

I think we'd go with http.reverse_proxy.request_header.* if we ever did, which matches the request_header handler.

@corny
Copy link

corny commented May 18, 2021

I have another related issue: If the upstream server returns a Content-Disposition header, it won't be copied into the response. I need to add this line to the handle_response block:

header Content-Disposition "{http.reverse_proxy.header.Content-Disposition}"

Update: Same applies to the Content-Type header.

@francislavoie
Copy link
Member Author

That's normal. No headers are copied by default in handle_response, you need to do it yourself for anything that's relevant. It just gives you a clean slate to re-handle the request based on the response from the upstream.

@corny
Copy link

corny commented May 18, 2021

I see that Set-Cookie, X-Request-Id and a couple of other headers are copied. That's a little bit confusing, isn't it?

@francislavoie
Copy link
Member Author

francislavoie commented May 18, 2021

Where do you see that @corny? The code doesn't copy any headers.

for i, rh := range h.HandleResponse {
if rh.Match != nil && !rh.Match.Match(res.StatusCode, res.Header) {
continue
}
// if configured to only change the status code, do that then continue regular proxy response
if statusCodeStr := rh.StatusCode.String(); statusCodeStr != "" {
statusCode, err := strconv.Atoi(repl.ReplaceAll(statusCodeStr, ""))
if err != nil {
return caddyhttp.Error(http.StatusInternalServerError, err)
}
if statusCode != 0 {
res.StatusCode = statusCode
}
break
}
// otherwise, if there are any routes configured, execute those as the
// actual response instead of what we got from the proxy backend
if len(rh.Routes) == 0 {
continue
}
res.Body.Close()
// set up the replacer so that parts of the original response can be
// used for routing decisions
for field, value := range res.Header {
repl.Set("http.reverse_proxy.header."+field, strings.Join(value, ","))
}
repl.Set("http.reverse_proxy.status_code", res.StatusCode)
repl.Set("http.reverse_proxy.status_text", res.Status)
h.logger.Debug("handling response", zap.Int("handler", i))
if routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req); routeErr != nil {
// wrap error in roundtripSucceeded so caller knows that
// the roundtrip was successful and to not retry
return roundtripSucceeded{routeErr}
}
}

What does your Caddyfile look like? How did you build Caddy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants