diff --git a/.nextchanges/cli/auth-profiles-validate.md b/.nextchanges/cli/auth-profiles-validate.md new file mode 100644 index 00000000000..ab4c679a937 --- /dev/null +++ b/.nextchanges/cli/auth-profiles-validate.md @@ -0,0 +1 @@ +* `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/acceptance/cmd/auth/profiles/output.txt b/acceptance/cmd/auth/profiles/output.txt index 2dd737198d6..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": "", - "valid": false + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" }, { "name": "account-profile", @@ -15,7 +16,8 @@ "account_id": "test-account-123", "cloud": "aws", "auth_type": "", - "valid": false + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" }, { "name": "unified-profile", @@ -24,7 +26,8 @@ "workspace_id": "[NUMID]", "cloud": "aws", "auth_type": "", - "valid": false + "valid": false, + "valid_reason": "validation skipped (--skip-validate)" } ] } diff --git a/acceptance/cmd/auth/profiles/server-error/out.test.toml b/acceptance/cmd/auth/profiles/server-error/out.test.toml new file mode 100644 index 00000000000..f784a183258 --- /dev/null +++ b/acceptance/cmd/auth/profiles/server-error/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/auth/profiles/server-error/output.txt b/acceptance/cmd/auth/profiles/server-error/output.txt new file mode 100644 index 00000000000..2e2b5abe024 --- /dev/null +++ b/acceptance/cmd/auth/profiles/server-error/output.txt @@ -0,0 +1,15 @@ + +=== 5xx from validation endpoint: profile is invalid (NO), full error in valid_reason +>>> [CLI] auth profiles --output json +{ + "profiles": [ + { + "name": "transient", + "host": "[DATABRICKS_URL]", + "cloud": "aws", + "auth_type": "pat", + "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 new file mode 100644 index 00000000000..4e4344aa0ff --- /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 8c317293322..7ac964ec65a 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -21,6 +21,17 @@ import ( "gopkg.in/ini.v1" ) +// 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" // validation succeeded + profileStatusInvalid profileStatus = "invalid" // validation failed (any error) + profileStatusSkipped profileStatus = "skipped" // --skip-validate; not checked +) + type profileMetadata struct { Name string `json:"name"` Host string `json:"host,omitempty"` @@ -28,14 +39,48 @@ type profileMetadata struct { 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:"-"` } func (c *profileMetadata) IsEmpty() bool { return c.Host == "" && c.AccountID == "" } +// 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: +// 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, "YES") + case profileStatusInvalid: + return cmdio.Red(ctx, "NO") + case profileStatusSkipped: + return cmdio.HiBlack(ctx, "??") + } + return cmdio.HiBlack(ctx, "??") +} + func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipValidate bool) { cfg := &config.Config{ Loaders: []config.Loader{config.ConfigFile}, @@ -64,6 +109,7 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV if skipValidate { c.Host = cfg.CanonicalHostName() c.AuthType = cfg.AuthType + c.setStatus(ctx, profileStatusSkipped, "validation skipped (--skip-validate)") return } @@ -72,35 +118,35 @@ 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) } + var err error switch configType { case config.AccountConfig: - a, err := databricks.NewAccountClient((*databricks.Config)(cfg)) - if err != nil { - return - } - _, err = a.Workspaces.List(ctx) - c.Host = cfg.Host - c.AuthType = cfg.AuthType - if err != nil { - return + var a *databricks.AccountClient + a, err = databricks.NewAccountClient((*databricks.Config)(cfg)) + if err == nil { + _, err = a.Workspaces.List(ctx) } - c.Valid = true case config.WorkspaceConfig: - w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg)) - if err != nil { - return - } - _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) - c.Host = cfg.Host - c.AuthType = cfg.AuthType - if err != nil { - return + var w *databricks.WorkspaceClient + w, err = databricks.NewWorkspaceClient((*databricks.Config)(cfg)) + if err == nil { + _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) } - c.Valid = true case config.InvalidConfig: - // Invalid configuration, skip validation + c.setStatus(ctx, profileStatusInvalid, "profile fields conflict (e.g. workspace and account configured together)") + return + } + + c.Host = cfg.Host + c.AuthType = cfg.AuthType + + // 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 { @@ -110,7 +156,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 2ebbeed0e60..05a7ccfa1b8 100644 --- a/cmd/auth/profiles_test.go +++ b/cmd/auth/profiles_test.go @@ -47,6 +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.False(t, profile.Valid, "Valid should be false when validation is skipped") + assert.Contains(t, profile.ValidReason, "skip-validate") } // TestProfileLoadSkipValidateMakesNoRequests guards the --skip-validate @@ -75,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.False(t, p.Valid) + assert.False(t, p.Valid, "Valid should be false when validation is skipped") } func TestProfilesDefaultMarker(t *testing.T) { @@ -169,33 +171,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, }, } @@ -224,7 +221,8 @@ func TestProfileLoadSPOGConfigType(t *testing.T) { } p.Load(t.Context(), configFile, false) - assert.Equal(t, tc.wantValid, p.Valid, "Valid mismatch") + 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") }) @@ -284,3 +282,114 @@ func TestProfileLoadNoDiscoveryStaysWorkspace(t *testing.T) { assert.NotEmpty(t, p.Host) assert.Equal(t, "pat", p.AuthType) } + +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 + } + + // 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.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.False(t, p.Valid) + assert.NotEmpty(t, p.ValidReason) + }) + + t.Run("500 -> invalid", func(t *testing.T) { + s := statusServer(t, http.StatusInternalServerError) + 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 + // can't pick a side and falls through to InvalidConfig. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/databricks-config" { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"host_type": "unified"}) + return + } + 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 + "\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.False(t, p.Valid) + assert.Contains(t, p.ValidReason, "fields conflict") + }) + + t.Run("skip-validate -> skipped", func(t *testing.T) { + s := statusServer(t, http.StatusOK) + p := loadFromHost(t, s.URL, withSkipValidate()) + assert.False(t, p.Valid) + assert.Contains(t, p.ValidReason, "skip-validate") + }) +} + +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 +}