Browse Source

fix(gcp): check for secret version exists in PushSecret (#5593)

Signed-off-by: Brendan Palkowski <b.palko@outlook.com>
Brendan Palkowski 4 months ago
parent
commit
a70586af08

+ 20 - 1
providers/v1/gcp/secretmanager/client.go

@@ -147,6 +147,7 @@ func parseError(err error) error {
 }
 
 // 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.
 func (c *Client) SecretExists(ctx context.Context, ref esv1.PushSecretRemoteRef) (bool, error) {
 	secretName := fmt.Sprintf(globalSecretPath, c.store.ProjectID, ref.GetRemoteKey())
 	gcpSecret, err := c.smClient.GetSecret(ctx, &secretmanagerpb.GetSecretRequest{
@@ -160,7 +161,25 @@ func (c *Client) SecretExists(ctx context.Context, ref esv1.PushSecretRemoteRef)
 		return false, err
 	}
 
-	return gcpSecret != nil, nil
+	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)
+
+	if err != nil {
+		if status.Code(err) == codes.NotFound {
+			// Secret exists but has no versions
+			return false, nil
+		}
+		return false, err
+	}
+
+	return true, nil
 }
 
 // PushSecret pushes a kubernetes secret key into gcp provider Secret.

+ 62 - 8
providers/v1/gcp/secretmanager/client_test.go

@@ -1009,11 +1009,12 @@ func TestPushSecret(t *testing.T) {
 
 func TestSecretExists(t *testing.T) {
 	tests := []struct {
-		name                string
-		ref                 esv1.PushSecretRemoteRef
-		getSecretMockReturn fakesm.SecretMockReturn
-		expectedSecret      bool
-		expectedErr         func(t *testing.T, err error)
+		name                          string
+		ref                           esv1.PushSecretRemoteRef
+		getSecretMockReturn           fakesm.SecretMockReturn
+		accessSecretVersionMockReturn fakesm.AccessSecretVersionMockReturn
+		expectedSecret                bool
+		expectedErr                   func(t *testing.T, err error)
 	}{
 		{
 			name: "secret exists",
@@ -1026,26 +1027,57 @@ func TestSecretExists(t *testing.T) {
 				},
 				Err: nil,
 			},
+			accessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{
+				Res: &secretmanagerpb.AccessSecretVersionResponse{
+					Name: "projects/foo/secrets/bar/versions/latest",
+					Payload: &secretmanagerpb.SecretPayload{
+						Data: []byte("test-data"),
+					},
+				},
+				Err: nil,
+			},
 			expectedSecret: true,
 			expectedErr: func(t *testing.T, err error) {
 				require.NoError(t, err)
 			},
 		},
 		{
-			name: "secret does not exists",
+			name: "secret exists but has no versions",
 			ref: v1alpha1.PushSecretRemoteRef{
 				RemoteKey: "bar",
 			},
 			getSecretMockReturn: fakesm.SecretMockReturn{
+				Secret: &secretmanagerpb.Secret{
+					Name: testSecretName,
+				},
 				Err: nil,
 			},
+			accessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{
+				Res: nil,
+				Err: status.Errorf(codes.NotFound, "secret version not found"),
+			},
 			expectedSecret: false,
 			expectedErr: func(t *testing.T, err error) {
 				require.NoError(t, err)
 			},
 		},
 		{
-			name: "unexpected error occurs",
+			name: "secret does not exists",
+			ref: v1alpha1.PushSecretRemoteRef{
+				RemoteKey: "bar",
+			},
+			getSecretMockReturn: fakesm.SecretMockReturn{
+				Secret: nil,
+				Err:    status.Errorf(codes.NotFound, "secret not found"),
+			},
+			accessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{},
+			expectedSecret:                false,
+			expectedErr: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+		{
+			name: "unexpected error when getting secret",
 			ref: v1alpha1.PushSecretRemoteRef{
 				RemoteKey: "bar2",
 			},
@@ -1055,17 +1087,39 @@ func TestSecretExists(t *testing.T) {
 				},
 				Err: errors.New("some error"),
 			},
-			expectedSecret: false,
+			accessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{},
+			expectedSecret:                false,
 			expectedErr: func(t *testing.T, err error) {
 				assert.ErrorContains(t, err, "some error")
 			},
 		},
+		{
+			name: "unexpected error occurs when accessing secret version",
+			ref: v1alpha1.PushSecretRemoteRef{
+				RemoteKey: "bar3",
+			},
+			getSecretMockReturn: fakesm.SecretMockReturn{
+				Secret: &secretmanagerpb.Secret{
+					Name: testSecretName,
+				},
+				Err: nil,
+			},
+			accessSecretVersionMockReturn: fakesm.AccessSecretVersionMockReturn{
+				Res: nil,
+				Err: errors.New("version access error"),
+			},
+			expectedSecret: false,
+			expectedErr: func(t *testing.T, err error) {
+				assert.ErrorContains(t, err, "version access error")
+			},
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			smClient := fakesm.MockSMClient{}
 			smClient.NewGetSecretFn(tc.getSecretMockReturn)
+			smClient.NewAccessSecretVersionFn(tc.accessSecretVersionMockReturn)
 
 			client := Client{
 				smClient: &smClient,