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 7f94ffdb51 [#10117] improvement(authz): Remove ownership relation
cache (#10119)
7f94ffdb51 is described below
commit 7f94ffdb51117346b092716eb00a9c318fc76705
Author: roryqi <[email protected]>
AuthorDate: Fri Mar 6 11:19:55 2026 +0800
[#10117] improvement(authz): Remove ownership relation cache (#10119)
### What changes were proposed in this pull request?
Remove ownership relation cache
### Why are the changes needed?
Fix: #10117
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing test cases.
---
.../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, 16 insertions(+), 153 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/Configs.java
b/core/src/main/java/org/apache/gravitino/Configs.java
index 3cc9d703e6..a26f3c36a5 100644
--- a/core/src/main/java/org/apache/gravitino/Configs.java
+++ b/core/src/main/java/org/apache/gravitino/Configs.java
@@ -320,15 +320,6 @@ 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 7965f173ad..3a4fbfc0a9 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java
@@ -152,16 +152,4 @@ 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 5b7ce28c98..13fa04a80d 100644
--- a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
+++ b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
@@ -25,7 +25,6 @@ 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;
@@ -37,7 +36,6 @@ 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;
@@ -120,7 +118,6 @@ 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);
@@ -136,33 +133,6 @@ 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 1724cdde07..bb8e85aee1 100644
--- a/docs/security/access-control.md
+++ b/docs/security/access-control.md
@@ -446,20 +446,17 @@ 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 and owner information. The cache configuration options allow you to tune
the cache behavior:
+Gravitino uses Caffeine caches to improve authorization performance by caching
role 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 or ownership are changed through the Gravitino API, the
corresponding cache entries are automatically invalidated to ensure
authorization decisions reflect the latest state.
+When role privileges 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 06e20b8ada..82a5d3a82a 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,10 +109,6 @@ 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 cea47e353b..524c90f032 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,7 +29,6 @@ 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;
@@ -87,8 +86,6 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer
{
*/
private Cache<Long, Boolean> loadedRoles;
- private Cache<Long, Optional<Long>> ownerRel;
-
private Executor executor = null;
@Override
@@ -99,8 +96,6 @@ 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());
@@ -121,11 +116,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
})
.build();
- ownerRel =
- Caffeine.newBuilder()
- .expireAfterAccess(cacheExpirationSecs, TimeUnit.SECONDS)
- .maximumSize(ownerCacheSize)
- .build();
+
executor =
Executors.newFixedThreadPool(
GravitinoEnv.getInstance()
@@ -218,15 +209,9 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
String metalake,
MetadataObject metadataObject,
AuthorizationRequestContext requestContext) {
- Long userId;
boolean result;
try {
- 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));
+ result = checkOwner(metalake, metadataObject, principal.getName());
} catch (Exception e) {
LOG.debug("Can not get entity id", e);
result = false;
@@ -387,14 +372,6 @@ 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) {
@@ -431,6 +408,9 @@ 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();
@@ -439,16 +419,13 @@ 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()),
@@ -518,11 +495,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
});
}
- private void loadOwnerPolicy(String metalake, MetadataObject metadataObject,
Long metadataId) {
- if (ownerRel.getIfPresent(metadataId) != null) {
- LOG.debug("Metadata {} OWNER has been loaded.", metadataId);
- return;
- }
+ private boolean checkOwner(String metalake, MetadataObject metadataObject,
String userName) {
try {
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
@@ -533,19 +506,19 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
SupportsRelationOperations.Type.OWNER_REL,
entityIdent,
Entity.EntityType.valueOf(metadataObject.type().name()));
- 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()));
+
+ for (Entity ownerEntity : owners) {
+ if (ownerEntity instanceof UserEntity) {
+ UserEntity user = (UserEntity) ownerEntity;
+ if (user.name().equals(userName)) {
+ return true;
}
}
}
} 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 f9ef64bab0..acb388c689 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,10 +112,6 @@ 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 4388ef0952..41088df93c 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,9 +36,7 @@ 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;
@@ -245,17 +243,7 @@ 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) {
@@ -379,28 +367,6 @@ 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);
@@ -440,10 +406,8 @@ 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")
@@ -454,14 +418,6 @@ 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 a96160da84..7a7cbad24a 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,10 +337,6 @@ 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 {}
}
