Skip to content

S3: fix trailing slash logic #12166

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

Merged
merged 1 commit into from
Jan 23, 2025
Merged

S3: fix trailing slash logic #12166

merged 1 commit into from
Jan 23, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jan 22, 2025

Motivation

We've got a reported in our Community Slack channel about issue with trailing slashes, especially with short key name (single characters). The trailing slashes would be missing:

awslocal s3api put-object --bucket sample-bucket2 --key "u/" --content-type "application/x-directory"
awslocal s3api list-objects-v2 --bucket sample-bucket2
...
       {
           "Key": "u",
           "LastModified": "2025-01-22T11:46:17.000Z",
           "ETag": "\"d41d8cd98f00b204e9800998ecf8427e\"",
           "Size": 0,
           "StorageClass": "STANDARD"
       },
...

I wrote a test for it and I had a hunch it came from our custom logic to parse URI parameters. We had custom logic for S3 because our router strips trailing slashes by default. We would check if the key name is in the path, and then split on the key and check if the leftover characters in the path are all slashes, to keep them.

However, to do so, we would partition the path on the parse Key parameter. Printing the result showed quite quickly what was the issue:

uri_params={'Bucket': 'test-bucket-ec3db341', 'Key': 'u'}
request.path.partition(uri_params["Key"])=('/test-b', 'u', 'cket-ec3db341/u/')

We would split/partition on the bucket name first while using path style. To avoid this, we need to start from the right side. That way, we properly get all the right side slashes.

Changes

  • add new tests for single characters with trailing slash
  • partition the trailing slashes the path by the Key, but starting from the right

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Jan 22, 2025
@bentsku bentsku added this to the 4.1 milestone Jan 22, 2025
@bentsku bentsku self-assigned this Jan 22, 2025
@bentsku bentsku requested a review from alexrashed as a code owner January 22, 2025 19:14
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   5m 6s ⏱️ +24s
443 tests +1  390 ✅ +1   53 💤 ±0  0 ❌ ±0 
886 runs  +2  780 ✅ +2  106 💤 ±0  0 ❌ ±0 

Results for commit cec157b. ± Comparison against base commit ed8c76e.

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 50m 27s ⏱️ - 2m 53s
4 005 tests +1  3 688 ✅ +1  317 💤 ±0  0 ❌ ±0 
4 007 runs  +1  3 688 ✅ +1  319 💤 ±0  0 ❌ ±0 

Results for commit cec157b. ± Comparison against base commit ed8c76e.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Whoof, nice catch! I guess this was pretty tricky to pin down! Nice, clean, and simple fix, coming with a nice clean and simple AWS validated snapshot test! 💯

Copy link
Member

Choose a reason for hiding this comment

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

praise: Nice and clean test! We are getting more and more edge cases covered by the S3 tests! 💯 ‏

@bentsku bentsku merged commit bdfdf15 into master Jan 23, 2025
47 checks passed
@bentsku bentsku deleted the fix-s3-trailing-slash branch January 23, 2025 11:56
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants