Return an informative error message when using 'api/v1' for S3 GW endpoint #7828

itaigilo · 2024-06-03T13:37:19Z

Change Description

Quite a few users are trying sometimes to access the S3 Gateway using an endpoint starting with /api/v1.
This used to lead to a NoSuchBucket 404 error.

After the fix, such scenario leads to a 404 error, but with a different message:

Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.

…point

github-actions · 2024-06-03T13:44:20Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks, great improvement! Requesting changes to help users who legitimately have a bucket "api" and a tag "v1".

arielshaqed · 2024-06-03T13:40:55Z

pkg/gateway/errors/errors.go

@@ -341,6 +342,11 @@ var Codes = errorCodeMap{
 		Description:    "The bucket lifecycle configuration does not exist",
 		HTTPStatusCode: http.StatusNotFound,
 	},
+	ErrNoSuchBucketPossibleAPIEndpoint: {
+		Code:           "ErrNoSuchBucketPossibleAPIEndpoint",
+		Description:    "Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.",


Perhaps afford people an explanation? I'm thinking of the poor user who has a repo named "api" and manages to mitype a branch name "v1"

Can you squeeze more into an error message?
If so - any suggestions on how to improve this?

Hmmm... good thing they don't deduct bandwidth for errors from my paycheck. I'd go with a definitive error message, followed by a suggestion:

Suggested change

Description: "Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.",

Description: `Repository "api" not found; this can happen if your endpoint URL mistakenly ends in "` + apiutil.BaseURL + `".`,

(the ` is an alternative quotation mark that reduces the number of backslashes needed here.)

arielshaqed · 2024-06-03T13:42:28Z

pkg/gateway/middleware.go

@@ -184,6 +184,13 @@ func EnrichWithRepositoryOrFallback(c *catalog.Catalog, authService auth.Gateway
 				fallbackProxy.ServeHTTP(w, req)
 				return
 			}
+
+			// see: github.com/treeverse/lakeFS/issues/3082


Or just state what this is. If I want to know who added this code, GitHub has excellent blame.

Makes sense. Done.

arielshaqed · 2024-06-03T13:43:01Z

pkg/gateway/middleware.go

@@ -184,6 +184,13 @@ func EnrichWithRepositoryOrFallback(c *catalog.Catalog, authService auth.Gateway
 				fallbackProxy.ServeHTTP(w, req)
 				return
 			}
+
+			// see: github.com/treeverse/lakeFS/issues/3082
+			if strings.HasPrefix(req.RequestURI, "/api/v1") {


This has to be defined on some constant. Can we use that constant here?

You are right. Done.

arielshaqed · 2024-06-03T13:43:56Z

pkg/gateway/middleware.go

@@ -184,6 +184,13 @@ func EnrichWithRepositoryOrFallback(c *catalog.Catalog, authService auth.Gateway
 				fallbackProxy.ServeHTTP(w, req)
 				return
 			}
+
+			// see: github.com/treeverse/lakeFS/issues/3082
+			if strings.HasPrefix(req.RequestURI, "/api/v1") {


Don't we need to fail to find a bucket first?

Nope. As far as I understand this, it's the last line before turning to the ErrNoSuchBucket error.

arielshaqed

Thanks!

How was this tested? OK to pull with only a manual test, but if you do then please also add a test in a later PR.

arielshaqed · 2024-06-03T14:55:09Z

pkg/gateway/errors/errors.go

@@ -341,6 +342,11 @@ var Codes = errorCodeMap{
 		Description:    "The bucket lifecycle configuration does not exist",
 		HTTPStatusCode: http.StatusNotFound,
 	},
+	ErrNoSuchBucketPossibleAPIEndpoint: {
+		Code:           "ErrNoSuchBucketPossibleAPIEndpoint",
+		Description:    "Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.",


Hmmm... good thing they don't deduct bandwidth for errors from my paycheck. I'd go with a definitive error message, followed by a suggestion:

Suggested change

Description: "Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.",

Description: `Repository "api" not found; this can happen if your endpoint URL mistakenly ends in "` + apiutil.BaseURL + `".`,

(the ` is an alternative quotation mark that reduces the number of backslashes needed here.)

arielshaqed

You added the test -- yay! -- but I suspect that it is broken because /api/v1 needs to be a prefix of the path of the URL. So now I'm requesting that change.

arielshaqed · 2024-06-03T14:59:32Z

esti/s3_gateway_test.go

+
+	accessKeyID := viper.GetString("access_key_id")
+	secretAccessKey := viper.GetString("secret_access_key")
+	endpoint := "/api/v1" + viper.GetString("s3_endpoint")


I think that this is backwards!?

N-o-Z

Thanks!
The code looks good, some comments on the test

N-o-Z · 2024-06-03T15:44:06Z

esti/s3_gateway_test.go

+
+	accessKeyID := viper.GetString("access_key_id")
+	secretAccessKey := viper.GetString("secret_access_key")
+	endpoint := "/api/v1" + viper.GetString("s3_endpoint")


Suggested change

endpoint := "/api/v1" + viper.GetString("s3_endpoint")

endpoint := viper.GetString("s3_endpoint") + apiutil.BaseURL

N-o-Z · 2024-06-03T15:44:54Z

esti/s3_gateway_test.go

+
+	t.Run("use_api_v1_for_client_endpoint", func(t *testing.T) {
+		_, err := minioClient.GetObject(ctx, repo, "main/some", minio.GetObjectOptions{})
+		require.Contains(t, err.Error(), "Using lakeFS API URI as the endpoint")


This will panic if err is nil. You need to check

also, not sure but I think you can use require.ErrorIs

N-o-Z · 2024-06-03T15:46:08Z

esti/s3_gateway_test.go

+		t.Fatalf("minio.New: %s", err)
+	}
+
+	t.Run("use_api_v1_for_client_endpoint", func(t *testing.T) {


Better for forward compatibility

Suggested change

t.Run("use_api_v1_for_client_endpoint", func(t *testing.T) {

t.Run("use_open_api_for_client_endpoint", func(t *testing.T) {

itaigilo · 2024-06-04T09:15:26Z

Well, the test I've added fails, for:

minio.New: Endpoint url cannot have fully qualified paths.

I'm using s3.local.lakefs.io:8000/api/v1 for the endpoint of minio, but this isn't allowed (no extra path is allowed, only hostname).

@arielshaqed @N-o-Z Can you think of another way to test this, or should I release it without the test?

N-o-Z · 2024-06-04T09:24:30Z

Well, the test I've added fails, for:

minio.New: Endpoint url cannot have fully qualified paths.

I'm using s3.local.lakefs.io:8000/api/v1 for the endpoint of minio, but this isn't allowed (no extra path is allowed, only hostname).

@arielshaqed @N-o-Z Can you think of another way to test this, or should I release it without the test?

One possible option is not using a minio client.
Bottom line you are testing an http server and want to check that when you initiate a request with a certain path prefix you get a specific error. There are several ways to test that I assume

itaigilo · 2024-06-04T09:29:14Z

One possible option is not using a minio client. Bottom line you are testing an http server and want to check that when you initiate a request with a certain path prefix you get a specific error. There are several ways to test that I assume

Yeah this makes sense -
But I don't think it's worth the effort for this case.
Unless you think it'll be a useful infra for the future (I don't think so, but I probably don't have enough context)?

N-o-Z · 2024-06-04T09:46:08Z

One possible option is not using a minio client. Bottom line you are testing an http server and want to check that when you initiate a request with a certain path prefix you get a specific error. There are several ways to test that I assume

Yeah this makes sense - But I don't think it's worth the effort for this case. Unless you think it'll be a useful infra for the future (I don't think so, but I probably don't have enough context)?

If there's a scenario, we need to test it.
Adding an automated test protects us from regression

N-o-Z

Some additional comments

N-o-Z · 2024-06-05T10:21:56Z

esti/s3_gateway_test.go

+		require.NotNil(t, listErr)
+		require.Contains(t, listErr.Error(), `Repository "api" not found`)


Suggested change

require.NotNil(t, listErr)

require.Contains(t, listErr.Error(), `Repository "api" not found`)

require.ErrorContains(t, listErr, gtwerrors.ErrNoSuchBucketPossibleAPIEndpoint.Error())

N-o-Z · 2024-06-05T10:22:15Z

esti/s3_gateway_test.go

+		s3Client, err := testutil.SetupTestS3Client(endpoint, accessKeyID, secretAccessKey)
+		require.NoError(t, err, "failed creating s3 client")
+
+		_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("test")})


Suggested change

_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("test")})

_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("not-exists")})

N-o-Z · 2024-06-05T10:24:36Z

esti/s3_gateway_test.go

+		_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("test")})
+		require.NotNil(t, listErr)
+		require.NotContains(t, listErr.Error(), `Repository "api" not found`)


Suggested change

_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("test")})

require.NotNil(t, listErr)

require.NotContains(t, listErr.Error(), `Repository "api" not found`)

_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("not-exist")})

require.ErrorContains(t, listErr, gtwerrors.ErrNoSuchBucket.Error())

itaigilo · 2024-06-05T14:50:32Z

Rewrote the tests, PTAL 🙏

N-o-Z

Excellent work! I have one suggestion but not blocking

N-o-Z · 2024-06-06T07:14:27Z

pkg/testutil/setup.go

N-o-Z · 2024-06-06T07:16:39Z

esti/s3_gateway_test.go

@@ -658,30 +658,27 @@ func TestS3ReadObjectRedirect(t *testing.T) {
 	})
 }

+func createS3Client(endpoint string, t *testing.T) *s3.Client {


Just as thought,
There are several places who use testutil.SetupTestS3Client to change the endpoint (as you already noticed).
Suggest to put this func under the testutil package and use it in all the places where testutil.SetupTestS3Client is called.

I would prefer not to do this. viper state is, by definition, global. So I want all uses of viper to appear in the test, rather than in a "don't worry about it" setup function.

arielshaqed

Thanks!

Would really like not to read config in testutil.

arielshaqed · 2024-06-06T09:01:49Z

esti/catalog_export_test.go

@@ -177,7 +177,7 @@ func testSymlinkS3Exporter(t *testing.T, ctx context.Context, repo string, tmplD
 	storageURL, err := url.Parse(symlinksPrefix)
 	require.NoError(t, err, "failed extracting bucket name")

-	s3Client, err := testutil.SetupTestS3Client("https://s3.amazonaws.com", testData.AWSAccessKeyID, testData.AWSSecretAccessKey)
+	s3Client, err := testutil.SetupTestS3Client("https://s3.amazonaws.com", testData.AWSAccessKeyID, testData.AWSSecretAccessKey, viper.GetBool("force_path_style"))


I'd prefer the same style as in delete_objects_test.go, below. But what I really want is for both calls to look the same.

arielshaqed · 2024-06-06T09:03:13Z

esti/s3_gateway_test.go

@@ -658,30 +658,27 @@ func TestS3ReadObjectRedirect(t *testing.T) {
 	})
 }

+func createS3Client(endpoint string, t *testing.T) *s3.Client {


I would prefer not to do this. viper state is, by definition, global. So I want all uses of viper to appear in the test, rather than in a "don't worry about it" setup function.

Return an informative error message when using 'api/v1' for S3 GW end…

a9cd490

…point

itaigilo requested a review from arielshaqed June 3, 2024 13:37

itaigilo added include-changelog PR description should be included in next release changelog area/gateway Changes to the gateway labels Jun 3, 2024

arielshaqed requested changes Jun 3, 2024

View reviewed changes

Update const and comment

a33c847

itaigilo requested review from arielshaqed and N-o-Z June 3, 2024 14:09

Add system test

b6f532b

arielshaqed approved these changes Jun 3, 2024

View reviewed changes

arielshaqed requested changes Jun 3, 2024

View reviewed changes

N-o-Z requested changes Jun 3, 2024

View reviewed changes

itaigilo added 3 commits June 3, 2024 22:03

Fix tests

d48f4fc

Update error message

a13940c

Fix lint

13af4df

itaigilo added 2 commits June 5, 2024 10:37

Update test

7a18b48

Fix test

d0a58f4

N-o-Z requested changes Jun 5, 2024

View reviewed changes

itaigilo added 4 commits June 5, 2024 13:45

Clarify test

140164c

Fix test

0f0fdde

Fix test

57a5be2

Refactor tests

1547bcf

itaigilo requested review from arielshaqed and N-o-Z June 5, 2024 14:51

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

View reviewed changes

arielshaqed approved these changes Jun 6, 2024

View reviewed changes

Improve code style

52f7251

itaigilo merged commit 7ef5337 into master Jun 6, 2024
35 checks passed

itaigilo deleted the feature/show-informative-error-for-s3-gw-api-endpoint-usage branch June 6, 2024 09:34

	Description: "Using lakeFS API URI as the endpoint. Try removing the 'api/v1/' part from the endpoint.",
	Description: `Repository "api" not found; this can happen if your endpoint URL mistakenly ends in "` + apiutil.BaseURL + `".`,

	endpoint := "/api/v1" + viper.GetString("s3_endpoint")
	endpoint := viper.GetString("s3_endpoint") + apiutil.BaseURL

	t.Run("use_api_v1_for_client_endpoint", func(t *testing.T) {
	t.Run("use_open_api_for_client_endpoint", func(t *testing.T) {

		require.NotNil(t, listErr)
		require.Contains(t, listErr.Error(), `Repository "api" not found`)

	require.NotNil(t, listErr)
	require.Contains(t, listErr.Error(), `Repository "api" not found`)
	require.ErrorContains(t, listErr, gtwerrors.ErrNoSuchBucketPossibleAPIEndpoint.Error())

	_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("test")})
	_, listErr := s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String("not-exists")})

Return an informative error message when using 'api/v1' for S3 GW endpoint #7828

Return an informative error message when using 'api/v1' for S3 GW endpoint #7828

Conversation

itaigilo commented Jun 3, 2024

Change Description

github-actions bot commented Jun 3, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

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

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

arielshaqed 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

itaigilo commented Jun 4, 2024

N-o-Z commented Jun 4, 2024

itaigilo commented Jun 4, 2024 • edited Loading

N-o-Z commented Jun 4, 2024

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

itaigilo commented Jun 5, 2024

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

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

itaigilo commented Jun 4, 2024 •

edited

Loading