-
Notifications
You must be signed in to change notification settings - Fork 200
auth: Enrich auth profiles output with validation failure reason #5216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
a6515c9
2466de9
f43cf53
c824c55
f84278d
1342969
628ba41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +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. | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| sethome "./home" | ||
|
|
||
| cat > "./home/.databrickscfg" <<EOF | ||
| [transient] | ||
| host = ${DATABRICKS_HOST} | ||
| token = test-token | ||
| EOF | ||
|
|
||
| title "5xx from validation endpoint: profile is invalid (NO), full error in valid_reason" | ||
| trace $CLI auth profiles --output json |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| Ignore = [ | ||
| "home" | ||
| ] | ||
|
|
||
| # 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 | ||
| Response.Body = ''' | ||
| { | ||
| "error_code": "INTERNAL_ERROR", | ||
| "message": "Internal server error" | ||
| } | ||
| ''' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,21 +21,66 @@ 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"` | ||
| 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. | ||
|
Comment on lines
+43
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [optional] I understand this is out of scope for this PR but it would be great to explicit about what "Valid" means. E.g. is it about the schema? The ability to authenticate? Whether a token is in the cache? IIUC, a profile is valid if it is possible to make a valid API call. I wonder if this creates a double standard:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see also my other reply. Valid YES/true currently means "able to use the profile for other authenticated calls as-is". I don't know if that is how customers actually interpret it, but that's the only thing our views let them reasonably infer. Adding ValidReason to the table view seemed really clunky UX (or requires custom logic to handle all sorts of errors and show them nicely) hence it's only in json output where we can dump the error. |
||
| 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) | ||
| } | ||
|
Comment on lines
+64
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [optional] It feels like we're increasingly mixing data model and rendering concerns. For example, I don't think we need to act on this as part of this PR. Though, we will have to decouple this as part of centralizing the profile read/write logic in a single package.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 - working with what we have right now, we currently only expose the boolean status to customers ("Am I able to use this profile?"). Initially this PR was prompted by intermittent/cryptic failures where I didn't know why I saw "Valid? NO". Would be great if the centralised logic could differentiate
|
||
|
|
||
| // 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,39 @@ 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 | ||
| } | ||
|
janniklasrose marked this conversation as resolved.
Outdated
|
||
|
|
||
| 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 | ||
| var w *databricks.WorkspaceClient | ||
| w, err = databricks.NewWorkspaceClient((*databricks.Config)(cfg)) | ||
| if err == nil { | ||
| _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) | ||
| } | ||
| _, err = w.CurrentUser.Me(ctx, iam.MeRequest{}) | ||
| c.Host = cfg.Host | ||
| c.AuthType = cfg.AuthType | ||
| if err != nil { | ||
| return | ||
| } | ||
| c.Valid = true | ||
| case config.InvalidConfig: | ||
| // Invalid configuration, skip validation | ||
| // Handled above with an early return; listed here for switch exhaustiveness. | ||
| } | ||
|
|
||
| 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 +160,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}}`), | ||
| }, | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.