-
Notifications
You must be signed in to change notification settings - Fork 136
[Feature] Add support for Unified Host with experimental flag #4260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1bf8921
c1bfe92
88eb854
4bfad21
6927440
e9130cb
e5c6b45
b47d88c
775e3db
0d2d2c9
3fb7f4a
f76160d
b525ef8
be37970
096fda3
f95751c
775219e
cd3727c
cc83a2a
be536d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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" | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| 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 |
| 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 |
| 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"] |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") | ||
| cmd.PersistentFlags().StringVar(&authArguments.WorkspaceId, "workspace-id", "", "Databricks Workspace ID") | ||
|
|
||
| cmd.AddCommand(newEnvCommand()) | ||
| cmd.AddCommand(newLoginCommand(&authArguments)) | ||
|
|
@@ -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) { | ||
tanmay-db marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Workspace ID is optional for unified hosts, so return empty string in non-interactive mode | ||
| return "", nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And then this should return an error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
| } | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.