diff --git a/CHANGELOG.md b/CHANGELOG.md index fc45fcc2b..6c67eaa15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `configure gcp`: pressing Enter at a `[r]un, [S]kip (default)?` step now skips + instead of creating the service account, granting roles, minting a key or + running `gcloud auth login`; type `r` to run a step (#1613). - `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 diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 47a3747f7..47fa8fa50 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -802,21 +802,21 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj fmt.Println("------------------------------") fmt.Println("This creates a GCP Service Account for CUDly via the IAM API.") fmt.Println() - fmt.Printf("[R]un, [S]kip? (creates service account '%s' via SDK) ", saName) + fmt.Printf("[r]un, [S]kip (default)? (creates service account '%s' via SDK) ", saName) choice, err := reader.ReadString('\n') if err != nil { return "", fmt.Errorf("failed to read service-account choice: %w", err) } switch strings.ToLower(strings.TrimSpace(choice)) { - case "r", "run", "": + case "r", "run": email, createErr := createGCPServiceAccount(ctx, projectID, saName) if createErr != nil { return "", createErr } fmt.Printf("Service account created: %s\n", email) return email, nil - case "s", "skip": + case "s", "skip", "": fmt.Println("Skipping Create Service Account") return promptExistingGCPServiceAccount(reader) default: @@ -897,7 +897,7 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm fmt.Println("Grant the required roles to the service account.") fmt.Println() fmt.Println("Prerequisite: enable recommender.googleapis.com in the service account's project (this step does not enable it).") - fmt.Printf("[R]un, [S]kip? (creates or validates %s with %s and %s with %s, then grants both and roles/compute.viewer to %s on project %s via SDK) ", + fmt.Printf("[r]un, [S]kip (default)? (creates or validates %s with %s and %s with %s, then grants both and roles/compute.viewer to %s on project %s via SDK) ", gcpPurchaserRole.id, gcpPurchaserRole.permission, gcpRecommendationReaderRole.id, gcpRecommendationReaderRole.permission, saEmail, projectID) choice, err := reader.ReadString('\n') @@ -905,7 +905,7 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm return fmt.Errorf("failed to read grant-role choice: %w", err) } switch strings.ToLower(strings.TrimSpace(choice)) { - case "r", "run", "": + case "r", "run": grants := []string{"roles/compute.viewer"} for _, spec := range []gcpCustomRole{gcpPurchaserRole, gcpRecommendationReaderRole} { role, roleErr := ensureGCPCustomRole(ctx, projectID, spec) @@ -920,7 +920,7 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm } fmt.Printf("Role %s granted to %s on project %s.\n", grant, saEmail, projectID) } - case "s", "skip": + case "s", "skip", "": fmt.Println("Skipping Grant IAM Roles") default: fmt.Printf("Unknown option, skipping\n") @@ -944,20 +944,20 @@ func gcpStepCreateKey(reader *bufio.Reader, saEmail string, beginUpload func() c fmt.Println("-------------------------------") fmt.Println("Create a JSON key file for the service account.") fmt.Println() - fmt.Printf("[R]un, [S]kip? (creates key for %s via SDK; the local copy is removed after upload) ", saEmail) + fmt.Printf("[r]un, [S]kip (default)? (creates key for %s via SDK; the local copy is removed after upload) ", saEmail) choice, err := reader.ReadString('\n') if err != nil { return "", "", errors.Join(fmt.Errorf("failed to read create-key choice: %w", err), removeMintedGCPKey(keyFile)) } switch strings.ToLower(strings.TrimSpace(choice)) { - case "r", "run", "": + case "r", "run": keyName, err = createGCPServiceAccountKey(beginUpload(), saEmail, keyFile) if err != nil { return "", "", errors.Join(err, removeMintedGCPKey(keyFile)) } return keyFile, keyName, nil - case "s", "skip": + case "s", "skip", "": fmt.Println("Skipping Create Key") default: fmt.Printf("Unknown option, skipping\n") @@ -1013,7 +1013,7 @@ func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (stri func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() - fmt.Printf("[R]un, [S]kip? ") + fmt.Printf("[r]un, [S]kip (default)? ") choice, err := readTrimmedLine(reader) if err != nil { @@ -1022,9 +1022,9 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd, program stri choice = strings.ToLower(choice) switch choice { - case "r", "run", "": + case "r", "run": return executeGCPCommand(reader, displayCmd, program, args...) - case "s", "skip": + case "s", "skip", "": fmt.Printf("Skipping %s\n", name) return nil default: diff --git a/cmd/configure_gcp_default_skip_test.go b/cmd/configure_gcp_default_skip_test.go new file mode 100644 index 000000000..ff57eeab2 --- /dev/null +++ b/cmd/configure_gcp_default_skip_test.go @@ -0,0 +1,143 @@ +package main + +import ( + "bufio" + "context" + "io" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// stubGCPAPI points every Google API call at a local counter and installs ADC +// fixtures, so no test input can reach real GCP. It returns the count of +// non-token API requests seen. +func stubGCPAPI(t *testing.T) *atomic.Int64 { + t.Helper() + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/token" { + _, _ = w.Write([]byte(`{"access_token":"fixture","token_type":"Bearer","expires_in":3600}`)) + return + } + calls.Add(1) + http.Error(w, `{"error":{"code":400,"message":"fixture"}}`, http.StatusBadRequest) + })) + t.Cleanup(server.Close) + + orig := http.DefaultTransport + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = nil + transport.DialTLSContext = func(ctx context.Context, _, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, "tcp", strings.TrimPrefix(server.URL, "http://")) + } + http.DefaultTransport = transport + t.Cleanup(func() { + http.DefaultTransport = orig + transport.CloseIdleConnections() + }) + + adc := filepath.Join(t.TempDir(), "adc.json") + require.NoError(t, os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"fixture","client_secret":"fixture","refresh_token":"fixture"}`), 0600)) + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) + return &calls +} + +// TestGCPWizardSteps_DefaultIsSkip proves Enter (empty input) skips every +// configure-gcp [r]un/[S]kip step instead of changing cloud identity state. +func TestGCPWizardSteps_DefaultIsSkip(t *testing.T) { + const sa = "cudly-service-account@fixture-project.iam.gserviceaccount.com" + steps := map[string]func(in string) error{ + "create-service-account": func(in string) error { + // A skip continues to the existing-account prompt; supply one so + // the step returns cleanly either way. + _, err := gcpStepCreateServiceAccount(context.Background(), bufio.NewReader(strings.NewReader(in+sa+"\n")), "fixture-project") + return err + }, + "grant-roles": func(in string) error { + return gcpStepGrantRole(context.Background(), bufio.NewReader(strings.NewReader(in)), "fixture-project", sa) + }, + "create-key": func(in string) error { + _, _, err := gcpStepCreateKey(bufio.NewReader(strings.NewReader(in)), sa, context.Background) + return err + }, + } + for name, step := range steps { + for _, tc := range []struct { + label, input string + wantCall bool + }{ + {"empty", "\n", false}, + {"r", "r\n", true}, + {"s", "s\n", false}, + } { + t.Run(name+"/"+tc.label, func(t *testing.T) { + calls := stubGCPAPI(t) + out := captureGCPStdout(t, func() { _ = step(tc.input) }) + if tc.input == "\n" { + assert.Contains(t, out, "Skipping") + assert.NotContains(t, out, "Unknown option") + } + if tc.wantCall { + assert.Positive(t, calls.Load(), "r must run the step") + } else { + assert.Zero(t, calls.Load(), "%q must not call GCP", tc.input) + } + }) + } + } +} + +func TestPromptAndRunGCPCommand_DefaultIsSkip(t *testing.T) { + for _, tc := range []struct { + label, input string + wantRun bool + }{{"empty", "\n", false}, {"r", "r\n", true}, {"s", "s\n", false}} { + t.Run(tc.label, func(t *testing.T) { + marker := filepath.Join(t.TempDir(), "ran") + gcloud := filepath.Join(t.TempDir(), "gcloud") + require.NoError(t, os.WriteFile(gcloud, []byte("#!/bin/sh\ntouch "+marker+"\n"), 0700)) + out := captureGCPStdout(t, func() { + require.NoError(t, promptAndRunGCPCommand(bufio.NewReader(strings.NewReader(tc.input)), "GCP Login", "gcloud auth login", gcloud)) + }) + if tc.input == "\n" { + assert.Contains(t, out, "Skipping GCP Login") + assert.NotContains(t, out, "Unknown option") + } + _, err := os.Stat(marker) + if tc.wantRun { + assert.NoError(t, err, "r must run the command") + } else { + assert.True(t, os.IsNotExist(err), "%q must not run the command", tc.input) + } + }) + } +} + +func captureGCPStdout(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + require.NoError(t, err) + orig := os.Stdout + os.Stdout = w + done := make(chan string) + go func() { + b, _ := io.ReadAll(r) + done <- string(b) + }() + func() { + defer func() { os.Stdout = orig }() + fn() + }() + require.NoError(t, w.Close()) + return <-done +} diff --git a/cmd/configure_gcp_iam_test.go b/cmd/configure_gcp_iam_test.go index aefe0381e..dafcff57f 100644 --- a/cmd/configure_gcp_iam_test.go +++ b/cmd/configure_gcp_iam_test.go @@ -26,7 +26,7 @@ func TestGCPStepGrantRole(t *testing.T) { runGCPIAMFixture(t, scenario) return } - for _, scenario := range []string{"create", "empty", "reuse", "extra-permission", "deleted", "disabled", "wrong-name", "bad-created", "forbidden", "write-error", "skip", "conditional-other", "conditional-member", "already-unconditional", "existing-admin", "coordinator", "coordinator-error", "coordinator-conflict", "coordinator-second-write", "reader-create", "reader-extra-permission", "reader-deleted", "reader-wrong-name"} { + for _, scenario := range []string{"create", "reuse", "extra-permission", "deleted", "disabled", "wrong-name", "bad-created", "forbidden", "write-error", "skip", "conditional-other", "conditional-member", "already-unconditional", "existing-admin", "coordinator", "coordinator-error", "coordinator-conflict", "coordinator-second-write", "reader-create", "reader-extra-permission", "reader-deleted", "reader-wrong-name"} { t.Run(scenario, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -109,7 +109,7 @@ func runGCPIAMFixture(t *testing.T, scenario string) { case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/roles/cudlyCommitmentPurchaser"): roleReads++ switch scenario { - case "create", "empty", "bad-created", "coordinator", "coordinator-conflict": + case "create", "bad-created", "coordinator", "coordinator-conflict": http.Error(w, `{"error":{"code":404,"message":"missing"}}`, http.StatusNotFound) case "forbidden", "coordinator-error": http.Error(w, `{"error":{"code":403,"message":"denied"}}`, http.StatusForbidden) @@ -162,9 +162,6 @@ func runGCPIAMFixture(t *testing.T, scenario string) { require.NoError(t, os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"fixture","client_secret":"fixture","refresh_token":"fixture"}`), 0600)) t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) input := "r\n" - if scenario == "empty" { - input = "\n" - } if scenario == "skip" { input = "s\n" } @@ -213,7 +210,7 @@ func runGCPIAMFixture(t *testing.T, scenario string) { require.Zero(t, keyCalls, "role setup must never mint a key or call unexpected endpoints") wantCreates := 0 switch scenario { - case "create", "empty", "bad-created", "coordinator", "coordinator-conflict", "reader-create": + case "create", "bad-created", "coordinator", "coordinator-conflict", "reader-create": wantCreates = 1 } require.Len(t, creates, wantCreates) diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 216f65724..19f5c299b 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -110,6 +110,8 @@ When `--skip-setup` is not set, the command runs an interactive guided flow: 5. **Grant IAM roles via SDK** - creates or validates the project custom role `cudlyCommitmentPurchaser`, then grants it and `roles/compute.viewer` to the Service Account. 6. **`gcloud iam service-accounts keys create ~/cudly-gcp-key.json`** - downloads a JSON key to your home directory. +Each step prompts `[r]un, [S]kip (default)?`: type `r` to run it. Pressing Enter or typing anything else skips it, so no cloud change happens by accident. + Choosing Skip at the service-account step prompts for the email of an existing service account, which is used for the role grant and key steps. After the guided steps (or with `--skip-setup --credentials-file `), the command reads and validates the JSON file and writes it to the `-GCPCredentials` secret.