Skip to content
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

Migrate Azure Service Principals with secrete stored in Databricks Secret to UC Storage Credentials #874

Merged
merged 76 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 75 commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
63d9d4c
init files for #339
qziyuan Feb 2, 2024
3c02941
1. draft azure_credentials.py framework, adding AzureServicePrincipal…
qziyuan Feb 3, 2024
477b5df
add _list_storage_credentials to azure service principal migration
qziyuan Feb 5, 2024
3b9979a
fix typo defining a set[str] for storage credentials
qziyuan Feb 5, 2024
808e123
add _check_sp_in_storage_credentials to filter out service principals…
qziyuan Feb 5, 2024
caaaec1
Add directory_id/tenant_id to crawled service principals. It is requi…
qziyuan Feb 5, 2024
d8dacfc
remove unused parameter customized_csv from load_spn_permission()
qziyuan Feb 5, 2024
a7a7820
add functions to create and validate the storage credential
qziyuan Feb 5, 2024
76e8280
rename the function for loading StoragePermissionMapping
qziyuan Feb 5, 2024
38beb9c
extract logic of reading databricks secret and decode the value to a …
qziyuan Feb 5, 2024
94d8338
simplify the cli function migrate_azure_service_principals by moving …
qziyuan Feb 5, 2024
79b30c8
fix a typo
qziyuan Feb 5, 2024
922af42
fix failed unit tests due to new directory_id added in StoragePermiss…
qziyuan Feb 5, 2024
6254293
remove unused parameters from for_cli in migration/azure_credentials.py
qziyuan Feb 6, 2024
2eced1e
add unit tests for _list_storage_credentials and _check_sp_in_storage…
qziyuan Feb 6, 2024
5f0aa41
fix some fmt errors
qziyuan Feb 6, 2024
40ab9cd
replace _check_sp_in_storage_credentials() by list comprehension
qziyuan Feb 11, 2024
fdccb90
change ServicePrincipalMigrationInfo from a dataclass to a namedtuple
qziyuan Feb 11, 2024
477d0d6
do not catch PermissionDenied and Unauthenticated exceptions when fet…
qziyuan Feb 11, 2024
feb3d58
use return not yield in _create_storage_credential
qziyuan Feb 11, 2024
7dc14a1
move azure check and service principal confirmation prompts to for_cl…
qziyuan Feb 11, 2024
9f6cfc9
add unit test for _read_databricks_secret
qziyuan Feb 12, 2024
b365488
dd unit tests for _fetch_client_secret and _print_action_plan
qziyuan Feb 12, 2024
a2a51cf
add unit tests for for_cli in azure_credentials.py
qziyuan Feb 12, 2024
1fa336c
add unit test for _generate_migration_list
qziyuan Feb 12, 2024
16de683
add unit test for execute_migration, set keyword arg filename when ca…
qziyuan Feb 12, 2024
54db4a3
add unit test for migrate_azure_service_principals cli
qziyuan Feb 12, 2024
d856520
refactor unit tests and apply make fmt changes
qziyuan Feb 12, 2024
e0fb9f8
fix failed unit tests in test_azure.py due to rebase to main.
qziyuan Feb 12, 2024
45e0c0e
moved all azure service principal to storage credential migration cod…
qziyuan Feb 12, 2024
260ad5d
1. catch the external location overlaps exception while validating th…
qziyuan Feb 13, 2024
c317767
add unit tests for _integration_test_flag and storage credential vali…
qziyuan Feb 13, 2024
5301a44
improve unit test for test_execute_migration
qziyuan Feb 14, 2024
87d1af8
add read_only flag to storage credential validation
qziyuan Feb 14, 2024
6099a56
remove some redundants
qziyuan Feb 14, 2024
1d8e69c
add integration tests for azure service principal migration.
qziyuan Feb 14, 2024
b55a812
apply make fmt changes
qziyuan Feb 14, 2024
e7a3867
fix leftover and failed unit tests after rebase
qziyuan Feb 14, 2024
ea73a1d
rename AzureServicePrincipalMigration to ServicePrincipalMigration, r…
qziyuan Feb 14, 2024
decc1ee
add guard rail to explicitly remove the client_secret from validation…
qziyuan Feb 15, 2024
bf0dbcc
switch ServicePrincipalMigrationInfo back to dataclass.
qziyuan Feb 15, 2024
8f96530
1. remove azure_ prefix from all service principal migratrion files 2…
qziyuan Feb 15, 2024
f4ec60d
improve azure/credentials.py for testability
qziyuan Feb 15, 2024
1eea9bc
imporve readability of integration test integration/azure/test_creden…
qziyuan Feb 15, 2024
09d66a6
shorten the vaiables name; use proper `| None` in dataclass type defi…
qziyuan Feb 15, 2024
d86f91c
shorten variable name when create storage credential
qziyuan Feb 15, 2024
1397b44
refactor unit tests to test different cases by calling public functi…
qziyuan Feb 16, 2024
64a5f75
fix import error after merge from main
qziyuan Feb 16, 2024
ae546c4
change _print_action_plan to take StoragePermissionMapping as input t…
qziyuan Feb 16, 2024
b86d1b0
decompose azure_service_principal returned by the storage credential …
qziyuan Feb 16, 2024
2b72a2f
small re-format
qziyuan Feb 16, 2024
f1c446d
clean up unnecessary keyword parameter in make_storage_credential_fro…
qziyuan Feb 16, 2024
f26886e
fix integration test due to parameter change
qziyuan Feb 16, 2024
d9675d2
remove patch("databricks.*") in unit test for migrate_azure_service_p…
qziyuan Feb 20, 2024
718ca75
fix failed tests after rebase
qziyuan Feb 20, 2024
dba32af
apply updates from make fmt. Correct AzureResourcePermissions.load to…
qziyuan Feb 20, 2024
958c37a
update variable name sp to spn based on pylint suggestion
qziyuan Feb 20, 2024
0a7413c
apply pylint recommendations on tests/unit/azure/test_credentials.py
qziyuan Feb 20, 2024
39a99bc
apply pylint recommendations for azure service principal to storage c…
qziyuan Feb 20, 2024
7fadc28
remove unnecessary keyword areguments in tests for azure service prin…
qziyuan Feb 20, 2024
89bd9f6
flip an assertion order in a unit test for readability
qziyuan Feb 20, 2024
f396d22
Apply SecretsMixin from PR #950
qziyuan Feb 20, 2024
ced30f1
add auth type to unit test
qziyuan Feb 21, 2024
7074827
add connect field to config.yml for unit test azure/test_credentials.py
qziyuan Feb 21, 2024
791bb4e
rename some functions and variables for readability
qziyuan Feb 21, 2024
d59abef
simplify side_effect_validate_storage_credential
qziyuan Feb 21, 2024
a275863
split test_validate_storage_credentials into 3 test functions for bet…
qziyuan Feb 22, 2024
b42856c
simplify the storage credential validation result to only keep failur…
qziyuan Feb 22, 2024
f58bd7c
further simplify function names and codes
qziyuan Feb 22, 2024
ee84a78
improve intgration tests.
qziyuan Feb 22, 2024
6f12b58
fix logging wording
qziyuan Feb 22, 2024
272d05b
update cli comment to use new method name principal_prefix_access
qziyuan Feb 22, 2024
cff18fa
update migrate_credentials cli; Refactor the code so MockInstallation…
qziyuan Feb 22, 2024
71643da
Remove StaticStorageCredentialManager and add include_names for integ…
qziyuan Feb 23, 2024
dab2bc5
Remove StaticServicePrincipalMigration and StaticResourcePermissions …
qziyuan Feb 23, 2024
448e641
let validate function take StoragePermissionMapping as input; Remove …
qziyuan Feb 23, 2024
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
3 changes: 3 additions & 0 deletions labs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,6 @@ commands:
Workspace Group Name\tMembers Count\tAccount Group Name\tMembers Count
{{range .}}{{.wf_group_name}}\t{{.wf_group_members_count}}\t{{.acc_group_name}}\t{{.acc_group_members_count}}
{{end}}

- name: migrate_credentials
description: Migrate credentials for storage access to UC storage credential
7 changes: 6 additions & 1 deletion src/databricks/labs/ucx/assessment/secrets.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ def _get_secret_if_exists(self, secret_scope, secret_key) -> str | None:
assert secret.value is not None
return base64.b64decode(secret.value).decode("utf-8")
except NotFound:
logger.warning(f'removed on the backend: {secret_scope}{secret_key}')
logger.warning(f'removed on the backend: {secret_scope}/{secret_key}')
return None
except UnicodeDecodeError:
logger.warning(
f"Secret {secret_scope}/{secret_key} has Base64 bytes that cannot be decoded to utf-8 string."
)
return None

def _get_value_from_config_key(self, config: dict, key: str, get_secret: bool = True) -> str | None:
Expand Down
3 changes: 3 additions & 0 deletions src/databricks/labs/ucx/azure/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ class StoragePermissionMapping:
client_id: str
principal: str
privilege: str
# Need this directory_id/tenant_id when create UC storage credentials using service principal
directory_id: str


class AzureResourcePermissions:
Expand Down Expand Up @@ -63,6 +65,7 @@ def _map_storage(self, storage: AzureResource) -> list[StoragePermissionMapping]
client_id=role_assignment.principal.client_id,
principal=role_assignment.principal.display_name,
privilege=privilege,
directory_id=role_assignment.principal.directory_id,
)
)
return out
Expand Down
276 changes: 276 additions & 0 deletions src/databricks/labs/ucx/azure/credentials.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,276 @@
import logging
from dataclasses import dataclass

from databricks.labs.blueprint.installation import Installation
from databricks.labs.blueprint.tui import Prompts
from databricks.sdk import WorkspaceClient
from databricks.sdk.errors.platform import InvalidParameterValue
from databricks.sdk.service.catalog import (
AzureServicePrincipal,
Privilege,
StorageCredentialInfo,
ValidationResultResult,
)

from databricks.labs.ucx.assessment.azure import AzureServicePrincipalCrawler
from databricks.labs.ucx.assessment.secrets import SecretsMixin
from databricks.labs.ucx.azure.access import (
AzureResourcePermissions,
StoragePermissionMapping,
)
from databricks.labs.ucx.azure.resources import AzureResources
from databricks.labs.ucx.config import WorkspaceConfig
from databricks.labs.ucx.framework.crawlers import StatementExecutionBackend
from databricks.labs.ucx.hive_metastore.locations import ExternalLocations

logger = logging.getLogger(__name__)


# A dataclass to host service_principal info and its client_secret info
@dataclass
class ServicePrincipalMigrationInfo:
permission_mapping: StoragePermissionMapping
client_secret: str


@dataclass
class StorageCredentialValidationResult:
name: str | None = None
nfx marked this conversation as resolved.
Show resolved Hide resolved
application_id: str | None = None
nfx marked this conversation as resolved.
Show resolved Hide resolved
directory_id: str | None = None
nfx marked this conversation as resolved.
Show resolved Hide resolved
read_only: bool | None = None
validated_on: str | None = None
failures: list[str] | None = None

@classmethod
def from_validation(cls, storage_credential: StorageCredentialInfo, failures: list[str] | None, prefix: str):
if storage_credential.azure_service_principal:
application_id = storage_credential.azure_service_principal.application_id
directory_id = storage_credential.azure_service_principal.directory_id

return cls(
storage_credential.name, application_id, directory_id, storage_credential.read_only, prefix, failures
)


class StorageCredentialManager:
nfx marked this conversation as resolved.
Show resolved Hide resolved
def __init__(self, ws: WorkspaceClient):
self._ws = ws

def list(self, include_names: set[str] | None = None) -> set[str]:
# list existed storage credentials that is using service principal, capture the service principal's application_id
application_ids = set()

storage_credentials = self._ws.storage_credentials.list(max_results=0)

if include_names:
# we only check UC storage credentials listed in include_names
for storage_credential in storage_credentials:
if not storage_credential.azure_service_principal:
continue
if storage_credential.name in include_names:
application_ids.add(storage_credential.azure_service_principal.application_id)
logger.info(
f"Found {len(application_ids)} distinct service principals already used in UC storage credentials listed in include_names"
)
return application_ids

for storage_credential in storage_credentials:
# only add service principal's application_id, ignore managed identity based storage_credential
if storage_credential.azure_service_principal:
application_ids.add(storage_credential.azure_service_principal.application_id)

logger.info(f"Found {len(application_ids)} distinct service principals already used in UC storage credentials")
return application_ids

def create_with_client_secret(self, spn: ServicePrincipalMigrationInfo) -> StorageCredentialInfo:
# prepare the storage credential properties
name = spn.permission_mapping.principal
service_principal = AzureServicePrincipal(
spn.permission_mapping.directory_id,
spn.permission_mapping.client_id,
spn.client_secret,
)
comment = (
f"Created by UCX during migration to UC using Azure Service Principal: {spn.permission_mapping.principal}"
)

# create the storage credential
return self._ws.storage_credentials.create(
name,
azure_service_principal=service_principal,
comment=comment,
read_only=spn.permission_mapping.privilege == Privilege.READ_FILES.value,
)

def validate(
self, storage_credential: StorageCredentialInfo, spn: ServicePrincipalMigrationInfo
) -> StorageCredentialValidationResult:
try:
validation = self._ws.storage_credentials.validate(
storage_credential_name=storage_credential.name,
url=spn.permission_mapping.prefix,
read_only=spn.permission_mapping.privilege == Privilege.READ_FILES.value,
)
except InvalidParameterValue:
logger.warning(
"There is an existing external location overlaps with the prefix that is mapped to "
"the service principal and used for validating the migrated storage credential. "
"Skip the validation"
)
return StorageCredentialValidationResult.from_validation(
storage_credential,
[
"The validation is skipped because an existing external location overlaps "
"with the location used for validation."
],
spn.permission_mapping.prefix,
)

if not validation.results:
return StorageCredentialValidationResult.from_validation(
storage_credential, ["Validation returned none results."], spn.permission_mapping.prefix
)

failures = []
for result in validation.results:
if result.operation is None:
continue
if result.result == ValidationResultResult.FAIL:
failures.append(f"{result.operation.value} validation failed with message: {result.message}")
return StorageCredentialValidationResult.from_validation(
storage_credential, None if not failures else failures, spn.permission_mapping.prefix
)


class ServicePrincipalMigration(SecretsMixin):

def __init__(
self,
installation: Installation,
ws: WorkspaceClient,
resource_permissions: AzureResourcePermissions,
service_principal_crawler: AzureServicePrincipalCrawler,
storage_credential_manager: StorageCredentialManager,
):
self._output_file = "azure_service_principal_migration_result.csv"
self._installation = installation
self._ws = ws
self._resource_permissions = resource_permissions
self._sp_crawler = service_principal_crawler
self._storage_credential_manager = storage_credential_manager

@classmethod
def for_cli(cls, ws: WorkspaceClient, installation: Installation, prompts: Prompts):
msg = (
"Have you reviewed the azure_storage_account_info.csv "
"and confirm listed service principals are allowed to be checked for migration?"
)
if not prompts.confirm(msg):
raise SystemExit()

config = installation.load(WorkspaceConfig)
sql_backend = StatementExecutionBackend(ws, config.warehouse_id)
azurerm = AzureResources(ws)
locations = ExternalLocations(ws, sql_backend, config.inventory_database)

resource_permissions = AzureResourcePermissions(installation, ws, azurerm, locations)
sp_crawler = AzureServicePrincipalCrawler(ws, sql_backend, config.inventory_database)

storage_credential_manager = StorageCredentialManager(ws)

return cls(installation, ws, resource_permissions, sp_crawler, storage_credential_manager)

def _fetch_client_secret(self, sp_list: list[StoragePermissionMapping]) -> list[ServicePrincipalMigrationInfo]:
# check AzureServicePrincipalInfo from AzureServicePrincipalCrawler, if AzureServicePrincipalInfo
# has secret_scope and secret_key not empty, fetch the client_secret and put it to the client_secret field
#
# The input StoragePermissionMapping may have managed identity mixed in, we will ignore them for now, as
# they won't have any client_secret, we will process managed identity in the future.

# fetch client_secrets of crawled service principal, if any
sp_info_with_client_secret: dict[str, str] = {}
sp_infos = self._sp_crawler.snapshot()

for sp_info in sp_infos:
if not sp_info.secret_scope:
continue
if not sp_info.secret_key:
continue

secret_value = self._get_secret_if_exists(sp_info.secret_scope, sp_info.secret_key)

if secret_value:
sp_info_with_client_secret[sp_info.application_id] = secret_value
else:
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.info(
logger.warning(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

f"Cannot fetch the service principal client_secret for {sp_info.application_id}. "
f"This service principal will be skipped for migration"
)

# update the list of ServicePrincipalMigrationInfo if client_secret is found
sp_list_with_secret = []
for spn in sp_list:
if spn.client_id in sp_info_with_client_secret:
sp_list_with_secret.append(
ServicePrincipalMigrationInfo(spn, sp_info_with_client_secret[spn.client_id])
)
return sp_list_with_secret

def _print_action_plan(self, sp_list: list[StoragePermissionMapping]):
# print action plan to console for customer to review.
for spn in sp_list:
logger.info(
f"Service Principal name: {spn.principal}, "
f"application_id: {spn.client_id}, "
f"privilege {spn.privilege} "
f"on location {spn.prefix}"
)

def _generate_migration_list(self, include_names: set[str] | None = None) -> list[ServicePrincipalMigrationInfo]:
"""
Create the list of SP that need to be migrated, output an action plan as a csv file for users to confirm
"""
# load sp list from azure_storage_account_info.csv
sp_list = self._resource_permissions.load()
# list existed storage credentials
sc_set = self._storage_credential_manager.list(include_names)
# check if the sp is already used in UC storage credential
filtered_sp_list = [sp for sp in sp_list if sp.client_id not in sc_set]
# fetch sp client_secret if any
sp_list_with_secret = self._fetch_client_secret(filtered_sp_list)

# output the action plan for customer to confirm
# but first make a copy of the list and strip out the client_secret
sp_candidates = [sp.permission_mapping for sp in sp_list_with_secret]
self._print_action_plan(sp_candidates)

return sp_list_with_secret

def save(self, migration_results: list[StorageCredentialValidationResult]) -> str:
return self._installation.save(migration_results, filename=self._output_file)

def run(self, prompts: Prompts, include_names: set[str] | None = None) -> list[StorageCredentialValidationResult]:

sp_list_with_secret = self._generate_migration_list(include_names)

plan_confirmed = prompts.confirm(
"Above Azure Service Principals will be migrated to UC storage credentials, please review and confirm."
)
if plan_confirmed is not True:
return []

execution_result = []
for spn in sp_list_with_secret:
storage_credential = self._storage_credential_manager.create_with_client_secret(spn)
execution_result.append(self._storage_credential_manager.validate(storage_credential, spn))

if execution_result:
results_file = self.save(execution_result)
logger.info(
f"Completed migration from Azure Service Principal to UC Storage credentials"
f"Please check {results_file} for validation results"
)
else:
logger.info("No Azure Service Principal migrated to UC Storage credentials")
return execution_result
7 changes: 6 additions & 1 deletion src/databricks/labs/ucx/azure/resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ class Principal:
client_id: str
display_name: str
object_id: str
# Need this directory_id/tenant_id when create UC storage credentials using service principal
directory_id: str


@dataclass
Expand Down Expand Up @@ -171,10 +173,13 @@ def _get_principal(self, principal_id: str) -> Principal | None:
client_id = raw.get("appId")
display_name = raw.get("displayName")
object_id = raw.get("id")
# Need this directory_id/tenant_id when create UC storage credentials using service principal
directory_id = raw.get("appOwnerOrganizationId")
assert client_id is not None
assert display_name is not None
assert object_id is not None
self._principals[principal_id] = Principal(client_id, display_name, object_id)
assert directory_id is not None
self._principals[principal_id] = Principal(client_id, display_name, object_id, directory_id)
return self._principals[principal_id]

def role_assignments(
Expand Down
23 changes: 23 additions & 0 deletions src/databricks/labs/ucx/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from databricks.labs.ucx.account import AccountWorkspaces, WorkspaceInfo
from databricks.labs.ucx.assessment.aws import AWSResourcePermissions
from databricks.labs.ucx.azure.access import AzureResourcePermissions
from databricks.labs.ucx.azure.credentials import ServicePrincipalMigration
from databricks.labs.ucx.config import WorkspaceConfig
from databricks.labs.ucx.framework.crawlers import StatementExecutionBackend
from databricks.labs.ucx.hive_metastore import ExternalLocations, TablesCrawler
Expand Down Expand Up @@ -282,5 +283,27 @@ def _aws_principal_prefix_access(w: WorkspaceClient, aws_profile: str):
logger.info(f"UC roles and bucket info saved {uc_role_path}")


@ucx.command
def migrate_credentials(w: WorkspaceClient):
"""For Azure, this command migrate Azure Service Principals, which have Storage Blob Data Contributor,
Storage Blob Data Reader, Storage Blob Data Owner roles on ADLS Gen2 locations that are being used in
Databricks, to UC storage credentials.
The Azure Service Principals to location mapping are listed in /Users/{user_name}/.ucx/azure_storage_account_info.csv
which is generated by principal_prefix_access command. Please review the file and delete the Service Principals
you do not want to be migrated.
The command will only migrate the Service Principals that have client secret stored in Databricks Secret.
"""
prompts = Prompts()
if w.config.is_azure:
logger.info("Running migrate_credentials for Azure")
installation = Installation.current(w, 'ucx')
service_principal_migration = ServicePrincipalMigration.for_cli(w, installation, prompts)
service_principal_migration.run(prompts)
if w.config.is_aws:
logger.error("migrate_credentials is not yet supported in AWS")
if w.config.is_gcp:
logger.error("migrate_credentials is not yet supported in GCP")


if __name__ == "__main__":
ucx()
Loading
Loading