Add support VHS #446

r.loginov · 2024-07-31T09:44:55Z

r.loginov commented

2024-07-31 09:44:55 +00:00

No description provided.

r.loginov self-assigned this 2024-07-31 09:44:55 +00:00

r.loginov added 2 commits 2024-07-31 09:44:59 +00:00

[##] Add support virtual-hosted-style 82eee8a246

Signed-off-by: Roman Loginov <r.loginov@yadro.com>

[##] Add tests for address style middleware

/ DCO (pull_request) Failing after 57s

Details

/ Vulncheck (pull_request) Successful in 1m4s

Details

/ Builds (1.21) (pull_request) Successful in 1m25s

Details

/ Builds (1.22) (pull_request) Successful in 1m21s

Details

/ Lint (pull_request) Successful in 2m13s

Details

/ Tests (1.21) (pull_request) Successful in 1m28s

Details

/ Tests (1.22) (pull_request) Successful in 1m24s

Details

f6ee43a650

Signed-off-by: Roman Loginov <r.loginov@yadro.com>

r.loginov force-pushed feature/vhs from f6ee43a650 to fe34d400f8

2024-07-31 09:52:09 +00:00

Compare

r.loginov reviewed 2024-07-31 10:09:20 +00:00

config/config.yaml

					
				@ -42,6 +42,13 @@ server:

				# Domains to be able to use virtual-hosted-style access to bucket.

				listen_domains:

r.loginov commented

2024-07-31 10:08:36 +00:00

In theory, it may be that the user enters an incorrect domain, for example:

s3dev<invalid-placeholder>.frostfs.devenv
s3dev.<wildcard.frostfs.devenv
s3dev.<wild.card>.frostfs.devenv
and others
Do you think we should check the validity of these placeholders, or is it the user's responsibility?

In theory, it may be that the user enters an incorrect domain, for example: - `s3dev<invalid-placeholder>.frostfs.devenv` - `s3dev.<wildcard.frostfs.devenv` - `s3dev.<wild.card>.frostfs.devenv` - and others Do you think we should check the validity of these placeholders, or is it the user's responsibility?

dkirillov commented

2024-07-31 11:26:58 +00:00

I think we should.

👍 1

r.loginov requested review from storage-services-committers 2024-07-31 10:20:02 +00:00

r.loginov requested review from storage-services-developers 2024-07-31 10:20:02 +00:00

dkirillov reviewed 2024-07-31 12:41:30 +00:00

api/middleware/address_style.go Outdated

					
				@ -0,0 +83,4 @@

					}

				}

				func checkDomain(requestURL string, domains []string) (bktName string, match bool) {

dkirillov commented

2024-07-31 12:12:04 +00:00

Isn't it better to rename requestURL to host?

Isn't it better to rename `requestURL` to `host`?

👍 1

dkirillov marked this conversation as resolved

api/middleware/address_style.go Outdated

					
				@ -0,0 +101,4 @@

					}

					var patternIndex, hostIndex int

					for i := 0; i < len(pattern); i++ {

dkirillov commented

2024-07-31 12:39:20 +00:00

Maybe the following be more readable (if not just ignore this comment):

i, j := len(host)-1, len(pattern)-1
for j >= 0 && (pattern[j] == wildcardPlaceholder || host[i] == pattern[j]) {
	i--
	j--
}

switch i {
case -1:
	return "", true
case 0:
	return host[0], true
default:
	return "", false
}

Maybe the following be more readable (if not just ignore this comment): ```golang i, j := len(host)-1, len(pattern)-1 for j >= 0 && (pattern[j] == wildcardPlaceholder || host[i] == pattern[j]) { i-- j-- } switch i { case -1: return "", true case 0: return host[0], true default: return "", false } ```

👍 1

r.loginov commented

2024-08-01 09:56:30 +00:00

Yes. I added another condition to switch in order to correctly handle the case when our domain and pattern contain the same number of parts and the first part does not match (an example of a test for this).

Yes. I added another condition to switch in order to correctly handle the case when our domain and pattern contain the same number of parts and the first part does not match ([an example of a test for this](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/be9944ba37c6f748e2fe9e2e3c083e681c38d825/api/middleware/address_style_test.go#L244)).

dkirillov commented

2024-08-01 12:44:37 +00:00

It seems we can use

switch {
	case i == -1:
		return "", true
	case i == 0 && (j != 0 || host[i] == pattern[j]):
		return host[0], true
	default:
		return "", false
}

then.

It seems we can use ```golang switch { case i == -1: return "", true case i == 0 && (j != 0 || host[i] == pattern[j]): return host[0], true default: return "", false } ``` then.

👍 1

api/middleware/reqinfo.go Outdated

					
				@ -232,4 +201,0 @@

								// we have to do this because of

								// https://github.com/go-chi/chi/issues/641

								// https://github.com/go-chi/chi/issues/642

								if obj, err := url.PathUnescape(reqInfo.ObjectName); err != nil {

dkirillov commented

2024-07-31 11:57:42 +00:00

Do we the same in preparePathStyleAddress or we shouldn't ?

Do we the same in `preparePathStyleAddress` or we shouldn't ?

r.loginov commented

2024-08-01 09:52:15 +00:00

I could not reproduce the problem described in these articles, but the search itself has not yet been closed and I do not see a solution to this problem there. So I added this to preparePathStyleAddress.

I could not reproduce the problem described in these articles, but the search itself has not yet been closed and I do not see a solution to this problem there. So I added this to `preparePathStyleAddress`.

api/router.go Outdated

					
				@ -156,3 +154,2 @@

					defaultRouter := chi.NewRouter()

					defaultRouter.Mount(fmt.Sprintf("/{%s}", s3middleware.BucketURLPrm), bucketRouter(cfg.Handler, cfg.Log))

					defaultRouter.Get("/", named("ListBuckets", cfg.Handler.ListBucketsHandler))

					defaultRouter.Mount(fmt.Sprintf("/{%s}", s3middleware.BucketURLPrm), bucketRouter(cfg.Handler))

dkirillov commented

2024-07-31 12:01:10 +00:00

I suppose now we can write:

defaultRouter.Mount("/{bucket}", bucketRouter(cfg.Handler))

I suppose now we can write: ``` defaultRouter.Mount("/{bucket}", bucketRouter(cfg.Handler)) ```

r.loginov commented

2024-08-01 09:52:34 +00:00

You mean: defaultRouter.Mount("/{bucket}", bucketRouter(cfg.Handler))?

You mean: `defaultRouter.Mount("/{bucket}", bucketRouter(cfg.Handler))`?

dkirillov commented

2024-08-01 11:58:49 +00:00

Yes, sure. I've just meant that s3middleware.BucketURLPrm isn't used for getting value, so we can drop separate const for this.

Yes, sure. I've just meant that `s3middleware.BucketURLPrm` isn't used for getting value, so we can drop separate const for this.

👍 1

api/router.go Outdated

					
				@ -164,3 +159,1 @@

						hr.Map(domain, bucketRouter(cfg.Handler, cfg.Log))

					}

					api.Mount("/", hr)

					vhsRouter := bucketRouter(cfg.Handler)

dkirillov commented

2024-07-31 12:08:05 +00:00

Should we also write attachErrorHandler(vhsRouter)?

Should we also write `attachErrorHandler(vhsRouter)`?

r.loginov commented

2024-08-01 09:52:56 +00:00

No, you shouldn't, because bucketRouter already has it.

No, you shouldn't, because `bucketRouter` already [has it](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/fe34d400f8c040513cf1ad560490ab5a9c4eb9c2/api/router.go#L395).

api/router.go Outdated

					
				@ -181,0 +203,4 @@

							cfg.Handler.ListBucketsHandler(w, r)

						} else {

							reqInfo.API = s3middleware.ListObjectsV1Operation

							cfg.Handler.ListObjectsV1Handler(w, r)

dkirillov commented

2024-07-31 12:03:44 +00:00

Should we check query params to determine which version of listing is used?
If I understand correctly it can be one of

ListObjectsV1
ListObjectsV2
ListMultipartUploads
ListObjectVersins

Should we check query params to determine which version of listing is used? If I understand correctly it can be one of * `ListObjectsV1` * `ListObjectsV2` * `ListMultipartUploads` * `ListObjectVersins`

r.loginov commented

2024-08-01 09:54:18 +00:00

Oh, that's right. There may even be not only a listing, but almost all bucket operations for a GET request. It seems the shortest solution seems to be to call the listWrapper function in the default bucket handler for GET requests. At the same time, it will not break the routing logic with path-style addressing.

Oh, that's right. There may even be not only a listing, but almost all bucket operations for a GET request. It seems the shortest solution seems to be to call the `listWrapper` function [in the default bucket handler for GET requests](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/be9944ba37c6f748e2fe9e2e3c083e681c38d825/api/router.go#L312). At the same time, it will not break the routing logic with path-style addressing.

dkirillov commented

2024-08-01 13:08:11 +00:00

Let's add test:

func TestRouterListObjectsV2Domains(t *testing.T) {
	chiRouter := prepareRouter(t, enableVHSDomains("domain.com"))

	chiRouter.handler.buckets["bucket"] = &data.BucketInfo{}

	w := httptest.NewRecorder()
	r := httptest.NewRequest(http.MethodGet, "/", nil)
	r.Host = "bucket.domain.com"
	query := make(url.Values)
	query.Set(s3middleware.ListTypeQuery, "2")
	r.URL.RawQuery = query.Encode()

	chiRouter.ServeHTTP(w, r)
	resp := readResponse(t, w)
	require.Equal(t, s3middleware.ListObjectsV2Operation, resp.Method)
}

func enableVHSDomains(domains ...string) option {
	return func(cfg *Config) {
		setting := cfg.MiddlewareSettings.(*middlewareSettingsMock)
		setting.vhsEnabled = true
		setting.domains = domains
	}
}

Let's add test: ```golang func TestRouterListObjectsV2Domains(t *testing.T) { chiRouter := prepareRouter(t, enableVHSDomains("domain.com")) chiRouter.handler.buckets["bucket"] = &data.BucketInfo{} w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/", nil) r.Host = "bucket.domain.com" query := make(url.Values) query.Set(s3middleware.ListTypeQuery, "2") r.URL.RawQuery = query.Encode() chiRouter.ServeHTTP(w, r) resp := readResponse(t, w) require.Equal(t, s3middleware.ListObjectsV2Operation, resp.Method) } func enableVHSDomains(domains ...string) option { return func(cfg *Config) { setting := cfg.MiddlewareSettings.(*middlewareSettingsMock) setting.vhsEnabled = true setting.domains = domains } } ```

👍 1

cmd/s3-gw/app.go Outdated

					
				@ -248,0 +260,4 @@

					defer s.mu.Unlock()

					s.domains = fetchDomains(v)

					s.vhsEnabled = v.GetBool(cfgVHSEnabled)

dkirillov commented

2024-07-31 11:52:29 +00:00

Let's keep critical section as small as possible.

domains := fetchDomains(v)
vhsEnabled := v.GetBool(cfgVHSEnabled)

s.mu.Lock()
defer s.mu.Unlock()

s.domains = domains
s.vhsEnabled = vhsEnabled

Let's keep critical section as small as possible. ```golang domains := fetchDomains(v) vhsEnabled := v.GetBool(cfgVHSEnabled) s.mu.Lock() defer s.mu.Unlock() s.domains = domains s.vhsEnabled = vhsEnabled ```

👍 1

dkirillov marked this conversation as resolved

cmd/s3-gw/app_settings.go Outdated

					
				@ -670,0 +703,4 @@

					}

					if len(vhsNamespacesEnabled) > 0 {

						return vhsNamespacesEnabled

dkirillov commented

2024-07-31 11:50:10 +00:00

It seems we can unconditionally return vhsNamespaceEnabled

It seems we can unconditionally return `vhsNamespaceEnabled`

👍 1

dkirillov marked this conversation as resolved

r.loginov force-pushed feature/vhs from fe34d400f8 to be9944ba37

2024-08-01 09:48:07 +00:00

Compare

alexvanin approved these changes 2024-08-01 12:07:50 +00:00

r.loginov force-pushed feature/vhs from be9944ba37 to 94070b107b

2024-08-01 14:20:31 +00:00

Compare

~~r.loginov referenced this pull request 2024-08-01 14:38:04 +00:00~~

Add support X-Frostfs-S3-VHS and X-Frostfs-Servername headers #449

alexvanin changed title from ~~Add support VHS~~ to Add support VHS

2024-08-02 08:12:10 +00:00

alexvanin changed target branch from master to feature/virtual-hosted-style

2024-08-02 08:12:10 +00:00

alexvanin merged commit 94070b107b into feature/virtual-hosted-style

2024-08-02 08:12:28 +00:00

alexvanin referenced this pull request from a commit

2024-08-02 08:12:29 +00:00

[#446] Add support virtual-hosted-style

alexvanin deleted branch feature/vhs

2024-08-02 08:12:29 +00:00

alexvanin referenced this pull request from a commit

2024-08-02 08:12:29 +00:00

[#446] Add tests for address style middleware