-
Notifications
You must be signed in to change notification settings - Fork 332
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.
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
Make ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS configurable per catalog #2688
Conversation
|
|
||
| CatalogEntity catalogEntity = | ||
| CatalogEntity.of( | ||
| findCatalogByName(catalogName) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 : )
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
| .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") |
There was a problem hiding this comment.
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:
- Explicitly block grant/revoke to namespaces/tables if the config is off, even if JIT entities are there.
- Disallow change the param from
truetofalseduring 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
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
…r federated catalog at catalog level (#2696) In #2688 (comment), we've identified that configuring polaris.config.enable-sub-catalog-rbac-for-federated-catalogs at catalog level should not be allowed in all cases, especially when the owner is not the same subject as the catalog user or admin. This PR add a feature flag, ALLOW_SETTING_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS to allow owner to disable catalog level setting polaris.config.enable-sub-catalog-rbac-for-federated-catalogs
In #2223 (comment), one follow-up we identify is to make JIT entities creation configurable per catalog instead of per realm. This PR leverage the feature configuration catalog level override to do this. The corresponding catalog property will be