Open dbaynard opened 11 months ago
I think my hypothesis was correct?
route @origin {
header X-Before-Proxy true
header >X-Before-Proxy-Deferred true
s3proxy {
bucket "bubblx-storage"
region "eu-west-1"
}
header X-After-Proxy true
header >X-After-Proxy-Deferred true
}
That resulted in this.
GET /s3proxy HTTP/1.1
Accept: */*
Accept-Encoding: identity
Connection: keep-alive
Host: api.example.com
User-Agent: HTTPie/3.2.2
origin: https://origin.example.com
x-preflight: true
HTTP/1.1 200 OK
Alt-Svc: h3=":443"; ma=2592000
Content-Type: audio/wav
Date: Wed, 08 Nov 2023 17:23:52 GMT
Etag: "<ELIDED>"
Last-Modified: <ELIDED>
Server: Caddy
Transfer-Encoding: chunked
X-Before-Proxy: true
So only the non-deferred header, works, and only before the call to s3Proxy
, which is what I'd expect to see for the non-deferred headers if the middleware doesn't call the next middleware in the chain.
I suspect this is the issue.
What I don't understand is why it is return nil
— as a result I'd be wary of changing it.
I suppose I could leave the default as is, and extend pass_through
for the success case?
OK it's not that; it's that io.Copy
doesn't add the deferred headers. This… this actually makes sense, though I don't quite understand it.
As described in http.ResponseWriter
, the Write
method calls WriteHeader
, and the headers can't be changed afterwards — as in, writing to the ResponseWriter
prevents any additional headers from being added.
This is what io.Copy
does. But unlike for ResponseWriter.Write
, it doesn't add the deferred headers.
Or, at least that's what I infer, as calling ResponseWriter.WriteHeader
before the io.Copy
results in the deferred headers appearing, and calling after, or replacing the return nil
with return next.ServeHttp(w,r)
does not. I think the io.Copy
is necessary, as per go - How to stream an io.ReadCloser into a http.ResponseWriter - Stack Overflow, so I'll add the WriteHeader(http.StatusOk)
on the line before (it doesn't cause issues with errors).
Plausibly it ought to return next.StatusHttp(w,r)
as there could be subsequent middleware, but I shan't change that, for now.
Following the CORS configuration in https://github.com/caddyserver/website/issues/324#issuecomment-1629691031 I see that my endpoint using caddy-s3-proxy doesn't defer the response headers.
Could it be because this module doesn't callEdit: it's not, the code needs to callnext.ServeHttp(w, r)
in the success case?ResponseWriter.WriteHeaders
.This is blocking CORS requests from working, as the
Access-Control-Allow-Origin
header is not present on the response.I've sanitized the following output (from httpie).
This is the non–caddy-s3-proxy endpoint.
This is the caddy-s3-proxy one.
Note, in particular, the lack of the
Access-Control-Allow-Origin
header; this means that CORS requests do not work.I've read #33 but this seems different, as the headers should be set already (due to the
defer
).Does this sound like a feature that is missing? And if so, and you don't have an alternative that you recommend for CORS, would you consider this a bug?
In that case, I can dig through the code to try to fix this, but it might be an obvious solution to you — I've skimmed https://github.com/lindenlab/caddy-s3-proxy/blob/850db193cb7f48546439d236f2a6de7bd7436e2e/s3proxy.go and found the code calls
return nil
inServeHttp
.https://github.com/lindenlab/caddy-s3-proxy/blob/850db193cb7f48546439d236f2a6de7bd7436e2e/s3proxy.go#L361-L374
Should it be calling
return next.ServeHttp(w, r)
, as it does in the error case?https://github.com/lindenlab/caddy-s3-proxy/blob/850db193cb7f48546439d236f2a6de7bd7436e2e/s3proxy.go#L398-L402