jerryshao commented on code in PR #13834:
URL: https://github.com/apache/gravitino/pull/13834#discussion_r4409338555
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -3814,6 +3820,108 @@ public CapabilityResult caseSensitiveOnName(Scope
scope) {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void
testMetalakeDropEvictsEveryMetadataTypeAndKeepsOtherMetalake(boolean peerReplay)
+ throws Exception {
+ // Use a dedicated metalake so the shared MetadataIdConverter stubs do not
leak into other
+ // tests. The other metalake name extends the dropped one to check the
name boundary.
+ String metalakeName = peerReplay ? "peer_drop_ml" : "local_drop_ml";
+ String otherMetalake = metalakeName + "_other";
+ GravitinoCache<String, Long> metadataIdCache =
getMetadataIdCache(jcasbinAuthorizer);
+ JcasbinAuthorizationLookups lookups =
+ new JcasbinAuthorizationLookups(metadataIdCache,
getOwnerRelCache(jcasbinAuthorizer));
+ MetadataObject.Type[] types = MetadataObject.Type.values();
+ for (int i = 0; i < types.length; i++) {
+ MetadataObject object = metadataObjectOfType(metalakeName, types[i]);
+ MetadataObject otherObject = metadataObjectOfType(otherMetalake,
types[i]);
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(object, metalakeName))
+ .thenReturn(Optional.of(1000L + i), Optional.of(2000L + i));
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(otherObject, otherMetalake))
+ .thenReturn(Optional.of(3000L + i));
+
+ assertEquals(Optional.of(1000L + i), resolve(lookups, object,
metalakeName));
+ assertEquals(Optional.of(3000L + i), resolve(lookups, otherObject,
otherMetalake));
+ assertEquals(
+ Optional.of(1000L + i),
+ metadataIdCache.getIfPresent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalakeName,
object)),
+ types[i].name());
+ }
+
+ metalakeDropInvalidation(metalakeName, peerReplay).run();
+
+ for (int i = 0; i < types.length; i++) {
+ MetadataObject object = metadataObjectOfType(metalakeName, types[i]);
+ MetadataObject otherObject = metadataObjectOfType(otherMetalake,
types[i]);
+ assertEquals(
+ Optional.empty(),
+ metadataIdCache.getIfPresent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalakeName,
object)),
+ types[i].name());
+ assertEquals(
+ Optional.of(3000L + i),
+ metadataIdCache.getIfPresent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(otherMetalake,
otherObject)),
+ types[i].name());
+ // The recreated metalake resolves same-named objects to their new IDs.
+ assertEquals(Optional.of(2000L + i), resolve(lookups, object,
metalakeName));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void
testRecreatedMetalakeDoesNotInheritOldOwnershipOrPermissions(boolean peerReplay)
+ throws Exception {
+ // Use a dedicated metalake so the shared mapper and MetadataIdConverter
stubs do not leak into
+ // other tests.
+ String metalakeName = peerReplay ? "peer_recreate_ml" :
"local_recreate_ml";
+ String roleName = "recreated_role";
+ long oldCatalogId = 7001L;
+ long oldUserId = 7002L;
+ long oldRoleId = 7003L;
+ long newCatalogId = 8001L;
+ long newUserId = 8002L;
+ long newRoleId = 8003L;
+ long otherCatalogId = 8004L;
+ long otherOwnerId = 9999L;
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ MetadataObject catalog = MetadataObjects.parse("cat",
MetadataObject.Type.CATALOG);
+ MetadataObject otherCatalog = MetadataObjects.parse("other_cat",
MetadataObject.Type.CATALOG);
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(catalog, metalakeName))
+ .thenReturn(Optional.of(oldCatalogId), Optional.of(newCatalogId));
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(otherCatalog, metalakeName))
+ .thenReturn(Optional.of(otherCatalogId));
+
when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(oldCatalogId),
eq("CATALOG")))
+ .thenReturn(new OwnerInfo(oldUserId, "USER"));
+
when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(newCatalogId),
eq("CATALOG")))
+ .thenReturn(new OwnerInfo(otherOwnerId, "USER"));
+
when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(otherCatalogId),
eq("CATALOG")))
+ .thenReturn(new OwnerInfo(newUserId, "USER"));
+
+ // Old metalake: the user owns "cat" and holds a role that grants
USE_CATALOG on it.
+ mockMetalakeUserWithRole(metalakeName, oldUserId, oldRoleId, roleName,
"cat");
+ assertTrue(authorizeUseCatalog(principal, metalakeName, catalog));
+ assertTrue(isOwner(principal, metalakeName, catalog));
+
+ // Drop the metalake, then recreate the same user, role and catalog names
with new IDs. The
+ // new role grants USE_CATALOG on "other_cat" instead, and someone else
owns "cat".
+ metalakeDropInvalidation(metalakeName, peerReplay).run();
+ mockedRoleVersions.remove(oldRoleId);
+ mockMetalakeUserWithRole(metalakeName, newUserId, newRoleId, roleName,
"other_cat");
Review Comment:
[Nit] This recreate test only exercises the user -> role path.
`mockMetalakeUserWithRole` (line 4692) stubs `userMetaMapper.getUserUpdatedAt`
and `roleMetaMapper.listRolesByUserId`, so the symmetric group path is never
touched, even though the review request that prompted this test named the
group-role cache as well.
`loadGroupRoles` has its own guard — `cached.getGroupId() == groupId &&
cached.getUpdatedAt() >= groupInfo.getUpdatedAt()`
(server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:1340-1347)
— which is the exact thing this test is meant to prove still holds after a
metalake is recreated with new entity IDs. Warming `groupRoleCache` with a
group-bound role and re-mocking the group with a new `groupId` after the drop
would close the gap cheaply, and would catch a future regression in the group
guard that the user guard would hide.
Verified by: reading `loadUserRoles` (1285-1307) and `loadGroupRoles`
(1331-1356) side by side, and the new helper's stubs at 4692-4716 — no
`GroupMetaMapper.getGroupUpdatedAt` or `listRolesByGroupId` stubbing anywhere
in the added test code.
##########
core/src/main/java/org/apache/gravitino/hook/MetalakeHookDispatcher.java:
##########
@@ -105,7 +105,11 @@ public boolean dropMetalake(NameIdentifier ident, boolean
force)
throws NonEmptyEntityException, MetalakeInUseException {
// Child catalog/schema/fileset write-through secrets are cleaned when
MetalakeManager
// force-drops catalogs via CatalogManager.dropCatalog →
FilesetCatalogOperations.dropSchema.
- return dispatcher.dropMetalake(ident, force);
+ boolean dropped = dispatcher.dropMetalake(ident, force);
+ if (dropped) {
Review Comment:
[Question] This is the one case where the node can keep stale entries:
`MetalakeManager.dropMetalake` maps a concurrent removal to `false` rather than
an exception
(core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:409-412,
`NoSuchMetalakeException | NoSuchEntityException` -> `return false`), and
`store.delete` also returns `false` when the row is already gone. In that path
this node skips the eviction while its `metadataIdCache` may still hold the
dropped metalake's prefix.
It does self-heal: the peer that won the delete wrote a METALAKE DROP row,
and this node's `JcasbinChangeListener` replays it within
`gravitino.authorization.jcasbin.changePollIntervalSecs` (default 3s),
re-entering the same prefix invalidation. So this is a question, not a blocker
— but since a `metadataIdCache` entry is always re-loadable, invalidating
regardless of the return value costs nothing and removes the window. Was the
guard deliberate (keeping the hook a no-op when this node changed nothing)? If
so, leaving it as is seems fine to me.
Verified by: reading `MetalakeManager.dropMetalake` (lines 369-424) for
every path that returns `false`, and confirming `selectEntityChanges` applies
no entity-type filter (EntityChangeLogBaseSQLProvider.java:52-59) so the peer's
METALAKE row is in fact replayed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
