Handle StorageID in S3 Gateway #8642

itaigilo · 2025-02-11T20:44:18Z

Closes #8641.

Change Description

Background

Making sure S3 Gateway plays way with StorageID.

Note that StorageID was already added to most of the relevant ObjectPointer objects,
So it's mainly about not allowing copy ops between two different StorageIDs.

Testing Details

AFAIK there's no coverage for the copy part,
So this was tested manually.

itaigilo · 2025-02-11T20:47:13Z

pkg/block/adapter.go

@@ -211,6 +211,7 @@ type Adapter interface {

 	BlockstoreType() string
 	BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error)
+	// GetStorageNamespaceInfo returns the StorageNamespaceInfo for storageID or nil if not found.


A comment that I promised @guy-har to add for my next OSS PR 🤓

itaigilo · 2025-02-11T20:47:48Z

pkg/gateway/operations/putobject_test.go

@@ -46,7 +47,7 @@ func TestWriteBlob(t *testing.T) {
 			reader := bytes.NewReader(data)
 			adapter := testutil.NewMockAdapter()
 			objectPointer := block.ObjectPointer{
-				StorageID:        "",
+				StorageID:        config.SingleBlockstoreID,


A bit of tidying up, in the area of this PR.

github-actions · 2025-02-11T20:52:44Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-02-11T20:53:12Z

E2E Test Results - Quickstart

nadavsteindler

Left a couple comments

nadavsteindler · 2025-02-12T08:16:25Z

pkg/gateway/operations/putobject.go

@@ -177,6 +177,12 @@ func handleUploadPart(w http.ResponseWriter, req *http.Request, o *PathOperation
 			}
 		}

+		if srcRepo.StorageID != o.Repository.StorageID {
+			o.Log(req).WithField("copy_source", copySource).WithError(err).Error("copy between different blockstores is not allowed")


There are now 4 places where we add copy_source to the logger via copy-pasted code. Why not use a constant or perhaps at the beginning of the code block do copySourceLogger := o.Log(req).WithFIeld(copy_source",copySource) and then use that logger in these 4 places

nadavsteindler · 2025-02-12T08:17:44Z

pkg/gateway/operations/putobject_test.go

@@ -46,7 +47,7 @@ func TestWriteBlob(t *testing.T) {
 			reader := bytes.NewReader(data)
 			adapter := testutil.NewMockAdapter()
 			objectPointer := block.ObjectPointer{
-				StorageID:        "",


We should also test non blank storage IDs

That's true!
But that's for a different Issue (opened in a different project).

arielshaqed

Thanks!

Mostly worried about missing test coverage.

arielshaqed · 2025-02-12T09:20:20Z

pkg/catalog/catalog.go

@@ -2738,6 +2738,10 @@ func (c *Catalog) CopyEntry(ctx context.Context, srcRepository, srcRef, srcPath,
 		if err != nil {
 			return nil, err
 		}
+
+		if srcRepo.StorageID != destRepo.StorageID {
+			return nil, fmt.Errorf("copy between different blockstores is not allowed: %w", graveler.ErrInvalidValue)


Suggested change

return nil, fmt.Errorf("copy between different blockstores is not allowed: %w", graveler.ErrInvalidValue)

return nil, fmt.Errorf("%w: cannot copy between different blockstores", graveler.ErrInvalidValue)

But also note that this error message is quite nasty. Consider using ErrInvalidStorageId, and also mentioning the two storage IDs.

arielshaqed · 2025-02-12T09:23:00Z

pkg/gateway/operations/listobjects.go

@@ -442,6 +442,7 @@ func handleListMultipartUploads(w http.ResponseWriter, req *http.Request, o *Rep
 		opts.KeyMarker = &keyMarker
 	}
 	mpuResp, err := o.BlockStore.ListMultipartUploads(req.Context(), block.ObjectPointer{
+		StorageID:        o.Repository.StorageID,


How could TestListMultipartUploads pass without this? I think perhaps still using blank StorageIDs in those tests?

AFAICT code was introduced here with no tests.

No tests indeed -
For almost the whole gateway/operations code...

Since it requires a decent amount of work, and it seems to me out of scope for this PR -
How about adding an Issue for adding unit-tests for these?

The issue is with adding untested code to the gateway. I am more comfortable with the new code as long as there are no storage IDs anywhere - I guess we can pull this code untested. But not sure of the benefit TBH.

arielshaqed · 2025-02-12T09:23:58Z

pkg/gateway/operations/putobject.go

@@ -177,6 +177,12 @@ func handleUploadPart(w http.ResponseWriter, req *http.Request, o *PathOperation
 			}
 		}

+		if srcRepo.StorageID != o.Repository.StorageID {
+			o.Log(req).WithField("copy_source", copySource).WithError(err).Error("copy between different blockstores is not allowed")


I don't understand "WithError(err)" - is there an error?

Also please rephrase the error message as above.

Copy-paste 🤦
Nice catch.

arielshaqed

;-(

arielshaqed · 2025-02-12T09:36:02Z

pkg/gateway/operations/listobjects.go

@@ -442,6 +442,7 @@ func handleListMultipartUploads(w http.ResponseWriter, req *http.Request, o *Rep
 		opts.KeyMarker = &keyMarker
 	}
 	mpuResp, err := o.BlockStore.ListMultipartUploads(req.Context(), block.ObjectPointer{
+		StorageID:        o.Repository.StorageID,


AFAICT code was introduced here with no tests.

itaigilo

@arielshaqed thanks for your review,
Your comments were fixed / addressed.

Please note that this is indeed not covered by unit-tests,
But this whole gateway part isn't, and adding tests to it might be a massive task.

It's been manually tested, and I think it's enough in this stage of the project -
But I'm interested in hearing your perspective.

itaigilo · 2025-02-13T07:20:04Z

pkg/gateway/operations/putobject.go

@@ -177,6 +177,12 @@ func handleUploadPart(w http.ResponseWriter, req *http.Request, o *PathOperation
 			}
 		}

+		if srcRepo.StorageID != o.Repository.StorageID {
+			o.Log(req).WithField("copy_source", copySource).WithError(err).Error("copy between different blockstores is not allowed")


Copy-paste 🤦
Nice catch.

itaigilo · 2025-02-13T07:22:32Z

pkg/gateway/operations/listobjects.go

@@ -442,6 +442,7 @@ func handleListMultipartUploads(w http.ResponseWriter, req *http.Request, o *Rep
 		opts.KeyMarker = &keyMarker
 	}
 	mpuResp, err := o.BlockStore.ListMultipartUploads(req.Context(), block.ObjectPointer{
+		StorageID:        o.Repository.StorageID,


No tests indeed -
For almost the whole gateway/operations code...

Since it requires a decent amount of work, and it seems to me out of scope for this PR -
How about adding an Issue for adding unit-tests for these?

itaigilo · 2025-02-13T07:33:50Z

pkg/upload/write_blob_test.go

While review this PR's comments, found out that this tests file has nothing to actually do with putobject -
It tests write_blob, hence moved here.

arielshaqed

Thanks! This also fixes Catalog.CopyEntry, which is obviously useful. Please make sure to write tests. I do know that the S3 gateway is not unit-tested. But it does have some integration tests in Esti - let's make sure that we at least run those with nonempty StorageIDs!

This is a requirement for releasing code that sets a nonempty StorageID.

arielshaqed · 2025-02-13T14:42:01Z

pkg/gateway/operations/listobjects.go

@@ -442,6 +442,7 @@ func handleListMultipartUploads(w http.ResponseWriter, req *http.Request, o *Rep
 		opts.KeyMarker = &keyMarker
 	}
 	mpuResp, err := o.BlockStore.ListMultipartUploads(req.Context(), block.ObjectPointer{
+		StorageID:        o.Repository.StorageID,


The issue is with adding untested code to the gateway. I am more comfortable with the new code as long as there are no storage IDs anywhere - I guess we can pull this code untested. But not sure of the benefit TBH.

itaigilo · 2025-02-14T08:08:26Z

Thanks! This also fixes Catalog.CopyEntry, which is obviously useful. Please make sure to write tests. I do know that the S3 gateway is not unit-tested. But it does have some integration tests in Esti - let's make sure that we at least run those with nonempty StorageIDs!

This is a requirement for releasing code that sets a nonempty StorageID.

Spent some extra time adding unit-tests for the Gateway, but it seems like a task that exceeds the scope of this Issue (and project).
I've added an Issue for covering S3 Gateway with unit-tests,
And tagged you (@arielshaqed) in another issue for covering this part with Esti tests.

Obviously I don't enjoy adding code with no tests covering it, but it should be covered before the project is released.

Merging - if you still think there's things that can be done, lmk.

itaigilo · 2025-02-14T08:11:03Z

@nadavsteindler thanks for reviewing -
Your comments have been fixed / addressed.

Can you PTAL again? 🙏

Handle StorageID in S3 Gateway

92a65a7

itaigilo requested review from N-o-Z and a team and removed request for N-o-Z February 11, 2025 20:44

Some minor additions

f604933

itaigilo added the exclude-changelog PR description should not be included in next release changelog label Feb 11, 2025

itaigilo commented Feb 11, 2025

View reviewed changes

nadavsteindler requested changes Feb 12, 2025

View reviewed changes

itaigilo requested review from N-o-Z, arielshaqed and guy-har February 12, 2025 08:54

arielshaqed approved these changes Feb 12, 2025

View reviewed changes

arielshaqed requested changes Feb 12, 2025

View reviewed changes

itaigilo added 2 commits February 13, 2025 09:08

Merge branch 'master' into task/handle-s3-gateway-storage-id

86afc9b

Fix PR comments

20dd150

itaigilo commented Feb 13, 2025

View reviewed changes

arielshaqed approved these changes Feb 13, 2025

View reviewed changes

itaigilo requested a review from nadavsteindler February 14, 2025 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle StorageID in S3 Gateway #8642

Handle StorageID in S3 Gateway #8642

itaigilo commented Feb 11, 2025

itaigilo Feb 11, 2025

itaigilo Feb 11, 2025

github-actions bot commented Feb 11, 2025

github-actions bot commented Feb 11, 2025

nadavsteindler left a comment

nadavsteindler Feb 12, 2025

nadavsteindler Feb 12, 2025

itaigilo Feb 12, 2025

arielshaqed left a comment

arielshaqed Feb 12, 2025

arielshaqed Feb 12, 2025

arielshaqed Feb 12, 2025

itaigilo Feb 13, 2025

arielshaqed Feb 13, 2025

arielshaqed Feb 12, 2025

itaigilo Feb 13, 2025

arielshaqed left a comment

arielshaqed Feb 12, 2025

itaigilo left a comment

itaigilo Feb 13, 2025

itaigilo Feb 13, 2025

itaigilo Feb 13, 2025

arielshaqed left a comment

arielshaqed Feb 13, 2025

itaigilo commented Feb 14, 2025

itaigilo commented Feb 14, 2025

	return nil, fmt.Errorf("copy between different blockstores is not allowed: %w", graveler.ErrInvalidValue)
	return nil, fmt.Errorf("%w: cannot copy between different blockstores", graveler.ErrInvalidValue)

Handle StorageID in S3 Gateway #8642

Are you sure you want to change the base?

Handle StorageID in S3 Gateway #8642

Conversation

itaigilo commented Feb 11, 2025

Change Description

Background

Testing Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Feb 11, 2025

E2E Test Results - Quickstart

nadavsteindler 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 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

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Feb 14, 2025

itaigilo commented Feb 14, 2025