Fixed the entitlements application for account-level groups

[renardeinside]

Addresses the issues in #488 .

Problem Statement

• Setup:
  • We have a ws and acc group.
  • Ws group has an entitlement
• Crawler:
  • During the inventorization, the entitlement is saved into the inventory without any issues into a Permissions object with object_type="entitlements" and object_id="workspace_group_id"
• Appy to backups:
  • Backup group is created
  • Entitlements are applied to the backup group
• Replace:
  • Simply replaces the groups
• Apply to acc groups (separate task)
  • Migration state becomes lost and there is no link between the workspace group id and the acc group id anymore.
  • Since there is no linkage in the migration state, the is_item_relevant method returns None, therefore it won’t apply the proper group entitlements.

Design

Together with @william-conti we've decided that the migration_state object needs to be persisted across the replace and apply_to_account tasks to properly save the state and avoid losing the logical association between ws and acc groups.

[codecov]

Codecov Report

Merging #529 (b43de34) into main (f9b6117) will increase coverage by 0.78%.
Report is 1 commits behind head on main.
The diff coverage is 85.57%.

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
+ Coverage   80.98%   81.77%   +0.78%     
==========================================
  Files          31       33       +2     
  Lines        3392     3478      +86     
  Branches      658      673      +15     
==========================================
+ Hits         2747     2844      +97     
+ Misses        491      475      -16     
- Partials      154      159       +5     

Files	Coverage Δ
...x/hive_metastore/hms_lineage_global_init_script.py	100.00% <100.00%> (ø)
src/databricks/labs/ucx/workspace_access/groups.py	75.38% <97.77%> (+9.64%)	⬆️
.../databricks/labs/ucx/workspace_access/migration.py	50.00% <50.00%> (ø)
.../databricks/labs/ucx/hive_metastore/hms_lineage.py	92.30% <92.30%> (ø)
src/databricks/labs/ucx/install.py	81.85% <90.00%> (+0.29%)	⬆️
src/databricks/labs/ucx/runtime.py	44.88% <10.00%> (-1.46%)	⬇️

[william-conti]

Migration state must be re-instatiated between each invocations, the same as we do in runtime.py.

The test should look like this then:

def test_scim(ws: WorkspaceClient, make_ucx_group, sql_backend, inventory_schema):
    """
    This test does the following:
    * create a ws group with roles and entitlements
    * migrate this group
    * verify that the migrated group has the same roles and entitlements
    :return:
    """
    ws_group, acc_group = make_ucx_group()

    _patch_by_id(ws, ws_group.id, "entitlements", [iam.ComplexValue(value="databricks-sql-access")])
    groups_config = GroupsConfig(selected=[ws_group.display_name])

    #Task 1 - crawl_permissions
    scim_support = ScimSupport(ws)
    pi = PermissionManager(sql_backend, inventory_schema, [scim_support])
    pi.cleanup()
    pi.inventorize_permissions()

    # Task 2 - apply_permissions_to_backup_groups
    group_manager = GroupManager(ws, groups_config)
    group_manager.prepare_groups_in_environment()
    pi.apply_group_permissions(group_manager.migration_state, destination="backup")

    #Task 3 - apply_permissions_to_account_groups
    group_manager = GroupManager(ws, groups_config)
    group_manager.prepare_groups_in_environment()
    group_manager.replace_workspace_groups_with_account_groups()

    #Task 4 - apply_permissions_to_account_groups
    migration_state = GroupManager.prepare_apply_permissions_to_account_groups(ws, groups_config.backup_group_prefix)
    pi.apply_group_permissions(migration_state, destination="account")
    assert iam.ComplexValue(value="databricks-sql-access") in ws.groups.get(acc_group.id).entitlements

Which, in my environment, doesn't pass

[nfx]

Keep in mind that we're getting rid of the backup groups very soon: #450

[renardeinside]

@nfx could you please tell us what "very soon" means specifically? is it in 2-3 days or we're speaking about longer time periods I would suggest going further with proper fix.

[nfx]

Changes are probably fine, but all integration tests must pass for this to merge.

Keep in mind that #450 is doing the fix fundamentally.

[renardeinside]

would be nice to address the backend topic (I have a feeling it's errorneous).