fix(cli): delete the minted GCP key when the credentials upload panics (#2233)

The deferred cleanup in uploadGCPCredentialsFile only deleted the remote
key when err was non-nil; during a panic the named err is still nil, so
the local file was removed while the key stayed active in GCP. Track
completion and treat a non-completed nil-error exit as an abort: delete
the key, report the outcome on stderr (the return value is lost) and let
the panic keep propagating.

Also assert that a failed local removal after a successful upload does
not delete the stored key remotely.

Refs #2117

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: cristim

cmd/configure_gcp.go

@@ -191,14 +191,25 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error {
 // removed on every path, and on failure the remote key is deleted via
 // deleteKey so it does not stay active. Cleanup failures are returned.
 func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName, credsFile, mintedKeyName string, deleteKey func(context.Context, string) error) (creds GCPCredentials, err error) {
+	done := false
 	if mintedKeyName != "" {
 		defer func() {
+			// A panic or Goexit leaves the named err nil and done false; the
+			// return value is lost, so report the outcome on stderr. The panic
+			// keeps propagating.
+			aborted := err == nil && !done
+			if aborted {
+				err = errors.New("upload aborted before completion")
+			}
 			if err != nil {
 				err = deleteMintedGCPKeyRemotely(err, mintedKeyName, deleteKey)
 			}
 			if rmErr := removeMintedGCPKey(credsFile); rmErr != nil {
 				err = errors.Join(err, rmErr)
 			}
+			if aborted {
+				fmt.Fprintln(os.Stderr, err)
+			}
 		}()
 	}
 
@@ -211,6 +222,7 @@ func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName
 		return GCPCredentials{}, err
 	}
 
+	done = true
 	return creds, nil
 }
 

cmd/configure_gcp_test.go

@@ -382,6 +382,36 @@ func TestUploadGCPCredentialsFile_RemovalFailureReported(t *testing.T) {
 	_, err := uploadGCPCredentialsFile(context.Background(), NewMockSecretsStore(), "stack", keyFile, keyName, m.DeleteKey)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "failed to remove the minted key file")
+	assert.False(t, m.deleteCalled, "the upload succeeded, so the stored key must stay active in GCP")
+}
+
+func TestUploadGCPCredentialsFile_PanicDeletesRemoteKey(t *testing.T) {
+	keyFile, keyName := mintTestGCPKey(t)
+	store := NewMockSecretsStore()
+	store.updateSecretFunc = func(context.Context, string, string) error { panic("store crashed") }
+	m := &mockGCPKeyProvisioner{}
+
+	assert.Panics(t, func() {
+		_, _ = uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, keyName, m.DeleteKey)
+	})
+	assert.True(t, m.deleteCalled, "the minted key must be deleted from GCP when the upload panics")
+	assert.Equal(t, testMintedKeyName, m.deletedKeyName)
+	assertMintedKeyGone(t, keyFile)
+}
+
+func TestUploadGCPCredentialsFile_PanicReportsFailedRemoteDelete(t *testing.T) {
+	keyFile, keyName := mintTestGCPKey(t)
+	store := NewMockSecretsStore()
+	store.updateSecretFunc = func(context.Context, string, string) error { panic("store crashed") }
+	m := &mockGCPKeyProvisioner{deleteErr: errors.New("delete 500")}
+
+	_, stderr := captureRealStd(t, func() {
+		assert.Panics(t, func() {
+			_, _ = uploadGCPCredentialsFile(context.Background(), store, "stack", keyFile, keyName, m.DeleteKey)
+		})
+	})
+	assert.Contains(t, stderr, gcloudDeleteKeyCommand(testMintedKeyName))
+	assertMintedKeyGone(t, keyFile)
 }
 
 func TestUploadGCPCredentialsFile_OperatorFileKept(t *testing.T) {