xunliu commented on code in PR #6575:
URL: https://github.com/apache/gravitino/pull/6575#discussion_r1978583886
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -745,6 +763,110 @@ public Boolean onGroupAcquired(Group group) {
return Boolean.TRUE;
}
+ @Override
+ public void close() throws IOException {
+ if (!isCreatedByPlugin) {
+ return;
+ }
+
+ try {
+ rangerClient.deleteService(rangerServiceName);
+ } catch (RangerServiceException rse) {
+ throw new AuthorizationPluginException(
+ "Fail to delete Ranger service %s, exception: %s",
rangerServiceName, rse.getMessage());
+ }
+ }
+
+ /** Generate authorization securable object */
+ public abstract AuthorizationSecurableObject
generateAuthorizationSecurableObject(
+ List<String> names,
+ String path,
+ AuthorizationMetadataObject.Type type,
+ Set<AuthorizationPrivilege> privileges);
+
+ public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
+ return securableObjects.stream()
+ .allMatch(
+ securableObject -> {
+ AtomicBoolean match = new AtomicBoolean(true);
+ securableObject.privileges().stream()
+ .forEach(
+ privilege -> {
+ if (!privilege.canBindTo(securableObject.type())) {
+ LOG.error(
+ "The privilege({}) is not supported for the
metadata object({})!",
+ privilege.name(),
+ securableObject.fullName());
+ match.set(false);
+ }
+ });
+ return match.get();
+ });
+ }
+
+ /**
+ * IF rename the SCHEMA, Need to rename these the relevant policies,
`{schema}`, `{schema}.*`,
+ * `{schema}.*.*` <br>
+ * IF rename the TABLE, Need to rename these the relevant policies,
`{schema}.*`, `{schema}.*.*`
+ * <br>
+ * IF rename the COLUMN, Only need to rename `{schema}.*.*` <br>
+ */
+ protected abstract void renameMetadataObject(
+ AuthorizationMetadataObject authzMetadataObject,
+ AuthorizationMetadataObject newAuthzMetadataObject);
+
+ protected abstract void removeMetadataObject(AuthorizationMetadataObject
authzMetadataObject);
+
+ /**
+ * Remove the policy by the metadata object names. <br>
+ *
+ * @param authzMetadataObject The authorization metadata object.
+ */
+ protected void removePolicyByMetadataObject(AuthorizationMetadataObject
authzMetadataObject) {
+ RangerPolicy policy = findManagedPolicy(authzMetadataObject);
+ if (policy != null) {
+ rangerHelper.removeAllGravitinoManagedPolicyItem(policy);
+ }
+ }
+
+ protected String getConfValue(Map<String, String> conf, String key, String
defaultValue) {
+ if (conf.containsKey(BaseCatalog.CATALOG_BYPASS_PREFIX + key)) {
+ return conf.get(BaseCatalog.CATALOG_BYPASS_PREFIX + key);
+ }
+ return defaultValue;
+ }
+
+ protected abstract String getServiceType();
+
+ protected abstract Map<String, String> getServiceConfigs(Map<String, String>
config);
+
+ private void createRangerServiceIfNecessary(Map<String, String> config,
String serviceName) {
+ try {
+ rangerClient.getService(serviceName);
+ } catch (RangerServiceException rse) {
+ if (rse.getStatus().equals(ClientResponse.Status.NOT_FOUND)) {
+ try {
+ RangerService rangerService = new RangerService();
+ rangerService.setType(getServiceType());
+ rangerService.setName(serviceName);
+ rangerService.setConfigs(getServiceConfigs(config));
+ rangerClient.createService(rangerService);
+ List<RangerPolicy> policies =
rangerClient.getPoliciesInService(serviceName);
+ for (RangerPolicy policy : policies) {
+ rangerClient.deletePolicy(policy.getId());
+ }
Review Comment:
Please add a description of why the policy should be deleted
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -909,59 +1031,4 @@ private void removePolicyItemIfEqualRoleName(
policyItem.getRoles().removeIf(roleName::equals);
}
}
-
- /**
- * IF rename the SCHEMA, Need to rename these the relevant policies,
`{schema}`, `{schema}.*`,
- * `{schema}.*.*` <br>
- * IF rename the TABLE, Need to rename these the relevant policies,
`{schema}.*`, `{schema}.*.*`
- * <br>
- * IF rename the COLUMN, Only need to rename `{schema}.*.*` <br>
- */
- protected abstract void renameMetadataObject(
- AuthorizationMetadataObject authzMetadataObject,
- AuthorizationMetadataObject newAuthzMetadataObject);
-
- protected abstract void removeMetadataObject(AuthorizationMetadataObject
authzMetadataObject);
-
- /**
- * Remove the policy by the metadata object names. <br>
- *
- * @param authzMetadataObject The authorization metadata object.
- */
- protected void removePolicyByMetadataObject(AuthorizationMetadataObject
authzMetadataObject) {
- RangerPolicy policy = findManagedPolicy(authzMetadataObject);
- if (policy != null) {
- rangerHelper.removeAllGravitinoManagedPolicyItem(policy);
- }
- }
-
- @Override
- public void close() throws IOException {}
-
- /** Generate authorization securable object */
- public abstract AuthorizationSecurableObject
generateAuthorizationSecurableObject(
- List<String> names,
- String path,
- AuthorizationMetadataObject.Type type,
- Set<AuthorizationPrivilege> privileges);
-
- public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
- return securableObjects.stream()
- .allMatch(
- securableObject -> {
- AtomicBoolean match = new AtomicBoolean(true);
- securableObject.privileges().stream()
- .forEach(
- privilege -> {
- if (!privilege.canBindTo(securableObject.type())) {
- LOG.error(
- "The privilege({}) is not supported for the
metadata object({})!",
- privilege.name(),
- securableObject.fullName());
- match.set(false);
- }
- });
- return match.get();
- });
- }
Review Comment:
Here it is because of the changes brought about by moving the code, which
makes it more difficult to review, and it is better to be able to fall back.
##########
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java:
##########
@@ -131,18 +131,17 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
return false;
}
- // If we call the authorization plugin after dropping catalog, we can't
load the plugin of the
- // catalog
Catalog catalog = dispatcher.loadCatalog(ident);
- boolean dropped = dispatcher.dropCatalog(ident, force);
- if (dropped && catalog != null) {
+ if (catalog != null) {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.CATALOG);
AuthorizationUtils.removeCatalogPrivileges(catalog, locations);
}
- return dropped;
+ // We should call the authorization plugin before dropping the catalog,
because the dropping
+ // catalog will close the authorization plugin.
+ return dispatcher.dropCatalog(ident, force);
Review Comment:
Why is the order of implementation being adjusted and what are the
implications?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -745,6 +763,110 @@ public Boolean onGroupAcquired(Group group) {
return Boolean.TRUE;
}
+ @Override
+ public void close() throws IOException {
+ if (!isCreatedByPlugin) {
+ return;
+ }
+
+ try {
+ rangerClient.deleteService(rangerServiceName);
+ } catch (RangerServiceException rse) {
+ throw new AuthorizationPluginException(
+ "Fail to delete Ranger service %s, exception: %s",
rangerServiceName, rse.getMessage());
+ }
+ }
+
+ /** Generate authorization securable object */
+ public abstract AuthorizationSecurableObject
generateAuthorizationSecurableObject(
+ List<String> names,
+ String path,
+ AuthorizationMetadataObject.Type type,
+ Set<AuthorizationPrivilege> privileges);
+
+ public boolean validAuthorizationOperation(List<SecurableObject>
securableObjects) {
+ return securableObjects.stream()
+ .allMatch(
+ securableObject -> {
+ AtomicBoolean match = new AtomicBoolean(true);
+ securableObject.privileges().stream()
+ .forEach(
+ privilege -> {
+ if (!privilege.canBindTo(securableObject.type())) {
+ LOG.error(
+ "The privilege({}) is not supported for the
metadata object({})!",
+ privilege.name(),
+ securableObject.fullName());
+ match.set(false);
+ }
+ });
+ return match.get();
+ });
+ }
+
+ /**
+ * IF rename the SCHEMA, Need to rename these the relevant policies,
`{schema}`, `{schema}.*`,
+ * `{schema}.*.*` <br>
+ * IF rename the TABLE, Need to rename these the relevant policies,
`{schema}.*`, `{schema}.*.*`
+ * <br>
+ * IF rename the COLUMN, Only need to rename `{schema}.*.*` <br>
+ */
+ protected abstract void renameMetadataObject(
+ AuthorizationMetadataObject authzMetadataObject,
+ AuthorizationMetadataObject newAuthzMetadataObject);
+
+ protected abstract void removeMetadataObject(AuthorizationMetadataObject
authzMetadataObject);
+
+ /**
+ * Remove the policy by the metadata object names. <br>
+ *
+ * @param authzMetadataObject The authorization metadata object.
+ */
+ protected void removePolicyByMetadataObject(AuthorizationMetadataObject
authzMetadataObject) {
+ RangerPolicy policy = findManagedPolicy(authzMetadataObject);
+ if (policy != null) {
+ rangerHelper.removeAllGravitinoManagedPolicyItem(policy);
+ }
+ }
Review Comment:
Here it is because of the changes brought about by moving the code, which
makes it more difficult to review, and it is better to be able to fall back.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -745,6 +763,110 @@ public Boolean onGroupAcquired(Group group) {
return Boolean.TRUE;
}
+ @Override
+ public void close() throws IOException {
+ if (!isCreatedByPlugin) {
+ return;
+ }
+
+ try {
+ rangerClient.deleteService(rangerServiceName);
+ } catch (RangerServiceException rse) {
+ throw new AuthorizationPluginException(
+ "Fail to delete Ranger service %s, exception: %s",
rangerServiceName, rse.getMessage());
+ }
Review Comment:
Why do we need the automated delete Reanger service?
Is the `RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT` only
used by ITs?
##########
authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerITEnv.java:
##########
@@ -156,11 +161,30 @@ public static void init(String metalakeName, boolean
allowAnyoneAccessHDFS) {
rangerAuthHDFSPlugin.ownerMappingRule(),
rangerAuthHDFSPlugin.policyResourceDefinesRule());
+ // Test to use UUID create authz plugin
+ new RangerAuthorizationHadoopSQLPlugin(
+ metalakeName,
+ ImmutableMap.of(
+ RangerAuthorizationProperties.RANGER_ADMIN_URL,
+ String.format(
+ "http://%s:%d";,
+ containerSuite.getRangerContainer().getContainerIpAddress(),
+ RangerContainer.RANGER_SERVER_PORT),
+ RangerAuthorizationProperties.RANGER_AUTH_TYPE,
+ RangerContainer.authType,
+ RangerAuthorizationProperties.RANGER_USERNAME,
+ RangerContainer.rangerUserName,
+ RangerAuthorizationProperties.RANGER_PASSWORD,
+ RangerContainer.rangerPassword,
+ BaseAuthorization.UUID,
+ "123",
+ RangerAuthorizationProperties.RANGER_SERVICE_TYPE,
+ "HadoopSQL",
+ RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT,
+ "true"));
Review Comment:
I think it would be better to add a check to see if Ranger service success
was created.
--
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: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
