Skip to content

Commit ac779ef

Browse files
JCZuurmondnfx
andauthored
Clean up left over uber principal resources for Azure (#2370)
## Changes Clean up the resources created for the Azure uber principal if the creation fails (half way). ### Linked issues Partially resolves #2360 (the azure part) Resolves #2363 ### Functionality - [x] modified existing command: `databricks labs ucx create-uber-principal` ### Tests - [ ] manually tested - [x] added unit tests - [x] added integration tests --------- Co-authored-by: Serge Smertin <[email protected]>
1 parent 53c9149 commit ac779ef

File tree

9 files changed

+1093
-260
lines changed

9 files changed

+1093
-260
lines changed

src/databricks/labs/ucx/azure/access.py

Lines changed: 289 additions & 108 deletions
Large diffs are not rendered by default.

src/databricks/labs/ucx/azure/resources.py

Lines changed: 172 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import time
33
from collections.abc import Iterable
44
from dataclasses import dataclass, field
5+
from datetime import timedelta
56
from typing import Any
67

78
from databricks.sdk.core import (
@@ -13,6 +14,7 @@
1314
credentials_strategy,
1415
)
1516
from databricks.sdk.errors import NotFound, PermissionDenied, ResourceConflict
17+
from databricks.sdk.retries import retried
1618

1719
from databricks.labs.ucx.assessment.crawlers import logger
1820

@@ -141,6 +143,7 @@ class PrincipalSecret:
141143

142144
@dataclass
143145
class AzureRoleAssignment:
146+
id: str
144147
resource: AzureResource
145148
scope: AzureResource
146149
principal: Principal
@@ -322,22 +325,130 @@ def create_service_principal(self, display_name: str) -> PrincipalSecret:
322325
assert principal_type is not None
323326
assert directory_id is not None
324327
assert secret is not None
325-
return PrincipalSecret(Principal(client_id, display_name, object_id, principal_type, directory_id), secret)
326-
327-
def delete_service_principal(self, principal_id: str):
328+
principal_secret = PrincipalSecret(
329+
Principal(client_id, display_name, object_id, principal_type, directory_id), secret
330+
)
331+
logger.info(
332+
f"Created service principal ({principal_secret.client.client_id}) with access to used storage accounts: "
333+
+ principal_secret.client.display_name
334+
)
335+
return principal_secret
336+
337+
def delete_service_principal(self, principal_id: str, *, safe: bool = False):
338+
"""Delete the service principal.
339+
340+
Parameters
341+
----------
342+
principal_id : str
343+
The principal id to delete.
344+
safe : bool, optional (default: True)
345+
If True, will not raise an error if the service principal does not exists.
346+
347+
Raises
348+
------
349+
NotFound :
350+
If the principal is not found.
351+
PermissionDenied :
352+
If missing permission to delete the service principal
353+
"""
328354
try:
329355
self._graph.delete(f"/v1.0/applications(appId='{principal_id}')")
330356
except PermissionDenied:
331357
msg = f"User doesnt have permission to delete application {principal_id}"
332358
logger.error(msg)
333359
raise PermissionDenied(msg) from None
360+
except NotFound:
361+
if safe:
362+
return
363+
raise
364+
365+
def _log_permission_denied_error_for_storage_permission(self, path: str) -> None:
366+
logger.error(
367+
"Permission denied. Please run this cmd under the identity of a user who has "
368+
f"create service principal permission: {path}"
369+
)
370+
371+
def get_storage_permission(
372+
self,
373+
storage_account: StorageAccount,
374+
role_guid: str,
375+
*,
376+
timeout: timedelta = timedelta(seconds=1),
377+
) -> AzureRoleAssignment | None:
378+
"""Get a storage permission.
379+
380+
Parameters
381+
----------
382+
storage_account : StorageAccount
383+
The storage account to get the permission for.
384+
role_guid : str
385+
The role guid to get the permission for.
386+
timeout : timedelta, optional (default: timedelta(seconds=1))
387+
The timeout to wait for the permission to be found.
388+
389+
Raises
390+
------
391+
PermissionDenied :
392+
If user is missing permission to get the storage permission.
393+
"""
394+
retry = retried(on=[NotFound], timeout=timeout)
395+
path = f"{storage_account.id}/providers/Microsoft.Authorization/roleAssignments/{role_guid}"
396+
try:
397+
response = retry(self._mgmt.get)(path, "2022-04-01")
398+
assignment = self._role_assignment(response, str(storage_account.id))
399+
return assignment
400+
except TimeoutError: # TimeoutError is raised by retried
401+
logger.warning(f"Storage permission not found: {path}") # not found because retry on NotFound
402+
return None
403+
except PermissionDenied:
404+
self._log_permission_denied_error_for_storage_permission(path)
405+
raise
406+
407+
def _get_storage_permissions(
408+
self,
409+
principal_id: str,
410+
storage_account: StorageAccount,
411+
) -> Iterable[AzureRoleAssignment]:
412+
"""Get storage permissions for a principal.
413+
414+
Parameters
415+
----------
416+
principal_id : str
417+
The principal id to get the storage permissions for.
418+
storage_account : StorageAccount
419+
The storage account to get the permission for.
420+
421+
Yields
422+
------
423+
AzureRoleAssignment :
424+
The role assignment
425+
426+
Raises
427+
------
428+
PermissionDenied :
429+
If user is missing permission to get the storage permission.
430+
"""
431+
path = (
432+
f"{storage_account.id}/providers/Microsoft.Authorization/roleAssignments"
433+
f"?$filter=principalId%20eq%20'{principal_id}'"
434+
)
435+
try:
436+
response = self._mgmt.get(path, "2022-04-01")
437+
except PermissionDenied:
438+
self._log_permission_denied_error_for_storage_permission(path)
439+
raise
440+
441+
for role_assignment in response.get("value", []):
442+
assignment = self._role_assignment(role_assignment, str(storage_account.id))
443+
if assignment:
444+
yield assignment
334445

335446
def apply_storage_permission(
336447
self, principal_id: str, storage_account: StorageAccount, role_name: str, role_guid: str
337448
):
449+
role_id = _ROLES[role_name]
450+
path = f"{storage_account.id}/providers/Microsoft.Authorization/roleAssignments/{role_guid}"
338451
try:
339-
role_id = _ROLES[role_name]
340-
path = f"{storage_account.id}/providers/Microsoft.Authorization/roleAssignments/{role_guid}"
341452
role_definition_id = f"/subscriptions/{storage_account.id.subscription_id}/providers/Microsoft.Authorization/roleDefinitions/{role_id}"
342453
body = {
343454
"properties": {
@@ -353,12 +464,54 @@ def apply_storage_permission(
353464
f" for spn {principal_id}."
354465
)
355466
except PermissionDenied:
356-
msg = (
357-
"Permission denied. Please run this cmd under the identity of a user who has "
358-
"create service principal permission."
467+
self._log_permission_denied_error_for_storage_permission(path)
468+
raise
469+
470+
def _delete_storage_permission(
471+
self, principal_id: str, storage_account: StorageAccount, *, safe: bool = False
472+
) -> None:
473+
"""See meth:delete_storage_permission"""
474+
try:
475+
storage_permissions = list(self._get_storage_permissions(principal_id, storage_account))
476+
except NotFound:
477+
if safe:
478+
return
479+
raise
480+
permission_denied_ids = []
481+
for permission in storage_permissions:
482+
try:
483+
self._mgmt.delete(permission.id, "2022-04-01")
484+
except PermissionDenied:
485+
self._log_permission_denied_error_for_storage_permission(permission.id)
486+
permission_denied_ids.append(permission.id)
487+
except NotFound:
488+
continue # Somehow deleted right in-between getting and deleting
489+
if permission_denied_ids:
490+
raise PermissionDenied(
491+
f"Permission denied for deleting role assignments: {', '.join(permission_denied_ids)}"
359492
)
360-
logger.error(msg)
361-
raise PermissionDenied(msg) from None
493+
494+
def delete_storage_permission(
495+
self, principal_id: str, *storage_accounts: StorageAccount, safe: bool = False
496+
) -> None:
497+
"""Delete storage permission(s) for a principal
498+
499+
Parameters
500+
----------
501+
principal_id : str
502+
The principal id to delete the role assignment(s) for.
503+
storage_accounts : StorageAccount
504+
The storage account(s) to delete permission for.
505+
safe : bool, optional (default: False)
506+
If True, will not raise an exception if no role assignment are found.
507+
508+
Raises
509+
------
510+
PermissionDenied :
511+
If user is missing permission to get the storage permission.
512+
"""
513+
for storage_account in storage_accounts:
514+
self._delete_storage_permission(principal_id, storage_account, safe=safe)
362515

363516
def tenant_id(self):
364517
token = self._mgmt.token()
@@ -427,20 +580,22 @@ def role_assignments(
427580
principal_types = ["ServicePrincipal"]
428581
result = self._mgmt.get(f"{resource_id}/providers/Microsoft.Authorization/roleAssignments", "2022-04-01")
429582
for role_assignment in result.get("value", []):
430-
assignment = self._role_assignment(role_assignment, resource_id, principal_types)
583+
principal_type = role_assignment.get("properties", {}).get("principalType")
584+
if not principal_type and principal_type not in principal_types:
585+
continue
586+
assignment = self._role_assignment(role_assignment, resource_id)
431587
if not assignment:
432588
continue
433589
yield assignment
434590

435-
def _role_assignment(
436-
self, role_assignment: dict, resource_id: str, principal_types: list[str]
437-
) -> AzureRoleAssignment | None:
591+
def _role_assignment(self, role_assignment: dict, resource_id: str) -> AzureRoleAssignment | None:
592+
id_ = role_assignment.get("id")
593+
if not id_:
594+
return None
438595
assignment_properties = role_assignment.get("properties", {})
439596
principal_type = assignment_properties.get("principalType")
440597
if not principal_type:
441598
return None
442-
if principal_type not in principal_types:
443-
return None
444599
principal_id = assignment_properties.get("principalId")
445600
if not principal_id:
446601
return None
@@ -460,6 +615,7 @@ def _role_assignment(
460615
if scope == "/":
461616
scope = resource_id
462617
return AzureRoleAssignment(
618+
id=id_,
463619
resource=AzureResource(resource_id),
464620
scope=AzureResource(scope),
465621
principal=principal,

0 commit comments

Comments
 (0)