Fixed the entitlements application for account-level groups by renardeinside · Pull Request #529 · databrickslabs/ucx

renardeinside · 2023-10-30T19:09:37Z

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 · 2023-10-31T16:01:25Z

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 · 2023-11-02T08:37:01Z

tests/integration/workspace_access/test_scim.py

+    pi.inventorize_permissions()
+    pi.apply_group_permissions(group_manager.migration_state, destination="backup")
+    group_manager.replace_workspace_groups_with_account_groups()
+    pi.apply_group_permissions(group_manager.migration_state, destination="account")


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 · 2023-11-02T09:15:40Z

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

renardeinside · 2023-11-02T10:14:03Z

@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).

src/databricks/labs/ucx/runtime.py

@william-conti

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. --------- Co-authored-by: William Conti <[email protected]>

**Breaking changes** (existing installations need to reinstall UCX and re-run assessment jobs) * Switched local group migration component to rename groups instead of creating backup groups ([#450](#450)). * Mitigate permissions loss in Table ACLs by folding grants belonging to the same principal, object id and object type together ([#512](#512)). **New features** * Added support for the experimental Databricks CLI launcher ([#517](#517)). * Added support for external Hive Metastores including AWS Glue ([#400](#400)). * Added more views to assessment dashboard ([#474](#474)). * Added rate limit for creating backup group to increase stability ([#500](#500)). * Added deduplication for mount point list ([#569](#569)). * Added documentation to describe interaction with external Hive Metastores ([#473](#473)). * Added failure injection for job failure message propagation ([#591](#591)). * Added uniqueness in the new warehouse name to avoid conflicts on installation ([#542](#542)). * Added a global init script to collect Hive Metastore lineage ([#513](#513)). * Added retry set/update permissions when possible and assess the changes in the workspace ([#519](#519)). * Use `~/.ucx/state.json` to store the state of both dashboards and jobs ([#561](#561)). **Bug fixes** * Fixed handling for `OWN` table permissions ([#571](#571)). * Fixed handling of keys with and without values. ([#514](#514)). * Fixed integration test failures related to concurrent group delete ([#584](#584)). * Fixed issue with workspace listing process on None type `object_type` ([#481](#481)). * Fixed missing group entitlement migration bug ([#583](#583)). * Fixed entitlement application for account-level groups ([#529](#529)). * Fixed assessment throwing an error when the owner of an object is empty ([#485](#485)). * Fixed installer to migrate between different configuration file versions ([#596](#596)). * Fixed cluster policy crawler to be aware of deleted policies ([#486](#486)). * Improved error message for not null constraints violated ([#532](#532)). * Improved integration test resiliency ([#597](#597), [#594](#594), [#586](#586)). * Introduced Safer access to workspace objects' properties. ([#530](#530)). * Mitigated permissions loss in Table ACLs by running appliers with single thread ([#518](#518)). * Running apply permission task before assessment should display message ([#487](#487)). * Split integration tests from blocking the merge queue ([#496](#496)). * Support more than one dashboard per step ([#472](#472)). * Update databricks-sdk requirement from ~=0.11.0 to ~=0.12.0 ([#505](#505)). * Update databricks-sdk requirement from ~=0.12.0 to ~=0.13.0 ([#575](#575)).

renardeinside requested a review from a team October 30, 2023 19:09

renardeinside had a problem deploying to account-admin October 31, 2023 12:43 — with GitHub Actions Failure

renardeinside had a problem deploying to account-admin October 31, 2023 15:59 — with GitHub Actions Failure

renardeinside had a problem deploying to account-admin October 31, 2023 18:15 — with GitHub Actions Failure

renardeinside had a problem deploying to account-admin November 1, 2023 12:55 — with GitHub Actions Failure

renardeinside had a problem deploying to account-admin November 1, 2023 12:58 — with GitHub Actions Failure

william-conti suggested changes Nov 2, 2023

View reviewed changes

renardeinside marked this pull request as draft November 2, 2023 10:14

renardeinside had a problem deploying to account-admin November 2, 2023 10:59 — with GitHub Actions Failure

william-conti had a problem deploying to account-admin November 2, 2023 13:55 — with GitHub Actions Failure

william-conti had a problem deploying to account-admin November 2, 2023 15:58 — with GitHub Actions Failure

william-conti had a problem deploying to account-admin November 2, 2023 16:57 — with GitHub Actions Failure

william-conti had a problem deploying to account-admin November 2, 2023 18:56 — with GitHub Actions Failure

william-conti marked this pull request as ready for review November 2, 2023 19:00

nfx reviewed Nov 2, 2023

View reviewed changes

renardeinside and others added 8 commits November 2, 2023 22:25

add explicit tests

e23b21b

add explicit tests

37a2fc8

revert conftest

a7ce8b7

add e2e test

779c0b4

Adding drafted changes

05ae227

fixing stuff

ae49525

last mile checks

1dd90bb

fixing tests

6dec120

william-conti force-pushed the bugfix/apply-entitlements-to-acc-groups branch from d426d96 to 6dec120 Compare November 2, 2023 22:17

william-conti had a problem deploying to account-admin November 2, 2023 22:17 — with GitHub Actions Failure

renardeinside commented Nov 3, 2023

View reviewed changes

src/databricks/labs/ucx/runtime.py Show resolved Hide resolved

fixing integration test

693dddd

william-conti had a problem deploying to account-admin November 3, 2023 12:40 — with GitHub Actions Error

fixing runtime issue

b43de34

william-conti temporarily deployed to account-admin November 3, 2023 12:52 — with GitHub Actions Inactive

william-conti approved these changes Nov 3, 2023

View reviewed changes

william-conti added this pull request to the merge queue Nov 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 3, 2023

nfx changed the title ~~fix the entitlements application for account-level groups~~ Fixed the entitlements application for account-level groups Nov 3, 2023

nfx merged commit d16a6ee into main Nov 3, 2023

nfx deleted the bugfix/apply-entitlements-to-acc-groups branch November 3, 2023 15:38

nfx mentioned this pull request Nov 17, 2023

Release v0.6.0 #598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the entitlements application for account-level groups#529

Fixed the entitlements application for account-level groups#529
nfx merged 10 commits intomainfrom
bugfix/apply-entitlements-to-acc-groups

renardeinside commented Oct 30, 2023 •

edited

Loading

Uh oh!

codecov bot commented Oct 31, 2023 •

edited

Loading

Uh oh!

william-conti Nov 2, 2023

Uh oh!

nfx commented Nov 2, 2023 •

edited

Loading

Uh oh!

renardeinside commented Nov 2, 2023

Uh oh!

nfx left a comment

Uh oh!

renardeinside left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

renardeinside commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

william-conti Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

nfx commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renardeinside commented Nov 2, 2023

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

renardeinside left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

renardeinside commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

nfx commented Nov 2, 2023 •

edited

Loading