yuqi1129 commented on code in PR #12517:
URL: https://github.com/apache/gravitino/pull/12517#discussion_r3819979828
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -523,9 +535,9 @@ public Boolean onOwnerSet(MetadataObject metadataObject,
Owner preOwner, Owner n
case CATALOG:
// The metalake and catalog use role to manage the owner
if (metadataObject.type() == MetadataObject.Type.METALAKE) {
- ownerRoleName = RangerHelper.GRAVITINO_METALAKE_OWNER_ROLE;
+ ownerRoleName =
RangerHelper.generateMetalakeOwnerRoleName(metalakeId);
} else {
- ownerRoleName = RangerHelper.GRAVITINO_CATALOG_OWNER_ROLE;
+ ownerRoleName = RangerHelper.generateCatalogOwnerRoleName(catalogId);
Review Comment:
For an existing Ranger policy, updatePolicyOwnerRole only adds this new
ID-specific role. It does not remove the old shared
GRAVITINO_CATALOG_OWNER_ROLE or GRAVITINO_METALAKE_OWNER_ROLE. After an
upgrade, users in the old shared role can still access other catalogs. Please
remove the matching old shared role when migrating an existing policy.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1301,14 +1302,26 @@ private Map<String, String>
getResolvedProperties(CatalogEntity entity) {
}
private BaseCatalog<?> createBaseCatalog(IsolatedClassLoader classLoader,
CatalogEntity entity) {
+ BaseMetalake metalakeEntity;
+ try {
+ metalakeEntity =
+ store.get(
+ NameIdentifier.of(entity.namespace().levels()),
+ EntityType.METALAKE,
+ BaseMetalake.class);
+ } catch (IOException e) {
+ throw new RuntimeException(
+ String.format("Failed to load metalake for catalog %s",
entity.nameIdentifier()), e);
+ }
+
// Load Catalog class instance
BaseCatalog<?> catalog = createCatalogInstance(classLoader,
entity.getProvider());
// Resolve secret URNs to plaintext for connector init only; entity
storage keeps URNs.
// Fileset FS merge assumes catalog conf is already plaintext at this
boundary.
catalog
.withCatalogConf(secretManager.toPlaintextProperties(entity.getProperties()))
.withCatalogEntity(entity);
- catalog.initAuthorizationPluginInstance(classLoader);
+ catalog.initAuthorizationPluginInstance(classLoader, metalakeEntity.id());
Review Comment:
How are existing owners migrated? Passing the IDs here only affects newly
created plugin actions. If a catalog owner does not change after an upgrade,
the new role is not granted and the old policy is not updated. Please add an
idempotent migration or reconciliation path for existing roles, members, and
policies.
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java:
##########
@@ -1492,8 +1493,8 @@ public void testOnOwnerSetCatalog() {
null,
null,
Lists.newArrayList(
- RangerHelper.GRAVITINO_METALAKE_OWNER_ROLE,
- RangerHelper.GRAVITINO_CATALOG_OWNER_ROLE));
+
RangerHelper.generateMetalakeOwnerRoleName(RangerITEnv.METALAKE_ID),
+
RangerHelper.generateCatalogOwnerRoleName(RangerITEnv.CATALOG_ID)));
Review Comment:
Could we add a regression test with two catalogs using the same Ranger
service, different IDs, and different owners? Please also start with a legacy
shared owner role and policy, then verify that they are migrated. The current
test uses one fresh catalog, so it does not reproduce issue #8674 or the
upgrade case.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -280,7 +280,13 @@ public AuthorizationPlugin getAuthorizationPlugin() {
return authorizationPlugin;
}
- public void initAuthorizationPluginInstance(IsolatedClassLoader classLoader)
{
+ /**
+ * Initializes the authorization plugin for this catalog.
+ *
+ * @param classLoader the catalog isolated class loader
+ * @param metalakeId the stable entity ID of the metalake containing this
catalog
+ */
+ public void initAuthorizationPluginInstance(IsolatedClassLoader classLoader,
long metalakeId) {
Review Comment:
This removes the old public one-argument method. Please confirm that
external catalog connectors do not use it. If it is part of the connector API,
please keep a compatible overload or use another way to pass the metalake ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
