8422 list multipart uploads #8531

ItamarYuran · 2025-01-21T14:22:59Z

Introducing List Multiupart uploads command to the s3 gateway.

example command:
✗ aws s3api --endpoint-url=http://127.0.0.1:8000/ list-multipart-uploads --bucket itamar --profile default

This will return all multipart uploads (upload id and path) that are held by the tracker.

This reverts commit e12badc.

github-actions · 2025-01-21T14:24:20Z

🎊 PR Preview 7b1500d has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8531.surge.sh

🕐 Build time: 0.01s

_{🤖 By surge-preview}

github-actions · 2025-01-21T14:30:57Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-01-21T14:31:04Z

E2E Test Results - Quickstart

idanovo · 2025-01-22T10:07:24Z

@ItamarYuran Can you please make sure the CI passes first? 🙏

ItamarYuran · 2025-01-28T09:39:08Z

Note - the marker works only when using both key marker & upload is marker. The key marker uses the physical address to mark the pagination. The physical address of the marked upload is of course returned as well when there are uploads left for next pagination.

nopcoder

I'm taking a step back and looking on the request which accepts Path and KeyMarker.

The underlaying storage can't provide this information as it holds the physical address and not the logical path the user provides.

I suggest having a test / use-case that test this code first to provide a working solution.
Implementation wise - it means that the information can be tracked and it means you will need this capability from the tracker. and in this case consider adding it to all the adapters and not specific s3 feature.

nopcoder · 2025-01-28T13:02:31Z

Update our (@ItamarYuran and myself) conversion output:

ListMultipartUploads implementation uses s3 adapter, similar to how ListParts works today (only on s3)
We verified that Apache Nifi PutS3 source code to understand that there is no use of 'prefix' or other special case paramters
The API should support pagination
Calling the API with 'prefix' or unsupported options should fail

idanovo

I have a few minor comments, but overall, it looks good!

esti/s3_gateway_test.go

idanovo · 2025-01-29T13:27:26Z

esti/s3_gateway_test.go

+	// unsupported headers, expect error
+	delimiter := aws.String("/")
+	prefix := aws.String("prefix")
+	encodingType := types.EncodingTypeUrl
+
+	output, err = s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: resp1.Bucket, Delimiter: delimiter})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")
+
+	output, err = s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: resp1.Bucket, Prefix: prefix})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")
+
+	output, err = s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: resp1.Bucket, EncodingType: encodingType})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")


I think these checks should be in a different test @nopcoder WDYT?

I think in general you are right but because we are going to skip this one for now for control-plane, might be nice to keep it in one spot. WDYT?

I think that the multipart upload was the only test that we used esti to check as it requires 5M uploads per part.
Agree with Idean - this part can be part the adapter package test as it should represent the spec we supprot.

idanovo · 2025-01-29T13:32:46Z

pkg/gateway/errors/errors.go

@@ -204,6 +205,11 @@ func (a APIErrorCode) ToAPIErr() APIError {
 // Codes - error code to APIError structure, these fields carry respective
 // descriptions for all the error responses.
 var Codes = errorCodeMap{
+	ErrInvalidArgument: {


This name is too generic for this error.
If I wouldn't read the description I could use this error in other places.
Maybe call it ErrInvalidMaxUploads?

This is the err i got from s3 while trying it with bad input

I missed this one - The problem is not with the generic name is the format of the message and how we map the errors in the gateway.
Each error is mapped latest inside:

// Codes - error code to APIError structure, these fields carry respective // descriptions for all the error responses. var Codes = errorCodeMap

Map the specific error message and add it into both places.

pkg/block/s3/adapter.go

pkg/gateway/operations/listobjects.go

nopcoder · 2025-01-29T13:59:56Z

pkg/gateway/operations/listobjects.go

 		NextKeyMarker:      *mpuResp.NextKeyMarker,
 		NextUploadIDMarker: *mpuResp.NextUploadIDMarker,


Use the AWS's helper function to get the value and check if the value is not nil - aws.StringValue

nopcoder · 2025-01-29T14:01:54Z

pkg/gateway/serde/xml.go

 	NextKeyMarker      string   `xml:"NextKeyMarker"`
 	NextUploadIDMarker string   `xml:"NextUploadIDMarker"`
+	IsTruncated        bool     `xml:"IsTruncated"`


Please verify that if there is no pagination required, does S3 remove these fields - which means that we need to set them as omitempty

in case there is no pagination, itTruncated does not appear in s3's answer

This is why we should use omitempty

nopcoder · 2025-01-29T21:51:28Z

pkg/gateway/operations/listobjects.go

+		maxUploads32 := int32(maxUploads)
+		opts.MaxUploads = &maxUploads32


We didn't verified that the value is 0-1000

Condition settings MaxUploads is above 0. The use of pointers in the AWS APIs is specify optionals and in case we do not pass values, we should use the same while working with the underlying SDK (at least try when it makes sense)

now aligned with s3, the input is checked and expects a value between 0-2147483647. other values will raise s3's err

pkg/gateway/operations/listobjects.go

nopcoder · 2025-01-29T22:12:23Z

esti/s3_gateway_test.go

@@ -289,6 +289,134 @@ func TestS3IfNoneMatch(t *testing.T) {
 	}
 }

+func TestListMultipartUploads(t *testing.T) {


This test should run only on S3 - I assume that the tests didn't pass for azure.

As @idanovo asked, these tests run for other adapters as well, only until the the first list mpu request in which they are expected to fail. then they are being skipped.

@nopcoder I wanted to make sure we get the right error for other block stores

Sure, but I don't understand how it works as we currently do not support this flow for all adapters.

if blockStoreType != "s3" { require.Contains(t, err.Error(), "NotImplemented") return }

ok, I see the check down the code of this function - but why do we even start to upload if we can't test it on non-s3 implementation?
we have a different test to check missing functionality

It can be tested in another test.
@ItamarYuran, my suggestion is to:

Change TestListMultipartUploads TestListMultipartUploadsUnsupported back to t.skipp with a proper message if blockStoreType != "s3"

Change TestListMultipartUploadsUnsupported name to indicate it checks only unsupported parameters

Add another test that only checks that ListMultipartUpload returns the right error if blockStoreType != "s3"
@nopcoder WDYT?

think that the latest change include the above

It doesn't

We are returning and not skipping

We don't test the error we get for object storage that are not s3

nopcoder · 2025-01-30T07:12:57Z

esti/s3_gateway_test.go

+
+}
+func TestListMultipartUploadsUnsupported(t *testing.T) {


Suggested change

}

func TestListMultipartUploadsUnsupported(t *testing.T) {

}

func TestListMultipartUploadsUnsupported(t *testing.T) {

nopcoder · 2025-01-30T07:14:16Z

esti/s3_gateway_test.go

+	delimiter := aws.String("/")
+	prefix := aws.String("prefix")
+	encodingType := types.EncodingTypeUrl
+
+	_, err := s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: Bucket, Delimiter: delimiter})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")
+
+	_, err = s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: Bucket, Prefix: prefix})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")
+
+	_, err = s3Client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{Bucket: Bucket, EncodingType: encodingType})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "NotImplemented")


As far as I remember we don't capture unsupported states. Wonder if need this extra test as we don't do it for all the adapters.

Can use t.Run to capture each state

pkg/gateway/operations/listobjects.go

idanovo

Please address all @nopcoder's comments before merging 🙏

nopcoder · 2025-01-30T15:55:23Z

pkg/gateway/operations/listobjects.go

+	maxUploadsListMPU = 2147483647
+	minUploadsListMPU = 0


if we leave the verification to the underlaying library - you can remove maxUploadsListMPU - it is MaxInt32 value and it will not pass this value as the cast will not let you.

if we plan to pass 0 as a valid value and let the underlying package handle it - we just need to check if maxUploads32 < 0

nopcoder

LGTM.

ItamarYuran added 5 commits October 23, 2024 11:43

Revert "docs: clarify auditing availble on enterprise (#8289)"

e7fb7b8

This reverts commit e12badc.

Merge branch 'master' of github.com:treeverse/lakeFS

cd5b80d

Merge branch 'master' of github.com:treeverse/lakeFS

299032a

no work no mess

d9ea540

got it

7b1500d

ItamarYuran added the include-changelog PR description should be included in next release changelog label Jan 21, 2025

ItamarYuran linked an issue Jan 21, 2025 that may be closed by this pull request

[Bug]: LakeFS does not support the S3 ListMultipartUploads API #8422

Open

f**** auditing

08ad93c

ItamarYuran added 9 commits January 21, 2025 17:13

with tests

1f7bf56

slight adjustment

2fbe42a

cross ya fingaz

11f8d46

yalla

08977ec

yalla

749f305

tests dont yet work

7d121bd

check output

e04cd1b

check output

dfc6699

clean

8aa34f5

ItamarYuran requested a review from a team January 22, 2025 10:04

no tests

f8c8215

ItamarYuran force-pushed the 8422-listmultipartuploads branch from ce35f96 to 47a576e Compare January 23, 2025 06:39

tests

cff37f2

ItamarYuran force-pushed the 8422-listmultipartuploads branch from 47a576e to cff37f2 Compare January 23, 2025 06:44

ItamarYuran added 4 commits January 23, 2025 11:49

ooooo baby

219d4f4

moving further

0a37251

check errr

52ef352

check errr

ea30b22

ItamarYuran requested a review from nopcoder January 28, 2025 09:25

nopcoder requested changes Jan 28, 2025

View reviewed changes

ItamarYuran added 2 commits January 28, 2025 13:38

testing key marker

62f8bbb

is truncated

8f55605

ItamarYuran added 2 commits January 28, 2025 16:17

with error not implemented

645e5cf

testts will now pass

4ecb691

ItamarYuran requested a review from nopcoder January 28, 2025 14:47

truncated for real

58961d1

ItamarYuran mentioned this pull request Jan 29, 2025

S3 GW to support ListMultipartUploads for various adapters #8563

Open

idanovo requested changes Jan 29, 2025

View reviewed changes

ItamarYuran mentioned this pull request Jan 29, 2025

List mpu docs #8565

Open

tests

fa64a69

nopcoder reviewed Jan 29, 2025

View reviewed changes

pkg/block/s3/adapter.go Outdated Show resolved Hide resolved

nopcoder requested changes Jan 29, 2025

View reviewed changes

final

82f74a3

ItamarYuran requested review from idanovo and nopcoder January 29, 2025 20:48

nopcoder requested changes Jan 30, 2025

View reviewed changes

kadima

dfbee5c

ItamarYuran requested a review from nopcoder January 30, 2025 08:43

ItamarYuran added 2 commits January 30, 2025 16:14

final adjusments

d21b831

max uploads added

8ea2f8e

idanovo approved these changes Jan 30, 2025

View reviewed changes

nopcoder requested changes Jan 30, 2025

View reviewed changes

ItamarYuran added 2 commits January 30, 2025 18:17

kadima

eaf8972

nu

ad126b7

ItamarYuran requested a review from nopcoder January 30, 2025 17:12

nopcoder approved these changes Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8422 list multipart uploads #8531

8422 list multipart uploads #8531

ItamarYuran commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025 •

edited

Loading

idanovo commented Jan 22, 2025

ItamarYuran commented Jan 28, 2025

nopcoder left a comment

nopcoder commented Jan 28, 2025

idanovo left a comment

idanovo Jan 29, 2025

ItamarYuran Jan 29, 2025

nopcoder Jan 30, 2025

idanovo Jan 29, 2025

ItamarYuran Jan 29, 2025

nopcoder Jan 30, 2025

nopcoder Jan 29, 2025

nopcoder Jan 29, 2025

ItamarYuran Jan 29, 2025

nopcoder Jan 30, 2025

nopcoder Jan 29, 2025

ItamarYuran Jan 30, 2025

nopcoder Jan 29, 2025

ItamarYuran Jan 30, 2025

idanovo Jan 30, 2025

nopcoder Jan 30, 2025

idanovo Jan 30, 2025

nopcoder Jan 30, 2025 •

edited

Loading

idanovo Jan 30, 2025

nopcoder Jan 30, 2025

idanovo Jan 30, 2025

nopcoder Jan 30, 2025

nopcoder Jan 30, 2025

idanovo left a comment

nopcoder Jan 30, 2025

nopcoder left a comment

		NextKeyMarker: *mpuResp.NextKeyMarker,
		NextUploadIDMarker: *mpuResp.NextUploadIDMarker,

		maxUploads32 := int32(maxUploads)
		opts.MaxUploads = &maxUploads32

8422 list multipart uploads #8531

Are you sure you want to change the base?

8422 list multipart uploads #8531

Conversation

ItamarYuran commented Jan 21, 2025 • edited Loading

github-actions bot commented Jan 21, 2025 • edited Loading

github-actions bot commented Jan 21, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Jan 21, 2025 • edited Loading

E2E Test Results - Quickstart

idanovo commented Jan 22, 2025

ItamarYuran commented Jan 28, 2025

nopcoder left a comment

Choose a reason for hiding this comment

nopcoder commented Jan 28, 2025

idanovo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idanovo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

ItamarYuran commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

nopcoder Jan 30, 2025 •

edited

Loading