search

lindenlab / caddy-s3-proxy

s3 proxy plugin for caddy
Apache License 2.0
72 stars 22 forks source link

S3 StatusCode: 304 is handled as errr #35

Closed christoph-kluge closed 3 years ago

christoph-kluge commented 3 years ago

After performing a small canary deployment with a little bit more load and use-cases I've discovered that a s3 may response with 304 response. Interesstingly this is handled as error and fallback's to pass_through. 

Nov 17 00:20:34 ip-172-20-141-199 caddy: {"level":"error","ts":1605572434.37534,"logger":"http.handlers.s3proxy","msg":"failed to get object","bucket":"XXXXXXXXXXXXXX","key":"www.customer1.de/vorstand","err":"NotModified: Not Modified\n\tstatus code: 304, request id: BP0WDTEY4RCKDV3J, host id: Utt9aT83RCxch2R79muG+HDjQJ5HfnRIqxz4UArEN0s22h3z7nikhS592xQuHIhYy2IZhD5FIh8="}
rayjlinden commented 3 years ago

Ok - I put in something I think will fix this issue. There are a set of errors one should probably not pass through because they were status that are potentially expected like like 304, or a problem in the calling code - like 412. I really need to have a better comprehensive mapping of aws error codes to http statuses...

I see why the caddy fileserver module only will ever pass through 404 errors... But I think it seems reasonable to pass through other types of errors - no? Like 403 or 500? Or maybe it isn't...

Thoughts?

christoph-kluge commented 3 years ago

I've checked your fix (I forgot to file a new PR for this last night) and saw that we solved it a little bit differently. You can check my version here: https://github.com/christoph-kluge/caddy-s3-proxy/commit/e6e556244cb660a237ee5e844834671eb0b75d42

Anyway I was curious if 304 is even possible for a GET response and this is what the RFC states:

An origin server MUST NOT perform the requested method if the condition evaluates to false; instead, the origin server MUST respond with either a) the 304 (Not Modified) status code if the request method is GET or HEAD or b) the 412 (Precondition Failed) status code for all other request methods.

I also need to understand the golang-code a little bit better. I'm still not yet sure where the difference is between a normal error and an awserror and the caddyerror. Not sure if I can give a qualified answer here:

As soon as I understand your idea: then I would perhaps pass all s3 status codes/responses to caddy unless pass_through takes place. The most important thingy is that misses or errors from s3 are logged correctly.

rayjlinden commented 3 years ago

304 and 412 is no longer allowed to pass-through. Not sure if others should be handled as well.

This could be handled by only allowing 404 errors to pass through.

If other errors should not pass through. Open a new ticket.

sweepies commented 3 years ago

I'm still getting these on 0.5.5.


caddy    | {"level":"error","ts":1610345719.462164,"logger":"http.handlers.s3proxy","msg":"failed to get object","bucket":"(redacted)","key":"/index.jpg","err":"{id=7enzuen59} caddy-s3-proxy.convertToCaddyError (errors.go:113): HTTP 304: NotModified: Not Modified\n\tstatus code: 304, request id: , host id:"}
caddy    | {"level":"error","ts":1610345719.5899541,"logger":"http.handlers.reverse_proxy","msg":"aborting with incomplete response","error":"http: request method or response status code does not allow body"}