dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1866461610
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisResolvedPathWrapper.java:
##########
@@ -19,62 +19,41 @@
package org.apache.polaris.core.persistence;
import java.util.List;
-import java.util.stream.Collectors;
import org.apache.polaris.core.entity.PolarisEntity;
/**
* Holds fully-resolved path of PolarisEntities representing the targetEntity
with all its grants
* and grant records.
*/
public class PolarisResolvedPathWrapper {
- private final List<ResolvedPolarisEntity> resolvedPath;
+ private final List<PolarisEntity> resolvedPath;
Review Comment:
Using plain `PolarisEntity` inside a "Resolved" entity path _wrapper_ does
not sound good to me purely from a code readability point of view.
I'm fine using `PolarisEntity` in Authrorizer inputs, but perhaps we should
rename the wrapper class and change its javadoc? What does "resolved" mean in
this context?
If we merely want a dedicate class to hold a path of nested entities, how
about `PolarisEntityPath`?
##########
polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java:
##########
@@ -226,7 +230,17 @@ public void run(PolarisApplicationConfig configuration,
Environment environment)
new PolarisCallContextCatalogFactory(
entityManagerFactory, metaStoreManagerFactory, taskExecutor,
fileIOFactory);
- PolarisAuthorizer authorizer = new
PolarisAuthorizerImpl(configurationStore);
+ ConcurrentHashMap<RealmContext, EntityCache> realmEntityCache = new
ConcurrentHashMap<>();
+ PolarisGrantManager.Factory factory =
+ new EntityCacheGrantManager.EntityCacheGrantManagerFactory(
Review Comment:
It looks like this code got refactored since my first comment (commit
b9ded774360a785a2543dce26505e9039e0b2e42).
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisResolvedPathWrapper.java:
##########
@@ -19,62 +19,41 @@
package org.apache.polaris.core.persistence;
import java.util.List;
-import java.util.stream.Collectors;
import org.apache.polaris.core.entity.PolarisEntity;
/**
* Holds fully-resolved path of PolarisEntities representing the targetEntity
with all its grants
* and grant records.
*/
public class PolarisResolvedPathWrapper {
- private final List<ResolvedPolarisEntity> resolvedPath;
+ private final List<PolarisEntity> resolvedPath;
// TODO: Distinguish between whether parentPath had a null in the chain or
whether only
// the leaf element was null.
- public PolarisResolvedPathWrapper(List<ResolvedPolarisEntity> resolvedPath) {
+ public PolarisResolvedPathWrapper(List<PolarisEntity> resolvedPath) {
this.resolvedPath = resolvedPath;
}
- public ResolvedPolarisEntity getResolvedLeafEntity() {
+ public PolarisEntity getResolvedLeafEntity() {
if (resolvedPath == null || resolvedPath.isEmpty()) {
return null;
}
return resolvedPath.get(resolvedPath.size() - 1);
}
public PolarisEntity getRawLeafEntity() {
- ResolvedPolarisEntity resolvedEntity = getResolvedLeafEntity();
- if (resolvedEntity != null) {
- return resolvedEntity.getEntity();
- }
- return null;
- }
-
- public List<ResolvedPolarisEntity> getResolvedFullPath() {
- return resolvedPath;
+ return getResolvedLeafEntity();
}
public List<PolarisEntity> getRawFullPath() {
- if (resolvedPath == null) {
- return null;
- }
- return
resolvedPath.stream().map(ResolvedPolarisEntity::getEntity).collect(Collectors.toList());
- }
-
- public List<ResolvedPolarisEntity> getResolvedParentPath() {
- if (resolvedPath == null) {
- return null;
- }
- return resolvedPath.subList(0, resolvedPath.size() - 1);
+ return resolvedPath;
Review Comment:
Why do we return the "resolved" path from `getRawFullPath()` but remove the
`getResolvedFullPath()` method?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.cache;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.auth.PolarisGrantManager;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.entity.PolarisGrantRecord;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+import org.apache.polaris.core.persistence.BaseResult;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * PolarisGrantManger implementation that uses an EntityCache to retrieve
entities and is backed by
+ * a delegate grant manager for persisting grant changes. This allows
consumers to reuse cache
+ * entities without necessarily being aware of the {@link EntityCache} or the
{@link
+ * EntityCacheEntry} specifics. Typically, the {@link
+ * org.apache.polaris.core.persistence.resolver.Resolver} is responsible for
validating the cache
+ * state. This class does no validation of the entity version or the grant
version of any cache
+ * record.
+ */
+public class EntityCacheGrantManager implements PolarisGrantManager {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(EntityCacheGrantManager.class);
+ private final PolarisGrantManager delegateGrantManager;
+ private final EntityCache entityCache;
+ private PolarisGrantRecord serviceAdminRootContainerGrant;
+ private PolarisBaseEntity serviceAdminEntity;
+
+ public static final class EntityCacheGrantManagerFactory implements
PolarisGrantManager.Factory {
+ private final PolarisGrantManager.Factory delegateGrantManagerFactory;
+ private final RealmEntityCacheFactory realmEntityCacheFactory;
+
+ public EntityCacheGrantManagerFactory(
+ Factory delegateGrantManagerFactory, RealmEntityCacheFactory
realmEntityCacheFactory) {
+ this.delegateGrantManagerFactory = delegateGrantManagerFactory;
+ this.realmEntityCacheFactory = realmEntityCacheFactory;
+ }
+
+ @Override
+ public PolarisGrantManager getGrantManagerForRealm(RealmContext realm) {
+ return new EntityCacheGrantManager(
+ delegateGrantManagerFactory.getGrantManagerForRealm(realm),
+ realmEntityCacheFactory.getOrCreateEntityCache(realm));
+ }
+ }
+
+ public EntityCacheGrantManager(
+ PolarisGrantManager delegateGrantManager, EntityCache entityCache) {
+ this.delegateGrantManager = delegateGrantManager;
+ this.entityCache = entityCache;
+ }
+
+ @Override
+ public @NotNull PrivilegeResult grantUsageOnRoleToGrantee(
+ @NotNull PolarisCallContext callCtx,
+ @Nullable PolarisEntityCore catalog,
+ @NotNull PolarisEntityCore role,
+ @NotNull PolarisEntityCore grantee) {
+ try {
+ return delegateGrantManager.grantUsageOnRoleToGrantee(callCtx, catalog,
role, grantee);
+ } finally {
+ LOGGER.debug("Invalidating cache for role {} and grantee {}", role,
grantee);
+ entityCache.removeCacheEntry(role);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult revokeUsageOnRoleFromGrantee(
+ @NotNull PolarisCallContext callCtx,
+ @Nullable PolarisEntityCore catalog,
+ @NotNull PolarisEntityCore role,
+ @NotNull PolarisEntityCore grantee) {
+ try {
+ return delegateGrantManager.revokeUsageOnRoleFromGrantee(callCtx,
catalog, role, grantee);
+ } finally {
+ LOGGER.debug("Invalidating cache for role {} and grantee {}", role,
grantee);
+ entityCache.removeCacheEntry(role);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult grantPrivilegeOnSecurableToRole(
+ @NotNull PolarisCallContext callCtx,
+ @NotNull PolarisEntityCore grantee,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @NotNull PolarisEntityCore securable,
+ @NotNull PolarisPrivilege privilege) {
+ try {
+ return delegateGrantManager.grantPrivilegeOnSecurableToRole(
+ callCtx, grantee, catalogPath, securable, privilege);
+ } finally {
+ LOGGER.debug("Invalidating cache for securable {} and grantee {}",
securable, grantee);
+ entityCache.removeCacheEntry(securable);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult revokePrivilegeOnSecurableFromRole(
+ @NotNull PolarisCallContext callCtx,
+ @NotNull PolarisEntityCore grantee,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @NotNull PolarisEntityCore securable,
+ @NotNull PolarisPrivilege privilege) {
+ try {
+ return delegateGrantManager.revokePrivilegeOnSecurableFromRole(
+ callCtx, grantee, catalogPath, securable, privilege);
+ } finally {
+ LOGGER.debug("Invalidating cache for securable {} and grantee {}",
securable, grantee);
+ entityCache.removeCacheEntry(securable);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull LoadGrantsResult loadGrantsOnSecurable(
+ @NotNull PolarisCallContext callCtx, long securableCatalogId, long
securableId) {
+ EntityCacheLookupResult lookupResult =
+ entityCache.getOrLoadEntityById(callCtx, securableCatalogId,
securableId);
+ if (lookupResult == null || lookupResult.getCacheEntry() == null) {
Review Comment:
I think this causes an "API skew". We never call `loadGrantsOnSecurable` on
`delegateGrantManager`, therefore we assume that the `entityCache` is somehow
connected to the same source of data as the `delegateGrantManager` but their
APIs are totally different.
Essentially this means that
`PolarisMetaStoreManagerImpl.loadCachedEntryById` and
`PolarisMetaStoreManagerImpl.loadGrantsOnSecurable()` must produce coherent
results.
It does not look like we achieve any clean API separation, which is kind of
implies by having the multitude of interfaces that `PolarisMetaStoreManager`
extends.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -602,15 +606,25 @@ public boolean hasTransitivePrivilege(
Set<Long> activatedGranteeIds,
PolarisPrivilege desiredPrivilege,
PolarisResolvedPathWrapper resolvedPath) {
+ PolarisGrantManager grantManager =
+ grantManagerFactory.getGrantManagerForRealm(
+ CallContext.getCurrentContext().getRealmContext());
+ PolarisCallContext callContext =
CallContext.getCurrentContext().getPolarisCallContext();
// Iterate starting at the parent, since the most common case should be to
manage grants as
// high up in the resource hierarchy as possible, so we expect earlier
termination.
- for (ResolvedPolarisEntity resolvedSecurableEntity :
resolvedPath.getResolvedFullPath()) {
- Preconditions.checkState(
- resolvedSecurableEntity.getGrantRecordsAsSecurable() != null,
- "Got null grantRecordsAsSecurable for resolvedSecurableEntity %s",
- resolvedSecurableEntity);
- for (PolarisGrantRecord grantRecord :
resolvedSecurableEntity.getGrantRecordsAsSecurable()) {
+ for (PolarisEntity resolvedSecurableEntity :
resolvedPath.getRawFullPath()) {
+ PolarisGrantManager.LoadGrantsResult grantsResult =
+ grantManager.loadGrantsOnSecurable(
Review Comment:
I'm generally fine with this change, but I'd like to clarify one point. My
impression from current code on `main` is that we want to run authZ check on
the exact same state of Polaris data as what the API services use for
generating API responses.
Is that a correct assumption?
If I am mistaken, that is fine and this change is fine too. However, if my
assumption is valid, how do we make sure the loaded grant records match the
other data, which is loaded independently by API services?
--
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]
