diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 5401e00f7..030ceb459 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -16,6 +16,7 @@ import ( "regexp" "slices" "strings" + "sync/atomic" "syscall" "time" "unicode" @@ -156,15 +157,30 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { } store := NewAWSSecretsStore(secretsmanager.NewFromConfig(cfg)) - var stop context.CancelFunc - defer func() { - if stop != nil { - stop() + var finished atomic.Bool + var release context.CancelFunc + finish := func() { + // Store before release(): the cancel wakes the goroutine, which reads it. + finished.Store(true) + if release != nil { + release() } - }() + } + defer finish() beginUpload := func() context.Context { - if stop == nil { - ctx, stop = signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + if release == nil { + sigCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + release = stop + ctx = sigCtx + go func() { + <-sigCtx.Done() + // Restore default signal handling so a second Ctrl-C aborts + // the cleanup; stop() also cancels sigCtx, hence the flag. + stop() + if !finished.Load() { + fmt.Fprintln(os.Stderr, "Interrupted: finishing cleanup (up to 60s). Press Ctrl-C again to abort immediately.") + } + }() } return ctx } @@ -178,7 +194,7 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { return err } - stop() + finish() if mintedKeyName != "" { fmt.Println("Removed the local copy of the minted key.") } @@ -201,10 +217,13 @@ func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName if aborted { err = errors.New("upload aborted before completion") } + // Remove the local copy first: a second Ctrl-C during the slow + // remote delete kills the process and must not leave it on disk. + rmErr := removeMintedGCPKey(credsFile) if err != nil { err = deleteMintedGCPKeyRemotely(err, mintedKeyName, deleteKey) } - if rmErr := removeMintedGCPKey(credsFile); rmErr != nil { + if rmErr != nil { err = errors.Join(err, rmErr) } if aborted { @@ -231,6 +250,7 @@ func uploadGCPCredentialsFile(ctx context.Context, store SecretsStore, stackName func deleteMintedGCPKeyRemotely(cause error, keyName string, deleteKey func(context.Context, string) error) error { ctx, cancel := context.WithTimeout(context.Background(), gcpSDKCallTimeout) defer cancel() + fmt.Fprintf(os.Stderr, "Deleting minted key; if aborted, delete it with: %s\n", gcloudDeleteKeyCommand(keyName)) if err := deleteKey(ctx, keyName); err != nil { return fmt.Errorf("%w; the minted key is still active in GCP and deleting it failed (%w); delete it with: %s", cause, err, gcloudDeleteKeyCommand(keyName)) } diff --git a/cmd/configure_gcp_signal_test.go b/cmd/configure_gcp_signal_test.go index 98bcb4b52..baebe9660 100644 --- a/cmd/configure_gcp_signal_test.go +++ b/cmd/configure_gcp_signal_test.go @@ -95,6 +95,57 @@ func TestRunConfigureGCP_InterruptAtPromptExits(t *testing.T) { require.NoFileExists(t, filepath.Join(dir, "minted")) } +func TestRunConfigureGCP_SecondInterruptAbortsSlowDelete(t *testing.T) { + child, dir, stdout, stderr := startGCPInterruptChild(t, "slowdelete") + defer stdout.Close() + requireMarker(t, child, stderr, "minted key.json with fixture secret", func() bool { + matches, err := filepath.Glob(filepath.Join(dir, "cudly-gcp-key-*", "key.json")) + if err != nil || len(matches) != 1 { + return false + } + data, err := os.ReadFile(matches[0]) + return err == nil && bytes.Contains(data, []byte("fixture-secret")) + }) + require.NoError(t, child.Process.Signal(os.Interrupt)) + // Key the second signal on the message the handler prints after it + // restores default signal handling; an earlier one would be swallowed. + requireMarker(t, child, stderr, "Interrupted message", func() bool { + return strings.Contains(stderr.String(), "Interrupted: finishing cleanup") + }) + requireMarker(t, child, stderr, "deleting marker", func() bool { + _, err := os.Stat(filepath.Join(dir, "deleting")) + return err == nil + }) + require.Contains(t, stderr.String(), "if aborted, delete it with: gcloud iam service-accounts keys delete fixture-key --iam-account=cudly-service-account@fixture-project.iam.gserviceaccount.com") + matches, err := filepath.Glob(filepath.Join(dir, "cudly-gcp-key-*")) + require.NoError(t, err) + require.Empty(t, matches, "local key must be removed before the slow remote delete") + require.NoError(t, child.Process.Signal(os.Interrupt)) + done := make(chan error, 1) + go func() { done <- child.Wait() }() + select { + case err := <-done: + require.Error(t, err, "child stderr: %s", stderr.String()) + status, ok := child.ProcessState.Sys().(syscall.WaitStatus) + require.True(t, ok) + require.True(t, status.Signaled()) + require.Equal(t, syscall.SIGINT, status.Signal()) + case <-time.After(5 * time.Second): + t.Fatalf("second SIGINT did not abort the child\nchild stderr: %q", stderr.String()) + } +} + +func TestRunConfigureGCP_SuccessPrintsNoInterruptText(t *testing.T) { + child, _, stdout, stderr := startGCPInterruptChild(t, "success") + defer stdout.Close() + out, err := io.ReadAll(stdout) + require.NoError(t, err) + require.NoError(t, child.Wait(), "child stderr: %s", stderr.String()) + require.Contains(t, string(out), "GCP configuration complete!") + require.NotContains(t, stderr.String(), "Interrupted") + require.NotContains(t, stderr.String(), "Deleting minted key") +} + // syncBuffer lets the test read child stderr while exec is still copying it. type syncBuffer struct { mu sync.Mutex @@ -187,6 +238,14 @@ func runGCPInterruptChild(dir string) error { case r.URL.Path == "/token": _, _ = io.WriteString(w, `{"access_token":"fixture","token_type":"Bearer","expires_in":3600}`) case r.Method == http.MethodDelete: + if os.Getenv("CUDLY_GCP_SIGNAL_PHASE") == "slowdelete" { + if err := os.WriteFile(filepath.Join(dir, "deleting"), nil, 0600); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + <-r.Context().Done() + return + } if err := os.WriteFile(filepath.Join(dir, "deleted"), []byte(r.URL.Path), 0600); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -212,7 +271,13 @@ func runGCPInterruptChild(dir string) error { material := base64.StdEncoding.EncodeToString([]byte(`{"type":"service_account","project_id":"fixture-project","client_email":"cudly-service-account@fixture-project.iam.gserviceaccount.com","private_key":"fixture-secret"}`)) _, _ = fmt.Fprintf(w, `{"name":"projects/fixture-project/serviceAccounts/cudly-service-account@fixture-project.iam.gserviceaccount.com/keys/fixture-key","privateKeyData":%q}`, material) case r.Header.Get("X-Amz-Target") == "secretsmanager.ListSecrets": + if os.Getenv("CUDLY_GCP_SIGNAL_PHASE") == "success" { + _, _ = io.WriteString(w, `{"SecretList":[]}`) + return + } <-r.Context().Done() + case os.Getenv("CUDLY_GCP_SIGNAL_PHASE") == "success" && r.Header.Get("X-Amz-Target") != "": + _, _ = io.WriteString(w, `{}`) default: http.Error(w, "unexpected fixture request", http.StatusBadRequest) } @@ -228,6 +293,14 @@ func runGCPInterruptChild(dir string) error { return err } gcpOpts = GCPConfigOptions{StackName: "fixture-stack"} + if os.Getenv("CUDLY_GCP_SIGNAL_PHASE") == "success" { + credsFile := filepath.Join(dir, "operator-key.json") + if err := os.WriteFile(credsFile, []byte(`{"type":"service_account","project_id":"fixture-project","client_email":"cudly-service-account@fixture-project.iam.gserviceaccount.com","private_key":"fixture-secret"}`), 0600); err != nil { + return err + } + gcpOpts.CredentialsFile = credsFile + return runConfigureGCP(nil, nil) + } err := runConfigureGCP(nil, nil) if !errors.Is(err, context.Canceled) { return fmt.Errorf("expected cancellation from the real coordinator, got %w", err)