diff --git a/cmd/dex/config.go b/cmd/dex/config.go index 3579616504..1d45a7ddc0 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,53 @@ 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"` + InitialDelay time.Duration `json:"initialDelay"` + MaxDelay time.Duration `json:"maxDelay"` + BackoffFactor float64 `json:"backoffFactor"` +} + +func (r *Retry) Validate() error { + // If retry is configured but empty, return an error + if r.MaxAttempts == 0 && r.InitialDelay == 0 && r.MaxDelay == 0 && 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") + } + + // TODO type of durations? + initialDelay, err := time.ParseDuration(r.InitialDelay.String()) + if err != nil || initialDelay <= 0 { + return fmt.Errorf("storage retry initial delay must be a positive duration") + } + + maxDelay, err := time.ParseDuration(r.MaxDelay.String()) + if err != nil || maxDelay <= 0 { + return fmt.Errorf("storage retry max delay must be a positive duration") + } + + 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 +351,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 +374,37 @@ func (s *Storage) UnmarshalJSON(b []byte) error { return fmt.Errorf("parse storage config: %v", err) } } + + // TODO when retry is empty? + maxAttempts := store.Retry.MaxAttempts + if maxAttempts == 0 { + maxAttempts = 5 + } + // TODO maybe only check for empty string? + initialDelay, err := time.ParseDuration(store.Retry.InitialDelay.String()) + if err != nil { + initialDelay = time.Second + } + + maxDelay, err := time.ParseDuration(store.Retry.MaxDelay.String()) + if err != nil { + maxDelay = 5 * time.Minute + } + + backoffFactor := store.Retry.BackoffFactor + if backoffFactor == 0 { + backoffFactor = 2 + } + *s = Storage{ - Type: store.Type, - Config: storageConfig, - RetryAttempts: store.RetryAttempts, - RetryDelay: store.RetryDelay, + Type: store.Type, + Config: storageConfig, + Retry: Retry{ + MaxAttempts: maxAttempts, + InitialDelay: initialDelay, + MaxDelay: maxDelay, + BackoffFactor: backoffFactor, + }, } return nil } diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index 401600cc01..0502a2ff31 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -4,10 +4,10 @@ import ( "log/slog" "os" "testing" + "time" "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 +26,12 @@ func TestValidConfiguration(t *testing.T) { Config: &sql.SQLite3{ File: "examples/dex.db", }, + Retry: Retry{ + MaxAttempts: 10, + InitialDelay: 1 * time.Second, + MaxDelay: 512 * time.Second, + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -55,7 +61,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) } @@ -153,6 +160,12 @@ additionalFeatures: [ ConnectionTimeout: 3, }, }, + Retry: Retry{ + MaxAttempts: 5, + InitialDelay: 0 * time.Second, + MaxDelay: 0 * time.Second, + BackoffFactor: 2, + }, }, Web: Web{ HTTPS: "127.0.0.1:5556", @@ -371,6 +384,12 @@ logger: ConnectionTimeout: 3, }, }, + Retry: Retry{ + MaxAttempts: 5, + InitialDelay: 0 * time.Second, + MaxDelay: 0 * time.Second, + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -449,22 +468,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..de09e29bb0 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 := storageConfig.Retry.InitialDelay + maxDelay := 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..77b73dd0b0 100644 --- a/cmd/dex/serve_test.go +++ b/cmd/dex/serve_test.go @@ -1,15 +1,14 @@ package main import ( - "context" - "errors" + "fmt" "log/slog" "testing" - - "github.com/stretchr/testify/require" + "time" "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/memory" + "github.com/stretchr/testify/require" ) func TestNewLogger(t *testing.T) { @@ -32,11 +31,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 +40,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: time.Second, + MaxDelay: 10 * time.Second, + BackoffFactor: 2, + }, }, Web: Web{ HTTP: "127.0.0.1:5556", @@ -59,7 +58,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 +75,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 }