Added `w.config.account_host` to get the relevant account host from a workspace client #390

nfx · 2023-10-06T18:11:37Z

Changes

This simplifies downstream integration testing as well as configuration

Tests

make test run locally
make fmt applied
relevant integration tests applied

codecov-commenter · 2023-10-06T18:13:30Z

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
databricks/sdk/azure.py	`100.00% <100.00%> (ø)`
databricks/sdk/core.py	`80.69% <100.00%> (+0.32%)`	⬆️

📢 Thoughts on this report? Let us know!.

mgyucht · 2023-10-12T13:07:19Z

databricks/sdk/azure.py

-                            active_directory_endpoint="https://login.microsoftonline.de/"),
+    GERMAN=AzureEnvironment(
+        name="AzureGermanCloud",
+        databricks_hostname_suffix='.databricks.azure.de', # TODO: i'm not sure about this one


I think this is OK because we don't run Databricks in this cloud (and it is being deprecated: https://learn.microsoft.com/en-us/previous-versions/azure/germany/germany-welcome)

mgyucht · 2023-10-12T13:12:48Z

databricks/sdk/core.py

@@ -617,8 +613,31 @@ def is_account_client(self) -> bool:
            return False
        return self.host.startswith("https://accounts.") or self.host.startswith("https://accounts-dod.")

+    @property
+    def account_host(self) -> str:


This won't work for dev/staging, right? We occasionally get internal issues where the SDK doesn't work well with dev/staging.

Also, can we label this somehow as experimental? In the future, if we introduce a workspace-level metadata service, the accounts endpoint can be exposed there, and we can remove this property.

can you suggest on the pattern here to make it work for dev/staging?

btw, once we introduce workspace-level metadata service, we should just print warnings from within this call.

One idea: have a list to lookup cloud/env/account host name based on domain suffix, from most-specific to least-specific:

# Most specific to least specific [ ('.dev.databricks.com', 'AWS', 'DEV', 'PUBLIC', 'accounts.dev.databricks.com'), ('.staging.cloud.databricks.com', 'AWS', 'STAGING', 'PUBLIC', 'accounts.staging.cloud.databricks.com'), ('.cloud.databricks.com', 'AWS', 'PROD', 'PUBLIC', 'accounts.cloud.databricks.com'), ('.cloud.databricks.us', 'AWS', 'PROD', 'GOV', 'accounts.cloud.databricks.us'), ('.dev.azuredatabricks.net', 'AZURE', 'DEV', 'PUBLIC', 'accounts.dev.azuredatabricks.net') ('.staging.azuredatabricks.net', 'AZURE', 'STAGING', 'PUBLIC', 'accounts.staging.azuredatabricks.net'), ('.azuredatabricks.net', 'AZURE', 'PROD', 'PUBLIC', 'accounts.azuredatabricks.net'), ('.databricks.azure.us', 'AZURE', 'PROD', 'GOV', 'accounts.databricks.azure.us'), ('.dev.gcp.databricks.com', 'GCP', 'DEV', 'PUBLIC', 'accounts.dev.gcp.databricks.com'), ('.staging.gcp.databricks.com', 'GCP', 'STAGING', 'PUBLIC', 'accounts.staging.gcp.databricks.com'), ('.gcp.databricks.com', 'GCP', 'PROD', 'PUBLIC', 'accounts.gcp.databricks.com'), ]

The simpler version is to just replace the most specific zone with accounts, which works everywhere today.

@mgyucht do we want to expose dev/staging?... if we do - then why don't we expose the azure_login_app_id along with it?

That's definitely fine by me!

mgyucht · 2023-10-12T13:14:12Z

One nit to make this work for dev/staging & label as experimental, but otherwise this seems reasonable to me if it unblocks you/customers.

hamza-db · 2023-11-08T20:49:53Z

all the info here is fine to be public ( and already public)

mgyucht

This is great, thank you for contributing! Couple nits/suggestions, otherwise this looks good to me.

mgyucht · 2023-11-09T08:54:18Z

databricks/sdk/core.py

@@ -617,8 +613,31 @@ def is_account_client(self) -> bool:
            return False
        return self.host.startswith("https://accounts.") or self.host.startswith("https://accounts-dod.")

+    @property
+    def account_host(self) -> str:


That's definitely fine by me!

databricks/sdk/core.py

mgyucht · 2023-11-09T08:56:07Z

databricks/sdk/core.py

+    cloud: Cloud
+
+    # domain suffixes are not very secret: https://crt.sh/?q=databricks
+    tld: str


nit: let's call this dns_zone (I think TLD would traditionally refer to com. or net.)

dns_zone it is then.

mgyucht · 2023-11-09T09:01:38Z

databricks/sdk/core.py

+    def deployment(self, deployment_name: str) -> str:
+        return f'https://{deployment_name}{self.tld}'


This is maybe just a bit abstract. It won't work in Azure the way a user might expect, where the DNS name can't be determined from the workspace ID alone. Given we're only using this for account host, I suggest we just inline it there.

it's already part of our response:

for now, we work it around in https://github.com/databrickslabs/ucx/blob/main/src/databricks/labs/ucx/account/workspaces.py#L101-L126, but @iaroslav-db fixed the mapping so that we don't have to work it around soon ;)

I need this function to create a workspace client from an account client. See:

https://github.com/databrickslabs/watchdog/blob/main/scan/scan.go#L144-L154

https://github.com/databrickslabs/ucx/blob/main/src/databricks/labs/ucx/account/workspaces.py#L122-L126

https://github.com/databricks/terraform-provider-databricks/blob/baa61c451055271e14e7a9bc3db43537dcfda8ea/mws/resource_mws_workspaces.go#L372-L377

I'll do a follow-up PR to account client once azure changes are rolled out to prod

Oh perfect, I actually didn't realize those workspace APIs worked in azure yet.

… workspace client This simplifies downstream integration testing as well as configuration introduce `DatabricksEnvironment` ..

nfx · 2023-11-09T11:10:35Z

this needs a bit more adjustments to work

pietern · 2023-11-09T12:22:34Z

Are similar PRs for the other SDKs tracked some place?

nfx · 2023-11-10T16:36:17Z

@pietern i'll have one for GoLang the following week - once i figure out the way to have it friendly for unit tests, need the same thing for two projects

…ere IMDS doesn't give host environment information (#700) ## Changes This PR allows determining Azure Environment from a Databricks account or workspace hostname, removing the need for a separate configuration/environment variable and complexities related to Azure MSI from within ACR. Similar functionality in Python SDK: - databricks/databricks-sdk-py#390 Stacked on top of: - #699 ## Tests  - [x] `make test` passing - [x] `make fmt` applied - [ ] relevant integration tests applied

nfx added the chore anything that makes the code better label Oct 6, 2023

nfx requested a review from a team October 6, 2023 18:11

mgyucht reviewed Oct 12, 2023

View reviewed changes

mgyucht approved these changes Nov 9, 2023

View reviewed changes

Added w.config.account_host to get the relevant account host from a…

cb830fc

… workspace client This simplifies downstream integration testing as well as configuration introduce `DatabricksEnvironment` ..

nfx force-pushed the feat/account-host branch from bea774f to cb830fc Compare November 9, 2023 09:21

nfx added the enhancement New feature or request label Nov 9, 2023

nfx added 2 commits November 9, 2023 10:40

..

a152dd4

... some changes

f134fe1

nfx added the do-not-merge label Nov 9, 2023

nfx mentioned this pull request Nov 20, 2023

Introduce DatabricksEnvironment and fix Azure MSI auth from ACR, where IMDS doesn't give host environment information databricks/databricks-sdk-go#700

Merged

3 tasks

nfx mentioned this pull request Nov 28, 2023

Added task filtering by cloud databrickslabs/ucx#630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `w.config.account_host` to get the relevant account host from a workspace client #390

Added `w.config.account_host` to get the relevant account host from a workspace client #390

nfx commented Oct 6, 2023

codecov-commenter commented Oct 6, 2023 •

edited

Loading

mgyucht Oct 12, 2023

mgyucht Oct 12, 2023

nfx Oct 16, 2023

mgyucht Oct 18, 2023

nfx Nov 7, 2023

mgyucht Nov 9, 2023

mgyucht commented Oct 12, 2023

hamza-db commented Nov 8, 2023

mgyucht left a comment

mgyucht Nov 9, 2023

mgyucht Nov 9, 2023

nfx Nov 9, 2023

mgyucht Nov 9, 2023

nfx Nov 9, 2023 •

edited

Loading

mgyucht Nov 9, 2023

nfx commented Nov 9, 2023

pietern commented Nov 9, 2023

nfx commented Nov 10, 2023 •

edited

Loading

		def deployment(self, deployment_name: str) -> str:
		return f'https://{deployment_name}{self.tld}'

Added w.config.account_host to get the relevant account host from a workspace client #390

Are you sure you want to change the base?

Added w.config.account_host to get the relevant account host from a workspace client #390

Conversation

nfx commented Oct 6, 2023

Changes

Tests

codecov-commenter commented Oct 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgyucht commented Oct 12, 2023

hamza-db commented Nov 8, 2023

mgyucht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfx Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfx commented Nov 9, 2023

pietern commented Nov 9, 2023

nfx commented Nov 10, 2023 • edited Loading

Added `w.config.account_host` to get the relevant account host from a workspace client #390

Added `w.config.account_host` to get the relevant account host from a workspace client #390

codecov-commenter commented Oct 6, 2023 •

edited

Loading

nfx Nov 9, 2023 •

edited

Loading

nfx commented Nov 10, 2023 •

edited

Loading