From ce5b13ff2e77ecd4b3c58cc4b8ca1b8a2a4713d7 Mon Sep 17 00:00:00 2001 From: devmentality Date: Tue, 17 Dec 2024 18:50:41 +0500 Subject: [PATCH 1/4] Added bucket-lookup-type field to s3 config --- pkg/objstore/providers/s3/bucket_client.go | 4 ++- pkg/objstore/providers/s3/config.go | 23 +++++++++++- pkg/objstore/providers/s3/config_test.go | 41 ++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/pkg/objstore/providers/s3/bucket_client.go b/pkg/objstore/providers/s3/bucket_client.go index 05630eddb3..5fca2d3eea 100644 --- a/pkg/objstore/providers/s3/bucket_client.go +++ b/pkg/objstore/providers/s3/bucket_client.go @@ -39,8 +39,10 @@ func newS3Config(cfg Config) (s3.Config, error) { } bucketLookupType := s3.AutoLookup - if cfg.ForcePathStyle { + if cfg.ForcePathStyle || cfg.BucketLookupType == PathStyleLookup { bucketLookupType = s3.PathLookup + } else if cfg.BucketLookupType == VirtualHostedStyleLookup { + bucketLookupType = s3.VirtualHostLookup } return s3.Config{ diff --git a/pkg/objstore/providers/s3/config.go b/pkg/objstore/providers/s3/config.go index 64f13f6601..c6307eb0b8 100644 --- a/pkg/objstore/providers/s3/config.go +++ b/pkg/objstore/providers/s3/config.go @@ -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. @@ -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"` @@ -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) @@ -108,6 +121,14 @@ func (cfg *Config) Validate() error { return errUnsupportedSignatureVersion } + if cfg.ForcePathStyle && cfg.BucketLookupType == VirtualHostedStyleLookup { + return errBucketLookupConfigConflict + } + + if !lo.Contains(supportedBucketLookupTypes, cfg.BucketLookupType) { + return errUnsupportedBucketLookupType + } + if err := cfg.SSE.Validate(); err != nil { return err } diff --git a/pkg/objstore/providers/s3/config_test.go b/pkg/objstore/providers/s3/config_test.go index 3a1d4a81f2..e827de30ab 100644 --- a/pkg/objstore/providers/s3/config_test.go +++ b/pkg/objstore/providers/s3/config_test.go @@ -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()) + }) + } +} From 7d22191337305f60efe64f15d6efdd7f8fa27c78 Mon Sep 17 00:00:00 2001 From: devmentality Date: Fri, 10 Jan 2025 19:01:49 +0700 Subject: [PATCH 2/4] Review fixes: validation & deprecation warning --- pkg/objstore/providers/s3/bucket_client.go | 11 +++++++++++ pkg/objstore/providers/s3/config.go | 2 +- pkg/phlare/phlare.go | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/objstore/providers/s3/bucket_client.go b/pkg/objstore/providers/s3/bucket_client.go index 5fca2d3eea..fed31b2bf7 100644 --- a/pkg/objstore/providers/s3/bucket_client.go +++ b/pkg/objstore/providers/s3/bucket_client.go @@ -7,6 +7,7 @@ package s3 import ( "github.com/go-kit/log" + "github.com/go-kit/log/level" "github.com/prometheus/common/model" "github.com/thanos-io/objstore" "github.com/thanos-io/objstore/providers/s3" @@ -19,6 +20,8 @@ func NewBucketClient(cfg Config, name string, logger log.Logger) (objstore.Bucke return nil, err } + warnForDeprecatedConfigFields(cfg, logger) + return s3.NewBucketWithConfig(logger, s3Cfg, name) } @@ -29,6 +32,8 @@ func NewBucketReaderClient(cfg Config, name string, logger log.Logger) (objstore return nil, err } + warnForDeprecatedConfigFields(cfg, logger) + return s3.NewBucketWithConfig(logger, s3Cfg, name) } @@ -69,3 +74,9 @@ func newS3Config(cfg Config) (s3.Config, error) { SignatureV2: cfg.SignatureVersion == SignatureVersionV2, }, nil } + +func warnForDeprecatedConfigFields(cfg Config, logger log.Logger) { + if cfg.ForcePathStyle { + level.Warn(logger).Log("msg", "S3 bucket client config has a deprecated s3.force-path-style flag set. Please, use s3.bucket-lookup-type instead.") + } +} diff --git a/pkg/objstore/providers/s3/config.go b/pkg/objstore/providers/s3/config.go index c6307eb0b8..c8724500bf 100644 --- a/pkg/objstore/providers/s3/config.go +++ b/pkg/objstore/providers/s3/config.go @@ -121,7 +121,7 @@ func (cfg *Config) Validate() error { return errUnsupportedSignatureVersion } - if cfg.ForcePathStyle && cfg.BucketLookupType == VirtualHostedStyleLookup { + if cfg.ForcePathStyle && cfg.BucketLookupType != AutoLookup && cfg.BucketLookupType != PathStyleLookup { return errBucketLookupConfigConflict } diff --git a/pkg/phlare/phlare.go b/pkg/phlare/phlare.go index 8f7205476f..05b55a9045 100644 --- a/pkg/phlare/phlare.go +++ b/pkg/phlare/phlare.go @@ -31,7 +31,6 @@ import ( "github.com/grafana/dskit/signals" "github.com/grafana/dskit/spanprofiler" wwtracing "github.com/grafana/dskit/tracing" - "github.com/grafana/pyroscope-go" grpcgw "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" "github.com/opentracing/opentracing-go" "github.com/prometheus/client_golang/prometheus" @@ -39,6 +38,7 @@ import ( "github.com/samber/lo" "google.golang.org/grpc/health" + "github.com/grafana/pyroscope-go" "github.com/grafana/pyroscope/pkg/api" apiversion "github.com/grafana/pyroscope/pkg/api/version" "github.com/grafana/pyroscope/pkg/cfg" From 948d25f5ac84dc8830e5ed3073da6902fcf44b88 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Mon, 13 Jan 2025 16:28:24 +0000 Subject: [PATCH 3/4] Update reference docs --- cmd/pyroscope/help-all.txt.tmpl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/pyroscope/help-all.txt.tmpl b/cmd/pyroscope/help-all.txt.tmpl index 09b32ba4fd..49a24a0756 100644 --- a/cmd/pyroscope/help-all.txt.tmpl +++ b/cmd/pyroscope/help-all.txt.tmpl @@ -899,6 +899,8 @@ Usage of ./pyroscope: JSON either from a Google Developers Console client_credentials.json file, or a Google Developers service account key. Needs to be valid JSON, not a filesystem path. -storage.s3.access-key-id string S3 access key ID + -storage.s3.bucket-lookup-type string + S3 bucket lookup style, use one of: [path-style virtual-hosted-style auto] (default "auto") -storage.s3.bucket-name string S3 bucket name -storage.s3.endpoint string @@ -906,7 +908,7 @@ Usage of ./pyroscope: -storage.s3.expect-continue-timeout duration The time to wait for a server's first response headers after fully writing the request headers if the request has an Expect header. 0 to send the request body immediately. (default 1s) -storage.s3.force-path-style - Set this to `true` to force the bucket lookup to be using path-style. + Deprecated, use s3.bucket-lookup-type instead. Set this to `true` to force the bucket lookup to be using path-style. -storage.s3.http.idle-conn-timeout duration The time an idle connection will remain idle before closing. (default 1m30s) -storage.s3.http.insecure-skip-verify From 41d346a9ce5e6d410e73c9afaab6e1d2fc7d7a9e Mon Sep 17 00:00:00 2001 From: devmentality Date: Tue, 14 Jan 2025 13:58:45 +0700 Subject: [PATCH 4/4] Formatted code --- pkg/phlare/phlare.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/phlare/phlare.go b/pkg/phlare/phlare.go index 05b55a9045..1a340ffdaa 100644 --- a/pkg/phlare/phlare.go +++ b/pkg/phlare/phlare.go @@ -39,6 +39,7 @@ import ( "google.golang.org/grpc/health" "github.com/grafana/pyroscope-go" + "github.com/grafana/pyroscope/pkg/api" apiversion "github.com/grafana/pyroscope/pkg/api/version" "github.com/grafana/pyroscope/pkg/cfg"