diff --git a/CHANGELOG.md b/CHANGELOG.md index 80ccb6dc2..fc45fcc2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `configure gcp` key cleanup: the minted key is now deleted when the upload + panics, a second Ctrl-C aborts the cleanup, the local key file is removed + first, and a possibly orphaned key is reported when `CreateKey` fails + ambiguously (cancellation, timeout, 5xx or network error), with the `gcloud` + commands to list and delete it (#2117). - `--min-savings-pct` now rejects values below 0 or above 100 at validation. Previously a negative value was accepted as no filter and a value above 100 silently dropped every recommendation (#2161). diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 030ceb459..47a3747f7 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -565,6 +565,19 @@ func (k *iamKeyProvisioner) CreateKey(ctx context.Context, saEmail string) (keyN return key.Name, key.PrivateKeyData, nil } +// createKeyError wraps a CreateKey failure. Only a 4xx googleapi error proves +// the server rejected the request; anything else (cancellation, timeout, 5xx, +// network error) may have followed a committed mint, so warn the operator. +func createKeyError(saEmail string, err error) error { + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code >= 400 && gerr.Code < 500 { + return fmt.Errorf("failed to create service account key: %w", err) + } + return fmt.Errorf("failed to create service account key: %w; the key may have been created before the request ended: "+ + "list keys with `gcloud iam service-accounts keys list --iam-account=%s --managed-by=user` and delete any key created just now "+ + "with `gcloud iam service-accounts keys delete --iam-account=%s`", err, saEmail, saEmail) +} + func (k *iamKeyProvisioner) DeleteKey(ctx context.Context, keyName string) error { _, err := k.svc.Projects.ServiceAccounts.Keys.Delete(keyName).Context(ctx).Do() return err @@ -633,7 +646,7 @@ func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, k keyName, privateKeyData, err := p.CreateKey(ctx, saEmail) if err != nil { - return "", fmt.Errorf("failed to create service account key: %w", err) + return "", createKeyError(saEmail, err) } // From here on, any failure must delete the newly minted remote key so it diff --git a/cmd/configure_gcp_signal_test.go b/cmd/configure_gcp_signal_test.go index baebe9660..3009ff113 100644 --- a/cmd/configure_gcp_signal_test.go +++ b/cmd/configure_gcp_signal_test.go @@ -305,6 +305,9 @@ func runGCPInterruptChild(dir string) error { if !errors.Is(err, context.Canceled) { return fmt.Errorf("expected cancellation from the real coordinator, got %w", err) } + if os.Getenv("CUDLY_GCP_SIGNAL_PHASE") == "mint" && !strings.Contains(err.Error(), "may have been created") { + return fmt.Errorf("interrupted CreateKey must warn that a key may exist, got %w", err) + } return nil } diff --git a/cmd/configure_gcp_test.go b/cmd/configure_gcp_test.go index 6b5b89f34..d65bdbea1 100644 --- a/cmd/configure_gcp_test.go +++ b/cmd/configure_gcp_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1" + "google.golang.org/api/googleapi" ) // --- addMemberToPolicyBinding ------------------------------------------------- @@ -219,19 +220,51 @@ func TestWriteServiceAccountKey_RollbackFailureSurfaced(t *testing.T) { // local file is left behind. func TestWriteServiceAccountKey_CreateFailureNoOrphan(t *testing.T) { m := &mockGCPKeyProvisioner{ - createErr: errors.New("iam 403"), + createErr: &googleapi.Error{Code: 403}, } keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") _, err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) require.Error(t, err) assert.Contains(t, err.Error(), "failed to create service account key") + assert.NotContains(t, err.Error(), "may have been created", "a 4xx rejection minted nothing") assert.False(t, m.deleteCalled, "no remote key was minted, so DeleteKey must not be called") _, statErr := os.Stat(keyFile) assert.True(t, os.IsNotExist(statErr), "the reserved key file must be removed when minting fails") } +// TestWriteServiceAccountKey_CreateFailureAmbiguous verifies that only a 4xx +// googleapi error is a definite failure; every other outcome may have minted a key. +func TestWriteServiceAccountKey_CreateFailureAmbiguous(t *testing.T) { + const sa = "sa@proj.iam.gserviceaccount.com" + cases := []struct { + name string + err error + ambiguous bool + }{ + {"403 definite", &googleapi.Error{Code: 403}, false}, + {"500 ambiguous", &googleapi.Error{Code: 500}, true}, + {"canceled ambiguous", context.Canceled, true}, + {"non-googleapi ambiguous", errors.New("connection reset"), true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := &mockGCPKeyProvisioner{createErr: tc.err} + _, err := writeServiceAccountKey(context.Background(), m, sa, filepath.Join(t.TempDir(), "k.json")) + require.Error(t, err) + require.ErrorIs(t, err, tc.err) + if !tc.ambiguous { + assert.NotContains(t, err.Error(), "may have been created") + return + } + assert.Contains(t, err.Error(), "may have been created") + assert.Contains(t, err.Error(), "gcloud iam service-accounts keys list --iam-account="+sa+" --managed-by=user") + assert.Contains(t, err.Error(), "gcloud iam service-accounts keys delete --iam-account="+sa) + }) + } +} + // TestWriteServiceAccountKey_ReserveFailureNoMint verifies that if the // destination file already exists (exclusive-create fails), the remote key is // never minted, so there is nothing to orphan.