Generate {application,text}/xml headers on gateway by client's choice #992
Conversation
If no header is specified, none is explicitly generated. Then the Go http server generates one by scanning content, it happens to be text/xml. If either of these headers is specified, generate that requested content type. If (only) other headers are generated, respond "Not Acceptable". This does not break backwards compatibility with clients that do not generate such a header (boto, mc). This should improve compatibility with a client that explicitly asks for application/xml (the Akka S3 client alpakka only accepts that Content-Type, so it had better ask for it). It *will* break a lying client that requests only anything-but-standard-XML.
|
Tested by capturing traffic (from the minio/mc client) to our gateway and adding various Should fix #987 , but ideally wait for customer confirmation that it does before pulling :-) |
|
@keimoon could you please take a look, too, and see whether this PR would fix your issue? |
Codecov Report
@@ Coverage Diff @@
## master #992 +/- ##
==========================================
+ Coverage 44.26% 44.72% +0.46%
==========================================
Files 140 147 +7
Lines 11570 11869 +299
==========================================
+ Hits 5121 5308 +187
- Misses 5799 5879 +80
- Partials 650 682 +32
Continue to review full report at Codecov.
|
|
Unfortunately, alpakka does not send |
Matches behavipours of S3 (verified by @ozkatz on encrypted with Postman) and minio (verified on plaintext with Wireshark)
|
Thanks! Also need to set the default to |
gateway/handler.go
Outdated
| @@ -358,6 +385,7 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| start := time.Now() | |||
| mrw := httputil.NewMetricResponseWriter(w) | |||
| setContentType(mrw, r) | |||
There was a problem hiding this comment.
Won't this return application/xml for non-xml responses? (i.e. GetObject?)
There was a problem hiding this comment.
Won't this return
application/xmlfor non-xml responses? (i.e.GetObject?)
Ouch, good catch! I'll need to figure out how to make go-swagger prefer application/xml :-(
|
Dismissed reviews because of Oz' issue with adding content types to files and heads from the backing store. Will re-request soon(-ish). |
All S3 gateway responses are application/xml, except when replying with contents from adapter. There keep the existing behaviour (Go http content type detection). We can add content-type forwarding to all our adapters, but that would be a separate change. Files returned from get-object keep their original Content-Type - auto-detected right now, but may change to forward from the block adapter if we decide to be more like S3 (in response to customer demand). Honour an Accepts header *if present*, allowing a client to prefer text/xml. Accepts headers are not usefully relevant to get-object, and continue to be ignored.
arielshaqed
force-pushed
the
feature/987-return-acceptable-content-type
branch
from
63fb096 to
7d4b24e
Compare
|
@johnnyaug @ozkatz PTAL, I cleaned up behaviour of |
gateway/handler.go
Outdated
| if ok { | ||
| defaultContentType := selectContentType(acceptable) | ||
| // No requested content type matched. This is OK at least for a GET or | ||
| // HEAD, so set up to auto-detect. |
There was a problem hiding this comment.
Rephrase comment, or put it somewhere else
gateway/handler.go
Outdated
| w.Header().Set("Content-Type", *defaultContentType) | ||
| } | ||
| } else { | ||
| // For all proxied content the type will be reset according to whatever headers |
There was a problem hiding this comment.
Also rephrase to clarify how this comment is related to this code
|
@ozkatz are we good to go with this? |
|
Thanks! |
If no header is specified, none is explicitly generated. Then the Go http server generates
one by scanning content, it happens to be text/xml. If either of these headers is
specified, generate that requested content type. If (only) other headers are generated,
respond "Not Acceptable".
This does not break backwards compatibility with clients that do not generate such a
header (boto, mc). This should improve compatibility with a client that explicitly asks for
application/xml (the Akka S3 client alpakka only accepts that Content-Type, so it had better
ask for it). It will break a lying client that requests only anything-but-standard-XML.