Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions acceptance/auth/credentials/unified-host/out.requests.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"headers": {
"Authorization": [
"Bearer dapi-unified-token"
],
"User-Agent": [
"cli/[DEV_VERSION] databricks-sdk-go/[SDK_VERSION] go/[GO_VERSION] os/[OS] cmd/current-user_me cmd-exec-id/[UUID] auth/pat"
],
"X-Databricks-Org-Id": [
"[NUMID]"
]
},
"method": "GET",
"path": "/api/2.0/preview/scim/v2/Me"
}
5 changes: 5 additions & 0 deletions acceptance/auth/credentials/unified-host/out.test.toml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions acceptance/auth/credentials/unified-host/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

=== With workspace_id
{
"id":"[USERID]",
"userName":"[USERNAME]"
}

=== Without workspace_id (should error)
Error: WorkspaceId must be set when using WorkspaceClient with unified host

Exit code: 1
12 changes: 12 additions & 0 deletions acceptance/auth/credentials/unified-host/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Test unified host authentication with PAT token
export DATABRICKS_TOKEN=dapi-unified-token
export DATABRICKS_ACCOUNT_ID=test-account-123
export DATABRICKS_WORKSPACE_ID=1234567890
export DATABRICKS_EXPERIMENTAL_IS_UNIFIED_HOST=true

title "With workspace_id\n"
$CLI current-user me

title "Without workspace_id (should error)\n"
unset DATABRICKS_WORKSPACE_ID
errcode $CLI current-user me
3 changes: 3 additions & 0 deletions acceptance/auth/credentials/unified-host/test.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Test unified host authentication with PAT tokens
# Include X-Databricks-Org-Id header to verify workspace_id is sent
IncludeRequestHeaders = ["Authorization", "User-Agent", "X-Databricks-Org-Id"]
8 changes: 8 additions & 0 deletions bundle/config/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ type Workspace struct {
AzureEnvironment string `json:"azure_environment,omitempty"`
AzureLoginAppID string `json:"azure_login_app_id,omitempty"`

// Unified host specific attributes.
ExperimentalIsUnifiedHost bool `json:"experimental_is_unified_host,omitempty"`
WorkspaceId string `json:"workspace_id,omitempty"`

// CurrentUser holds the current user.
// This is set after configuration initialization.
CurrentUser *User `json:"current_user,omitempty" bundle:"readonly"`
Expand Down Expand Up @@ -117,6 +121,10 @@ func (w *Workspace) Config() *config.Config {
AzureTenantID: w.AzureTenantID,
AzureEnvironment: w.AzureEnvironment,
AzureLoginAppID: w.AzureLoginAppID,

// Unified host
Experimental_IsUnifiedHost: w.ExperimentalIsUnifiedHost,
WorkspaceId: w.WorkspaceId,
}

for k := range config.ConfigAttributes {
Expand Down
6 changes: 6 additions & 0 deletions bundle/internal/schema/annotations.yml
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,9 @@ github.com/databricks/cli/bundle/config.Workspace:
"client_id":
"description": |-
The client ID for the workspace
"experimental_is_unified_host":
"description": |-
Experimental feature flag to indicate if the host is a unified host
"file_path":
"description": |-
The file path to use within the workspace for both deployments and workflow runs
Expand All @@ -445,6 +448,9 @@ github.com/databricks/cli/bundle/config.Workspace:
"state_path":
"description": |-
The workspace state path
"workspace_id":
"description": |-
The Databricks workspace ID
github.com/databricks/cli/bundle/config/resources.Alert:
"create_time":
"description": |-
Expand Down
8 changes: 8 additions & 0 deletions bundle/schema/jsonschema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions cmd/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ GCP: https://docs.gcp.databricks.com/dev-tools/auth/index.html`,
var authArguments auth.AuthArguments
cmd.PersistentFlags().StringVar(&authArguments.Host, "host", "", "Databricks Host")
cmd.PersistentFlags().StringVar(&authArguments.AccountID, "account-id", "", "Databricks Account ID")
cmd.PersistentFlags().BoolVar(&authArguments.IsUnifiedHost, "experimental-is-unified-host", false, "Flag to indicate if the host is a unified host")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question - can we reliably determine if host is unified based on regex on host? Then we don't need to ask users that and there is no risk of mismatch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found this: databricks/databricks-sdk-go#1403 it looks like this flag is going to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @denik , we have taken the decision to keep the flag for now and not rely on regex as there are a number of use cases that will not be supported (dependent on cloud etc...) so we keep the usage of unified host as opt-in to set the right expectations.

The second PR is a PoC based on the internal pattern matching, it is not going to be merged for now. In future we would have support for cloud agnostic hosts so the flag won't be required at that time.

cmd.PersistentFlags().StringVar(&authArguments.WorkspaceId, "workspace-id", "", "Databricks Workspace ID")

cmd.AddCommand(newEnvCommand())
cmd.AddCommand(newLoginCommand(&authArguments))
Expand Down Expand Up @@ -55,3 +57,16 @@ func promptForAccountID(ctx context.Context) (string, error) {
prompt.AllowEdit = true
return prompt.Run()
}

func promptForWorkspaceID(ctx context.Context) (string, error) {
if !cmdio.IsPromptSupported(ctx) {
// Workspace ID is optional for unified hosts, so return empty string in non-interactive mode
return "", nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it optional? If I understand correctly, we cannot make API calls without a workspace ID.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then this should return an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is optional because without workspace ID we configure the client for account to make account requests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the caller error if neither an account ID nor a workspace ID is specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Error: fetching oauth config: fetching OAuth endpoints: failed to get OAuth endpoints: received HTML response instead of JSON

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not actionable. I was thinking of a proper error saying that you need either an account ID or a workspace ID.

}

prompt := cmdio.Prompt(ctx)
prompt.Label = "Databricks workspace ID (optional - provide only if using this profile for workspace operations, leave empty for account operations)"
prompt.Default = ""
prompt.AllowEdit = true
return prompt.Run()
}
104 changes: 79 additions & 25 deletions cmd/auth/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,15 @@ depends on the existing profiles you have set in your configuration file
if err != nil {
return err
}

// Load unified host flags from the profile if not explicitly set via CLI flag
if !cmd.Flag("experimental-is-unified-host").Changed && existingProfile != nil {
authArguments.IsUnifiedHost = existingProfile.IsUnifiedHost
}
if !cmd.Flag("workspace-id").Changed && existingProfile != nil {
authArguments.WorkspaceId = existingProfile.WorkspaceId
}

err = setHostAndAccountId(ctx, existingProfile, authArguments, args)
if err != nil {
return err
Expand All @@ -155,9 +164,11 @@ depends on the existing profiles you have set in your configuration file
// We need the config without the profile before it's used to initialise new workspace client below.
// Otherwise it will complain about non existing profile because it was not yet saved.
cfg := config.Config{
Host: authArguments.Host,
AccountID: authArguments.AccountID,
AuthType: "databricks-cli",
Host: authArguments.Host,
AccountID: authArguments.AccountID,
WorkspaceId: authArguments.WorkspaceId,
Experimental_IsUnifiedHost: authArguments.IsUnifiedHost,
AuthType: "databricks-cli",
}
databricksCfgFile := os.Getenv("DATABRICKS_CONFIG_FILE")
if databricksCfgFile != "" {
Expand Down Expand Up @@ -202,13 +213,15 @@ depends on the existing profiles you have set in your configuration file

if profileName != "" {
err = databrickscfg.SaveToProfile(ctx, &config.Config{
Profile: profileName,
Host: cfg.Host,
AuthType: cfg.AuthType,
AccountID: cfg.AccountID,
ClusterID: cfg.ClusterID,
ConfigFile: cfg.ConfigFile,
ServerlessComputeID: cfg.ServerlessComputeID,
Profile: profileName,
Host: cfg.Host,
AuthType: cfg.AuthType,
AccountID: cfg.AccountID,
WorkspaceId: authArguments.WorkspaceId,
Experimental_IsUnifiedHost: authArguments.IsUnifiedHost,
ClusterID: cfg.ClusterID,
ConfigFile: cfg.ConfigFile,
ServerlessComputeID: cfg.ServerlessComputeID,
})
if err != nil {
return err
Expand Down Expand Up @@ -260,24 +273,65 @@ func setHostAndAccountId(ctx context.Context, existingProfile *profile.Profile,
}
}

// If the account-id was not provided as a cmd line flag, try to read it from
// the specified profile.
//nolint:staticcheck // SA1019: IsAccountClient is deprecated but is still used here to avoid breaking changes
isAccountClient := (&config.Config{Host: authArguments.Host}).IsAccountClient()
accountID := authArguments.AccountID
if isAccountClient && accountID == "" {
if existingProfile != nil && existingProfile.AccountID != "" {
authArguments.AccountID = existingProfile.AccountID
} else {
// Prompt user for the account-id if it we could not get it from a
// profile.
accountId, err := promptForAccountID(ctx)
if err != nil {
return err
// Determine the host type and handle account ID / workspace ID accordingly
cfg := &config.Config{
Host: authArguments.Host,
AccountID: authArguments.AccountID,
WorkspaceId: authArguments.WorkspaceId,
Experimental_IsUnifiedHost: authArguments.IsUnifiedHost,
}

switch cfg.HostType() {
case config.AccountHost:
// Account host - prompt for account ID if not provided
if authArguments.AccountID == "" {
if existingProfile != nil && existingProfile.AccountID != "" {
authArguments.AccountID = existingProfile.AccountID
} else {
accountId, err := promptForAccountID(ctx)
if err != nil {
return err
}
authArguments.AccountID = accountId
}
}
case config.UnifiedHost:
// Unified host requires an account ID for OAuth URL construction
if authArguments.AccountID == "" {
if existingProfile != nil && existingProfile.AccountID != "" {
authArguments.AccountID = existingProfile.AccountID
} else {
accountId, err := promptForAccountID(ctx)
if err != nil {
return err
}
authArguments.AccountID = accountId
}
}

// Workspace ID is optional and determines API access level:
// - With workspace ID: workspace-level APIs
// - Without workspace ID: account-level APIs
// If neither is provided via flags, prompt for workspace ID (most common case)
hasWorkspaceID := authArguments.WorkspaceId != ""
if !hasWorkspaceID {
if existingProfile != nil && existingProfile.WorkspaceId != "" {
authArguments.WorkspaceId = existingProfile.WorkspaceId
} else {
// Prompt for workspace ID for workspace-level access
workspaceId, err := promptForWorkspaceID(ctx)
if err != nil {
return err
}
authArguments.WorkspaceId = workspaceId
}
authArguments.AccountID = accountId
}
case config.WorkspaceHost:
// Workspace host - no additional prompts needed
default:
return fmt.Errorf("unknown host type: %v", cfg.HostType())
}

return nil
}

Expand Down
68 changes: 68 additions & 0 deletions cmd/auth/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,74 @@ func TestSetAccountId(t *testing.T) {
assert.EqualError(t, err, "the command is being run in a non-interactive environment, please specify an account ID using --account-id")
}

func TestSetWorkspaceIdForUnifiedHost(t *testing.T) {
var authArguments auth.AuthArguments
t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg")
ctx, _ := cmdio.SetupTest(context.Background(), cmdio.TestOptions{})

unifiedWorkspaceProfile := loadTestProfile(t, ctx, "unified-workspace")
unifiedAccountProfile := loadTestProfile(t, ctx, "unified-account")

// Test setting workspace-id from flag for unified host
authArguments = auth.AuthArguments{
Host: "https://unified.databricks.com",
AccountID: "test-unified-account",
WorkspaceId: "val from --workspace-id",
IsUnifiedHost: true,
}
err := setHostAndAccountId(ctx, unifiedWorkspaceProfile, &authArguments, []string{})
assert.NoError(t, err)
assert.Equal(t, "https://unified.databricks.com", authArguments.Host)
assert.Equal(t, "test-unified-account", authArguments.AccountID)
assert.Equal(t, "val from --workspace-id", authArguments.WorkspaceId)

// Test setting workspace_id from profile for unified host
authArguments = auth.AuthArguments{
Host: "https://unified.databricks.com",
AccountID: "test-unified-account",
IsUnifiedHost: true,
}
err = setHostAndAccountId(ctx, unifiedWorkspaceProfile, &authArguments, []string{})
assert.NoError(t, err)
assert.Equal(t, "https://unified.databricks.com", authArguments.Host)
assert.Equal(t, "test-unified-account", authArguments.AccountID)
assert.Equal(t, "123456789", authArguments.WorkspaceId)

// Test workspace_id is optional - should default to empty in non-interactive mode
authArguments = auth.AuthArguments{
Host: "https://unified.databricks.com",
AccountID: "test-unified-account",
IsUnifiedHost: true,
}
err = setHostAndAccountId(ctx, unifiedAccountProfile, &authArguments, []string{})
assert.NoError(t, err)
assert.Equal(t, "https://unified.databricks.com", authArguments.Host)
assert.Equal(t, "test-unified-account", authArguments.AccountID)
assert.Equal(t, "", authArguments.WorkspaceId) // Empty is valid for account-level access

// Test workspace_id is optional - should default to empty when no profile exists
authArguments = auth.AuthArguments{
Host: "https://unified.databricks.com",
AccountID: "test-unified-account",
IsUnifiedHost: true,
}
err = setHostAndAccountId(ctx, nil, &authArguments, []string{})
assert.NoError(t, err)
assert.Equal(t, "https://unified.databricks.com", authArguments.Host)
assert.Equal(t, "test-unified-account", authArguments.AccountID)
assert.Equal(t, "", authArguments.WorkspaceId) // Empty is valid for account-level access
}

func TestPromptForWorkspaceIdInNonInteractiveMode(t *testing.T) {
// Setup non-interactive context
ctx, _ := cmdio.SetupTest(context.Background(), cmdio.TestOptions{})

// Test that promptForWorkspaceID returns empty string (no error) in non-interactive mode
workspaceID, err := promptForWorkspaceID(ctx)
assert.NoError(t, err)
assert.Equal(t, "", workspaceID)
}

func TestLoadProfileByNameAndClusterID(t *testing.T) {
testCases := []struct {
name string
Expand Down
9 changes: 6 additions & 3 deletions cmd/auth/profiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
return
}

//nolint:staticcheck // SA1019: IsAccountClient is deprecated but is still used here to avoid breaking changes
if cfg.IsAccountClient() {
switch cfg.ConfigType() {
case config.AccountConfig:
a, err := databricks.NewAccountClient((*databricks.Config)(cfg))
if err != nil {
return
Expand All @@ -64,7 +64,7 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
return
}
c.Valid = true
} else {
case config.WorkspaceConfig:
w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg))
if err != nil {
return
Expand All @@ -76,6 +76,9 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
return
}
c.Valid = true
case config.InvalidConfig:
// Invalid configuration, skip validation
return
}
}

Expand Down
11 changes: 11 additions & 0 deletions cmd/auth/testdata/.databrickscfg
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,14 @@ cluster_id = cluster-from-config
[invalid-profile]
# This profile is missing the required 'host' field
cluster_id = some-cluster-id

[unified-workspace]
host = https://unified.databricks.com
account_id = test-unified-account
workspace_id = 123456789
experimental_is_unified_host = true

[unified-account]
host = https://unified.databricks.com
account_id = test-unified-account
experimental_is_unified_host = true
Loading