From d37e53f8467bdc7d5cc9aa056918bbdd56c0b82d Mon Sep 17 00:00:00 2001 From: adityawalvekar Date: Thu, 26 Mar 2026 13:26:32 +0530 Subject: [PATCH] Fix: handle failure of key deletion and rollback --- internal/credential/rotate.go | 15 +++++++++++++-- internal/credential/rotate_test.go | 29 ++++++++++++++++++++++++++--- internal/credential/testing.go | 12 ++++++++++-- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/internal/credential/rotate.go b/internal/credential/rotate.go index 17a1ef2..f3f6120 100644 --- a/internal/credential/rotate.go +++ b/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 diff --git a/internal/credential/rotate_test.go b/internal/credential/rotate_test.go index 2e792a8..fd08233 100644 --- a/internal/credential/rotate_test.go +++ b/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, }, diff --git a/internal/credential/testing.go b/internal/credential/testing.go index 10e1475..c523e45 100644 --- a/internal/credential/testing.go +++ b/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 }