Fix: handle failure of key deletion and rollback

internal/credential/rotate.go

@@ -134,11 +134,22 @@ func (c *Config) RotateServiceAccountKey(
 		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: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId),
+		Name: oldKeyName,
 	})
+	// 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 {
-		return status.Errorf(codes.Internal, "error deleting service account key: %v", err)
+		newKeyName := fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId)
+		rollbackErr := iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
+			Name: newKeyName,
+		})
+		// 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 {
+			return status.Errorf(codes.Internal, "error deleting service account key: %v; error rolling back new rotated service account key: %v", err, rollbackErr)
+		}
+
+		return status.Errorf(codes.Internal, "error deleting service account key: %v; successfully rolled back new rotated service account key", err)
 	}
 
 	c.PrivateKey = newConfig.PrivateKey

internal/credential/rotate_test.go

@@ -76,7 +76,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 			expectedError: errors.New("create key error"),
 		},
 		{
-			name: "DeleteServiceAccountKey returns error",
+			name: "DeleteServiceAccountKey old key fails and rollback fails",
 			config: &Config{
 				PrivateKey:                       "some-private-key",
 				PrivateKeyId:                     "some-private-key-id",
@@ -85,9 +85,31 @@ func TestRotateServiceAccountKey(t *testing.T) {
 			},
 			setup: func() {
 				testIAMAdminServer.testCreateServiceAccountKeyError = nil
-				testIAMAdminServer.testDeleteServiceAccountKeyError = errors.New("delete key error")
+				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyErrors = []error{
+					errors.New("delete key error"),
+					errors.New("rollback key error"),
+				}
 			},
-			expectedError: errors.New("delete key error"),
+			expectedError: errors.New("error rolling back new rotated service account key"),
+		},
+		{
+			name: "DeleteServiceAccountKey old key 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{
+					errors.New("delete key error"),
+					nil,
+				}
+			},
+			expectedError: errors.New("successfully rolled back new rotated service account key"),
 		},
 		{
 			name: "Successful rotation",
@@ -100,6 +122,7 @@ func TestRotateServiceAccountKey(t *testing.T) {
 			setup: func() {
 				testIAMAdminServer.testCreateServiceAccountKeyError = nil
 				testIAMAdminServer.testDeleteServiceAccountKeyError = nil
+				testIAMAdminServer.testDeleteServiceAccountKeyErrors = nil
 			},
 			expectedError: nil,
 		},

internal/credential/testing.go

@@ -18,8 +18,9 @@ import (
 
 type testIAMAdminServer struct {
 	adminpb.UnimplementedIAMServer
-	testCreateServiceAccountKeyError error
-	testDeleteServiceAccountKeyError error
+	testCreateServiceAccountKeyError  error
+	testDeleteServiceAccountKeyError  error
+	testDeleteServiceAccountKeyErrors []error
 }
 
 func NewTestIAMAdminServer(createServiceAccountKeyError error, deleteServiceAccountKeyError error) *testIAMAdminServer {
@@ -46,6 +47,13 @@ func (f *testIAMAdminServer) CreateServiceAccountKey(ctx context.Context, req *a
 }
 
 func (f *testIAMAdminServer) DeleteServiceAccountKey(ctx context.Context, req *adminpb.DeleteServiceAccountKeyRequest) (*emptypb.Empty, error) {
+	// Use queued delete results so tests can separately control deleting the old key and deleting the new key during rollback.
+	if len(f.testDeleteServiceAccountKeyErrors) > 0 {
+		err := f.testDeleteServiceAccountKeyErrors[0]
+		f.testDeleteServiceAccountKeyErrors = f.testDeleteServiceAccountKeyErrors[1:]
+		return &emptypb.Empty{}, err
+	}
+
 	return &emptypb.Empty{}, f.testDeleteServiceAccountKeyError
 }