10000 fix S3 parser with empty integer query string parameters by bentsku · Pull Request #9603 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

fix S3 parser with empty integer query string parameters #9603

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 1 commit into from
Nov 13, 2023
Merged

fix S3 parser with empty integer query string parameters #9603

merged 1 commit into from
Nov 13, 2023

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Nov 10, 2023 •
edited
Loading

Motivation

When scrolling over my GitHub homepage, I stumbled upon this (edit: apparently dead) library: https://github.com/beyondstorage/go-storage

(Story: https://xuanwo.io/en-us/2023/01-beyond-storage-why-we-failed/, all commits are from dependabot now 😅 and it seems OpenDAL (repo) is superseding the project, I've ran their Rust test suite (106 tests!) and it's all green 🎉)

It seems it's has been sponsored by Vercel, and I saw they had coverage for S3, and an automated in CI integration test suite.
I've forked the repo and ran the suite locally with LocalStack 😄 I saw they had support for minio too.

I've ran it locally and found only one test failure:

  When ListMultiPart 
      ListMultipart error should be nil ✔✔
      Next error should be nil ✘
      The part number and size should be match 🔥

With the following trace:

    File "/Users/benjaminsimon/Projects/localstack/localstack/localstack/aws/protocol/parser.py", line 266, in _parse_shape
      raise ProtocolParserError(
  localstack.aws.protocol.parser.ProtocolParserError: Invalid type when parsing PartNumberMarker: '' cannot be parsed to integer.

I've checked the request we received:

GET s3.localhost.localstack.cloud:4566/test/3452e591-f8a8-4bc6-ac1c-7f98e4a045c9/e2e0d2d4-ecc1-40f1-a721-0545dee865c8?max-parts=200&part-number-marker=&uploadId=lEfzr1V8eelmpBbNK_jZfPZps2I3fQjspBA_WzlIISL8_6nY8N026B16MYT0kRB7r3G4nxTWCraDKtbHE6vM5ZXCbzFx6D-wnikrY96R3fHebKq662uvnukbdcmf4JXx&x-id=ListParts
# note the ?max-parts=200&part-number-marker=&...`

So it seems S3 accepts empty query string parameters that are of integer type in the specs.

After fixing the issue, here's the test result (87 successful tests!):

Test Results

$ make integration_test

go test -count=1 -race -covermode=atomic -v ./tests
=== RUN   TestStorage
    utils_test.go:15: Setup test for s3
=== RUN   TestStorage/TestDelete
=== RUN   TestStorage/TestDelete/TestDelete
=== RUN   TestStorage/TestList
=== RUN   TestStorage/TestList/TestList
=== RUN   TestStorage/TestList/TestListEmptyDir
=== RUN   TestStorage/TestList/TestListWithoutListMode
=== RUN   TestStorage/TestMetadata
=== RUN   TestStorage/TestPath
=== RUN   TestStorage/TestPath/TestAbsPath
=== RUN   TestStorage/TestPath/TestBackslash
=== RUN   TestStorage/TestRead
=== RUN   TestStorage/TestRead/TestRead
=== RUN   TestStorage/TestRead/TestReadWithIoCallback
=== RUN   TestStorage/TestRead/TestReadWithOffset
=== RUN   TestStorage/TestRead/TestReadWithSize
=== RUN   TestStorage/TestRead/TestReadWithSizeAndOffset
=== RUN   TestStorage/TestStat
=== RUN   TestStorage/TestStat/TestStat
=== RUN   TestStorage/TestString
=== RUN   TestStorage/TestWrite
=== RUN   TestStorage/TestWrite/TestWrite
=== RUN   TestStorage/TestWrite/TestWriteViaNilReader
=== RUN   TestStorage/TestWrite/TestWriteViaNilReaderAndValidSize
=== RUN   TestStorage/TestWrite/TestWriteViaValidReaderAndZeroSize
=== RUN   TestStorage/TestWrite/TestWriteWithIoCallback
=== RUN   TestStorage/TestWrite/TestWriteWithSize
--- PASS: TestStorage (0.82s)
    --- PASS: TestStorage/TestDelete (0.15s)
        --- PASS: TestStorage/TestDelete/TestDelete (0.15s)
    --- PASS: TestStorage/TestList (0.06s)
        --- PASS: TestStorage/TestList/TestList (0.03s)
        --- PASS: TestStorage/TestList/TestListEmptyDir (0.01s)
        --- PASS: TestStorage/TestList/TestListWithoutListMode (0.01s)
    --- PASS: TestStorage/TestMetadata (0.00s)
    --- PASS: TestStorage/TestPath (0.12s)
        --- PASS: TestStorage/TestPath/TestAbsPath (0.02s)
        --- PASS: TestStorage/TestPath/TestBackslash (0.10s)
    --- PASS: TestStorage/TestRead (0.23s)
        --- PASS: TestStorage/TestRead/TestRead (0.07s)
        --- PASS: TestStorage/TestRead/TestReadWithIoCallback (0.06s)
        --- PASS: TestStorage/TestRead/TestReadWithOffset (0.03s)
        --- PASS: TestStorage/TestRead/TestReadWithSize (0.05s)
        --- PASS: TestStorage/TestRead/TestReadWithSizeAndOffset (0.02s)
    --- PASS: TestStorage/TestStat (0.05s)
        --- PASS: TestStorage/TestStat/TestStat (0.05s)
    --- PASS: TestStorage/TestString (0.00s)
    --- PASS: TestStorage/TestWrite (0.20s)
        --- PASS: TestStorage/TestWrite/TestWrite (0.01s)
        --- PASS: TestStorage/TestWrite/TestWriteViaNilReader (0.04s)
        --- PASS: TestStorage/TestWrite/TestWriteViaNilReaderAndValidSize (0.01s)
        --- PASS: TestStorage/TestWrite/TestWriteViaValidReaderAndZeroSize (0.02s)
        --- PASS: TestStorage/TestWrite/TestWriteWithIoCallback (0.10s)
        --- PASS: TestStorage/TestWrite/TestWriteWithSize (0.02s)
=== RUN   TestMultiparter
    utils_test.go:15: Setup test for s3

  Given a basic Storager 
    When CreateMultipart 
      The first returned error should be nil ✔
      The second returned error also should be nil ✔
      The Object Mode should be part ✔✔
      The Object must have multipart id ✔
    When Delete with multipart id 
      The first returned error should be nil ✔
      The second returned error also should be nil ✔
    When Stat with multipart id 
      The error should be nil ✔✔
      The Object Mode should be part ✔✔
      The Object must have multipart id ✔✔
    When Create with multipart id 
      The Object Mode should be part ✔✔
      The Object must have multipart id ✔✔
    When WriteMultipart 
      The error should be nil ✔
      The part should not be nil ✔
      The size should be match ✔
    When ListMultiPart 
      ListMultipart error should be nil ✔✔
      Next error should be nil ✔✔
      The part number and size should be match ✔✔
    When List with part type 
      The error should be nil ✔
      The iterator should not be nil ✔
      Next error should be nil ✔✔
      The path and multipart id should be match ✔✔✔✔
    When CompletePart 
      The error should be nil ✔
      The object should be readable after complete ✔✔✔


38 total assertions

--- PASS: TestMultiparter (0.72s)
=== RUN   TestDirer
    utils_test.go:15: Setup test for s3

  Given a basic Storager 
    When CreateDir 
      The first returned error should be nil ✔
      The second returned error also should be nil ✔
      The Object Path should equal to the input path ✔
      The Object Mode should be dir ✔
    When Create with ModeDir 
      The Object Path should equal to the input path ✔
      The Object Mode should be dir ✔
    When Stat with ModeDir 
      The error should be nil ✔
      The Object Path should equal to the input path ✔
      The Object Mode should be dir ✔
    When Delete with ModeDir 
      The first returned error should be nil ✔
      The second returned error also should be nil ✔


49 total assertions

--- PASS: TestDirer (0.06s)
=== RUN   TestLinker
    utils_test.go:15: Setup test for s3

  Given a basic Storager 
    When create a link object 
      The error should be nil ✔
      The object mode should be link ✔
      The linkTarget of the object must be the same as the target ✔✔
      Stat should get path object without error 
        The error should be nil ✔
        The object mode should be link ✔
        The linkTarget of the object must be the same as the target ✔✔
    When create a link object from a not existing target 
      The error should be nil ✔
      The object mode should be link ✔
      The linkTarget of the object must be the same as the target ✔✔
      Stat should get path object without error 
        The error should be nil ✔
        The object mode should be link ✔
        The linkTarget of the object must be the same as the target ✔✔
    When CreateLink to an existing path 
      The first returned error should be nil ✔
      The second returned error should also be nil ✔
      The object mode should be link ✔
      The linkTarget of the object must be the same as the secondTarget ✔✔


70 total assertions

--- PASS: TestLinker (0.81s)
=== RUN   TestHTTPSigner
    utils_test.go:15: Setup test for s3

  Given a basic Storager 
    When Write via QuerySignHTTPWrite 
      The error should be nil ✔✔✔
      The request returned error should be nil ✔
      Read should get object data without error 
        The content should be match ✔✔✔✔


78 total assertions

    utils_test.go:15: Setup test for s3

  Given a basic Storager 
    When Read via QuerySignHTTPRead 
      The error should be nil ✔✔✔
      The request returned error should be nil ✔✔
      The content should be match ✔✔✔✔


87 total assertions

--- PASS: TestHTTPSigner (1.96s)
=== RUN   TestIssue741
    utils_test.go:15: Setup test for s3
--- PASS: TestIssue741 (0.00s)
PASS
coverage: [no statements]
ok  	github.com/beyondstorage/go-storage/services/s3/v3/tests	6.009s	coverage: [no statements]

I'll validate the pre-signed requests, for now they pass because we disable validations but I believe they would fail and I'll need to investigate.

Changes

  • I've created a test with a parameter validation disabled boto client to validate the behavior (something we should start doing more often to properly validate parsing of our requests).
  • Modified the S3 parser to accept empty query string parameters. It might be worth to verify in a follow-up if other services would accept it, and if it should be global behavior in our parser.
  • change the condition for some xfail: it does not take a callable as input and will treat it as True, unlike skip_snapshot_verify

Testing

You can also run the test suite by cloning the repo, navigating to ./services/s3 and for ease set an Makefile.env with the following:

export STORAGE_S3_INTEGRATION_TEST=on
export STORAGE_S3_CREDENTIAL=hmac:test:test
export STORAGE_S3_NAME=test
export STORAGE_S3_LOCATION=us-east-1
export STORAGE_S3_ENDPOINT=http:s3.localhost.localstack.cloud:4566

Start LocalStack, run awslocal s3 mb s3://test to create the test bucket.
Then you can run make integration_test and see it go ✅

For the OpenDAL test suite, you need to checkout: https://github.com/apache/incubator-opendal
Then navigate to core.
Run awslocal s3 mb s3://opendal-testing
(Install cargo-nextest if you don't have it: https://nexte.st/book/pre-built-binaries.html)
Create and load the following .env file:

export OPENDAL_TEST=s3
export OPENDAL_S3_ACCESS_KEY_ID=test
export OPENDAL_S3_ROOT=CI/
export OPENDAL_S3_ACCESS_KEY_ID=test
export OPENDAL_S3_BUCKET=opendal-testing
export OPENDAL_S3_REGION=us-east-1
export OPENDAL_S3_ENDPOINT=http://s3.localhost.localstack.cloud:4566

Run cargo nextest run behavior --features tests
And see the following:

OpenDAL Results 
Finished test [unoptimized + debuginfo] target(s) in 0.13s
    Starting 106 tests across 2 binaries (132 skipped)
        PASS [   0.462s] opendal::behavior behavior::test_blocking_copy_non_existing_source
        PASS [   0.512s] opendal::behavior behavior::test_blocking_copy_source_dir
        PASS [   0.560s] opendal::behavior behavior::test_blocking_create_dir_existing
        PASS [   0.580s] opendal::behavior behavior::test_blocking_copy_target_dir
        PASS [   0.583s] opendal::behavior behavior::test_blocking_create_dir
        PASS [   0.598s] opendal::behavior behavior::test_blocking_copy_self
        PASS [   0.158s] opendal::behavior behavior::test_blocking_list_non_exist_dir
        PASS [   0.627s] opendal::behavior behavior::test_blocking_list_dir_with_metakey
        PASS [   0.631s] opendal::behavior behavior::test_blocking_delete_file
        PASS [   0.648s] opendal::behavior behavior::test_blocking_copy_file
        PASS [   0.767s] opendal::behavior behavior::test_blocking_list_dir
        PASS [   0.774s] opendal::behavior behavior::test_blocking_fuzz_range_reader
        PASS [   0.812s] opendal::behavior behavior::test_blocking_list_dir_with_metakey_complete
        PASS [   0.241s] opendal::behavior behavior::test_blocking_read_not_exist
        PASS [   0.876s] opendal::behavior behavior::test_blocking_copy_nested
        PASS [   0.281s] opendal::behavior behavior::test_blocking_stat_dir
        PASS [   0.341s] opendal::behavior behavior::test_blocking_remove_all
        PASS [   0.317s] opendal::behavior behavior::test_blocking_scan
        PASS [   0.308s] opendal::behavior behavior::test_blocking_stat_file
        PASS [   0.983s] opendal::behavior behavior::test_blocking_fuzz_part_reader
        PASS [   0.219s] opendal::behavior behavior::test_blocking_stat_not_exist
        PASS [   0.246s] opendal::behavior behavior::test_blocking_stat_with_special_chars
        PASS [   0.407s] opendal::behavior behavior::test_blocking_remove_one_file
        PASS [   0.565s] opendal::behavior behavior::test_blocking_read_full
        PASS [   1.076s] opendal::behavior behavior::test_blocking_fuzz_offset_reader
        PASS [   0.198s] opendal::behavior behavior::test_check
        PASS [   0.316s] opendal::behavior behavior::test_blocking_write_file
        PASS [   1.135s] opendal::behavior behavior::test_blocking_copy_overwrite
        PASS [   0.615s] opendal::behavior behavior::test_blocking_read_large_range
        PASS [   0.389s] opendal::behavior behavior::test_blocking_write_with_dir_path
        PASS [   0.652s] opendal::behavior behavior::test_blocking_read_range
        PASS [   0.411s] opendal::behavior behavior::test_blocking_write_with_special_chars
        PASS [   0.307s] opendal::behavior behavior::test_copy_non_existing_source
        PASS [   0.332s] opendal::behavior behavior::test_copy_source_dir
        PASS [   0.477s] opendal::behavior behavior::test_copy_file_with_ascii_name
        PASS [   0.357s] opendal::behavior behavior::test_create_dir
        PASS [   0.359s] opendal::behavior behavior::test_create_dir_existing
        PASS [   0.448s] opendal::behavior behavior::test_copy_self
        PASS [   0.350s] opendal::behavior behavior::test_delete_empty_dir
        PASS [   0.334s] opendal::behavior behavior::test_delete_not_existing
        PASS [   0.439s] opendal::behavior behavior::test_copy_target_dir
        PASS [   0.390
8000
s] opendal::behavior behavior::test_delete_file
        PASS [   0.605s] opendal::behavior behavior::test_copy_nested
        PASS [   0.300s] opendal::behavior behavior::test_fuzz_issue_2717
        PASS [   0.405s] opendal::behavior behavior::test_delete_with_special_chars
        PASS [   0.306s] opendal::behavior behavior::test_fuzz_pr_3395_case_1
        PASS [   0.784s] opendal::behavior behavior::test_copy_file_with_non_ascii_name
        PASS [   0.308s] opendal::behavior behavior::test_fuzz_pr_3395_case_2
        PASS [   0.780s] opendal::behavior behavior::test_copy_overwrite
        PASS [   0.334s] opendal::behavior behavior::test_list_dir_with_file_path
        PASS [   0.569s] opendal::behavior behavior::test_fuzz_offset_reader
        PASS [   0.334s] opendal::behavior behavior::test_list_empty_dir
        PASS [   0.406s] opendal::behavior behavior::test_list_dir_with_metakey
        PASS [   0.589s] opendal::behavior behavior::test_fuzz_part_reader
        PASS [   0.452s] opendal::behavior behavior::test_list_dir
        PASS [   0.337s] opendal::behavior behavior::test_list_nested_dir
        PASS [   0.509s] opendal::behavior behavior::test_invalid_reader_seek
        PASS [   0.271s] opendal::behavior behavior::test_list_non_exist_dir
        PASS [   0.276s] opendal::behavior behavior::test_list_sub_dir
        PASS [   0.283s] opendal::behavior behavior::test_list_with_start_after
        PASS [   0.562s] opendal::behavior behavior::test_list_dir_with_metakey_complete
        PASS [   0.712s] opendal::behavior behavior::test_fuzz_reader_with_range
        PASS [   0.997s] opendal::behavior behavior::test_delete_stream
        PASS [   0.320s] opendal::behavior behavior::test_read_not_exist
        PASS [   0.382s] opendal::behavior behavior::test_read_full
        PASS [   0.326s] opendal::behavior behavior::test_read_with_dir_path
        PASS [   0.631s] opendal::behavior behavior::test_list_rich_dir
        PASS [   0.379s] opendal::behavior behavior::test_read_with_if_none_match
        PASS [   0.405s] opendal::behavior behavior::test_read_with_if_match
        PASS [   0.483s] opendal::behavior behavior::test_read_large_range
        PASS [   0.549s] opendal::behavior behavior::test_read_range
        PASS [   0.226s] opendal::behavior behavior::test_remove_all
        PASS [   0.209s] opendal::behavior behavior::test_scan
        PASS [   0.187s] opendal::behavior behavior::test_scan_root
        PASS [   0.328s] opendal::behavior behavior::test_reader_range
        PASS [   0.260s] opendal::behavior behavior::test_remove_one_file
        PASS [   0.428s] opendal::behavior behavior::test_reader_from
        PASS [   0.186s] opendal::behavior behavior::test_stat_dir
        PASS [   0.416s] opendal::behavior behavior::test_reader_tail
        PASS [   0.211s] opendal::behavior behavior::test_stat_not_exist
        PASS [   0.624s] opendal::behavior behavior::test_read_with_special_chars
        PASS [   0.223s] opendal::behavior behavior::test_stat_root
        PASS [   0.387s] opendal::behavior behavior::test_stat_file
        PASS [   0.257s] opendal::behavior behavior::test_write_only
        PASS [   0.177s] opendal::behavior behavior::test_write_with_content_disposition
        PASS [   0.406s] opendal::behavior behavior::test_stat_not_cleaned_path
        PASS [   0.302s] opendal::behavior behavior::test_stat_with_special_chars
        PASS [   0.403s] opendal::behavior behavior::test_stat_with_if_none_match
        PASS [   0.456s] opendal::behavior behavior::test_stat_with_if_match
        PASS [   0.299s] opendal::behavior behavior::test_write_with_content_type
        PASS [   0.364s] opendal::behavior behavior::test_write_with_cache_control
        PASS [   0.218s] opendal::behavior behavior::test_write_with_empty_content
        PASS [   0.285s] opendal::behavior behavior::test_write_with_dir_path
        PASS [   1.400s] opendal::behavior behavior::test_presign_stat
        PASS [   0.246s] opendal::behavior behavior::test_writer_abort
        PASS [   1.354s] opendal::behavior behavior::test_presign_write
        PASS [   1.276s] opendal::behavior behavior::test_read_with_override_cache_control
        PASS [   1.148s] opendal::behavior behavior::test_read_with_override_content_type
        PASS [   1.212s] opendal::behavior behavior::test_read_with_override_content_disposition
        PASS [   0.336s] opendal::behavior behavior::test_write_with_special_chars
        PASS [   1.574s] opendal::behavior behavior::test_presign_read
        PASS [   1.525s] opendal::behavior behavior::test_writer_copy
        PASS [   1.488s] opendal::behavior behavior::test_writer_sink
        PASS [   1.471s] opendal::behavior behavior::test_writer_write
        PASS [   1.621s] opendal::behavior behavior::test_writer_futures_copy
        PASS [   5.758s] opendal::behavior behavior::test_fuzz_unsized_writer
------------
     Summary [   7.229s] 106 tests run: 106 passed, 132 skipped

@bentsku bentsku added aws:s3 Amazon Simple Storage Service area: asf semver: patch Non-breaking changes which can be included in patch releases labels Nov 10, 2023
@bentsku bentsku self-assigned this Nov 10, 2023
@coveralls
Copy link
coveralls commented Nov 11, 2023 •
edited
Loading

Coverage Status

coverage: 84.011% (+0.002%) from 84.009%
when pulling 82f9771 on run-s3-go-storage-tests
into 78999d0 on master.

@github-actions GitHub Actions
Copy link
github-actions bot commented Nov 11, 2023 •
edited
Loading

LocalStack Community integration with Pro

       2 files         2 suites   1h 13m 43s ⏱️
2 308 tests 2 010 ✔️ 298 💤 0 ❌
2 309 runs  2 010 ✔️ 299 💤 0 ❌

Results for commit 82f9771a.

♻️ This comment has been updated with latest results.

This was referenced Nov 11, 2023
Merged
Merged
@bentsku bentsku added this to the 3.0 milestone Nov 12, 2023
alexrashed
alexrashed approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member
@alexrashed alexrashed 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.

A really nice catch of another nitty-gritty detail of S3 parsing! 💯
It's awesome to see that you can basically run anything against our S3 implementation, and there's only some really minor details we need to iron out (and with every single fix we're getting even closer to absolute parity with AWS). 🚀

@bentsku bentsku force-pushed the run-s3-go-storage-tests branch from 2681e3c to 82f9771 Compare November 13, 2023 10:03
@bentsku bentsku merged commit 0750764 into master Nov 13, 2023
@bentsku bentsku deleted the run-s3-go-storage-tests branch November 13, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

Assignees

@bentsku bentsku

Labels
area: asf aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
3.0
Development

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