Repository navigation
Add opt-in strict profile matching for bundle targets - #7109
Conversation
When DATABRICKS_BUNDLE_STRICT_PROFILE_MATCH is set, the target's host and workspace ID must match exactly one profile in the config file, and that profile is used even if credentials are set in the environment. A profile without workspace_id only matches targets without one. A missing host, no match, several matches, or a different workspace.profile or --profile fails before authentication. Co-authored-by: Isaac <no-reply@databricks.com>
Account profiles on a shared host have no workspace_id and could match a target without one, so strict matching now only considers workspace profiles. The acceptance test also pins that [DEFAULT] counts like any other profile and that a missing config file fails. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 505a230
Top 3 slowest tests (at least 2 minutes):
|
Substitute only the test variables in the templates, so the bundle script expands $CLI at run time instead of getting a Windows path with backslashes. Use $TEST_TMP_DIR in traced commands, because $PWD is a POSIX path on Windows and is not masked in the output. Co-authored-by: Isaac <no-reply@databricks.com>
|
Commit: Regression test reportSelected 7 changed tests (limit=20, 0 not selected) 14 tests: regression=12 unreleased=0 coverage=2 failing=0
TestAccept/bundle/strict_profile_match/config_file/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/config_file/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/default_profile/host/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/default_profile/host/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/default_profile/no_host/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/default_profile/no_host/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/host/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/host/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/no_host/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/no_host/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/profile/DMS=: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) TestAccept/bundle/strict_profile_match/profile/DMS=true: branch ✅ | main (be6a8e1) ❌ | latest (v1.21.0) ❌main (be6a8e1): latest (v1.21.0): same output as main (be6a8e1) |
The acceptance envsubst.py substitutes every variable and ignores the variable list, so on Windows the bundle script still got the CLI path with backslashes. Move the script to an included scripts.yml that is not substituted, so bash expands $CLI at run time. Co-authored-by: Isaac <no-reply@databricks.com>
| configureStrictProfile(cmd.Context(), b, profile) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
If this is integrated in the series of loaders it can be the first one to run and fail hard if there is no match.
| trace errcode env DATABRICKS_CONFIG_FILE=$TEST_TMP_DIR/account.databrickscfg $CLI bundle validate -t custom_host_without_workspace_id | ||
|
|
||
| title "Strict mode, a missing config file is rejected" | ||
| trace errcode env DATABRICKS_CONFIG_FILE=$TEST_TMP_DIR/missing.databrickscfg $CLI bundle validate -t classic |
There was a problem hiding this comment.
This acc test is pretty hard to review.
Is there a logical separation to make for different categories of failure modes, and turn them into separate acc tests, or is this not worth the squeeze?
There was a problem hiding this comment.
Split it into 5 tests: disabled, host, profile, no_host, config_file.
A target without a host takes its profile from --profile or DATABRICKS_CONFIG_PROFILE, then workspace.profile, then [__settings__].default_profile, the same order as without strict mode. The profile must be a workspace profile in the config file, and if the target sets a workspace ID, the profile must have it. This lets a caller that writes the config file run bundles without a host without passing a profile. Co-authored-by: Isaac <no-reply@databricks.com>
Strict profile matching is now a loader next to ResolveProfileFromHost, so there is one mechanism that selects a profile. When strict mode is on, Workspace.Client uses ProfileAuthLoaders with the strict loader before the config file. The loader does the same host and workspace_id lookup as ResolveProfileFromHost, but always runs and fails when there is no single match. Targets without a host use the profile that configureProfile already selects from --profile, workspace.profile or default_profile. Split the acceptance test into separate tests for the disabled mode, targets with a host, explicit profiles, targets without a host and config file cases. Co-authored-by: Isaac <no-reply@databricks.com>
With [__settings__].default_profile set, targets without a host use the default profile and must agree with its workspace ID, while targets with a host still match by host and workspace ID and ignore the default. Co-authored-by: Isaac <no-reply@databricks.com>
Follow-up to #7105. The comments on #6989 and #7109 were 34-44 KB, close to the 60 KB cap, mostly because a new test fails the same way on main and on the latest release and the same diff was printed twice. - A failing run whose output equals an earlier column's (ignoring the random temp dir) now says "same output as <column>". - When the comment is over the cap, whole failure details are dropped (with a count) instead of cutting one in the middle and leaving its markup open. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
pietern
left a comment
There was a problem hiding this comment.
Please add unit tests to loader_test.go like the existing ones.
It helps to see the specific cases side by side and I think it would have helped surface the error returned for empty configs (this should not error).
|
|
||
| // StrictProfileLoaders is ProfileAuthLoaders with strictProfileLoader before | ||
| // the config file, so the profile always comes from the config file, even when | ||
| // auth env vars are set. |
There was a problem hiding this comment.
This comment should be more specific. The intent is that the profile name always comes from the databrickscfg config file, based on the workspace host and workspace ID in the bundle configuration.
| // one that the profile must have. | ||
| func checkNamedProfile(cfg *config.Config, configFile *config.File) error { | ||
| if cfg.Profile == "" { | ||
| return errors.New("strict profile matching: set workspace.host, or select a profile with --profile, workspace.profile or [__settings__].default_profile") |
There was a problem hiding this comment.
This means a hard failure if the neither the host nor the profile are set in the bundle configuration.
I thought it would have to fall through to the default profile in this case.
The strict matching only kicks in when you have a host and/or a workspace ID in the incoming config.
There was a problem hiding this comment.
I think there was a misunderstanding. Fixed. If there is no host or workspace id it falls through like in non-strict mode.
| if err != nil || sectionValue(s, hostAttr) == "" || !isWorkspaceSection(s) { | ||
| return fmt.Errorf("strict profile matching: %s is not a workspace profile with a host in the config file", cfg.Profile) | ||
| } | ||
| if cfg.WorkspaceID != "" && sectionValue(s, "workspace_id") != cfg.WorkspaceID { |
There was a problem hiding this comment.
We don't check the workspace host here?
There was a problem hiding this comment.
There's no host in the bundle config in this branch, so there is nothing to compare.
if cfg.Host == "" {
return checkNamedProfile(cfg, configFile)
}
Added a comment there.
|
|
||
| cfg := w.Config(ctx) | ||
|
|
||
| _, strict := env.StrictProfileMatch(ctx) |
There was a problem hiding this comment.
The value is ignored, meaning false or 0 still enabled it.
IIRC there is precedence for interpreting the value as boolish.
There was a problem hiding this comment.
Good catch. Updated it to use env.GetBool.
…e ID A target without a host or workspace ID has nothing to match, so it now resolves as without strict mode instead of failing. The loader also returns nil in that case. Read DATABRICKS_BUNDLE_STRICT_PROFILE_MATCH as a boolean, so "0" or "false" leave strict mode off. Add unit tests for the strict loader next to the existing loader tests. Co-authored-by: Isaac <no-reply@databricks.com>
## Release v1.22.0 ### CLI * Fix `databricks aitools install` overwriting newer raw skills with an older pinned release. CLI 1.22.0 and newer now install the latest published skills and prevent automatic downgrades. ([#7069](#7069)) * Fix `auth login --timeout` not applying to the browser login, which waited for the OAuth redirect indefinitely. ([#7103](#7103)) * Remove the 50-workspace limit in `auth login` and support searching workspaces by name or ID. ([#7071](#7071)) * `databricks ssh connect` now fails right away with the workspace's reason when port forwarding to serverless compute is disabled, instead of waiting out the server start timeout. ([#7057](#7057)) ### AI Runtime * `air run` now spends less time before submitting: it fetches only the user name from SCIM. ([#7153](#7153)) * Align multi-image container name validation with backend services: allow hyphens and up to 32 characters, and reject uppercase letters and underscores. ([#7154](#7154)) ### Bundles * The `compute_id` deprecation warning for a target now shows the full path (`targets.<name>.compute_id`). ([#7048](#7048)) * Add `DATABRICKS_BUNDLE_STRICT_PROFILE_MATCH`, which requires a target's host and workspace ID to match exactly one profile in the config file before authenticating. Targets without a host or workspace ID are not affected. ([#7109](#7109)) * Zero-value overrides of `presets`, `lifecycle.prevent_destroy` and `deployment` settings in includes now replace non-zero base values, and `bundle validate -o json` shows explicit zero presets. ([#7117](#7117)) * `bundle deployment bind` and `unbind` now accept a resource as `type.key`, e.g. `jobs.foo`. ([#7128](#7128)) * Retry more transient server errors during `bundle deploy` and `bundle destroy`, including when releasing the deploy lock and reading state. ([#7129](#7129)) * Fix migration from Terraform failing when a deployed resource references a resource that was never deployed. ([#7157](#7157)) * Add `app-appkit` to bundle templates to simplify starting a Databricks Apps project. ([#7004](#7004), [#7007](#7007)) * Add support for the `features` resource type. ([#6952](#6952)) * Fix `bundle validate` warning that `sync.include` and `sync.exclude` patterns do not match any files when `sync.paths` includes a directory outside the bundle root. ([#7122](#7122)) ### Dependency Updates * Bump dependencies with known vulnerabilities. ([#7083](#7083)) * Bump Go to 1.27.2. Release binaries now require macOS 13 Ventura or later. ([#7052](#7052)) * Update the Go SDK to [v0.189.0](https://github.com/databricks/databricks-sdk-go/releases/tag/v0.189.0). ([#7138](#7138)) * Bump Go toolchain to 1.27.3. ([#7116](#7116))
Changes
Add
DATABRICKS_BUNDLE_STRICT_PROFILE_MATCH, read as a boolean. When it is true and the target has a host or workspace ID, a strict loader runs before the config file, so the profile name always comes from the config file, even ifDATABRICKS_TOKENis set. The host and workspace ID must match exactly one workspace profile, and a profile withoutworkspace_idonly matches targets without one. A target with a workspace ID but no host must name a profile with that ID. A target with neither resolves as without strict mode.The loader does the same lookup as
ResolveProfileFromHost, but fails when there is no single match, a different profile is selected, or the file is missing.Why
With env auth the CLI skips host-based profile matching, so the token goes to whatever host the target sets. A caller that passes its own token and a file of trusted profiles needs the target checked against them first, including on a custom host shared by several workspaces.
Tests
Unit tests in
loader_test.golist the loader cases side by side. Acceptance tests underacceptance/bundle/strict_profile_match/cover the same rules end to end, including--profile,workspace.profile,default_profileand the behavior without the variable. Accepted cases print the auth a bundle script receives, which shows the selected profile and host.This pull request and its description were written by Isaac.