Add support VHS #446

Merged
alexvanin merged 1 commit from r.loginov/frostfs-s3-gw:feature/vhs into feature/virtual-hosted-style 2024-09-04 19:51:14 +00:00
Member
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
Signed-off-by: Roman Loginov <r.loginov@yadro.com>
[##] Add tests for address style middleware
Some checks failed
/ DCO (pull_request) Failing after 57s
/ Vulncheck (pull_request) Successful in 1m4s
/ Builds (1.21) (pull_request) Successful in 1m25s
/ Builds (1.22) (pull_request) Successful in 1m21s
/ Lint (pull_request) Successful in 2m13s
/ Tests (1.21) (pull_request) Successful in 1m28s
/ Tests (1.22) (pull_request) Successful in 1m24s
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
@ -42,6 +42,13 @@ server:
# Domains to be able to use virtual-hosted-style access to bucket.
listen_domains:
Author
Member

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?
Member

I think we should.

I think we should.
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
@ -0,0 +83,4 @@
}
}
func checkDomain(requestURL string, domains []string) (bktName string, match bool) {
Member

Isn't it better to rename requestURL to host?

Isn't it better to rename `requestURL` to `host`?
dkirillov marked this conversation as resolved
@ -0,0 +101,4 @@
}
var patternIndex, hostIndex int
for i := 0; i < len(pattern); i++ {
Member

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 } ```
Author
Member

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)).
Member

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.
@ -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 {
Member

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

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

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))
Member

I suppose now we can write:

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

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

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

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.
api/router.go Outdated
@ -164,3 +159,1 @@
hr.Map(domain, bucketRouter(cfg.Handler, cfg.Log))
}
api.Mount("/", hr)
vhsRouter := bucketRouter(cfg.Handler)
Member

Should we also write attachErrorHandler(vhsRouter)?

Should we also write `attachErrorHandler(vhsRouter)`?
Author
Member

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)
Member

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`
Author
Member

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.
Member

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 } } ```
cmd/s3-gw/app.go Outdated
@ -248,0 +260,4 @@
defer s.mu.Unlock()
s.domains = fetchDomains(v)
s.vhsEnabled = v.GetBool(cfgVHSEnabled)
Member

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 ```
dkirillov marked this conversation as resolved
@ -670,0 +703,4 @@
}
if len(vhsNamespacesEnabled) > 0 {
return vhsNamespacesEnabled
Member

It seems we can unconditionally return vhsNamespaceEnabled

It seems we can unconditionally return `vhsNamespaceEnabled`
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
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 deleted branch feature/vhs 2024-08-02 08:12:29 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#446
No description provided.