Oracle provider retry (#2762)

* add oracle provider retry capabilities

Signed-off-by: Andrei Ilas <andrei.ilas@oracle.com>

* add oracle provider retry capabilities unit test

Signed-off-by: Andrei Ilas <andrei.ilas@oracle.com>

* Update unit tests for the Oracle provider retry config

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

---------

Signed-off-by: Andrei Ilas <andrei.ilas@oracle.com>
Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>
Co-authored-by: Andrei Ilas <andrei.ilas@oracle.com>
Co-authored-by: Andrei Ilas <andrei.cva@gmail.com>

pkg/provider/oracle/oracle.go

@@ -18,6 +18,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
+	"time"
 
 	"github.com/oracle/oci-go-sdk/v56/common"
 	"github.com/oracle/oci-go-sdk/v56/common/auth"
@@ -36,10 +37,6 @@ import (
 )
 
 const (
-	VaultEndpointEnv = "ORACLE_VAULT_ENDPOINT"
-	STSEndpointEnv   = "ORACLE_STS_ENDPOINT"
-	SVMEndpointEnv   = "ORACLE_SVM_ENDPOINT"
-
 	errOracleClient                          = "cannot setup new oracle client: %w"
 	errORACLECredSecretName                  = "invalid oracle SecretStore resource: missing oracle APIKey"
 	errUninitalizedOracleProvider            = "provider oracle is not initialized"
@@ -165,6 +162,7 @@ func (vms *VaultManagementService) NewClient(ctx context.Context, store esv1beta
 		err                   error
 		configurationProvider common.ConfigurationProvider
 	)
+
 	if oracleSpec.Auth == nil {
 		configurationProvider, err = auth.InstancePrincipalConfigurationProvider()
 	} else {
@@ -188,6 +186,32 @@ func (vms *VaultManagementService) NewClient(ctx context.Context, store esv1beta
 
 	kmsVaultClient.SetRegion(oracleSpec.Region)
 
+	if storeSpec.RetrySettings != nil {
+		opts := []common.RetryPolicyOption{common.WithShouldRetryOperation(common.DefaultShouldRetryOperation)}
+
+		if mr := storeSpec.RetrySettings.MaxRetries; mr != nil {
+			opts = append(opts, common.WithMaximumNumberAttempts(uint(*mr)))
+		}
+
+		if ri := storeSpec.RetrySettings.RetryInterval; ri != nil {
+			i, err := time.ParseDuration(*storeSpec.RetrySettings.RetryInterval)
+			if err != nil {
+				return nil, fmt.Errorf(errOracleClient, err)
+			}
+			opts = append(opts, common.WithFixedBackoff(i))
+		}
+
+		customRetryPolicy := common.NewRetryPolicyWithOptions(opts...)
+
+		secretManagementService.SetCustomClientConfiguration(common.CustomClientConfiguration{
+			RetryPolicy: &customRetryPolicy,
+		})
+
+		kmsVaultClient.SetCustomClientConfiguration(common.CustomClientConfiguration{
+			RetryPolicy: &customRetryPolicy,
+		})
+	}
+
 	return &VaultManagementService{
 		Client:         secretManagementService,
 		KmsVaultClient: kmsVaultClient,

pkg/provider/oracle/oracle_test.go

@@ -15,17 +15,25 @@ package oracle
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
 	"encoding/base64"
+	"encoding/pem"
 	"fmt"
 	"reflect"
 	"strings"
 	"testing"
 
-	secrets "github.com/oracle/oci-go-sdk/v56/secrets"
-	utilpointer "k8s.io/utils/ptr"
+	"github.com/oracle/oci-go-sdk/v56/secrets"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/ptr"
+	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
-	v1 "github.com/external-secrets/external-secrets/apis/meta/v1"
+	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	fakeoracle "github.com/external-secrets/external-secrets/pkg/provider/oracle/fake"
 )
 
@@ -74,8 +82,8 @@ func makeValidRef() *esv1beta1.ExternalSecretDataRemoteRef {
 
 func makeValidAPIInput() *secrets.GetSecretBundleByNameRequest {
 	return &secrets.GetSecretBundleByNameRequest{
-		SecretName: utilpointer.To("test-secret"),
-		VaultId:    utilpointer.To("test-vault"),
+		SecretName: ptr.To("test-secret"),
+		VaultId:    ptr.To("test-vault"),
 	}
 }
 
@@ -113,10 +121,10 @@ func TestOracleVaultGetSecret(t *testing.T) {
 	setSecretString := func(smtc *vaultTestCase) {
 		smtc.apiOutput = &secrets.GetSecretBundleByNameResponse{
 			SecretBundle: secrets.SecretBundle{
-				SecretId:      utilpointer.To("test-id"),
-				VersionNumber: utilpointer.To(int64(1)),
+				SecretId:      ptr.To("test-id"),
+				VersionNumber: ptr.To(int64(1)),
 				SecretBundleContent: secrets.Base64SecretBundleContentDetails{
-					Content: utilpointer.To(base64.StdEncoding.EncodeToString([]byte(secretValue))),
+					Content: ptr.To(base64.StdEncoding.EncodeToString([]byte(secretValue))),
 				},
 			},
 		}
@@ -147,7 +155,7 @@ func TestGetSecretMap(t *testing.T) {
 	// good case: default version & deserialization
 	setDeserialization := func(smtc *vaultTestCase) {
 		smtc.apiOutput.SecretBundleContent = secrets.Base64SecretBundleContentDetails{
-			Content: utilpointer.To(base64.StdEncoding.EncodeToString([]byte(`{"foo":"bar"}`))),
+			Content: ptr.To(base64.StdEncoding.EncodeToString([]byte(`{"foo":"bar"}`))),
 		}
 		smtc.expectedData["foo"] = []byte("bar")
 	}
@@ -155,7 +163,7 @@ func TestGetSecretMap(t *testing.T) {
 	// bad case: invalid json
 	setInvalidJSON := func(smtc *vaultTestCase) {
 		smtc.apiOutput.SecretBundleContent = secrets.Base64SecretBundleContentDetails{
-			Content: utilpointer.To(base64.StdEncoding.EncodeToString([]byte(`-----------------`))),
+			Content: ptr.To(base64.StdEncoding.EncodeToString([]byte(`-----------------`))),
 		}
 		smtc.expectError = "unable to unmarshal secret"
 	}
@@ -220,7 +228,7 @@ func withSecretAuth(user, tenancy string) storeModifier {
 }
 func withPrivateKey(name, key string, namespace *string) storeModifier {
 	return func(store *esv1beta1.SecretStore) *esv1beta1.SecretStore {
-		store.Spec.Provider.Oracle.Auth.SecretRef.PrivateKey = v1.SecretKeySelector{
+		store.Spec.Provider.Oracle.Auth.SecretRef.PrivateKey = esmeta.SecretKeySelector{
 			Name:      name,
 			Key:       key,
 			Namespace: namespace,
@@ -230,7 +238,7 @@ func withPrivateKey(name, key string, namespace *string) storeModifier {
 }
 func withFingerprint(name, key string, namespace *string) storeModifier {
 	return func(store *esv1beta1.SecretStore) *esv1beta1.SecretStore {
-		store.Spec.Provider.Oracle.Auth.SecretRef.Fingerprint = v1.SecretKeySelector{
+		store.Spec.Provider.Oracle.Auth.SecretRef.Fingerprint = esmeta.SecretKeySelector{
 			Name:      name,
 			Key:       key,
 			Namespace: namespace,
@@ -304,3 +312,195 @@ func TestValidateStore(t *testing.T) {
 		}
 	}
 }
+
+func TestVaultManagementService_NewClient(t *testing.T) {
+	t.Parallel()
+
+	namespace := "default"
+	authSecretName := "oracle-auth"
+
+	auth := &esv1beta1.OracleAuth{
+		User:    "user",
+		Tenancy: "tenancy",
+		SecretRef: esv1beta1.OracleSecretRef{
+			PrivateKey: esmeta.SecretKeySelector{
+				Name: authSecretName,
+				Key:  "privateKey",
+			},
+			Fingerprint: esmeta.SecretKeySelector{
+				Name: authSecretName,
+				Key:  "fingerprint",
+			},
+		},
+	}
+
+	tests := []struct {
+		desc        string
+		secretStore *esv1beta1.SecretStore
+		expectedErr string
+	}{
+		{
+			desc: "no retry settings",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth:   auth,
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "fill all the retry settings",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth:   auth,
+						},
+					},
+					RetrySettings: &esv1beta1.SecretStoreRetrySettings{
+						RetryInterval: ptr.To("1s"),
+						MaxRetries:    ptr.To(int32(5)),
+					},
+				},
+			},
+		},
+		{
+			desc: "partially configure the retry settings - retry interval",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth:   auth,
+						},
+					},
+					RetrySettings: &esv1beta1.SecretStoreRetrySettings{
+						RetryInterval: ptr.To("1s"),
+					},
+				},
+			},
+		},
+		{
+			desc: "partially configure the retry settings - max retries",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth:   auth,
+						},
+					},
+					RetrySettings: &esv1beta1.SecretStoreRetrySettings{
+						MaxRetries: ptr.To(int32(5)),
+					},
+				},
+			},
+		},
+		{
+			desc: "auth secret does not exist",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth: &esv1beta1.OracleAuth{
+								User:    "user",
+								Tenancy: "tenancy",
+								SecretRef: esv1beta1.OracleSecretRef{
+									PrivateKey: esmeta.SecretKeySelector{
+										Name: "non-existing-secret",
+										Key:  "privateKey",
+									},
+									Fingerprint: esmeta.SecretKeySelector{
+										Name: "non-existing-secret",
+										Key:  "fingerprint",
+									},
+								},
+							},
+						},
+					},
+					RetrySettings: &esv1beta1.SecretStoreRetrySettings{
+						RetryInterval: ptr.To("invalid"),
+					},
+				},
+			},
+			expectedErr: `could not fetch SecretAccessKey secret: secrets "non-existing-secret"`,
+		},
+		{
+			desc: "invalid retry interval",
+			secretStore: &esv1beta1.SecretStore{
+				Spec: esv1beta1.SecretStoreSpec{
+					Provider: &esv1beta1.SecretStoreProvider{
+						Oracle: &esv1beta1.OracleProvider{
+							Vault:  vaultOCID,
+							Region: region,
+							Auth:   auth,
+						},
+					},
+					RetrySettings: &esv1beta1.SecretStoreRetrySettings{
+						RetryInterval: ptr.To("invalid"),
+					},
+				},
+			},
+			expectedErr: "cannot setup new oracle client: time: invalid duration",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			provider := &VaultManagementService{
+				Client:         &fakeoracle.OracleMockClient{},
+				KmsVaultClient: nil,
+				vault:          vaultOCID,
+			}
+
+			pk, err := rsa.GenerateKey(rand.Reader, 2048)
+			if err != nil {
+				t.Fatalf("failed to create a private key: %v", err)
+			}
+			schema := runtime.NewScheme()
+			if err := corev1.AddToScheme(schema); err != nil {
+				t.Fatalf("failed to add to schema: %v", err)
+			}
+			builder := clientfake.NewClientBuilder().WithRuntimeObjects(&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      authSecretName,
+					Namespace: namespace,
+				},
+				Data: map[string][]byte{
+					"privateKey": pem.EncodeToMemory(&pem.Block{
+						Type:  "RSA PRIVATE KEY",
+						Bytes: x509.MarshalPKCS1PrivateKey(pk),
+					}),
+					"fingerprint": []byte("fingerprint"),
+				},
+			})
+
+			_, err = provider.NewClient(context.Background(), tc.secretStore, builder.Build(), namespace)
+			if err != nil {
+				if tc.expectedErr == "" {
+					t.Fatalf("failed to call NewClient: %v", err)
+				}
+
+				if !strings.Contains(err.Error(), tc.expectedErr) {
+					t.Fatalf("received an unexpected error: %q should have contained %q", err.Error(), tc.expectedErr)
+				}
+				return
+			}
+
+			if tc.expectedErr != "" {
+				t.Fatalf("expeceted to receive an error but got nil")
+			}
+		})
+	}
+}