-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
S3: add heuristic for S3 requests from IaC/SDK with no prefix #12678
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
Draft
bentsku
wants to merge
4
commits into
main
Choose a base branch
from
fix-cdk-s3-vhost
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+52
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 32s ⏱️ Results for commit 78e5c8a. |
Test Results - Alternative Providers597 tests 420 ✅ 14m 41s ⏱️ Results for commit 78e5c8a. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 24m 0s ⏱️ Results for commit 78e5c8a. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
aws:s3
Amazon Simple Storage Service
semver: patch
Non-breaking changes which can be included in patch releases
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR implements a small workaround for S3 Virtual-Hosted requests done without an
s3.
prefixed endpoint.We sometimes have the case that we cannot update the S3-only endpoint URL for some tools interacting with LocalStack, like IaC (CDK, for ex.) or SDKs.
For context, we implemented a prefixed endpoint for S3 to make sure we can properly parse the request, and know what is the bucket name in the host (anything before the
s3.
prefix, if there is anything, otherwise, we fallback to the path style access).This can lead to issues where a request done to
http://test-bucket.localhost.localstack.cloud:4566/test-key
would not be properly parsed, because S3 would not try to parse the bucket name out of the host, and seetest-key
as the bucket name.This workaround allows the S3 parser to parse the request if and only if there is something before the
LOCALSTACK_HOST
environment variable. This can enable some use-cases, but not all. We're still not entirely sure this could not lead to more complications, like requests being interpreted as virtual-hosted when not (what about thelocation
service, which could have an endpoint namedhttp://location.localhost.localstack.cloud:4566
, this request would be directed to thelocation
service even though it could be a valid bucket name. Ordeadline
)We can park this work for now and revisit if we have a strong need for it. It was validated that it did fix some simple use-cases, but it has some other limitations:
\cc @joe4dev
Changes
LOCALSTACK_HOST
value and be able to parse the requestTODO
We still need to add some tests around the limitations to have them documented and tested