Skip to content

Commit 3a1bb89

Browse files
committed
Address review: trim verbose comments
Make the comments added in this PR more concise while keeping the core rationale (loader docs, the per-call-site precedence notes, test intent, and the acceptance script setup notes).
1 parent d259532 commit 3a1bb89

9 files changed

Lines changed: 93 additions & 147 deletions

File tree

acceptance/cmd/api/profile-overrides-env/script

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
sethome "./home"
22

3-
# A profile carries full credentials; a second profile carries only a host. Both
4-
# point at the test server, while the auth env vars below point elsewhere.
3+
# One profile with full credentials, one host-only; both point at the test
4+
# server while the auth env vars below point elsewhere.
55
cat > "./home/.databrickscfg" <<EOF
66
[my-workspace]
77
host = $DATABRICKS_HOST
@@ -11,8 +11,8 @@ token = $DATABRICKS_TOKEN
1111
host = $DATABRICKS_HOST
1212
EOF
1313

14-
# direnv-style auth env vars pointing at a different (dev) workspace. Before the
15-
# fix for #5096 these shadowed the profile selected with --profile.
14+
# direnv-style auth env vars for a different workspace; before #5096 these
15+
# shadowed the profile selected with --profile.
1616
real_token=$DATABRICKS_TOKEN
1717
export DATABRICKS_HOST=https://dev.cloud.databricks.test
1818
export DATABRICKS_TOKEN=dev-token
@@ -21,8 +21,8 @@ title "api --profile overrides auth env vars (#5096)\n"
2121
MSYS_NO_PATHCONV=1 trace $CLI api get /api/2.0/clusters/list --profile my-workspace
2222
trace print_requests.py --get //api/2.0/clusters/list
2323

24-
# A host-only profile relies on the environment for the credential: the profile
25-
# must win for the host, but the env must still fill the token it omits (#5096).
24+
# Host-only profile: the profile wins for the host, but env fills the token it
25+
# omits (#5096).
2626
export DATABRICKS_TOKEN=$real_token
2727
title "api host-only --profile fills the token from the environment (#5096)\n"
2828
MSYS_NO_PATHCONV=1 trace $CLI api get /api/2.0/clusters/list --profile host-only

acceptance/cmd/auth/describe/profile-overrides-env/script

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,17 @@ token = $DATABRICKS_TOKEN
1010
host = $DATABRICKS_HOST
1111
EOF
1212

13-
# direnv-style auth env vars pointing at a different (dev) workspace. Before the
14-
# fix for #5096 these shadowed the profile selected with --profile.
13+
# direnv-style auth env vars for a different workspace; before #5096 these
14+
# shadowed the profile selected with --profile.
1515
real_token=$DATABRICKS_TOKEN
1616
export DATABRICKS_HOST=https://dev.cloud.databricks.test
1717
export DATABRICKS_TOKEN=dev-token
1818

1919
title "Describe with --profile overrides auth env vars (#5096)\n"
2020
trace $CLI auth describe --profile my-workspace
2121

22-
# A host-only profile relies on the environment for the credential: the profile
23-
# must win for the host, but the env must still fill the token it omits (#5096).
22+
# Host-only profile: the profile wins for the host, but env fills the token it
23+
# omits (#5096).
2424
export DATABRICKS_TOKEN=$real_token
2525
title "Describe with a host-only --profile fills the token from the environment (#5096)\n"
2626
trace $CLI auth describe --profile host-only

bundle/config/workspace.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -178,12 +178,10 @@ func (w *Workspace) Client(ctx context.Context) (*databricks.WorkspaceClient, er
178178

179179
switch {
180180
case w.Profile != "":
181-
// An explicit profile (from --profile or workspace.profile) must take
182-
// precedence over auth environment variables; see
183-
// databrickscfg.ProfileAuthLoaders (#5096). When the bundle sets both
184-
// host and profile the profile now wins for auth, and the
185-
// ValidateConfigAndProfileHost check below still enforces that the
186-
// bundle host matches the profile host.
181+
// An explicit profile (--profile or workspace.profile) takes precedence
182+
// over auth env vars; see databrickscfg.ProfileAuthLoaders (#5096). The
183+
// ValidateConfigAndProfileHost check below still enforces that the bundle
184+
// host matches the profile host.
187185
cfg.Loaders = databrickscfg.ProfileAuthLoaders
188186
case w.Host != "":
189187
// If only the host is configured, we try and unambiguously match it to

bundle/config/workspace_test.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -156,9 +156,8 @@ func TestWorkspaceClientNormalizesHostBeforeProfileResolution(t *testing.T) {
156156
}
157157

158158
func TestWorkspaceClientProfileOverridesAuthEnv(t *testing.T) {
159-
// An explicit profile (from --profile or workspace.profile) must win over
160-
// authentication environment variables, mirroring MustWorkspaceClient.
161-
// See https://github.com/databricks/cli/issues/5096.
159+
// An explicit profile (--profile or workspace.profile) must win over auth
160+
// env vars, mirroring MustWorkspaceClient (#5096).
162161
setupWorkspaceTest(t)
163162

164163
err := databrickscfg.SaveToProfile(t.Context(), &config.Config{
@@ -181,9 +180,8 @@ func TestWorkspaceClientProfileOverridesAuthEnv(t *testing.T) {
181180
}
182181

183182
func TestWorkspaceClientProfileFillsAuthFromEnv(t *testing.T) {
184-
// A host-only profile relies on the environment for credentials. The profile
185-
// must take precedence for the host, but the env must still fill the token
186-
// the profile does not provide (#5096).
183+
// Host-only profile: the profile wins for the host, but env fills the token
184+
// it omits (#5096).
187185
setupWorkspaceTest(t)
188186

189187
err := databrickscfg.SaveToProfile(t.Context(), &config.Config{

cmd/api/api.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,10 @@ func makeCommand(method string) *cobra.Command {
9898
}
9999

100100
if hasProfileFlag {
101-
// An explicit --profile takes precedence over auth environment
102-
// variables; see databrickscfg.ProfileAuthLoaders (#5096). We also
103-
// skip NormalizeDatabricksConfigFromEnv here: with --profile the
104-
// host comes from the profile, not from DATABRICKS_HOST, so
105-
// promoting that env var's ?o=/?a= query params would be wrong.
101+
// An explicit --profile takes precedence over auth env vars; see
102+
// databrickscfg.ProfileAuthLoaders (#5096). NormalizeDatabricksConfigFromEnv
103+
// is skipped too: the host comes from the profile, not DATABRICKS_HOST,
104+
// so promoting its ?o=/?a= query params would be wrong.
106105
cfg.Loaders = databrickscfg.ProfileAuthLoaders
107106
} else {
108107
auth.NormalizeDatabricksConfigFromEnv(cmd.Context(), cfg)

cmd/root/auth.go

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -201,11 +201,10 @@ func MustAccountClient(cmd *cobra.Command, args []string) error {
201201
pr, hasProfileFlag := profileFlagValue(cmd)
202202
if hasProfileFlag {
203203
cfg.Profile = pr
204-
// An explicit --profile takes precedence over auth environment
205-
// variables; see databrickscfg.ProfileAuthLoaders (#5096). We also skip
206-
// NormalizeDatabricksConfigFromEnv here: with --profile the host comes
207-
// from the profile, not from DATABRICKS_HOST, so promoting that env
208-
// var's ?o=/?a= query params would be wrong.
204+
// An explicit --profile takes precedence over auth env vars; see
205+
// databrickscfg.ProfileAuthLoaders (#5096). NormalizeDatabricksConfigFromEnv
206+
// is skipped too: the host comes from the profile, not DATABRICKS_HOST, so
207+
// promoting its ?o=/?a= query params would be wrong.
209208
cfg.Loaders = databrickscfg.ProfileAuthLoaders
210209
} else {
211210
auth.NormalizeDatabricksConfigFromEnv(ctx, cfg)
@@ -331,14 +330,12 @@ func MustWorkspaceClient(cmd *cobra.Command, args []string) error {
331330
profile, hasProfileFlag := profileFlagValue(cmd)
332331
if hasProfileFlag {
333332
cfg.Profile = profile
334-
// An explicit --profile takes precedence over auth environment
335-
// variables; see databrickscfg.ProfileAuthLoaders (#5096). We also skip
336-
// NormalizeDatabricksConfigFromEnv here: with --profile the host comes
337-
// from the profile, not from DATABRICKS_HOST, so promoting that env
338-
// var's ?o=/?a= query params would be wrong. The one edge this drops is
339-
// a host-less profile combined with a SPOG-style DATABRICKS_HOST
340-
// (https://host/?o=123): the workspace_id is no longer extracted from
341-
// the query, an accepted trade-off for that unusual combination.
333+
// An explicit --profile takes precedence over auth env vars; see
334+
// databrickscfg.ProfileAuthLoaders (#5096). NormalizeDatabricksConfigFromEnv
335+
// is skipped too: the host comes from the profile, not DATABRICKS_HOST, so
336+
// promoting its ?o=/?a= query params would be wrong. Trade-off: a host-less
337+
// profile + SPOG-style DATABRICKS_HOST (?o=123) no longer extracts the
338+
// workspace_id from the query.
342339
cfg.Loaders = databrickscfg.ProfileAuthLoaders
343340
} else {
344341
auth.NormalizeDatabricksConfigFromEnv(ctx, cfg)

cmd/root/auth_test.go

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -454,8 +454,8 @@ token = tst-token
454454
require.NoError(t, err)
455455

456456
t.Setenv("DATABRICKS_CONFIG_FILE", configFile)
457-
// direnv-style auth env vars pointing at a different (dev) workspace. Before
458-
// the fix for #5096 these shadowed the profile selected with --profile.
457+
// direnv-style auth env vars pointing at a different workspace. Before #5096
458+
// these shadowed the profile selected with --profile.
459459
t.Setenv("DATABRICKS_HOST", "https://dev.cloud.databricks.test")
460460
t.Setenv("DATABRICKS_TOKEN", "dev-token")
461461

@@ -471,7 +471,7 @@ token = tst-token
471471

472472
w := cmdctx.WorkspaceClient(cmd.Context())
473473
require.NotNil(t, w)
474-
// The explicitly selected profile must win over the auth env vars.
474+
// The selected profile must win over the auth env vars.
475475
assert.Equal(t, "tst-svc", w.Config.Profile)
476476
assert.Equal(t, "https://tst.cloud.databricks.test", w.Config.Host)
477477
assert.Equal(t, "tst-token", w.Config.Token)
@@ -480,10 +480,8 @@ token = tst-token
480480
func TestMustWorkspaceClientProfileFlagFillsAuthFromEnv(t *testing.T) {
481481
testutil.CleanupEnvironment(t)
482482

483-
// A host-only profile relies on the environment for credentials. This is a
484-
// common CI pattern: the host lives in .databrickscfg while DATABRICKS_TOKEN
485-
// is injected by the runner. The profile must take precedence for the host,
486-
// but the env must still fill the token the profile does not provide (#5096).
483+
// Host-only profile + DATABRICKS_TOKEN from env, a common CI pattern: the
484+
// profile wins for the host, but env fills the token it omits (#5096).
487485
configFile := filepath.Join(t.TempDir(), ".databrickscfg")
488486
err := os.WriteFile(configFile, []byte(`
489487
[host-only]
@@ -524,9 +522,8 @@ token = tst-token
524522
require.NoError(t, err)
525523

526524
t.Setenv("DATABRICKS_CONFIG_FILE", configFile)
527-
// The profile is selected via DATABRICKS_CONFIG_PROFILE, not the --profile
528-
// flag. Unlike --profile, this keeps the SDK's default env-first precedence,
529-
// so the auth env vars below must still shadow the profile (#5096).
525+
// Selected via DATABRICKS_CONFIG_PROFILE, not --profile: this keeps the SDK's
526+
// env-first precedence, so the auth env vars below must still win (#5096).
530527
t.Setenv("DATABRICKS_CONFIG_PROFILE", "tst-svc")
531528
t.Setenv("DATABRICKS_HOST", "https://dev.cloud.databricks.test")
532529
t.Setenv("DATABRICKS_TOKEN", "dev-token")
@@ -540,8 +537,8 @@ token = tst-token
540537

541538
w := cmdctx.WorkspaceClient(cmd.Context())
542539
require.NotNil(t, w)
543-
// The profile name is still resolved, but the auth env vars win over its
544-
// host and credentials (env-first precedence for DATABRICKS_CONFIG_PROFILE).
540+
// The profile name is resolved, but the auth env vars win over its host and
541+
// credentials.
545542
assert.Equal(t, "tst-svc", w.Config.Profile)
546543
assert.Equal(t, "https://dev.cloud.databricks.test", w.Config.Host)
547544
assert.Equal(t, "dev-token", w.Config.Token)
@@ -560,7 +557,7 @@ token = tst-token
560557
require.NoError(t, err)
561558

562559
t.Setenv("DATABRICKS_CONFIG_FILE", configFile)
563-
// Auth env vars pointing at a different account host. The profile must win.
560+
// Auth env vars for a different account host; the profile must win.
564561
t.Setenv("DATABRICKS_HOST", "https://accounts.dev.databricks.test")
565562
t.Setenv("DATABRICKS_TOKEN", "dev-token")
566563

@@ -573,7 +570,7 @@ token = tst-token
573570

574571
a := cmdctx.AccountClient(cmd.Context())
575572
require.NotNil(t, a)
576-
// The explicitly selected profile must win over the auth env vars.
573+
// The selected profile must win over the auth env vars.
577574
assert.Equal(t, "acc-tst", a.Config.Profile)
578575
assert.Equal(t, "https://accounts.cloud.databricks.test", a.Config.Host)
579576
assert.Equal(t, "tst-token", a.Config.Token)

libs/databrickscfg/loader.go

Lines changed: 38 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -14,50 +14,31 @@ import (
1414

1515
var ResolveProfileFromHost = profileFromHostLoader{}
1616

17-
// ResolveNonAuthFromEnv reads configuration from environment variables, except
18-
// for the host and any authentication credential. It runs before the config
19-
// file loader when the user has explicitly selected a profile (via the
20-
// --profile flag or workspace.profile), so that the profile takes precedence
21-
// over auth environment variables.
22-
//
23-
// The SDK's default loader order is environment first, then config file, and a
24-
// loader never overwrites a field that is already set. As a result auth env
25-
// vars (DATABRICKS_HOST, DATABRICKS_TOKEN, ...) shadow the selected profile.
26-
// Skipping them here lets the subsequent config-file loader populate host and
27-
// auth from the profile instead. A trailing config.ConfigAttributes loader can
28-
// still fill auth fields the profile leaves empty (e.g. a host-only profile
29-
// combined with DATABRICKS_TOKEN). See
30-
// https://github.com/databricks/cli/issues/5096.
17+
// ResolveNonAuthFromEnv reads config from environment variables, except for the
18+
// host and any auth credential. It runs before the config file loader when a
19+
// profile is explicitly selected, so the profile wins over auth env vars: the
20+
// SDK's default chain reads env before the config file and never overwrites a
21+
// set field, which would otherwise let env vars shadow the profile (#5096).
3122
var ResolveNonAuthFromEnv = nonAuthEnvLoader{}
3223

33-
// ProfileAuthLoaders is the SDK loader chain to use when the user has
34-
// explicitly selected a profile (via the --profile flag or a bundle's
35-
// workspace.profile). It is the single source of truth for that precedence
36-
// rule; call sites should reference it rather than restating the rationale.
37-
//
38-
// The selected profile must determine the host, routing identifiers, and
39-
// authentication, taking precedence over the matching environment variables
40-
// (DATABRICKS_HOST, DATABRICKS_TOKEN, ...). The SDK's default chain reads the
41-
// environment before the config file and never overwrites an already-set
42-
// field, so the env vars would otherwise shadow the profile (issue #5096).
24+
// ProfileAuthLoaders is the loader chain to use when a profile is explicitly
25+
// selected (--profile or a bundle's workspace.profile), and the single source
26+
// of truth for that precedence rule. The profile must win for host, routing and
27+
// auth over the matching env vars (DATABRICKS_HOST, DATABRICKS_TOKEN, ...),
28+
// which the SDK's default env-first chain would otherwise let shadow it (#5096).
4329
//
44-
// Note: this intentionally only governs an explicitly selected profile. A
45-
// profile picked up from DATABRICKS_CONFIG_PROFILE keeps the SDK's default
46-
// env-first precedence, because reordering precedence between two environment
47-
// signals (DATABRICKS_CONFIG_PROFILE vs DATABRICKS_HOST) is the SDK's domain;
48-
// we only override when the profile is named out-of-band (the --profile flag
49-
// or a bundle's workspace.profile).
30+
// This only governs an explicitly selected profile. One picked up from
31+
// DATABRICKS_CONFIG_PROFILE keeps env-first precedence: reordering two
32+
// environment signals (DATABRICKS_CONFIG_PROFILE vs DATABRICKS_HOST) is the
33+
// SDK's domain; we only override when the profile is named out-of-band.
5034
//
51-
// The order matters:
52-
// 1. ResolveNonAuthFromEnv loads non-auth, non-routing attributes from the
53-
// environment (e.g. cluster_id), preserving env-wins precedence for those.
54-
// 2. ConfigFile loads the selected profile, populating host, routing and auth.
55-
// 3. ConfigAttributes loads any remaining attributes from the environment,
56-
// filling fields the profile did not provide (e.g. a host-only profile
57-
// combined with DATABRICKS_TOKEN). It never overwrites a value the profile
58-
// already set, so the profile still wins for #5096. This gap-fill is a
59-
// deliberate, tested contract (host-only profiles are a common CI pattern
60-
// where the credential is injected via the environment).
35+
// Order:
36+
// 1. ResolveNonAuthFromEnv: non-auth, non-routing env attrs (e.g. cluster_id),
37+
// keeping env-wins precedence for those.
38+
// 2. ConfigFile: the selected profile (host, routing, auth).
39+
// 3. ConfigAttributes: gap-fills only fields the profile left empty (e.g. a
40+
// host-only profile + DATABRICKS_TOKEN, a common CI pattern); it never
41+
// overwrites a profile value, so the profile still wins.
6142
var ProfileAuthLoaders = []config.Loader{
6243
ResolveNonAuthFromEnv,
6344
config.ConfigFile,
@@ -110,38 +91,25 @@ func findMatchingProfile(configFile *config.File, matcher func(*ini.Section) boo
11091
return matching[0], nil
11192
}
11293

113-
// nonAuthEnvSkipAttrs lists SDK config attribute names that nonAuthEnvLoader
114-
// must not read from the environment, beyond those caught by HasAuthAttribute.
115-
//
116-
// Criterion: an attribute belongs here if it identifies the target
117-
// workspace/account (host, routing IDs) or selects/steers the authentication
118-
// method, but the SDK does NOT tag it `auth:"..."` (so HasAuthAttribute can't
119-
// catch it). The SDK collapses an `auth:"-"` tag to an empty auth tag (marking
120-
// the field "internal"), which is why these auth-steering fields slip past
121-
// HasAuthAttribute and must be listed explicitly. Leaving any of them to the
122-
// environment would let the matching env var shadow the selected profile, the
123-
// same bug as #5096. Skipping here only changes precedence: the trailing
124-
// ConfigAttributes loader still fills any of these the profile leaves empty
125-
// from the environment (the same gap-fill host and credentials get).
94+
// nonAuthEnvSkipAttrs lists SDK config attributes nonAuthEnvLoader must not read
95+
// from the environment, beyond those caught by HasAuthAttribute. They identify
96+
// the target workspace/account (host, routing IDs) or steer the auth method but
97+
// are tagged auth:"-" (collapsed to Internal), so HasAuthAttribute misses them;
98+
// leaving them to env would let an env var shadow the selected profile (#5096).
99+
// Skipping only changes precedence: the trailing ConfigAttributes loader still
100+
// gap-fills any the profile leaves empty.
126101
//
127-
// - host: has no `auth` struct tag at all.
128-
// - workspace_id (DATABRICKS_WORKSPACE_ID): routing identifier; a profile
129-
// that sets it must win, or a stray env var routes the profile's
130-
// credentials to a different workspace.
131-
// - account_id (DATABRICKS_ACCOUNT_ID): account routing identifier, same
132-
// reasoning as workspace_id.
133-
// - auth_type (DATABRICKS_AUTH_TYPE): forces a specific auth method.
134-
// - discovery_url (DATABRICKS_DISCOVERY_URL): redirects OIDC discovery.
135-
// - audience (DATABRICKS_TOKEN_AUDIENCE): selects the token audience for
136-
// OIDC/workload-identity flows.
137-
// - cloud (DATABRICKS_CLOUD): steers cloud-specific auth (Azure/GCP/AWS).
102+
// - host: no `auth` tag at all.
103+
// - workspace_id / account_id: routing identifiers; an env var must not route
104+
// the profile's credentials elsewhere.
105+
// - auth_type: forces a specific auth method.
106+
// - discovery_url: redirects OIDC discovery.
107+
// - audience: selects the OIDC/workload-identity token audience.
108+
// - cloud: steers cloud-specific auth (Azure/GCP/AWS).
138109
//
139-
// Non-auth env-backed attributes tagged `auth:"-"` (e.g. oauth_callback_port,
140-
// debug_headers, rate_limit) are intentionally NOT skipped: they don't change
141-
// which credentials authenticate the request or where it is routed, so
142-
// env-wins precedence is fine. TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs
143-
// guards that every auth-steering internal attribute stays classified across
144-
// SDK bumps.
110+
// Non-auth attrs tagged auth:"-" (oauth_callback_port, debug_headers, ...) are
111+
// intentionally not skipped; TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs
112+
// guards that every auth-steering internal attribute stays classified.
145113
var nonAuthEnvSkipAttrs = map[string]bool{
146114
"host": true,
147115
"workspace_id": true,

0 commit comments

Comments
 (0)