Enforce same-region storage for new repositories #7847

itaigilo · 2024-06-10T10:36:38Z

Closes #7846.

Change Description

Disable (by default) the option to create a repository in a cloud region different than the region in which the block storage resides in.

It can be enabled by a server configuration option.

Background

Currently, users can create repositories using storage that resides in a region different than the region of the block store.
This hurts both performance and costs, and should be disabled by default.

Testing Details

Tested manually -
Unit tests are TBD (working on it now).

Breaking Change?

This changes the default behavior -
But I don't think it should be considered breaking-change.
If it's important, this can easily be solved by defaulting the config value to true.

Additional info

Example of the error:

github-actions · 2024-06-10T10:37:50Z

♻️ PR Preview c0e873c has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2024-06-10T10:44:11Z

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z

@itaigilo Thank you,

This is a partial review, if you intend on adding unit tests as part of this PR I would prefer if you finish with the PR before submitting it for review.
It makes it harder for me as a reviewer to provide partial reviews of code and keep track of added logic

N-o-Z · 2024-06-10T13:46:41Z

pkg/config/defaults.go

@@ -34,6 +34,7 @@ func setDefaults(cfgType string) {
 		viper.SetDefault("auth.encrypt.secret_key", DefaultAuthSecret)
 		viper.SetDefault(BlockstoreTypeKey, "local")
 	}
+	viper.SetDefault("installation.allow_inter_region_storage", true)


N-o-Z · 2024-06-10T13:48:06Z

pkg/block/transient/adapter.go

@@ -144,6 +144,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeTransient
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {


Need to also add this to mock adapter

Added, thanks.

N-o-Z · 2024-06-10T13:49:40Z

pkg/block/s3/adapter.go

@@ -832,6 +832,13 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeS3
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {
+	region, _ := a.clients.GetBucketRegionDefault(ctx, "")


You must check err and return err from this function as necessary.
Even if the current implementation will never actually return an error. You can't know when this implementation will change

Make sense. Updating this.

N-o-Z · 2024-06-10T13:51:11Z

pkg/block/transient/adapter.go

@@ -144,6 +144,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeTransient
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {
+	// not implemented at the moment
+	return block.BlockstoreMetadata{Region: nil}


Should return an error

For example, RuntimeStats doesn't return an error when un-implemented...

Thinking again, it makes more sense to return block.BlockstoreMetadata, error.
Changing this.

N-o-Z · 2024-06-10T13:52:06Z

docs/reference/configuration.md

@@ -305,6 +305,7 @@ An object describing the local (on-disk) cache of metadata from permanent storag
 * `installation.user_name` `(string : )` - When specified, an initial admin user will be created when the server is first run. Works only when `database.type` is set to local. Requires `installation.access_key_id` and `installation.secret_access_key`. 
 * `installation.access_key_id` `(string : )` - Admin's initial access key id (used once in the initial setup process)
 * `installation.secret_access_key` `(string : )` - Admin's initial secret access key (used once in the initial setup process)
+* `installation.allow_inter_region_storage` `(bool : false)` - Allow storage in a different region than the one the server is running in.


Update the default (true)

N-o-Z · 2024-06-10T13:54:32Z

pkg/api/controller.go

+	storeRegion := c.BlockAdapter.BlockstoreMetadata(ctx).Region
+	if storeRegion == nil {


Suggested change

storeRegion := c.BlockAdapter.BlockstoreMetadata(ctx).Region

if storeRegion == nil {

region := c.BlockAdapter.BlockstoreMetadata(ctx).Region

if region == nil {

I prefer being explicit, and explain what I'm actually using (which is the block-store region).

N-o-Z · 2024-06-10T13:56:41Z

pkg/block/azure/adapter.go

@@ -583,6 +583,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeAzure
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {


Why are we not implementing it for Azure as well?

N-o-Z · 2024-06-10T13:56:59Z

pkg/block/gs/adapter.go

@@ -630,6 +630,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeGS
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {


Why are we not implementing it for GCS as well?

N-o-Z · 2024-06-10T13:57:52Z

pkg/block/s3/adapter.go

@@ -861,6 +868,15 @@ func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType
 	return block.DefaultResolveNamespace(storageNamespace, key, identifierType)
 }

+func (a *Adapter) GetRegion(ctx context.Context, storageNamespace string) (string, error) {
+	bucket, found := strings.CutPrefix(storageNamespace, "s3://")


I'm not sure this validation is required - we should have already validated the namespace before

It's not about validating -
It's about sending the bucket name (for example aaa) to the AWS API, and not the namespace (s3://aaa).
(And I also tried sending s3://aaa to the API - it didn't work.)

Lets add a GetSchema method to the block adapter or at least use the BlockstoreTypeS3 const.

N-o-Z · 2024-06-10T14:10:14Z

pkg/api/controller.go

@@ -1946,6 +1947,14 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo
 		return
 	}

+	allowInterRegionStorage := c.Config.Installation.AllowInterRegionStorage
+	if !allowInterRegionStorage {


I feel like this check should be inside validateInterRegionStorage

Interesting.
I don't really mind actually, both seem ok to me 😄

itaigilo

@N-o-Z thanks for your review.

Will add impl for Azure and GCS,
As well as tests.

The point of sending for review this before adding tests, is to validate the direction is solid first. Will update once it's done, so we can reduce review iterations, of course.

itaigilo · 2024-06-10T15:06:38Z

pkg/api/controller.go

@@ -1946,6 +1947,14 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo
 		return
 	}

+	allowInterRegionStorage := c.Config.Installation.AllowInterRegionStorage
+	if !allowInterRegionStorage {


Interesting.
I don't really mind actually, both seem ok to me 😄

itaigilo · 2024-06-10T15:08:11Z

pkg/api/controller.go

+	storeRegion := c.BlockAdapter.BlockstoreMetadata(ctx).Region
+	if storeRegion == nil {


I prefer being explicit, and explain what I'm actually using (which is the block-store region).

itaigilo · 2024-06-10T15:10:41Z

pkg/block/s3/adapter.go

@@ -861,6 +868,15 @@ func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType
 	return block.DefaultResolveNamespace(storageNamespace, key, identifierType)
 }

+func (a *Adapter) GetRegion(ctx context.Context, storageNamespace string) (string, error) {
+	bucket, found := strings.CutPrefix(storageNamespace, "s3://")


It's not about validating -
It's about sending the bucket name (for example aaa) to the AWS API, and not the namespace (s3://aaa).
(And I also tried sending s3://aaa to the API - it didn't work.)

itaigilo · 2024-06-10T15:10:54Z

pkg/block/transient/adapter.go

@@ -144,6 +144,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeTransient
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {


Added, thanks.

itaigilo · 2024-06-10T15:12:18Z

pkg/block/transient/adapter.go

@@ -144,6 +144,11 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeTransient
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) block.BlockstoreMetadata {
+	// not implemented at the moment
+	return block.BlockstoreMetadata{Region: nil}


For example, RuntimeStats doesn't return an error when un-implemented...

itaigilo · 2024-06-13T14:06:00Z

pkg/api/controller.go

@@ -1946,6 +1947,13 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo
 		return
 	}

+	if !c.Config.Installation.AllowInterRegionStorage {
+		if err := block.ValidateInterRegionStorage(r.Context(), c.BlockAdapter, body.StorageNamespace); err != nil {


According to @arielshaqed 's suggestion, extracted this into a separate file.
It encapsulates it from this huge file, and make it easier to test.

itaigilo · 2024-06-13T14:07:13Z

pkg/api/controller_test.go

@@ -4576,7 +4575,7 @@ func TestController_PrepareGarbageCollectionCommitted(t *testing.T) {
 }

 func TestController_ClientDisconnect(t *testing.T) {
-	handler, deps := setupHandlerWithWalkerFactory(t, store.NewFactory(nil))
+	handler, deps := setupHandler(t)


Started from an attempt to add my tests here -
Ended up not to, but on the way I did a small cleanup, which I think worth keeping (mainly in server_test.go.

itaigilo · 2024-06-13T14:08:26Z

pkg/testutil/adapter.go

@@ -0,0 +1,165 @@
+package testutil


Moved mockAdapter into testutil, so it can be used by other tests.

itaigilo

@N-o-Z done some refactoring and add tests,
PTAL again.

And regarding implementing for Azure and CGS -
Opening now a separate issue, and will start working on it right after this one.

N-o-Z

Thanks,
Some more comments

N-o-Z · 2024-06-13T18:55:07Z

pkg/gateway/operations/putobject_test.go

@@ -6,6 +6,7 @@ import (
 	"crypto/md5" //nolint:gosec
 	"crypto/rand"
 	"fmt"
+	"github.com/treeverse/lakefs/pkg/testutil"


gofmt imports

make validate-fmt says Your code formatting is according to gofmt standards.

Am I missing something?

N-o-Z · 2024-06-13T18:57:44Z

pkg/block/s3/adapter.go

-	region, _ := a.clients.GetBucketRegionDefault(ctx, "")
-	return block.BlockstoreMetadata{
-		Region: &region,
+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {


Prefer to return a pointer + err

N-o-Z · 2024-06-16T07:48:31Z

pkg/block/azure/adapter.go

@@ -583,6 +583,10 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeAzure
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {


The issue discusses adding support for same-region storage on all block adapters. Please open followup issues to support this on Azure and GCS

Issue #7868 .

N-o-Z · 2024-06-16T07:55:26Z

pkg/block/s3/adapter.go

@@ -861,6 +868,15 @@ func (a *Adapter) ResolveNamespace(storageNamespace, key string, identifierType
 	return block.DefaultResolveNamespace(storageNamespace, key, identifierType)
 }

+func (a *Adapter) GetRegion(ctx context.Context, storageNamespace string) (string, error) {
+	bucket, found := strings.CutPrefix(storageNamespace, "s3://")


Lets add a GetSchema method to the block adapter or at least use the BlockstoreTypeS3 const.

N-o-Z · 2024-06-16T07:56:24Z

pkg/block/validations_test.go

+package block_test
+
+import (
+	"context"


make validate-fmt says Your code formatting is according to gofmt standards.

Am I missing something?

It ignores it since this is a test file but goimports requires separating standard library from 3rd party imports (this will fail on non-test files).
We exclude test files from lint and fmt due to coding style failures we have there for testing reasons but we still would like to maintain some of the formatting conventions.
If you run goimports on the file you will see the errors

If it's important - why not apply only imports validation on the test files?

Anyway, fixed.

N-o-Z · 2024-06-16T07:58:52Z

pkg/block/validations_test.go

+	"testing"
+)
+
+func TestController_ValidateInterRegionStorage(t *testing.T) {


You can use test cases to remove code repetition.

N-o-Z · 2024-06-16T08:04:14Z

pkg/block/validations.go

+
+func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageNamespace string) error {
+	blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx)
+	if errors.Is(err, ErrOperationNotSupported) || blockstoreMetadata.Region == nil {


I think it's a bug to check for blockstoreMetadata.Region == nil
In which situations are we expected to get a nil value?
It seems to me we either get an err or the Region field should be populated

Please add a debug log in this case

The idea was to make BlockstoreMetadata more general.
So for example, in the future, it will also include other properties.
In this case, BlockstoreMetadata may return the other props and not region, which says it won't be an error, but just not implemented.

If you find this more confusing, I can remove this check.

My question is, not whether Metadata stores other information regarding the blockstore but rather if there is any real life scenario where the Region property will be nil.
If it makes sense - leave this as it is.
If there's no current scenario where this can actually be valid - we should remove this

N-o-Z · 2024-06-16T08:06:43Z

pkg/block/s3/adapter.go

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {
+	region, err := a.clients.GetBucketRegionDefault(ctx, "")
+	if err != nil {
+		return block.BlockstoreMetadata{}, err


return nil, err

itaigilo

@N-o-Z all your comments have been fixed or addressed - PTAL again.

itaigilo · 2024-06-16T12:11:33Z

pkg/block/validations_test.go

+package block_test
+
+import (
+	"context"


make validate-fmt says Your code formatting is according to gofmt standards.

Am I missing something?

itaigilo · 2024-06-16T12:16:56Z

pkg/gateway/operations/putobject_test.go

@@ -6,6 +6,7 @@ import (
 	"crypto/md5" //nolint:gosec
 	"crypto/rand"
 	"fmt"
+	"github.com/treeverse/lakefs/pkg/testutil"


make validate-fmt says Your code formatting is according to gofmt standards.

Am I missing something?

itaigilo · 2024-06-16T12:36:06Z

pkg/block/validations.go

+
+func ValidateInterRegionStorage(ctx context.Context, adapter Adapter, storageNamespace string) error {
+	blockstoreMetadata, err := adapter.BlockstoreMetadata(ctx)
+	if errors.Is(err, ErrOperationNotSupported) || blockstoreMetadata.Region == nil {


The idea was to make BlockstoreMetadata more general.
So for example, in the future, it will also include other properties.
In this case, BlockstoreMetadata may return the other props and not region, which says it won't be an error, but just not implemented.

If you find this more confusing, I can remove this check.

itaigilo · 2024-06-16T12:44:53Z

pkg/block/azure/adapter.go

@@ -583,6 +583,10 @@ func (a *Adapter) BlockstoreType() string {
 	return block.BlockstoreTypeAzure
 }

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {


Issue #7868 .

itaigilo · 2024-06-16T12:45:23Z

pkg/block/s3/adapter.go

-	region, _ := a.clients.GetBucketRegionDefault(ctx, "")
-	return block.BlockstoreMetadata{
-		Region: &region,
+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {


itaigilo · 2024-06-16T12:45:28Z

pkg/block/s3/adapter.go

+func (a *Adapter) BlockstoreMetadata(ctx context.Context) (block.BlockstoreMetadata, error) {
+	region, err := a.clients.GetBucketRegionDefault(ctx, "")
+	if err != nil {
+		return block.BlockstoreMetadata{}, err


N-o-Z

@itaigilo thanks,
Only 2 comments we need to close

itaigilo

@N-o-Z fixed, PTAL again.

itaigilo · 2024-06-17T04:21:17Z

pkg/block/validations_test.go

+package block_test
+
+import (
+	"context"


If it's important - why not apply only imports validation on the test files?

Anyway, fixed.

itaigilo · 2024-06-17T04:22:10Z

pkg/gateway/operations/putobject_test.go

@@ -6,6 +6,7 @@ import (
 	"crypto/md5" //nolint:gosec
 	"crypto/rand"
 	"fmt"
+	"github.com/treeverse/lakefs/pkg/testutil"


N-o-Z

Approved, thanks

Block inter-region storage

e4d4174

itaigilo added new-feature Issues that introduce new feature or capability area/block-adapter include-changelog PR description should be included in next release changelog AWS labels Jun 10, 2024

itaigilo requested a review from a team June 10, 2024 10:40

itaigilo added 3 commits June 10, 2024 14:29

Fix lint

d8bfba3

Change default flag value

0be6f94

Add mockAdapter impl for interface

7c505c5

N-o-Z requested changes Jun 10, 2024

View reviewed changes

Fix PR comments

44328df

itaigilo commented Jun 10, 2024

View reviewed changes

itaigilo added 2 commits June 11, 2024 10:34

Improve error handling

83bcd6b

Refactor and add tests

f35f3ce

itaigilo commented Jun 13, 2024

View reviewed changes

itaigilo requested a review from N-o-Z June 13, 2024 14:09

N-o-Z requested changes Jun 16, 2024

View reviewed changes

Change error format

5be675b

itaigilo commented Jun 16, 2024

View reviewed changes

itaigilo requested a review from N-o-Z June 16, 2024 12:52

N-o-Z requested changes Jun 16, 2024

View reviewed changes

itaigilo added 2 commits June 17, 2024 07:20

Code cleanup

3a3c15b

Organize imports

8669a8c

itaigilo commented Jun 17, 2024

View reviewed changes

itaigilo requested a review from N-o-Z June 17, 2024 04:23

Merge branch 'master' into feature/block-inter-region-storage

c0e873c

N-o-Z approved these changes Jun 17, 2024

View reviewed changes

itaigilo merged commit 33ee454 into master Jun 17, 2024
36 checks passed

itaigilo deleted the feature/block-inter-region-storage branch June 17, 2024 11:05

		storeRegion := c.BlockAdapter.BlockstoreMetadata(ctx).Region
		if storeRegion == nil {

Enforce same-region storage for new repositories #7847

Enforce same-region storage for new repositories #7847

Conversation

itaigilo commented Jun 10, 2024

Change Description

Background

Testing Details

Breaking Change?

Additional info

github-actions bot commented Jun 10, 2024 • edited Loading

github-actions bot commented Jun 10, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z left a comment • 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

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

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

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

itaigilo left a comment • edited Loading

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

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

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

Choose a reason for hiding this 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

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

N-o-Z left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2024 •

edited

Loading

N-o-Z left a comment •

edited

Loading

itaigilo left a comment •

edited

Loading