Browse Source

feat: Support retry settings for Doppler provider (#5608)

* create a retry client and use it

Signed-off-by: Praboda Ariyasinghe <prabodamadushan@gmail.com>
Co-authored-by: Zois Pagoulatos <zpagoulatos@hotmail.com>

* fix lint errors and a few todos after rebasing

Signed-off-by: Praboda Ariyasinghe <prabodamadushan@gmail.com>
Co-authored-by: Zois Pagoulatos <zpagoulatos@hotmail.com>

* sonar cloud issue fix

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

---------

Signed-off-by: Praboda Ariyasinghe <prabodamadushan@gmail.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Co-authored-by: Zois Pagoulatos <zpagoulatos@hotmail.com>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Praboda Ariyasinghe 4 months ago
parent
commit
ccaa461ab1

+ 1 - 1
docs/snippets/full-secret-store.yaml

@@ -17,7 +17,7 @@ spec:
   # You can specify retry settings for the http connection
   # these fields allow you to set a maxRetries before failure, and
   # an interval between the retries.
-  # Current supported providers: AWS, Hashicorp Vault, IBM
+  # Current supported providers: AWS, Hashicorp Vault, IBM, Doppler
   retrySettings:
     maxRetries: 5
     retryInterval: "10s"

+ 1 - 1
providers/v1/doppler/go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/external-secrets/external-secrets/runtime v0.0.0
 	github.com/google/go-cmp v0.7.0
 	k8s.io/api v0.34.1
+	k8s.io/client-go v0.34.1
 	sigs.k8s.io/controller-runtime v0.22.3
 )
 
@@ -82,7 +83,6 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	k8s.io/apiextensions-apiserver v0.34.1 // indirect
 	k8s.io/apimachinery v0.34.1 // indirect
-	k8s.io/client-go v0.34.1 // indirect
 	k8s.io/klog/v2 v2.130.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 // indirect
 	k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 // indirect

+ 32 - 2
providers/v1/doppler/provider.go

@@ -22,13 +22,14 @@ import (
 	"fmt"
 	"os"
 	"strconv"
+	"time"
 
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
-	"github.com/external-secrets/external-secrets/runtime/esutils"
 	dclient "github.com/external-secrets/external-secrets/providers/v1/doppler/client"
+	"github.com/external-secrets/external-secrets/runtime/esutils"
 )
 
 const (
@@ -93,7 +94,13 @@ func (p *Provider) NewClient(ctx context.Context, store esv1.GenericStore, kube
 		}
 	}
 
-	client.doppler = doppler
+	// Wrap the Doppler client with retry logic if retrySettings are configured
+	wrappedClient, err := p.setRetrySettings(doppler, storeSpec)
+	if err != nil {
+		return nil, err
+	}
+
+	client.doppler = wrappedClient
 	client.project = client.store.Project
 	client.config = client.store.Config
 	client.nameTransformer = client.store.NameTransformer
@@ -118,6 +125,29 @@ func (p *Provider) ValidateStore(store esv1.GenericStore) (admission.Warnings, e
 	return nil, nil
 }
 
+func (p *Provider) setRetrySettings(doppler *dclient.DopplerClient, storeSpec *esv1.SecretStoreSpec) (SecretsClientInterface, error) {
+	if storeSpec.RetrySettings == nil {
+		return doppler, nil
+	}
+
+	maxRetries := 3 // default value
+	retryInterval := time.Duration(0)
+
+	if storeSpec.RetrySettings.MaxRetries != nil {
+		maxRetries = int(*storeSpec.RetrySettings.MaxRetries)
+	}
+
+	if storeSpec.RetrySettings.RetryInterval != nil {
+		var err error
+		retryInterval, err = time.ParseDuration(*storeSpec.RetrySettings.RetryInterval)
+		if err != nil {
+			return nil, fmt.Errorf(errNewClient, fmt.Errorf("invalid retry interval: %w", err))
+		}
+	}
+
+	return newRetryableClient(doppler, maxRetries, retryInterval), nil
+}
+
 // NewProvider creates a new Provider instance.
 func NewProvider() esv1.Provider {
 	return &Provider{}

+ 111 - 0
providers/v1/doppler/retry_client.go

@@ -0,0 +1,111 @@
+/*
+Copyright © 2025 ESO Maintainer Team
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package doppler
+
+import (
+	"net/url"
+	"time"
+
+	"k8s.io/client-go/util/retry"
+
+	dclient "github.com/external-secrets/external-secrets/providers/v1/doppler/client"
+)
+
+// retryableClient wraps a Doppler client with retry logic.
+type retryableClient struct {
+	client     SecretsClientInterface
+	maxRetries int
+	retryDelay time.Duration
+}
+
+// newRetryableClient creates a new retryable Doppler client wrapper.
+func newRetryableClient(client SecretsClientInterface, maxRetries int, retryInterval time.Duration) *retryableClient {
+	return &retryableClient{
+		client:     client,
+		maxRetries: maxRetries,
+		retryDelay: retryInterval,
+	}
+}
+
+// BaseURL returns the base URL of the wrapped client.
+func (r *retryableClient) BaseURL() *url.URL {
+	return r.client.BaseURL()
+}
+
+// Authenticate authenticates with retry logic.
+func (r *retryableClient) Authenticate() error {
+	backoff := retry.DefaultBackoff
+	if r.retryDelay > 0 {
+		backoff.Duration = r.retryDelay
+	}
+	if r.maxRetries > 0 {
+		backoff.Steps = r.maxRetries
+	}
+	return retry.OnError(backoff, func(error) bool { return true }, func() error {
+		return r.client.Authenticate()
+	})
+}
+
+// GetSecret retrieves a secret with retry logic.
+func (r *retryableClient) GetSecret(request dclient.SecretRequest) (*dclient.SecretResponse, error) {
+	var result *dclient.SecretResponse
+	backoff := retry.DefaultBackoff
+	if r.retryDelay > 0 {
+		backoff.Duration = r.retryDelay
+	}
+	if r.maxRetries > 0 {
+		backoff.Steps = r.maxRetries
+	}
+	err := retry.OnError(backoff, func(error) bool { return true }, func() error {
+		var err error
+		result, err = r.client.GetSecret(request)
+		return err
+	})
+	return result, err
+}
+
+// GetSecrets retrieves secrets with retry logic.
+func (r *retryableClient) GetSecrets(request dclient.SecretsRequest) (*dclient.SecretsResponse, error) {
+	var result *dclient.SecretsResponse
+	backoff := retry.DefaultBackoff
+	if r.retryDelay > 0 {
+		backoff.Duration = r.retryDelay
+	}
+	if r.maxRetries > 0 {
+		backoff.Steps = r.maxRetries
+	}
+	err := retry.OnError(backoff, func(error) bool { return true }, func() error {
+		var err error
+		result, err = r.client.GetSecrets(request)
+		return err
+	})
+	return result, err
+}
+
+// UpdateSecrets updates secrets with retry logic.
+func (r *retryableClient) UpdateSecrets(request dclient.UpdateSecretsRequest) error {
+	backoff := retry.DefaultBackoff
+	if r.retryDelay > 0 {
+		backoff.Duration = r.retryDelay
+	}
+	if r.maxRetries > 0 {
+		backoff.Steps = r.maxRetries
+	}
+	return retry.OnError(backoff, func(error) bool { return true }, func() error {
+		return r.client.UpdateSecrets(request)
+	})
+}

+ 258 - 0
providers/v1/doppler/retry_client_test.go

@@ -0,0 +1,258 @@
+/*
+Copyright © 2025 ESO Maintainer Team
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package doppler
+
+import (
+	"errors"
+	"net/url"
+	"testing"
+	"time"
+
+	"github.com/external-secrets/external-secrets/providers/v1/doppler/client"
+)
+
+const testSecretValue = "value"
+
+// mockClient implements SecretsClientInterface for testing retry logic.
+type mockClient struct {
+	authenticateCalls  int
+	getSecretCalls     int
+	getSecretsCalls    int
+	updateSecretsCalls int
+	failUntilCall      int
+	returnError        error
+	secretResponse     *client.SecretResponse
+	secretsResponse    *client.SecretsResponse
+}
+
+func (m *mockClient) BaseURL() *url.URL {
+	return &url.URL{Scheme: "https", Host: "api.doppler.com"}
+}
+
+func (m *mockClient) Authenticate() error {
+	m.authenticateCalls++
+	if m.authenticateCalls < m.failUntilCall {
+		return m.returnError
+	}
+	return nil
+}
+
+func (m *mockClient) GetSecret(_ client.SecretRequest) (*client.SecretResponse, error) {
+	m.getSecretCalls++
+	if m.getSecretCalls < m.failUntilCall {
+		return nil, m.returnError
+	}
+	return m.secretResponse, nil
+}
+
+func (m *mockClient) GetSecrets(_ client.SecretsRequest) (*client.SecretsResponse, error) {
+	m.getSecretsCalls++
+	if m.getSecretsCalls < m.failUntilCall {
+		return nil, m.returnError
+	}
+	return m.secretsResponse, nil
+}
+
+func (m *mockClient) UpdateSecrets(_ client.UpdateSecretsRequest) error {
+	m.updateSecretsCalls++
+	if m.updateSecretsCalls < m.failUntilCall {
+		return m.returnError
+	}
+	return nil
+}
+
+func TestRetryClientSuccessOnFirstAttempt(t *testing.T) {
+	mock := &mockClient{
+		failUntilCall:   1, // succeed on first call
+		secretResponse:  &client.SecretResponse{Name: "test", Value: testSecretValue},
+		secretsResponse: &client.SecretsResponse{Secrets: client.Secrets{"test": testSecretValue}},
+	}
+
+	retryClient := newRetryableClient(mock, 3, 10*time.Millisecond)
+
+	// Test Authenticate
+	if err := retryClient.Authenticate(); err != nil {
+		t.Errorf("Authenticate should succeed on first attempt, got error: %v", err)
+	}
+	if mock.authenticateCalls != 1 {
+		t.Errorf("Expected 1 authenticate call, got %d", mock.authenticateCalls)
+	}
+
+	// Test GetSecret
+	resp, err := retryClient.GetSecret(client.SecretRequest{Name: "test"})
+	if err != nil {
+		t.Errorf("GetSecret should succeed on first attempt, got error: %v", err)
+	}
+	if resp == nil || resp.Value != testSecretValue {
+		t.Errorf("GetSecret returned unexpected response: %v", resp)
+	}
+	if mock.getSecretCalls != 1 {
+		t.Errorf("Expected 1 getSecret call, got %d", mock.getSecretCalls)
+	}
+
+	// Test GetSecrets
+	respSecrets, err := retryClient.GetSecrets(client.SecretsRequest{})
+	if err != nil {
+		t.Errorf("GetSecrets should succeed on first attempt, got error: %v", err)
+	}
+	if respSecrets == nil || respSecrets.Secrets["test"] != testSecretValue {
+		t.Errorf("GetSecrets returned unexpected response: %v", respSecrets)
+	}
+	if mock.getSecretsCalls != 1 {
+		t.Errorf("Expected 1 getSecrets call, got %d", mock.getSecretsCalls)
+	}
+
+	// Test UpdateSecrets
+	if err := retryClient.UpdateSecrets(client.UpdateSecretsRequest{}); err != nil {
+		t.Errorf("UpdateSecrets should succeed on first attempt, got error: %v", err)
+	}
+	if mock.updateSecretsCalls != 1 {
+		t.Errorf("Expected 1 updateSecrets call, got %d", mock.updateSecretsCalls)
+	}
+}
+
+func TestRetryClientSuccessAfterRetries(t *testing.T) {
+	testError := errors.New("temporary error")
+	mock := &mockClient{
+		failUntilCall:   3, // fail twice, succeed on third attempt
+		returnError:     testError,
+		secretResponse:  &client.SecretResponse{Name: "test", Value: testSecretValue},
+		secretsResponse: &client.SecretsResponse{Secrets: client.Secrets{"test": testSecretValue}},
+	}
+
+	retryClient := newRetryableClient(mock, 5, 1*time.Millisecond)
+
+	// Test Authenticate - should retry and eventually succeed
+	if err := retryClient.Authenticate(); err != nil {
+		t.Errorf("Authenticate should succeed after retries, got error: %v", err)
+	}
+	if mock.authenticateCalls != 3 {
+		t.Errorf("Expected 3 authenticate calls (2 failures + 1 success), got %d", mock.authenticateCalls)
+	}
+
+	// Reset for GetSecret test
+	mock.getSecretCalls = 0
+	resp, err := retryClient.GetSecret(client.SecretRequest{Name: "test"})
+	if err != nil {
+		t.Errorf("GetSecret should succeed after retries, got error: %v", err)
+	}
+	if resp == nil || resp.Value != testSecretValue {
+		t.Errorf("GetSecret returned unexpected response: %v", resp)
+	}
+	if mock.getSecretCalls != 3 {
+		t.Errorf("Expected 3 getSecret calls (2 failures + 1 success), got %d", mock.getSecretCalls)
+	}
+}
+
+func TestRetryClientFailureAfterMaxRetries(t *testing.T) {
+	testError := errors.New("persistent error")
+	mock := &mockClient{
+		failUntilCall: 100, // always fail
+		returnError:   testError,
+	}
+
+	retryClient := newRetryableClient(mock, 3, 1*time.Millisecond)
+
+	// Test Authenticate - should fail after max retries
+	err := retryClient.Authenticate()
+	if err == nil {
+		t.Error("Authenticate should fail after max retries")
+	}
+	if !errors.Is(err, testError) {
+		t.Errorf("Expected error %v, got %v", testError, err)
+	}
+	// With maxRetries=3, backoff.Steps=3 means 3 total attempts
+	if mock.authenticateCalls != 3 {
+		t.Errorf("Expected 3 authenticate calls, got %d", mock.authenticateCalls)
+	}
+
+	// Reset for GetSecret test
+	mock.getSecretCalls = 0
+	_, err = retryClient.GetSecret(client.SecretRequest{Name: "test"})
+	if err == nil {
+		t.Error("GetSecret should fail after max retries")
+	}
+	if !errors.Is(err, testError) {
+		t.Errorf("Expected error %v, got %v", testError, err)
+	}
+	if mock.getSecretCalls != 3 {
+		t.Errorf("Expected 3 getSecret calls, got %d", mock.getSecretCalls)
+	}
+}
+
+func TestRetryClientRetryInterval(t *testing.T) {
+	testError := errors.New("temporary error")
+	mock := &mockClient{
+		failUntilCall: 3, // fail twice
+		returnError:   testError,
+	}
+
+	retryInterval := 20 * time.Millisecond
+	retryClient := newRetryableClient(mock, 5, retryInterval)
+
+	start := time.Now()
+	_ = retryClient.Authenticate()
+	elapsed := time.Since(start)
+
+	// Should have waited at least retryInterval (for the first retry)
+	// Note: DefaultBackoff has Factor=5.0 and Jitter=0.1, so delays increase exponentially
+	// We just verify it took some reasonable time with retries
+	minExpected := retryInterval
+	if elapsed < minExpected {
+		t.Errorf("Expected at least %v elapsed time for retries, got %v", minExpected, elapsed)
+	}
+
+	// Sanity check - with exponential backoff (Factor=5.0), shouldn't take too excessively long
+	// First retry: ~20ms, Second retry: ~100ms (20ms * 5), Total: ~120ms + execution time
+	maxExpected := 1 * time.Second
+	if elapsed > maxExpected {
+		t.Errorf("Expected less than %v elapsed time, got %v (may indicate exponential backoff issue)", maxExpected, elapsed)
+	}
+}
+
+func TestRetryClientBaseURL(t *testing.T) {
+	mock := &mockClient{}
+	retryClient := newRetryableClient(mock, 3, 10*time.Millisecond)
+
+	baseURL := retryClient.BaseURL()
+	if baseURL == nil {
+		t.Error("BaseURL should not be nil")
+	}
+	if baseURL.Host != "api.doppler.com" {
+		t.Errorf("Expected host 'api.doppler.com', got '%s'", baseURL.Host)
+	}
+}
+
+func TestRetryClientZeroRetries(t *testing.T) {
+	testError := errors.New("error")
+	mock := &mockClient{
+		failUntilCall: 5, // fail multiple times
+		returnError:   testError,
+	}
+
+	// maxRetries = 0 means we don't override Steps, so it uses DefaultBackoff.Steps = 4
+	retryClient := newRetryableClient(mock, 0, 1*time.Millisecond)
+
+	err := retryClient.Authenticate()
+	if err == nil {
+		t.Error("Expected error with failing calls")
+	}
+	// With maxRetries=0, Steps is not overridden, so it uses DefaultBackoff.Steps=4
+	if mock.authenticateCalls != 4 {
+		t.Errorf("Expected 4 authenticate calls (DefaultBackoff.Steps), got %d", mock.authenticateCalls)
+	}
+}