From 6dfcba4d6e0b19f299791bdfe92fbff494896465 Mon Sep 17 00:00:00 2001 From: Akshay Singla Date: Sun, 7 Jun 2026 01:02:12 +0000 Subject: [PATCH] =?UTF-8?q?lakebox:=20bugfixes=20=E2=80=94=20start.go=20he?= =?UTF-8?q?lp=20text,=20create=20default-clobber,=20keyhash=20trailing-new?= =?UTF-8?q?line,=20plus=20UX?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness fixes (BLOCKERs from a final ship-readiness audit) and several smaller UX cleanups, bundled because each is small and they all sit on the same `register → ssh` path that gets exercised together. BLOCKERs: 1. `start.go` `Long:` said "blocks until the sandbox reaches Running (or up to 5 minutes)" while the actual `startWaitTimeout` constant is 10 minutes. Documented bound mismatched the enforced bound — fix the text. 2. `create.go` was using `if _, err := api.get(currentDefault); err != nil` to decide whether to overwrite the saved default. Any non-404 error (transient 5xx, network blip, rate limit) silently clobbered the user's default. Every sibling (`delete`, `default`, `ssh`) already uses `errors.Is(err, apierr.ErrNotFound)` for this — `create` now matches. 3. `keyHash` included a trailing newline in its sha256 input for `.pub` files without an OpenSSH comment. Locally-generated keys are fine (`ssh-keygen -C "lakebox"` always adds a comment), but a user-supplied key without `-C` would produce a hash that doesn't match the server's, making `verifyKeyRegistered` falsely report "key not registered". Fix: `strings.TrimSpace` the input first. New test cases pin the behavior for `\n` and surrounding whitespace. UX fixes: - `status.go` displayed an `fqdn` field even though `api.go` documents FQDN as the manager's internal routing host — drop from non-JSON output (still present in `-o json` for completeness). - `status.go` wrapped 404 errors as "failed to get lakebox X: ..." while `delete`, `default`, and `ssh` all return the friendlier "no lakebox named X — `databricks lakebox list` shows available IDs". Add the same branch. - `ui.go` `warn()` used cyan, the same color as `ok()` and the spinner ✓ marker — so a warning was visually indistinguishable from success. Switch to yellow (already in the palette via `cmdio.Yellow`). - `delete.go` returned `errors.New("aborted")` on user-cancel, which surfaced as `Error: aborted` with a non-zero exit. Print "Cancelled." and return nil instead. - Inconsistent capitalization across `warn()` messages (some sentence case, some lowercase) — standardize on sentence case to match the rest of the repo. Co-authored-by: Isaac --- cmd/lakebox/create.go | 9 +++++++-- cmd/lakebox/delete.go | 3 ++- cmd/lakebox/keyhash.go | 4 ++++ cmd/lakebox/keyhash_test.go | 13 +++++++++++++ cmd/lakebox/register.go | 2 +- cmd/lakebox/ssh.go | 6 +++--- cmd/lakebox/start.go | 2 +- cmd/lakebox/status.go | 11 ++++++++--- cmd/lakebox/ui.go | 5 +++-- 9 files changed, 42 insertions(+), 13 deletions(-) diff --git a/cmd/lakebox/create.go b/cmd/lakebox/create.go index c6e5b7e1be0..84034214e5d 100644 --- a/cmd/lakebox/create.go +++ b/cmd/lakebox/create.go @@ -2,11 +2,13 @@ package lakebox import ( "encoding/json" + "errors" "fmt" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/spf13/cobra" ) @@ -68,10 +70,13 @@ Examples: _ = setGatewayHost(ctx, profile, result.GatewayHost) _ = upsertSandbox(ctx, profile, result.SandboxID, name) + // Only clobber an existing default if it's actually gone + // (404). Transient errors (5xx, network blip, rate limit) + // must not silently overwrite the user's chosen default. currentDefault := getDefault(ctx, profile) shouldSetDefault := currentDefault == "" - if !shouldSetDefault && currentDefault != "" { - if _, err := api.get(ctx, currentDefault); err != nil { + if !shouldSetDefault { + if _, err := api.get(ctx, currentDefault); errors.Is(err, apierr.ErrNotFound) { shouldSetDefault = true } } diff --git a/cmd/lakebox/delete.go b/cmd/lakebox/delete.go index f43c8eb7357..91800700b7b 100644 --- a/cmd/lakebox/delete.go +++ b/cmd/lakebox/delete.go @@ -76,7 +76,8 @@ Examples: return err } if !confirmed { - return errors.New("aborted") + cmdio.LogString(ctx, "Cancelled.") + return nil } } diff --git a/cmd/lakebox/keyhash.go b/cmd/lakebox/keyhash.go index 823eb1e1682..238847fe447 100644 --- a/cmd/lakebox/keyhash.go +++ b/cmd/lakebox/keyhash.go @@ -3,6 +3,7 @@ package lakebox import ( "crypto/sha256" "encoding/hex" + "strings" ) // keyHash returns the identifier the lakebox SSH-keys API assigns to a @@ -10,7 +11,10 @@ import ( // the first 16 bytes and hex-encoded; the OpenSSH comment (anything after // the second whitespace-separated token) is stripped before hashing, so // registering the same key under different comments yields the same hash. +// Leading and trailing whitespace are trimmed first — `.pub` files end +// with a newline that would otherwise be hashed in for comment-less keys. func keyHash(publicKey string) string { + publicKey = strings.TrimSpace(publicKey) end := len(publicKey) spaces := 0 for i, c := range publicKey { diff --git a/cmd/lakebox/keyhash_test.go b/cmd/lakebox/keyhash_test.go index 638f1d8f34c..32d98b310b4 100644 --- a/cmd/lakebox/keyhash_test.go +++ b/cmd/lakebox/keyhash_test.go @@ -46,6 +46,19 @@ func TestKeyHash(t *testing.T) { input: "", want: "e3b0c44298fc1c149afbf4c8996fb924", }, + { + // `.pub` files end with a newline. Without trimming, a + // comment-less key would hash with `\n` mixed in and + // stop matching the server's value. + name: "trailing newline does not change hash", + input: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDUMMY\n", + want: "2b366430eb9743668b652921d3b22d54", + }, + { + name: "leading and trailing whitespace stripped", + input: " ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDUMMY \n", + want: "2b366430eb9743668b652921d3b22d54", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/lakebox/register.go b/cmd/lakebox/register.go index ca19a91865b..9c7538f47d3 100644 --- a/cmd/lakebox/register.go +++ b/cmd/lakebox/register.go @@ -85,7 +85,7 @@ Examples: // Remote-SSH and plain `ssh @lakebox-gw` both work without // the user pasting any config block (see maybeWriteSSHConfig). if err := maybeWriteSSHConfig(ctx, keyPath, w.Config.Host); err != nil { - warn(ctx, fmt.Sprintf("registered key, but failed to update ~/.ssh/config: %v", err)) + warn(ctx, fmt.Sprintf("Registered key, but failed to update ~/.ssh/config: %v", err)) } blank(stderr) diff --git a/cmd/lakebox/ssh.go b/cmd/lakebox/ssh.go index 73715bd2040..d5768004aca 100644 --- a/cmd/lakebox/ssh.go +++ b/cmd/lakebox/ssh.go @@ -132,7 +132,7 @@ Examples: _ = clearDefault(ctx, profile) return fmt.Errorf("saved default %q no longer exists (cleared) — run `databricks lakebox create` to provision a new one, or `databricks lakebox default ` to point at an existing sandbox", def) default: - warn(ctx, fmt.Sprintf("could not validate default %s: %v", def, err)) + warn(ctx, fmt.Sprintf("Could not validate default %s: %v", def, err)) lakeboxID = def } } else { @@ -151,7 +151,7 @@ Examples: case errors.Is(err, apierr.ErrNotFound): return fmt.Errorf("no lakebox named %q — `databricks lakebox list` shows available IDs", lakeboxID) default: - warn(ctx, fmt.Sprintf("could not validate lakebox %s: %v", lakeboxID, err)) + warn(ctx, fmt.Sprintf("Could not validate lakebox %s: %v", lakeboxID, err)) } } @@ -221,7 +221,7 @@ func verifyKeyRegistered(ctx context.Context, api *lakeboxAPI, keyPath string) e keys, err := api.listKeys(ctx) if err != nil { - warn(ctx, fmt.Sprintf("could not verify SSH key registration: %v", err)) + warn(ctx, fmt.Sprintf("Could not verify SSH key registration: %v", err)) return nil } for _, k := range keys { diff --git a/cmd/lakebox/start.go b/cmd/lakebox/start.go index 6abb6cd4861..1e1d2305167 100644 --- a/cmd/lakebox/start.go +++ b/cmd/lakebox/start.go @@ -28,7 +28,7 @@ func newStartCommand() *cobra.Command { Long: `Start a stopped Lakebox environment. Boots the backing microVM and blocks until the sandbox reaches -Running (or up to 5 minutes). 'databricks lakebox ssh' already +Running (or up to 10 minutes). 'databricks lakebox ssh' already auto-starts a stopped sandbox on connection, so this command is mostly useful for pre-warming an environment without immediately connecting, or when a script needs to be sure the sandbox is up diff --git a/cmd/lakebox/status.go b/cmd/lakebox/status.go index 0a88b0b91cf..5f149bd2915 100644 --- a/cmd/lakebox/status.go +++ b/cmd/lakebox/status.go @@ -2,11 +2,13 @@ package lakebox import ( "encoding/json" + "errors" "fmt" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/spf13/cobra" ) @@ -44,6 +46,9 @@ Example: entry, err := api.get(ctx, lakeboxID) if err != nil { + if errors.Is(err, apierr.ErrNotFound) { + return fmt.Errorf("no lakebox named %q — `databricks lakebox list` shows available IDs", lakeboxID) + } return fmt.Errorf("failed to get lakebox %s: %w", lakeboxID, err) } @@ -66,9 +71,9 @@ Example: if entry.GatewayHost != "" { field(ctx, out, "gateway", cmdio.Faint(ctx, entry.GatewayHost)) } - if entry.FQDN != "" { - field(ctx, out, "fqdn", cmdio.Faint(ctx, entry.FQDN)) - } + // FQDN is the manager's internal routing host (per api.go); + // keep it in the JSON shape above for completeness but + // don't surface it to humans. field(ctx, out, "autostop", cmdio.Faint(ctx, entry.autoStopLabel())) blank(out) return nil diff --git a/cmd/lakebox/ui.go b/cmd/lakebox/ui.go index 203809ec982..2a2e9e924e7 100644 --- a/cmd/lakebox/ui.go +++ b/cmd/lakebox/ui.go @@ -78,9 +78,10 @@ func ok(ctx context.Context, msg string) { cmdio.LogString(ctx, " "+cmdio.Cyan(ctx, "✓")+" "+msg) } -// warn prints " ! message" to stderr via the cmdio context. +// warn prints " ! message" to stderr via the cmdio context. Yellow so +// it visually differs from `ok`'s cyan ✓ and `spinner` cyan markers. func warn(ctx context.Context, msg string) { - cmdio.LogString(ctx, " "+cmdio.Cyan(ctx, "!")+" "+msg) + cmdio.LogString(ctx, " "+cmdio.Yellow(ctx, "!")+" "+msg) } // blank prints an empty line to w.