[#147] Add Kludge profiles #568
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#568
Loading…
Reference in a new issue
No description provided.
Delete branch "pogpp/frostfs-s3-gw:feature/147_kludge_profiles"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
close #147
Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com
@ -82,1 +82,4 @@
func (c *configMock) KludgeProfiles() map[string][]KludgeParams {
var params map[string][]KludgeParams
return params
nitpick: can just
return map[string][]KludgeParams{}
This creates and returns a
nil
map. So we can simplify toreturn nil
.But probably we should return not
nil
but empty map then we should use:make(map[string][]KludgeParams)
ormap[string][]KludgeParams{}
as @r.loginov suggested@ -335,0 +334,4 @@
var profile []KludgeParams
profiles := h.cfg.KludgeProfiles()
for s := range profiles {
if strings.Contains(r.UserAgent(), s) {
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?
@pogpp what about this question?
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.
@ -335,0 +340,4 @@
}
defBypass := h.cfg.BypassContentEncodingInChunks()
if !chunkedEncoding && !defBypass && (profile == nil || (profile != nil && profile[1].IsSet && defBypass != profile[1].Value)) {
It seems that it is possible not to do the check
profile != nil
.@ -460,0 +474,4 @@
}
}
println("here", profile)
it looks like it needs to be deleted
@ -37,3 +37,4 @@
ResolveZoneList() []string
IsResolveListAllow() bool
BypassContentEncodingInChunks() bool
KludgeProfiles() map[string][]KludgeParams
Actually we should use the same approach for
BypassContentEncodingInChunks
and in other kludge settings as forNewXMLDecoder
. So just pass user-agent. That way we don't need addingKludgeProfiles() map[string][]KludgeParams
method to interface@ -29,3 +29,3 @@
)
if err := p.NewDecoder(tee).Decode(cors); err != nil {
if err := p.NewDecoder(tee, "").Decode(cors); err != nil {
I would expect user-agent here also.
@ -188,3 +188,3 @@
func (h *handlerMock) HeadObjectHandler(http.ResponseWriter, *http.Request) {
//TODO implement me
// TODO implement me
Why?
I would not like to see this unrelated changes. Or at least it should be separate commit.
@ -458,2 +467,3 @@
s.mu.RLock()
if s.defaultXMLNS {
var profile []handler.KludgeParams
profiles := s.KludgeProfiles()
Actually, as we have already locked we can juset use
s.kludgeProfiles
@ -460,3 +479,4 @@
if s.defaultXMLNS && (profile == nil || (profile != nil && profile[0].IsSet && profile[0].Value)) {
dec.DefaultSpace = awsDefaultNamespace
}
s.mu.RUnlock()
Let's move this near to
s.mu.RLock()
like:@ -563,6 +564,34 @@ func fetchDefaultCopiesNumbers(l *zap.Logger, v *viper.Viper) []uint32 {
return result
}
func fetchKludgeProfiles(v *viper.Viper) map[string][]handler.KludgeParams {
We should use something like this:
@ -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
It seem we should count starts with
0
, not1
S3_GW_MULTINET_SUBNETS*
messed me up
dc9184c8b1
to510a20fd66
510a20fd66
to95637630b9
@ -59,6 +59,11 @@ type (
type RetryStrategy string
type KludgeParams struct {
Seems to be unused
@ -399,0 +407,4 @@
}
}
return s.bypassContentEncodingInChunks || (profile != nil && profile.BypassContentEncodingCheckInChunks)
I guess if
profile != nil
we should just returnprofile.BypassContentEncodingCheckInChunks
@ -399,0 +410,4 @@
return s.bypassContentEncodingInChunks || (profile != nil && profile.BypassContentEncodingCheckInChunks)
}
func (s *appSettings) KludgeProfiles() map[string]*KludgeParams {
Seems to be unused
@ -460,0 +484,4 @@
}
}
if s.defaultXMLNS || (profile != nil && profile.UseDefaultXMLNS) {
The same as for
profile.BypassContentEncodingCheckInChunks
95637630b9
to65dec718cc
65dec718cc
to2a4b11dfb2
2a4b11dfb2
to17ec2aa0f1
928ecc1d9f
to539af718b2
LGTM, but see comment #568 (comment)
@ -398,0 +404,4 @@
for p := range profiles {
if strings.Contains(agent, p) {
profile = profiles[p]
break
I think you can simplify this even further.
The similar thing with xml decoder
I'm not sure about xml decoder
It seems the correct way is
Otherwise if we override
use_default_xmlns
in profile asfalse
(but globally it'strue
) we could get wrong behaviorOh I see. If we matched kludge profile, we should ignore
s.defaultXMLNS
. My bad. So it should be@ -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")
nitpick: you can define
bypass_content_encoding_check_in_chunks
anduse_default_xmlns
and reuse it.@ -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. |
See how we define attributes in
tracing
section.profile
should be a separate message defined in the document.Please, update
configuration.md
.539af718b2
toe81491f8b2
New commits pushed, approval review dismissed automatically according to repository settings
e81491f8b2
to66c0a8be02
@ -643,0 +661,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|-------------------------------------------|----------|---------------|---------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `user_agent` | `string` | yes | | Profile user agent. |
Can we mention that it's be matched as substring?
See #568 (comment)
66c0a8be02
to8fcb04e209
8fcb04e209
to7a1a7be970