diff --git a/docs/rfcs/0001-cli-auth-and-client-provisioning.md b/docs/rfcs/0001-cli-auth-and-client-provisioning.md index 0fdc7c94..745f646e 100644 --- a/docs/rfcs/0001-cli-auth-and-client-provisioning.md +++ b/docs/rfcs/0001-cli-auth-and-client-provisioning.md @@ -45,6 +45,21 @@ > server-side revoke needs the `POST /auth/revoke` endpoint (backend#887, not built) > + a CLI `logout`→call (backend#845 shipped only the `revoke()` primitive) — > §6.3/§7.5/§9/§13/C.6. The earlier "revokes server-side via #845" claim overstated it. +> +> **Rev 8 (2026-07-07)** product deviation on the two zero-prompt inputs (cli#137, +> installer UX v2 — decided by Lukas 2026-07-06): +> - **Name is `-NN`, not the hostname.** The auto-generated name is now +> `-NN` from the signed-in identity (fallback: email local-part), +> numbered per account — the hostname is neither stable nor account-scoped (§6.6/§7.7). +> - **Location is optional, not auto-detected.** With no `--location` the CLI omits +> it and the backend records the client with **no location** (an explicit "not set" +> state — backend#993), rather than auto-detecting a zone at provision time. The +> cloud-metadata auto-detect (`internal/geo`, cli#93) is **removed** — the silent +> path no longer detects, and the backend is the source of truth for valid zones +> (a bad `--location` surfaces as its real create error). §6.7/§7.7. +> +> The §6.6/§6.7/§7.7 bodies below are retained as the original design-of-record; the +> inline **Rev 8** callouts mark where the shipped behavior now differs. ## 0. Decisions settled in this revision @@ -53,7 +68,7 @@ the review. They are now decided; the rest of the doc assumes them. | # | Decision | Choice | |---|---|---| -| D1 | **Setup is silent / auto, not interactive.** | Common path asks **zero questions**: name = sanitized hostname, location = auto-detect, both *surfaced* in progress and correctable with flags — never prompted. (§6.7, §7.7, §8) | +| D1 | **Setup is silent / auto, not interactive.** | Common path asks **zero questions**: name = sanitized hostname, location = auto-detect, both *surfaced* in progress and correctable with flags — never prompted. (§6.7, §7.7, §8) *(Amended Rev 8 / cli#137: name = `-NN`; location optional & omitted when unset — see §6.6/§6.7 callouts.)* | | D2 | **The machine credential is never shown.** | `client create` prints only name + status. The credential is written straight into the cluster secret (mode `0600`) + stored hashed in the backend, and never touches stdout, scrollback, the clipboard, or `~/.tracebloc`. Rotation = delete + recreate. (§7.1, §7.8, §9) | | D3 | **Clients are referred to by a human handle, never a secret or backend id.** | The handle is the per-account-unique namespace **slug** (e.g. `munich-hospital-radiology`); bare `use` / `delete` open an arrow-key picker. The UUID / username / password are never displayed. (§7.1) | @@ -379,6 +394,15 @@ Today there are effectively two names: `first_name` (display) and `namespace` (k8s). Asking for both is redundant; in the silent flow we ask for **neither** (§6.7) — we derive both from the hostname. +> **Amended — Rev 8 / cli#137:** the silent flow no longer derives the name from the +> hostname. It auto-generates `-NN` from the signed-in identity +> (the account — and thus the user's first name — is known at create time; the +> hostname is neither stable nor account-scoped). `NN` is the next free two-digit +> number across the account's existing client names/namespaces, so a second machine +> is `lukas-02` rather than a slug `-2` bump. The derived name is already slug-clean, +> so the derive-once / set-both / freeze namespace rule below is unchanged (name = +> namespace by construction). `--name` / `TRACEBLOC_CLIENT_NAME` still override. + > **All of this is net-new.** Today the backend does *no* namespace processing — it > stores the client-reported `namespace` verbatim (§4.2), with no slug derivation, > no format validation, and no uniqueness. The slug rule below, setting `namespace` @@ -425,6 +449,16 @@ client can be created with no location and `carbon_intensity` defaults to `0` i.e. it silently reads as "carbon-free", quietly corrupting the exact metric tracebloc sells. +> **Amended — Rev 8 / cli#137:** location is no longer auto-detected on the silent +> path. With no `--location` the CLI omits it (`CreateClientRequest.location` is +> `omitempty`) and the backend records the client with **no location** — the explicit +> "not set" state (backend#993), not a silent zero and not a provision-time GeoIP +> guess. The cloud-metadata auto-detect (`internal/geo`, cli#93) is **removed**, not +> parked: the silent path never detects, and the backend is the source of truth for +> valid zones (a bad `--location` surfaces as its real create error). The +> "never block / never fake a zero" principle below stands; "auto-detect silently" +> is the part that's gone. + **Proposal: auto-detect the zone and use it silently; never prompt, never block, never fake a zero.** @@ -614,6 +648,12 @@ default / `unset` rather than block (§6.7). Surface the chosen name + zone in friendly progress (*"Setting up gpu-box-01 in DE"*) — visible but not interactive; correct later with `--name` / `--location` or in the dashboard. +> **Amended — Rev 8 / cli#137:** name = `-NN` from the signed-in identity +> (not the hostname), and location is **optional** — omitted when no `--location` is +> given, leaving the client in the explicit "not set" state (backend#993) rather than +> auto-detected at provision time. See the §6.6 and §6.7 Rev 8 callouts. Both inputs +> stay zero-prompt on the common path; `--name` / `--location` still override. + ### 7.8 If nothing is shown, how does the user manage it later? — **[D2]** **Risk.** "Never show the credential" (D2) is right — but the user still needs to diff --git a/internal/api/client.go b/internal/api/client.go index cb0dff1e..6b182ce1 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -331,9 +331,10 @@ func (c *Client) PollToken(ctx context.Context, deviceCode string) (string, erro // Identity is the signed-in user, from GET /userinfo/. type Identity struct { - Email string `json:"email"` - Type string `json:"type"` - Account string `json:"account"` + Email string `json:"email"` + FirstName string `json:"first_name"` + Type string `json:"type"` + Account string `json:"account"` } // WhoAmI fetches the signed-in user from the backend, authenticating with the @@ -394,8 +395,11 @@ type ProvisionedClient struct { type CreateClientRequest struct { Name string `json:"first_name"` Namespace string `json:"namespace"` - Location string `json:"location"` - Password string `json:"password"` + // Location is optional (cli#137): omitted when the operator gives no --location, + // so the backend records the client with no location rather than a silent + // default (backend#993). EdgeDevice.location is blank=True server-side. + Location string `json:"location,omitempty"` + Password string `json:"password"` // ClusterID anchors the client to this cluster (the kube-system namespace UID) // so create is get-or-create keyed on it (RFC-0001 §7.2 / backend#883). Omitted // when the cluster identity can't be read (dual-mode / legacy → plain mint). diff --git a/internal/cli/auth.go b/internal/cli/auth.go index b30deb71..e59c8fa0 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -109,6 +109,12 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { cfg.CurrentEnv = env prof := cfg.Profile(env) prof.Token = tok + // Clear any identity carried over from a PREVIOUS sign-in on this env + // before the best-effort lookup: if WhoAmI fails here, a re-login as a + // different user on a shared box would otherwise keep the prior user's + // email/first name — and cli#137 would then auto-name the new client + // after the wrong person. Only a successful WhoAmI repopulates these. + prof.Email, prof.FirstName = "", "" // Confirm the freshly-issued token actually authenticates, and // capture the account to show + store. Best-effort: don't fail a // successful sign-in just because this lookup couldn't run. @@ -116,6 +122,7 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { p.Detailf("authorized — confirming the token with the backend …") if id, werr := client.WhoAmI(ctx); werr == nil { prof.Email = id.Email + prof.FirstName = id.FirstName } if err := cfg.Save(); err != nil { return &exitError{code: 1, err: err} diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 8b9b3fbc..61429308 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -290,3 +290,41 @@ func TestAuthStatus_NotSignedIn(t *testing.T) { t.Errorf("got:\n%s", out) } } + +// TestLogin_ClearsStaleIdentityOnWhoAmIFailure (review #3): a re-login as a +// different user must not inherit the previous user's identity if the WhoAmI +// confirmation fails — otherwise cli#137 would auto-name the new client after the +// wrong person. +func TestLogin_ClearsStaleIdentityOnWhoAmIFailure(t *testing.T) { + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/device/code": + _, _ = w.Write([]byte(`{"device_code":"dc","user_code":"WDJB-MJHT","verification_uri":"https://x/activate","expires_in":600,"interval":5}`)) + case "/device/token": + _, _ = w.Write([]byte(`{"token":"bob_tok"}`)) + case "/userinfo/": + w.WriteHeader(http.StatusInternalServerError) // confirmation fails + default: + t.Errorf("unexpected request path %s", r.URL.Path) + } + }) + // Pre-existing session for a DIFFERENT user (Alice) on this env. + if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ + "dev": {Token: "alice_tok", Email: "alice@co", FirstName: "Alice"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "login"); err != nil { + t.Fatalf("login should still succeed when WhoAmI fails: %v", err) + } + + cfg, _ := config.Load() + prof := cfg.Current() + if prof.Token != "bob_tok" { + t.Errorf("token = %q, want the new bob_tok", prof.Token) + } + if prof.FirstName != "" || prof.Email != "" { + t.Errorf("stale identity leaked: FirstName=%q Email=%q (want both cleared)", prof.FirstName, prof.Email) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index 239052df..9f5e4e7f 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -17,7 +17,6 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" - "github.com/tracebloc/cli/internal/geo" "github.com/tracebloc/cli/internal/slug" "github.com/tracebloc/cli/internal/ui" ) @@ -51,17 +50,21 @@ func newClientCreateCmd() *cobra.Command { var yes bool cmd := &cobra.Command{ Use: "create", - Short: "Provision a new client for this machine (--name, --location)", + Short: "Provision a tracebloc client for this machine (auto-named; no flags required)", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runClientCreate(cmd.Context(), printerFor(cmd), clientPrompter(), clientCreateOpts{name: name, location: location, kubeconfigPath: kubeconfigPath, contextOverride: contextOverride, credentialFile: credentialFile, yes: yes}) }, } - cmd.Flags().StringVar(&name, "name", "", - "human-readable client name (shown on your dashboard + carbon reports)") - cmd.Flags().StringVar(&location, "location", "", - "location zone for carbon footprint (e.g. DE); prompted if omitted") + // --name / --location default from their TRACEBLOC_CLIENT_* env vars so an + // unattended run (or the installer) can set them without the flag; an explicit + // flag still wins. Empty --name → auto-generated -NN; empty + // --location → sent as unset (no silent default). + cmd.Flags().StringVar(&name, "name", os.Getenv("TRACEBLOC_CLIENT_NAME"), + "client name (default: $TRACEBLOC_CLIENT_NAME, else auto-generated -NN; shown on your dashboard + carbon reports)") + cmd.Flags().StringVar(&location, "location", os.Getenv("TRACEBLOC_CLIENT_LOCATION"), + "optional location zone for carbon reporting, e.g. DE (default: $TRACEBLOC_CLIENT_LOCATION; omitted if unset)") cmd.Flags().StringVar(&kubeconfigPath, "kubeconfig", "", "path to the kubeconfig for the target cluster (default: $KUBECONFIG, then ~/.kube/config) — read to anchor the client to this cluster") cmd.Flags().StringVar(&contextOverride, "context", "", @@ -136,10 +139,6 @@ func authedClient() (*api.Client, *config.Config, error) { return client, cfg, nil } -// detectZone suggests a location zone (cloud metadata → GeoIP). A seam so tests -// stay hermetic (no network). -var detectZone = geo.Detect - func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clientCreateOpts) (err error) { // Always leave a full provision trace on disk, even on a quiet/headless run // (RFC-0001 §8.5). On any failure, point at the (idempotent) resume command @@ -174,39 +173,15 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien ilog.Logf("authenticated; provisioning against the signed-in account") name, location := opts.name, opts.location - - // Gather inputs first (flags win; prompt only what's missing, and only on a - // TTY), then show one review + confirm — matching the dataset-push flow. - if name == "" { - if pr == nil { - return errMissingFlag("--name") - } - if name, err = pr.Input("Client name", "shown on your dashboard + carbon reports", "", validateNonEmpty); err != nil { - return mapClientErr(err) - } - } - if location == "" { - if pr == nil { - return errMissingFlag("--location") - } - // Auto-detect a suggested zone (cloud metadata → IP geolocation) and - // pre-fill it as the prompt default; the user confirms with Enter or - // overrides. Never silent (it's a prompt), never empty (validateNonEmpty). - suggested := "" - help := "electricityMaps zone for the carbon footprint (e.g. DE)" - if z := detectZone(ctx); z != nil { - suggested = z.Code - help = fmt.Sprintf("detected %s via %s (%s confidence) — Enter to accept, or type your zone", - z.Code, z.Source, z.Confidence) - } - if location, err = pr.Input("Location zone (e.g. DE)", help, suggested, validateNonEmpty); err != nil { - return mapClientErr(err) - } - } - // Reflect the resolved (possibly prompted) name + location back into opts, so - // the failure-path resume command includes them — opts otherwise carries only - // the flags, omitting anything the user typed at a prompt (Bugbot). - opts.name, opts.location = name, location + // cli#137 — the installer path provisions with zero flags and zero prompts: + // • name: auto-generated below (-NN) once the account's client + // list is in hand, so --name is never required; --name / + // TRACEBLOC_CLIENT_NAME still override. + // • location: optional — never prompted, never required. With no --location we + // send nothing (CreateClientRequest.Location is omitempty) and the backend + // records the client with no location rather than a silent default + // (backend#993). The backend is the source of truth for valid zones — a bad + // --location surfaces as its real create error, not a CLI-side guess. // Read the cluster anchor (kube-system UID) so create is get-or-create keyed on // it — re-running on the same cluster adopts the existing client instead of @@ -227,6 +202,46 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien ilog.Logf("client list failed (non-fatal): %v", listErr) } + // Auto-name when neither --name nor TRACEBLOC_CLIENT_NAME was given (cli#137): + // no prompt, ever. -NN, NN = the next free two-digit number across + // the account's existing clients — a second machine on the account lands on + // lukas-02, not a slug -2 bump. The derived name is already slug-clean, so it + // passes through slug.Derive unchanged (display name = namespace = handle). + if name == "" { + // Numbering is only unique if we could actually read the account's clients. + // A list failure would otherwise number against an empty set and mint a + // DETERMINISTIC duplicate (`-01` almost always already exists), whose + // name AND namespace collide with no server-side uniqueness to catch it. So + // fail closed here, exactly like the adopt pre-flight — retry, or name it by + // hand. (A supplied --name still tolerates a list blip: it's best-effort for + // slug-collision avoidance only.) + if listErr != nil { + // A 426 means the CLI is too old — retrying won't help, so surface the + // upgrade signal verbatim instead of framing it as a transient outage. + var ue *api.UpgradeRequiredError + if errors.As(listErr, &ue) { + return &exitError{code: 1, err: ue} + } + return &exitError{code: 1, err: fmt.Errorf( + "couldn't reach the backend to choose a unique client name (%v) — retry, "+ + "or pass --name explicitly", listErr)} + } + // A re-run on a cluster already anchored to a client will ADOPT that client + // (get-or-create by cluster_id), so reuse its existing name — otherwise the + // review/confirm and POST body would describe a freshly-numbered handle + // (lukas-02) that the backend then ignores in favour of the anchored record. + if anchored := anchoredClient(accountClients, clusterID); anchored != nil { + name = anchored.Name + ilog.Logf("reusing anchored client name %q (re-run on this cluster adopts it)", name) + } else { + name = autoClientName(cfg.Current(), accountClients) + ilog.Logf("auto-named client %q (no --name/TRACEBLOC_CLIENT_NAME)", name) + } + } + // Reflect the resolved name + location back into opts so the failure-path resume + // command reproduces them — opts otherwise carries only the raw flags (Bugbot). + opts.name, opts.location = name, location + var pc *api.ProvisionedClient var adopted bool // password is the freshly generated machine credential; set only on the mint @@ -255,9 +270,14 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien // get-or-create). Derive the namespace slug, avoiding collisions with the // account's OTHER clients — skip the one already anchored here (a re-run // adopts it, so its namespace isn't a collision and must not bump the slug). + // Track whether this cluster is already anchored to a client: then + // CreateClient will adopt (HTTP 200, no credential minted or printed), so the + // consent guard below must not block that idempotent re-run. var existing []string + willAdopt := false for _, c := range accountClients { if clusterID != "" && c.ClusterID == clusterID { + willAdopt = true continue } if c.Namespace != "" { @@ -280,6 +300,28 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien p.Hintf("Cancelled.") return nil } + } else if pr == nil && !opts.yes && opts.credentialFile == "" && !willAdopt { + // Non-interactive with no way to confirm AND no --credential-file: a fresh + // MINT here would side-effect silently and print the machine credential to + // stdout (into whatever captured it). Require an explicit signal first — + // --yes to consent, or --credential-file to keep the secret off stdout. + // Skipped when this cluster is already anchored (willAdopt): that re-run + // adopts and prints no credential, so it stays zero-friction. The installer + // passes both flags anyway; this only stops an accidental bare + // `client create` in a pipe / CI from leaking a freshly minted credential. + if listErr != nil { + // The list failed, so willAdopt is unknown — we can't tell a fresh mint + // (would leak a credential) from an idempotent adopt (safe). Fail closed, + // but name the real cause: a retry once the backend is reachable will + // adopt an existing client without any flag. + return &exitError{code: 1, err: fmt.Errorf( + "couldn't read the account's client list to tell whether this cluster is new "+ + "or already registered (%v) — retry when the backend is reachable (a re-run "+ + "adopts an existing client), or pass --yes/--credential-file to provision now", listErr)} + } + return &exitError{code: 1, err: errors.New( + "refusing to provision non-interactively without confirmation — pass --yes to " + + "confirm, and --credential-file to write the credential to a file instead of stdout")} } // The machine credential: the CLI generates the password, the backend stores @@ -664,24 +706,87 @@ func renderClientReview(p *ui.Printer, name, namespace, location, clusterID stri p.Section("Review") p.Field("name", name) p.Field("namespace", namespace) - p.Field("location", location) + // Location is optional (cli#137) — only show the field when one was given, so a + // zero-prompt create doesn't render a blank "location:" line. + if location != "" { + p.Field("location", location) + } if clusterID != "" { p.Field("cluster", clusterID+" (anchors this client — re-runs adopt it)") } } -// errMissingFlag reports a required flag absent in a non-interactive run (no TTY -// to prompt — CI, a pipe, or output redirected). -func errMissingFlag(flag string) error { - return &exitError{code: 1, err: fmt.Errorf("%s is required (non-interactive — no TTY to prompt)", flag)} +// anchoredClient returns the account client already bound to clusterID (the +// kube-system UID), or nil. A non-nil result means a re-run on this cluster will +// adopt that client, so callers should reuse its name rather than mint a new one. +func anchoredClient(clients []api.ProvisionedClient, clusterID string) *api.ProvisionedClient { + if clusterID == "" { + return nil + } + for i := range clients { + if clients[i].ClusterID == clusterID { + return &clients[i] + } + } + return nil } -// validateNonEmpty rejects blank prompt input. -func validateNonEmpty(s string) error { - if strings.TrimSpace(s) == "" { - return errors.New("required") +// autoClientName derives the client name used when neither --name nor +// TRACEBLOC_CLIENT_NAME was given (cli#137): -NN, where base is the +// signed-in user's first name (slugified), falling back to the email local-part, +// then a generic "client". NN is the lowest two-digit number ≥ 1 not already used +// by an existing client's name OR namespace — so a second machine on the account +// lands on lukas-02 rather than the slug package's -2 collision bump, and the +// derived name is guaranteed collision-free through slug.Derive (name = namespace). +func autoClientName(prof *config.Profile, existing []api.ProvisionedClient) string { + base := "" + if prof != nil { + if base = slug.Slugify(prof.FirstName); base == "" { + base = slug.Slugify(emailLocalPart(prof.Email)) + } } - return nil + if base == "" { + base = "client" + } + // Reserve each existing client's handle in BOTH raw and slugified form: a legacy + // client stored with a display name like "Lukas 01" (and a blank/legacy + // namespace) must still block the derived handle "lukas-01", which is what + // slug.Derive would produce for it. + taken := make(map[string]struct{}, 4*len(existing)) + reserve := func(s string) { + if s == "" { + return + } + taken[s] = struct{}{} + taken[slug.Slugify(s)] = struct{}{} + } + for _, c := range existing { + reserve(c.Name) + reserve(c.Namespace) + } + for n := 1; ; n++ { + suffix := fmt.Sprintf("-%02d", n) + // Keep the whole handle within the DNS-1123 label cap so it survives + // slug.Derive unchanged — otherwise a long first_name yields name != namespace + // and reintroduces the exact slug -2 bump this numbering exists to avoid. + b := base + if len(b)+len(suffix) > slug.MaxLabelLength { + b = strings.TrimRight(b[:slug.MaxLabelLength-len(suffix)], "-") + } + cand := b + suffix + if _, clash := taken[cand]; !clash { + return cand + } + } +} + +// emailLocalPart returns the part of an email before '@' (the whole string when +// there's no '@'), the fallback source for an auto-name when first_name is empty. +func emailLocalPart(email string) string { + if i := strings.IndexByte(email, '@'); i >= 0 { + return email[:i] + } + return email } // mapClientErr turns a cancelled interactive prompt into a clean exit. diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index fb3fa8af..55233624 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "os" @@ -16,7 +17,6 @@ import ( "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/config" - "github.com/tracebloc/cli/internal/geo" "github.com/tracebloc/cli/internal/ui" ) @@ -76,13 +76,25 @@ func stubInClusterClient(t *testing.T, lc *cluster.InClusterClient, err error) { t.Cleanup(func() { readInClusterClient = orig }) } -// stubDetect replaces the location auto-detector so command tests stay hermetic -// (no real cloud-metadata / GeoIP probes). -func stubDetect(t *testing.T, z *geo.Zone) { +// signInAs sets the active profile's identity (first name + email) so the cli#137 +// auto-name (-NN) is deterministic in a test. Call after +// withClientBackend, which creates the profile. +func signInAs(t *testing.T, firstName, email string) { t.Helper() - orig := detectZone - detectZone = func(context.Context) *geo.Zone { return z } - t.Cleanup(func() { detectZone = orig }) + // Guard against writing to the developer's real ~/.tracebloc: this helper only + // makes sense once withClientBackend has redirected config to a temp dir. + if os.Getenv("TRACEBLOC_CONFIG_DIR") == "" { + t.Fatal("signInAs: TRACEBLOC_CONFIG_DIR is unset — call withClientBackend first") + } + cfg, err := config.Load() + if err != nil { + t.Fatal(err) + } + p := cfg.Current() + p.FirstName, p.Email = firstName, email + if err := cfg.Save(); err != nil { + t.Fatal(err) + } } func TestClientCreate_Success(t *testing.T) { @@ -286,18 +298,14 @@ func TestClientCreate_Interactive(t *testing.T) { posted = true _ = json.NewDecoder(r.Body).Decode(&body) w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":9,"first_name":"Lab One","username":"u-9","namespace":"lab-one","location":"DE"}`)) + _, _ = w.Write([]byte(`{"id":9,"first_name":"lab-01","username":"u-9","namespace":"lab-01"}`)) } }) - stubDetect(t, nil) // hermetic: no real cloud/GeoIP probes + signInAs(t, "Lab", "lab@example.com") // auto-name base "lab" + // No name/location prompts anymore (cli#137): the name is auto-derived and + // location is optional, so an interactive run only reaches the confirm. confirmYes := true - pr := &fakePrompter{ - answers: map[string]string{ - "Client name": "Lab One", - "Location zone (e.g. DE)": "DE", - }, - confirm: &confirmYes, - } + pr := &fakePrompter{confirm: &confirmYes} var out bytes.Buffer if err := runClientCreate(context.Background(), ui.New(&out), pr, clientCreateOpts{}); err != nil { t.Fatalf("interactive create: %v", err) @@ -305,8 +313,11 @@ func TestClientCreate_Interactive(t *testing.T) { if !posted { t.Fatal("expected a POST after the user confirmed") } - if body.Name != "Lab One" || body.Namespace != "lab-one" || body.Location != "DE" { - t.Errorf("create body = %+v", body) + if body.Name != "lab-01" || body.Namespace != "lab-01" { + t.Errorf("auto-named create body = %+v, want name/namespace lab-01", body) + } + if body.Location != "" { + t.Errorf("location = %q, want empty (no --location given, none sent)", body.Location) } if !strings.Contains(out.String(), "Review") { t.Errorf("expected a review section before the confirm, got:\n%s", out.String()) @@ -321,15 +332,9 @@ func TestClientCreate_InteractiveCancel(t *testing.T) { } _, _ = w.Write([]byte(`[]`)) }) - stubDetect(t, nil) + signInAs(t, "Lab", "lab@example.com") confirmNo := false - pr := &fakePrompter{ - answers: map[string]string{ - "Client name": "Lab Two", - "Location zone (e.g. DE)": "US", - }, - confirm: &confirmNo, - } + pr := &fakePrompter{confirm: &confirmNo} var out bytes.Buffer if err := runClientCreate(context.Background(), ui.New(&out), pr, clientCreateOpts{}); err != nil { t.Fatalf("declining the confirm should be a clean exit, got: %v", err) @@ -680,7 +685,7 @@ func TestClientCreate_ReRunReviewShowsAdoptedNamespace(t *testing.T) { }) stubClusterID(t, "uid-1", nil) confirmYes := true - pr := &fakePrompter{answers: map[string]string{}, confirm: &confirmYes} + pr := &fakePrompter{confirm: &confirmYes} var out bytes.Buffer if err := runClientCreate(context.Background(), ui.New(&out), pr, clientCreateOpts{name: "Lab One", location: "DE"}); err != nil { @@ -691,7 +696,71 @@ func TestClientCreate_ReRunReviewShowsAdoptedNamespace(t *testing.T) { } } -func TestClientCreate_AcceptsDetectedZone(t *testing.T) { +// TestClientCreate_AutoNameNoLocation is the cli#137 headline acceptance case: +// a non-interactive create with NO name and NO location flags still succeeds — +// the name is auto-generated from the signed-in identity and no location is sent. +func TestClientCreate_AutoNameNoLocation(t *testing.T) { + var body api.CreateClientRequest + rawBody := "" + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) // no existing clients on the account + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + b, _ := io.ReadAll(r.Body) + rawBody = string(b) + _ = json.Unmarshal(b, &body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":5,"first_name":"lukas-01","username":"u-5","namespace":"lukas-01"}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + + // pr == nil → non-interactive (the installer path). No flags at all. + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{yes: true}); err != nil { + t.Fatalf("zero-flag non-interactive create should succeed, got: %v", err) + } + if body.Name != "lukas-01" || body.Namespace != "lukas-01" { + t.Errorf("auto-name = %+v, want name/namespace lukas-01", body) + } + // "no location sent" must mean the key is absent from the JSON (omitempty), + // not just an empty string — the backend distinguishes unset from blank. + if strings.Contains(rawBody, "location") { + t.Errorf("request body carried a location key, want it omitted: %s", rawBody) + } +} + +// TestClientCreate_AutoNameNumbering: a second machine on the same account with +// the same first name numbers up (lukas-02), rather than stacking a slug -2 bump. +func TestClientCreate_AutoNameNumbering(t *testing.T) { + var body api.CreateClientRequest + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + // lukas-01 already exists on the account. + _, _ = w.Write([]byte(`[{"id":1,"first_name":"lukas-01","username":"u-1","namespace":"lukas-01"}]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":2,"first_name":"lukas-02","username":"u-2","namespace":"lukas-02"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{yes: true}); err != nil { + t.Fatalf("create: %v", err) + } + if body.Name != "lukas-02" || body.Namespace != "lukas-02" { + t.Errorf("second machine = %+v, want lukas-02 (numbered, not a slug -2 bump)", body) + } +} + +// TestClientCreate_AutoNameEmailFallback: with no first name on the profile, the +// auto-name base falls back to the email local-part. +func TestClientCreate_AutoNameEmailFallback(t *testing.T) { var body api.CreateClientRequest withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { switch { @@ -700,21 +769,307 @@ func TestClientCreate_AcceptsDetectedZone(t *testing.T) { case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": _ = json.NewDecoder(r.Body).Decode(&body) w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":3,"first_name":"Edge","username":"u-3","namespace":"edge","location":"FR"}`)) + _, _ = w.Write([]byte(`{"id":7,"first_name":"jane-doe-01","username":"u-7","namespace":"jane-doe-01"}`)) } }) - // Detector suggests FR; the user accepts it — no scripted answer for the - // location prompt, so the fake returns the pre-filled default. - stubDetect(t, &geo.Zone{Code: "FR", Source: "aws", Confidence: geo.High}) - confirmYes := true - pr := &fakePrompter{ - answers: map[string]string{"Client name": "Edge"}, - confirm: &confirmYes, + signInAs(t, "", "jane.doe@tracebloc.io") // no first name → local-part "jane.doe" → slug "jane-doe" + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{yes: true}); err != nil { + t.Fatalf("create: %v", err) + } + if body.Name != "jane-doe-01" { + t.Errorf("auto-name = %q, want jane-doe-01 (email local-part fallback)", body.Name) + } +} + +// TestClientCreate_FlagsStillHonored: explicit --name/--location are passed +// through verbatim and suppress the auto-name. +func TestClientCreate_FlagsStillHonored(t *testing.T) { + var body api.CreateClientRequest + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":4,"first_name":"lab-one","username":"u-4","namespace":"lab-one","location":"US"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") // would auto-name lukas-01 if not overridden + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{name: "Lab One", location: "US", yes: true}); err != nil { + t.Fatalf("create: %v", err) + } + if body.Name != "Lab One" || body.Location != "US" { + t.Errorf("create body = %+v, want name 'Lab One' location US (flags verbatim)", body) + } +} + +// TestClientCreate_NonInteractiveNeedsConsent (review #1): a bare non-interactive +// run (no TTY, no --yes, no --credential-file) must NOT silently mint and print +// the credential to stdout — it fails closed with guidance before any POST. +func TestClientCreate_NonInteractiveNeedsConsent(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + // pr == nil (non-interactive), yes == false, no credential file. + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{}) + if code := ExitCodeFromError(err); code != 1 { + t.Fatalf("want exit 1, got %d (err=%v)", code, err) + } + if err == nil || !strings.Contains(err.Error(), "refusing to provision non-interactively") { + t.Errorf("want a consent-required error, got: %v", err) + } + if posted { + t.Error("no client should be minted without --yes/--credential-file") + } +} + +// TestClientCreate_NonInteractiveWithYes: --yes alone is sufficient consent for a +// non-interactive mint (the confirm can't run, but the user opted in). +func TestClientCreate_NonInteractiveWithYes(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":5,"first_name":"lukas-01","username":"u-5","namespace":"lukas-01"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}); err != nil { + t.Fatalf("--yes should authorize a non-interactive mint, got: %v", err) + } + if !posted { + t.Error("expected a mint with --yes") + } +} + +// TestClientCreate_AutoNameFailsClosedOnListError (review #2): if the account's +// client list can't be read, auto-naming would number against an empty set and +// mint a deterministic duplicate. It must fail closed instead — never POST. +func TestClientCreate_AutoNameFailsClosedOnListError(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + w.WriteHeader(http.StatusBadGateway) // transient list failure + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}) + if code := ExitCodeFromError(err); code != 1 { + t.Fatalf("want exit 1 on list failure, got %d (err=%v)", code, err) } - if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), pr, clientCreateOpts{}); err != nil { + if err == nil || !strings.Contains(err.Error(), "unique client name") { + t.Errorf("want a 'couldn't pick a unique name' error, got: %v", err) + } + if posted { + t.Error("must not mint (a duplicate) when the client list is unreadable") + } +} + +// TestClientCreate_AutoNameFailsClosedOnListError still lets an explicit --name +// through a list blip (the list is only best-effort for slug-collision avoidance). +func TestClientCreate_ExplicitNameToleratesListError(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + w.WriteHeader(http.StatusBadGateway) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":5,"first_name":"lab","username":"u-5","namespace":"lab"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, + clientCreateOpts{name: "lab", yes: true}); err != nil { + t.Fatalf("explicit --name should tolerate a list blip, got: %v", err) + } + if !posted { + t.Error("expected a mint with an explicit --name despite the list error") + } +} + +// TestClientCreate_AutoNameCapsAt63 (review #4): a very long first name must still +// produce name == namespace within the 63-char DNS label cap — no slug -NN bump. +func TestClientCreate_AutoNameCapsAt63(t *testing.T) { + var body api.CreateClientRequest + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":5,"first_name":"x","username":"u-5","namespace":"x"}`)) + } + }) + signInAs(t, strings.Repeat("a", 70), "long@tracebloc.io") // slugifies to 63 a's + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}); err != nil { t.Fatalf("create: %v", err) } - if body.Location != "FR" { - t.Errorf("location = %q, want FR (detected zone accepted as the default)", body.Location) + if body.Name != body.Namespace { + t.Errorf("name != namespace for a long first name: %q vs %q", body.Name, body.Namespace) + } + if len(body.Name) > 63 { + t.Errorf("name exceeds the 63-char DNS label cap: %d chars (%q)", len(body.Name), body.Name) + } + if !strings.HasSuffix(body.Name, "-01") { + t.Errorf("expected a -01 suffix, got %q", body.Name) + } +} + +// TestClientCreate_AutoNameSurfacesUpgradeRequired (Bugbot #144-A): a 426 while +// listing clients for the auto-name must surface as an upgrade signal, not a +// "retry" reachability error — retrying an outdated CLI never succeeds. +func TestClientCreate_AutoNameSurfacesUpgradeRequired(t *testing.T) { + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/edge-device/" { + w.WriteHeader(http.StatusUpgradeRequired) // 426 + _, _ = w.Write([]byte(`{"error":"upgrade_required","min_version":"1.2.3"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}) + if code := ExitCodeFromError(err); code != 1 { + t.Fatalf("want exit 1, got %d (err=%v)", code, err) + } + if err == nil || !strings.Contains(err.Error(), "too old") { + t.Errorf("want an upgrade-required message, got: %v", err) + } + if err != nil && strings.Contains(err.Error(), "couldn't reach the backend") { + t.Errorf("a 426 must not be framed as a transient reachability error: %v", err) + } +} + +// TestClientCreate_AutoNameReservesSluggedNames (Bugbot #144-B): a legacy client +// whose display name slugifies to the same handle (e.g. "Lukas 01" → lukas-01), +// even with a blank namespace, must reserve that handle so auto-naming skips it. +func TestClientCreate_AutoNameReservesSluggedNames(t *testing.T) { + var body api.CreateClientRequest + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + // Legacy client: raw display name, no namespace stored. + _, _ = w.Write([]byte(`[{"id":1,"first_name":"Lukas 01","username":"u-1","namespace":""}]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":2,"first_name":"lukas-02","username":"u-2","namespace":"lukas-02"}`)) + } + }) + signInAs(t, "Lukas", "lukas@tracebloc.io") + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}); err != nil { + t.Fatalf("create: %v", err) + } + if body.Name != "lukas-02" { + t.Errorf("auto-name = %q, want lukas-02 (lukas-01 reserved by legacy \"Lukas 01\")", body.Name) + } +} + +// TestClientCreate_NonInteractiveAdoptNeedsNoConsent (Bugbot follow-up): the +// non-interactive consent guard must NOT block an idempotent re-run on an +// already-anchored cluster — that path adopts (HTTP 200) and prints no +// credential, so it stays zero-friction without --yes/--credential-file. +func TestClientCreate_NonInteractiveAdoptNeedsNoConsent(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + // This cluster is already anchored to an existing client. + _, _ = w.Write([]byte(`[{"id":8,"first_name":"box","username":"u-8","namespace":"existing","cluster_id":"uid-1"}]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + w.WriteHeader(http.StatusOK) // 200 = adopted + _, _ = w.Write([]byte(`{"id":8,"first_name":"box","username":"u-8","namespace":"existing","cluster_id":"uid-1"}`)) + } + }) + stubClusterID(t, "uid-1", nil) + signInAs(t, "Lukas", "lukas@tracebloc.io") + // pr == nil, no --yes, no --credential-file: a fresh mint would be blocked, but + // this is an adopt, so it must proceed. + var out bytes.Buffer + if err := runClientCreate(context.Background(), ui.New(&out), nil, clientCreateOpts{}); err != nil { + t.Fatalf("non-interactive re-run on an anchored cluster should adopt, got: %v", err) + } + if !posted { + t.Error("expected the adopt POST") + } + if !strings.Contains(out.String(), "already registered") { + t.Errorf("expected an adopt message, got:\n%s", out.String()) + } + if strings.Contains(out.String(), "Machine credential") { + t.Errorf("adopt must not print a credential:\n%s", out.String()) + } +} + +// TestClientCreate_AutoNameReusesAnchoredClientName (Bugbot follow-up): a re-run +// without --name on an already-anchored cluster reuses the anchored client's +// name, not a freshly-numbered handle the backend would ignore on adopt. +func TestClientCreate_AutoNameReusesAnchoredClientName(t *testing.T) { + var body api.CreateClientRequest + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + // lukas-01 is already anchored to this cluster. + _, _ = w.Write([]byte(`[{"id":8,"first_name":"lukas-01","username":"u-8","namespace":"lukas-01","cluster_id":"uid-1"}]`)) + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusOK) // adopt + _, _ = w.Write([]byte(`{"id":8,"first_name":"lukas-01","username":"u-8","namespace":"lukas-01","cluster_id":"uid-1"}`)) + } + }) + stubClusterID(t, "uid-1", nil) + signInAs(t, "Lukas", "lukas@tracebloc.io") // would auto-name lukas-02 (lukas-01 taken) if not reused + if err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{yes: true}); err != nil { + t.Fatalf("create: %v", err) + } + if body.Name != "lukas-01" { + t.Errorf("re-run should reuse the anchored name lukas-01, got %q (a fresh handle the backend would discard)", body.Name) + } +} + +// TestClientCreate_NonInteractiveListFailureExplainsCause (Bugbot follow-up): when +// the consent guard fires because the client list couldn't be read (willAdopt +// unknown), the error must name the real cause and note that a retry adopts — +// not just tell the user to pass --yes. +func TestClientCreate_NonInteractiveListFailureExplainsCause(t *testing.T) { + posted := false + withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/edge-device/": + w.WriteHeader(http.StatusBadGateway) // list fails → willAdopt unknown + case r.Method == http.MethodPost && r.URL.Path == "/edge-device/": + posted = true + } + }) + stubClusterID(t, "uid-1", nil) // a cluster that could well be adopt-only + signInAs(t, "Lukas", "lukas@tracebloc.io") + // --name given so the auto-name fast-fail is skipped and we reach the guard. + err := runClientCreate(context.Background(), ui.New(&bytes.Buffer{}), nil, clientCreateOpts{name: "lab"}) + if got := ExitCodeFromError(err); got != 1 { + t.Fatalf("exit code = %d, want 1", got) + } + if err == nil || !strings.Contains(err.Error(), "couldn't read the account's client list") { + t.Errorf("want a list-failure explanation, got: %v", err) + } + if posted { + t.Error("must not mint when the client list is unreadable") } } diff --git a/internal/cli/verbose_install_test.go b/internal/cli/verbose_install_test.go index 2c1f241c..0a4e5e00 100644 --- a/internal/cli/verbose_install_test.go +++ b/internal/cli/verbose_install_test.go @@ -139,7 +139,7 @@ func TestClientCreate_CancelLogsCancelledNotDone(t *testing.T) { t.Cleanup(func() { readClusterID = origCID }) confirmNo := false - pr := &fakePrompter{answers: map[string]string{}, confirm: &confirmNo} + pr := &fakePrompter{confirm: &confirmNo} var out bytes.Buffer if err := runClientCreate(context.Background(), ui.New(&out), pr, clientCreateOpts{name: "Lab", location: "DE"}); err != nil { @@ -161,17 +161,17 @@ func TestClientCreate_CancelLogsCancelledNotDone(t *testing.T) { } } -// TestClientCreate_ResumeCommandIncludesPromptedValues pins the Bugbot fix: when -// name/location come from interactive prompts (not flags), a failed provision's -// resume command must still include them — opts alone would omit them. -func TestClientCreate_ResumeCommandIncludesPromptedValues(t *testing.T) { +// TestClientCreate_ResumeCommandPinsAutoName pins that a failed provision's resume +// command carries the AUTO-DERIVED name (cli#137), not a bare re-invocation — so a +// retry reproduces the same client name instead of re-deriving/renumbering it. +func TestClientCreate_ResumeCommandPinsAutoName(t *testing.T) { t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) if err := (&config.Config{CurrentEnv: "dev", Profiles: map[string]*config.Profile{ - "dev": {Token: "tok"}, + "dev": {Token: "tok", FirstName: "Lukas"}, }}).Save(); err != nil { t.Fatal(err) } - // list ok; the provision POST 500s after the user confirms. + // list ok; the provision POST 500s. srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet { _, _ = w.Write([]byte(`[]`)) @@ -189,18 +189,13 @@ func TestClientCreate_ResumeCommandIncludesPromptedValues(t *testing.T) { } t.Cleanup(func() { readClusterID = origCID }) - confirmYes := true - pr := &fakePrompter{answers: map[string]string{ - "Client name": "Prompted Lab", - "Location zone (e.g. DE)": "FR", - }, confirm: &confirmYes} var out bytes.Buffer - // No name/location flags — both come from the prompts. - if err := runClientCreate(context.Background(), ui.New(&out), pr, clientCreateOpts{}); err == nil { + // No flags at all — the name is auto-derived to lukas-01. + if err := runClientCreate(context.Background(), ui.New(&out), nil, clientCreateOpts{yes: true}); err == nil { t.Fatal("expected the provision to fail (POST 500)") } - if !strings.Contains(out.String(), "--name 'Prompted Lab' --location FR") { - t.Errorf("resume command should carry the PROMPTED name + location, got:\n%s", out.String()) + if !strings.Contains(out.String(), "--name lukas-01") { + t.Errorf("resume command should pin the auto-derived name, got:\n%s", out.String()) } } diff --git a/internal/config/config.go b/internal/config/config.go index ca43568f..0ce0b81e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -39,6 +39,7 @@ const defaultEnv = "prod" // partially-configured profile stays small and forward-compatible. type Profile struct { Email string `json:"email,omitempty"` // who is signed in (display only) + FirstName string `json:"first_name,omitempty"` // signed-in user's first name; auto-names clients (cli#137) Token string `json:"token,omitempty"` // user token from device login ExpiresAt string `json:"expires_at,omitempty"` // token expiry (RFC 3339), when known ActiveClientID string `json:"active_client_id,omitempty"` // client this machine enrolls as, for THIS env diff --git a/internal/geo/geo.go b/internal/geo/geo.go deleted file mode 100644 index 6e341b95..00000000 --- a/internal/geo/geo.go +++ /dev/null @@ -1,198 +0,0 @@ -// Package geo best-effort detects the host's electricityMaps zone (backend -// ZONE_CHOICES) to pre-fill `client create`'s location prompt: cloud instance -// metadata first (high confidence — the VM reports its own region), then IP -// geolocation (low confidence — flagged). The result is only ever a SUGGESTED -// default the user confirms or overrides; detection failing just means an empty -// default (RFC-0001 location auto-detect, cli#84). -package geo - -import ( - "context" - "io" - "net/http" - "strings" - "time" -) - -// Confidence levels for a detected zone. -const ( - High = "high" // cloud instance metadata — the host runs in this region - Low = "low" // IP geolocation — can be wrong behind VPN / proxy / egress NAT -) - -// Zone is a best-effort location guess. Code is an ISO 3166-1 alpha-2 country -// (always a valid top-level electricityMaps zone); Source names how it was found. -type Zone struct { - Code string - Source string - Confidence string -} - -// Metadata / GeoIP endpoints — package vars so tests can point them at httptest. -var ( - awsIMDSBase = "http://169.254.169.254" - gcpMetaBase = "http://metadata.google.internal" - azureIMDSBase = "http://169.254.169.254" - geoIPURL = "https://www.cloudflare.com/cdn-cgi/trace" -) - -const ( - cloudProbeTimeout = 1500 * time.Millisecond - geoIPTimeout = 3 * time.Second -) - -var ( - // Metadata endpoints are link-local — never via a proxy, and fail fast. - metadataClient = &http.Client{Transport: &http.Transport{Proxy: nil}} - // GeoIP is a public host — honor the corporate proxy like the API client. - geoIPClient = &http.Client{Transport: &http.Transport{Proxy: http.ProxyFromEnvironment}} -) - -// Detect returns a best-effort zone, or nil if nothing could be determined -// (offline, egress-restricted, or bare metal with no usable IP geolocation). It -// never blocks long: the cloud probes share one short deadline and run -// concurrently; GeoIP is a single call only reached when the host isn't a -// recognized cloud region. -func Detect(ctx context.Context) *Zone { - if region, provider := probeCloud(ctx); region != "" { - if cc, ok := regionCountry(region); ok { - return &Zone{Code: cc, Source: provider, Confidence: High} - } - // A cloud host whose region isn't in the map — fall through to GeoIP for - // a VALID zone rather than suggest an unknown string the backend rejects. - } - if cc := probeGeoIP(ctx); cc != "" { - return &Zone{Code: cc, Source: "geoip", Confidence: Low} - } - return nil -} - -// probeCloud runs the three cloud probes concurrently under one deadline and -// returns the first that reports a region (so a real cloud host answers in one -// round-trip instead of waiting through the others' timeouts). -func probeCloud(ctx context.Context) (region, provider string) { - ctx, cancel := context.WithTimeout(ctx, cloudProbeTimeout) - defer cancel() - type res struct{ region, provider string } - // Snapshot the endpoint bases synchronously, before spawning the goroutines, - // so each probe reads a captured local — never the package var. We return on - // the first winner and leave the losers running to their deadline; if they - // read the globals directly, a test's t.Cleanup (which restores those vars) - // races the still-running goroutines (go test -race). - awsBase, gcpBase, azBase := awsIMDSBase, gcpMetaBase, azureIMDSBase - probes := []struct { - name string - fn func(context.Context) string - }{ - {"aws", func(c context.Context) string { return detectAWS(c, awsBase) }}, - {"gcp", func(c context.Context) string { return detectGCP(c, gcpBase) }}, - {"azure", func(c context.Context) string { return detectAzure(c, azBase) }}, - } - ch := make(chan res, len(probes)) - for _, p := range probes { - p := p - go func() { ch <- res{p.fn(ctx), p.name} }() - } - for range probes { - if r := <-ch; r.region != "" { - return r.region, r.provider - } - } - return "", "" -} - -// detectAWS reads the region from EC2 IMDS, preferring IMDSv2 (token) and -// falling back to IMDSv1 (no token) if the token PUT is refused. -func detectAWS(ctx context.Context, base string) string { - var token string - if req, err := http.NewRequestWithContext(ctx, http.MethodPut, base+"/latest/api/token", nil); err == nil { - req.Header.Set("X-aws-ec2-metadata-token-ttl-seconds", "60") - if t, ok := doText(metadataClient, req); ok { - token = t - } - } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, base+"/latest/meta-data/placement/region", nil) - if err != nil { - return "" - } - if token != "" { - req.Header.Set("X-aws-ec2-metadata-token", token) - } - region, _ := doText(metadataClient, req) - return region -} - -// detectGCP reads the instance zone and trims the trailing zone letter to a -// region ("projects/N/zones/europe-west3-c" → "europe-west3"). -func detectGCP(ctx context.Context, base string) string { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, base+"/computeMetadata/v1/instance/zone", nil) - if err != nil { - return "" - } - req.Header.Set("Metadata-Flavor", "Google") - zone, ok := doText(metadataClient, req) - if !ok || zone == "" { - return "" - } - if i := strings.LastIndex(zone, "/"); i >= 0 { - zone = zone[i+1:] - } - if i := strings.LastIndex(zone, "-"); i >= 0 { - zone = zone[:i] - } - return zone -} - -// detectAzure reads the compute location from Azure IMDS (already a region-like -// string, e.g. "germanywestcentral"). -func detectAzure(ctx context.Context, base string) string { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, - base+"/metadata/instance/compute/location?api-version=2021-02-01&format=text", nil) - if err != nil { - return "" - } - req.Header.Set("Metadata", "true") - loc, _ := doText(metadataClient, req) - return loc -} - -// probeGeoIP reads the ISO country from Cloudflare's trace endpoint (the `loc=` -// line) — HTTPS, no API key, returns a 2-letter country code. -func probeGeoIP(ctx context.Context) string { - ctx, cancel := context.WithTimeout(ctx, geoIPTimeout) - defer cancel() - req, err := http.NewRequestWithContext(ctx, http.MethodGet, geoIPURL, nil) - if err != nil { - return "" - } - body, ok := doText(geoIPClient, req) - if !ok { - return "" - } - for _, line := range strings.Split(body, "\n") { - if cc, found := strings.CutPrefix(line, "loc="); found { - cc = strings.TrimSpace(cc) - if len(cc) == 2 { - return strings.ToUpper(cc) - } - } - } - return "" -} - -// doText runs req and returns the trimmed body on a 2xx, else ("", false). -func doText(client *http.Client, req *http.Request) (string, bool) { - resp, err := client.Do(req) - if err != nil { - return "", false - } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return "", false - } - b, err := io.ReadAll(io.LimitReader(resp.Body, 4096)) - if err != nil { - return "", false - } - return strings.TrimSpace(string(b)), true -} diff --git a/internal/geo/geo_test.go b/internal/geo/geo_test.go deleted file mode 100644 index a91c4eff..00000000 --- a/internal/geo/geo_test.go +++ /dev/null @@ -1,150 +0,0 @@ -package geo - -import ( - "context" - "net/http" - "net/http/httptest" - "testing" -) - -// setEndpoints points the metadata / GeoIP endpoints at test servers. -func setEndpoints(t *testing.T, aws, gcp, azure, geoip string) { - t.Helper() - oa, og, oz, ogi := awsIMDSBase, gcpMetaBase, azureIMDSBase, geoIPURL - awsIMDSBase, gcpMetaBase, azureIMDSBase, geoIPURL = aws, gcp, azure, geoip - t.Cleanup(func() { awsIMDSBase, gcpMetaBase, azureIMDSBase, geoIPURL = oa, og, oz, ogi }) -} - -// notFoundServer is a stand-in for an absent provider (every probe 404s fast). -func notFoundServer(t *testing.T) string { - t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - t.Cleanup(srv.Close) - return srv.URL -} - -func TestDetect_AWS(t *testing.T) { - aws := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodPut && r.URL.Path == "/latest/api/token": - _, _ = w.Write([]byte("tok-123")) - case r.Method == http.MethodGet && r.URL.Path == "/latest/meta-data/placement/region": - if r.Header.Get("X-aws-ec2-metadata-token") != "tok-123" { - w.WriteHeader(http.StatusUnauthorized) // enforce the IMDSv2 token - return - } - _, _ = w.Write([]byte("eu-central-1")) - default: - w.WriteHeader(http.StatusNotFound) - } - })) - t.Cleanup(aws.Close) - nf := notFoundServer(t) - setEndpoints(t, aws.URL, nf, nf, nf) - - if z := Detect(context.Background()); z == nil || z.Code != "DE" || z.Source != "aws" || z.Confidence != High { - t.Fatalf("got %+v, want DE/aws/high", z) - } -} - -func TestDetect_GCP(t *testing.T) { - gcp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/computeMetadata/v1/instance/zone" && r.Header.Get("Metadata-Flavor") == "Google" { - _, _ = w.Write([]byte("projects/123/zones/europe-west3-c")) - return - } - w.WriteHeader(http.StatusNotFound) - })) - t.Cleanup(gcp.Close) - nf := notFoundServer(t) - setEndpoints(t, nf, gcp.URL, nf, nf) - - if z := Detect(context.Background()); z == nil || z.Code != "DE" || z.Source != "gcp" || z.Confidence != High { - t.Fatalf("got %+v, want DE/gcp/high", z) - } -} - -func TestDetect_Azure(t *testing.T) { - az := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/metadata/instance/compute/location" && r.Header.Get("Metadata") == "true" { - _, _ = w.Write([]byte("germanywestcentral")) - return - } - w.WriteHeader(http.StatusNotFound) - })) - t.Cleanup(az.Close) - nf := notFoundServer(t) - setEndpoints(t, nf, nf, az.URL, nf) - - if z := Detect(context.Background()); z == nil || z.Code != "DE" || z.Source != "azure" || z.Confidence != High { - t.Fatalf("got %+v, want DE/azure/high", z) - } -} - -func TestDetect_GeoIPFallback(t *testing.T) { - geoip := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("fl=1f\nip=1.2.3.4\nloc=FR\ncolo=CDG\n")) - })) - t.Cleanup(geoip.Close) - nf := notFoundServer(t) - setEndpoints(t, nf, nf, nf, geoip.URL) - - if z := Detect(context.Background()); z == nil || z.Code != "FR" || z.Source != "geoip" || z.Confidence != Low { - t.Fatalf("got %+v, want FR/geoip/low", z) - } -} - -func TestDetect_UnmappedRegionFallsBackToGeoIP(t *testing.T) { - // A cloud region we don't map must NOT be suggested verbatim (the backend - // would reject it) — Detect falls through to GeoIP for a valid zone. - aws := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case "/latest/api/token": - _, _ = w.Write([]byte("t")) - case "/latest/meta-data/placement/region": - _, _ = w.Write([]byte("antarctica-south-1")) - default: - w.WriteHeader(http.StatusNotFound) - } - })) - t.Cleanup(aws.Close) - geoip := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("loc=US\n")) - })) - t.Cleanup(geoip.Close) - nf := notFoundServer(t) - setEndpoints(t, aws.URL, nf, nf, geoip.URL) - - if z := Detect(context.Background()); z == nil || z.Code != "US" || z.Source != "geoip" { - t.Fatalf("got %+v, want US/geoip (unmapped region → GeoIP)", z) - } -} - -func TestDetect_Nothing(t *testing.T) { - nf := notFoundServer(t) - setEndpoints(t, nf, nf, nf, nf) - if z := Detect(context.Background()); z != nil { - t.Fatalf("got %+v, want nil", z) - } -} - -func TestRegionCountry(t *testing.T) { - cases := map[string]string{ - "eu-central-1": "DE", // AWS - "europe-west3": "DE", // GCP - "germanywestcentral": "DE", // Azure - "us-east-1": "US", - "ap-southeast-1": "SG", - "EU-WEST-2": "GB", // case-insensitive - } - for region, want := range cases { - if got, ok := regionCountry(region); !ok || got != want { - t.Errorf("regionCountry(%q) = %q,%v; want %q,true", region, got, ok, want) - } - } - if _, ok := regionCountry("mars-north-1"); ok { - t.Error("unmapped region should return ok=false") - } -} diff --git a/internal/geo/regions.go b/internal/geo/regions.go deleted file mode 100644 index e22588d3..00000000 --- a/internal/geo/regions.go +++ /dev/null @@ -1,67 +0,0 @@ -package geo - -import "strings" - -// regionCountry maps a cloud region / location string to its ISO 3166-1 alpha-2 -// country — always a valid top-level electricityMaps zone (backend ZONE_CHOICES). -// It covers the common AWS / GCP / Azure regions; an unmapped region falls -// through to IP geolocation in Detect, so this need not be exhaustive — extend -// as new regions appear. Country = where the region's datacenters physically sit. -func regionCountry(region string) (string, bool) { - cc, ok := regionToCountry[strings.ToLower(strings.TrimSpace(region))] - return cc, ok -} - -var regionToCountry = map[string]string{ - // ── AWS ── - "us-east-1": "US", "us-east-2": "US", "us-west-1": "US", "us-west-2": "US", - "ca-central-1": "CA", "ca-west-1": "CA", - "eu-west-1": "IE", "eu-west-2": "GB", "eu-west-3": "FR", - "eu-central-1": "DE", "eu-central-2": "CH", - "eu-north-1": "SE", "eu-south-1": "IT", "eu-south-2": "ES", - "ap-south-1": "IN", "ap-south-2": "IN", - "ap-southeast-1": "SG", "ap-southeast-2": "AU", "ap-southeast-3": "ID", "ap-southeast-4": "AU", - "ap-northeast-1": "JP", "ap-northeast-2": "KR", "ap-northeast-3": "JP", - "ap-east-1": "HK", - "sa-east-1": "BR", - "me-south-1": "BH", "me-central-1": "AE", - "af-south-1": "ZA", - "il-central-1": "IL", - - // ── GCP ── - "us-central1": "US", "us-east1": "US", "us-east4": "US", "us-east5": "US", - "us-west1": "US", "us-west2": "US", "us-west3": "US", "us-west4": "US", "us-south1": "US", - "northamerica-northeast1": "CA", "northamerica-northeast2": "CA", - "southamerica-east1": "BR", "southamerica-west1": "CL", - "europe-west1": "BE", "europe-west2": "GB", "europe-west3": "DE", "europe-west4": "NL", - "europe-west6": "CH", "europe-west8": "IT", "europe-west9": "FR", "europe-west10": "DE", "europe-west12": "IT", - "europe-central2": "PL", "europe-north1": "FI", "europe-southwest1": "ES", - "asia-east1": "TW", "asia-east2": "HK", - "asia-northeast1": "JP", "asia-northeast2": "JP", "asia-northeast3": "KR", - "asia-south1": "IN", "asia-south2": "IN", - "asia-southeast1": "SG", "asia-southeast2": "ID", - "australia-southeast1": "AU", "australia-southeast2": "AU", - "me-west1": "IL", "me-central1": "QA", "me-central2": "SA", - - // ── Azure ── - "eastus": "US", "eastus2": "US", "centralus": "US", "northcentralus": "US", - "southcentralus": "US", "westus": "US", "westus2": "US", "westus3": "US", "westcentralus": "US", - "canadacentral": "CA", "canadaeast": "CA", - "brazilsouth": "BR", "brazilsoutheast": "BR", - "northeurope": "IE", "westeurope": "NL", - "uksouth": "GB", "ukwest": "GB", - "francecentral": "FR", "francesouth": "FR", - "germanywestcentral": "DE", "germanynorth": "DE", - "switzerlandnorth": "CH", "switzerlandwest": "CH", - "norwayeast": "NO", "norwaywest": "NO", - "swedencentral": "SE", "polandcentral": "PL", "italynorth": "IT", "spaincentral": "ES", - "eastasia": "HK", "southeastasia": "SG", - "japaneast": "JP", "japanwest": "JP", - "koreacentral": "KR", "koreasouth": "KR", - "centralindia": "IN", "southindia": "IN", "westindia": "IN", - "australiaeast": "AU", "australiasoutheast": "AU", "australiacentral": "AU", - "uaenorth": "AE", "uaecentral": "AE", - "qatarcentral": "QA", - "southafricanorth": "ZA", - "israelcentral": "IL", -}