Browse Source

📝 Add docstrings to `improve-gcpsm-provider-secret-exists`

Docstrings generation was requested by @tosih.

* https://github.com/external-secrets/external-secrets/pull/5610#issuecomment-3667343718

The following files were modified:

* `providers/v1/gcp/secretmanager/client.go`
* `providers/v1/gcp/secretmanager/fake/fake.go`
coderabbitai[bot] 5 months ago
parent
commit
15411c1733
2 changed files with 122 additions and 38 deletions
  1. 27 37
      providers/v1/gcp/secretmanager/client.go
  2. 95 1
      providers/v1/gcp/secretmanager/fake/fake.go

+ 27 - 37
providers/v1/gcp/secretmanager/client.go

@@ -138,6 +138,8 @@ func (c *Client) DeleteSecret(ctx context.Context, remoteRef esv1.PushSecretRemo
 	return err
 }
 
+// parseError converts a GCP API NotFound error into an esv1.NoSecretError.
+// If the provided error is not a NotFound APIError, it returns the original error.
 func parseError(err error) error {
 	var gerr *apierror.APIError
 	if errors.As(err, &gerr) && gerr.GRPCStatus().Code() == codes.NotFound {
@@ -146,42 +148,16 @@ func parseError(err error) error {
 	return err
 }
 
-// SecretExists checks if a secret exists in Google Cloud Secret Manager.
-// It verifies the existence of a secret in Google Cloud Secret Manager AND that it has at least one version.
+// SecretExists checks if a secret exists in Google Cloud Secret Manager and has at least one accessible version.
 func (c *Client) SecretExists(ctx context.Context, ref esv1.PushSecretRemoteRef) (bool, error) {
-	secretName := fmt.Sprintf(globalSecretPath, c.store.ProjectID, ref.GetRemoteKey())
-	if c.store.Location != "" {
-		secretName = fmt.Sprintf(regionalSecretPath, c.store.ProjectID, c.store.Location, ref.GetRemoteKey())
-	}
-	gcpSecret, err := c.smClient.GetSecret(ctx, &secretmanagerpb.GetSecretRequest{
-		Name: secretName,
-	})
-	if err != nil {
-		if status.Code(err) == codes.NotFound {
-			return false, nil
-		}
-
-		return false, err
-	}
-
-	if gcpSecret == nil {
-		return false, nil
-	}
-	// Check if the secret has at least one version
-	versionName := fmt.Sprintf("%s/versions/latest", secretName)
-	_, err = c.smClient.AccessSecretVersion(ctx, &secretmanagerpb.AccessSecretVersionRequest{
-		Name: versionName,
-	})
-	metrics.ObserveAPICall(constants.ProviderGCPSM, constants.CallGCPSMAccessSecretVersion, err)
-
+	secretName := getName(c.store.ProjectID, c.store.Location, ref.GetRemoteKey())
+	_, err := getLatestEnabledVersion(ctx, c.smClient, secretName)
 	if err != nil {
 		if status.Code(err) == codes.NotFound {
-			// Secret exists but has no versions
 			return false, nil
 		}
 		return false, err
 	}
-
 	return true, nil
 }
 
@@ -215,7 +191,7 @@ func (c *Client) PushSecret(ctx context.Context, secret *corev1.Secret, pushSecr
 			return err
 		}
 
-		var replication = &secretmanagerpb.Replication{
+		replication := &secretmanagerpb.Replication{
 			Replication: &secretmanagerpb.Replication_Automatic_{
 				Automatic: &secretmanagerpb.Replication_Automatic{},
 			},
@@ -733,25 +709,39 @@ func isErrSecretDestroyedOrDisabled(err error) bool {
 		(strings.Contains(st.Message(), "DESTROYED state") || strings.Contains(st.Message(), "DISABLED state"))
 }
 
+// getLatestEnabledVersion selects the enabled secret version with the most recent creation time under the given secret name and returns the result of accessing that version.
+// If no enabled versions are found, it attempts to access the "latest" version which may return NotFound, FailedPrecondition, or other errors. It also returns any iterator or access errors encountered.
 func getLatestEnabledVersion(ctx context.Context, client GoogleSecretManagerClient, name string) (*secretmanagerpb.AccessSecretVersionResponse, error) {
 	iter := client.ListSecretVersions(ctx, &secretmanagerpb.ListSecretVersionsRequest{
 		Parent: name,
 		Filter: "state:ENABLED",
 	})
+
 	latestCreateTime := time.Unix(0, 0)
-	latestVersion := &secretmanagerpb.SecretVersion{}
+	versionName := "latest"
 	for {
 		version, err := iter.Next()
-		if errors.Is(err, iterator.Done) {
-			break
+		if err != nil {
+			if errors.Is(err, iterator.Done) {
+				break
+			}
+			return nil, err
 		}
 		if version.CreateTime.AsTime().After(latestCreateTime) {
 			latestCreateTime = version.CreateTime.AsTime()
-			latestVersion = version
+			versionName = version.Name
 		}
 	}
+
+	// If no enabled versions found, versionName remains "latest"
+	// This will return the appropriate error (NotFound, FailedPrecondition, etc.)
 	req := &secretmanagerpb.AccessSecretVersionRequest{
-		Name: fmt.Sprintf("%s/versions/%s", name, latestVersion.Name),
+		Name: fmt.Sprintf("%s/versions/%s", name, versionName),
 	}
-	return client.AccessSecretVersion(ctx, req)
-}
+	version, err := client.AccessSecretVersion(ctx, req)
+	metrics.ObserveAPICall(constants.ProviderGCPSM, constants.CallGCPSMAccessSecretVersion, err)
+	if err != nil {
+		return nil, err
+	}
+	return version, nil
+}

+ 95 - 1
providers/v1/gcp/secretmanager/fake/fake.go

@@ -20,12 +20,15 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
+	"unsafe"
 
 	secretmanager "cloud.google.com/go/secretmanager/apiv1"
 	"cloud.google.com/go/secretmanager/apiv1/secretmanagerpb"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/googleapis/gax-go/v2"
+	"google.golang.org/api/iterator"
 )
 
 type MockSMClient struct {
@@ -59,6 +62,88 @@ type AddSecretVersionMockReturn struct {
 	Err           error
 }
 
+type ListSecretVersionsMockReturn struct {
+	Res *secretmanager.SecretVersionIterator
+}
+
+// NewSecretVersionIterator creates a mock iterator for testing.
+// This is simplified to only support what our tests actually need:
+// - Return 0 or 1 versions
+// NewSecretVersionIterator creates a mock *secretmanager.SecretVersionIterator that yields the provided
+// SecretVersion slice on the first page and returns iterator.Done on subsequent iterations.
+// 
+// The iterator simulates a single-page response: all versions are returned once and then the iterator
+// is exhausted. This helper is intended for tests and initializes the iterator's internal state to
+// produce predictable pagination behaviour.
+func NewSecretVersionIterator(versions []*secretmanagerpb.SecretVersion) *secretmanager.SecretVersionIterator {
+	it := &secretmanager.SecretVersionIterator{}
+
+	// Simple helper to set an unexported field using reflection
+	setField := func(name string, value any) {
+		v := reflect.ValueOf(it).Elem()
+		field := v.FieldByName(name)
+		if field.IsValid() {
+			//#nosec G103 -- Audited this use and it's only for mocking in testing
+			reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).
+				Elem().Set(reflect.ValueOf(value))
+		}
+	}
+
+	// Simple state: are we done?
+	done := false
+
+	// InternalFetch returns all versions on first call, then nothing
+	it.InternalFetch = func(pageSize int, pageToken string) ([]*secretmanagerpb.SecretVersion, string, error) {
+		if done {
+			return nil, "", nil
+		}
+		done = true
+		return versions, "", nil
+	}
+
+	// Simple fetch: call InternalFetch and store results in items field
+	fetch := func(pageSize int, pageToken string) (string, error) {
+		results, nextToken, err := it.InternalFetch(pageSize, pageToken)
+		if err != nil {
+			return "", err
+		}
+		setField("items", results)
+		return nextToken, nil
+	}
+
+	// bufLen: return length of items field
+	bufLen := func() int {
+		v := reflect.ValueOf(it).Elem()
+		itemsField := v.FieldByName("items")
+		if itemsField.IsValid() {
+			return itemsField.Len()
+		}
+		return 0
+	}
+
+	// takeBuf: return and clear items field
+	takeBuf := func() any {
+		v := reflect.ValueOf(it).Elem()
+		itemsField := v.FieldByName("items")
+		if itemsField.IsValid() {
+			items := itemsField.Interface()
+			setField("items", []*secretmanagerpb.SecretVersion(nil))
+			return items
+		}
+		return []*secretmanagerpb.SecretVersion(nil)
+	}
+
+	// Create the PageInfo and nextFunc using the iterator package
+	pageInfo, nextFunc := iterator.NewPageInfo(fetch, bufLen, takeBuf)
+
+	// Set the required unexported fields
+	setField("pageInfo", pageInfo)
+	setField("nextFunc", nextFunc)
+	setField("items", []*secretmanagerpb.SecretVersion(nil))
+
+	return it
+}
+
 type SecretMockReturn struct {
 	Secret *secretmanagerpb.Secret
 	Err    error
@@ -67,11 +152,13 @@ type SecretMockReturn struct {
 func (mc *MockSMClient) DeleteSecret(ctx context.Context, req *secretmanagerpb.DeleteSecretRequest, _ ...gax.CallOption) error {
 	return mc.DeleteSecretFn(ctx, req)
 }
+
 func (mc *MockSMClient) NewDeleteSecretFn(err error) {
 	mc.DeleteSecretFn = func(_ context.Context, _ *secretmanagerpb.DeleteSecretRequest, _ ...gax.CallOption) error {
 		return err
 	}
 }
+
 func (mc *MockSMClient) GetSecret(ctx context.Context, req *secretmanagerpb.GetSecretRequest, _ ...gax.CallOption) (*secretmanagerpb.Secret, error) {
 	return mc.GetSecretFn(ctx, req)
 }
@@ -95,6 +182,7 @@ func (mc *MockSMClient) NewAccessSecretVersionFn(mock AccessSecretVersionMockRet
 func (mc *MockSMClient) ListSecrets(ctx context.Context, req *secretmanagerpb.ListSecretsRequest, _ ...gax.CallOption) *secretmanager.SecretIterator {
 	return mc.ListSecretsFn(ctx, req)
 }
+
 func (mc *MockSMClient) Close() error {
 	return mc.closeFn()
 }
@@ -207,6 +295,12 @@ func (mc *MockSMClient) ListSecretVersions(ctx context.Context, req *secretmanag
 	return mc.ListSecretVersionsFn(ctx, req)
 }
 
+func (mc *MockSMClient) NewListSecretVersionsFn(mock ListSecretVersionsMockReturn) {
+	mc.ListSecretVersionsFn = func(_ context.Context, _ *secretmanagerpb.ListSecretVersionsRequest, _ ...gax.CallOption) *secretmanager.SecretVersionIterator {
+		return mock.Res
+	}
+}
+
 func (mc *MockSMClient) WithValue(_ context.Context, req *secretmanagerpb.AccessSecretVersionRequest, val *secretmanagerpb.AccessSecretVersionResponse, err error) {
 	if mc != nil {
 		mc.accessSecretFn = func(paramCtx context.Context, paramReq *secretmanagerpb.AccessSecretVersionRequest, paramOpts ...gax.CallOption) (*secretmanagerpb.AccessSecretVersionResponse, error) {
@@ -218,4 +312,4 @@ func (mc *MockSMClient) WithValue(_ context.Context, req *secretmanagerpb.Access
 			return val, err
 		}
 	}
-}
+}