Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added bucket-lookup-type field to s3 config #3788

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/objstore/providers/s3/bucket_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ func newS3Config(cfg Config) (s3.Config, error) {
}

bucketLookupType := s3.AutoLookup
if cfg.ForcePathStyle {
if cfg.ForcePathStyle || cfg.BucketLookupType == PathStyleLookup {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we also should add a deprecation log warning when ForcePath argument is used, so users can notice it and migrate to BucketLookupType

Wdyt?

bucketLookupType = s3.PathLookup
} else if cfg.BucketLookupType == VirtualHostedStyleLookup {
bucketLookupType = s3.VirtualHostLookup
}

return s3.Config{
Expand Down
23 changes: 22 additions & 1 deletion pkg/objstore/providers/s3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,25 @@ const (
// SSES3 config type constant to configure S3 server side encryption with AES-256
// https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingServerSideEncryption.html
SSES3 = "SSE-S3"

// https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#path-style-access
PathStyleLookup = "path-style"

// https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#virtual-hosted-style-access
VirtualHostedStyleLookup = "virtual-hosted-style"

AutoLookup = "auto"
)

var (
supportedSignatureVersions = []string{SignatureVersionV4, SignatureVersionV2}
supportedSSETypes = []string{SSEKMS, SSES3}
supportedBucketLookupTypes = []string{PathStyleLookup, VirtualHostedStyleLookup, AutoLookup}
errUnsupportedSignatureVersion = errors.New("unsupported signature version")
errUnsupportedSSEType = errors.New("unsupported S3 SSE type")
errInvalidSSEContext = errors.New("invalid S3 SSE encryption context")
errBucketLookupConfigConflict = errors.New("cannot use s3.force-path-style = true and s3.bucket-lookup-type = virtual-hosted-style at the same time")
errUnsupportedBucketLookupType = errors.New("invalid S3 bucket lookup type")
)

// HTTPConfig stores the http.Transport configuration for the s3 minio client.
Expand Down Expand Up @@ -78,6 +89,7 @@ type Config struct {
Insecure bool `yaml:"insecure" category:"advanced"`
SignatureVersion string `yaml:"signature_version" category:"advanced"`
ForcePathStyle bool `yaml:"force_path_style" category:"advanced"`
BucketLookupType string `yaml:"bucket_lookup_type" category:"advanced"`

SSE SSEConfig `yaml:"sse"`
HTTP HTTPConfig `yaml:"http"`
Expand All @@ -96,7 +108,8 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.StringVar(&cfg.Region, prefix+"s3.region", "", "S3 region. If unset, the client will issue a S3 GetBucketLocation API call to autodetect it.")
f.StringVar(&cfg.Endpoint, prefix+"s3.endpoint", "", "The S3 bucket endpoint. It could be an AWS S3 endpoint listed at https://docs.aws.amazon.com/general/latest/gr/s3.html or the address of an S3-compatible service in hostname:port format.")
f.BoolVar(&cfg.Insecure, prefix+"s3.insecure", false, "If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.")
f.BoolVar(&cfg.ForcePathStyle, prefix+"s3.force-path-style", false, "Set this to `true` to force the bucket lookup to be using path-style.")
f.BoolVar(&cfg.ForcePathStyle, prefix+"s3.force-path-style", false, "Deprecated, use s3.bucket-lookup-type instead. Set this to `true` to force the bucket lookup to be using path-style.")
f.StringVar(&cfg.BucketLookupType, prefix+"s3.bucket-lookup-type", AutoLookup, fmt.Sprintf("S3 bucket lookup style, use one of: %v", supportedBucketLookupTypes))
f.StringVar(&cfg.SignatureVersion, prefix+"s3.signature-version", SignatureVersionV4, fmt.Sprintf("The signature version to use for authenticating against S3. Supported values are: %s.", strings.Join(supportedSignatureVersions, ", ")))
cfg.SSE.RegisterFlagsWithPrefix(prefix+"s3.sse.", f)
cfg.HTTP.RegisterFlagsWithPrefix(prefix, f)
Expand All @@ -108,6 +121,14 @@ func (cfg *Config) Validate() error {
return errUnsupportedSignatureVersion
}

if cfg.ForcePathStyle && cfg.BucketLookupType == VirtualHostedStyleLookup {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Might be slightly more failsafe, once there is another lookup type added (I hope not 😆 )

Suggested change
if cfg.ForcePathStyle && cfg.BucketLookupType == VirtualHostedStyleLookup {
if cfg.ForcePathStyle && cfg.BucketLookupType != AutoLookup && cfg.BucketLookupType != PathStyleLookup {

return errBucketLookupConfigConflict
}

if !lo.Contains(supportedBucketLookupTypes, cfg.BucketLookupType) {
return errUnsupportedBucketLookupType
}

if err := cfg.SSE.Validate(); err != nil {
return err
}
Expand Down
41 changes: 41 additions & 0 deletions pkg/objstore/providers/s3/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,44 @@ func TestParseKMSEncryptionContext(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, expected, actual)
}

func TestConfig_Validate(t *testing.T) {
tests := map[string]struct {
setup func() *Config
expected error
}{
"should pass with default config": {
setup: func() *Config {
cfg := &Config{}
flagext.DefaultValues(cfg)

return cfg
},
},
"should fail on invalid bucket lookup style": {
setup: func() *Config {
cfg := &Config{}
flagext.DefaultValues(cfg)
cfg.BucketLookupType = "invalid"
return cfg
},
expected: errUnsupportedBucketLookupType,
},
"should fail if force-path-style conflicts with bucket-lookup-type": {
setup: func() *Config {
cfg := &Config{}
flagext.DefaultValues(cfg)
cfg.ForcePathStyle = true
cfg.BucketLookupType = VirtualHostedStyleLookup
return cfg
},
expected: errBucketLookupConfigConflict,
},
}

for testName, testData := range tests {
t.Run(testName, func(t *testing.T) {
assert.Equal(t, testData.expected, testData.setup().Validate())
})
}
}
Loading