Fix S3 routing in moto to allow upload of favicon.ico files#7868
Fix S3 routing in moto to allow upload of favicon.ico files#7868
Conversation
There was a problem hiding this comment.
LGTM! Thanks a lot for taking care of this, this feels like a proper edge case.
This new test will also reveal handy for testing the new provider matching on something else than s3.localhost.localstack.cloud, which I'm going to tackle next like you suggested. Thanks again!
note: the test will fail against the new provider, but I'm currently working on a PR to fix it.
LocalStack integration with Pro 2 files - 1 2 suites - 1 1h 37m 29s ⏱️ - 8m 19s Results for commit 22c3112. ± Comparison against base commit fd25513. ♻️ This comment has been updated with latest results. |
bentsku
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding the missing parameter in the docstring!
|
Changes Unknown when pulling 462e63a on s3-favicon into ** on master**. |
Fix S3 routing in moto to allow upload of
favicon.icofiles.This issue surfaces primarily for hostname-based bucket addressing for Pro users with DNS integration, if an S3 client is making a request to
<bucket>.s3.amazonaws.comdomains.Moto defines the following logic to handle
/favicon.icopaths as a special case:On a general note, I think we should verify if the routing to
<bucket>.s3.amazonaws.comdomains (i.e., defined via theHostHTTP header) is properly supported in the new S3 provider (for the different combinations of path-/host-based addressing). This is a fairly common use case for CloudFormation custom resources (e.g., via CDK templates). Would be great to get your help on this for the v2 switch @bentsku @thrau 🙌