Added bucket-lookup-type field to s3 config (grafana#3788)

* Added bucket-lookup-type field to s3 config

* Review fixes: validation & deprecation warning

* Update reference docs

* Formatted code

---------

Co-authored-by: Christian Simon <[email protected]>

Author: 2 people

cmd/pyroscope/help-all.txt.tmpl

@@ -901,14 +901,16 @@ 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
     	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.
   -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

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)
 }
 
@@ -39,8 +44,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{
@@ -67,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.")
+	}
+}

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 != AutoLookup && cfg.BucketLookupType != PathStyleLookup {
+		return errBucketLookupConfigConflict
+	}
+
+	if !lo.Contains(supportedBucketLookupTypes, cfg.BucketLookupType) {
+		return errUnsupportedBucketLookupType
+	}
+
 	if err := cfg.SSE.Validate(); err != nil {
 		return err
 	}

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())
+		})
+	}
+}

pkg/phlare/phlare.go

@@ -31,14 +31,15 @@ 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"
 	"github.com/prometheus/common/version"
 	"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"