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.

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

Fix bug in multipart upload s3 #7512

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Fix bug in multipart upload s3 #7512

merged 1 commit into from
Feb 26, 2024

Conversation

idanovo
Copy link
Contributor

@idanovo idanovo commented Feb 25, 2024 •
edited
Loading

No description provided.

@idanovo idanovo self-assigned this Feb 25, 2024
@idanovo idanovo removed the request for review from nopcoder February 25, 2024 16:16
@idanovo idanovo linked an issue Feb 25, 2024 that may be closed by this pull request
[Bug]: lakectl fs upload fails to upload large files after 15 minutes when using presigned mode #7501
Closed
@itaiad200
Copy link
Contributor

Can you elaborate on how this connects to the issue?

@github-actions GitHub Actions
Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

@idanovo idanovo added the include-changelog PR description should be included in next release changelog label Feb 25, 2024
@idanovo
Copy link
Contributor Author

idanovo commented Feb 25, 2024

@arielshaqed @itaiad200 I've looked at s3 docs and it seems like pre-signed URLs can have an expiration of up to 7 days.
That means that changing the value for the blockstore.s3.pre_signed_expiry config flag should solve this issue.
Verified by running lakeFS locally with s3 object storage.
We just forgot to pass this value to the CreateMultipartUpload call.

@itaiad200
Copy link
Contributor

@arielshaqed @itaiad200 I've looked at s3 docs and it seems like pre-signed URLs can have an expiration of up to 7 days. That means that changing the value for the blockstore.s3.pre_signed_expiry config flag should solve this issue. Verified by running lakeFS locally with s3 object storage. We just forgot to pass this value to the CreateMultipartUpload call.

  1. I remember @arielshaqed saying this is not something users can easily change for their aws account.
  2. Can we add test for this field? If we missed it once we can miss it again.

@idanovo
Copy link
Contributor Author

idanovo commented Feb 25, 2024 •
edited
Loading

@arielshaqed @itaiad200 I've looked at s3 docs and it seems like pre-signed URLs can have an expiration of up to 7 days. That means that changing the value for the blockstore.s3.pre_signed_expiry config flag should solve this issue. Verified by running lakeFS locally with s3 object storage. We just forgot to pass this value to the CreateMultipartUpload call.

  1. I remember @arielshaqed saying this is not something users can easily change for their aws account.
  2. Can we add test for this field? If we missed it once we can miss it again.
  1. I think he have meant for something else. There's a different flag that we use to generate web tokens for the AWS account we use. Anyway, even if it is something that users can't easily configure, the current behaviour is a bug that should be fixed.
  2. I'm not exactly sure what there's to test. This is a specific call we make for s3. If someone create a new logic that calls s3, how can I make a test that will fail if this flag is not passed?

arielshaqed
arielshaqed reviewed Feb 25, 2024
View reviewed changes
pkg/block/s3/adapter.go
@@ -666,6 +666,7 @@ func (a *Adapter) CreateMultiPartUpload(ctx context.Context, obj block.ObjectPoi
Bucket: aws.String(bucket),
Key: aws.String(key),
ContentType: aws.String(""),
Expires: aws.Time(time.Now().Add(a.preSignedExpiry)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Note however that this is not necessarily the expiration time that the generated URL will have! That is capped also by the STS expiry.

itaiad200
itaiad200 approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

LGTM

@idanovo idanovo merged commit 35f2cf5 into master Feb 26, 2024
36 of 39 checks passed
@idanovo idanovo deleted the fix-bug-multiparts-upload-presigned-s3 branch February 26, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arielshaqed arielshaqed arielshaqed left review comments

@itaiad200 itaiad200 itaiad200 approved these changes

Assignees

@idanovo idanovo

Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: lakectl fs upload fails to upload large files after 15 minutes when using presigned mode
3 participants
@idanovo @itaiad200 @arielshaqed