Breaking change: Mitigate permissions loss in Table ACLs by folding grants belonging to the same principal, object id and object type together

[mwojtyczka]

Applying grants that belong to the same principle, object id, and object type concurrently is not supported in legacy Table ACLs currently:

TableAcl grant/revoke operations are not atomic. When granting the permissions, the service would first get all existing permissions, append with the new permissions, and set the full list in the database. If there are concurrent grant requests, both requests might succeed and emit the audit logs, but what actually happens could be that the new permission list from one request overrides the other one, causing permission loss.

This PR will:

• Resolve issue 499.
• Improve performance of migrating Table ACLs permissions
• Added tests covering objects ownership
• Increased test coverage around Table ACLs

Example

Instead of executing grants in separate transactions:
GRANT SELECT ON TABLE hive_metastore.db_a.table_a TO group_a
GRANT MODIFY ON TABLE hive_metastore.db_a.table_a TO group_a

Folding and executing in one statement:
GRANT SELECT, MODIFY ON TABLE hive_metastore.db_a.table_a TO group_a

[codecov]

Codecov Report

Merging #512 (4a3fc6f) into main (18e3b54) will increase coverage by 0.08%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
+ Coverage   80.64%   80.73%   +0.08%     
==========================================
  Files          31       31              
  Lines        3240     3265      +25     
  Branches      626      633       +7     
==========================================
+ Hits         2613     2636      +23     
  Misses        478      478              
- Partials      149      151       +2     

Files	Coverage Δ
...rc/databricks/labs/ucx/workspace_access/manager.py	82.35% <100.00%> (ø)
src/databricks/labs/ucx/workspace_access/tacl.py	96.36% <93.10%> (-0.31%)	⬇️

... and 1 file with indirect coverage changes

[nfx]

Suggested change

	folded = collections.defaultdict(lambda: {"action_type": set()})
	folded = collections.defaultdict(set)

Why do we need more than a set?

[mwojtyczka]

corrected

[nfx]

Why do we need this?

[mwojtyczka]

I simplified this a bit. We need to fold (put together) action types for all grants that have the same principal, object id and type. We then put the actions into the grant object. We only take one grant object per principal, object id and type, and just update its actions. The rest of the grants can be skipped.

[nfx]

Don't assert on JSON string, assert on fields. Keys will be serialized in different order each time and flake the test

[mwojtyczka]

corrected

[nfx]

Don't assert on JSON string

[mwojtyczka]

corrected

[nfx]

this is a risk of corrupting data, can't accept this PR

[mwojtyczka]

corrected

[nfx]

do reconstruction of grants, not the overwrite

Index: src/databricks/labs/ucx/workspace_access/tacl.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/databricks/labs/ucx/workspace_access/tacl.py b/src/databricks/labs/ucx/workspace_access/tacl.py
--- a/src/databricks/labs/ucx/workspace_access/tacl.py	(revision 45bae4a880095ce35719cc01c27c05f1786ac711)
+++ b/src/databricks/labs/ucx/workspace_access/tacl.py	(date 1698304643087)
@@ -1,5 +1,5 @@
+import collections
 import dataclasses
-import functools
 import json
 from collections.abc import Callable, Iterator
 from functools import partial
@@ -21,18 +21,37 @@
         self._sql_backend = sql_backend
 
     def get_crawler_tasks(self) -> Iterator[Callable[..., Permissions | None]]:
-        def inner(grant: Grant) -> Permissions:
-            object_type, object_key = grant.this_type_and_key()
-            return Permissions(object_type=object_type, object_id=object_key, raw=json.dumps(dataclasses.asdict(grant)))
-
+        folded = collections.defaultdict(set)
         for grant in self._grants_crawler.snapshot():
-            yield functools.partial(inner, grant)
+            folded[(grant.principal, grant.this_type_and_key())].add(grant.action_type)
+        for (principal, (object_type, key)), actions in folded.items():
+            yield lambda: Permissions(
+                object_type=object_type,
+                object_id=f'{principal}/{key}',
+                raw=json.dumps(sorted(actions)))
 
     def object_types(self) -> set[str]:
         return {"TABLE", "DATABASE", "VIEW", "CATALOG", "ANONYMOUS FUNCTION", "ANY FILE"}
 
+    def _from_reduced(self, item: Permissions):
+        actions = ', '.join(json.loads(item.raw))
+        principal, object_id = item.object_id
+        raw = {'principal': principal, 'action_type': actions}
+        match item.object_type:
+            case 'TABLE':
+                return raw | {'table': object_id}
+            case 'VIEW':
+                return raw | {'view': object_id}
+            case 'DATABASE':
+                return raw | {'database': object_id}
+            case 'ANONYMOUS FUNCTION':
+                return raw | {'anonymous_function': object_id}
+            case 'CATALOG':
+                return raw | {'catalog': object_id}
+
     def get_apply_task(self, item: Permissions, migration_state: GroupMigrationState, destination: Destination):
-        grant = Grant(**json.loads(item.raw))
+        raw = self._from_reduced(item)
+        grant = Grant(**raw)
         target_principal = migration_state.get_target_principal(grant.principal, destination)
         if target_principal is None:
             # this is a grant for user, service principal, or irrelevant group

[mwojtyczka]

do reconstruction of grants, not the overwrite

Index: src/databricks/labs/ucx/workspace_access/tacl.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/databricks/labs/ucx/workspace_access/tacl.py b/src/databricks/labs/ucx/workspace_access/tacl.py
--- a/src/databricks/labs/ucx/workspace_access/tacl.py	(revision 45bae4a880095ce35719cc01c27c05f1786ac711)
+++ b/src/databricks/labs/ucx/workspace_access/tacl.py	(date 1698304643087)
@@ -1,5 +1,5 @@
+import collections
 import dataclasses
-import functools
 import json
 from collections.abc import Callable, Iterator
 from functools import partial
@@ -21,18 +21,37 @@
         self._sql_backend = sql_backend
 
     def get_crawler_tasks(self) -> Iterator[Callable[..., Permissions | None]]:
-        def inner(grant: Grant) -> Permissions:
-            object_type, object_key = grant.this_type_and_key()
-            return Permissions(object_type=object_type, object_id=object_key, raw=json.dumps(dataclasses.asdict(grant)))
-
+        folded = collections.defaultdict(set)
         for grant in self._grants_crawler.snapshot():
-            yield functools.partial(inner, grant)
+            folded[(grant.principal, grant.this_type_and_key())].add(grant.action_type)
+        for (principal, (object_type, key)), actions in folded.items():
+            yield lambda: Permissions(
+                object_type=object_type,
+                object_id=f'{principal}/{key}',
+                raw=json.dumps(sorted(actions)))
 
     def object_types(self) -> set[str]:
         return {"TABLE", "DATABASE", "VIEW", "CATALOG", "ANONYMOUS FUNCTION", "ANY FILE"}
 
+    def _from_reduced(self, item: Permissions):
+        actions = ', '.join(json.loads(item.raw))
+        principal, object_id = item.object_id
+        raw = {'principal': principal, 'action_type': actions}
+        match item.object_type:
+            case 'TABLE':
+                return raw | {'table': object_id}
+            case 'VIEW':
+                return raw | {'view': object_id}
+            case 'DATABASE':
+                return raw | {'database': object_id}
+            case 'ANONYMOUS FUNCTION':
+                return raw | {'anonymous_function': object_id}
+            case 'CATALOG':
+                return raw | {'catalog': object_id}
+
     def get_apply_task(self, item: Permissions, migration_state: GroupMigrationState, destination: Destination):
-        grant = Grant(**json.loads(item.raw))
+        raw = self._from_reduced(item)
+        grant = Grant(**raw)
         target_principal = migration_state.get_target_principal(grant.principal, destination)
         if target_principal is None:
             # this is a grant for user, service principal, or irrelevant group

Maybe I am wrong here but IMO this implementation proposal does not make sense for several reasons:

• The raw permissions is incomplete. It only contains actions. This may introduce a lot of bugs because the consumers may not be aware that the permissions object is incomplete. It will also break a lot of existing tests.
• object id will contain both the object and principal. Why an object id should ever contain information about a principal? it does not fit logically. It will also break a lot of existing tests.
• The permissions is reconstructed wrongly. A lot of fields will be missing in the raw field. Again this will break a lot of existing tests.

IMO the crawler should return complete permissions to avoid issues with consumers of the method that may not be aware of the limitation that the raw object is incomplete. A better approach would be to create a complete permissions by creating a Grant based on the information at hand. I pushed an updated version for doing exactly that. It reuses the proposed code partially but do this on the crawler side, not applier.