Support Namespace/Table level RBAC for external passthrough catalogs

[poojanilangekar]

This PR creates missing synthetic entities for securables in external passthrough catalogs.
Based on Option 1 discussed in the RBAC section of this doc.

In the future, we could remove calls to PolarisEntity.Builder() and replace them with entities fetched from the remote catalog. (enabling Option 2).

Testing:
Unit test primarily based on mocking path resolution and the metastore API.

[poojanilangekar]

CC: @dennishuo

[dennishuo]

Generally LGTM, but just three important fixes:

1. Use NamespaceEntity.Builder and make sure to call setParentNamespace
2. Use IcebergTableLikeEntity.Builder and make sure to call setTableIdentifier
3. For now, if subTypes includes ICEBERG_TABLE we should make our IcebergTableLikeEntity have setSubType(ICEBERG_TABLE) and else if contains VIEW we should setSubType(VIEW) instead of taking subTypes.get(0) because we don't actually support federation for GENERIC_TABLES yet.

[dennishuo]

We may also want to add a FeatureConfiguration to gate this path; even if federation is still "experimental" so we can change default behaviors, this persistence-backfill is definitely something people might still potentially decide to disable.

I'd make the feature configuration be a service-level config (no catalogConfig) because the gate really is more of something the service owner would want to be able to prevent catalog-users from using than a self opt-in per-catalog.

Maybe something like ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS, though maybe you can come up with a better config name :)

[poojanilangekar]

Done.

[dennishuo]

Some code comments would be nice here to explain why the subList starts at 1 (is it because 0 is the CatalogEntity?)

Also this all might be more readable if we shift-right by one, instead of needing matchingLevel + 1 in all the places that use it. Basically make it a count instead of the level:

int numMatchingLevels = 0;
....
... (allNamespaceLevels[numMatchingLevels])...
...
for (int i = numMatchingLevels; ...
...
...Arrays.copyOf(allNamespaceLevels, i)...

Though the semantics of "matchingLevel" does make sense in the normal case, relying on matchingLevel == -1 for i = matchingLevel + 1 in the case where there are 0 matches is a bit jarring when reading the code.

[poojanilangekar]

Done.

[dennishuo]

Unfortunately, we probably don't want to do this. Looks like the current behavior will be to create a GENERIC_TABLE for the synthetic entity, which will cause problems once we want to start leaning more on the JIT-created entities for other things like actually having the metadata location, etc.

It might need to be ugly at first -- it's even better to just special-case ICEBERG_TABLE vs VIEW (and ignore GENERIC_TABLE at first) for now. Maybe extract a helper to hide the ugliness that takes subTypes and basically does:

if (subTypes.contains(ICEBERG_TABLE)) {
  return ICEBERG_TABLE;
} else if (subTypes.contains(VIEW) {
  return VIEW;
} else {
  throw new IllegalStateException(...)
 }

For now that is "correct" because our federation catalog factory (last I checked) only supports Iceberg remote catalogs, and nothing for GENERIC_TABLE.

Once we also support GENERIC_TABLE federation, we'd need the intended type at the callsite anyways to even know whether the "remote catalog for mediating the backfill" should be instantiated via an Iceberg RESTCatalog factory or a different factory for GenericCatalogs.

That might be a big hurdle because of ICEBERG_TABLE and GENERIC_TABLE living in the same "type" namespace, but that will be a separate workstream anyways.

We can document this issue in another // TODO: comment.

[poojanilangekar]

Done.

[dennishuo]

We should try to use the IcebergTableLikeEntity.Builder here instead of the general builder. I understand it might be a bit messy due to the metadataLocation argument in the builder constructor, but as far as I can tell it's graceful enough to accept "" for the metadataLocation, and actually this is kind of working as intended for the Builder to help enforce the existence of expected fields in the entity type.

Once we have the remote-mediated backfill, we'll get to actually plug in the real metadataLocation to the constructor which will benefit everyone.

Also, it's important to setTableIdentifier because listGrants* depends on it (otherwise, normally a leaf entity doesn't have a representation of its parent)

[poojanilangekar]

Done.

[dennishuo]

We should try to use the NamespaceEntity.Builder if possible.

Also, it's necessary to call setParentNamespace because listGrants* relies on it.

[poojanilangekar]

Done.

[snazy]

I have some of questions related to this approach.

1. In case the "remote" namespace/table/view gets renamed, how is that reflected in the "source"? What happens to child namespaces/tables/views in the "source".
2. In case the "remote" namespace/table/view gets re-created, how is that reflected in the "source"? What happens, if e.g. a table is dropped and the name's reused for a namespace?
3. In case the "remote" namespace/table/view is deleted, how is that reflected in the "source"? Do we pile up unused data?

[poojanilangekar]

Your concerns are valid and will be addressed as we implement the subsequent milestones for federation as outlined in Apache Polaris Catalog Federation Proposal and Apache Polaris Federation - Non-REST Remotes, Credential Vending and Table-Level RBAC Outline.

As @dennishuo outlined in the document, the initial MVP creates synthetic entities. These synthetic entities are not "true" entities that are consistent with the source of truth catalog. Instead they allow Polaris to treat the components of a federated catalog as securables, much like entities in internal catalogs. Without this PR, users can only set catalog-level grants on federated catalogs -- making it unsuitable in case they want to restrict access to tables/namespaces.

The next step would be to JIT create entities, i.e., "Require actually fetching a successful response from the remote catalog before producing the synthetic passthrough facade entity" (as stated in the document). Lastly, when we implement all other parts of federation to support the ultimate goal of catalog migration, Polaris will support passthrough facade with caching and strongly-consistent cache-invalidation. This is PR is a building block needed to enable the future steps.

[dennishuo]

LGTM!

Echoing Pooja's comments about next steps, we'll need to add the remote-catalog fetch to improve the coherence between federated "facade entities" in Polaris and the remote catalog. One natural way to detect table rename/recreation is using the Iceberg table-uuid.

However, this will by nature be "best-effort" in various ways as long as Polaris and the remote catalog aren't a single transactional system, because for example:

1. Iceberg doesn't have the concept of namespace-uuid yet so we can't do the same for distinguishing incarnations of a namespace
2. Iceberg's table-uuid is "tamperable" since it's just a string in the metadata JSON file
3. It's possible to use registerTable to cause different Polaris entity ids to have the same uuid already

If we do end up distinguishing by uuid, then any orphaned PolarisEntities would indeed just be small bits of garbage to be collected, same as potentially orphaned GrantRecords today in the AtomicOperationMetaStoreManager where GrantRecord deletion is best-effort after Entity deletion.

If we don't distinguish by uuid, the semantics of grants in federated catalogs behave equivalently to "name-based" access policies, which isn't that bad since that's standard for various segregated-policy systems like using Apache Ranger.

I'd agree we need to think through the name vs id modes and document the options thoroughly in any case.

For now, since this is a cleanly flag-gated feature that defaults to being disabled, perhaps we can merge and iterate.

[snazy]

I think this needs more attention and some more clarity on how things work.

So let's wait with this and not merge yet, but give everybody enough time to think about this.

[snazy]

The implementation of PolarisAdminService.createSyntheticNamespaceEntities() looks racy to me. I suspect that will fail in various ways when concurrent requests target the same table/view or namespace elements.

[poojanilangekar]

@snazy If the creation of the synthetic namespace fails due to a concurrent request, the else condition subsequently reads the concurrently created namespace. (The same goes for tables/views.) Shouldn't this alleviate your concerns? Can you please elaborate on the potential race condition here?

CC: @dennishuo

[snazy]

The resolutionManifest doesn't know that, no?

Overall, this should be an atomic change - all or nothing, including that "ephemeral table/view". But that's not possible.

I'm not convinced that this works w/ e.g. serializable isolation and in all cases.

[pingtimeout]

@poojanilangekar maybe a naive question: the documentation of PolarisMetaStoreManager::createEntityIfNotExists says:

Returns the newly created entity. If this entity was already created, we will simply return the already created entity. We will return null if a different entity with the same name exists or if the catalogPath couldn't be resolved. If null is returned, the client app should retry this operation.

And the code is not retrying that operation. Shouldn't it attempt it multiple times instead of using such if/else/if/else blocks?

[snazy]

The "create-if-not-already-exists" logic to return the conflicting row fails with the serializable isolation level. That's the nature of that isolation level.

Further, the "resolution manifest" holds the "local" state, but is never updated with "concurrent" changes.

[dennishuo]

Yeah good point about the resolution manifest. I believe it's a bug - line 1819 should be calling getPassthroughResolvedPath instead of getResolvedPath.

Re: serializable isolation, given the fact that the remote catalog itself is fundamentally non-atomic with anything we see on the local Polaris (in particular, there's no atomic multi-table+multi-namespace GET in Iceberg REST anyways), we're fundamentally shooting for more of an "eventual consistency" semantic in federated catalogs.

While concurrent jit-creation might be semi-common due to different workloads trying to hit the same not-yet-JIT-created entity at the same time, it'd be more rare that a JIT-created entity also then gets dropped/unlinked locally within the race condition followed by the attempt to re-read the conflicting entity.

So if I understand the race condition identified here correctly, it seems fine to throw some kind of UnknownStateException in this case where the re-read is null and have the caller figure out if they just want to retry or figure out the state manually first.

[HonahX]

Thanks @dennishuo for the detailed explanation!

I believe it's a bug - line 1819 should be calling getPassthroughResolvedPath

+1, IMHO, the overall order might be

• resolutionManifest tries to resolve the path
• some or all JIT entities not found, backfill triggered.
• resolutionManifest getPassthroughPath to resolve the path again
• Proceed if the whole path resolved, fail with UnkownStateException/ConcurrentModification... if not

We could try to see if it worth to add some option to make the batch creation api, createEntitiesIfNotExist not fail on EntityAlreadyExistException but simply return the entity if it is already there, this way we could push down the actual creation into the persistence layer instead of in the for-loop. But this may need broader discussion as this is persistence API change.

[poojanilangekar]

I can fix line 1819.

However, @HonahX, what you're suggesting is not something Polaris currently supports, as we don't have any backfill functionality yet. I think for the backfill to work, we need to add Passthrough Facade support for the federated catalog.

[HonahX]

Hi @poojanilangekar, for backfill I mean creating JIT dummy entities like what the current PR do. But yes, the remaining persistence layer changes are not yet supported in Polaris and also does not need to be in this PR.

For this one, using getPassthroughResolvedPath to get the fresh entity should be enough : )

[dennishuo]

Yeah my preference would be to go ahead with simply throwing the exception for now, doing the basic fix to call getPassthroughResolvedPath, merging and then iterate on improvements afterwards.

On the terminology of backfill here, my understanding was that the "backfill triggered" @HonahX is mentioning isn't describing any fundamentally new/different code flow, but is just talking about the JIT-creation here itself as the backfill. So I think we're in alignment, just describing the current PR's flow in slightly different terms.

[HonahX]

Hi @poojanilangekar , thanks for working on this! I'm also interested in table-level RBAC for federated catalog. Do you mind if I continue work on this PR if you do not have bandwidth?

[dimas-b]

A plain RuntimeException will probably show up as a 500 response at the REST API level... Is that intended?

[poojanilangekar]

Is there another exception you'd prefer? If so, I can change it.

[dennishuo]

The most appropriate one in this case is probably UnprocessableEntityException

[HonahX]

Since currently we just create synthetic entities without performing a passthrough, the expectation for createSyntheticNamespaceEntities is to create all synthetic entities or make sure they all exist. If something like CommitStateUnknown (https://github.com/apache/polaris/pull/2223/files#r2366658329) happens, it should be thrown inside the createSyntheticNamespaceEntities.

At this point, I am leaning towards 500 as this should actually be an illegalState after the backfill. UnprocessableEntityException as 422 may imply that repeating request will fail with the same error, but seems we cannot guarantee that here. WDYT?

[dennishuo]

Ah you're right, maybe not UnprocessableEntityException, I misremembered the meaning.

409 is also potentially accurate, just that it happens to be a server-side conflict where the caller doesn't necessarily need to do anything different in the request to retry it successfully. So we could return CommitFailedException.

But it's also true that as long as we don't yet drop facade entities, it's an unexpected illegal state. Then the "raw" RuntimeException correctly gets mapped to INTERNAL_SERVER_ERROR.

Maybe for now we could just add a TODO here to possibly update the exception thrown as we refine the possible retry scenarios?

[HonahX]

That sounds good, we will have to update the spec anyway later since the new error code we send back is not documented in the response code list.

[dimas-b]

A for the status code, 500 seems reasonable to me given the current implementation.

I guess the point I was trying to make is that this exception is thrown based on a plain (boolean) condition that something is not available. Ideally the error (and its message) should be based on the real underlying error.... but I do not insist on addressing this in current PR.

[dimas-b]

What is the purpose of this TODO comment? Could you clarify?

[dennishuo]

I believe this refers to the Require actually fetching a successful response from the remote catalog before producing the synthetic passthrough facade entity portion of https://docs.google.com/document/d/19Vm0Uy-EyEYtgd2-bZpPICODOB3rYQJvHeqL1ODOOLc/edit?tab=t.0#bookmark=id.7cguk2qz0p88

Admittedly the wording is a bit confusing here; it's not so much changing the flow to some totally different "backfill" mechanism, but just changing whether we contact the remote catalog to check the existence/state of the implied namespace entity before creating it.

Probably we should update the comment to say something like TODO: Instead of blindly creating the passthrough-facade entity, standardize onto some kind of "createOrUpdatePassthroughFacadeEntityBasedOnRemoteCatalog" helper.

[dimas-b]

The proposed message update certainly sounds more clear :)

[dimas-b]

I believe it might be preferable to have a separate API for filling in synthetic namespaces rather that to do that implicitly during grants.

Namespaces are first-class entities in Polaris. They may be used / seen / affect other call paths, not just grants.

[poojanilangekar]

Can you please elaborate on your suggestion? Does that mean we manage two different types of path resolution, one for regular namespaces and if the regular namespace fails, we look for synthetic namespaces? Or do you suggest we add some sort of metadata to the entity to indicate that it's synthetic?

Additionally, what happens when we backfill information from the source of truth catalog? Do we still want it to be synthetic or does it change the type?

[dennishuo]

Relating to my comment in the other file #2223 (comment) we probably don't need to necessarily care about the synthetic nature of the entity so much as its role as a "Passthrough Facade".

@dimas-b I agree we should extract common "backfill/JIT-creation" logic, possibly as a fast-follow or we can do it inline here if we really want as well, but it would be "in order to" use that common logic for the implicit backfill during grants. rather than precluding that backfill.

It would then also be shared for implicit backfill during loadTable (could be configured as an option, if the serving of the loadTable doesn't strictly require the entity to be persisted).

This design tradeoff of whether to JIT-create automatically vs requiring the separate API is discussed a bit here: https://docs.google.com/document/d/1Q6eEytxb0btpOPcL8RtkULskOlYUCo_3FLvFRnHkzBY/edit?tab=t.0#bookmark=id.cf85r4rp8tz - Passthrough Facade with JIT-Creation instead of requiring upfront facade creation

I suppose we could ultimately also make it a configurable option if we expose an actual REST API endpoint for materializing the facade entity, but historically the requirement of upfront facade creation has demonstrated to be a barrier to adoption.

I'd say these JIT-created facade entities are still first-class entities as intended, just that they provide the semantic of the Passthrough Facade rather than a source-of-truth entity. In general a Passthrough Facade would mean:

1. Listing the parent doesn't get to short-circuit with passthrough facades -- "list" is strictly defined by the remote catalog
2. GET on the entity represented by the facade still requires some readthrough to the remote catalog, but the contents of a facade entity will augment or optimize the response. For example:
   • Cached metadata representation - a facade may have a local copy of the full TableMetadata JSON along with the etag associated with it from the remote - the passthrough loadTable may use the etag to efficiently determine that the local copy is already fresh, and thus serve the response directly from the local copy
   • Grants semantics - as this PR adds, the facade entity serves as the relational entity within the Polaris RBAC grants system
   • Credential vending - In upcoming related work outlined in the addendum https://docs.google.com/document/d/19Vm0Uy-EyEYtgd2-bZpPICODOB3rYQJvHeqL1ODOOLc/edit?tab=t.0 the local TableMetadata of the facade entity allows us to safely reuse credential-subscoping logic which takes in the logical table data/metadata read/write paths as part of the subscoping policy

[dimas-b]

To clarify my first comment: I meant having an API to create the synthetic entries that is separate from the grant API. Essentially from the client side the flow will have two stages:

1. Decide what synthetic namespace are required and create them (new API)
2. Grant (old API)

[dimas-b]

I do not have a strong objection against on demand creation of synthetic entities, but in that case I think the rbacForFederatedCatalogsEnabled flag should be configurable per catalog (not per realm).... if it's not too hard to implement.

From the perspective of a self-hosted Polaris, admin users may want to manage this behaviour separately in different catalogs.

[dennishuo]

Yeah that definitely seems reasonable. We might want a way to convey allowing a service owner to force a realm-level setting (at least if the service owner wants to force not JIT-creating entities because they don't want to use up lots of database storage), but generally it seems convenient to be able to allow per-catalog settings.

That matches what I imagined as well, which would be that per-catalog owners may eventually choose to even update the passthrough or caching or preemptive-sync/migration settings over the lifetime of a catalog, maybe starting with keeping things as fully "implicit/passthrough" as much as possible then enabling JIT-creation once they decide they want more advanced caching/migration features.

[dimas-b]

How can this Namespace be identified as a "synthetic" namespace later?

[dennishuo]

We should probably sort out the nuances of where the "synthetic" term fits in vs "passthrough facade" (synthetic is more just descriptive of the process by which the entity is formulated, but ultimately the "type" we're interested in identifying later is that the entity represents a "passthrough facade", even if its origins in the future may or may not be considered "synthetic" in the same way).

To answer your question - right now the code basically defines all entities within an ExternalCatalog to be passthrough facades if they exist, so the identity is inherited from its parent catalog. I suppose we could also set a flag of some sort that explicitly identifies it as being a passthrough facade, but we might want to do some more design discussion about that. Individually identifying entities would mostly be important in a couple potential scenarios:

1. If we support "mixed-mode" catalogs where some entities are natively owned in Polaris while others are basically "symlinks" to remote tables. Then we presumably need to know if any given table is local or remote. By default, we probably don't want to support this use case unless we have a really compelling reason.
2. A variation of (1) would be if we support gradual "migration" scenarios. In such a case, we want the ability to incrementally transfer individual tables into having the local Polaris server be the source of truth, so it's also similar to a "mixed-mode" catalog, except the key difference is that we'd have much more constrained intermediate states, and all the tables "originally" came from the same remote catalog.

The basic intent of a Catalog's ConnectionConfig was to be resolved hierarchically the same way the StorageConfig is technically resolved hierarchically, so that if it's present on a parent namespace or on the table itself, the "nearest" definition wins. So one possibility is that we set the ConnectionConfig directly on the table to indicate that it's a passthrough facade.

[poojanilangekar]

@HonahX, I can work on the changes once we have an agreement on how to handle the ResolutionManifests that fail creation.

[HonahX]

@poojanilangekar Thanks for the contribution! I think we're very close! I have some follow-up in mind but those should not block this PR. The current PR IMHO is a great and solid starting point to support advanced catalog federation as described in: https://docs.google.com/document/d/19Vm0Uy-EyEYtgd2-bZpPICODOB3rYQJvHeqL1ODOOLc/edit?tab=t.0#heading=h.e4lu42ax5kvq. Given all the features are guarded by feature parm default to false, we could safely iterate the feature in follow-up PRs.

[HonahX]

I think it would be better if we check the result status is ENTITY_ALREADY_EXISTS before entering the getPassthroughResolvedPath below. Though ENTITY_ALREADY_EXISTS is the only error status now, there could be more in the future and we also have a general UNEXPECTED_ERROR status, where we do not want to proceed but just throw an exception.

[HonahX]

[non-blocker] We may consider extract this and the table creation one out from PolarisAdminService to a separate class, or within the ResolutionManifest, to make PolarisAdminService focusing on the admin logics. But we could do any big refactoring like this in follow-ups

[HonahX]

Suggested change

	.setCreateTimestamp(System.currentTimeMillis())
	.setCreateTimestamp(System.currentTimeMillis())
	.setParentId(parentNamespaceEntity.getId())

Need setParentId here too to make it resolvable later

[HonahX]

In case of nested namespace, this will fail at

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java

Lines 181 to 186 in 06d5231

	diagnostics.check(
	passthroughPaths.containsKey(key),
	"invalid_key_for_passthrough_resolved_path",
	"key={} passthroughPaths={}",
	key,
	passthroughPaths);

since we never added the partialNamespace key to the (passthrough) paths. Only the full namespace in the path.

We could just use the full namespace as the identifier and get a newer version of the partial path. If the synthetic namespace entity still not appear in the leaf, we throw the error. If the new partial path contains even more child entities than expected (e.g. Try to create ns1.ns1a.ns1aa, expect ns1.ns1a but get ns1.ns1a.ns1aa), we could either validate and reconcile or throw an error.

[dimas-b]

As I commented in more specific threads on this PR, there are areas to improve from my POV. However, the feature flags appear to protect the main call paths enough so as to allow existing Polaris features to function properly. So, I would not mind merging this PR in its current form (pending conflict resolution) and improving the codebase in follow-up PRs.

[HonahX]

Thanks everyone for reviewing! This PR was merged in #2673 with merge conflicts fixed and some additional test. Let's keep evolving and improving this in follow-up PRs. I will close this one as complete.

Especially thanks @poojanilangekar for the great work!