[Catalog Federation] Add feature flag to disallow setting sub-RBAC for federated catalog at catalog level

[HonahX]

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

[XJDKC]

Overall LGTM!

[XJDKC]

We need this validation when updating the catalog entity as well

[HonahX]

Yes, that is added in line 249 https://github.com/apache/polaris/pull/2696/files#diff-da3b7ab11bab566a4c161978634dda3e2d194a81827d36edaa4fb8ee23691348R249. I also added a test to cover that case

[eric-maynard]

Do we really want to control whether or not you can set the property? Or do we want to control whether or not RBAC settings can be applied / will get respected?

(Let ALLOW_SETTING_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS be C)

In other words, is it the intended use case that I can start with C as true, set ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS on my catalog, then set C to false, and later continue to change RBAC settings within my catalog? When is this preferable to C just meaning that RBAC settings within my catalog no longer apply or can no longer be changed?

[HonahX]

Great question! Here is some more context: #2688 (comment),
#2688 (comment).

TLDR, we want to support configuring ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS per catalog instead of per realm, but with some limitaiton. This PR is the first step that at least the owner could disable any changes to ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS on the catalog.

IMHO, more limitation should be imposed on ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS, for example to prevent it from turning off if turned on, to prevent unnecessary complexity if you first step up some grants and suddenly lose the power to change it, but existing one still take effect. This could be a general enhancement option for our feature flag infra : )

In other words, is it the intended use case that I can start with C as true, set ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS on my catalog, then set C to false, and later continue to change RBAC settings within my catalog?

I do not think this is the intended use-case. IMHO, ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS should not be changed frequently

[eric-maynard]

I think you're right that there's a more general gap in the flag framework and that it might be nice to be able to, for example, configure a catalog such that no catalog overrides apply to it or can be set on it. What @dimas-b wrote in your linked comment makes sense to me.

But what's being done here is specific to one setting... creating a second flag for every flag where we want this level of control seems like a bad precedent to set. Further, in this context, the message used in the implementation (Explicitly setting %s is not allowed.) seems a little unhelpful. Why is it not allowed? Is there a setting I can change to make it allowed?

[HonahX]

I've revised the error message to contain info that ALLOW_SETTING_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS need to be set to allow explicitly setting sub-rbac at catalog-level.

But what's being done here is specific to one setting... creating a second flag for every flag where we want this level of control seems like a bad precedent to set.

Yeah, I agree this does not scale up in the long term. But I think at this time, we probably need to identify more use-cases to make a comprehensive infra enhancement for this type of utility. How about adding a general flag

  public static final FeatureConfiguration<List<String>> CATALOG_CONFIG_OVERRIDE_BLOCKLIST = PolarisConfiguration.<List<String>>builder()
      .key("CATALOG_CONFIG_OVERRIDE_BLOCKLIST")
      .description("A list of feature configuration keys that are not allowed to be overridden at the catalog level.\n" +
              "Any key included here will always use the realm-level setting, and attempts to override it per catalog will be rejected.")
      .defaultValue(
          List.of())
      .buildFeatureConfiguration();

So that people could register feature keys that they want to block catalog overrides in the future?

[eric-maynard]

Something like that could work, though I could imagine wanting to set it at a catalog level as well.

[dimas-b]

+1 to following up on this and making config overrides and allow/block checks more general.

[dimas-b]

LGTM 👍 Thanks @HonahX !

Adding a CHANGELOG notice would be nice :)

[HonahX]

Thanks @dimas-b @eric-maynard @XJDKC for reviewing!