Setting kv driver defaults as configuration (#4553)

nopcoder · web-flow · commit 9a7577881ea7 · 2022-11-08T15:45:11.000Z
diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go
@@ -7,14 +7,35 @@ import (
 )
 
 const (
-	DatabaseTypeKey     = "database.type"
-	DefaultDatabaseType = "local"
+	DatabaseTypeKey   = "database.type"
+	LocalDatabaseType = "local"
 
-	DatabaseKVLocalPath        = "database.local.path"
-	DefaultDatabaseLocalKVPath = "~/lakefs/metadata"
+	DatabaseLocalPathKey     = "database.local.path"
+	DefaultDatabaseLocalPath = "~/lakefs/metadata"
 
-	BlockstoreTypeKey     = "blockstore.type"
-	DefaultBlockstoreType = "local"
+	DatabaseLocalPrefetchSizeKey     = "database.local.prefetch_size"
+	DefaultDatabaseLocalPrefetchSize = 256
+
+	DatabaseDynamodbTableNameKey     = "database.dynamodb.table_name"
+	DefaultDatabaseDynamodbTableName = "kvstore"
+
+	DatabaseDynamodbReadCapacityUnitsKey     = "database.dynamodb.read_capacity_units"
+	DefaultDatabaseDynamodbReadCapacityUnits = 1000
+
+	DatabaseDynamodbWriteCapacityUnitsKey     = "database.dynamodb.write_capacity_units"
+	DefaultDatabaseDynamodbWriteCapacityUnits = 1000
+
+	DatabasePostgresMaxOpenConnectionsKey     = "database.postgres.max_open_connections"
+	DefaultDatabasePostgresMaxOpenConnections = 25
+
+	DatabasePostgresMaxIdleConnectionsKey     = "database.postgres.max_idle_connections"
+	DefaultDatabasePostgresMaxIdleConnections = 25
+
+	PostgresConnectionMaxLifetimeKey     = "database.postgres.connection_max_lifetime"
+	DefaultPostgresConnectionMaxLifetime = "5m"
+
+	BlockstoreTypeKey   = "blockstore.type"
+	LocalBlockstoreType = "local"
 
 	BlockstoreLocalPathKey     = "blockstore.local.path"
 	DefaultBlockstoreLocalPath = "~/lakefs/data/block"
@@ -88,8 +109,8 @@ const (
 	LoggingFilesKeepKey     = "logging.files_keep"
 	LoggingAuditLogLevel    = "logging.audit_log_level"
 
-	AuthEncryptSecretKey        = "auth.encrypt.secret_key"            // #nosec
-	DefaultAuthEncryptSecretKey = "THIS_MUST_BE_CHANGED_IN_PRODUCTION" // #nosec
+	AuthEncryptSecretKey      = "auth.encrypt.secret_key"            // #nosec
+	LocalAuthEncryptSecretKey = "THIS_MUST_BE_CHANGED_IN_PRODUCTION" // #nosec
 
 	ActionsEnabledKey = "actions.enabled"
 
@@ -134,17 +155,11 @@ const (
 	UIEnabledKey = "ui.enabled"
 )
 
-func setDefaultLocalConfig() {
-	viper.SetDefault(DatabaseTypeKey, DefaultDatabaseType)
-	viper.SetDefault(DatabaseKVLocalPath, DefaultDatabaseLocalKVPath)
-	viper.SetDefault(BlockstoreLocalPathKey, DefaultBlockstoreLocalPath)
-	viper.SetDefault(AuthEncryptSecretKey, DefaultAuthEncryptSecretKey)
-	viper.SetDefault(BlockstoreTypeKey, DefaultBlockstoreType)
-}
-
 func setDefaults(local bool) {
 	if local {
-		setDefaultLocalConfig()
+		viper.SetDefault(DatabaseTypeKey, LocalDatabaseType)
+		viper.SetDefault(AuthEncryptSecretKey, LocalAuthEncryptSecretKey)
+		viper.SetDefault(BlockstoreTypeKey, LocalBlockstoreType)
 	}
 
 	viper.SetDefault(ListenAddressKey, DefaultListenAddr)
@@ -166,7 +181,6 @@ func setDefaults(local bool) {
 	viper.SetDefault(AuthLogoutRedirectURL, DefaultAuthLogoutRedirectURL)
 
 	viper.SetDefault(BlockstoreLocalPathKey, DefaultBlockstoreLocalPath)
-	viper.SetDefault(BlockstoreTypeKey, DefaultBlockstoreType)
 	viper.SetDefault(BlockstoreS3RegionKey, DefaultBlockstoreS3Region)
 	viper.SetDefault(BlockstoreS3StreamingChunkSizeKey, DefaultBlockstoreS3StreamingChunkSize)
 	viper.SetDefault(BlockstoreS3StreamingChunkTimeoutKey, DefaultBlockstoreS3StreamingChunkTimeout)
@@ -206,4 +220,18 @@ func setDefaults(local bool) {
 	viper.SetDefault(LakefsEmailBaseURLKey, DefaultLakefsEmailBaseURL)
 
 	viper.SetDefault(UIEnabledKey, DefaultUIEnabled)
+
+	viper.SetDefault(BlockstoreLocalPathKey, DefaultBlockstoreLocalPath)
+
+	viper.SetDefault(DatabaseLocalPathKey, DefaultDatabaseLocalPath)
+	viper.SetDefault(DatabaseLocalPrefetchSizeKey, DefaultDatabaseLocalPrefetchSize)
+
+	viper.SetDefault(DatabaseDynamodbTableNameKey, DefaultDatabaseDynamodbTableName)
+
+	viper.SetDefault(DatabaseDynamodbReadCapacityUnitsKey, DefaultDatabaseDynamodbReadCapacityUnits)
+	viper.SetDefault(DatabaseDynamodbWriteCapacityUnitsKey, DefaultDatabaseDynamodbWriteCapacityUnits)
+
+	viper.SetDefault(DatabasePostgresMaxOpenConnectionsKey, DefaultDatabasePostgresMaxOpenConnections)
+	viper.SetDefault(DatabasePostgresMaxIdleConnectionsKey, DefaultDatabasePostgresMaxIdleConnections)
+	viper.SetDefault(PostgresConnectionMaxLifetimeKey, DefaultPostgresConnectionMaxLifetime)
 }
diff --git a/pkg/config/template.go b/pkg/config/template.go
@@ -77,7 +77,7 @@ type configuration struct {
 		// DropTables Development flag to delete tables after successful migration to KV
 		DropTables bool `mapstructure:"drop_tables"`
 		// Type Name of the KV Store driver DB implementation which is available according to the kv package Drivers function
-		Type string `mapstructure:"type"`
+		Type string `mapstructure:"type" validate:"required"`
 
 		Local *struct {
 			// Path - Local directory path to store the DB files
diff --git a/pkg/config/testdata/aws_credentials.yaml b/pkg/config/testdata/aws_credentials.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 auth:
   encrypt:
     secret_key: "required in config"
diff --git a/pkg/config/testdata/aws_credentials_with_alias.yaml b/pkg/config/testdata/aws_credentials_with_alias.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 auth:
   encrypt:
     secret_key: "required in config"
diff --git a/pkg/config/testdata/domain_name_prefix.yaml b/pkg/config/testdata/domain_name_prefix.yaml
@@ -1,4 +1,10 @@
 ---
+database:
+  type: local
+
+blockstore:
+  type: local
+
 gateways:
   s3:
     domain_name:
diff --git a/pkg/config/testdata/valid_config.yaml b/pkg/config/testdata/valid_config.yaml
@@ -5,6 +5,7 @@ logging:
   output: "-"
 
 database:
+  type: postgres
   postgres:
     connection_string: test:///dev/null
     max_open_connections: 12
diff --git a/pkg/config/testdata/valid_custom_badger_config.yaml b/pkg/config/testdata/valid_custom_badger_config.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 logging:
   format: text
   level: NONE
diff --git a/pkg/config/testdata/valid_gs_adapter_config.yaml b/pkg/config/testdata/valid_gs_adapter_config.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 logging:
   format: text
   level: NONE
diff --git a/pkg/config/testdata/valid_json_logger_config.yaml b/pkg/config/testdata/valid_json_logger_config.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 logging:
   format: json
   level: DEBUG
diff --git a/pkg/config/testdata/valid_oidc_config.yaml b/pkg/config/testdata/valid_oidc_config.yaml
@@ -5,6 +5,7 @@ logging:
   output: "-"
 
 database:
+  type: postgres
   postgres:
     connection_string: test:///dev/null
     max_open_connections: 12
diff --git a/pkg/config/testdata/valid_s3_adapter_config.yaml b/pkg/config/testdata/valid_s3_adapter_config.yaml
@@ -1,4 +1,7 @@
 ---
+database:
+  type: local
+
 logging:
   format: text
   level: NONE
diff --git a/pkg/kv/dynamodb/store.go b/pkg/kv/dynamodb/store.go
@@ -47,32 +47,13 @@ const (
 	PartitionKey = "PartitionKey"
 	ItemKey      = "ItemKey"
 	ItemValue    = "ItemValue"
-
-	DefaultDynamoDBTableName = "kvstore"
-	// TODO (niro): Which values to use for DynamoDB tables?
-	DefaultDynamoDBReadCapacityUnits  = 1000
-	DefaultDynamoDBWriteCapacityUnits = 1000
 )
 
 //nolint:gochecknoinits
 func init() {
 	kv.Register(DriverName, &Driver{})
 }
 
-func normalizeDBParams(p *kvparams.DynamoDB) {
-	if len(p.TableName) == 0 {
-		p.TableName = DefaultDynamoDBTableName
-	}
-
-	if p.ReadCapacityUnits == 0 {
-		p.ReadCapacityUnits = DefaultDynamoDBReadCapacityUnits
-	}
-
-	if p.WriteCapacityUnits == 0 {
-		p.WriteCapacityUnits = DefaultDynamoDBWriteCapacityUnits
-	}
-}
-
 // Open - opens and returns a KV store over DynamoDB. This function creates the DB session
 // and sets up the KV table.
 func (d *Driver) Open(ctx context.Context, kvParams kvparams.KV) (kv.Store, error) {
@@ -81,7 +62,6 @@ func (d *Driver) Open(ctx context.Context, kvParams kvparams.KV) (kv.Store, erro
 		return nil, fmt.Errorf("missing %s settings: %w", DriverName, kv.ErrDriverConfiguration)
 	}
 
-	normalizeDBParams(params)
 	sess, err := session.NewSessionWithOptions(session.Options{
 		SharedConfigState: session.SharedConfigEnable,
 		Profile:           params.AwsProfile,
diff --git a/pkg/kv/local/driver.go b/pkg/kv/local/driver.go
@@ -12,9 +12,7 @@ import (
 )
 
 const (
-	DriverName           = "local"
-	DefaultDirectoryPath = "~/lakefs/metadata"
-	DefaultPrefetchSize  = 256
+	DriverName = "local"
 )
 
 var (
@@ -24,21 +22,11 @@ var (
 
 type Driver struct{}
 
-func normalizeDBParams(p *kvparams.Local) {
-	if len(p.Path) == 0 {
-		p.Path = DefaultDirectoryPath
-	}
-	if p.PrefetchSize <= 0 {
-		p.PrefetchSize = DefaultPrefetchSize
-	}
-}
-
 func (d *Driver) Open(ctx context.Context, kvParams kvparams.KV) (kv.Store, error) {
 	params := kvParams.Local
 	if params == nil {
 		return nil, fmt.Errorf("missing %s settings: %w", DriverName, kv.ErrDriverConfiguration)
 	}
-	normalizeDBParams(params)
 
 	driverLock.Lock()
 	defer driverLock.Unlock()
diff --git a/pkg/kv/params/database.go b/pkg/kv/params/database.go
@@ -27,7 +27,7 @@ type Postgres struct {
 	MaxOpenConnections    int32
 	MaxIdleConnections    int32
 	ConnectionMaxLifetime time.Duration
-	ScanPageSize          int32
+	ScanPageSize          int
 	Metrics               bool
 }
 
diff --git a/pkg/kv/postgres/store.go b/pkg/kv/postgres/store.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
-	"time"
 
 	"github.com/IBM/pgxpoolprometheus"
 	"github.com/georgysavva/scany/pgxscan"
@@ -42,37 +41,15 @@ const (
 
 	// DefaultPartitions Changing the below value means repartitioning and probably a migration.
 	// Change it only if you really know what you're doing.
-	DefaultPartitions = 100
-
-	DefaultMaxOpenConnections    = 25
-	DefaultMaxIdleConnections    = 25
-	DefaultConnectionMaxLifetime = 5 * time.Minute
-	DefaultScanPageSize          = 1000
+	DefaultPartitions   = 100
+	DefaultScanPageSize = 1000
 )
 
 //nolint:gochecknoinits
 func init() {
 	kv.Register(DriverName, &Driver{})
 }
 
-func normalizeDBParams(p *kvparams.Postgres) {
-	if p.MaxOpenConnections == 0 {
-		p.MaxOpenConnections = DefaultMaxOpenConnections
-	}
-
-	if p.MaxIdleConnections == 0 {
-		p.MaxIdleConnections = DefaultMaxIdleConnections
-	}
-
-	if p.ConnectionMaxLifetime == 0 {
-		p.ConnectionMaxLifetime = DefaultConnectionMaxLifetime
-	}
-
-	if p.ScanPageSize == 0 {
-		p.ScanPageSize = DefaultScanPageSize
-	}
-}
-
 func (d *Driver) Open(ctx context.Context, kvParams kvparams.KV) (kv.Store, error) {
 	if kvParams.Postgres == nil {
 		return nil, fmt.Errorf("missing %s settings: %w", DriverName, kv.ErrDriverConfiguration)
@@ -131,14 +108,19 @@ func (d *Driver) Open(ctx context.Context, kvParams kvparams.KV) (kv.Store, erro
 }
 
 func newPgxpoolConfig(kvParams kvparams.KV) (*pgxpool.Config, error) {
-	normalizeDBParams(kvParams.Postgres)
 	config, err := pgxpool.ParseConfig(kvParams.Postgres.ConnectionString)
 	if err != nil {
 		return nil, fmt.Errorf("%w: %s", kv.ErrDriverConfiguration, err)
 	}
-	config.MaxConns = kvParams.Postgres.MaxOpenConnections
-	config.MinConns = kvParams.Postgres.MaxIdleConnections
-	config.MaxConnLifetime = kvParams.Postgres.ConnectionMaxLifetime
+	if kvParams.Postgres.MaxOpenConnections > 0 {
+		config.MaxConns = kvParams.Postgres.MaxOpenConnections
+	}
+	if kvParams.Postgres.MaxIdleConnections > 0 {
+		config.MinConns = kvParams.Postgres.MaxIdleConnections
+	}
+	if kvParams.Postgres.ConnectionMaxLifetime > 0 {
+		config.MaxConnLifetime = kvParams.Postgres.ConnectionMaxLifetime
+	}
 	return config, err
 }
 
@@ -162,7 +144,9 @@ func parseStoreConfig(runtimeParams map[string]string, pgParams *kvparams.Postgr
 	}
 
 	p.SanitizedTableName = pgx.Identifier{p.TableName}.Sanitize()
-	p.ScanPageSize = int(pgParams.ScanPageSize)
+	if pgParams.ScanPageSize > 0 {
+		p.ScanPageSize = pgParams.ScanPageSize
+	}
 	return p
 }
 
