serverless-image-proxy icon indicating copy to clipboard operation
serverless-image-proxy copied to clipboard

add ETag to header output

Open dohomi opened this issue 6 years ago • 7 comments

Add ETag to header output based on s3 ETag Attempt to close #13 @schickling can you have a look? Thanks!

dohomi avatar Mar 05 '18 06:03 dohomi

@kbrandwijk what do you suggest? I think because the images basically will never change - because if you change the image on graphcool it will be a new file anyways - the hardcoded cache value basically let the file stay in cache more-less forever? I think thats alright in this context.

dohomi avatar Mar 06 '18 00:03 dohomi

You are actually right! But if resources never change, what's the use for using ETag? Also, when using ETag, if you would query the server with If-None-Match, it should return 304 without a body. Right now, it doesn't. So combined with the fact that files never change, and the hard cache-control value, I don't think this is the right way to deal with ETag.

kbrandwijk avatar Mar 06 '18 10:03 kbrandwijk

@kbrandwijk I want to get rid of downgrade in PageSpeed and YSlow due to missing Expired or ETag in the imageproxy. So what do you think is the right approach? Only show Last-Modified?

I was reading through several posts and it seems that IF-None-Match would still work if the date is set correctly?

As far as I understand if 304 is the response and the ETag hasnt changed the browser will fetch the correct image and extends the lifetime of the cached image based on the max-age. http://www.benhallbenhall.com/2012/03/http-codes-200-from-cache-304/

If the client has performed a conditional GET request and access is allowed, but the document has not been modified, the server SHOULD respond with this status code. The 304 response MUST NOT contain a message-body, and thus is always terminated by the first empty line after the header fields.

For me this is all new topic so I was reading through many blog posts to make sure doing the right thing.

https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching https://stackoverflow.com/questions/6350384/http-combining-expiration-and-validation-caching

dohomi avatar Mar 07 '18 01:03 dohomi

@dohomi You are mostly right. I'm just saying that in order to provide a meaningful implementation of the ETag header, the part that says: 'return a 304' should also be implemented, which isn't part of the implementation right now.

Also, returning the ETag from S3 might not be sufficient, because all the processing (cropping, resizing etc.) happens after the image is retrieved from S3. This would mean that different representations (based on the cropping/resizing arguments) would return the same ETag, which is also not right.

I like the idea of supporting ETag, but I think it should be supported properly, instead of just passing along a header from S3. In my opinion, these things are missing right now:

  • Generate a different ETag for different representations
  • Respond correctly to If-None-Match and other ETag-related requests (304 and more)

kbrandwijk avatar Mar 07 '18 10:03 kbrandwijk

@kbrandwijk you are right, I added the npm etag module to generate an accurate etag for the body.

To return a proper 304 we would need to extract the right header from the event. Then doing basically the same modification to the original image and check if the HTTP request ETag equals the generated etag(body). Do you know how to read out the headers correctly?

dohomi avatar Mar 07 '18 23:03 dohomi

I think I found a way. Could you have a look if that looks alright to you?

dohomi avatar Mar 08 '18 00:03 dohomi

@kbrandwijk @schickling I cloned this project and tested the behaviour on my aws account and it worked as expected. Anything I can do to get this merged?

dohomi avatar Mar 09 '18 07:03 dohomi