You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "collado-mike (via GitHub)" <gi...@apache.org> on 2024/11/22 00:10:01 UTC
[PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
collado-mike opened a new pull request, #465:
URL: https://github.com/apache/polaris/pull/465
# Description
Currently, the `PolarisAuthorizer` implementation relies on the `ResolvedPolarisEntity` to return a list of grants for each entity in order to determine if a Principal has authorization to perform an operation on a given target. The grant records are populated by the `Resolver`, utilizing the `EntityCache` for a given realm to avoid unnecessary roundtrips to the persistence store for, e.g., the grants held by a given `PrincipalRole` or `CatalogRole`.
Because the `PolarisAuthorizer` relies on the grant records in the `ResolvedPolarisEntity`, there's no opportunity to utilize custom grant logic (for example, to declare a target to be globally viewable or attach PrincipalRoles dynamically) without either updating the Resolver or writing custom grant records prior to the Resolver execution.
This change decouples the `PolarisAuthorizer` from the `ResolvedPolarisEntity` so that it depends entirely on the `PolarisGrantManager` to declare which grants exist on a securable. It implements a `EntityCacheGrantManager` that, under the hood, still works with the `EntityCache` so that the lookups from the `Resolver` populate the cache with both entities and grants. However, the `PolarisAuthorizer` doesn't need to know anything about the `EntityCache` or the `Resolver` types. For convenience, it still uses the `PolarisResolvedPathWrapper`, as it's still a useful container for passing around full paths for entities.
The logic in the `PolarisAuthorizer` is unchanged and all tests still pass. An additional test class is added to verify the `EntityCacheGrantManager` works directly with the cache and will delegate to the underlying `PolarisGrantManager` when necessary.
Note that the logic that implicitly grants the `service_admin` role `SERVICE_MANAGE_ACCESS` privileges on the implicit root container has been moved to the `EntityCacheGrantManager`. I ran the tests without this move and they all still passed, so it seems that, at some point, the root container is being persisted and the grants are recorded. However, to ensure backward compatibility, I ported the logic. We may want to get rid of that at some point.
Fixes # (issue)
## Type of change
Please delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation update
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
# How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [X] EntityCacheGrantManagerTest
**Test Configuration**:
* Hardware:
* Toolchain:
* SDK:
# Checklist:
Please delete options that are not relevant.
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1890997366
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -460,10 +462,13 @@ public class PolarisAuthorizerImpl implements PolarisAuthorizer {
}
private final PolarisConfigurationStore featureConfig;
+ private final Provider<PolarisGrantManager> grantManagerFactory;
@Inject
- public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
+ public PolarisAuthorizerImpl(
+ PolarisConfigurationStore featureConfig, Provider<PolarisGrantManager> grantManagerFactory) {
Review Comment:
`PolarisAuthorizerImpl` is a light-weight object. Does it hurt to make it request-scoped? Cross-scope references resolved at call time are indeed to be avoided, but, IMHO, the `Provider` falls into the same category.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854798722
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
+ entityCache.removeCacheEntry(roleEntity);
+ }
+ }
+
+ @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);
+ invalidate(role);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList =
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ // make it a mutable list
+ .collect(
Review Comment:
yep
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854869550
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
+ entityCache.removeCacheEntry(roleEntity);
+ }
+ }
+
+ @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);
+ invalidate(role);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList =
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ // make it a mutable list
+ .collect(
+ Collector.of(
+ ArrayList::new,
+ List::add,
+ (l, r) -> {
+ l.addAll(r);
+ return l;
+ },
+ Collector.Characteristics.IDENTITY_FINISH));
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
+ if (serviceAdminEntity == null || serviceAdminRootContainerGrant == null) {
+ EntityCacheLookupResult serviceAdminRole =
+ entityCache.getOrLoadEntityByName(
+ callCtx,
+ new EntityCacheByNameKey(
+ 0L,
+ 0L,
+ PolarisEntityType.PRINCIPAL_ROLE,
+ PolarisEntityConstants.getNameOfPrincipalServiceAdminRole()));
+ if (serviceAdminRole == null || serviceAdminRole.getCacheEntry() == null) {
+ LOGGER.error("Failed to resolve service_admin PrincipalRole");
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ serviceAdminRootContainerGrant =
+ new PolarisGrantRecord(
+ 0L,
+ 0L,
+ serviceAdminRole.getCacheEntry().getEntity().getCatalogId(),
+ serviceAdminRole.getCacheEntry().getEntity().getId(),
+ PolarisPrivilege.SERVICE_MANAGE_ACCESS.getCode());
+ serviceAdminEntity = serviceAdminRole.getCacheEntry().getEntity();
+ }
+ grantRecords.add(serviceAdminRootContainerGrant);
+ granteeList.add(serviceAdminEntity);
+ }
+ return new LoadGrantsResult(
+ lookupResult.getCacheEntry().getEntity().getGrantRecordsVersion(),
+ grantRecords,
+ granteeList);
+ }
+
+ @Override
+ public @NotNull LoadGrantsResult loadGrantsToGrantee(
+ PolarisCallContext callCtx, long granteeCatalogId, long granteeId) {
+ EntityCacheLookupResult lookupResult =
+ entityCache.getOrLoadEntityById(callCtx, granteeCatalogId, granteeId);
+ if (lookupResult == null || lookupResult.getCacheEntry() == null) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisBaseEntity> granteeList =
+ lookupResult.getCacheEntry().getGrantRecordsAsGrantee().stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getSecurableCatalogId(), gr.getSecurableId()))
+ .takeWhile(
+ lr -> {
+ if (lr == null || lr.getCacheEntry() == null) {
+ throw new IllegalStateException("Grantee not found");
+ }
+ return true;
+ })
Review Comment:
Converted to a filter and changed to throw exception if the size doesn't match the expected
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "adutra (via GitHub) via issues" <is...@polaris.apache.org>.
adutra commented on PR #465:
URL: https://github.com/apache/polaris/pull/465#issuecomment-6660763744
This PR is almost 2 years old, and I don't think it's relevant anymore. Can we close it? @collado-mike @flyrain
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854231909
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -50,4 +51,17 @@ public interface MetaStoreManagerFactory extends Discoverable {
/** Purge all metadata for the realms provided */
void purgeRealms(List<String> realms);
+
+ /**
+ * Default {@link PolarisGrantManager} factory that returns the {@link PolarisMetaStoreManager} as
+ * the grant manager.
+ *
+ * @param realm the current realm
+ * @return the {@link PolarisMetaStoreManager} returned by {@link
+ * #getOrCreateMetaStoreManager(RealmContext)}
+ */
+ @Override
+ default PolarisGrantManager getGrantManagerForRealm(RealmContext realm) {
Review Comment:
It feel like we have a clash between Encapsulation / Decoupling and Dependency Injection concerns here. My impression from #417 was that we wanted to separate different "managers" (which, I agree, is a good idea). However, it looks like call paths to those managers still lead to one `MetaStoreManagerFactory`. From my POV I do not see any other reason for this but dependency injection. `MetaStoreManagerFactory` is provided the the application on startup and all smaller "managers" stem from it.
I just had an idea that perhaps we could make a java services-based mechanism for discovering specific metastore managers and have them isolated from each other at the interface level. Would you be interested in a POC PR for that (I can give it a try)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #465:
URL: https://github.com/apache/polaris/pull/465#issuecomment-2512389207
@dennishuo
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1890920052
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -460,10 +462,13 @@ public class PolarisAuthorizerImpl implements PolarisAuthorizer {
}
private final PolarisConfigurationStore featureConfig;
+ private final Provider<PolarisGrantManager> grantManagerFactory;
@Inject
- public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
+ public PolarisAuthorizerImpl(
+ PolarisConfigurationStore featureConfig, Provider<PolarisGrantManager> grantManagerFactory) {
Review Comment:
The `PolarisAuthorizer` is in a different scope from the `PolarisGrantManager`. Since the authorizer isn't realm or request-scoped, I prefer fetching the realm-scoped bean when it's needed. I know I can rely on runtime proxies, but... I dislike runtime proxies when they're not needed :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1881527419
##########
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:
This code, like the existing code, assumes that all of the entities are in the cache once the Resolver runs. That process puts all of the entities and their grants into the cache so that by the time the Authorizer retrieves the grants, it fetches them from the cache. This approach is basically hiding that fact by wrapping the cache in a `PolarisGrantManager`. The impact on cache hits is the same, but we can make other changes to the persistence interface and the principal role resolution without affecting the Authorizer.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1884675150
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.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 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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList = new ArrayList<>();
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ .forEach(granteeList::add);
+ if (granteeList.size() != lookupResult.getCacheEntry().getGrantRecordsAsSecurable().size()) {
+ LOGGER.error(
+ "Failed to resolve all grantees for securable {}",
+ lookupResult.getCacheEntry().getEntity());
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
Review Comment:
Special cases by entity name looks risky to me. The entity here is not restricted, right? So, it can be a namespace, I suppose, and might have the name of `root_container`, right?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1866415904
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -34,7 +35,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
+public interface MetaStoreManagerFactory extends Discoverable, PolarisGrantManager.Factory {
Review Comment:
I believe this change conflicts with #470, but for some reason GH does not flag it as a conflict.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867106093
##########
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:
We're eliminating the `ResolvedPolarisEntity` type and just using `PolarisEntity` types everywhere.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.
dimas-b closed pull request #465: Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl
URL: https://github.com/apache/polaris/pull/465
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1881528089
##########
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:
Originally, it referred to the types generated by the Resolver. Now, it really means nothing. I'm happy to change it to get rid of the old reference
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #465:
URL: https://github.com/apache/polaris/pull/465#issuecomment-2538026366
@dimas-b @adutra now that the CDI stuff is in, can we move forward with this PR? This is part of the overall move to help support the federated identities (removing the dependency on the resolver to obtain principal role grants). There are a lot of refactoring PRs in flight and I'm worried that there are too many conflicting approaches to the overall problem.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1884675150
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.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 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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList = new ArrayList<>();
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ .forEach(granteeList::add);
+ if (granteeList.size() != lookupResult.getCacheEntry().getGrantRecordsAsSecurable().size()) {
+ LOGGER.error(
+ "Failed to resolve all grantees for securable {}",
+ lookupResult.getCacheEntry().getEntity());
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
Review Comment:
Special cases by entity name look risky to me. The entity here is not restricted, right? So, it can be a namespace, I suppose, and might have the name of `root_container`, right?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1904537673
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -460,10 +462,13 @@ public class PolarisAuthorizerImpl implements PolarisAuthorizer {
}
private final PolarisConfigurationStore featureConfig;
+ private final Provider<PolarisGrantManager> grantManagerFactory;
@Inject
- public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
+ public PolarisAuthorizerImpl(
+ PolarisConfigurationStore featureConfig, Provider<PolarisGrantManager> grantManagerFactory) {
Review Comment:
@collado-mike : from my POV this is the last remaining concern on this PR :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854824624
##########
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:
I mean this factory (line 235) provides the `getGrantManagerForRealm()` method. This factory implementation is not the same as `MetaStoreManagerFactory.getGrantManagerForRealm()`. Why does `MetaStoreManagerFactory` extend `PolarisGrantManager.Factory`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854800984
##########
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:
The `metastoreManagerFactory` passed to the constructor on the next line implements the `PolarisGrantManager.Factory`. The factory method is invoked by the `EntityCacheGrantManagerFactory` constructor here: https://github.com/apache/polaris/pull/465/files/3b10cb2021cecfc85dc93b2ed51ea3a4489439e4#diff-dec23d1ca2299b42bd2926a823cad0da6f0a8334ae5093346467f410280e77a2R69
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1881536275
##########
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:
Wait? Are you saying that cache eviction breaks requests?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867791881
##########
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:
This thread is about the overall assumptions on the consistency of data in storage in the presence of concurrent changes.
With this code, grants for different objects in the same logical API-level operation may be loaded from different states of the data (e.g. results of two of more RDBMS transaction). So my question is whether this is a concern. I'm not proposing any particular approach, just wondering for my own understanding.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.
dimas-b commented on PR #465:
URL: https://github.com/apache/polaris/pull/465#issuecomment-6672006494
Closing as stale. @collado-mike : Please feel free to re-open.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867137713
##########
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:
If this is a reference to the same `PolarisMetaStoreManagerImpl.loadCachedEntryById` and `PolarisMetaStoreManagerImpl.loadGrantsOnSecurable` API difference you call out below, I do think there's more work to line the cache up with the metastore API calls. Ultimately, I think the `PolarisServerImpl` should be fetching the same cached grant records that are being returned to the Authorizer so that the return values and the authz assertions are the same.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854268946
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
+ entityCache.removeCacheEntry(roleEntity);
+ }
+ }
+
+ @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);
+ invalidate(role);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList =
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ // make it a mutable list
+ .collect(
Review Comment:
isn't `forEach(granteeList::add)` easier?
##########
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:
`PolarisGrantManager` objects returned from this factory are very different from what would be returned from `MetaStoreManagerFactory.getGrantManagerForRealm()`... What's the reason for having `getGrantManagerForRealm` in `MetaStoreManagerFactory`? I do not really see any call paths leading to the latter factory method.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
+ entityCache.removeCacheEntry(roleEntity);
+ }
+ }
+
+ @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);
+ invalidate(role);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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);
+ invalidate(securable);
+ invalidate(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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList =
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ // make it a mutable list
+ .collect(
+ Collector.of(
+ ArrayList::new,
+ List::add,
+ (l, r) -> {
+ l.addAll(r);
+ return l;
+ },
+ Collector.Characteristics.IDENTITY_FINISH));
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
+ if (serviceAdminEntity == null || serviceAdminRootContainerGrant == null) {
+ EntityCacheLookupResult serviceAdminRole =
+ entityCache.getOrLoadEntityByName(
+ callCtx,
+ new EntityCacheByNameKey(
+ 0L,
+ 0L,
+ PolarisEntityType.PRINCIPAL_ROLE,
+ PolarisEntityConstants.getNameOfPrincipalServiceAdminRole()));
+ if (serviceAdminRole == null || serviceAdminRole.getCacheEntry() == null) {
+ LOGGER.error("Failed to resolve service_admin PrincipalRole");
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ serviceAdminRootContainerGrant =
+ new PolarisGrantRecord(
+ 0L,
+ 0L,
+ serviceAdminRole.getCacheEntry().getEntity().getCatalogId(),
+ serviceAdminRole.getCacheEntry().getEntity().getId(),
+ PolarisPrivilege.SERVICE_MANAGE_ACCESS.getCode());
+ serviceAdminEntity = serviceAdminRole.getCacheEntry().getEntity();
+ }
+ grantRecords.add(serviceAdminRootContainerGrant);
+ granteeList.add(serviceAdminEntity);
+ }
+ return new LoadGrantsResult(
+ lookupResult.getCacheEntry().getEntity().getGrantRecordsVersion(),
+ grantRecords,
+ granteeList);
+ }
+
+ @Override
+ public @NotNull LoadGrantsResult loadGrantsToGrantee(
+ PolarisCallContext callCtx, long granteeCatalogId, long granteeId) {
+ EntityCacheLookupResult lookupResult =
+ entityCache.getOrLoadEntityById(callCtx, granteeCatalogId, granteeId);
+ if (lookupResult == null || lookupResult.getCacheEntry() == null) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisBaseEntity> granteeList =
+ lookupResult.getCacheEntry().getGrantRecordsAsGrantee().stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getSecurableCatalogId(), gr.getSecurableId()))
+ .takeWhile(
+ lr -> {
+ if (lr == null || lr.getCacheEntry() == null) {
+ throw new IllegalStateException("Grantee not found");
+ }
+ return true;
+ })
Review Comment:
we're not actually creating a logical "sub-stream" here, but fail on a certain condition... IMHO `.filter()` would be more intuitive.
##########
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<>();
Review Comment:
Realms are never removed from this map... is that a concern (for large deployments)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854949369
##########
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:
The `getGrantManagerForRealm` method implemented by this factory calls `delegateGrantManagerFactory.getGrantManagerForRealm(realm)`, which is invoking the `getGrantManagerForRealm` method on the `MetaStoreManagerFactory`. The `EntityCacheGrantManagerFactory` constructor takes in a `PolarisGrantManager.Factory` argument - here, we're passing in the `MetaStoreManagerFactory` as the delegate implementation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854799666
##########
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<>();
Review Comment:
The `RealmEntityManagerFactory` already stores the cache in a map that's never cleaned up. I'm hoping to address this with the `@RealmScoped` annotation in the HK2 branch - we could consolidate all the realm-specific caches in a single location with its own read/write-based TTL
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867106780
##########
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:
This sounds fine to me - the main thing for me is to remove the different entity types so that each interface is dealing directly with the entity types (albeit, with a convenience wrapper in this case).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1889038734
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.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 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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList = new ArrayList<>();
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ .forEach(granteeList::add);
+ if (granteeList.size() != lookupResult.getCacheEntry().getGrantRecordsAsSecurable().size()) {
+ LOGGER.error(
+ "Failed to resolve all grantees for securable {}",
+ lookupResult.getCacheEntry().getEntity());
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
Review Comment:
That's a good callout. This is intended to replicate the "simulated" root container entity logic that was in the `PolarisEntityManager`, but I'll add logic and a test to ensure we aren't adding privileges to random other entity types.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854831138
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -50,4 +51,17 @@ public interface MetaStoreManagerFactory extends Discoverable {
/** Purge all metadata for the realms provided */
void purgeRealms(List<String> realms);
+
+ /**
+ * Default {@link PolarisGrantManager} factory that returns the {@link PolarisMetaStoreManager} as
+ * the grant manager.
+ *
+ * @param realm the current realm
+ * @return the {@link PolarisMetaStoreManager} returned by {@link
+ * #getOrCreateMetaStoreManager(RealmContext)}
+ */
+ @Override
+ default PolarisGrantManager getGrantManagerForRealm(RealmContext realm) {
Review Comment:
Sure. Let's continue the DI discussion on the mailing list.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1866415904
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -34,7 +35,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
-public interface MetaStoreManagerFactory extends Discoverable {
+public interface MetaStoreManagerFactory extends Discoverable, PolarisGrantManager.Factory {
Review Comment:
I believe this change conflicts with #470
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1866531771
##########
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 implied by having the multitude of interfaces that `PolarisMetaStoreManager` extends.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867791881
##########
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:
This thread is about the overall assumptions on the consistency of data in storage in the presence of concurrent changes.
With this code, grants for different objects in the same logical API-level operation may be loaded from different states of the data (e.g. results of two of more RDBMS transactions). So my question is whether this is a concern. I'm not proposing any particular approach, just wondering for my own understanding.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854240233
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
Review Comment:
Why do we need a get before remove (next line)? Why not add remove-by-id method?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854231909
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -50,4 +51,17 @@ public interface MetaStoreManagerFactory extends Discoverable {
/** Purge all metadata for the realms provided */
void purgeRealms(List<String> realms);
+
+ /**
+ * Default {@link PolarisGrantManager} factory that returns the {@link PolarisMetaStoreManager} as
+ * the grant manager.
+ *
+ * @param realm the current realm
+ * @return the {@link PolarisMetaStoreManager} returned by {@link
+ * #getOrCreateMetaStoreManager(RealmContext)}
+ */
+ @Override
+ default PolarisGrantManager getGrantManagerForRealm(RealmContext realm) {
Review Comment:
It feel like we have a clash between Encapsulation / Decoupling and Dependency Injection concerns here. My impression from $417 was that we wanted to separate different "managers" (which, I agree, is a good idea). However, it looks like call paths to those managers still lead to one `MetaStoreManagerFactory`. From my POV I do not see any other reason for this but dependency injection. `MetaStoreManagerFactory` is provided the the application on startup and all smaller "managers" stem from it.
I just had an idea that perhaps we could make a java services-based mechanism for discovering specific metastore managers and have them isolated from each other at the interface level. Would you be interested in a POC PR for that (I can give it a try)?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #465:
URL: https://github.com/apache/polaris/pull/465#issuecomment-4042652998
Hi @collado-mike , are we still pursuing this? If not, can we close it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1881524417
##########
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'd like to leave this concern to the proposal in https://docs.google.com/document/d/1MNCdW-uKVZaR5Ua91FDwxhreBeGjPOoV-CyF0WOWwIg/edit?tab=t.0#heading=h.e7gcs110cqli
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867782525
##########
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:
My comment is specific to the internal code "logic" in `PolarisResolvedPathWrapper`. What does "resolved" mean in the context of that class?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1867125345
##########
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:
yeah, the API separation still needs a lot of work. Originally, I wanted to break the cache methods out of the `PolarisMetaStoreManager` entirely, but without the DI support, it was too hard to cast underlying impls to the right interfaces.
But, in reality, this API skew has really always been there. It's just more clearly evident now that we're explicitly invoking `loadCachedEntryById` when `loadGrantsOnSecurable` is called. Previously, this was just hidden by the fact that the authorizer simply got passed the `ResolvedPolarisEntity` and the Resolver just loaded everything from the cache without any pretense of talking to the `PolarisMetaStoreManager`.
Eventually, I think the `EntityCache` and the `PolarisRemoteCache` types need to be addressed, but first is to encapsulate their usage into more generalized types so that they can be changed without impacting the rest of the calling code.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854866233
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
Review Comment:
So ultimately, this does a lookup for the entity in order to generate a cache key anyway. I just moved this logic into the `EntityCache`, so the caller just invokes `removeCacheEntry(PolarisEntityCore)` and it does the lookup and cache key generation internally.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854797836
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,247 @@
+/*
+ * 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.Collector;
+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);
+ invalidate(role);
+ invalidate(grantee);
+ }
+ }
+
+ private void invalidate(@NotNull PolarisEntityCore role) {
+ EntityCacheEntry roleEntity = entityCache.getEntityById(role.getId());
+ if (roleEntity != null) {
Review Comment:
yeah, that's a good idea. I was just using what was already there :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1854797397
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -50,4 +51,17 @@ public interface MetaStoreManagerFactory extends Discoverable {
/** Purge all metadata for the realms provided */
void purgeRealms(List<String> realms);
+
+ /**
+ * Default {@link PolarisGrantManager} factory that returns the {@link PolarisMetaStoreManager} as
+ * the grant manager.
+ *
+ * @param realm the current realm
+ * @return the {@link PolarisMetaStoreManager} returned by {@link
+ * #getOrCreateMetaStoreManager(RealmContext)}
+ */
+ @Override
+ default PolarisGrantManager getGrantManagerForRealm(RealmContext realm) {
Review Comment:
Part of the reasoning for this PR is to move toward dependency injection without relying on it fully implemented. #417 was, in large part, to encourage callers to declare only the most specific dependency interface they need (e.g., `PolarisGrantManager` to manage grants, `CredentialVendor` to manage credentials) so that we could use dependency injection to support different manager implementations. Today, the `PolarisMetaStoreManager` implements all of the component interfaces, so it's easy to make this interface implement the `PolarisGrantManager.Factory` interface. In the future, I imagine we might break the interface inheritance so that a `PolarisMetaStoreManager` doesn't necessarily implement, e.g., `CredentialVendor`, since the persistence implementation might not want to know anything about credential vending. But for now, this factory method seems reasonable.
But we can still take advantage of the different interfaces even if all roads eventually lead to the `PolarisMetaStoreManager`. The caching implementation of the `PolarisGrantManager` is one such example. I'd also like to rewrite the `StorageCredentialsCache` as an implementation of `CredentialVendor`, even if the `PolarisMetaStoreManager` still implements the credential vending methods.
I think we should resolve the DI discussion on the mailing list before we introduce another service-locator approach.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1884666490
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -460,10 +462,13 @@ public class PolarisAuthorizerImpl implements PolarisAuthorizer {
}
private final PolarisConfigurationStore featureConfig;
+ private final Provider<PolarisGrantManager> grantManagerFactory;
@Inject
- public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
+ public PolarisAuthorizerImpl(
+ PolarisConfigurationStore featureConfig, Provider<PolarisGrantManager> grantManagerFactory) {
Review Comment:
Why are we injecting a `Provider` instead of `PolarisGrantManager` directly?
##########
dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/PolarisApplication.java:
##########
@@ -301,7 +303,12 @@ protected void configure() {
// manager
bindFactory(PolarisMetaStoreManagerFactory.class)
.in(RealmScoped.class)
+ .named("delegateGrantManager")
Review Comment:
The name looks too generic. It does not convey the purpose of the beam. Also, the system could potentially have multiple delegates. How about `primaryGrantManager`, `persistingGrantManager`?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.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 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);
Review Comment:
I understand the internals of caches and persistence are still a mix intermixed, but overall, this pattern looks concerning to me. Having to invalidate the cache for `role` upon granting a privilege to it is non-intuitive. I know I made API skew comments elsewhere already, so please treat it as just another example... I do not think we should address this in the current PR, but I think we ought to address it at some point :)
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.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 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) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList = new ArrayList<>();
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ .forEach(granteeList::add);
+ if (granteeList.size() != lookupResult.getCacheEntry().getGrantRecordsAsSecurable().size()) {
+ LOGGER.error(
+ "Failed to resolve all grantees for securable {}",
+ lookupResult.getCacheEntry().getEntity());
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND, null);
+ }
+
+ // If the securable is the root container, then we need to add a grant record for the
+ // service_admin PrincipalRole, which is the only role that has the SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
Review Comment:
Special cases by entity name look risky to me. The entity here is not restricted, right? So, it can be a namespace, I suppose, and might have the name of `root_container`, right?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org