Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl #465

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+913 −267
Open

Update PolarisAuthorizerImpl to use PolarisGrantManager interface and add EntityCache based PolarisGrantManager impl #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d161dc
Update PolarisAuthorizerImpl to use PolarisGrantManager interface and…
collado-mike Nov 21, 2024
4d0be15
Remove ResolvedPolarisEntity and changed callers to only use PolarisE…
collado-mike Nov 21, 2024
2d5fa0c
Add additional comments
collado-mike Nov 22, 2024
9d04888
Address comments in EntityCacheGrantManager
collado-mike Nov 22, 2024
87ce0b0
Fix RealmEntityManagerFactory to reuse existing EntityCache
collado-mike Nov 23, 2024
8ef6313
Fix merge from main
collado-mike Dec 10, 2024
2ec5473
validate root container in entity cache grant manager
sfc-gh-mcollado Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions ...izard/service/src/main/java/org/apache/polaris/service/dropwizard/PolarisApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import io.opentelemetry.semconv.ServiceAttributes;
import io.prometheus.metrics.exporter.servlet.jakarta.PrometheusMetricsServlet;
import jakarta.inject.Inject;
import jakarta.inject.Named;
import jakarta.inject.Provider;
import jakarta.inject.Singleton;
import jakarta.servlet.DispatcherType;
Expand Down Expand Up @@ -93,6 +94,7 @@
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.cache.EntityCacheGrantManager;
import org.apache.polaris.core.persistence.cache.PolarisRemoteCache;
import org.apache.polaris.service.admin.PolarisServiceImpl;
import org.apache.polaris.service.admin.api.PolarisCatalogsApi;
Expand Down Expand Up @@ -302,7 +304,12 @@ protected void configure() {
// manager
bindFactory(PolarisMetaStoreManagerFactory.class)
.in(RealmScoped.class)
.named("delegateGrantManager")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.to(PolarisGrantManager.class);
bindFactory(EntityCacheGrantManagerFactory.class)
.in(RealmScoped.class)
.to(PolarisGrantManager.class)
.ranked(100);
polarisMetricRegistry.init(
IcebergRestCatalogApi.class,
IcebergRestConfigurationApi.class,
Expand Down Expand Up @@ -527,6 +534,23 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
}
}

private static class EntityCacheGrantManagerFactory implements Factory<PolarisGrantManager> {
@Inject
@Named("delegateGrantManager")
PolarisGrantManager grantManager;

@Inject EntityCache entityCache;

@RealmScoped
@Override
public PolarisGrantManager provide() {
return new EntityCacheGrantManager(grantManager, entityCache);
}

@Override
public void dispose(PolarisGrantManager instance) {}
}

/** Factory to create a CallContext based on the request contents. */
@RequestScoped
private static class CallContextFactory implements Factory<CallContext> {
Expand Down
22 changes: 16 additions & 6 deletions ...rvice/src/test/java/org/apache/polaris/service/dropwizard/admin/PolarisAuthzTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.apache.polaris.core.persistence.PolarisEntityManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.cache.EntityCacheGrantManager;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.core.storage.cache.StorageCredentialCache;
import org.apache.polaris.service.admin.PolarisAdminService;
Expand Down Expand Up @@ -132,12 +133,7 @@ public abstract class PolarisAuthzTestBase {
new Schema(
required(3, "id", Types.IntegerType.get(), "unique ID 🤪"),
required(4, "data", Types.StringType.get()));
protected final PolarisAuthorizer polarisAuthorizer =
new PolarisAuthorizerImpl(
new DefaultConfigurationStore(
Map.of(
PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING.key,
true)));
protected PolarisAuthorizer polarisAuthorizer;

protected BasePolarisCatalog baseCatalog;
protected PolarisAdminService adminService;
Expand All @@ -164,6 +160,7 @@ public void before() {
Map<String, Object> configMap =
Map.of(
"ALLOW_SPECIFYING_FILE_IO_IMPL", true, "ALLOW_EXTERNAL_METADATA_FILE_LOCATION", true);

PolarisCallContext polarisContext =
new PolarisCallContext(
metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get(),
Expand All @@ -183,6 +180,19 @@ public void before() {
callContext = CallContext.of(realmContext, polarisContext);
CallContext.setCurrentContext(callContext);

EntityCache entityCache = new EntityCache(metaStoreManager);
polarisAuthorizer =
new PolarisAuthorizerImpl(
new DefaultConfigurationStore(
Map.of(
PolarisConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING
.key,
true)),
() -> new EntityCacheGrantManager(metaStoreManager, entityCache));
this.entityManager =
new PolarisEntityManager(metaStoreManager, new StorageCredentialCache(), entityCache);
this.metaStoreManager = metaStoreManager;

PrincipalEntity rootEntity =
new PrincipalEntity(
PolarisEntity.of(
Expand Down
10 changes: 7 additions & 3 deletions ...e/src/test/java/org/apache/polaris/service/dropwizard/catalog/BasePolarisCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.cache.EntityCacheGrantManager;
import org.apache.polaris.core.storage.PolarisCredentialProperty;
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
Expand Down Expand Up @@ -158,9 +159,9 @@ public void before() {
}
},
Clock.systemDefaultZone());
EntityCache entityCache = new EntityCache(metaStoreManager);
entityManager =
new PolarisEntityManager(
metaStoreManager, new StorageCredentialCache(), new EntityCache(metaStoreManager));
new PolarisEntityManager(metaStoreManager, new StorageCredentialCache(), entityCache);

CallContext callContext = CallContext.of(realmContext, polarisContext);
CallContext.setCurrentContext(callContext);
Expand All @@ -185,7 +186,10 @@ public void before() {
entityManager,
metaStoreManager,
authenticatedRoot,
new PolarisAuthorizerImpl(new PolarisConfigurationStore() {}));
new PolarisAuthorizerImpl(
new PolarisConfigurationStore() {},
() -> new EntityCacheGrantManager(metaStoreManager, entityCache)));

String storageLocation = "s3://my-bucket/path/to/data";
storageConfigModel =
AwsStorageConfigInfo.builder()
Expand Down
9 changes: 6 additions & 3 deletions ...c/test/java/org/apache/polaris/service/dropwizard/catalog/BasePolarisCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.apache.polaris.core.persistence.PolarisEntityManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.cache.EntityCacheGrantManager;
import org.apache.polaris.core.storage.cache.StorageCredentialCache;
import org.apache.polaris.service.admin.PolarisAdminService;
import org.apache.polaris.service.catalog.BasePolarisCatalog;
Expand Down Expand Up @@ -90,9 +91,9 @@ public void before() {
},
Clock.systemDefaultZone());

EntityCache entityCache = new EntityCache(metaStoreManager);
PolarisEntityManager entityManager =
new PolarisEntityManager(
metaStoreManager, new StorageCredentialCache(), new EntityCache(metaStoreManager));
new PolarisEntityManager(metaStoreManager, new StorageCredentialCache(), entityCache);

CallContext callContext = CallContext.of(null, polarisContext);
CallContext.setCurrentContext(callContext);
Expand All @@ -117,7 +118,9 @@ public void before() {
entityManager,
metaStoreManager,
authenticatedRoot,
new PolarisAuthorizerImpl(new PolarisConfigurationStore() {}));
new PolarisAuthorizerImpl(
new PolarisConfigurationStore() {},
() -> new EntityCacheGrantManager(metaStoreManager, entityCache)));
adminService.createCatalog(
new CatalogEntity.Builder()
.setName(CATALOG_NAME)
Expand Down
29 changes: 21 additions & 8 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,20 +94,22 @@
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.inject.Inject;
import jakarta.inject.Provider;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisConfiguration;
import org.apache.polaris.core.PolarisConfigurationStore;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.entity.PolarisEntityCore;
import org.apache.polaris.core.entity.PolarisGrantRecord;
import org.apache.polaris.core.entity.PolarisPrivilege;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we injecting a Provider instead of PolarisGrantManager directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@collado-mike : from my POV this is the last remaining concern on this PR :)

this.featureConfig = featureConfig;
this.grantManagerFactory = grantManagerFactory;
}

/**
Expand Down Expand Up @@ -604,15 +609,23 @@ public boolean hasTransitivePrivilege(
Set<Long> activatedGranteeIds,
PolarisPrivilege desiredPrivilege,
PolarisResolvedPathWrapper resolvedPath) {
PolarisGrantManager grantManager = grantManagerFactory.get();
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dimas-b dimas-b Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait? Are you saying that cache eviction breaks requests?

callContext, resolvedSecurableEntity.getCatalogId(), resolvedSecurableEntity.getId());
callContext
.getDiagServices()
.check(
grantsResult.isSuccess(),
"no_grants_for_securable",
"unable to load grants for securable %s",
resolvedSecurableEntity.getId());
for (PolarisGrantRecord grantRecord : grantsResult.getGrantRecords()) {
if (matchesOrIsSubsumedBy(
desiredPrivilege, PolarisPrivilege.fromCode(grantRecord.getPrivilegeCode()))) {
// Found a potential candidate for satisfying our authz goal.
Expand Down
1 change: 1 addition & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisGrantManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

/** Manage grants for Polaris entities. */
public interface PolarisGrantManager {

/**
* Grant usage on a role to a grantee, for example granting usage on a catalog role to a principal
* role or granting a principal role to a principal.
Expand Down
71 changes: 13 additions & 58 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,10 @@

import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.entity.PolarisEntitySubType;
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.cache.EntityCache;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.core.persistence.resolver.Resolver;
Expand All @@ -45,8 +40,18 @@ public class PolarisEntityManager {

private final StorageCredentialCache credentialCache;

// Lazily instantiated only a single time per entity manager.
private ResolvedPolarisEntity implicitResolvedRootContainerEntity = null;
// For now, the root container is only implicit and doesn't exist in the entity store, and only
// the service_admin PrincipalRole has the SERVICE_MANAGE_ACCESS grant on this entity. If it
// becomes possible to grant other PrincipalRoles with SERVICE_MANAGE_ACCESS or other privileges
// on this root entity, then we must actually create a representation of this root entity in the
// entity store itself.
private final PolarisEntity implicitResolvedRootContainerEntity =
new PolarisEntity.Builder()
.setId(0L)
.setCatalogId(0L)
.setType(PolarisEntityType.ROOT)
.setName("root")
.build();

/**
* @param metaStoreManager the metastore manager for the current realm
Expand Down Expand Up @@ -85,60 +90,10 @@ public PolarisResolutionManifest prepareResolutionManifest(
PolarisResolutionManifest manifest =
new PolarisResolutionManifest(
callContext, this, authenticatedPrincipal, referenceCatalogName);
manifest.setSimulatedResolvedRootContainerEntity(
getSimulatedResolvedRootContainerEntity(callContext));
manifest.setSimulatedResolvedRootContainerEntity(implicitResolvedRootContainerEntity);
return manifest;
}

/**
* Returns a ResolvedPolarisEntity representing the realm-level "root" entity that is the implicit
* parent container of all things in this realm.
*/
private synchronized ResolvedPolarisEntity getSimulatedResolvedRootContainerEntity(
CallContext callContext) {
if (implicitResolvedRootContainerEntity == null) {
// For now, the root container is only implicit and doesn't exist in the entity store, and
// only
// the service_admin PrincipalRole has the SERVICE_MANAGE_ACCESS grant on this entity. If it
// becomes
// possible to grant other PrincipalRoles with SERVICE_MANAGE_ACCESS or other privileges on
// this
// root entity, then we must actually create a representation of this root entity in the
// entity store itself.
PolarisEntity serviceAdminPrincipalRole =
PolarisEntity.of(
metaStoreManager
.readEntityByName(
callContext.getPolarisCallContext(),
null,
PolarisEntityType.PRINCIPAL_ROLE,
PolarisEntitySubType.NULL_SUBTYPE,
PolarisEntityConstants.getNameOfPrincipalServiceAdminRole())
.getEntity());
if (serviceAdminPrincipalRole == null) {
throw new IllegalStateException("Failed to resolve service_admin PrincipalRole");
}
PolarisEntity rootContainerEntity =
new PolarisEntity.Builder()
.setId(0L)
.setCatalogId(0L)
.setType(PolarisEntityType.ROOT)
.setName("root")
.build();
PolarisGrantRecord serviceAdminGrant =
new PolarisGrantRecord(
0L,
0L,
serviceAdminPrincipalRole.getCatalogId(),
serviceAdminPrincipalRole.getId(),
PolarisPrivilege.SERVICE_MANAGE_ACCESS.getCode());

implicitResolvedRootContainerEntity =
new ResolvedPolarisEntity(rootContainerEntity, null, List.of(serviceAdminGrant));
}
return implicitResolvedRootContainerEntity;
}

public StorageCredentialCache getCredentialCache() {
return credentialCache;
}
Expand Down
Loading
Loading