From a06ee771281b9a6194c1cb8a204e40dcaab0e7ac Mon Sep 17 00:00:00 2001 From: moeneuron Date: Sat, 17 Aug 2024 21:57:46 +0200 Subject: [PATCH 1/4] feat: Add retry mechanism for storage initialization Signed-off-by: moeneuron --- cmd/dex/config.go | 18 ++++++++++++------ cmd/dex/serve.go | 37 ++++++++++++++++++++++++++++++++++++- config.yaml.dist | 5 +++++ examples/config-dev.yaml | 5 +++++ 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/cmd/dex/config.go b/cmd/dex/config.go index dd6d2e2ab9..3579616504 100644 --- a/cmd/dex/config.go +++ b/cmd/dex/config.go @@ -254,8 +254,10 @@ type GRPC struct { // Storage holds app's storage configuration. type Storage struct { - Type string `json:"type"` - Config StorageConfig `json:"config"` + Type string `json:"type"` + Config StorageConfig `json:"config"` + RetryAttempts int `json:"retryAttempts"` + RetryDelay string `json:"retryDelay"` } // StorageConfig is a configuration that can create a storage. @@ -297,8 +299,10 @@ var storages = map[string]func() StorageConfig{ // dynamically determine the type of the storage config. func (s *Storage) UnmarshalJSON(b []byte) error { var store struct { - Type string `json:"type"` - Config json.RawMessage `json:"config"` + Type string `json:"type"` + Config json.RawMessage `json:"config"` + RetryAttempts int `json:"retryAttempts"` + RetryDelay string `json:"retryDelay"` } if err := json.Unmarshal(b, &store); err != nil { return fmt.Errorf("parse storage: %v", err) @@ -320,8 +324,10 @@ func (s *Storage) UnmarshalJSON(b []byte) error { } } *s = Storage{ - Type: store.Type, - Config: storageConfig, + Type: store.Type, + Config: storageConfig, + RetryAttempts: store.RetryAttempts, + RetryDelay: store.RetryDelay, } return nil } diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 6fcca04da3..5e46ed36d2 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -196,11 +196,12 @@ func runServe(options serveOptions) error { grpcOptions = append(grpcOptions, grpc.Creds(credentials.NewTLS(tlsConfig))) } - s, err := c.Storage.Config.Open(logger) + s, err := initializeStorageWithRetry(c.Storage, logger) if err != nil { return fmt.Errorf("failed to initialize storage: %v", err) } defer s.Close() + defer s.Close() logger.Info("config storage", "storage_type", c.Storage.Type) @@ -689,3 +690,37 @@ func loadTLSConfig(certFile, keyFile, caFile string, baseConfig *tls.Config) (*t func recordBuildInfo() { buildInfo.WithLabelValues(version, runtime.Version(), fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH)).Set(1) } + +// initializeStorageWithRetry opens a connection to the storage backend with a retry mechanism. +func initializeStorageWithRetry(storageConfig Storage, logger *slog.Logger) (storage.Storage, error) { + var s storage.Storage + var err error + + retryAttempts := storageConfig.RetryAttempts + if retryAttempts == 0 { + retryAttempts = 5 // Default to 5 attempts + } + + retryDelay, err := time.ParseDuration(storageConfig.RetryDelay) + if err != nil { + retryDelay = 5 * time.Second // Default to 5 seconds + } + + for attempt := 1; attempt <= retryAttempts; attempt++ { + s, err = storageConfig.Config.Open(logger) + if err == nil { + return s, nil + } + + logger.Error("Failed to initialize storage", + "attempt", fmt.Sprintf("%d/%d", attempt, retryAttempts), + "error", err) + + if attempt < retryAttempts { + logger.Info("Retrying storage initialization", + "nextAttemptIn", retryDelay) + time.Sleep(retryDelay) + } + } + return nil, fmt.Errorf("failed to initialize storage after %d attempts: %v", retryAttempts, err) +} diff --git a/config.yaml.dist b/config.yaml.dist index b7e1410ffc..a67cbacef1 100644 --- a/config.yaml.dist +++ b/config.yaml.dist @@ -47,6 +47,11 @@ storage: # config: # kubeConfigFile: $HOME/.kube/config + # Configuration of the retry mechanism upon a failure to storage database + # If not defined, the default will apply 5 attempts with 5s timeout + # retryAttempts: 5 + # retryDelay: "5s" + # HTTP service configuration web: http: 127.0.0.1:5556 diff --git a/examples/config-dev.yaml b/examples/config-dev.yaml index 147597a265..47831c3d13 100644 --- a/examples/config-dev.yaml +++ b/examples/config-dev.yaml @@ -45,6 +45,11 @@ storage: # config: # kubeConfigFile: $HOME/.kube/config + # Configuration of the retry mechanism upon a failure to storage database + # If not defined, the default will apply 5 attempts with 5s timeout + # retryAttempts: 5 + # retryDelay: "5s" + # Configuration for the HTTP endpoints. web: http: 0.0.0.0:5556 From abb1a0a188066e347e8b03ce8026cc42d591c121 Mon Sep 17 00:00:00 2001 From: moeneuron Date: Sun, 18 Aug 2024 16:36:02 +0200 Subject: [PATCH 2/4] test: retry mechanism upon failed storage initialization Signed-off-by: moeneuron --- cmd/dex/config_test.go | 21 +++++++++++++++++ cmd/dex/serve.go | 1 - cmd/dex/serve_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index c6d37cb03e..401600cc01 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -7,6 +7,7 @@ import ( "github.com/ghodss/yaml" "github.com/kylelemons/godebug/pretty" + "github.com/stretchr/testify/require" "github.com/dexidp/dex/connector/mock" "github.com/dexidp/dex/connector/oidc" @@ -447,3 +448,23 @@ logger: t.Errorf("got!=want: %s", diff) } } + +func TestUnmarshalConfigWithRetry(t *testing.T) { + rawConfig := []byte(` +storage: + type: postgres + config: + host: 10.0.0.1 + port: 65432 + retryAttempts: 10 + retryDelay: "1s" +`) + + var c Config + err := yaml.Unmarshal(rawConfig, &c) + require.NoError(t, err) + + require.Equal(t, "postgres", c.Storage.Type) + require.Equal(t, 10, c.Storage.RetryAttempts) + require.Equal(t, "1s", c.Storage.RetryDelay) +} diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 5e46ed36d2..3e0741b3eb 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -201,7 +201,6 @@ func runServe(options serveOptions) error { return fmt.Errorf("failed to initialize storage: %v", err) } defer s.Close() - defer s.Close() logger.Info("config storage", "storage_type", c.Storage.Type) diff --git a/cmd/dex/serve_test.go b/cmd/dex/serve_test.go index 9e214480d3..7e64edaa4f 100644 --- a/cmd/dex/serve_test.go +++ b/cmd/dex/serve_test.go @@ -1,10 +1,15 @@ package main import ( + "context" + "errors" "log/slog" "testing" "github.com/stretchr/testify/require" + + "github.com/dexidp/dex/storage" + "github.com/dexidp/dex/storage/memory" ) func TestNewLogger(t *testing.T) { @@ -27,3 +32,50 @@ func TestNewLogger(t *testing.T) { require.Equal(t, (*slog.Logger)(nil), logger) }) } + +func TestStorageInitializationRetry(t *testing.T) { + _, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Create a mock storage that fails a certain number of times before succeeding + mockStorage := &mockRetryStorage{ + failuresLeft: 3, + } + + config := Config{ + Issuer: "http://127.0.0.1:5556/dex", + Storage: Storage{ + Type: "mock", + Config: mockStorage, + RetryAttempts: 5, + RetryDelay: "1s", + }, + Web: Web{ + HTTP: "127.0.0.1:5556", + }, + Logger: Logger{ + Level: slog.LevelInfo, + Format: "json", + }, + } + + logger, _ := newLogger(config.Logger.Level, config.Logger.Format) + + s, err := initializeStorageWithRetry(config.Storage, logger) + require.NoError(t, err) + require.NotNil(t, s) + + require.Equal(t, 0, mockStorage.failuresLeft) +} + +type mockRetryStorage struct { + failuresLeft int +} + +func (m *mockRetryStorage) Open(logger *slog.Logger) (storage.Storage, error) { + if m.failuresLeft > 0 { + m.failuresLeft-- + return nil, errors.New("mock storage failure") + } + return memory.New(logger), nil +} From 255cce4ea143d93b63aea59646ff3f217fd5345e Mon Sep 17 00:00:00 2001 From: moeneuron Date: Sun, 18 Aug 2024 17:06:57 +0200 Subject: [PATCH 3/4] fix: Format retry delay as string in logs Signed-off-by: moeneuron --- cmd/dex/serve.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 3e0741b3eb..0ee5e40357 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -717,7 +717,7 @@ func initializeStorageWithRetry(storageConfig Storage, logger *slog.Logger) (sto if attempt < retryAttempts { logger.Info("Retrying storage initialization", - "nextAttemptIn", retryDelay) + "nextAttemptIn", retryDelay.String()) time.Sleep(retryDelay) } } From 5baca043386750cc926ed643ec46b744313c0a3a Mon Sep 17 00:00:00 2001 From: moeneuron Date: Tue, 3 Sep 2024 23:26:23 +0200 Subject: [PATCH 4/4] feat: adopt an exponential backoff as a retry mechanism and integrate review findings --- cmd/dex/config.go | 87 ++++++++++++++++++++++++++++++++++------ cmd/dex/config_test.go | 71 ++++++++++++++++++++++---------- cmd/dex/serve.go | 31 +++++++------- cmd/dex/serve_test.go | 27 ++++++------- config.yaml.dist | 9 +++-- examples/config-dev.yaml | 9 +++-- 6 files changed, 167 insertions(+), 67 deletions(-) diff --git a/cmd/dex/config.go b/cmd/dex/config.go index 3579616504..7e4d89d1db 100644 --- a/cmd/dex/config.go +++ b/cmd/dex/config.go @@ -5,10 +5,12 @@ import ( "encoding/json" "fmt" "log/slog" + "math" "net/http" "net/netip" "os" "strings" + "time" "golang.org/x/crypto/bcrypt" @@ -78,6 +80,13 @@ func (c Config) Validate() error { {c.GRPC.TLSMaxVersion != "" && c.GRPC.TLSMinVersion != "" && c.GRPC.TLSMinVersion > c.GRPC.TLSMaxVersion, "TLSMinVersion greater than TLSMaxVersion"}, } + if err := c.Storage.Retry.Validate(); err != nil { + checks = append(checks, struct { + bad bool + errMsg string + }{true, err.Error()}) + } + var checkErrors []string for _, check := range checks { @@ -254,10 +263,52 @@ type GRPC struct { // Storage holds app's storage configuration. type Storage struct { - Type string `json:"type"` - Config StorageConfig `json:"config"` - RetryAttempts int `json:"retryAttempts"` - RetryDelay string `json:"retryDelay"` + Type string `json:"type"` + Config StorageConfig `json:"config"` + Retry Retry `json:"retry"` +} + +// Retry holds retry mechanism configuration. +type Retry struct { + MaxAttempts int `json:"maxAttempts"` // Defaults to 5 + InitialDelay string `json:"initialDelay"` // Defaults to 1s + MaxDelay string `json:"maxDelay"` // Defaults to 5s + BackoffFactor float64 `json:"backoffFactor"` // Defaults to 2 +} + +func (r *Retry) Validate() error { + // If retry is configured but empty, return an error + if r.MaxAttempts == 0 && r.InitialDelay == "" && r.MaxDelay == "" && r.BackoffFactor == 0 { + return fmt.Errorf("empty configuration is supplied for storage retry") + } + + if r.MaxAttempts < 1 { + return fmt.Errorf("storage retry max attempts must be at least 1") + } + + initialDelay, err := time.ParseDuration(r.InitialDelay) + if err != nil || initialDelay <= 0 { + return fmt.Errorf("storage retry initial delay must be a positive duration in go time format") + } + + maxDelay, err := time.ParseDuration(r.MaxDelay) + if err != nil || maxDelay <= 0 { + return fmt.Errorf("storage retry max delay must be a positive duration in go time format") + } + + if maxDelay < initialDelay { + return fmt.Errorf("storage retry max delay must be greater than or equal to initial delay") + } + + if r.BackoffFactor <= 1 { + return fmt.Errorf("storage retry backoff factor must be greater than 1") + } + // exponential backoff algorithm-specific check + if float64(maxDelay) < float64(initialDelay)*math.Pow(r.BackoffFactor, float64(r.MaxAttempts-1)) { + return fmt.Errorf("storage retry max delay is too small for the given initial delay, backoff factor, and max attempts") + } + + return nil } // StorageConfig is a configuration that can create a storage. @@ -299,10 +350,9 @@ var storages = map[string]func() StorageConfig{ // dynamically determine the type of the storage config. func (s *Storage) UnmarshalJSON(b []byte) error { var store struct { - Type string `json:"type"` - Config json.RawMessage `json:"config"` - RetryAttempts int `json:"retryAttempts"` - RetryDelay string `json:"retryDelay"` + Type string `json:"type"` + Config json.RawMessage `json:"config"` + Retry Retry `json:"retry"` } if err := json.Unmarshal(b, &store); err != nil { return fmt.Errorf("parse storage: %v", err) @@ -323,11 +373,16 @@ func (s *Storage) UnmarshalJSON(b []byte) error { return fmt.Errorf("parse storage config: %v", err) } } + *s = Storage{ - Type: store.Type, - Config: storageConfig, - RetryAttempts: store.RetryAttempts, - RetryDelay: store.RetryDelay, + Type: store.Type, + Config: storageConfig, + Retry: Retry{ + MaxAttempts: value(store.Retry.MaxAttempts, 5), + InitialDelay: value(store.Retry.InitialDelay, time.Second.String()), + MaxDelay: value(store.Retry.MaxDelay, (5 * time.Second).String()), + BackoffFactor: value(store.Retry.BackoffFactor, 2), + }, } return nil } @@ -428,3 +483,11 @@ type RefreshToken struct { AbsoluteLifetime string `json:"absoluteLifetime"` ValidIfNotUsedFor string `json:"validIfNotUsedFor"` } + +func value[T comparable](val, defaultValue T) T { + var zero T + if val == zero { + return defaultValue + } + return val +} diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index 401600cc01..af5dc52096 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -7,7 +7,6 @@ import ( "github.com/ghodss/yaml" "github.com/kylelemons/godebug/pretty" - "github.com/stretchr/testify/require" "github.com/dexidp/dex/connector/mock" "github.com/dexidp/dex/connector/oidc" @@ -26,6 +25,12 @@ func TestValidConfiguration(t *testing.T) { Config: &sql.SQLite3{ File: "examples/dex.db", }, + Retry: Retry{ + MaxAttempts: 5, + InitialDelay: "1s", + MaxDelay: "5m", + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -55,7 +60,8 @@ func TestInvalidConfiguration(t *testing.T) { wanted := `invalid Config: - no issuer specified in config file - no storage supplied in config file - - must supply a HTTP/HTTPS address to listen on` + - must supply a HTTP/HTTPS address to listen on + - empty configuration is supplied for storage retry` if got != wanted { t.Fatalf("Expected error message to be %q, got %q", wanted, got) } @@ -73,6 +79,11 @@ storage: maxIdleConns: 3 connMaxLifetime: 30 connectionTimeout: 3 + retry: + maxAttempts: 10 + initialDelay: "4s" + maxDelay: "5m" + backoffFactor: 2 web: https: 127.0.0.1:5556 tlsMinVersion: 1.3 @@ -153,6 +164,12 @@ additionalFeatures: [ ConnectionTimeout: 3, }, }, + Retry: Retry{ + MaxAttempts: 10, + InitialDelay: "4s", + MaxDelay: "5m", + BackoffFactor: 2, + }, }, Web: Web{ HTTPS: "127.0.0.1:5556", @@ -294,6 +311,11 @@ storage: maxIdleConns: 3 connMaxLifetime: 30 connectionTimeout: 3 + retry: + maxAttempts: 5 + initialDelay: 2s + maxDelay: 10s + backoffFactor: 2 web: http: 127.0.0.1:5556 @@ -371,6 +393,12 @@ logger: ConnectionTimeout: 3, }, }, + Retry: Retry{ + MaxAttempts: 5, + InitialDelay: "2s", + MaxDelay: "10s", + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -449,22 +477,23 @@ logger: } } -func TestUnmarshalConfigWithRetry(t *testing.T) { - rawConfig := []byte(` -storage: - type: postgres - config: - host: 10.0.0.1 - port: 65432 - retryAttempts: 10 - retryDelay: "1s" -`) - - var c Config - err := yaml.Unmarshal(rawConfig, &c) - require.NoError(t, err) - - require.Equal(t, "postgres", c.Storage.Type) - require.Equal(t, 10, c.Storage.RetryAttempts) - require.Equal(t, "1s", c.Storage.RetryDelay) -} +// func TestUnmarshalConfigWithRetry(t *testing.T) { +// rawConfig := []byte(` +// storage: +// type: postgres +// config: +// host: 10.0.0.1 +// port: 65432 +// retry: +// attempts: 10 +// delay: 1s +// `) + +// var c Config +// err := yaml.Unmarshal(rawConfig, &c) +// require.NoError(t, err) + +// require.Equal(t, "postgres", c.Storage.Type) +// require.Equal(t, 10, c.Storage.Retry.Attempts) +// require.Equal(t, "1s", c.Storage.Retry.Delay) +// } diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 0ee5e40357..88910cd9b6 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -695,31 +695,34 @@ func initializeStorageWithRetry(storageConfig Storage, logger *slog.Logger) (sto var s storage.Storage var err error - retryAttempts := storageConfig.RetryAttempts - if retryAttempts == 0 { - retryAttempts = 5 // Default to 5 attempts - } + maxAttempts := storageConfig.Retry.MaxAttempts + initialDelay, _ := time.ParseDuration(storageConfig.Retry.InitialDelay) + maxDelay, _ := time.ParseDuration(storageConfig.Retry.MaxDelay) + backoffFactor := storageConfig.Retry.BackoffFactor - retryDelay, err := time.ParseDuration(storageConfig.RetryDelay) - if err != nil { - retryDelay = 5 * time.Second // Default to 5 seconds - } + delay := initialDelay - for attempt := 1; attempt <= retryAttempts; attempt++ { + for attempt := 1; attempt <= maxAttempts; attempt++ { s, err = storageConfig.Config.Open(logger) if err == nil { return s, nil } logger.Error("Failed to initialize storage", - "attempt", fmt.Sprintf("%d/%d", attempt, retryAttempts), + "attempt", fmt.Sprintf("%d/%d", attempt, maxAttempts), "error", err) - if attempt < retryAttempts { + if attempt < maxAttempts { logger.Info("Retrying storage initialization", - "nextAttemptIn", retryDelay.String()) - time.Sleep(retryDelay) + "nextAttemptIn", delay.String()) + time.Sleep(delay) + + // Calculate next delay using exponential backoff + delay = time.Duration(float64(delay) * backoffFactor) + if delay > maxDelay { + delay = maxDelay + } } } - return nil, fmt.Errorf("failed to initialize storage after %d attempts: %v", retryAttempts, err) + return nil, fmt.Errorf("failed to initialize storage after %d attempts: %v", maxAttempts, err) } diff --git a/cmd/dex/serve_test.go b/cmd/dex/serve_test.go index 7e64edaa4f..1942abda4b 100644 --- a/cmd/dex/serve_test.go +++ b/cmd/dex/serve_test.go @@ -1,15 +1,13 @@ package main import ( - "context" - "errors" + "fmt" "log/slog" "testing" - "github.com/stretchr/testify/require" - "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/memory" + "github.com/stretchr/testify/require" ) func TestNewLogger(t *testing.T) { @@ -32,11 +30,7 @@ func TestNewLogger(t *testing.T) { require.Equal(t, (*slog.Logger)(nil), logger) }) } - func TestStorageInitializationRetry(t *testing.T) { - _, cancel := context.WithCancel(context.Background()) - defer cancel() - // Create a mock storage that fails a certain number of times before succeeding mockStorage := &mockRetryStorage{ failuresLeft: 3, @@ -45,10 +39,14 @@ func TestStorageInitializationRetry(t *testing.T) { config := Config{ Issuer: "http://127.0.0.1:5556/dex", Storage: Storage{ - Type: "mock", - Config: mockStorage, - RetryAttempts: 5, - RetryDelay: "1s", + Type: "mock", + Config: mockStorage, + Retry: Retry{ + MaxAttempts: 5, + InitialDelay: "1s", + MaxDelay: "10s", + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -59,7 +57,8 @@ func TestStorageInitializationRetry(t *testing.T) { }, } - logger, _ := newLogger(config.Logger.Level, config.Logger.Format) + logger, err := newLogger(config.Logger.Level, config.Logger.Format) + require.NoError(t, err) s, err := initializeStorageWithRetry(config.Storage, logger) require.NoError(t, err) @@ -75,7 +74,7 @@ type mockRetryStorage struct { func (m *mockRetryStorage) Open(logger *slog.Logger) (storage.Storage, error) { if m.failuresLeft > 0 { m.failuresLeft-- - return nil, errors.New("mock storage failure") + return nil, fmt.Errorf("mock storage failure") } return memory.New(logger), nil } diff --git a/config.yaml.dist b/config.yaml.dist index a67cbacef1..5712afbd92 100644 --- a/config.yaml.dist +++ b/config.yaml.dist @@ -48,9 +48,12 @@ storage: # kubeConfigFile: $HOME/.kube/config # Configuration of the retry mechanism upon a failure to storage database - # If not defined, the default will apply 5 attempts with 5s timeout - # retryAttempts: 5 - # retryDelay: "5s" + # If not defined, the defaults below are applied + # retry: + # maxAttempts: 5 + # initialDelay: "1s" + # maxDelay: "5s" + # backoffFactor: 2 # HTTP service configuration web: diff --git a/examples/config-dev.yaml b/examples/config-dev.yaml index 47831c3d13..1d0d542d54 100644 --- a/examples/config-dev.yaml +++ b/examples/config-dev.yaml @@ -46,9 +46,12 @@ storage: # kubeConfigFile: $HOME/.kube/config # Configuration of the retry mechanism upon a failure to storage database - # If not defined, the default will apply 5 attempts with 5s timeout - # retryAttempts: 5 - # retryDelay: "5s" + # If not defined, the defaults below are applied + # retry: + # maxAttempts: 5 + # initialDelay: "1s" + # maxDelay: "5s" + # backoffFactor: 2 # Configuration for the HTTP endpoints. web: