add comment for ValidateCredsCallback

- add validation callback will enable the host service to validate the newly created key before the deleting the old key

- wrapped both the call to validate permissions and validate callback in a retry because sometimes it takes some time for the newly created key to be active on GCP services. This throws an error and prevents host catalog creation

internal/credential/config.go

@@ -98,6 +98,7 @@ func (c *Config) toCredentials() *credentials {
 func (c *Config) clone() *Config {
 	return &Config{
 		ProjectId:              c.ProjectId,
+		Zone:                   c.Zone,
 		PrivateKey:             c.PrivateKey,
 		PrivateKeyId:           c.PrivateKeyId,
 		ClientEmail:            c.ClientEmail,

internal/credential/rotate.go

@@ -6,8 +6,10 @@ package credential
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"slices"
+	"time"
 
 	admin "cloud.google.com/go/iam/admin/apiv1"
 	"cloud.google.com/go/iam/admin/apiv1/adminpb"
@@ -19,9 +21,20 @@ import (
 )
 
 const (
-	ComputeInstancesListPermission        = "compute.instances.list"
+	// ComputeInstancesListPermission is the IAM permission required
+	// to list compute instances.
+	ComputeInstancesListPermission = "compute.instances.list"
+	// IAMServiceAccountKeysCreatePermission is the IAM permission
+	// required to create a service account key.
+	// This permission is required to rotate service account keys.
 	IAMServiceAccountKeysCreatePermission = "iam.serviceAccountKeys.create"
+	// IAMServiceAccountKeysDeletePermission is the IAM permission
+	// required to delete a service account key.
+	// This permission is required to rotate service account keys.
 	IAMServiceAccountKeysDeletePermission = "iam.serviceAccountKeys.delete"
+	// validateServiceAccountKeyTimeout is the timeout for validating
+	// a service account key.
+	validateServiceAccountKeyTimeout = 5 * time.Second
 )
 
 // ServiceAccountPrivateKey represents a decoded PrivateKeyData
@@ -40,6 +53,10 @@ type ServiceAccountPrivateKey struct {
 	ClientX509CertURL       string `json:"client_x509_cert_url"`
 }
 
+// ValidateCredsCallback is called to validate the credentials after rotating the service account key.
+// The callback should return an error if the credentials are invalid.
+type ValidateCredsCallback func(*Config, ...option.ClientOption) error
+
 // RotateServiceAccountKey takes the private key from this credentials config
 // and first creates a new private key and private key id, then deletes the
 // old private key.
@@ -47,7 +64,12 @@ type ServiceAccountPrivateKey struct {
 // If deletion of the old private key is successful, the new private key and
 // private key id are written into the credentials config and nil is returned.
 // On any error, the old credentials are not overwritten.
-func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []string, opts ...option.ClientOption) error {
+func (c *Config) RotateServiceAccountKey(
+	ctx context.Context,
+	permissions []string,
+	validateCredsCallback ValidateCredsCallback,
+	opts ...option.ClientOption,
+) error {
 	if c.PrivateKey == "" {
 		return status.Error(codes.InvalidArgument, "cannot rotate credentials when private key is not set")
 	}
@@ -104,10 +126,10 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri
 	clientOptions = []option.ClientOption{option.WithTokenSource(newCreds.TokenSource)}
 	clientOptions = append(clientOptions, opts...)
 
-	// Validate that the new credentials have the necessary permissions.
-	_, err = newConfig.ValidateIamPermissions(ctx, permissions, clientOptions...)
+	// Validate the new service account key
+	err = newConfig.ValidateServiceAccountKey(ctx, permissions, validateCredsCallback, clientOptions...)
 	if err != nil {
-		return status.Errorf(codes.PermissionDenied, "error testing IAM permissions with rotated service account key: %v", err)
+		return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v", err)
 	}
 
 	iamClient, err = admin.NewIamClient(ctx, clientOptions...)
@@ -128,6 +150,32 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri
 	return nil
 }
 
+// ValidateServiceAccountKey validates the service account key by checking the IAM permissions
+// and calling the validation callback.
+// The function will retry validation until the timeout is reached.
+func (c *Config) ValidateServiceAccountKey(
+	ctx context.Context,
+	permissions []string,
+	validateCredsCallback ValidateCredsCallback,
+	opts ...option.ClientOption) error {
+	var validatePermissionsErr error
+	var validationCallbackErr error
+
+	for start := time.Now(); time.Since(start) < validateServiceAccountKeyTimeout; time.Sleep(200 * time.Millisecond) {
+		_, validatePermissionsErr = c.ValidateIamPermissions(ctx, permissions, opts...)
+
+		if validateCredsCallback != nil {
+			validationCallbackErr = validateCredsCallback(c, opts...)
+		}
+
+		if validatePermissionsErr == nil && validationCallbackErr == nil {
+			return nil
+		}
+	}
+
+	return errors.Join(validatePermissionsErr, validationCallbackErr)
+}
+
 func (c *Config) DeletePrivateKey(ctx context.Context, opts ...option.ClientOption) error {
 	if c.PrivateKey == "" {
 		return status.Error(codes.InvalidArgument, "cannot delete credentials when private key is not set")
@@ -172,18 +220,7 @@ func (c *Config) ValidateIamPermissions(ctx context.Context, permissions []strin
 		return nil, status.Error(codes.InvalidArgument, "permissions are required")
 	}
 
-	creds, err := c.GenerateCredentials(ctx)
-	if err != nil {
-		return nil, status.Errorf(codes.Unauthenticated, "error generating credentials: %v", err)
-	}
-	if creds.TokenSource == nil {
-		return nil, status.Error(codes.Unauthenticated, "error generating credentials: token source is nil")
-	}
-
-	clientOptions := []option.ClientOption{option.WithTokenSource(creds.TokenSource)}
-	clientOptions = append(clientOptions, opts...)
-
-	rmClient, err := resourcemanager.NewProjectsClient(ctx, clientOptions...)
+	rmClient, err := resourcemanager.NewProjectsClient(ctx, opts...)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to create Resource Manager client: %v", err)
 	}

internal/credential/rotate_test.go

@@ -128,8 +128,14 @@ func TestRotateServiceAccountKey(t *testing.T) {
 				IAMServiceAccountKeysCreatePermission,
 				IAMServiceAccountKeysDeletePermission,
 			}
+			validateCredsCallbackCalled := false
 
-			err = tt.config.RotateServiceAccountKey(ctx, permissions, testOptions...)
+			validateCredsCallBack := func(c *Config, opts ...googleOption.ClientOption) error {
+				validateCredsCallbackCalled = true
+				return nil
+			}
+
+			err = tt.config.RotateServiceAccountKey(ctx, permissions, validateCredsCallBack, testOptions...)
 			if tt.expectedError != nil {
 				require.ErrorContains(t, err, tt.expectedError.Error())
 				require.Equal(t, initialClientEmail, tt.config.ClientEmail)
@@ -143,6 +149,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 			require.NotEmpty(t, tt.config.PrivateKeyId)
 			require.NotEqual(t, initialPrivateKey, tt.config.PrivateKey)
 			require.NotEqual(t, initialPrivateKeyId, tt.config.PrivateKeyId)
+			require.True(t, validateCredsCallbackCalled)
 		})
 	}
 }
@@ -183,16 +190,6 @@ func TestValidateIamPermissions(t *testing.T) {
 			wantErr:     true,
 			expectedErr: "permissions are required",
 		},
-		{
-			name: "Failed to Generate Credentials",
-			config: &Config{
-				ProjectId:  "test-project-id",
-				PrivateKey: "test-private-key",
-			},
-			permissions: []string{"permission1", "permission2"},
-			wantErr:     true,
-			expectedErr: "failed to generate credentials",
-		},
 		{
 			name: "Failed to Test IAM Permissions",
 			config: &Config{

internal/credential/state.go

@@ -38,7 +38,11 @@ func NewPersistedState(opt ...Option) (*PersistedState, error) {
 }
 
 // RotateCreds rotates the credentials for the GCP catalog and updates the last rotated time.
-func (s *PersistedState) RotateCreds(ctx context.Context, permissions []string, opts ...option.ClientOption) error {
+func (s *PersistedState) RotateCreds(
+	ctx context.Context,
+	permissions []string,
+	validateCredsCallback ValidateCredsCallback,
+	opts ...option.ClientOption) error {
 	if s.CredentialsConfig == nil {
 		return status.Error(codes.InvalidArgument, "missing credentials config")
 	}
@@ -46,7 +50,7 @@ func (s *PersistedState) RotateCreds(ctx context.Context, permissions []string,
 		return status.Error(codes.InvalidArgument, "cannot rotate non-rotatable credentials")
 	}
 
-	err := s.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, opts...)
+	err := s.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, validateCredsCallback, opts...)
 	if err != nil {
 		return status.Errorf(codes.Internal, "failed to rotate credentials: %v", err)
 	}

plugin/service/host/attributes.go

@@ -108,7 +108,7 @@ func buildListInstancesRequest(attributes *SetAttributes, catalog *CatalogAttrib
 		return nil, fmt.Errorf("error building filters: %w", err)
 	}
 
-	if len(filters) > 0 {
+	if len(filters) > 1 {
 		filters := strings.Join(filters, " AND ")
 		request.Filter = &filters
 	}

plugin/service/host/attributes_test.go

@@ -183,23 +183,6 @@ func TestBuildListInstancesRequest(t *testing.T) {
 			},
 			expectedErrContains: "error building filters",
 		},
-		{
-			name: "valid request with no filters",
-			attributes: &SetAttributes{
-				Filters: []string{},
-			},
-			catalog: &CatalogAttributes{
-				CredentialAttributes: &cred.CredentialAttributes{
-					ProjectId: "test-12345",
-					Zone:      "us-central-1a",
-				},
-			},
-			expected: &computepb.ListInstancesRequest{
-				Project: "test-12345",
-				Zone:    "us-central-1a",
-				Filter:  stringPtr("status = running"),
-			},
-		},
 	}
 
 	for _, tc := range cases {

plugin/service/host/plugin.go

@@ -88,7 +88,17 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo
 	}
 
 	if credState.CredentialsConfig.IsRotatable() && !catalogAttributes.DisableCredentialRotation {
-		if err := credState.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil {
+		state, err := newGCPCatalogPersistedState(
+			append([]gcpCatalogPersistedStateOption{
+				withCredentials(credState),
+			}, p.testCatalogStateOpts...)...,
+		)
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "error setting up persisted state: %s", err)
+		}
+		validateCredsCallback := state.ValidateCredsCallbackFn(ctx, p.testGCPClientOpts...)
+
+		if err := credState.RotateCreds(ctx, permissions, validateCredsCallback, p.testGCPClientOpts...); err != nil {
 			return nil, err
 		}
 	}
@@ -215,7 +225,17 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo
 		// If we're enabling rotation now but didn't before, or have
 		// freshly replaced credentials, we can rotate here.
 		if !newCatalogAttributes.DisableCredentialRotation && credState.CredsLastRotatedTime.IsZero() {
-			if err := credState.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil {
+			state, err := newGCPCatalogPersistedState(
+				append([]gcpCatalogPersistedStateOption{
+					withCredentials(credState),
+				}, p.testCatalogStateOpts...)...,
+			)
+			if err != nil {
+				return nil, status.Errorf(codes.Internal, "error setting up persisted state: %s", err)
+			}
+			validateCredsCallback := state.ValidateCredsCallbackFn(ctx, p.testGCPClientOpts...)
+
+			if err := credState.RotateCreds(ctx, permissions, validateCredsCallback, p.testGCPClientOpts...); err != nil {
 				return nil, err
 			}
 		}

plugin/service/host/state.go

@@ -13,6 +13,7 @@ import (
 	"github.com/googleapis/gax-go/v2"
 	"github.com/hashicorp/boundary-plugin-gcp/internal/credential"
 	pb "github.com/hashicorp/boundary/sdk/pbs/plugin"
+	"google.golang.org/api/iterator"
 	"google.golang.org/api/option"
 	"google.golang.org/protobuf/types/known/structpb"
 )
@@ -99,3 +100,30 @@ func (s *gcpCatalogPersistedState) InstancesClient(
 
 	return client, nil
 }
+
+// validateCredsCallbackFn returns a ValidateCredsCallback function that
+// validates the credentials by listing instances in the project and zone.
+func (s *gcpCatalogPersistedState) ValidateCredsCallbackFn(
+	ctx context.Context,
+	opts ...option.ClientOption,
+) credential.ValidateCredsCallback {
+	return func(config *credential.Config, opts ...option.ClientOption) error {
+		s.CredentialsConfig = config
+		instancesClient, err := s.InstancesClient(ctx, opts...)
+		if err != nil {
+			return fmt.Errorf("error creating instances client: %w", err)
+		}
+
+		it := instancesClient.List(ctx, &computepb.ListInstancesRequest{
+			Project: s.CredentialsConfig.ProjectId,
+			Zone:    s.CredentialsConfig.Zone,
+		})
+
+		_, err = it.Next()
+		if err != nil && err != iterator.Done {
+			return fmt.Errorf("error listing instances: %w", err)
+		}
+
+		return nil
+	}
+}

plugin/service/host/state_test.go

@@ -5,9 +5,11 @@ package host
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
+	computepb "cloud.google.com/go/compute/apiv1/computepb"
 	"github.com/hashicorp/boundary-plugin-gcp/internal/credential"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -87,3 +89,44 @@ func TestInstancesClient(t *testing.T) {
 		assert.Contains(t, err.Error(), "error creating instances client")
 	})
 }
+func TestValidateCredsCallbackFn(t *testing.T) {
+	ctx := context.Background()
+	cred := &credential.PersistedState{
+		CredentialsConfig: &credential.Config{
+			ProjectId: "test-project-id",
+			Zone:      "test-zone",
+		},
+	}
+	state := &gcpCatalogPersistedState{PersistedState: cred}
+
+	t.Run("List instances error", func(t *testing.T) {
+		state.testInstancesAPIFunc = newTestMockInstances(ctx,
+			nil,
+			testMockInstancesWithListInstancesOutput(nil),
+			testMockInstancesWithListInstancesError(errors.New("error listing instances")),
+		)
+
+		callback := state.ValidateCredsCallbackFn(ctx)
+		err := callback(&credential.Config{
+			ProjectId: "test-project-id",
+			Zone:      "test-zone",
+		})
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "error listing instances")
+	})
+
+	t.Run("Success", func(t *testing.T) {
+		state.testInstancesAPIFunc = newTestMockInstances(ctx,
+			nil,
+			testMockInstancesWithListInstancesOutput(&computepb.InstanceList{}),
+			testMockInstancesWithListInstancesError(nil),
+		)
+
+		callback := state.ValidateCredsCallbackFn(ctx)
+		err := callback(&credential.Config{
+			ProjectId: "test-project-id",
+			Zone:      "test-zone",
+		})
+		assert.NoError(t, err)
+	})
+}