Skip to content
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

Fix object_store multipart uploads on S3 Compatible Stores #2731

Merged
merged 1 commit into from Sep 15, 2022

Conversation

mildbyte
Copy link
Contributor

Which issue does this PR close?

I couldn't find an issue for this in the arrow-rs repo. Some investigation here: splitgraph/seafowl#112

Rationale for this change

The official Minio SDK uses /?uploads= as the URL when it initiates a multipart upload instead of /?uploads (source). This affects the query param string that goes into the AWSV4 signature and causes object_store to fail a signature check when initiating a multipart upload to Minio.

NOTE: This is a deviation from the AWS S3 REST API docs: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html#API_CreateMultipartUpload_RequestSyntax (which do use /?uploads). I haven't tested that this doesn't now break S3 multipart uploads.

As an alternative, we could instead alter the query param string in the signature itself, though from a very quick glance at the Minio source code it looks like it uses the query string from the original request URL.

What changes are included in this PR?

Change the URL to initiate a multipart upload to have ?uploads= instead of ?uploads.

Are there any user-facing changes?

None

The official Minio SDK uses "uploads=" as the URL when it initiates a
multipart upload instead of "uploads". This affects the AWSV4 signature
and causes object_store to fail a signature check when initiating the
upload to Minio.

It's possible that this contradicts the AWS S3 API docs:
https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html#API_CreateMultipartUpload_RequestSyntax
and we need to instead keep the URL as `?uploads` and
change the URL that goes into the signature instead.
@github-actions github-actions bot added the object-store Object Store Interface label Sep 15, 2022
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Turns out this is also needed for communicating with S3, should probably investigate more thorough integration testing for this

@tustvold tustvold changed the title Fix object_store multipart uploads on Minio Fix object_store multipart uploads on S3 Compatible Stores Sep 15, 2022
@tustvold tustvold merged commit 5f441ee into apache:master Sep 15, 2022
@tustvold
Copy link
Contributor

Thank you 😄

@ursabot
Copy link

ursabot commented Sep 15, 2022

Benchmark runs are scheduled for baseline = fb01656 and contender = 5f441ee. 5f441ee is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@mildbyte mildbyte deleted the bugfix/multipart-uploads-minio branch September 16, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants