Preserve profile fields on re-login instead of wiping all keys#4597
Open
simonfaltum wants to merge 2 commits intomainfrom
Open
Preserve profile fields on re-login instead of wiping all keys#4597simonfaltum wants to merge 2 commits intomainfrom
simonfaltum wants to merge 2 commits intomainfrom
Conversation
SaveToProfile previously deleted all keys in a profile section before writing, destroying fields like cluster_id, warehouse_id, scopes, and custom keys on every `auth login` or `configure` invocation. Switch to merge semantics: existing keys not mentioned in the new config are preserved. Add a clearKeys variadic parameter so callers can explicitly remove incompatible keys on auth-type transitions (e.g., OAuth login clears PAT/M2M/Azure/GCP credentials; PAT configure clears auth_type and OAuth metadata). Handle boolean zero-value for experimental_is_unified_host by clearing it explicitly when false. Scopes are preserved across re-logins in both auth login and inline login (auth token): when --scopes is not passed, the existing profile's scopes are read back and used for the OAuth challenge, keeping the profile truthful and the user's preference intact. Non-auth config properties like azure_environment are preserved. Clear serverless_compute_id when cluster_id is set (via flag or env). Clear experimental_is_unified_host and databricks_cli_path during PAT configure.
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 738f27e
15 interesting tests: 7 KNOWN, 7 SKIP, 1 RECOVERED
Top 34 slowest tests (at least 2 minutes):
|
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| // auth-metadata fields from other auth methods that are incompatible with | ||
| // auth_type=databricks-cli. Identity fields (host, account_id, workspace_id) | ||
| // and client settings (http_timeout_seconds, etc.) are NOT cleared. | ||
| func oauthLoginClearKeys() []string { |
Contributor
There was a problem hiding this comment.
This list can evolve if we support more auth in the future. That was probably the rationale behind nuking everything before and having an allow list. Not a blocker just wanted to flag this.
| // when saving a PAT-based profile via `databricks configure`. This prevents | ||
| // stale auth credentials from other methods (OAuth, Azure, GCP, etc.) from | ||
| // remaining in the profile and causing multi-auth validation failures. | ||
| var patConfigureClearKeys = []string{ |
Contributor
There was a problem hiding this comment.
We have auth.ConfigAttrs in SDK that encapsulate all auth fields. Is that something we can leverage here to make this more robust?
For example, here's what we do to clear env vars in the CLI:
// EnvVars returns the list of environment variables that the SDK reads to configure
// authentication.
// This is useful for spawning subprocesses since you can unset all auth environment
// variables to clean up the environment before configuring authentication for the
// child process.
func envVars() []string {
var out []string
for _, attr := range config.ConfigAttributes {
if len(attr.EnvVars) == 0 {
continue
}
out = append(out, attr.EnvVars...)
}
return out
}
Contributor
There was a problem hiding this comment.
Fields that are part of a authentication method like pat are annotated as such.
| os.Stdin = inp | ||
|
|
||
| cmd := cmd.New(ctx) | ||
| cmd.SetArgs([]string{"configure", "--token", "--host", "https://host"}) |
Contributor
There was a problem hiding this comment.
You can just extend the existing acceptance test rather than having these separate tests.
Contributor
There was a problem hiding this comment.
I think these tests could be made table tests? The test data will admittedly be a little big but it will make the tests easier to read and maintain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
SaveToProfiledeleted every key in a.databrickscfgprofile section before writing, then only wrote back the small subset of fields the caller explicitly set. This meant everydatabricks auth loginordatabricks configuresilently destroyed fields likecluster_id,warehouse_id,scopes,azure_environment, and any custom keys the user had added to their profile.This is especially problematic as profiles carry more explicit fields in the host-agnostic auth work (
workspace_id,account_id,azure_environment). Users shouldn't have to re-specify everything every time they re-login.Changes
Core:
SaveToProfilemerge semantics (libs/databrickscfg/ops.go)Before: Delete all keys in the section, then write only non-zero fields from the new config.
After: Existing keys not mentioned in the new config are preserved. Non-zero fields from the new config overwrite existing values. A new
clearKeys ...stringvariadic parameter lets callers explicitly remove specific keys.auth login(cmd/auth/login.go)Before: Re-login wiped everything.
cluster_idandserverless_compute_idwere manually read back from the existing profile in the default case (no--configure-cluster/--configure-serverless), butwarehouse_id,azure_environment, custom keys, etc. were always lost.After:
--configure-clusterexplicitly clearsserverless_compute_id(and vice versa) for mutual exclusion.experimental_is_unified_hostis explicitly cleared whenfalse(sincefalseis a zero value and would otherwise be skipped by the merge, leaving a staletruefrom a previous login).--scopesis not passed, existing scopes from the profile are read back and used for the OAuth challenge. This means the minted token matches the profile's scope configuration. Previously, scopes were always wiped and the defaultall-apiswas used.Inline login in
auth token(cmd/auth/token.go)Before:
runInlineLogin(the "create new profile" path in the interactiveauth tokenflow) saved a minimal set of fields and wiped everything else. Did not handle scopes orexperimental_is_unified_hostclearing.After:
auth login.experimental_is_unified_hostexplicitly cleared whenfalse.auth login).databricks configure(cmd/configure/configure.go)Before: PAT configure wiped all keys including
auth_type,scopes,azure_environment, etc. — which was correct forauth_type/scopesbut destroyed useful non-auth fields.After:
cluster_id,warehouse_id,azure_environment,account_id,workspace_id, custom keys) are preserved.auth_type,scopes,databricks_cli_path) is explicitly cleared — a PAT profile shouldn't keep OAuth artifacts.experimental_is_unified_hostis always cleared (PAT profiles don't use unified hosts).serverless_compute_idis cleared whencluster_idis set — whether via--configure-clusterflag or viaDATABRICKS_CLUSTER_IDenv var (previously only the flag was checked).Profile struct (
libs/databrickscfg/profile/)Scopesfield toprofile.Profilestruct and read it from the INI file inLoadProfiles. This allowsauth loginandauth tokento read existing scopes back from the profile.Test plan
Unit tests (
libs/databrickscfg/ops_test.go):TestSaveToProfile_MergePreservesExistingKeys— token survives when only auth_type is writtenTestSaveToProfile_ClearKeysRemovesSpecifiedKeys— token and cluster_id cleared, serverless_compute_id addedTestSaveToProfile_OverwritesExistingValues— host updated from old to newTestSaveToProfile_ClearKeysOnNonExistentKeyIsNoop— clearing nonexistent keys doesn't errorUnit tests (
cmd/configure/configure_test.go):TestConfigureClearsOAuthAuthType— PAT configure clearsauth_typeandscopesfrom a previously OAuth-configured profileTestConfigureClearsUnifiedHostMetadata— PAT configure clearsexperimental_is_unified_hostwhile preservingaccount_id/workspace_idTestConfigureClearsServerlessWhenClusterFromEnv—serverless_compute_idcleared whenDATABRICKS_CLUSTER_IDenv var provides cluster_idAcceptance test (
acceptance/cmd/auth/login/preserve-fields/):cluster_id,warehouse_id,azure_environment, andcustom_key→ all four surviveauth loginre-login without--configure-cluster/--configure-serverlessExisting tests:
auth loginacceptance tests pass (includingconfigure-serverlesswhich verifiescluster_idis still cleared)cmd/auth/unit tests passcmd/configure/unit tests passmake checkscleanmake lintfullclean (0 issues)🤖 Generated with Claude Code