config: ensure storage config defaults apply to named stores (#9650)

**What this PR does / why we need it**: Since named store config does not register any flags, storage configs defined under it do not get the defaults. For example [aws_storage_config](https://grafana.com/docs/loki/latest/configuration/#aws_storage_config) sets the default `storage_class` to `STANDARD`, but the same doesn't get applied by default when using named stores. This PR ensures that named storage configs are always assigned default values when they are unmarshalled by implementing `yaml.Unmarshaler` interface
grafana · Jun 9, 2023 · 98d1307 · 98d1307
1 parent 4cebc2d
commit 98d1307
Show file tree

Hide file tree

Showing 7 changed files with 314 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -45,6 +45,7 @@
 * [9463](https://github.com/grafana/loki/pull/9463) **Totalus**: Fix OpenStack Swift client object listing to fetch all the objects properly.
 * [9471](https://github.com/grafana/loki/pull/9471) **sandeepsukhani**: query-scheduler: fix query distribution in SSD mode.
 * [9495](https://github.com/grafana/loki/pull/9495) **thampiotr**: Promtail: Fix potential goroutine leak in file tailer.
+* [9650](https://github.com/grafana/loki/pull/9650) **ashwanthgoli**: Config: ensure storage config defaults apply to named stores.
 * [9629](https://github.com/grafana/loki/pull/9629) **periklis**: Fix duplicate label values from ingester streams.
 
 ##### Changes

diff --git a/pkg/loki/config_wrapper_test.go b/pkg/loki/config_wrapper_test.go
@@ -16,10 +16,14 @@ import (
 	"github.com/grafana/loki/pkg/distributor"
 	"github.com/grafana/loki/pkg/loki/common"
 	"github.com/grafana/loki/pkg/storage/bucket/swift"
+	"github.com/grafana/loki/pkg/storage/chunk/client/alibaba"
 	"github.com/grafana/loki/pkg/storage/chunk/client/aws"
 	"github.com/grafana/loki/pkg/storage/chunk/client/azure"
 	"github.com/grafana/loki/pkg/storage/chunk/client/baidubce"
 	"github.com/grafana/loki/pkg/storage/chunk/client/gcp"
+	"github.com/grafana/loki/pkg/storage/chunk/client/ibmcloud"
+	"github.com/grafana/loki/pkg/storage/chunk/client/local"
+	"github.com/grafana/loki/pkg/storage/chunk/client/openstack"
 	"github.com/grafana/loki/pkg/storage/config"
 	"github.com/grafana/loki/pkg/util"
 	"github.com/grafana/loki/pkg/util/cfg"
@@ -1702,3 +1706,136 @@ common:
 		assert.Equal(t, []string{"ringsshouldusethis"}, config.CompactorConfig.CompactorRing.InstanceInterfaceNames)
 	})
 }
+
+func TestNamedStores_applyDefaults(t *testing.T) {
+	namedStoresConfig := `storage_config:
+  named_stores:
+    aws:
+      store-1:
+        s3: "s3.test"
+        storage_class: GLACIER
+        dynamodb:
+          dynamodb_url: "dynamo.test"
+    azure:
+      store-2:
+        environment: AzureGermanCloud
+        account_name: foo
+        container_name: bar
+    bos:
+      store-3:
+        bucket_name: foobar
+    gcs:
+      store-4:
+        bucket_name: foobar
+        enable_http2: false
+    cos:
+      store-5:
+        endpoint: cos.test
+        http_config:
+          idle_conn_timeout: 30s
+    filesystem:
+      store-6:
+        directory: foobar
+    swift:
+      store-7:
+        container_name: foobar
+        request_timeout: 30s
+    alibabacloud:
+      store-8:
+        bucket: foobar
+        endpoint: oss.test
+`
+	// make goconst happy
+	bucketName := "foobar"
+
+	config, defaults, err := configWrapperFromYAML(t, namedStoresConfig, nil)
+	require.NoError(t, err)
+
+	nsCfg := config.StorageConfig.NamedStores
+
+	t.Run("aws", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.AWS, 1)
+
+		// expect the defaults to be set on named store config
+		expected := defaults.StorageConfig.AWSStorageConfig
+		assert.NoError(t, expected.DynamoDB.Set("dynamo.test"))
+		assert.NoError(t, expected.S3.Set("s3.test"))
+		// override defaults
+		expected.StorageClass = "GLACIER"
+
+		assert.Equal(t, expected, (aws.StorageConfig)(nsCfg.AWS["store-1"]))
+	})
+
+	t.Run("azure", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.Azure, 1)
+
+		expected := defaults.StorageConfig.AzureStorageConfig
+		expected.StorageAccountName = "foo"
+		expected.ContainerName = "bar"
+		// override defaults
+		expected.Environment = "AzureGermanCloud"
+
+		assert.Equal(t, expected, (azure.BlobStorageConfig)(nsCfg.Azure["store-2"]))
+	})
+
+	t.Run("bos", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.BOS, 1)
+
+		expected := defaults.StorageConfig.BOSStorageConfig
+		expected.BucketName = bucketName
+
+		assert.Equal(t, expected, (baidubce.BOSStorageConfig)(nsCfg.BOS["store-3"]))
+	})
+
+	t.Run("gcs", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.GCS, 1)
+
+		expected := defaults.StorageConfig.GCSConfig
+		expected.BucketName = bucketName
+		// override defaults
+		expected.EnableHTTP2 = false
+
+		assert.Equal(t, expected, (gcp.GCSConfig)(nsCfg.GCS["store-4"]))
+	})
+
+	t.Run("cos", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.COS, 1)
+
+		expected := defaults.StorageConfig.COSConfig
+		expected.Endpoint = "cos.test"
+		// override defaults
+		expected.HTTPConfig.IdleConnTimeout = 30 * time.Second
+
+		assert.Equal(t, expected, (ibmcloud.COSConfig)(nsCfg.COS["store-5"]))
+	})
+
+	t.Run("filesystem", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.Filesystem, 1)
+
+		expected := defaults.StorageConfig.FSConfig
+		expected.Directory = bucketName
+
+		assert.Equal(t, expected, (local.FSConfig)(nsCfg.Filesystem["store-6"]))
+	})
+
+	t.Run("swift", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.Swift, 1)
+
+		expected := defaults.StorageConfig.Swift
+		expected.ContainerName = bucketName
+		// override defaults
+		expected.RequestTimeout = 30 * time.Second
+
+		assert.Equal(t, expected, (openstack.SwiftConfig)(nsCfg.Swift["store-7"]))
+	})
+
+	t.Run("alibabacloud", func(t *testing.T) {
+		assert.Len(t, config.StorageConfig.NamedStores.AlibabaCloud, 1)
+
+		expected := defaults.StorageConfig.AlibabaStorageConfig
+		expected.Bucket = bucketName
+		expected.Endpoint = "oss.test"
+
+		assert.Equal(t, expected, (alibaba.OssConfig)(nsCfg.AlibabaCloud["store-8"]))
+	})
+}
diff --git a/pkg/storage/factory.go b/pkg/storage/factory.go
@@ -12,6 +12,8 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 
+	"github.com/grafana/dskit/flagext"
+
 	"github.com/grafana/loki/pkg/storage/chunk/cache"
 	"github.com/grafana/loki/pkg/storage/chunk/client"
 	"github.com/grafana/loki/pkg/storage/chunk/client/alibaba"
@@ -64,16 +66,103 @@ type StoreLimits interface {
 	CardinalityLimit(string) int
 }
 
+// Storage configs defined as Named stores don't get any defaults as they do not
+// register flags. To get around this we implement Unmarshaler interface that
+// assigns the defaults before calling unmarshal.
+
+// We cannot implement Unmarshaler directly on aws.StorageConfig or other stores
+// as it would end up overriding values set as part of ApplyDynamicConfig().
+// Note: we unmarshal a second time after applying dynamic configs
+//
+// Implementing the Unmarshaler for Named*StorageConfig types is fine as
+// we do not apply any dynamic config on them.
+
+type NamedAWSStorageConfig aws.StorageConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedAWSStorageConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*aws.StorageConfig)(cfg))
+	return unmarshal((*aws.StorageConfig)(cfg))
+}
+
+func (cfg *NamedAWSStorageConfig) Validate() error {
+	return (*aws.StorageConfig)(cfg).Validate()
+}
+
+type NamedBlobStorageConfig azure.BlobStorageConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedBlobStorageConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*azure.BlobStorageConfig)(cfg))
+	return unmarshal((*azure.BlobStorageConfig)(cfg))
+}
+
+func (cfg *NamedBlobStorageConfig) Validate() error {
+	return (*azure.BlobStorageConfig)(cfg).Validate()
+}
+
+type NamedBOSStorageConfig baidubce.BOSStorageConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedBOSStorageConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*baidubce.BOSStorageConfig)(cfg))
+	return unmarshal((*baidubce.BOSStorageConfig)(cfg))
+}
+
+type NamedFSConfig local.FSConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedFSConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*local.FSConfig)(cfg))
+	return unmarshal((*local.FSConfig)(cfg))
+}
+
+type NamedGCSConfig gcp.GCSConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedGCSConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*gcp.GCSConfig)(cfg))
+	return unmarshal((*gcp.GCSConfig)(cfg))
+}
+
+type NamedOssConfig alibaba.OssConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedOssConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*alibaba.OssConfig)(cfg))
+	return unmarshal((*alibaba.OssConfig)(cfg))
+}
+
+type NamedSwiftConfig openstack.SwiftConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedSwiftConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*openstack.SwiftConfig)(cfg))
+	return unmarshal((*openstack.SwiftConfig)(cfg))
+}
+
+func (cfg *NamedSwiftConfig) Validate() error {
+	return (*openstack.SwiftConfig)(cfg).Validate()
+}
+
+type NamedCOSConfig ibmcloud.COSConfig
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (cfg *NamedCOSConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	flagext.DefaultValues((*ibmcloud.COSConfig)(cfg))
+	return unmarshal((*ibmcloud.COSConfig)(cfg))
+}
+
 // NamedStores helps configure additional object stores from a given storage provider
 type NamedStores struct {
-	AWS          map[string]aws.StorageConfig         `yaml:"aws"`
-	Azure        map[string]azure.BlobStorageConfig   `yaml:"azure"`
-	BOS          map[string]baidubce.BOSStorageConfig `yaml:"bos"`
-	Filesystem   map[string]local.FSConfig            `yaml:"filesystem"`
-	GCS          map[string]gcp.GCSConfig             `yaml:"gcs"`
-	AlibabaCloud map[string]alibaba.OssConfig         `yaml:"alibabacloud"`
-	Swift        map[string]openstack.SwiftConfig     `yaml:"swift"`
-	COS          map[string]ibmcloud.COSConfig        `yaml:"cos"`
+	AWS          map[string]NamedAWSStorageConfig  `yaml:"aws"`
+	Azure        map[string]NamedBlobStorageConfig `yaml:"azure"`
+	BOS          map[string]NamedBOSStorageConfig  `yaml:"bos"`
+	Filesystem   map[string]NamedFSConfig          `yaml:"filesystem"`
+	GCS          map[string]NamedGCSConfig         `yaml:"gcs"`
+	AlibabaCloud map[string]NamedOssConfig         `yaml:"alibabacloud"`
+	Swift        map[string]NamedSwiftConfig       `yaml:"swift"`
+	COS          map[string]NamedCOSConfig         `yaml:"cos"`
 
 	// contains mapping from named store reference name to store type
 	storeType map[string]string `yaml:"-"`
@@ -508,43 +597,47 @@ func NewObjectClient(name string, cfg Config, clientMetrics ClientMetrics) (clie
 	case config.StorageTypeAlibabaCloud:
 		ossCfg := cfg.AlibabaStorageConfig
 		if namedStore != "" {
-			var ok bool
-			ossCfg, ok = cfg.NamedStores.AlibabaCloud[namedStore]
+			nsCfg, ok := cfg.NamedStores.AlibabaCloud[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named alibabacloud oss storage config %s", name)
 			}
+
+			ossCfg = (alibaba.OssConfig)(nsCfg)
 		}
 		return alibaba.NewOssObjectClient(context.Background(), ossCfg)
 	case config.StorageTypeGCS:
 		gcsCfg := cfg.GCSConfig
 		if namedStore != "" {
-			var ok bool
-			gcsCfg, ok = cfg.NamedStores.GCS[namedStore]
+			nsCfg, ok := cfg.NamedStores.GCS[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named gcs storage config %s", name)
 			}
+
+			gcsCfg = (gcp.GCSConfig)(nsCfg)
 		}
 
 		return gcp.NewGCSObjectClient(context.Background(), gcsCfg, cfg.Hedging)
 	case config.StorageTypeAzure:
 		azureCfg := cfg.AzureStorageConfig
 		if namedStore != "" {
-			var ok bool
-			azureCfg, ok = cfg.NamedStores.Azure[namedStore]
+			nsCfg, ok := cfg.NamedStores.Azure[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named azure storage config %s", name)
 			}
+
+			azureCfg = (azure.BlobStorageConfig)(nsCfg)
 		}
 
 		return azure.NewBlobStorage(&azureCfg, clientMetrics.AzureMetrics, cfg.Hedging)
 	case config.StorageTypeSwift:
 		swiftCfg := cfg.Swift
 		if namedStore != "" {
-			var ok bool
-			swiftCfg, ok = cfg.NamedStores.Swift[namedStore]
+			nsCfg, ok := cfg.NamedStores.Swift[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named swift storage config %s", name)
 			}
+
+			swiftCfg = (openstack.SwiftConfig)(nsCfg)
 		}
 
 		return openstack.NewSwiftObjectClient(swiftCfg, cfg.Hedging)
@@ -553,34 +646,37 @@ func NewObjectClient(name string, cfg Config, clientMetrics ClientMetrics) (clie
 	case config.StorageTypeFileSystem:
 		fsCfg := cfg.FSConfig
 		if namedStore != "" {
-			var ok bool
-			fsCfg, ok = cfg.NamedStores.Filesystem[namedStore]
+			nsCfg, ok := cfg.NamedStores.Filesystem[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named filesystem storage config %s", name)
 			}
+
+			fsCfg = (local.FSConfig)(nsCfg)
 		}
 
 		return local.NewFSObjectClient(fsCfg)
 	case config.StorageTypeBOS:
 		bosCfg := cfg.BOSStorageConfig
 		if namedStore != "" {
-			var ok bool
-			bosCfg, ok = cfg.NamedStores.BOS[namedStore]
+			nsCfg, ok := cfg.NamedStores.BOS[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named bos storage config %s", name)
 			}
+
+			bosCfg = (baidubce.BOSStorageConfig)(nsCfg)
 		}
 
 		return baidubce.NewBOSObjectStorage(&bosCfg)
 
 	case config.StorageTypeCOS:
 		cosCfg := cfg.COSConfig
 		if namedStore != "" {
-			var ok bool
-			cosCfg, ok = cfg.NamedStores.COS[namedStore]
+			nsCfg, ok := cfg.NamedStores.COS[namedStore]
 			if !ok {
 				return nil, fmt.Errorf("Unrecognized named cos storage config %s", name)
 			}
+
+			cosCfg = (ibmcloud.COSConfig)(nsCfg)
 		}
 		return ibmcloud.NewCOSObjectClient(cosCfg, cfg.Hedging)
 	default: