auth: Enrich auth profiles output with validation failure reason#5216
auth: Enrich auth profiles output with validation failure reason#5216janniklasrose wants to merge 7 commits into
Conversation
…th 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
GitHub Actions
Inactive
GitHub Actions
Inactive
Approval status: pending
|
janniklasrose
changed the title
auth profilesauth profiles
…ofile-validate # Conflicts: # NEXT_CHANGELOG.md # acceptance/auth/host-metadata-cache/output.txt # cmd/auth/profiles.go
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
GitHub Actions
Inactive
GitHub Actions
Inactive
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
GitHub Actions
Inactive
GitHub Actions
Inactive
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
GitHub Actions
Inactive
GitHub Actions
Inactive
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
GitHub Actions
Inactive
GitHub Actions
Inactive
- 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
GitHub Actions
Inactive
GitHub Actions
Inactive
janniklasrose
changed the title
auth profiles
| // 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. |
There was a problem hiding this comment.
[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:
- Everything except U2M: the profile is correct and works in its environment.
- U2M: whether there is a token in the cache for that profile. That is, we do not actually validate the profile beyond checking that it exists.
There was a problem hiding this comment.
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.
| func (c *profileMetadata) setStatus(ctx context.Context, status profileStatus, reason string) { | ||
| c.Valid = status == profileStatusValid | ||
| c.ValidReason = reason | ||
| c.StatusDisplay = renderStatusCell(ctx, status) | ||
| } |
There was a problem hiding this comment.
[optional] It feels like we're increasingly mixing data model and rendering concerns. For example, Valid and StatusDisplay seem to be "views" over the status.
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.
There was a problem hiding this comment.
+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
- expired, requiring re-auth
- bad config, requiring fix/
doctor - intermittent issues (can't reach host, etc)
2 participants
Changes
databricks auth profilesnow explains why a profile isn't valid, instead of an overloadedvalid: false.Validcolumn renders three states:YES(green, validated),NO(red, validation failed for any reason), and??(grey, validation skipped via--skip-validate). No extra columns.--output json) — keeps the booleanvalidfield (backwards compatible) and adds avalid_reasonstring that explains avalid: falseresult: the full underlying error when validation failed, or a skip note under--skip-validate.valid_reasonis empty only on success.Why
A single
valid: falsewas overloaded across very different conditions — expired token, no network, 5xx from the workspace, malformed config, missing host. Users seeing redNOeverywhere had no actionable signal. The model here keeps the display dead simple (YES/NO/??) whilevalid_reasonin JSON carries the full detail for anyone who needs to debug or script against it. Every real error isNO;??is reserved for the all-or-nothing--skip-validate, so there's no confusing "NO" on a profile that was never checked.This PR is intentionally scoped to the output change. A separate follow-up adds a per-profile validation timeout —
auth profilescurrently stalls for ~5 minutes on a host the SDK retries (connection refused, connect/TLS timeout, retriable 5xx) — with its own regression test.Tests
cmd/auth/profiles_test.go:TestProfileLoadStatusMatrix— viahttptest: 401, 403, 500, andInvalidConfigall reportvalid: falsewith a non-emptyvalid_reason;--skip-validatereportsvalid: falsewith the skip note.Valid/ValidReason.acceptance/cmd/auth/profiles/server-error/— a 5xx from the validation endpoint producesvalid: falsewith the backend error echoed invalid_reason, exercising the JSON serialization end-to-end.auth/login,auth/logout,auth/switch, andauth/host-metadata-cachetests that includeauth profilesoutput).origin/main; fixed theInvalidConfigfixture for the SDK bump that replaced theexperimental_is_unified_hostflag with host-metadatahost_type.This pull request and its description were written by Isaac, an AI coding agent.