[#147] Add Kludge profiles #568

Merged
alexvanin merged 1 commit from pogpp/frostfs-s3-gw:feature/147_kludge_profiles into master 2024-12-13 11:25:08 +00:00
Member

close #147
Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

close #147 Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp requested review from alexvanin 2024-12-05 14:47:32 +00:00
pogpp requested review from dkirillov 2024-12-05 14:47:32 +00:00
pogpp requested review from mbiryukova 2024-12-05 14:48:00 +00:00
pogpp requested review from r.loginov 2024-12-05 14:48:01 +00:00
pogpp requested review from nzinkevich 2024-12-05 14:48:02 +00:00
r.loginov reviewed 2024-12-05 20:58:54 +00:00
@ -82,1 +82,4 @@
func (c *configMock) KludgeProfiles() map[string][]KludgeParams {
var params map[string][]KludgeParams
return params
Member

nitpick: can just return map[string][]KludgeParams{}

nitpick: can just `return map[string][]KludgeParams{}`
Member

This creates and returns a nil map. So we can simplify to return nil.
But probably we should return not nil but empty map then we should use:

  • make(map[string][]KludgeParams) or
  • map[string][]KludgeParams{} as @r.loginov suggested
This creates and returns a `nil` map. So we can simplify to `return nil`. But probably we should return not `nil` but empty map then we should use: * `make(map[string][]KludgeParams)` or * `map[string][]KludgeParams{}` as @r.loginov suggested
r.loginov marked this conversation as resolved
@ -335,0 +334,4 @@
var profile []KludgeParams
profiles := h.cfg.KludgeProfiles()
for s := range profiles {
if strings.Contains(r.UserAgent(), s) {
Member

question: The issue comment states that the user-agent should have a regular expression, but here we just look at the presence of a substring in the string. Is this how it should be?

question: The [issue](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/147#issuecomment-58291) comment states that the user-agent should have a regular expression, but here we just look at the presence of a substring in the string. Is this how it should be?
Member

@pogpp what about this question?

@pogpp what about this question?
Owner

I mentioned regexp in the issue, but I don't have strong opinion on that. Regexp is very powerful tool, so basic string matching should be good for the beginning, I think.

I mentioned regexp in the issue, but I don't have strong opinion on that. Regexp is very powerful tool, so basic string matching should be good for the beginning, I think.
r.loginov marked this conversation as resolved
@ -335,0 +340,4 @@
}
defBypass := h.cfg.BypassContentEncodingInChunks()
if !chunkedEncoding && !defBypass && (profile == nil || (profile != nil && profile[1].IsSet && defBypass != profile[1].Value)) {
Member

It seems that it is possible not to do the check profile != nil.

It seems that it is possible not to do the check `profile != nil`.
r.loginov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -460,0 +474,4 @@
}
}
println("here", profile)
Member

it looks like it needs to be deleted

it looks like it needs to be deleted
r.loginov marked this conversation as resolved
dkirillov reviewed 2024-12-06 13:56:53 +00:00
@ -37,3 +37,4 @@
ResolveZoneList() []string
IsResolveListAllow() bool
BypassContentEncodingInChunks() bool
KludgeProfiles() map[string][]KludgeParams
Member

Actually we should use the same approach for BypassContentEncodingInChunks and in other kludge settings as for NewXMLDecoder. So just pass user-agent. That way we don't need adding KludgeProfiles() map[string][]KludgeParams method to interface

Actually we should use the same approach for `BypassContentEncodingInChunks` and in other kludge settings as for `NewXMLDecoder`. So just pass user-agent. That way we don't need adding `KludgeProfiles() map[string][]KludgeParams` method to interface
dkirillov marked this conversation as resolved
@ -29,3 +29,3 @@
)
if err := p.NewDecoder(tee).Decode(cors); err != nil {
if err := p.NewDecoder(tee, "").Decode(cors); err != nil {
Member

I would expect user-agent here also.

I would expect user-agent here also.
dkirillov marked this conversation as resolved
@ -188,3 +188,3 @@
func (h *handlerMock) HeadObjectHandler(http.ResponseWriter, *http.Request) {
//TODO implement me
// TODO implement me
Member

Why?
I would not like to see this unrelated changes. Or at least it should be separate commit.

Why? I would not like to see this unrelated changes. Or at least it should be separate commit.
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -458,2 +467,3 @@
s.mu.RLock()
if s.defaultXMLNS {
var profile []handler.KludgeParams
profiles := s.KludgeProfiles()
Member

Actually, as we have already locked we can juset use s.kludgeProfiles

Actually, as we have already locked we can juset use `s.kludgeProfiles`
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -460,3 +479,4 @@
if s.defaultXMLNS && (profile == nil || (profile != nil && profile[0].IsSet && profile[0].Value)) {
dec.DefaultSpace = awsDefaultNamespace
}
s.mu.RUnlock()
Member

Let's move this near to s.mu.RLock() like:

s.mu.RLock()
defer s.mu.RUnlock()
Let's move this near to `s.mu.RLock()` like: ```golang s.mu.RLock() defer s.mu.RUnlock() ```
dkirillov marked this conversation as resolved
@ -563,6 +564,34 @@ func fetchDefaultCopiesNumbers(l *zap.Logger, v *viper.Viper) []uint32 {
return result
}
func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams {
Member

We should use something like this:

diff --git a/cmd/s3-gw/app_settings.go b/cmd/s3-gw/app_settings.go
index 2e767e10..06502b7a 100644
--- a/cmd/s3-gw/app_settings.go
+++ b/cmd/s3-gw/app_settings.go
@@ -564,8 +564,13 @@ func fetchDefaultCopiesNumbers(l *zap.Logger, v *viper.Viper) []uint32 {
 	return result
 }
 
-func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams {
-	kludgeProfiles := make(map[string][]handler.KludgeParams)
+type KludgeParams struct {
+	UseDefaultXMLNS                    bool
+	BypassContentEncodingCheckInChunks bool
+}
+
+func fetchKludgeProfiles(v *viper.Viper) map[string]*KludgeParams {
+	kludgeProfiles := make(map[string]*KludgeParams)
 	for i := 0; ; i++ {
 		key := cfgKludgeProfile + "." + strconv.Itoa(i) + "."
 		userAgent := v.GetString(key + "user_agent")
@@ -573,20 +578,19 @@ func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams {
 			break
 		}
 
-		flags := make([]handler.KludgeParams, 2)
-		isXMLNSSet := v.IsSet(key + "use_default_xmlns")
-		if isXMLNSSet {
-			flags[0].IsSet = true
-			flags[0].Value = v.GetBool(key + "use_default_xmlns")
+		kludgeParam := &KludgeParams{
+			UseDefaultXMLNS:                    v.GetBool(cfgKludgeUseDefaultXMLNS),
+			BypassContentEncodingCheckInChunks: v.GetBool(cfgKludgeBypassContentEncodingCheckInChunks),
+		}
+		if v.IsSet(key + "use_default_xmlns") {
+			kludgeParam.UseDefaultXMLNS = v.GetBool(key + "use_default_xmlns")
 		}
 
-		isBypassSet := v.IsSet(key + "bypass_content_encoding_check_in_chunks")
-		if isBypassSet {
-			flags[1].IsSet = true
-			flags[1].Value = v.GetBool(key + "bypass_content_encoding_check_in_chunks")
+		if v.IsSet(key + "bypass_content_encoding_check_in_chunks") {
+			kludgeParam.BypassContentEncodingCheckInChunks = v.GetBool(key + "bypass_content_encoding_check_in_chunks")
 		}
 
-		kludgeProfiles[userAgent] = flags
+		kludgeProfiles[userAgent] = kludgeParam
 	}
 
 	return kludgeProfiles

We should use something like this: ```diff diff --git a/cmd/s3-gw/app_settings.go b/cmd/s3-gw/app_settings.go index 2e767e10..06502b7a 100644 --- a/cmd/s3-gw/app_settings.go +++ b/cmd/s3-gw/app_settings.go @@ -564,8 +564,13 @@ func fetchDefaultCopiesNumbers(l *zap.Logger, v *viper.Viper) []uint32 { return result } -func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams { - kludgeProfiles := make(map[string][]handler.KludgeParams) +type KludgeParams struct { + UseDefaultXMLNS bool + BypassContentEncodingCheckInChunks bool +} + +func fetchKludgeProfiles(v *viper.Viper) map[string]*KludgeParams { + kludgeProfiles := make(map[string]*KludgeParams) for i := 0; ; i++ { key := cfgKludgeProfile + "." + strconv.Itoa(i) + "." userAgent := v.GetString(key + "user_agent") @@ -573,20 +578,19 @@ func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams { break } - flags := make([]handler.KludgeParams, 2) - isXMLNSSet := v.IsSet(key + "use_default_xmlns") - if isXMLNSSet { - flags[0].IsSet = true - flags[0].Value = v.GetBool(key + "use_default_xmlns") + kludgeParam := &KludgeParams{ + UseDefaultXMLNS: v.GetBool(cfgKludgeUseDefaultXMLNS), + BypassContentEncodingCheckInChunks: v.GetBool(cfgKludgeBypassContentEncodingCheckInChunks), + } + if v.IsSet(key + "use_default_xmlns") { + kludgeParam.UseDefaultXMLNS = v.GetBool(key + "use_default_xmlns") } - isBypassSet := v.IsSet(key + "bypass_content_encoding_check_in_chunks") - if isBypassSet { - flags[1].IsSet = true - flags[1].Value = v.GetBool(key + "bypass_content_encoding_check_in_chunks") + if v.IsSet(key + "bypass_content_encoding_check_in_chunks") { + kludgeParam.BypassContentEncodingCheckInChunks = v.GetBool(key + "bypass_content_encoding_check_in_chunks") } - kludgeProfiles[userAgent] = flags + kludgeProfiles[userAgent] = kludgeParam } return kludgeProfiles ```
dkirillov marked this conversation as resolved
@ -189,0 +189,4 @@
# Kludge profiles
S3_GW_KLUDGE_PROFILE_1_USER_AGENT=aws-cli
S3_GW_KLUDGE_PROFILE_1_USE_DEFAULT_XMLNS=true
S3_GW_KLUDGE_PROFILE_1_BYPASS_CONTENT_ENCODING_CHECK_IN_CHUNKS=true
Member

It seem we should count starts with 0, not 1

It seem we should count starts with `0`, not `1`
Author
Member

It seem we should count starts with 0, not 1

S3_GW_MULTINET_SUBNETS*
messed me up

> It seem we should count starts with `0`, not `1` `S3_GW_MULTINET_SUBNETS*` messed me up
dkirillov marked this conversation as resolved
pogpp self-assigned this 2024-12-06 13:57:37 +00:00
pogpp force-pushed feature/147_kludge_profiles from dc9184c8b1 to 510a20fd66 2024-12-08 11:52:42 +00:00 Compare
pogpp force-pushed feature/147_kludge_profiles from 510a20fd66 to 95637630b9 2024-12-08 12:02:43 +00:00 Compare
mbiryukova reviewed 2024-12-09 04:25:25 +00:00
@ -59,6 +59,11 @@ type (
type RetryStrategy string
type KludgeParams struct {
Member

Seems to be unused

Seems to be unused
mbiryukova marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -399,0 +407,4 @@
}
}
return s.bypassContentEncodingInChunks || (profile != nil && profile.BypassContentEncodingCheckInChunks)
Member

I guess if profile != nil we should just return profile.BypassContentEncodingCheckInChunks

I guess if `profile != nil` we should just return `profile.BypassContentEncodingCheckInChunks`
mbiryukova marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -399,0 +410,4 @@
return s.bypassContentEncodingInChunks || (profile != nil && profile.BypassContentEncodingCheckInChunks)
}
func (s *appSettings) KludgeProfiles() map[string]*KludgeParams {
Member

Seems to be unused

Seems to be unused
mbiryukova marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -460,0 +484,4 @@
}
}
if s.defaultXMLNS || (profile != nil && profile.UseDefaultXMLNS) {
Member

The same as for profile.BypassContentEncodingCheckInChunks

The same as for `profile.BypassContentEncodingCheckInChunks`
mbiryukova marked this conversation as resolved
pogpp force-pushed feature/147_kludge_profiles from 95637630b9 to 65dec718cc 2024-12-09 08:25:54 +00:00 Compare
pogpp force-pushed feature/147_kludge_profiles from 65dec718cc to 2a4b11dfb2 2024-12-09 08:32:45 +00:00 Compare
pogpp force-pushed feature/147_kludge_profiles from 2a4b11dfb2 to 17ec2aa0f1 2024-12-09 08:58:59 +00:00 Compare
pogpp force-pushed feature/147_kludge_profiles from 928ecc1d9f to 539af718b2 2024-12-09 09:16:59 +00:00 Compare
dkirillov approved these changes 2024-12-10 14:26:37 +00:00
Dismissed
dkirillov left a comment
Member

LGTM, but see comment #568 (comment)

LGTM, but see comment https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/568#issuecomment-60205
alexvanin added this to the v0.32.0 milestone 2024-12-11 07:39:32 +00:00
alexvanin reviewed 2024-12-11 12:53:43 +00:00
cmd/s3-gw/app.go Outdated
@ -398,0 +404,4 @@
for p := range profiles {
if strings.Contains(agent, p) {
profile = profiles[p]
break
Owner

I think you can simplify this even further.

for p := range profiles {
    if strings.Contains(agent, p) {
        return profiles[p].BypassContentEncodingCheckInChunks
    }
}
return s.bypassContentEncodingInChunks

The similar thing with xml decoder

for p := range s.kludgeProfiles {
	if strings.Contains(agent, p) && s.kludgeProfiles[p].UseDefaultXMLNS {
		dec.DefaultSpace = awsDefaultNamespace
		return
	}
}
if s.defaultXMLNS {
	dec.DefaultSpace = awsDefaultNamespace
}
I think you can simplify this even further. ```go for p := range profiles { if strings.Contains(agent, p) { return profiles[p].BypassContentEncodingCheckInChunks } } return s.bypassContentEncodingInChunks ``` The similar thing with xml decoder ```go for p := range s.kludgeProfiles { if strings.Contains(agent, p) && s.kludgeProfiles[p].UseDefaultXMLNS { dec.DefaultSpace = awsDefaultNamespace return } } if s.defaultXMLNS { dec.DefaultSpace = awsDefaultNamespace } ```
Member

I'm not sure about xml decoder
It seems the correct way is

for p := range s.kludgeProfiles {
	if strings.Contains(agent, p) {
	    if s.kludgeProfiles[p].UseDefaultXMLNS {
		    dec.DefaultSpace = awsDefaultNamespace
		}
		return
	}
}

Otherwise if we override use_default_xmlns in profile as false (but globally it's true) we could get wrong behavior

I'm not sure about xml decoder It seems the correct way is ```golang for p := range s.kludgeProfiles { if strings.Contains(agent, p) { if s.kludgeProfiles[p].UseDefaultXMLNS { dec.DefaultSpace = awsDefaultNamespace } return } } ``` Otherwise if we override `use_default_xmlns` in profile as `false` (but globally it's `true`) we could get wrong behavior
Owner

Oh I see. If we matched kludge profile, we should ignore s.defaultXMLNS. My bad. So it should be

for p := range s.kludgeProfiles {
	if strings.Contains(agent, p) {
	    if s.kludgeProfiles[p].UseDefaultXMLNS {
		    dec.DefaultSpace = awsDefaultNamespace
		}
		return dec
	}
}
if s.defaultXMLNS {
	dec.DefaultSpace = awsDefaultNamespace
}
return dec
Oh I see. If we matched kludge profile, we should ignore `s.defaultXMLNS`. My bad. So it should be ```go for p := range s.kludgeProfiles { if strings.Contains(agent, p) { if s.kludgeProfiles[p].UseDefaultXMLNS { dec.DefaultSpace = awsDefaultNamespace } return dec } } if s.defaultXMLNS { dec.DefaultSpace = awsDefaultNamespace } return dec ```
alexvanin reviewed 2024-12-11 12:56:51 +00:00
@ -566,0 +587,4 @@
}
if v.IsSet(key + "bypass_content_encoding_check_in_chunks") {
kludgeParam.BypassContentEncodingCheckInChunks = v.GetBool(key + "bypass_content_encoding_check_in_chunks")
Owner

nitpick: you can define bypass_content_encoding_check_in_chunks and use_default_xmlns and reuse it.

nitpick: you can define `bypass_content_encoding_check_in_chunks` and `use_default_xmlns` and reuse it.
alexvanin marked this conversation as resolved
alexvanin reviewed 2024-12-11 12:58:31 +00:00
@ -639,6 +645,7 @@ kludge:
| `use_default_xmlns` | `bool` | yes | `false` | Enable using default xml namespace `http://s3.amazonaws.com/doc/2006-03-01/` when parse xml bodies. |
| `bypass_content_encoding_check_in_chunks` | `bool` | yes | `false` | Use this flag to be able to use [chunked upload approach](https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html) without having `aws-chunked` value in `Content-Encoding` header. |
| `default_namespaces` | `[]string` | yes | `["","root"]` | Namespaces that should be handled as default. |
| `profile.user_agent` | `string` | yes | | Request UserAgent value. |
Owner

See how we define attributes in tracing section. profile should be a separate message defined in the document.

See how we define attributes in `tracing` section. `profile` should be a separate message defined in the document.
alexvanin marked this conversation as resolved
alexvanin requested changes 2024-12-11 12:59:52 +00:00
Dismissed
alexvanin left a comment
Owner

Please, update configuration.md.

Please, update `configuration.md`.
pogpp force-pushed feature/147_kludge_profiles from 539af718b2 to e81491f8b2 2024-12-11 14:28:29 +00:00 Compare
pogpp dismissed dkirillov's review 2024-12-11 14:28:30 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

pogpp force-pushed feature/147_kludge_profiles from e81491f8b2 to 66c0a8be02 2024-12-11 14:34:14 +00:00 Compare
dkirillov reviewed 2024-12-12 07:41:01 +00:00
@ -643,0 +661,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|-------------------------------------------|----------|---------------|---------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `user_agent` | `string` | yes | | Profile user agent. |
Member

Can we mention that it's be matched as substring?

Can we mention that it's be matched as substring?
dkirillov marked this conversation as resolved
dkirillov requested changes 2024-12-12 07:41:17 +00:00
Dismissed
dkirillov left a comment
Member
See https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/568#issuecomment-61362
pogpp force-pushed feature/147_kludge_profiles from 66c0a8be02 to 8fcb04e209 2024-12-12 09:17:55 +00:00 Compare
pogpp force-pushed feature/147_kludge_profiles from 8fcb04e209 to 7a1a7be970 2024-12-12 10:42:42 +00:00 Compare
alexvanin approved these changes 2024-12-12 13:54:53 +00:00
dkirillov approved these changes 2024-12-12 14:00:08 +00:00
mbiryukova approved these changes 2024-12-13 04:23:25 +00:00
r.loginov approved these changes 2024-12-13 05:27:28 +00:00
alexvanin merged commit d986e74897 into master 2024-12-13 11:25:08 +00:00
alexvanin deleted branch feature/147_kludge_profiles 2024-12-13 11:25:20 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 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#568
No description provided.