This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 2ac779ca81 Revert "[#10117] improvement(authz): Remove ownership
relation cache" (#10279)
2ac779ca81 is described below
commit 2ac779ca81f47ff76dc182db7b934fc4a7583611
Author: Qi Yu <[email protected]>
AuthorDate: Fri Mar 6 16:50:50 2026 +0800
Revert "[#10117] improvement(authz): Remove ownership relation cache"
(#10279)
Reverts apache/gravitino#10119
---
.../main/java/org/apache/gravitino/Configs.java | 9 ++++
.../authorization/GravitinoAuthorizer.java | 12 +++++
.../gravitino/authorization/OwnerManager.java | 30 ++++++++++++
docs/security/access-control.md | 7 ++-
.../authorization/PassThroughAuthorizer.java | 4 ++
.../authorization/jcasbin/JcasbinAuthorizer.java | 55 ++++++++++++++++------
.../authorization/MockGravitinoAuthorizer.java | 4 ++
.../jcasbin/TestJcasbinAuthorizer.java | 44 +++++++++++++++++
.../filter/TestGravitinoInterceptionService.java | 4 ++
9 files changed, 153 insertions(+), 16 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/Configs.java
b/core/src/main/java/org/apache/gravitino/Configs.java
index a26f3c36a5..3cc9d703e6 100644
--- a/core/src/main/java/org/apache/gravitino/Configs.java
+++ b/core/src/main/java/org/apache/gravitino/Configs.java
@@ -320,6 +320,15 @@ public class Configs {
.longConf()
.createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE);
+ public static final long DEFAULT_GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE =
100000L;
+
+ public static final ConfigEntry<Long>
GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE =
+ new ConfigBuilder("gravitino.authorization.jcasbin.ownerCacheSize")
+ .doc("The maximum size of the owner cache for authorization")
+ .version(ConfigConstants.VERSION_1_1_1)
+ .longConf()
+ .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE);
+
public static final ConfigEntry<List<String>> SERVICE_ADMINS =
new ConfigBuilder("gravitino.authorization.serviceAdmins")
.doc("The admins of Gravitino service")
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
index 3a4fbfc0a9..7965f173ad 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
@@ -152,4 +152,16 @@ public interface GravitinoAuthorizer extends Closeable {
throw new RuntimeException("Can not get Role Entity", e);
}
}
+
+ /**
+ * This method is called to clear the owner relationship in jcasbin when the
owner of the metadata
+ * changes.
+ *
+ * @param metalake metalake;
+ * @param oldOwnerId The old owner id;
+ * @param nameIdentifier The metadata name identifier;
+ * @param type entity type
+ */
+ void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type);
}
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
index 13fa04a80d..5b7ce28c98 100644
--- a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
+++ b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
@@ -25,6 +25,7 @@ import java.util.Optional;
import lombok.Getter;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityStore;
+import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.SupportsRelationOperations;
@@ -36,6 +37,7 @@ import org.apache.gravitino.lock.TreeLockUtils;
import org.apache.gravitino.meta.GroupEntity;
import org.apache.gravitino.meta.UserEntity;
import org.apache.gravitino.utils.MetadataObjectUtil;
+import org.apache.gravitino.utils.NameIdentifierUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -118,6 +120,7 @@ public class OwnerManager implements OwnerDispatcher {
metadataObject,
authorizationPlugin ->
authorizationPlugin.onOwnerSet(metadataObject,
originOwner.orElse(null), newOwner));
+ originOwner.ifPresent(owner -> notifyOwnerChange(owner, metalake,
metadataObject));
} catch (NoSuchEntityException nse) {
LOG.warn(
"Metadata object {} or owner {} is not found",
metadataObject.fullName(), ownerName, nse);
@@ -133,6 +136,33 @@ public class OwnerManager implements OwnerDispatcher {
}
}
+ private void notifyOwnerChange(Owner oldOwner, String metalake,
MetadataObject metadataObject) {
+ GravitinoAuthorizer gravitinoAuthorizer =
GravitinoEnv.getInstance().gravitinoAuthorizer();
+ if (gravitinoAuthorizer != null) {
+ if (oldOwner.type() == Owner.Type.USER) {
+ try {
+ UserEntity userEntity =
+ GravitinoEnv.getInstance()
+ .entityStore()
+ .get(
+ NameIdentifierUtil.ofUser(metalake, oldOwner.name()),
+ Entity.EntityType.USER,
+ UserEntity.class);
+ gravitinoAuthorizer.handleMetadataOwnerChange(
+ metalake,
+ userEntity.id(),
+ MetadataObjectUtil.toEntityIdent(metalake, metadataObject),
+ Entity.EntityType.valueOf(metadataObject.type().name()));
+ } catch (IOException e) {
+ LOG.warn(e.getMessage(), e);
+ }
+ } else {
+ throw new UnsupportedOperationException(
+ "Notification for Group Owner is not supported yet.");
+ }
+ }
+ }
+
@Override
public Optional<Owner> getOwner(String metalake, MetadataObject
metadataObject) {
NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
diff --git a/docs/security/access-control.md b/docs/security/access-control.md
index bb8e85aee1..1724cdde07 100644
--- a/docs/security/access-control.md
+++ b/docs/security/access-control.md
@@ -446,17 +446,20 @@ To enable access control in Gravitino, configure the
following settings in your
| `gravitino.authorization.serviceAdmins` | Comma-separated
list of service administrator usernames | (none) | Yes
(when authorization is enabled) | 0.5.0 |
| `gravitino.authorization.jcasbin.cacheExpirationSecs` | The expiration
time in seconds for authorization cache entries | `3600` | No
| 1.1.1 |
| `gravitino.authorization.jcasbin.roleCacheSize` | The maximum size
of the role cache for authorization | `10000` | No
| 1.1.1 |
+| `gravitino.authorization.jcasbin.ownerCacheSize` | The maximum size
of the owner cache for authorization | `100000` | No
| 1.1.1 |
### Authorization Cache
-Gravitino uses Caffeine caches to improve authorization performance by caching
role information. The cache configuration options allow you to tune the cache
behavior:
+Gravitino uses Caffeine caches to improve authorization performance by caching
role and owner information. The cache configuration options allow you to tune
the cache behavior:
- **`cacheExpirationSecs`**: Controls how long cache entries remain valid.
After this time, entries are automatically evicted and reloaded from the
backend on the next access. Lower values provide more up-to-date authorization
decisions but may increase load on the backend.
- **`roleCacheSize`**: Controls the maximum number of role entries that can be
cached. When the cache reaches this size, the least recently used entries are
evicted.
+- **`ownerCacheSize`**: Controls the maximum number of owner relationship
entries that can be cached. This cache maps metadata object IDs to their owner
IDs.
+
:::info
-When role privileges are changed through the Gravitino API, the corresponding
cache entries are automatically invalidated to ensure authorization decisions
reflect the latest state.
+When role privileges or ownership are changed through the Gravitino API, the
corresponding cache entries are automatically invalidated to ensure
authorization decisions reflect the latest state.
:::
### Important Notes
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java
index 82a5d3a82a..06e20b8ada 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java
@@ -109,6 +109,10 @@ public class PassThroughAuthorizer implements
GravitinoAuthorizer {
@Override
public void handleRolePrivilegeChange(String metalake, String roleName) {}
+ @Override
+ public void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type) {}
+
@Override
public void close() throws IOException {}
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
index 524c90f032..cea47e353b 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
@@ -29,6 +29,7 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
+import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
@@ -86,6 +87,8 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer
{
*/
private Cache<Long, Boolean> loadedRoles;
+ private Cache<Long, Optional<Long>> ownerRel;
+
private Executor executor = null;
@Override
@@ -96,6 +99,8 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer
{
.get(Configs.GRAVITINO_AUTHORIZATION_CACHE_EXPIRATION_SECS);
long roleCacheSize =
GravitinoEnv.getInstance().config().get(Configs.GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE);
+ long ownerCacheSize =
+
GravitinoEnv.getInstance().config().get(Configs.GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE);
// Initialize enforcers before the caches that reference them in removal
listeners
allowEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new
GravitinoAdapter());
@@ -116,7 +121,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
})
.build();
-
+ ownerRel =
+ Caffeine.newBuilder()
+ .expireAfterAccess(cacheExpirationSecs, TimeUnit.SECONDS)
+ .maximumSize(ownerCacheSize)
+ .build();
executor =
Executors.newFixedThreadPool(
GravitinoEnv.getInstance()
@@ -209,9 +218,15 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
String metalake,
MetadataObject metadataObject,
AuthorizationRequestContext requestContext) {
+ Long userId;
boolean result;
try {
- result = checkOwner(metalake, metadataObject, principal.getName());
+ Long metadataId = MetadataIdConverter.getID(metadataObject, metalake);
+ loadOwnerPolicy(metalake, metadataObject, metadataId);
+ UserEntity userEntity = getUserEntity(principal.getName(), metalake);
+ userId = userEntity.id();
+ metadataId = MetadataIdConverter.getID(metadataObject, metalake);
+ result = Objects.equals(Optional.of(userId),
ownerRel.getIfPresent(metadataId));
} catch (Exception e) {
LOG.debug("Can not get entity id", e);
result = false;
@@ -372,6 +387,14 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
loadedRoles.invalidate(roleId);
}
+ @Override
+ public void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type) {
+ MetadataObject metadataObject =
NameIdentifierUtil.toMetadataObject(nameIdentifier, type);
+ Long metadataId = MetadataIdConverter.getID(metadataObject, metalake);
+ ownerRel.invalidate(metadataId);
+ }
+
@Override
public void close() throws IOException {
if (executor != null) {
@@ -408,9 +431,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
AuthorizationRequestContext requestContext) {
Long metadataId;
Long userId;
- if (AuthConstants.OWNER.equals(privilege)) {
- return checkOwner(metalake, metadataObject, username);
- }
try {
UserEntity userEntity = getUserEntity(username, metalake);
userId = userEntity.id();
@@ -419,13 +439,16 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
LOG.debug("Can not get entity id", e);
return false;
}
-
loadRolePrivilege(metalake, username, userId, requestContext);
return authorizeByJcasbin(userId, metadataObject, metadataId, privilege);
}
private boolean authorizeByJcasbin(
Long userId, MetadataObject metadataObject, Long metadataId, String
privilege) {
+ if (AuthConstants.OWNER.equals(privilege)) {
+ Optional<Long> owner = ownerRel.getIfPresent(metadataId);
+ return Objects.equals(Optional.of(userId), owner);
+ }
return enforcer.enforce(
String.valueOf(userId),
String.valueOf(metadataObject.type()),
@@ -495,7 +518,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
});
}
- private boolean checkOwner(String metalake, MetadataObject metadataObject,
String userName) {
+ private void loadOwnerPolicy(String metalake, MetadataObject metadataObject,
Long metadataId) {
+ if (ownerRel.getIfPresent(metadataId) != null) {
+ LOG.debug("Metadata {} OWNER has been loaded.", metadataId);
+ return;
+ }
try {
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
@@ -506,19 +533,19 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
SupportsRelationOperations.Type.OWNER_REL,
entityIdent,
Entity.EntityType.valueOf(metadataObject.type().name()));
-
- for (Entity ownerEntity : owners) {
- if (ownerEntity instanceof UserEntity) {
- UserEntity user = (UserEntity) ownerEntity;
- if (user.name().equals(userName)) {
- return true;
+ if (owners.isEmpty()) {
+ ownerRel.put(metadataId, Optional.empty());
+ } else {
+ for (Entity ownerEntity : owners) {
+ if (ownerEntity instanceof UserEntity) {
+ UserEntity user = (UserEntity) ownerEntity;
+ ownerRel.put(metadataId, Optional.of(user.id()));
}
}
}
} catch (IOException e) {
LOG.warn("Can not load metadata owner", e);
}
- return false;
}
private void loadPolicyByRoleEntity(RoleEntity roleEntity) {
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java
index acb388c689..f9ef64bab0 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java
@@ -112,6 +112,10 @@ public class MockGravitinoAuthorizer implements
GravitinoAuthorizer {
@Override
public void handleRolePrivilegeChange(Long roleId) {}
+ @Override
+ public void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type) {}
+
@Override
public void close() {}
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
index 41088df93c..4388ef0952 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
@@ -36,7 +36,9 @@ import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.lang.reflect.Field;
import java.security.Principal;
+import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
import java.util.concurrent.Executor;
import java.util.stream.Collectors;
import org.apache.commons.lang3.reflect.FieldUtils;
@@ -243,7 +245,17 @@ public class TestJcasbinAuthorizer {
eq(SupportsRelationOperations.Type.OWNER_REL),
eq(catalogIdent),
eq(Entity.EntityType.CATALOG));
+ getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
assertTrue(doAuthorizeOwner(currentPrincipal));
+ doReturn(new ArrayList<>())
+ .when(supportsRelationOperations)
+ .listEntitiesByRelation(
+ eq(SupportsRelationOperations.Type.OWNER_REL),
+ eq(catalogIdent),
+ eq(Entity.EntityType.CATALOG));
+ jcasbinAuthorizer.handleMetadataOwnerChange(
+ METALAKE, USER_ID, catalogIdent, Entity.EntityType.CATALOG);
+ assertFalse(doAuthorizeOwner(currentPrincipal));
}
private Boolean doAuthorize(Principal currentPrincipal) {
@@ -367,6 +379,28 @@ public class TestJcasbinAuthorizer {
assertNull(loadedRoles.getIfPresent(testRoleId));
}
+ @Test
+ public void testOwnerCacheInvalidation() throws Exception {
+ // Get the ownerRel cache via reflection
+ Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
+
+ // Manually add an owner relation to the cache
+ ownerRel.put(CATALOG_ID, Optional.of(USER_ID));
+
+ // Verify it's in the cache
+ assertNotNull(ownerRel.getIfPresent(CATALOG_ID));
+
+ // Create a mock NameIdentifier for the metadata object
+ NameIdentifier catalogIdent = NameIdentifierUtil.ofCatalog(METALAKE,
"testCatalog");
+
+ // Call handleMetadataOwnerChange which should invalidate the cache entry
+ jcasbinAuthorizer.handleMetadataOwnerChange(
+ METALAKE, USER_ID, catalogIdent, Entity.EntityType.CATALOG);
+
+ // Verify it's removed from the cache
+ assertNull(ownerRel.getIfPresent(CATALOG_ID));
+ }
+
@Test
public void testRoleCacheSynchronousRemovalListenerDeletesPolicy() throws
Exception {
makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
@@ -406,8 +440,10 @@ public class TestJcasbinAuthorizer {
public void testCacheInitialization() throws Exception {
// Verify that caches are initialized
Cache<Long, Boolean> loadedRoles = getLoadedRolesCache(jcasbinAuthorizer);
+ Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer);
assertNotNull(loadedRoles, "loadedRoles cache should be initialized");
+ assertNotNull(ownerRel, "ownerRel cache should be initialized");
}
@SuppressWarnings("unchecked")
@@ -418,6 +454,14 @@ public class TestJcasbinAuthorizer {
return (Cache<Long, Boolean>) field.get(authorizer);
}
+ @SuppressWarnings("unchecked")
+ private static Cache<Long, Optional<Long>>
getOwnerRelCache(JcasbinAuthorizer authorizer)
+ throws Exception {
+ Field field = JcasbinAuthorizer.class.getDeclaredField("ownerRel");
+ field.setAccessible(true);
+ return (Cache<Long, Optional<Long>>) field.get(authorizer);
+ }
+
private static Enforcer getAllowEnforcer(JcasbinAuthorizer authorizer)
throws Exception {
Field field = JcasbinAuthorizer.class.getDeclaredField("allowEnforcer");
field.setAccessible(true);
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
index 7a7cbad24a..a96160da84 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
@@ -337,6 +337,10 @@ public class TestGravitinoInterceptionService {
@Override
public void handleRolePrivilegeChange(Long roleId) {}
+ @Override
+ public void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type) {}
+
@Override
public void close() throws IOException {}
}
