From a6515c90616aafe876bfaf101c24223ec4a29bd9 Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Fri, 8 May 2026 11:29:36 +0200 Subject: [PATCH 1/6] auth: distinguish "validation failed" from "couldn't validate" in `auth profiles` Replace the binary `valid: true|false` with a three-state `status` (valid / invalid / unknown) plus an optional `error` description, so users can tell an expired token apart from a network blip. The legacy `valid` field is now a `*bool`: emitted only when the result is conclusive, omitted for transient errors that previously misreported as `valid: false`. Adds a 10s per-profile validation timeout (also bounding `EnsureResolved`'s host-metadata fetch via `HTTPTimeoutSeconds`/`RetryTimeoutSeconds`) so a single dead host no longer stalls the whole listing. Co-authored-by: Isaac --- NEXT_CHANGELOG.md | 2 + .../auth/host-metadata-cache/output.txt | 4 +- .../cmd/auth/login/discovery/output.txt | 2 +- acceptance/cmd/auth/login/nominal/output.txt | 2 +- .../output.txt | 4 +- .../auth/profiles/expired-token/out.test.toml | 3 + .../auth/profiles/expired-token/output.txt | 16 ++ .../cmd/auth/profiles/expired-token/script | 10 + .../cmd/auth/profiles/expired-token/test.toml | 15 ++ acceptance/cmd/auth/profiles/output.txt | 6 +- .../auth/profiles/server-error/out.test.toml | 3 + .../cmd/auth/profiles/server-error/output.txt | 15 ++ .../cmd/auth/profiles/server-error/script | 10 + .../cmd/auth/profiles/server-error/test.toml | 16 ++ .../cmd/auth/profiles/spog-account/output.txt | 1 + acceptance/cmd/auth/switch/nominal/output.txt | 8 +- cmd/auth/profiles.go | 183 ++++++++++++--- cmd/auth/profiles_test.go | 214 +++++++++++++++++- 18 files changed, 463 insertions(+), 51 deletions(-) create mode 100644 acceptance/cmd/auth/profiles/expired-token/out.test.toml create mode 100644 acceptance/cmd/auth/profiles/expired-token/output.txt create mode 100644 acceptance/cmd/auth/profiles/expired-token/script create mode 100644 acceptance/cmd/auth/profiles/expired-token/test.toml create mode 100644 acceptance/cmd/auth/profiles/server-error/out.test.toml create mode 100644 acceptance/cmd/auth/profiles/server-error/output.txt create mode 100644 acceptance/cmd/auth/profiles/server-error/script create mode 100644 acceptance/cmd/auth/profiles/server-error/test.toml diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 00152d550ea..fcd8bd0b02b 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -4,6 +4,8 @@ ### CLI +* `databricks auth profiles` now distinguishes "validation failed" from "couldn't validate". The JSON output adds a `status` field (`valid`, `invalid`, `unknown`, or `unvalidated`) and an `error` description for non-valid profiles. The legacy `valid` field is still emitted as `true` when validation succeeded and `false` when the profile is provably bad (auth/config error); it is omitted for transient/unknown cases that previously misreported as `valid: false`. Each profile is validated with a 10s timeout so a single dead host no longer stalls the listing. + ### Bundles ### Dependency updates diff --git a/acceptance/auth/host-metadata-cache/output.txt b/acceptance/auth/host-metadata-cache/output.txt index 0b99e9579c8..af4872d7468 100644 --- a/acceptance/auth/host-metadata-cache/output.txt +++ b/acceptance/auth/host-metadata-cache/output.txt @@ -7,7 +7,7 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "", - "valid": false + "status": "unvalidated" } ] } @@ -20,7 +20,7 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "", - "valid": false + "status": "unvalidated" } ] } diff --git a/acceptance/cmd/auth/login/discovery/output.txt b/acceptance/cmd/auth/login/discovery/output.txt index c687b07fd5e..7213fe73846 100644 --- a/acceptance/cmd/auth/login/discovery/output.txt +++ b/acceptance/cmd/auth/login/discovery/output.txt @@ -5,7 +5,7 @@ Profile discovery-test was successfully saved >>> [CLI] auth profiles Name Host Valid -discovery-test (Default) [DATABRICKS_URL] YES +discovery-test (Default) [DATABRICKS_URL] valid >>> print_requests.py --get //tokens/introspect { diff --git a/acceptance/cmd/auth/login/nominal/output.txt b/acceptance/cmd/auth/login/nominal/output.txt index 4200636bc2d..9ce51ea84a1 100644 --- a/acceptance/cmd/auth/login/nominal/output.txt +++ b/acceptance/cmd/auth/login/nominal/output.txt @@ -4,4 +4,4 @@ Profile test was successfully saved >>> [CLI] auth profiles Name Host Valid -test (Default) [DATABRICKS_URL] YES +test (Default) [DATABRICKS_URL] valid diff --git a/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt b/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt index 29c2d40709e..d24d213661b 100644 --- a/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt +++ b/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt @@ -2,7 +2,7 @@ === Profiles before logout — logfood should be valid >>> [CLI] auth profiles Name Host Valid -logfood (Default) [DATABRICKS_URL] YES +logfood (Default) [DATABRICKS_URL] valid === Token cache keys before logout [ @@ -32,7 +32,7 @@ default_profile = logfood === Profiles after logout — logfood should be invalid >>> [CLI] auth profiles Name Host Valid -logfood (Default) [DATABRICKS_URL] NO +logfood (Default) [DATABRICKS_URL] unknown === Logged out profile should no longer return a token >>> musterr [CLI] auth token --profile logfood diff --git a/acceptance/cmd/auth/profiles/expired-token/out.test.toml b/acceptance/cmd/auth/profiles/expired-token/out.test.toml new file mode 100644 index 00000000000..f784a183258 --- /dev/null +++ b/acceptance/cmd/auth/profiles/expired-token/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/auth/profiles/expired-token/output.txt b/acceptance/cmd/auth/profiles/expired-token/output.txt new file mode 100644 index 00000000000..0a22f49a2e4 --- /dev/null +++ b/acceptance/cmd/auth/profiles/expired-token/output.txt @@ -0,0 +1,16 @@ + +=== Expired token: profile is reported invalid with remediation hint +>>> [CLI] auth profiles --output json +{ + "profiles": [ + { + "name": "expired", + "host": "[DATABRICKS_URL]", + "cloud": "aws", + "auth_type": "pat", + "status": "invalid", + "error": "authentication failed (token may have expired — try 'databricks auth login -p expired')", + "valid": false + } + ] +} diff --git a/acceptance/cmd/auth/profiles/expired-token/script b/acceptance/cmd/auth/profiles/expired-token/script new file mode 100644 index 00000000000..1a48a8951b5 --- /dev/null +++ b/acceptance/cmd/auth/profiles/expired-token/script @@ -0,0 +1,10 @@ +sethome "./home" + +cat > "./home/.databrickscfg" <>> [CLI] auth profiles --output json +{ + "profiles": [ + { + "name": "transient", + "host": "[DATABRICKS_URL]", + "cloud": "aws", + "auth_type": "pat", + "status": "unknown", + "error": "server error: 500" + } + ] +} diff --git a/acceptance/cmd/auth/profiles/server-error/script b/acceptance/cmd/auth/profiles/server-error/script new file mode 100644 index 00000000000..7871aadfb9e --- /dev/null +++ b/acceptance/cmd/auth/profiles/server-error/script @@ -0,0 +1,10 @@ +sethome "./home" + +cat > "./home/.databrickscfg" <>> [CLI] auth profiles --skip-validate Name Host Valid -profile-a (Default) [DATABRICKS_URL] NO -profile-b [DATABRICKS_URL] NO +profile-a (Default) [DATABRICKS_URL] - +profile-b [DATABRICKS_URL] - === Switch to profile-b @@ -28,5 +28,5 @@ default_profile = profile-b >>> [CLI] auth profiles --skip-validate Name Host Valid -profile-a [DATABRICKS_URL] NO -profile-b (Default) [DATABRICKS_URL] NO +profile-a [DATABRICKS_URL] - +profile-b (Default) [DATABRICKS_URL] - diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go index 51c397a9ea9..93e9b47bb80 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "io/fs" + "net" + "net/url" "sync" "time" @@ -15,32 +17,148 @@ import ( "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/config" "github.com/spf13/cobra" "gopkg.in/ini.v1" ) +// profileStatus is the three-state result of validating a profile, plus a +// fourth state for "validation skipped". Reported as `status` in JSON output. +type profileStatus string + +const ( + profileStatusValid profileStatus = "valid" // API call succeeded + profileStatusInvalid profileStatus = "invalid" // proven bad: auth/config error + profileStatusUnknown profileStatus = "unknown" // could not determine (network, transient) + profileStatusUnvalidated profileStatus = "unvalidated" // --skip-validate +) + +// profileValidationTimeout bounds the per-profile validation API call so a +// single hung profile (dead host, no route) does not stall the whole listing. +const profileValidationTimeout = 10 * time.Second + type profileMetadata struct { - Name string `json:"name"` - Host string `json:"host,omitempty"` - AccountID string `json:"account_id,omitempty"` - WorkspaceID string `json:"workspace_id,omitempty"` - Cloud string `json:"cloud"` - AuthType string `json:"auth_type"` - Valid bool `json:"valid"` - Default bool `json:"default,omitempty"` + Name string `json:"name"` + Host string `json:"host,omitempty"` + AccountID string `json:"account_id,omitempty"` + WorkspaceID string `json:"workspace_id,omitempty"` + Cloud string `json:"cloud"` + AuthType string `json:"auth_type"` + Status profileStatus `json:"status"` + Error string `json:"error,omitempty"` + Default bool `json:"default,omitempty"` + + // Valid is the legacy compat field. Emitted only when Status is conclusively + // valid or invalid; absent (omitempty) for unknown/unvalidated. Old scripts + // that branch on `valid: true` keep working; scripts that branch on + // `valid: false` keep working for proven-bad profiles and now see the field + // missing for the cases we previously misreported as "false". + Valid *bool `json:"valid,omitempty"` + + // statusDisplay is the colored cell rendered for the text "Valid" column. + // Populated by Load; never serialized. + StatusDisplay string `json:"-"` } func (c *profileMetadata) IsEmpty() bool { return c.Host == "" && c.AccountID == "" } +// setStatus records the classification result and keeps the legacy Valid field +// in sync. Valid is left nil for unknown/unvalidated so omitempty drops it. +func (c *profileMetadata) setStatus(ctx context.Context, status profileStatus, msg string) { + c.Status = status + c.Error = msg + switch status { + case profileStatusValid: + t := true + c.Valid = &t + case profileStatusInvalid: + f := false + c.Valid = &f + case profileStatusUnknown, profileStatusUnvalidated: + // Leave Valid nil; omitempty drops the legacy field. + } + c.StatusDisplay = renderStatusCell(ctx, status) +} + +// renderStatusCell formats a profileStatus for the text "Valid" column. We +// keep the values short so the column width stays close to the previous +// YES/NO output. +func renderStatusCell(ctx context.Context, s profileStatus) string { + switch s { + case profileStatusValid: + return cmdio.Green(ctx, "valid") + case profileStatusInvalid: + return cmdio.Red(ctx, "invalid") + case profileStatusUnknown: + return cmdio.Yellow(ctx, "unknown") + case profileStatusUnvalidated: + return "-" + } + return "-" +} + +// classifyValidationError maps an error returned by the validation API call +// to a (status, message) pair. The profile name is interpolated into auth +// remediation hints. Order matters: timeout is checked first because the +// SDK can wrap context.DeadlineExceeded in url.Error, and APIError sentinels +// before the generic *url.Error / *net.OpError fallthrough. +func classifyValidationError(profileName string, err error) (profileStatus, string) { + if err == nil { + return profileStatusValid, "" + } + if errors.Is(err, context.DeadlineExceeded) { + return profileStatusUnknown, "validation timed out" + } + if errors.Is(err, apierr.ErrUnauthenticated) { + return profileStatusInvalid, fmt.Sprintf("authentication failed (token may have expired — try 'databricks auth login -p %s')", profileName) + } + if errors.Is(err, apierr.ErrPermissionDenied) { + return profileStatusInvalid, "credentials lack permission for the validation API call" + } + var apiErr *apierr.APIError + if errors.As(err, &apiErr) && apiErr.StatusCode >= 500 { + return profileStatusUnknown, fmt.Sprintf("server error: %d", apiErr.StatusCode) + } + if reason, ok := networkErrorReason(err); ok { + return profileStatusUnknown, "could not reach host: " + reason + } + return profileStatusUnknown, err.Error() +} + +// networkErrorReason returns the short reason from a *url.Error / *net.OpError +// chain (skipping the URL prefix that url.Error.Error() prepends). The second +// return is false when err is not a network error. +func networkErrorReason(err error) (string, bool) { + var urlErr *url.Error + if errors.As(err, &urlErr) && urlErr.Err != nil { + return urlErr.Err.Error(), true + } + var netErr *net.OpError + if errors.As(err, &netErr) { + return netErr.Error(), true + } + return "", false +} + func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipValidate bool) { + timeoutSeconds := int(profileValidationTimeout / time.Second) cfg := &config.Config{ Loaders: []config.Loader{config.ConfigFile}, ConfigFile: configFilePath, Profile: c.Name, DatabricksCliPath: env.Get(ctx, "DATABRICKS_CLI_PATH"), + + // Bound the SDK's per-request and total-retry budgets to the same + // per-profile ceiling. EnsureResolved fetches host metadata via the + // SDK's retrier, which defaults to 5 minutes — without this, a single + // dead host stalls the listing well past the validation context's + // timeout (the EnsureResolved call uses context.Background internally, + // so our context.WithTimeout below cannot reach it). + HTTPTimeoutSeconds: timeoutSeconds, + RetryTimeoutSeconds: timeoutSeconds, } _ = cfg.EnsureResolved() if cfg.IsAws() { @@ -54,6 +172,7 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV if skipValidate { c.Host = cfg.CanonicalHostName() c.AuthType = cfg.AuthType + c.setStatus(ctx, profileStatusUnvalidated, "") return } @@ -62,35 +181,37 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV log.Debugf(ctx, "Profile %q: overrode config type from %s to %s (SPOG host)", c.Name, cfg.ConfigType(), configType) } + if configType == config.InvalidConfig { + c.setStatus(ctx, profileStatusInvalid, "profile fields conflict (e.g. workspace and account configured together)") + return + } + + callCtx, cancel := context.WithTimeout(ctx, profileValidationTimeout) + defer cancel() + + var err error switch configType { case config.AccountConfig: - a, err := databricks.NewAccountClient((*databricks.Config)(cfg)) - if err != nil { - return + var a *databricks.AccountClient + a, err = databricks.NewAccountClient((*databricks.Config)(cfg)) + if err == nil { + _, err = a.Workspaces.List(callCtx) } - _, err = a.Workspaces.List(ctx) - c.Host = cfg.Host - c.AuthType = cfg.AuthType - if err != nil { - return - } - c.Valid = true case config.WorkspaceConfig: - w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg)) - if err != nil { - return + var w *databricks.WorkspaceClient + w, err = databricks.NewWorkspaceClient((*databricks.Config)(cfg)) + if err == nil { + _, err = w.CurrentUser.Me(callCtx) } - _, err = w.CurrentUser.Me(ctx) - c.Host = cfg.Host - c.AuthType = cfg.AuthType - if err != nil { - return - } - c.Valid = true case config.InvalidConfig: - // Invalid configuration, skip validation - return + // Handled above with an early return; listed here for switch exhaustiveness. } + + c.Host = cfg.Host + c.AuthType = cfg.AuthType + + status, msg := classifyValidationError(c.Name, err) + c.setStatus(ctx, status, msg) } func newProfilesCommand() *cobra.Command { @@ -100,7 +221,7 @@ func newProfilesCommand() *cobra.Command { Annotations: map[string]string{ "template": cmdio.Heredoc(` {{header "Name"}} {{header "Host"}} {{header "Valid"}} - {{range .Profiles}}{{.Name | green}}{{if .Default}} (Default){{end}} {{.Host|cyan}} {{bool .Valid}} + {{range .Profiles}}{{.Name | green}}{{if .Default}} (Default){{end}} {{.Host|cyan}} {{.StatusDisplay}} {{end}}`), }, } diff --git a/cmd/auth/profiles_test.go b/cmd/auth/profiles_test.go index 59803e210cf..46db0e7d02a 100644 --- a/cmd/auth/profiles_test.go +++ b/cmd/auth/profiles_test.go @@ -1,15 +1,19 @@ package auth import ( + "context" "encoding/json" + "errors" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "runtime" "testing" "github.com/databricks/cli/libs/databrickscfg" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -46,6 +50,8 @@ func TestProfiles(t *testing.T) { assert.Equal(t, "https://abc.cloud.databricks.com", profile.Host) assert.Equal(t, "aws", profile.Cloud) assert.Equal(t, "pat", profile.AuthType) + assert.Equal(t, profileStatusUnvalidated, profile.Status) + assert.Nil(t, profile.Valid, "Valid should be unset for unvalidated") } func TestProfilesDefaultMarker(t *testing.T) { @@ -139,33 +145,28 @@ func TestProfileLoadSPOGConfigType(t *testing.T) { host string accountID string workspaceID string - wantValid bool }{ { name: "SPOG account profile validated as account", host: spogServer.URL, accountID: "spog-acct", - wantValid: true, }, { name: "SPOG workspace profile validated as workspace", host: spogServer.URL, accountID: "spog-acct", workspaceID: "ws-123", - wantValid: true, }, { name: "SPOG profile with workspace_id=none validated as account", host: spogServer.URL, accountID: "spog-acct", workspaceID: "none", - wantValid: true, }, { name: "classic workspace with account_id from discovery stays workspace", host: wsServer.URL, accountID: "ws-acct", - wantValid: true, }, } @@ -194,7 +195,9 @@ func TestProfileLoadSPOGConfigType(t *testing.T) { } p.Load(t.Context(), configFile, false) - assert.Equal(t, tc.wantValid, p.Valid, "Valid mismatch") + assert.Equal(t, profileStatusValid, p.Status, "status mismatch") + require.NotNil(t, p.Valid) + assert.True(t, *p.Valid) assert.NotEmpty(t, p.Host, "Host should be set") assert.NotEmpty(t, p.AuthType, "AuthType should be set") }) @@ -250,7 +253,204 @@ func TestProfileLoadNoDiscoveryStaysWorkspace(t *testing.T) { } p.Load(t.Context(), configFile, false) - assert.True(t, p.Valid, "should validate as workspace when discovery is unavailable") + assert.Equal(t, profileStatusValid, p.Status, "should validate as workspace when discovery is unavailable") assert.NotEmpty(t, p.Host) assert.Equal(t, "pat", p.AuthType) } + +func TestClassifyValidationError(t *testing.T) { + cases := []struct { + name string + err error + wantStatus profileStatus + wantMsgSub string + }{ + { + name: "nil error -> valid", + err: nil, + wantStatus: profileStatusValid, + }, + { + name: "deadline exceeded -> unknown timeout", + err: context.DeadlineExceeded, + wantStatus: profileStatusUnknown, + wantMsgSub: "validation timed out", + }, + { + name: "url.Error wrapping deadline -> unknown timeout", + err: &url.Error{Op: "Get", URL: "https://x.test/", Err: context.DeadlineExceeded}, + wantStatus: profileStatusUnknown, + wantMsgSub: "validation timed out", + }, + { + name: "401 -> invalid with auth remediation", + err: &apierr.APIError{StatusCode: 401, Message: "unauthorized"}, + wantStatus: profileStatusInvalid, + wantMsgSub: "databricks auth login -p test-profile", + }, + { + name: "403 -> invalid with permission message", + err: &apierr.APIError{StatusCode: 403, Message: "forbidden"}, + wantStatus: profileStatusInvalid, + wantMsgSub: "credentials lack permission", + }, + { + name: "500 -> unknown server error", + err: &apierr.APIError{StatusCode: 500, Message: "internal"}, + wantStatus: profileStatusUnknown, + wantMsgSub: "server error: 500", + }, + { + name: "503 -> unknown server error", + err: &apierr.APIError{StatusCode: 503, Message: "unavailable"}, + wantStatus: profileStatusUnknown, + wantMsgSub: "server error: 503", + }, + { + name: "network error -> unknown could-not-reach", + err: &url.Error{Op: "Get", URL: "https://x.test/", Err: errors.New("dial tcp: lookup x.test: no such host")}, + wantStatus: profileStatusUnknown, + wantMsgSub: "could not reach host", + }, + { + name: "fallthrough -> unknown with raw message", + err: errors.New("strange unknown failure"), + wantStatus: profileStatusUnknown, + wantMsgSub: "strange unknown failure", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status, msg := classifyValidationError("test-profile", tc.err) + assert.Equal(t, tc.wantStatus, status) + if tc.wantMsgSub == "" { + assert.Empty(t, msg) + } else { + assert.Contains(t, msg, tc.wantMsgSub) + } + }) + } +} + +func TestProfileLoadStatusMatrix(t *testing.T) { + // statusServer returns a configurable HTTP status for the validation + // endpoint. .well-known returns 404 so we land on WorkspaceConfig and + // CurrentUser.Me is the validation API call. + statusServer := func(t *testing.T, code int) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/.well-known/databricks-config": + w.WriteHeader(http.StatusNotFound) + case "/api/2.0/preview/scim/v2/Me": + w.WriteHeader(code) + _, _ = w.Write([]byte(`{"error_code":"X","message":"x"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + return server + } + + t.Run("401 -> invalid", func(t *testing.T) { + s := statusServer(t, http.StatusUnauthorized) + p := loadFromHost(t, s.URL) + assert.Equal(t, profileStatusInvalid, p.Status) + require.NotNil(t, p.Valid) + assert.False(t, *p.Valid) + assert.Contains(t, p.Error, "databricks auth login") + }) + + t.Run("403 -> invalid", func(t *testing.T) { + s := statusServer(t, http.StatusForbidden) + p := loadFromHost(t, s.URL) + assert.Equal(t, profileStatusInvalid, p.Status) + require.NotNil(t, p.Valid) + assert.False(t, *p.Valid) + assert.Contains(t, p.Error, "permission") + }) + + t.Run("500 -> unknown", func(t *testing.T) { + s := statusServer(t, http.StatusInternalServerError) + p := loadFromHost(t, s.URL) + assert.Equal(t, profileStatusUnknown, p.Status) + assert.Nil(t, p.Valid, "Valid is omitted for unknown") + assert.Contains(t, p.Error, "server error") + }) + + t.Run("network down -> unknown", func(t *testing.T) { + // Start and immediately close the server to simulate a dead host. + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + s.Close() + p := loadFromHost(t, s.URL) + assert.Equal(t, profileStatusUnknown, p.Status) + assert.Nil(t, p.Valid) + assert.Contains(t, p.Error, "could not reach host") + }) + + t.Run("InvalidConfig -> invalid", func(t *testing.T) { + // experimental_is_unified_host=true forces HostType=UnifiedHost. + // Without an account_id (or a SPOG-shaped DiscoveryURL), ResolveConfigType + // can't pick a side and falls through to InvalidConfig. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(server.Close) + + dir := t.TempDir() + configFile := filepath.Join(dir, ".databrickscfg") + t.Setenv("HOME", dir) + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", dir) + } + content := "[bad]\nhost = " + server.URL + "\nexperimental_is_unified_host = true\ntoken = test-token\n" + require.NoError(t, os.WriteFile(configFile, []byte(content), 0o600)) + + p := &profileMetadata{Name: "bad", Host: server.URL} + p.Load(t.Context(), configFile, false) + assert.Equal(t, profileStatusInvalid, p.Status) + require.NotNil(t, p.Valid) + assert.False(t, *p.Valid) + assert.Contains(t, p.Error, "fields conflict") + }) + + t.Run("skip-validate -> unvalidated", func(t *testing.T) { + s := statusServer(t, http.StatusOK) + p := loadFromHost(t, s.URL, withSkipValidate()) + assert.Equal(t, profileStatusUnvalidated, p.Status) + assert.Nil(t, p.Valid) + assert.Empty(t, p.Error) + }) +} + +type loadOpts struct { + skipValidate bool +} + +type loadOpt func(*loadOpts) + +func withSkipValidate() loadOpt { return func(o *loadOpts) { o.skipValidate = true } } + +// loadFromHost writes a single PAT profile pointing at host into a temp +// .databrickscfg, runs Load, and returns the populated profileMetadata. +func loadFromHost(t *testing.T, host string, opts ...loadOpt) *profileMetadata { + t.Helper() + o := loadOpts{} + for _, opt := range opts { + opt(&o) + } + dir := t.TempDir() + configFile := filepath.Join(dir, ".databrickscfg") + t.Setenv("HOME", dir) + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", dir) + } + content := "[test-profile]\nhost = " + host + "\ntoken = test-token\n" + require.NoError(t, os.WriteFile(configFile, []byte(content), 0o600)) + + p := &profileMetadata{Name: "test-profile", Host: host} + p.Load(t.Context(), configFile, o.skipValidate) + return p +} From f43cf53102e1196cf1ceb3842e82bbf7c9cb2136 Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Tue, 14 Jul 2026 23:33:06 +0200 Subject: [PATCH 2/6] auth profiles: simplify to YES/NO/?? with valid_reason in JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse the profile validation display to three states in the Valid column: YES (validated), NO (any validation error), and ?? (validation skipped via --skip-validate). Drop the separate Reason column from the text table — every real error becomes NO, and ?? is reserved for skip-validate, so the table stays scannable with no per-error curation. JSON output keeps the boolean `valid` field and carries the full underlying error (or a skip note) in `valid_reason`; it is empty only on success. This removes the earlier classifyValidationError/networkErrorReason machinery and the status/error JSON fields in favor of the simpler model. Co-authored-by: Isaac --- .nextchanges/cli/auth-profiles-validate.md | 2 +- .../auth/host-metadata-cache/output.txt | 2 - .../cmd/auth/login/discovery/output.txt | 2 +- acceptance/cmd/auth/login/nominal/output.txt | 2 +- .../output.txt | 4 +- .../auth/profiles/expired-token/output.txt | 7 +- .../cmd/auth/profiles/expired-token/script | 2 +- acceptance/cmd/auth/profiles/output.txt | 9 +- .../cmd/auth/profiles/server-error/output.txt | 6 +- .../cmd/auth/profiles/server-error/script | 2 +- .../cmd/auth/profiles/spog-account/output.txt | 1 - acceptance/cmd/auth/switch/nominal/output.txt | 8 +- cmd/auth/profiles.go | 144 ++++++------------ cmd/auth/profiles_test.go | 133 +++------------- 14 files changed, 91 insertions(+), 233 deletions(-) diff --git a/.nextchanges/cli/auth-profiles-validate.md b/.nextchanges/cli/auth-profiles-validate.md index da4f3d2b27a..dac17c91ba1 100644 --- a/.nextchanges/cli/auth-profiles-validate.md +++ b/.nextchanges/cli/auth-profiles-validate.md @@ -1 +1 @@ -* `databricks auth profiles` now distinguishes "validation failed" from "couldn't validate". The JSON output adds a `status` field (`valid`, `invalid`, `unknown`, or `unvalidated`) and an `error` description for non-valid profiles. The legacy `valid` field is still emitted as `true` when validation succeeded and `false` when the profile is provably bad (auth/config error); it is omitted for transient/unknown cases that previously misreported as `valid: false`. Each profile is validated with a 10s timeout so a single dead host no longer stalls the listing. +* `databricks auth profiles` now explains why a profile is not valid. The JSON output keeps the boolean `valid` field (backwards compatible) and adds a `valid_reason` string explaining a `valid: false` result — the full underlying error when validation fails, or a note when it was skipped via `--skip-validate` (empty only on success). The text table's Valid column now renders `YES` (validated), `NO` (validation failed), or `??` (validation skipped via `--skip-validate`). Each profile is validated with a 10s timeout so a single dead host no longer stalls the listing. diff --git a/acceptance/auth/host-metadata-cache/output.txt b/acceptance/auth/host-metadata-cache/output.txt index 1034132378d..d059d2bbc4b 100644 --- a/acceptance/auth/host-metadata-cache/output.txt +++ b/acceptance/auth/host-metadata-cache/output.txt @@ -7,7 +7,6 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "pat", - "status": "valid", "valid": true } ] @@ -21,7 +20,6 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "pat", - "status": "valid", "valid": true } ] diff --git a/acceptance/cmd/auth/login/discovery/output.txt b/acceptance/cmd/auth/login/discovery/output.txt index 7213fe73846..c687b07fd5e 100644 --- a/acceptance/cmd/auth/login/discovery/output.txt +++ b/acceptance/cmd/auth/login/discovery/output.txt @@ -5,7 +5,7 @@ Profile discovery-test was successfully saved >>> [CLI] auth profiles Name Host Valid -discovery-test (Default) [DATABRICKS_URL] valid +discovery-test (Default) [DATABRICKS_URL] YES >>> print_requests.py --get //tokens/introspect { diff --git a/acceptance/cmd/auth/login/nominal/output.txt b/acceptance/cmd/auth/login/nominal/output.txt index 9ce51ea84a1..4200636bc2d 100644 --- a/acceptance/cmd/auth/login/nominal/output.txt +++ b/acceptance/cmd/auth/login/nominal/output.txt @@ -4,4 +4,4 @@ Profile test was successfully saved >>> [CLI] auth profiles Name Host Valid -test (Default) [DATABRICKS_URL] valid +test (Default) [DATABRICKS_URL] YES diff --git a/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt b/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt index 3273faad165..8c3218ad06e 100644 --- a/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt +++ b/acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt @@ -2,7 +2,7 @@ === Profiles before logout — logfood should be valid >>> [CLI] auth profiles Name Host Valid -logfood (Default) [DATABRICKS_URL] valid +logfood (Default) [DATABRICKS_URL] YES === Token cache keys before logout [ @@ -32,7 +32,7 @@ default_profile = logfood === Profiles after logout — logfood should be invalid >>> [CLI] auth profiles Name Host Valid -logfood (Default) [DATABRICKS_URL] unknown +logfood (Default) [DATABRICKS_URL] NO === Logged out profile should no longer return a token >>> musterr [CLI] auth token --profile logfood diff --git a/acceptance/cmd/auth/profiles/expired-token/output.txt b/acceptance/cmd/auth/profiles/expired-token/output.txt index 0a22f49a2e4..22c645c9232 100644 --- a/acceptance/cmd/auth/profiles/expired-token/output.txt +++ b/acceptance/cmd/auth/profiles/expired-token/output.txt @@ -1,5 +1,5 @@ -=== Expired token: profile is reported invalid with remediation hint +=== Expired token: profile is invalid (NO), full error in valid_reason >>> [CLI] auth profiles --output json { "profiles": [ @@ -8,9 +8,8 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "pat", - "status": "invalid", - "error": "authentication failed (token may have expired — try 'databricks auth login -p expired')", - "valid": false + "valid": false, + "valid_reason": "Token is invalid or expired." } ] } diff --git a/acceptance/cmd/auth/profiles/expired-token/script b/acceptance/cmd/auth/profiles/expired-token/script index 1a48a8951b5..d75fefe2cc5 100644 --- a/acceptance/cmd/auth/profiles/expired-token/script +++ b/acceptance/cmd/auth/profiles/expired-token/script @@ -6,5 +6,5 @@ host = ${DATABRICKS_HOST} token = test-token EOF -title "Expired token: profile is reported invalid with remediation hint" +title "Expired token: profile is invalid (NO), full error in valid_reason" trace $CLI auth profiles --output json diff --git a/acceptance/cmd/auth/profiles/output.txt b/acceptance/cmd/auth/profiles/output.txt index d34c231de68..49fa7a3812c 100644 --- a/acceptance/cmd/auth/profiles/output.txt +++ b/acceptance/cmd/auth/profiles/output.txt @@ -7,7 +7,8 @@ "host": "https://workspace.cloud.databricks.test", "cloud": "aws", "auth_type": "", - "status": "unvalidated" + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" }, { "name": "account-profile", @@ -15,7 +16,8 @@ "account_id": "test-account-123", "cloud": "aws", "auth_type": "", - "status": "unvalidated" + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" }, { "name": "unified-profile", @@ -24,7 +26,8 @@ "workspace_id": "[NUMID]", "cloud": "aws", "auth_type": "", - "status": "unvalidated" + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" } ] } diff --git a/acceptance/cmd/auth/profiles/server-error/output.txt b/acceptance/cmd/auth/profiles/server-error/output.txt index 96ad4dc30ee..2e2b5abe024 100644 --- a/acceptance/cmd/auth/profiles/server-error/output.txt +++ b/acceptance/cmd/auth/profiles/server-error/output.txt @@ -1,5 +1,5 @@ -=== 5xx from validation endpoint: profile reported as unknown (transient) +=== 5xx from validation endpoint: profile is invalid (NO), full error in valid_reason >>> [CLI] auth profiles --output json { "profiles": [ @@ -8,8 +8,8 @@ "host": "[DATABRICKS_URL]", "cloud": "aws", "auth_type": "pat", - "status": "unknown", - "error": "server error: 500" + "valid": false, + "valid_reason": "Internal server error" } ] } diff --git a/acceptance/cmd/auth/profiles/server-error/script b/acceptance/cmd/auth/profiles/server-error/script index 7871aadfb9e..4e4344aa0ff 100644 --- a/acceptance/cmd/auth/profiles/server-error/script +++ b/acceptance/cmd/auth/profiles/server-error/script @@ -6,5 +6,5 @@ host = ${DATABRICKS_HOST} token = test-token EOF -title "5xx from validation endpoint: profile reported as unknown (transient)" +title "5xx from validation endpoint: profile is invalid (NO), full error in valid_reason" trace $CLI auth profiles --output json diff --git a/acceptance/cmd/auth/profiles/spog-account/output.txt b/acceptance/cmd/auth/profiles/spog-account/output.txt index 9ba3630eea5..7ec701d700e 100644 --- a/acceptance/cmd/auth/profiles/spog-account/output.txt +++ b/acceptance/cmd/auth/profiles/spog-account/output.txt @@ -10,7 +10,6 @@ "workspace_id": "none", "cloud": "aws", "auth_type": "pat", - "status": "valid", "valid": true } ] diff --git a/acceptance/cmd/auth/switch/nominal/output.txt b/acceptance/cmd/auth/switch/nominal/output.txt index 68da9441899..2c3af8d53b9 100644 --- a/acceptance/cmd/auth/switch/nominal/output.txt +++ b/acceptance/cmd/auth/switch/nominal/output.txt @@ -12,8 +12,8 @@ default_profile = profile-a >>> [CLI] auth profiles --skip-validate Name Host Valid -profile-a (Default) [DATABRICKS_URL] - -profile-b [DATABRICKS_URL] - +profile-a (Default) [DATABRICKS_URL] ?? +profile-b [DATABRICKS_URL] ?? === Switch to profile-b @@ -28,5 +28,5 @@ default_profile = profile-b >>> [CLI] auth profiles --skip-validate Name Host Valid -profile-a [DATABRICKS_URL] - -profile-b (Default) [DATABRICKS_URL] - +profile-a [DATABRICKS_URL] ?? +profile-b (Default) [DATABRICKS_URL] ?? diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go index 57093dff07b..aef9f328475 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -5,8 +5,6 @@ import ( "errors" "fmt" "io/fs" - "net" - "net/url" "sync" "time" @@ -17,22 +15,21 @@ import ( "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" - "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/service/iam" "github.com/spf13/cobra" "gopkg.in/ini.v1" ) -// profileStatus is the three-state result of validating a profile, plus a -// fourth state for "validation skipped". Reported as `status` in JSON output. +// profileStatus is the three-state result of listing a profile. It drives the +// YES/NO/?? cell in the text table. Only YES (validated) and NO (validation +// failed) reflect an actual check; ?? means validation was skipped. type profileStatus string const ( - profileStatusValid profileStatus = "valid" // API call succeeded - profileStatusInvalid profileStatus = "invalid" // proven bad: auth/config error - profileStatusUnknown profileStatus = "unknown" // could not determine (network, transient) - profileStatusUnvalidated profileStatus = "unvalidated" // --skip-validate + profileStatusValid profileStatus = "valid" // validation succeeded + profileStatusInvalid profileStatus = "invalid" // validation failed (any error) + profileStatusSkipped profileStatus = "skipped" // --skip-validate; not checked ) // profileValidationTimeout bounds the per-profile validation API call so a @@ -40,25 +37,24 @@ const ( const profileValidationTimeout = 10 * time.Second type profileMetadata struct { - Name string `json:"name"` - Host string `json:"host,omitempty"` - AccountID string `json:"account_id,omitempty"` - WorkspaceID string `json:"workspace_id,omitempty"` - Cloud string `json:"cloud"` - AuthType string `json:"auth_type"` - Status profileStatus `json:"status"` - Error string `json:"error,omitempty"` - Default bool `json:"default,omitempty"` - - // Valid is the legacy compat field. Emitted only when Status is conclusively - // valid or invalid; absent (omitempty) for unknown/unvalidated. Old scripts - // that branch on `valid: true` keep working; scripts that branch on - // `valid: false` keep working for proven-bad profiles and now see the field - // missing for the cases we previously misreported as "false". - Valid *bool `json:"valid,omitempty"` - - // statusDisplay is the colored cell rendered for the text "Valid" column. - // Populated by Load; never serialized. + Name string `json:"name"` + Host string `json:"host,omitempty"` + AccountID string `json:"account_id,omitempty"` + WorkspaceID string `json:"workspace_id,omitempty"` + Cloud string `json:"cloud"` + AuthType string `json:"auth_type"` + + // Valid is true only when validation conclusively succeeded. ValidReason + // explains a false result: the full underlying error when validation failed, + // or a "skipped" note under --skip-validate. It is empty only on success. + // The text table shows only YES/NO/?? (via StatusDisplay); the reason detail + // is reserved for --output json. + Valid bool `json:"valid"` + ValidReason string `json:"valid_reason,omitempty"` + Default bool `json:"default,omitempty"` + + // StatusDisplay is the colored YES/NO/?? cell for the text table. Not + // serialized; the JSON form uses Valid + ValidReason instead. StatusDisplay string `json:"-"` } @@ -66,82 +62,27 @@ func (c *profileMetadata) IsEmpty() bool { return c.Host == "" && c.AccountID == "" } -// setStatus records the classification result and keeps the legacy Valid field -// in sync. Valid is left nil for unknown/unvalidated so omitempty drops it. -func (c *profileMetadata) setStatus(ctx context.Context, status profileStatus, msg string) { - c.Status = status - c.Error = msg - switch status { - case profileStatusValid: - t := true - c.Valid = &t - case profileStatusInvalid: - f := false - c.Valid = &f - case profileStatusUnknown, profileStatusUnvalidated: - // Leave Valid nil; omitempty drops the legacy field. - } +// setStatus records the validation result: Valid is true only for a +// conclusively valid profile, and reason explains a non-valid result (the +// underlying error, or a skip note) for JSON output. +func (c *profileMetadata) setStatus(ctx context.Context, status profileStatus, reason string) { + c.Valid = status == profileStatusValid + c.ValidReason = reason c.StatusDisplay = renderStatusCell(ctx, status) } -// renderStatusCell formats a profileStatus for the text "Valid" column. We -// keep the values short so the column width stays close to the previous -// YES/NO output. +// renderStatusCell formats a profileStatus for the text "Valid" column: +// YES (green, validated) / NO (red, validation failed) / ?? (grey, skipped). func renderStatusCell(ctx context.Context, s profileStatus) string { switch s { case profileStatusValid: - return cmdio.Green(ctx, "valid") + return cmdio.Green(ctx, "YES") case profileStatusInvalid: - return cmdio.Red(ctx, "invalid") - case profileStatusUnknown: - return cmdio.Yellow(ctx, "unknown") - case profileStatusUnvalidated: - return "-" + return cmdio.Red(ctx, "NO") + case profileStatusSkipped: + return cmdio.HiBlack(ctx, "??") } - return "-" -} - -// classifyValidationError maps an error returned by the validation API call -// to a (status, message) pair. The profile name is interpolated into auth -// remediation hints. Order matters: timeout is checked first because the -// SDK can wrap context.DeadlineExceeded in url.Error, and APIError sentinels -// before the generic *url.Error / *net.OpError fallthrough. -func classifyValidationError(profileName string, err error) (profileStatus, string) { - if err == nil { - return profileStatusValid, "" - } - if errors.Is(err, context.DeadlineExceeded) { - return profileStatusUnknown, "validation timed out" - } - if errors.Is(err, apierr.ErrUnauthenticated) { - return profileStatusInvalid, fmt.Sprintf("authentication failed (token may have expired — try 'databricks auth login -p %s')", profileName) - } - if errors.Is(err, apierr.ErrPermissionDenied) { - return profileStatusInvalid, "credentials lack permission for the validation API call" - } - var apiErr *apierr.APIError - if errors.As(err, &apiErr) && apiErr.StatusCode >= 500 { - return profileStatusUnknown, fmt.Sprintf("server error: %d", apiErr.StatusCode) - } - if reason, ok := networkErrorReason(err); ok { - return profileStatusUnknown, "could not reach host: " + reason - } - return profileStatusUnknown, err.Error() -} - -// networkErrorReason returns the short reason from a *url.Error / *net.OpError -// chain (skipping the URL prefix that url.Error.Error() prepends). The second -// return is false when err is not a network error. -func networkErrorReason(err error) (string, bool) { - var urlErr *url.Error - if errors.As(err, &urlErr) && urlErr.Err != nil { - return urlErr.Err.Error(), true - } - var netErr *net.OpError - if errors.As(err, &netErr) { - return netErr.Error(), true - } - return "", false + return cmdio.HiBlack(ctx, "??") } func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipValidate bool) { @@ -182,7 +123,7 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV if skipValidate { c.Host = cfg.CanonicalHostName() c.AuthType = cfg.AuthType - c.setStatus(ctx, profileStatusUnvalidated, "") + c.setStatus(ctx, profileStatusSkipped, "validation skipped (--skip-validate)") return } @@ -220,8 +161,13 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV c.Host = cfg.Host c.AuthType = cfg.AuthType - status, msg := classifyValidationError(c.Name, err) - c.setStatus(ctx, status, msg) + // Any validation error means NO; the full error is preserved in ValidReason + // for --output json. The text table shows only the YES/NO/?? cell. + if err != nil { + c.setStatus(ctx, profileStatusInvalid, err.Error()) + return + } + c.setStatus(ctx, profileStatusValid, "") } func newProfilesCommand() *cobra.Command { diff --git a/cmd/auth/profiles_test.go b/cmd/auth/profiles_test.go index d5084cd562d..47c8b2830da 100644 --- a/cmd/auth/profiles_test.go +++ b/cmd/auth/profiles_test.go @@ -1,12 +1,9 @@ package auth import ( - "context" "encoding/json" - "errors" "net/http" "net/http/httptest" - "net/url" "os" "path/filepath" "runtime" @@ -14,7 +11,6 @@ import ( "testing" "github.com/databricks/cli/libs/databrickscfg" - "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -51,8 +47,8 @@ func TestProfiles(t *testing.T) { assert.Equal(t, "https://abc.cloud.databricks.com", profile.Host) assert.Equal(t, "aws", profile.Cloud) assert.Equal(t, "pat", profile.AuthType) - assert.Equal(t, profileStatusUnvalidated, profile.Status) - assert.Nil(t, profile.Valid, "Valid should be unset for unvalidated") + assert.False(t, profile.Valid, "Valid should be false when validation is skipped") + assert.Contains(t, profile.ValidReason, "skip-validate") } // TestProfileLoadSkipValidateMakesNoRequests guards the --skip-validate @@ -81,7 +77,7 @@ func TestProfileLoadSkipValidateMakesNoRequests(t *testing.T) { assert.Zero(t, requests.Load(), "expected no network calls with skipValidate") assert.Equal(t, server.URL, p.Host) - assert.Nil(t, p.Valid, "Valid should be unset for unvalidated") + assert.False(t, p.Valid, "Valid should be false when validation is skipped") } func TestProfilesDefaultMarker(t *testing.T) { @@ -225,9 +221,8 @@ func TestProfileLoadSPOGConfigType(t *testing.T) { } p.Load(t.Context(), configFile, false) - assert.Equal(t, profileStatusValid, p.Status, "status mismatch") - require.NotNil(t, p.Valid) - assert.True(t, *p.Valid) + assert.True(t, p.Valid) + assert.Empty(t, p.ValidReason) assert.NotEmpty(t, p.Host, "Host should be set") assert.NotEmpty(t, p.AuthType, "AuthType should be set") }) @@ -283,86 +278,11 @@ func TestProfileLoadNoDiscoveryStaysWorkspace(t *testing.T) { } p.Load(t.Context(), configFile, false) - assert.Equal(t, profileStatusValid, p.Status, "should validate as workspace when discovery is unavailable") + assert.True(t, p.Valid, "should validate as workspace when discovery is unavailable") assert.NotEmpty(t, p.Host) assert.Equal(t, "pat", p.AuthType) } -func TestClassifyValidationError(t *testing.T) { - cases := []struct { - name string - err error - wantStatus profileStatus - wantMsgSub string - }{ - { - name: "nil error -> valid", - err: nil, - wantStatus: profileStatusValid, - }, - { - name: "deadline exceeded -> unknown timeout", - err: context.DeadlineExceeded, - wantStatus: profileStatusUnknown, - wantMsgSub: "validation timed out", - }, - { - name: "url.Error wrapping deadline -> unknown timeout", - err: &url.Error{Op: "Get", URL: "https://x.test/", Err: context.DeadlineExceeded}, - wantStatus: profileStatusUnknown, - wantMsgSub: "validation timed out", - }, - { - name: "401 -> invalid with auth remediation", - err: &apierr.APIError{StatusCode: 401, Message: "unauthorized"}, - wantStatus: profileStatusInvalid, - wantMsgSub: "databricks auth login -p test-profile", - }, - { - name: "403 -> invalid with permission message", - err: &apierr.APIError{StatusCode: 403, Message: "forbidden"}, - wantStatus: profileStatusInvalid, - wantMsgSub: "credentials lack permission", - }, - { - name: "500 -> unknown server error", - err: &apierr.APIError{StatusCode: 500, Message: "internal"}, - wantStatus: profileStatusUnknown, - wantMsgSub: "server error: 500", - }, - { - name: "503 -> unknown server error", - err: &apierr.APIError{StatusCode: 503, Message: "unavailable"}, - wantStatus: profileStatusUnknown, - wantMsgSub: "server error: 503", - }, - { - name: "network error -> unknown could-not-reach", - err: &url.Error{Op: "Get", URL: "https://x.test/", Err: errors.New("dial tcp: lookup x.test: no such host")}, - wantStatus: profileStatusUnknown, - wantMsgSub: "could not reach host", - }, - { - name: "fallthrough -> unknown with raw message", - err: errors.New("strange unknown failure"), - wantStatus: profileStatusUnknown, - wantMsgSub: "strange unknown failure", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - status, msg := classifyValidationError("test-profile", tc.err) - assert.Equal(t, tc.wantStatus, status) - if tc.wantMsgSub == "" { - assert.Empty(t, msg) - } else { - assert.Contains(t, msg, tc.wantMsgSub) - } - }) - } -} - func TestProfileLoadStatusMatrix(t *testing.T) { // statusServer returns a configurable HTTP status for the validation // endpoint. .well-known returns 404 so we land on WorkspaceConfig and @@ -384,40 +304,36 @@ func TestProfileLoadStatusMatrix(t *testing.T) { return server } + // Any validation error (auth, permission, server, network) is reported as + // NO with the full error preserved in ValidReason for --output json. t.Run("401 -> invalid", func(t *testing.T) { s := statusServer(t, http.StatusUnauthorized) p := loadFromHost(t, s.URL) - assert.Equal(t, profileStatusInvalid, p.Status) - require.NotNil(t, p.Valid) - assert.False(t, *p.Valid) - assert.Contains(t, p.Error, "databricks auth login") + assert.False(t, p.Valid) + assert.NotEmpty(t, p.ValidReason) }) t.Run("403 -> invalid", func(t *testing.T) { s := statusServer(t, http.StatusForbidden) p := loadFromHost(t, s.URL) - assert.Equal(t, profileStatusInvalid, p.Status) - require.NotNil(t, p.Valid) - assert.False(t, *p.Valid) - assert.Contains(t, p.Error, "permission") + assert.False(t, p.Valid) + assert.NotEmpty(t, p.ValidReason) }) - t.Run("500 -> unknown", func(t *testing.T) { + t.Run("500 -> invalid", func(t *testing.T) { s := statusServer(t, http.StatusInternalServerError) p := loadFromHost(t, s.URL) - assert.Equal(t, profileStatusUnknown, p.Status) - assert.Nil(t, p.Valid, "Valid is omitted for unknown") - assert.Contains(t, p.Error, "server error") + assert.False(t, p.Valid) + assert.NotEmpty(t, p.ValidReason) }) - t.Run("network down -> unknown", func(t *testing.T) { + t.Run("network down -> invalid", func(t *testing.T) { // Start and immediately close the server to simulate a dead host. s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) s.Close() p := loadFromHost(t, s.URL) - assert.Equal(t, profileStatusUnknown, p.Status) - assert.Nil(t, p.Valid) - assert.Contains(t, p.Error, "could not reach host") + assert.False(t, p.Valid) + assert.NotEmpty(t, p.ValidReason) }) t.Run("InvalidConfig -> invalid", func(t *testing.T) { @@ -445,18 +361,15 @@ func TestProfileLoadStatusMatrix(t *testing.T) { p := &profileMetadata{Name: "bad", Host: server.URL} p.Load(t.Context(), configFile, false) - assert.Equal(t, profileStatusInvalid, p.Status) - require.NotNil(t, p.Valid) - assert.False(t, *p.Valid) - assert.Contains(t, p.Error, "fields conflict") + assert.False(t, p.Valid) + assert.Contains(t, p.ValidReason, "fields conflict") }) - t.Run("skip-validate -> unvalidated", func(t *testing.T) { + t.Run("skip-validate -> skipped", func(t *testing.T) { s := statusServer(t, http.StatusOK) p := loadFromHost(t, s.URL, withSkipValidate()) - assert.Equal(t, profileStatusUnvalidated, p.Status) - assert.Nil(t, p.Valid) - assert.Empty(t, p.Error) + assert.False(t, p.Valid) + assert.Contains(t, p.ValidReason, "skip-validate") }) } From c824c55fe419c91c252d460d90c6126051a5e90f Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Wed, 15 Jul 2026 00:12:40 +0200 Subject: [PATCH 3/6] auth profiles: drop per-profile validation timeout (split to follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep this PR focused on the YES/NO/?? + valid_reason output change. The per-profile validation timeout (HTTPTimeoutSeconds/RetryTimeoutSeconds + context.WithTimeout) is a separate fix for a real stall — a host the SDK retries (connection refused, connect/TLS timeout, retriable 5xx) blocks `auth profiles` for ~5 minutes — and will land in its own PR with a regression test. Remove it here along with the network-down test case, which is the only one that depends on the bound to avoid hanging. Co-authored-by: Isaac --- cmd/auth/profiles.go | 21 ++------------------- cmd/auth/profiles_test.go | 9 --------- 2 files changed, 2 insertions(+), 28 deletions(-) diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go index aef9f328475..3c7c0ea698a 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -32,10 +32,6 @@ const ( profileStatusSkipped profileStatus = "skipped" // --skip-validate; not checked ) -// profileValidationTimeout bounds the per-profile validation API call so a -// single hung profile (dead host, no route) does not stall the whole listing. -const profileValidationTimeout = 10 * time.Second - type profileMetadata struct { Name string `json:"name"` Host string `json:"host,omitempty"` @@ -86,21 +82,11 @@ func renderStatusCell(ctx context.Context, s profileStatus) string { } func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipValidate bool) { - timeoutSeconds := int(profileValidationTimeout / time.Second) cfg := &config.Config{ Loaders: []config.Loader{config.ConfigFile}, ConfigFile: configFilePath, Profile: c.Name, DatabricksCliPath: env.Get(ctx, "DATABRICKS_CLI_PATH"), - - // Bound the SDK's per-request and total-retry budgets to the same - // per-profile ceiling. EnsureResolved fetches host metadata via the - // SDK's retrier, which defaults to 5 minutes — without this, a single - // dead host stalls the listing well past the validation context's - // timeout (the EnsureResolved call uses context.Background internally, - // so our context.WithTimeout below cannot reach it). - HTTPTimeoutSeconds: timeoutSeconds, - RetryTimeoutSeconds: timeoutSeconds, } if skipValidate { // EnsureResolved fetches /.well-known/databricks-config to enrich @@ -137,22 +123,19 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV return } - callCtx, cancel := context.WithTimeout(ctx, profileValidationTimeout) - defer cancel() - var err error switch configType { case config.AccountConfig: var a *databricks.AccountClient a, err = databricks.NewAccountClient((*databricks.Config)(cfg)) if err == nil { - _, err = a.Workspaces.List(callCtx) + _, err = a.Workspaces.List(ctx) } case config.WorkspaceConfig: var w *databricks.WorkspaceClient w, err = databricks.NewWorkspaceClient((*databricks.Config)(cfg)) if err == nil { - _, err = w.CurrentUser.Me(callCtx, iam.MeRequest{}) + _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) } case config.InvalidConfig: // Handled above with an early return; listed here for switch exhaustiveness. diff --git a/cmd/auth/profiles_test.go b/cmd/auth/profiles_test.go index 47c8b2830da..05a7ccfa1b8 100644 --- a/cmd/auth/profiles_test.go +++ b/cmd/auth/profiles_test.go @@ -327,15 +327,6 @@ func TestProfileLoadStatusMatrix(t *testing.T) { assert.NotEmpty(t, p.ValidReason) }) - t.Run("network down -> invalid", func(t *testing.T) { - // Start and immediately close the server to simulate a dead host. - s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - s.Close() - p := loadFromHost(t, s.URL) - assert.False(t, p.Valid) - assert.NotEmpty(t, p.ValidReason) - }) - t.Run("InvalidConfig -> invalid", func(t *testing.T) { // Host metadata reporting host_type=unified forces HostType=UnifiedHost. // Without an account_id (or a SPOG-shaped DiscoveryURL), ResolveConfigType From f84278db1fab2b12eeb7339bf51bf849727a67fa Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Wed, 15 Jul 2026 10:12:59 +0200 Subject: [PATCH 4/6] auth profiles: drop redundant expired-token acceptance test Under the simplified YES/NO/?? model, a 401 and a 5xx produce the identical outcome (valid:false + the backend error echoed in valid_reason), so the expired-token test no longer exercises a distinct path from server-error. Keep server-error as the single end-to-end check that valid_reason renders in --output json; remove expired-token. Co-authored-by: Isaac --- .../cmd/auth/profiles/expired-token/out.test.toml | 3 --- .../cmd/auth/profiles/expired-token/output.txt | 15 --------------- acceptance/cmd/auth/profiles/expired-token/script | 10 ---------- .../cmd/auth/profiles/expired-token/test.toml | 15 --------------- 4 files changed, 43 deletions(-) delete mode 100644 acceptance/cmd/auth/profiles/expired-token/out.test.toml delete mode 100644 acceptance/cmd/auth/profiles/expired-token/output.txt delete mode 100644 acceptance/cmd/auth/profiles/expired-token/script delete mode 100644 acceptance/cmd/auth/profiles/expired-token/test.toml diff --git a/acceptance/cmd/auth/profiles/expired-token/out.test.toml b/acceptance/cmd/auth/profiles/expired-token/out.test.toml deleted file mode 100644 index f784a183258..00000000000 --- a/acceptance/cmd/auth/profiles/expired-token/out.test.toml +++ /dev/null @@ -1,3 +0,0 @@ -Local = true -Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/auth/profiles/expired-token/output.txt b/acceptance/cmd/auth/profiles/expired-token/output.txt deleted file mode 100644 index 22c645c9232..00000000000 --- a/acceptance/cmd/auth/profiles/expired-token/output.txt +++ /dev/null @@ -1,15 +0,0 @@ - -=== Expired token: profile is invalid (NO), full error in valid_reason ->>> [CLI] auth profiles --output json -{ - "profiles": [ - { - "name": "expired", - "host": "[DATABRICKS_URL]", - "cloud": "aws", - "auth_type": "pat", - "valid": false, - "valid_reason": "Token is invalid or expired." - } - ] -} diff --git a/acceptance/cmd/auth/profiles/expired-token/script b/acceptance/cmd/auth/profiles/expired-token/script deleted file mode 100644 index d75fefe2cc5..00000000000 --- a/acceptance/cmd/auth/profiles/expired-token/script +++ /dev/null @@ -1,10 +0,0 @@ -sethome "./home" - -cat > "./home/.databrickscfg" < Date: Wed, 15 Jul 2026 10:16:12 +0200 Subject: [PATCH 5/6] auth profiles: refresh stale server-error test comment The comment described the old four-state model (500 -> "unknown" vs "invalid"). Update it to the current model: any validation error is valid=false with the backend error echoed in valid_reason. Co-authored-by: Isaac --- acceptance/cmd/auth/profiles/server-error/test.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acceptance/cmd/auth/profiles/server-error/test.toml b/acceptance/cmd/auth/profiles/server-error/test.toml index d2310dcf2d6..80f6b5aa29b 100644 --- a/acceptance/cmd/auth/profiles/server-error/test.toml +++ b/acceptance/cmd/auth/profiles/server-error/test.toml @@ -2,9 +2,9 @@ Ignore = [ "home" ] -# A 500 from the validation endpoint says nothing about the profile itself — -# the platform is broken. The status should be "unknown", not "invalid", -# so users don't think their credentials are bad. +# A 500 from the validation endpoint fails validation like any other error: +# the profile is reported valid=false with the backend error echoed verbatim +# in valid_reason, so users can see it was the platform, not their credentials. [[Server]] Pattern = "GET /api/2.0/preview/scim/v2/Me" Response.StatusCode = 500 From 628ba41987d61295ad51fb5f752918ec12b60b28 Mon Sep 17 00:00:00 2001 From: Jan Rose Date: Wed, 15 Jul 2026 13:22:19 +0200 Subject: [PATCH 6/6] auth profiles: address review comments - Handle InvalidConfig in its own switch case instead of an early-return if before the switch (per review). - Shorten the changelog fragment and link the PR. Co-authored-by: Isaac --- .nextchanges/cli/auth-profiles-validate.md | 2 +- cmd/auth/profiles.go | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/.nextchanges/cli/auth-profiles-validate.md b/.nextchanges/cli/auth-profiles-validate.md index dac17c91ba1..ab4c679a937 100644 --- a/.nextchanges/cli/auth-profiles-validate.md +++ b/.nextchanges/cli/auth-profiles-validate.md @@ -1 +1 @@ -* `databricks auth profiles` now explains why a profile is not valid. The JSON output keeps the boolean `valid` field (backwards compatible) and adds a `valid_reason` string explaining a `valid: false` result — the full underlying error when validation fails, or a note when it was skipped via `--skip-validate` (empty only on success). The text table's Valid column now renders `YES` (validated), `NO` (validation failed), or `??` (validation skipped via `--skip-validate`). Each profile is validated with a 10s timeout so a single dead host no longer stalls the listing. +* `databricks auth profiles` now explains why a profile is not valid in the JSON output and shows `??` in the Table output when invoked with `--skip-validate` ([#5216](https://github.com/databricks/cli/pull/5216)). diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go index 3c7c0ea698a..7ac964ec65a 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -118,11 +118,6 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV log.Debugf(ctx, "Profile %q: overrode config type from %s to %s (SPOG host)", c.Name, cfg.ConfigType(), configType) } - if configType == config.InvalidConfig { - c.setStatus(ctx, profileStatusInvalid, "profile fields conflict (e.g. workspace and account configured together)") - return - } - var err error switch configType { case config.AccountConfig: @@ -138,7 +133,8 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) } case config.InvalidConfig: - // Handled above with an early return; listed here for switch exhaustiveness. + c.setStatus(ctx, profileStatusInvalid, "profile fields conflict (e.g. workspace and account configured together)") + return } c.Host = cfg.Host