tengqm commented on code in PR #6575:
URL: https://github.com/apache/gravitino/pull/6575#discussion_r1976539692
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -91,9 +94,42 @@ protected RangerAuthorizationPlugin(String metalake,
Map<String, String> config)
rangerAdminName =
config.get(RangerAuthorizationProperties.RANGER_USERNAME);
// Apache Ranger Password should be minimum 8 characters with min one
alphabet and one numeric.
String password =
config.get(RangerAuthorizationProperties.RANGER_PASSWORD);
- rangerServiceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
+
+ String serviceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
rangerClient = new RangerClientExtension(rangerUrl, authType,
rangerAdminName, password);
+ if (Boolean.parseBoolean(
+
config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {
Review Comment:
How can we be so sure that this property is there?
##########
authorizations/authorization-common/src/main/java/org/apache/gravitino/authorization/common/RangerAuthorizationProperties.java:
##########
@@ -76,15 +76,19 @@ public void validate() {
Preconditions.checkArgument(
properties.get(RANGER_ADMIN_URL) != null,
String.format("%s is required", RANGER_ADMIN_URL));
- Preconditions.checkArgument(
- properties.get(RANGER_SERVICE_NAME) != null,
- String.format("%s is required", RANGER_SERVICE_NAME));
Preconditions.checkArgument(
properties.get(RANGER_AUTH_TYPE) != null,
String.format("%s is required", RANGER_AUTH_TYPE));
Preconditions.checkArgument(
properties.get(RANGER_USERNAME) != null, String.format("%s is
required", RANGER_USERNAME));
Preconditions.checkArgument(
properties.get(RANGER_PASSWORD) != null, String.format("%s is
required", RANGER_PASSWORD));
+
+ if
(!Boolean.parseBoolean(properties.get(RANGER_SERVICE_CREATE_IF_ABSENT))) {
+ Preconditions.checkArgument(
+ properties.get(RANGER_SERVICE_NAME) != null,
+ String.format(
+ "%s is required if you don't create the absent ranger service",
RANGER_SERVICE_NAME));
Review Comment:
```suggestion
"%s is required unless %s is specified", RANGER_SERVICE_NAME,
RANGER_SERVICE_CREATE_IF_ABSENT));
```
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -679,4 +680,57 @@ public Boolean onMetadataUpdated(MetadataObjectChange...
changes) throws Runtime
}
return Boolean.TRUE;
}
+
+ @Override
+ protected String getServiceType() {
+ return "hdfs";
+ }
+
+ @Override
+ protected Map<String, String> getServiceConfigs(Map<String, String> config) {
+ String usernameKey = "username";
+ String usernameVal = "admin";
Review Comment:
Same here.
Make them some final string members of the class?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -91,9 +94,42 @@ protected RangerAuthorizationPlugin(String metalake,
Map<String, String> config)
rangerAdminName =
config.get(RangerAuthorizationProperties.RANGER_USERNAME);
// Apache Ranger Password should be minimum 8 characters with min one
alphabet and one numeric.
String password =
config.get(RangerAuthorizationProperties.RANGER_PASSWORD);
- rangerServiceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
+
+ String serviceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
rangerClient = new RangerClientExtension(rangerUrl, authType,
rangerAdminName, password);
+ if (Boolean.parseBoolean(
+
config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {
+ if (serviceName == null) {
+ serviceName = config.get(BaseAuthorization.UUID);
+ }
+
+ try {
+ rangerClient.getService(serviceName);
+ } catch (RangerServiceException rse) {
+ if (rse.getStatus().equals(ClientResponse.Status.NOT_FOUND)) {
Review Comment:
Are we sure the status is 404 rather than a 400?
I still remember that `getRole()` returns 400 if a role is not found.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -679,4 +680,57 @@ public Boolean onMetadataUpdated(MetadataObjectChange...
changes) throws Runtime
}
return Boolean.TRUE;
}
+
+ @Override
+ protected String getServiceType() {
+ return "hdfs";
Review Comment:
better not use hardcoded string here?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -935,8 +971,19 @@ protected void
removePolicyByMetadataObject(AuthorizationMetadataObject authzMet
}
}
+ protected abstract String getServiceType();
+
+ protected abstract Map<String, String> getServiceConfigs(Map<String, String>
config);
+
@Override
- public void close() throws IOException {}
+ public void close() throws IOException {
Review Comment:
Where is this method invoked?
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -91,9 +94,42 @@ protected RangerAuthorizationPlugin(String metalake,
Map<String, String> config)
rangerAdminName =
config.get(RangerAuthorizationProperties.RANGER_USERNAME);
// Apache Ranger Password should be minimum 8 characters with min one
alphabet and one numeric.
String password =
config.get(RangerAuthorizationProperties.RANGER_PASSWORD);
- rangerServiceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
+
+ String serviceName =
config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
rangerClient = new RangerClientExtension(rangerUrl, authType,
rangerAdminName, password);
+ if (Boolean.parseBoolean(
+
config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {
+ if (serviceName == null) {
+ serviceName = config.get(BaseAuthorization.UUID);
+ }
+
+ 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());
+ }
+ } catch (RangerServiceException crse) {
+ throw new AuthorizationPluginException(
+ "Fail to create ranger service %s, exception: %s",
serviceName, crse.getMessage());
+ }
Review Comment:
I'd vote for extracting this logic into a dedicated method.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -679,4 +680,57 @@ public Boolean onMetadataUpdated(MetadataObjectChange...
changes) throws Runtime
}
return Boolean.TRUE;
}
+
+ @Override
+ protected String getServiceType() {
+ return "hdfs";
+ }
+
+ @Override
+ protected Map<String, String> getServiceConfigs(Map<String, String> config) {
+ String usernameKey = "username";
+ String usernameVal = "admin";
+ if (config.containsKey(BaseCatalog.CATALOG_BYPASS_PREFIX + usernameKey)) {
+ usernameVal = config.get(BaseCatalog.CATALOG_BYPASS_PREFIX +
usernameKey);
+ }
+
+ String passwordKey = "password";
+ String passwordVal = "admin";
Review Comment:
This .... is dangerous?
--
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
