refactor(provider): apply review feedback

- wrap errors

- clarify PushSecret updatePolicy comment

- remove duplicate data.GetRemoteKey calls

- call errors.Is once

- improve pushNewSecret clarity

- document 'Not Found' OKMS error code

Signed-off-by: Jordan Sauvain <jordan.sauvain@ovhcloud.com>

providers/v1/ovh/client_delete_secret.go

@@ -19,6 +19,7 @@ package ovh
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
@@ -29,8 +30,13 @@ import (
 func (cl *ovhClient) DeleteSecret(ctx context.Context, remoteRef esv1.PushSecretRemoteRef) error {
 	err := cl.okmsClient.DeleteSecretV2(ctx, cl.okmsID, remoteRef.GetRemoteKey())
 
-	if err != nil && errors.Is(handleOkmsError(err), esv1.NoSecretErr) {
-		return nil
+	if err != nil {
+		err = handleOkmsError(err)
+		if errors.Is(err, esv1.NoSecretErr) {
+			return nil
+		}
+		return fmt.Errorf("failed to delete secret at path %q: %w", remoteRef.GetRemoteKey(), err)
 	}
-	return err
+
+	return nil
 }

providers/v1/ovh/client_get_all_secrets.go

@@ -19,6 +19,7 @@ package ovh
 import (
 	"context"
 	"errors"
+	"fmt"
 	"regexp"
 
 	"github.com/google/uuid"
@@ -34,10 +35,17 @@ func (cl *ovhClient) GetAllSecrets(ctx context.Context, ref esv1.ExternalSecretF
 	// List Secret Manager secrets.
 	secrets, err := getSecretsList(ctx, cl.okmsClient, cl.okmsID, ref.Path)
 	if err != nil {
-		return map[string][]byte{}, err
+		return map[string][]byte{}, fmt.Errorf("failed to retrieve multiple secrets: %w", err)
 	}
 	if len(secrets) == 0 {
-		return map[string][]byte{}, errors.New("no secrets found in the secret manager")
+		path := ""
+		if ref.Path != nil {
+			path = *ref.Path
+		}
+		return map[string][]byte{}, fmt.Errorf(
+			"failed to retrieve multiple secrets: no secrets under path %q were found in the secret manager",
+			path,
+		)
 	}
 
 	// Compile the regular expression defined in ref.Name.RegExp, if present.
@@ -45,12 +53,25 @@ func (cl *ovhClient) GetAllSecrets(ctx context.Context, ref esv1.ExternalSecretF
 
 	if ref.Name != nil {
 		regex, err = regexp.Compile(ref.Name.RegExp)
-		if err != nil || regex == nil {
-			return map[string][]byte{}, errors.New("failed to parse regexp")
+		if err != nil {
+			return map[string][]byte{}, fmt.Errorf(
+				"failed to retrieve multiple secrets: could not parse regex: %w", err,
+			)
+		}
+		if regex == nil {
+			return map[string][]byte{}, fmt.Errorf(
+				"failed to retrieve multiple secrets: compiled regex is nil for expression %q",
+				ref.Name.RegExp,
+			)
 		}
 	}
 
-	return filterSecretsListWithRegexp(ctx, cl, secrets, regex, ref)
+	secretsMap, err := filterSecretsListWithRegexp(ctx, cl, secrets, regex, ref)
+	if err != nil {
+		return map[string][]byte{}, fmt.Errorf("failed to retrieve multiple secrets: %w", err)
+	}
+
+	return secretsMap, nil
 }
 
 // Retrieve secrets located under the specified path.
@@ -122,7 +143,7 @@ func recursivelyGetSecretsList(ctx context.Context, okmsClient OkmsClient, okmsI
 		return []string{}, nil
 	}
 	if secrets, err = okmsClient.GetSecretsMetadata(ctx, okmsID, path, true); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("could not list secrets at path %q: %w", path, err)
 	}
 	if secrets == nil || secrets.Data == nil || secrets.Data.Keys == nil || len(*secrets.Data.Keys) == 0 {
 		return nil, nil
@@ -190,7 +211,15 @@ func filterSecretsListWithRegexp(ctx context.Context, cl *ovhClient, secrets []s
 		}
 	}
 	if len(secretsDataMap) == 0 {
-		return map[string][]byte{}, errors.New("no secrets could be retrieved")
+		expr := ""
+		if ref.Name != nil {
+			expr = ref.Name.RegExp
+		}
+		path := ""
+		if ref.Path != nil {
+			path = *ref.Path
+		}
+		return map[string][]byte{}, fmt.Errorf("regex expression %q did not match any secret at path %q", expr, path)
 	}
 	return secretsDataMap, nil
 }

providers/v1/ovh/client_get_secret.go

@@ -18,6 +18,8 @@ package ovh
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
@@ -28,7 +30,9 @@ import (
 func (cl *ovhClient) GetSecret(ctx context.Context, ref esv1.ExternalSecretDataRemoteRef) ([]byte, error) {
 	// Retrieve the KMS secret using the OVH SDK.
 	secretData, _, err := getSecretWithOvhSDK(ctx, cl.okmsClient, cl.okmsID, ref)
-	if err != nil {
+	if err != nil && !errors.Is(err, esv1.NoSecretErr) {
+		return []byte{}, fmt.Errorf("failed to retrieve secret at path %q: %w", ref.Key, err)
+	} else if err != nil {
 		return []byte{}, err
 	}
 

providers/v1/ovh/client_get_secret_map.go

@@ -19,6 +19,8 @@ package ovh
 import (
 	"context"
 	"encoding/json"
+	"errors"
+	"fmt"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 	"github.com/external-secrets/external-secrets/runtime/esutils"
@@ -31,7 +33,9 @@ import (
 func (cl *ovhClient) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
 	// Retrieve secret from KMS.
 	secretDataBytes, _, err := getSecretWithOvhSDK(ctx, cl.okmsClient, cl.okmsID, ref)
-	if err != nil {
+	if err != nil && !errors.Is(err, esv1.NoSecretErr) {
+		return map[string][]byte{}, fmt.Errorf("failed to retrieve secret at path %q: %w", ref.Key, err)
+	} else if err != nil {
 		return map[string][]byte{}, err
 	}
 	if len(secretDataBytes) == 0 {
@@ -43,7 +47,7 @@ func (cl *ovhClient) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDa
 	var rawSecretDataMap map[string]any
 	err = json.Unmarshal(secretDataBytes, &rawSecretDataMap)
 	if err != nil {
-		return map[string][]byte{}, err
+		return map[string][]byte{}, fmt.Errorf("failed to retrieve secret at path %q: %w", ref.Key, err)
 	}
 
 	// Convert the map[string]any into map[string][]byte.
@@ -51,7 +55,7 @@ func (cl *ovhClient) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDa
 	for key := range rawSecretDataMap {
 		secretDataMap[key], err = esutils.GetByteValueFromMap(rawSecretDataMap, key)
 		if err != nil {
-			return map[string][]byte{}, err
+			return map[string][]byte{}, fmt.Errorf("failed to retrieve secret at path %q: %w", ref.Key, err)
 		}
 	}
 

providers/v1/ovh/client_push_secret.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 
 	"github.com/google/uuid"
 	"github.com/ovh/okms-sdk-go/types"
@@ -29,38 +30,39 @@ import (
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
 
-// Create or update a secret.
-//
-// If updatePolicy is set to Replace, the secret will be written, possibly overwriting an existing secret.
-// If set to IfNotExists, it will not overwrite an existing secret.
+// PushSecret pushes a secret to the Secret Manager according to the updatePolicy
+// defined in the PushSecret (create or update).
 func (cl *ovhClient) PushSecret(ctx context.Context, secret *corev1.Secret, data esv1.PushSecretData) error {
+	remoteKey := data.GetRemoteKey()
+
 	if secret == nil {
-		return errors.New("nil secret")
+		return fmt.Errorf("failed to push secret at path %q: provided secret is nil", remoteKey)
 	}
 	if len(secret.Data) == 0 {
-		return errors.New("cannot push empty secret")
+		return fmt.Errorf("failed to push secret at path %q: provided secret is empty", remoteKey)
 	}
 
 	// Check if the secret already exists.
 	// This determines which method to use: create or update.
 	remoteSecret, currentVersion, err := getSecretWithOvhSDK(ctx, cl.okmsClient, cl.okmsID, esv1.ExternalSecretDataRemoteRef{
-		Key: data.GetRemoteKey(),
+		Key: remoteKey,
 	})
-	if err != nil && !errors.Is(err, esv1.NoSecretErr) {
-		return err
+	noSecretErr := errors.Is(err, esv1.NoSecretErr)
+	if err != nil && !noSecretErr {
+		return fmt.Errorf("failed to push secret at path %q: %w", remoteKey, err)
 	}
-	secretExists := !errors.Is(err, esv1.NoSecretErr)
+	secretExists := !noSecretErr
 
 	// Build the secret to be pushed.
 	secretToPush, err := buildSecretToPush(secret, data)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to push secret at path %q: %w", remoteKey, err)
 	}
 
 	// Compare the data of secretToPush with that of remoteSecret.
 	equal, err := compareSecretsData(secretToPush, remoteSecret)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to push secret at path %q: %w", remoteKey, err)
 	}
 	if equal {
 		return nil
@@ -72,14 +74,11 @@ func (cl *ovhClient) PushSecret(ctx context.Context, secret *corev1.Secret, data
 	}
 
 	// Push the secret.
-	return pushNewSecret(
-		ctx,
-		cl.okmsClient,
-		cl.okmsID,
-		secretToPush,
-		data.GetRemoteKey(),
-		currentVersion,
-		secretExists)
+	err = pushNewSecret(ctx, cl.okmsClient, cl.okmsID, secretToPush, remoteKey, currentVersion, secretExists)
+	if err != nil {
+		return fmt.Errorf("failed to push secret at path %q: %w", remoteKey, err)
+	}
+	return nil
 }
 
 // Compare the secret to push with the remote secret.
@@ -91,7 +90,7 @@ func compareSecretsData(secretToPush map[string]any, remoteSecret []byte) (bool,
 
 	secretToPushByte, err := json.Marshal(secretToPush)
 	if err != nil {
-		return false, err
+		return false, fmt.Errorf("could not compare remote secret with secret to push: %w", err)
 	}
 
 	return bytes.Equal(secretToPushByte, remoteSecret), nil
@@ -143,7 +142,7 @@ func extractAllSecretValues(data map[string][]byte) (map[string]any, error) {
 		}
 		var cleanJSON []byte
 		if cleanJSON, err = json.Marshal(decoded); err != nil {
-			return map[string]any{}, err
+			return map[string]any{}, fmt.Errorf("could not extract secret's values to push: %w", err)
 		}
 
 		secretValueToPush[key] = json.RawMessage(cleanJSON)
@@ -157,7 +156,9 @@ func extractSecretKeyValue(data map[string][]byte, secretKey string) (map[string
 
 	value, ok := data[secretKey]
 	if !ok {
-		return nil, errors.New("secretKey not found in secret data")
+		return nil, fmt.Errorf(
+			"could not extract secret key value to push: secretKey %q not found in secret data", secretKey,
+		)
 	}
 	var decoded any
 	if json.Unmarshal(value, &decoded) != nil {
@@ -172,9 +173,11 @@ func extractSecretKeyValue(data map[string][]byte, secretKey string) (map[string
 // This pushes the created/updated secret.
 func pushNewSecret(ctx context.Context, okmsClient OkmsClient, okmsID uuid.UUID, secretToPush map[string]any, path string, cas *uint32, secretExists bool) error {
 	var err error
+	var operation string
 
 	if !secretExists {
 		// Create a secret.
+		operation = "create"
 		_, err = okmsClient.PostSecretV2(ctx, okmsID, types.PostSecretV2Request{
 			Path: path,
 			Version: types.SecretV2VersionShort{
@@ -183,6 +186,7 @@ func pushNewSecret(ctx context.Context, okmsClient OkmsClient, okmsID uuid.UUID,
 		})
 	} else {
 		// Update a secret.
+		operation = "update"
 		_, err = okmsClient.PutSecretV2(ctx, okmsID, path, cas, types.PutSecretV2Request{
 			Version: &types.SecretV2VersionShort{
 				Data: &secretToPush,
@@ -190,5 +194,8 @@ func pushNewSecret(ctx context.Context, okmsClient OkmsClient, okmsID uuid.UUID,
 		})
 	}
 
-	return err
+	if err != nil {
+		return fmt.Errorf("could not %s remote secret %q: %w", operation, path, err)
+	}
+	return nil
 }

providers/v1/ovh/client_secret_exists.go

@@ -19,19 +19,22 @@ package ovh
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
 
 func (cl *ovhClient) SecretExists(ctx context.Context, remoteRef esv1.PushSecretRemoteRef) (bool, error) {
+	remoteKey := remoteRef.GetRemoteKey()
+
 	// Check if the secret exists using the OVH SDK.
-	_, err := cl.okmsClient.GetSecretV2(ctx, cl.okmsID, remoteRef.GetRemoteKey(), nil, nil)
+	_, err := cl.okmsClient.GetSecretV2(ctx, cl.okmsID, remoteKey, nil, nil)
 	if err != nil {
 		err = handleOkmsError(err)
 		if errors.Is(err, esv1.NoSecretErr) {
 			return false, nil
 		}
-		return false, err
+		return false, fmt.Errorf("failed to check existence of secret %q: %w", remoteKey, err)
 	}
 
 	return true, nil

providers/v1/ovh/client_utils.go

@@ -34,7 +34,7 @@ import (
 func getSecretWithOvhSDK(ctx context.Context, kmsClient OkmsClient, okmsID uuid.UUID, ref esv1.ExternalSecretDataRemoteRef) ([]byte, *uint32, error) {
 	// Check if the remoteRef key is empty.
 	if ref.Key == "" {
-		return []byte{}, nil, errors.New("spec.data.remoteRef.key cannot be empty")
+		return []byte{}, nil, errors.New("remote key cannot be empty (spec.data.remoteRef.key)")
 	}
 
 	// Check MetaDataPolicy (not supported).
@@ -123,7 +123,7 @@ func handleOkmsError(err error) error {
 
 	if okmsError == nil {
 		return fmt.Errorf("failed to parse okms error: %w", err)
-	} else if okmsError.ErrorCode == 17125377 {
+	} else if okmsError.ErrorCode == 17125377 { // 17125377: returned by OKMS when secret was not found
 		return esv1.NoSecretErr
 	}
 	return okmsError

providers/v1/ovh/provider.go

@@ -75,14 +75,14 @@ func (p *Provider) NewClient(ctx context.Context, store esv1.GenericStore, kube
 	}
 
 	if kube == nil {
-		return nil, errors.New("controller-runtime client is nil")
+		return nil, errors.New("failed to create new ovh provider client: controller-runtime client is nil")
 	}
 
 	ovhStore := store.GetSpec().Provider.Ovh
 	// ovhClient configuration.
 	okmsID, err := uuid.Parse(ovhStore.OkmsID)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create new ovh provider client: %w", err)
 	}
 
 	cas := false
@@ -114,7 +114,10 @@ func (p *Provider) NewClient(ctx context.Context, store esv1.GenericStore, kube
 		err = configureHTTPMTLSClient(ctx, p, cl,
 			ovhStore.Server, ovhStore.Auth.ClientMTLS)
 	}
-	return cl, err
+	if err != nil {
+		return nil, fmt.Errorf("failed to create new ovh provider client: %w", err)
+	}
+	return cl, nil
 }
 
 // Configure the client to use the provided token for HTTP requests.

providers/v1/ovh/validate.go

@@ -18,6 +18,7 @@ package ovh
 
 import (
 	"context"
+	"fmt"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
@@ -28,7 +29,7 @@ import (
 func (cl *ovhClient) Validate() (esv1.ValidationResult, error) {
 	_, err := cl.okmsClient.ListSecretV2(context.Background(), cl.okmsID, nil, nil)
 	if err != nil {
-		return esv1.ValidationResultError, err
+		return esv1.ValidationResultError, fmt.Errorf("failed to validate secret store: %w", err)
 	}
 	return esv1.ValidationResultReady, nil
 }