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

Allowing skipping TACLs migration during table migration. #3384

Merged
merged 3 commits into from
Dec 2, 2024
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,7 @@ access the configuration file from the command line. Here's the description of c
* `num_threads`: An optional integer representing the number of threads to use for migration.
* `database_to_catalog_mapping`: An optional dictionary mapping source database names to target catalog names.
* `default_catalog`: An optional string representing the default catalog name.
* `skip_tacl_migration`: Optional flag, allow skipping TACL migration when migrating tables or creating catalogs and schemas.
* `default_owner_group`: Assigns this group to all migrated objects (catalogs, databases, tables, views, etc.). The group has to be an account group and the user running the migration has to be a member of this group.
* `log_level`: An optional string representing the log level.
* `workspace_start_path`: A string representing the starting path for notebooks and directories crawler in the workspace.
Expand Down
3 changes: 3 additions & 0 deletions src/databricks/labs/ucx/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ class WorkspaceConfig: # pylint: disable=too-many-instance-attributes
# Default table owner, assigned to a group
default_owner_group: str | None = None

# Skip TACL migration during table migration
skip_tacl_migration: bool = False

def replace_inventory_variable(self, text: str) -> str:
return text.replace("$inventory", f"hive_metastore.{self.inventory_database}")

Expand Down
1 change: 1 addition & 0 deletions src/databricks/labs/ucx/contexts/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ def migrate_grants(self) -> MigrateGrants:
self.sql_backend,
self.group_manager,
grant_loaders,
skip_tacl_migration=self.config.skip_tacl_migration,
)

@cached_property
Expand Down
7 changes: 7 additions & 0 deletions src/databricks/labs/ucx/hive_metastore/grants.py
Original file line number Diff line number Diff line change
Expand Up @@ -777,13 +777,20 @@ def __init__(
sql_backend: SqlBackend,
group_manager: GroupManager,
grant_loaders: list[Callable[[], Iterable[Grant]]],
*,
skip_tacl_migration: bool = False,
):
self._sql_backend = sql_backend
self._group_manager = group_manager
# List of grant loaders, the assumption that the first one is a loader for ownership grants
self._grant_loaders = grant_loaders
if skip_tacl_migration:
logger.warning("Skipping TACL migration")
self._skip_tacl_migration = skip_tacl_migration

def apply(self, src: SecurableObject, dst: SecurableObject) -> bool:
if self._skip_tacl_migration:
return True
grants = self._match_grants(src)
for grant in grants:
acl_migrate_sql = grant.uc_grant_sql(dst.kind, dst.full_name)
Expand Down
3 changes: 3 additions & 0 deletions src/databricks/labs/ucx/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ def _prompt_for_new_installation(self) -> WorkspaceConfig:
configure_groups.run()
include_databases = self._select_databases()

skip_tacl_migration = self.prompts.confirm("Do you want to skip TACL migration when migrating tables?")

# Checking if the user wants to define a default owner group.
default_owner_group = None
if self.prompts.confirm("Do you want to define a default owner group for all tables and schemas? "):
Expand All @@ -262,6 +264,7 @@ def _prompt_for_new_installation(self) -> WorkspaceConfig:
group_match_by_external_id=configure_groups.group_match_by_external_id, # type: ignore[arg-type]
include_group_names=configure_groups.include_group_names,
renamed_group_prefix=configure_groups.renamed_group_prefix,
skip_tacl_migration=skip_tacl_migration,
log_level=log_level,
num_threads=num_threads,
include_databases=include_databases,
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/hive_metastore/test_grants.py
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,41 @@ def test_grant_supports_history(mock_backend, grant_record: Grant, history_recor
assert rows == [history_record]


def test_migrate_grants_skip():
group_manager = create_autospec(GroupManager)
backend = MockBackend()

src = Table("hive_metastore", "database", "table", "MANAGED", "DELTA")
grant = Grant("user", "SELECT")
dst = Table("catalog", "database", "table", "MANAGED", "DELTA")

def grant_loader() -> list[Grant]:
catalog = src.catalog
database = src.database
table = src.name
return [
dataclasses.replace(
grant,
catalog=catalog,
database=database,
table=table,
),
]

migrate_grants = MigrateGrants(
backend,
group_manager,
[grant_loader],
skip_tacl_migration=True,
)

migrate_grants.apply(src, dst)

for query in backend.queries:
assert not query.startswith("GRANT")
group_manager.assert_not_called()


# Testing the validation in retrival of the default owner group. 666 is the current_user user_id.
@pytest.mark.parametrize("user_id, expected", [("666", True), ("777", False)])
def test_default_owner(user_id, expected) -> None:
Expand Down
1 change: 1 addition & 0 deletions tests/unit/install/test_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ def result():
),
(r"Min workers for auto-scale.*", "2", {"min_workers": 2}),
(r"Max workers for auto-scale.*", "20", {"max_workers": 20}),
(r"Do you want to skip TACL.*", "yes", {"skip_tacl_migration": True}),
],
)
def test_configure_sets_expected_workspace_configuration_values(
Expand Down