Fix S3 gateway cross-repo copies #7468

arielshaqed · 2024-02-15T14:20:55Z

putobject and friends were confusing src and dst repositories, which will not do.

Fixes #7467.

putobject and friends were confusing src and dst repositories, which will not do. Fixes #7467.

arielshaqed · 2024-02-15T14:23:33Z

Reviewers: Adding relevant test to Esti, but please review the change for now.

github-actions · 2024-02-15T14:28:11Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed · 2024-02-15T14:40:37Z

Reviewers: Adding relevant test to Esti, but please review the change for now.

Actually... testing this may not be possible with the current S3 gateway test, which uses the MinIO client, which I am not sure supports the multipart copies API. May take a while, will open a tech-debt issue if I cannot get this done here today.

N-o-Z

@arielshaqed Thanks, code is great no comments on the changes. However, can you please add a test scenario for that

arielshaqed · 2024-02-15T14:46:13Z

@arielshaqed Thanks, code is great no comments on the changes. However, can you please add a test scenario for that

Thanks! There are some issues with testing, I'm still trying to figure out if I can use MinIO client ComposeObject to do this.

N-o-Z · 2024-02-15T14:46:54Z

may not be pos

Reviewers: Adding relevant test to Esti, but please review the change for now.

Actually... testing this may not be possible with the current S3 gateway test, which uses the MinIO client, which I am not sure supports the multipart copies API. May take a while, will open a tech-debt issue if I cannot get this done here today.

@arielshaqed for that specific test you can use an S3 client and limit the test for s3 blockstore only, though I think minio does support multipart upload

We might only have a problem with sigV2, which is considerably less important by now.

This should make MPU copies that are signed using sigV2 work with the S3 gateway.

Otherwise infinite loop on upload >:-(

Exercise another code path, don't triple data size, and keep the originally intended logic.

Repositories not always deleted, so if we don't do this we get a conflict.

arielshaqed · 2024-02-15T21:02:41Z

@guy-har maybe you know? I managed to get Esti to pass, except for...

SigV2. I am still missing some headers to sign. This is less urgent, I doubt we have many users of MPU copy with sigV2. So I am happy to open a tech debt issue and pull without it.
Azure tests fail due to "InvalidBlockList". This would need to be an error in the Azure block adapter. The obvious suspect is the conversion from symmetrical bounds in the S3 API (bytes [start,end]) to asymmetrical bounds in the Azure BlobStore API (bytes [offset, offset+count)). But I had a look and we do add 1 to get the required bounds. Any ideas what else to try?

copyPartRange should stage a block on the destination. This matters When source and destination containers are different.

arielshaqed

@N-o-Z I added an integration test to Esti. That was a great call because it uncovered an issue with the Azure block adapter! So I fixed that one too. Please re-re-review.

@guy-har I'd love to have your review of everything, especially of the changes to that Azure block adapter :-)

Please note that sigv2 still doesn't work. Opened #7472 to do that. But we should pull anyway: nobody should use sigV2, it's really hard to do nowadays, it solves the original problem for Boto3/Python users who are currently complaining, and lakeFS with this code is better than without it!

guy-har

Looks Great!
Thanks!

guy-har · 2024-02-18T15:16:33Z

esti/s3_gateway_test.go

@@ -328,6 +328,89 @@ func TestS3HeadBucket(t *testing.T) {
 	})
 }

+func TestS3CopyObjectMultipart(t *testing.T) {
+	const minDataContentLengthForMultipart = 5 << 20


nit: it's a bit confusing that minDataContentLengthForMultipart and largeDataContentLength exist in different files and only used here

N-o-Z

Thanks for adding the test!!
Some optional suggestion

N-o-Z · 2024-02-19T07:53:09Z

esti/s3_gateway_test.go

@@ -328,6 +328,88 @@ func TestS3HeadBucket(t *testing.T) {
 	})
 }

+func TestS3CopyObjectMultipart(t *testing.T) {


The only thing tests are good for are finding bugs. And fixing them. And preventing regressions. Automatically. Not sure it's worth the effort.

Although... in this case this test actually found bugs. And helped fix one of them. And another test in the file prevented a regression. Automatically. Actually the case against writing tests has never been weaker 🥳 .

N-o-Z · 2024-02-19T07:55:27Z

pkg/block/azure/multipart_block_writer.go

-	blobSizesURL := destinationContainer.NewBlockBlobClient(destinationObjName + sizeSuffix)
-	_, err = blobSizesURL.StageBlock(ctx, base64Etag, streaming.NopCloser(strings.NewReader(sizeData)), nil)
+	sizeData := fmt.Sprintf("%d\n", count)
+	blobSizesBlob := destinationContainer.NewBlockBlobClient(destinationKey.BlobURL + sizeSuffix)


Consider moving sizeSuffix and idSuffix to this file

Not done in this PR.

N-o-Z · 2024-02-19T08:05:02Z

pkg/testutil/random.go

+}
+
+// RandomReader returns a reader that will return size bytes from rand.
+func RandomReader(rand *rand.Rand, size int64) io.Reader {


Suggested change

func RandomReader(rand *rand.Rand, size int64) io.Reader {

func NewRandomReader(rand *rand.Rand, size int64) io.Reader {

?

Nice one, yes please!

Because it returns a new random reader, and also that matches the Go naming conventions. :-)

arielshaqed

Thanks!

Pulling once all tests pass.

arielshaqed · 2024-02-19T08:17:41Z

pkg/block/azure/multipart_block_writer.go

-	blobSizesURL := destinationContainer.NewBlockBlobClient(destinationObjName + sizeSuffix)
-	_, err = blobSizesURL.StageBlock(ctx, base64Etag, streaming.NopCloser(strings.NewReader(sizeData)), nil)
+	sizeData := fmt.Sprintf("%d\n", count)
+	blobSizesBlob := destinationContainer.NewBlockBlobClient(destinationKey.BlobURL + sizeSuffix)


Not done in this PR.

arielshaqed · 2024-02-19T08:20:55Z

esti/s3_gateway_test.go

@@ -328,6 +328,88 @@ func TestS3HeadBucket(t *testing.T) {
 	})
 }

+func TestS3CopyObjectMultipart(t *testing.T) {


The only thing tests are good for are finding bugs. And fixing them. And preventing regressions. Automatically. Not sure it's worth the effort.

Although... in this case this test actually found bugs. And helped fix one of them. And another test in the file prevented a regression. Automatically. Actually the case against writing tests has never been weaker 🥳 .

arielshaqed · 2024-02-19T08:21:19Z

pkg/testutil/random.go

+}
+
+// RandomReader returns a reader that will return size bytes from rand.
+func RandomReader(rand *rand.Rand, size int64) io.Reader {


Nice one, yes please!

arielshaqed · 2024-02-19T08:45:56Z

license/cla stuck. But I work here, I literally signed on paper to grant the project my rights for work on lakeFS. Pulling by adminiforce.

Fix S3 gateway cross-repo copies

754398d

putobject and friends were confusing src and dst repositories, which will not do. Fixes #7467.

arielshaqed requested review from nopcoder and guy-har February 15, 2024 14:22

arielshaqed added bug Something isn't working area/gateway Changes to the gateway include-changelog PR description should be included in next release changelog labels Feb 15, 2024

arielshaqed requested a review from N-o-Z February 15, 2024 14:33

N-o-Z requested changes Feb 15, 2024

View reviewed changes

Test S3 gateway multipart copy in Esti

1dcdf87

arielshaqed force-pushed the bug/7467-s3-gw-copy-from-boto3 branch from 37b3281 to 1dcdf87 Compare February 15, 2024 16:06

arielshaqed added 7 commits February 15, 2024 21:06

[esti] Use sigV4 for S3 gateway MPU copy tests

b37f996

We might only have a problem with sigV2, which is considerably less important by now.

Add copy-source to sigV2 signed headers and restore sigV2 signing

b2602ae

This should make MPU copies that are signed using sigV2 work with the S3 gateway.

[bug] Make randomReader return EOF when it's done

52fef46

Otherwise infinite loop on upload >:-(

Try again with sigv4

52b6c8e

[bug] Actually request multipart-range in copy

c7fef45

Exercise another code path, don't triple data size, and keep the originally intended logic.

[bug] Use distinct repository name from TestS3CopyObject

155cfdc

Repositories not always deleted, so if we don't do this we get a conflict.

[bug] Start, End on MinIO client CopySrcOptions are inclusive bounds

767ebc3

arielshaqed force-pushed the bug/7467-s3-gw-copy-from-boto3 branch 3 times, most recently from df0ada8 to 2d1fdf6 Compare February 18, 2024 09:01

[bug] Azure copyPartRange should stage block on destination

02411e1

copyPartRange should stage a block on the destination. This matters When source and destination containers are different.

arielshaqed force-pushed the bug/7467-s3-gw-copy-from-boto3 branch from 2d1fdf6 to 02411e1 Compare February 18, 2024 14:11

arielshaqed mentioned this pull request Feb 18, 2024

S3 gateway fails with client cross-repository multipart copies signed with sigV2 #7472

Open

arielshaqed commented Feb 18, 2024

View reviewed changes

arielshaqed requested a review from N-o-Z February 18, 2024 14:37

guy-har approved these changes Feb 18, 2024

View reviewed changes

[CR] Put all "const ...ContentLength..." together and document them

f623b3f

N-o-Z approved these changes Feb 19, 2024

View reviewed changes

[CR] Rename RandomReader -> NewRandomReader

b9227a7

Because it returns a new random reader, and also that matches the Go naming conventions. :-)

arielshaqed commented Feb 19, 2024

View reviewed changes

arielshaqed enabled auto-merge (squash) February 19, 2024 08:22

arielshaqed disabled auto-merge February 19, 2024 08:46

arielshaqed merged commit 2cc79fa into master Feb 19, 2024
34 checks passed

arielshaqed deleted the bug/7467-s3-gw-copy-from-boto3 branch February 19, 2024 08:46

N-o-Z mentioned this pull request Feb 20, 2024

[BUG]: TestS3CopyObjectMultipart sporadically fails in esti tests #7485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix S3 gateway cross-repo copies #7468

Fix S3 gateway cross-repo copies #7468

arielshaqed commented Feb 15, 2024

arielshaqed commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

arielshaqed commented Feb 15, 2024

N-o-Z left a comment

arielshaqed commented Feb 15, 2024

N-o-Z commented Feb 15, 2024

arielshaqed commented Feb 15, 2024

arielshaqed left a comment

guy-har left a comment

guy-har Feb 18, 2024

N-o-Z left a comment

N-o-Z Feb 19, 2024

arielshaqed Feb 19, 2024

N-o-Z Feb 19, 2024

arielshaqed Feb 19, 2024

N-o-Z Feb 19, 2024

arielshaqed Feb 19, 2024

arielshaqed left a comment

arielshaqed Feb 19, 2024

arielshaqed Feb 19, 2024

arielshaqed Feb 19, 2024

arielshaqed commented Feb 19, 2024

	func RandomReader(rand *rand.Rand, size int64) io.Reader {
	func NewRandomReader(rand *rand.Rand, size int64) io.Reader {

Fix S3 gateway cross-repo copies #7468

Fix S3 gateway cross-repo copies #7468

Conversation

arielshaqed commented Feb 15, 2024

arielshaqed commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed commented Feb 15, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

arielshaqed commented Feb 15, 2024

N-o-Z commented Feb 15, 2024

arielshaqed commented Feb 15, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z 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

arielshaqed 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

arielshaqed commented Feb 19, 2024