-
Notifications
You must be signed in to change notification settings - Fork 535
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
Image expires header #349
base: main
Are you sure you want to change the base?
Image expires header #349
Conversation
247f335
to
d9877bc
Compare
Co-Authored-By: Markus Horst Becker <[email protected]>
d9877bc
to
d4602fc
Compare
@markuscolourbox thank you for your contribution. We will look into it and add it to our backlog. |
Hi @markuscolourbox Thanks for the excellent PR. It's entirely possible I'm wrong. I think it would be great if adding |
We considered this too. The reason we went with the approach we did, is only that the signature so far is only created over the path (the base64 encoded json). This would make the expire field the first & only query parameter to be part of the signature. If this is preferred, I can adjust the PR. |
Does anyone have any opinions on what the header(s) should be called. If we followed the s3 style we could have paths like: (Current branch state is WIP, will return after a decision on this was made) |
@markuscolourbox I prefer the S3 style, purely for ease of debugging as it makes the two values obvious. But either work for me - just looking forward to this feature getting included! |
@fisenkodv what is the ETA of getting PR accepted. As noted by @markuscolourbox we are open to change the format to match AWS's preferences - just let us know. |
This pr has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
@aws-solutions-github-bot Please leave open |
Hi @markuscolourbox, I'm evaluating this change now, and so far I think it makes sense as an addition in the Serverless Image Handler. My main concern here is that including the expires field will cause a notable decrease to the cache hit rate, requiring that we reprocess an image even if the previously generated version isn't expired yet (Since the signature/expires parameters would change and are used when caching). This isn't a deal breaker since it would only affect customers using this feature but is worth noting. Let me know if you have any thoughts, |
Hi @markuscolourbox, Thanks for your contributions to SIH :) |
Issue #, if available:
#345
Description of changes:
Expires
response headerBy setting the
{ "headers": { "expires": "..." } }
value to a UTCTimeString the link can be made time-limited. Useful when combined with signed urls. The setexpires
takes precedence over S3 Object expires and expired requests get served 403 Forbidden.We placed the expiry date in the
headers
section because of semantics, moving it to the root of the request JSON, or renaming it would be simple.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.