8000 Fix S3 routing in moto to allow upload of favicon.ico files by whummer · Pull Request #7868 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Fix S3 routing in moto to allow upload of favicon.ico files #7868

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 15, 2023
Merged

Fix S3 routing in moto to allow upload of favicon.ico files #7868

merged 4 commits into from
Mar 15, 2023

Conversation

whummer
Copy link
Member
@whummer whummer commented Mar 14, 2023 •
edited
Loading

Fix S3 routing in moto to allow upload of favicon.ico files.

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.com domains.

Moto defines the following logic to handle /favicon.ico paths as a special case:

if path_info.startswith("/moto-api") or path_info == "/favicon.ico":
    host = "moto_api"

On a general note, I think we should verify if the routing to <bucket>.s3.amazonaws.com domains (i.e., defined via the Host HTTP 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 🙌

@whummer whummer requested review from thrau and bentsku March 14, 2023 21:16
@whummer whummer temporarily deployed to localstack-ext-tests March 14, 2023 21:16 — with GitHub Actions Inactive
bentsku
bentsku approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor
@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bentsku bentsku mentioned this pull request Mar 14, 2023
Merged
@github-actions
Copy link
github-actions bot commented Mar 14, 2023 •
edited
Loading

LocalStack integration with Pro

       2 files   -     1         2 suites   - 1   1h 37m 29s ⏱️ - 8m 19s
1 799 tests +    1  1 414 ✔️  -     2  385 💤 +    3  0 ❌ ±0 
2 154 runs   - 370  1 589 ✔️  - 201  565 💤  - 169  0 ❌ ±0 

Results for commit 22c3112. ± Comparison against base commit fd25513.

♻️ This comment has been updated with latest results.

@whummer whummer temporarily deployed to localstack-ext-tests March 14, 2023 23:36 — with GitHub Actions Inactive
@whummer whummer temporarily deployed to localstack-ext-tests March 15, 2023 00:23 — with GitHub Actions Inactive
@whummer whummer requested a review from macnev2013 as a code owner March 15, 2023 11:00
@whummer whummer temporarily deployed to localstack-ext-tests March 15, 2023 11:01 — with GitHub Actions Inactive
@whummer whummer temporarily deployed to localstack-ext-tests March 15, 2023 11:05 — with GitHub Actions Inactive
bentsku
bentsku approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor
@bentsku bentsku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding the missing parameter in the docstring!

@whummer whummer merged commit 080d53e into master Mar 15, 2023
@whummer whummer deleted the s3-favicon branch March 15, 2023 11:47
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 462e63a on s3-favicon into ** on master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@thrau thrau Awaiting requested review from thrau

@macnev2013 macnev2013 Awaiting requested review from macnev2013

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@whummer @coveralls @bentsku
0