Address review comments

internal/credential/rotate.go

@@ -60,6 +60,8 @@ type ValidateCredsCallback func(*Config, ...option.ClientOption) error
 //
 // 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.
+// If validation fails or deleting the old private key fails, the function
+// attempts to delete the newly created key as a rollback operation.
 // On any error, the old credentials are not overwritten.
 func (c *Config) RotateServiceAccountKey(
 	ctx context.Context,
@@ -126,23 +128,30 @@ func (c *Config) RotateServiceAccountKey(
 	// Validate the new service account key
 	err = newConfig.ValidateServiceAccountKey(ctx, permissions, validateCredsCallback, clientOptions...)
 	if err != nil {
-		return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v", err)
+		// Roll back the new key if validation fails.
+		rollbackErr := iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
+			Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId),
+		})
+		if rollbackErr != nil {
+			return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v; error rolling back new rotated service account key: %v", err, rollbackErr)
+		}
+
+		return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v; successfully rolled back new rotated service account key", err)
 	}
 
-	iamClient, err = admin.NewIamClient(ctx, clientOptions...)
+	newClient, err := admin.NewIamClient(ctx, clientOptions...)
 	if err != nil {
 		return status.Errorf(codes.Internal, "error creating IAM client with rotated service account key: %v", err)
 	}
 
-	oldKeyName := fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId)
-	err = iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
-		Name: oldKeyName,
+	err = newClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
+		Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId),
 	})
 	// If deletion of the old key fails, attempt to delete the new key to avoid leaving a dangling key, then return an error.
 	if err != nil {
-		newKeyName := fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId)
+		// delete the new key using old client since the new client may not have permissions that the old client has.
 		rollbackErr := iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
-			Name: newKeyName,
+			Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId),
 		})
 		// If rollback also fails, return an error indicating both the failure to delete the old key and the failure to roll back the new key.
 		if rollbackErr != nil {

internal/credential/rotate_test.go

@@ -72,9 +72,44 @@ func TestRotateServiceAccountKey(t *testing.T) {
 			},
 			setup: func() {
 				testIAMAdminServer.testCreateServiceAccountKeyError = errors.New("create key error")
+				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyErrors = nil
+				testResourceServer.testIamPermissionsError = nil
 			},
 			expectedError: errors.New("create key error"),
 		},
+		{
+			name: "ValidateServiceAccountKey fails and rollback succeeds",
+			config: &Config{
+				PrivateKey:                       "some-private-key",
+				PrivateKeyId:                     "some-private-key-id",
+				ClientEmail:                      "client@example.com",
+				validateServiceAccountKeyTimeout: testValidateTimeout,
+			},
+			setup: func() {
+				testIAMAdminServer.testCreateServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyErrors = []error{nil}
+				testResourceServer.testIamPermissionsError = errors.New("iam permissions error")
+			},
+			expectedError: errors.New("successfully rolled back new rotated service account key"),
+		},
+		{
+			name: "ValidateServiceAccountKey fails and rollback fails",
+			config: &Config{
+				PrivateKey:                       "some-private-key",
+				PrivateKeyId:                     "some-private-key-id",
+				ClientEmail:                      "client@example.com",
+				validateServiceAccountKeyTimeout: testValidateTimeout,
+			},
+			setup: func() {
+				testIAMAdminServer.testCreateServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyErrors = []error{errors.New("rollback key error")}
+				testResourceServer.testIamPermissionsError = errors.New("iam permissions error")
+			},
+			expectedError: errors.New("error rolling back new rotated service account key"),
+		},
 		{
 			name: "DeleteServiceAccountKey old key fails and rollback fails",
 			config: &Config{
@@ -90,6 +125,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 					errors.New("delete key error"),
 					errors.New("rollback key error"),
 				}
+				testResourceServer.testIamPermissionsError = nil
 			},
 			expectedError: errors.New("error rolling back new rotated service account key"),
 		},
@@ -108,6 +144,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 					errors.New("delete key error"),
 					nil,
 				}
+				testResourceServer.testIamPermissionsError = nil
 			},
 			expectedError: errors.New("successfully rolled back new rotated service account key"),
 		},
@@ -123,6 +160,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 				testIAMAdminServer.testCreateServiceAccountKeyError = nil
 				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
 				testIAMAdminServer.testDeleteServiceAccountKeyErrors = nil
+				testResourceServer.testIamPermissionsError = nil
 			},
 			expectedError: nil,
 		},