Skip to content

Make ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS configurable per catalog #2688

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

Merged
merged 2 commits into from
Sep 25, 2025
Merged

Make ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS configurable per catalog #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
140dc26
Update ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS to be configura…
HonahX Sep 25, 2025
ce36a4d
Fix test failures
HonahX Sep 25, 2025
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
1 change: 1 addition & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ public static void enforceFeatureEnabledOrThrow(
.description(
"When enabled, allows RBAC operations to create synthetic entities for"
+ " entities in federated catalogs that don't exist in the local metastore.")
.catalogConfig("polaris.config.enable-sub-catalog-rbac-for-federated-catalogs")
Copy link
Contributor Author

@HonahX HonahX Sep 25, 2025

Choose a reason for hiding this comment

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

To continue the discussion #2223 (comment),
I am +1 on making this configurable at catalog level too. But I am a little bit worried about allowing user/catalog admin to to turned it off after it turned on.

Currently we have 2 model,
Without JIT creation, user will find grant to namespace/tables fail with 404

With JIT creation, user should be able to grant to namespace/tables as if they exists.

If we allow users to turn JIT creation off after turned it on for some time, the federated catalog will be in a weird status where some of namespaces/tables with JIT entities backfilled, could still accept new grants/revokes, other namespaces/tables grants will fail with 404.

To address this inconsistency, I see 2 option:

  1. Explicitly block grant/revoke to namespaces/tables if the config is off, even if JIT entities are there.
  2. Disallow change the param from true to false during catalog update.

IMHO, 2 is safer since even if we block further grants, the existing grant records will still take effect during loadTable/createTable/..etc.

But would love to hear more thoughts on this! We could also decide this in a follow-up PR

cc: @dennishuo @dimas-b

Copy link
Contributor

@dimas-b dimas-b Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, @HonahX !

In general, this feels like we need to enhance our feature flags support to allow more control on the Polaris "owner" side.

I do believe setting this particular flag per catalog is useful for self-hosted environments (where owner == admin user).

For Polaris-as-a-Service deployments, we probably need to restrict certain settings to be managed only by the owner, who may not be the same subject as the catalog user or administrator.

In short term, WDYT about adding another feature flag (defaulting to "allowed") to control whether polaris.config.enable-sub-catalog-rbac-for-federated-catalogs may be added to Catalog properties (existing similar use case: #2442)?

Copy link
Contributor Author

@HonahX HonahX Sep 25, 2025

Choose a reason for hiding this comment

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

Thanks @dimas-b , nice suggestion! I think adding a mechanism for the "owner" to optionally restrict changes to potentially high-impact parameters on a per-catalog basis would help mitigate this situation.

Given the refactoring and follow-up changes we’ve already identified, I’ll go ahead and merge this first and open other ones.

.defaultValue(false)
.buildFeatureConfiguration();

Expand Down
15 changes: 12 additions & 3 deletions runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,13 +511,18 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
}
}

CatalogEntity catalogEntity =
CatalogEntity.of(
findCatalogByName(catalogName)
Copy link
Contributor

@dimas-b dimas-b Sep 25, 2025

Choose a reason for hiding this comment

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

Is it not part of resolutionManifest already?

Copy link
Contributor Author

@HonahX HonahX Sep 25, 2025

Choose a reason for hiding this comment

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

The name of this helper is somewhat misleading:

polaris/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines 193 to 196 in 925c00b

private Optional<CatalogEntity> findCatalogByName(String name) {
return Optional.ofNullable(resolutionManifest.getResolvedReferenceCatalogEntity())
.map(path -> CatalogEntity.of(path.getRawLeafEntity()));
}

Underlying it just extract the entity from the resolutionManifest, the name is ignored : )

Copy link
Contributor

@dimas-b dimas-b Sep 25, 2025

Choose a reason for hiding this comment

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

Sweet :)

Mind removing the unused parameter for clarity?.. maybe in another PR, if you prefer :)

Copy link
Contributor Author

@HonahX HonahX Sep 25, 2025

Choose a reason for hiding this comment

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

I will do it in a follow-up as it will affect other part of the code.

.orElseThrow(() -> new NotFoundException("Catalog %s not found", catalogName)));
PolarisResolvedPathWrapper tableLikeWrapper =
resolutionManifest.getResolvedPath(
identifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE, true);
boolean rbacForFederatedCatalogsEnabled =
getCurrentPolarisContext()
.getRealmConfig()
.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS);
.getConfig(
FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS, catalogEntity);
if (!(resolutionManifest.getIsPassthroughFacade() && rbacForFederatedCatalogsEnabled)
&& !subTypes.contains(tableLikeWrapper.getRawLeafEntity().getSubType())) {
CatalogHandler.throwNotFoundExceptionForTableLikeEntity(identifier, subTypes);
Expand Down Expand Up @@ -1710,7 +1715,9 @@ public PrivilegeResult grantPrivilegeOnNamespaceToRole(
boolean rbacForFederatedCatalogsEnabled =
getCurrentPolarisContext()
.getRealmConfig()
.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS);
.getConfig(
FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS,
catalogEntity);
if (resolutionManifest.getIsPassthroughFacade() && rbacForFederatedCatalogsEnabled) {
resolvedPathWrapper =
createSyntheticNamespaceEntities(catalogEntity, namespace, resolvedPathWrapper);
Expand Down Expand Up @@ -2136,7 +2143,9 @@ private PrivilegeResult grantPrivilegeOnTableLikeToRole(
boolean rbacForFederatedCatalogsEnabled =
getCurrentPolarisContext()
.getRealmConfig()
.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS);
.getConfig(
FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS,
catalogEntity);
if (resolutionManifest.getIsPassthroughFacade() && rbacForFederatedCatalogsEnabled) {
resolvedPathWrapper =
createSyntheticTableLikeEntities(
Expand Down
10 changes: 10 additions & 0 deletions runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;

public class PolarisAdminServiceTest {
Expand Down Expand Up @@ -90,6 +91,9 @@ void setUp() throws Exception {
// Default feature configuration - enabled by default
when(realmConfig.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS))
.thenReturn(true);
when(realmConfig.getConfig(
eq(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS), Mockito.any()))
.thenReturn(true);

when(resolutionManifestFactory.createResolutionManifest(any(), any(), any()))
.thenReturn(resolutionManifest);
Expand Down Expand Up @@ -358,6 +362,9 @@ void testGrantPrivilegeOnNamespaceToRole_PassthroughFacade_FeatureDisabled() thr
// Disable the feature configuration
when(realmConfig.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS))
.thenReturn(false);
when(realmConfig.getConfig(
eq(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS), Mockito.any()))
.thenReturn(false);

PolarisEntity catalogEntity = createEntity(catalogName, PolarisEntityType.CATALOG);
PolarisResolvedPathWrapper catalogWrapper = mock(PolarisResolvedPathWrapper.class);
Expand Down Expand Up @@ -522,6 +529,9 @@ void testGrantPrivilegeOnTableLikeToRole_PassthroughFacade_FeatureDisabled() thr
// Disable the feature configuration
when(realmConfig.getConfig(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS))
.thenReturn(false);
when(realmConfig.getConfig(
eq(FeatureConfiguration.ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS), Mockito.any()))
.thenReturn(false);

PolarisEntity catalogEntity = createEntity(catalogName, PolarisEntityType.CATALOG);
PolarisResolvedPathWrapper catalogWrapper = mock(PolarisResolvedPathWrapper.class);
Expand Down
2 changes: 1 addition & 1 deletion runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ public Map<String, String> getConfigOverrides() {
.put("polaris.features.\"DROP_WITH_PURGE_ENABLED\"", "true")
.put("polaris.behavior-changes.\"ALLOW_NAMESPACE_CUSTOM_LOCATION\"", "true")
.put("polaris.features.\"ENABLE_CATALOG_FEDERATION\"", "true")
.put("polaris.features.\"ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS\"", "true")
.build();
}
}
Expand Down Expand Up @@ -303,6 +302,7 @@ public void before(TestInfo testInfo) {
realmConfig,
storageConfigModelForFederatedCatalog,
storageLocationForFederatedCatalog)
.addProperty("polaris.config.enable-sub-catalog-rbac-for-federated-catalogs", "true")
.build();
ExternalCatalog externalCatalog =
ExternalCatalog.builder()
Expand Down