[#6536] improvement(authz): Create Ranger service if service is absent #6575

jerqi · 2025-02-28T11:32:43Z

What changes were proposed in this pull request?

Create Ranger service if service is absent

Why are the changes needed?

Fix: #6536

Does this PR introduce any user-facing change?

Yes, I will add the document.

How was this patch tested?

Add a UT.

...n/src/main/java/org/apache/gravitino/authorization/common/RangerAuthorizationProperties.java

tengqm · 2025-03-02T04:33:25Z

...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java

+
+  @Override
+  protected String getServiceType() {
+    return "hdfs";


better not use hardcoded string here?

I will put these constants to one place to manage them

...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java

tengqm · 2025-03-02T04:35:09Z

...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java

+    }
+
+    String passwordKey = "password";
+    String passwordVal = "admin";


This .... is dangerous?

We could pass fake value. I can add comment to avoid others worrying about the code.

tengqm · 2025-03-02T04:37:15Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

    rangerClient = new RangerClientExtension(rangerUrl, authType, rangerAdminName, password);

+    if (Boolean.parseBoolean(
+        config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {


How can we be so sure that this property is there?

Boolean.parseBoolean can handle null value. Null value equals false.

Please make sure there won't be uncaught exceptions.

You can see the code

public static boolean parseBoolean(String s) { return ((s != null) && s.equalsIgnoreCase("true")); }

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

tengqm · 2025-03-02T04:41:03Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

  @Override
-  public void close() throws IOException {}
+  public void close() throws IOException {


Where is this method invoked?

When we dropCatalog, this method will be invoked.

tengqm · 2025-03-03T14:13:34Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

    rangerClient = new RangerClientExtension(rangerUrl, authType, rangerAdminName, password);

+    if (Boolean.parseBoolean(
+        config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {


Please make sure there won't be uncaught exceptions.

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

tengqm · 2025-03-03T14:16:29Z

core/src/main/java/org/apache/gravitino/connector/authorization/BaseAuthorization.java

@@ -45,6 +45,9 @@
 public abstract class BaseAuthorization<T extends BaseAuthorization>
    implements AuthorizationProvider, Closeable {

+  /** UUID represents */


Sorry, I will correct it.

tengqm · 2025-03-03T14:16:45Z

core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java

+    // catalog
+    // will close the authorization plugin.


Correct. Gradle format will cause this issue.

xunliu · 2025-03-04T04:22:35Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+          List<RangerPolicy> policies = rangerClient.getPoliciesInService(serviceName);
+          for (RangerPolicy policy : policies) {
+            rangerClient.deletePolicy(policy.getId());
+          }


Please add a description of why the policy should be deleted

OK. I can add the comment.

xunliu · 2025-03-04T04:26:06Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+    try {
+      rangerClient.deleteService(rangerServiceName);
+    } catch (RangerServiceException rse) {
+      throw new AuthorizationPluginException(
+          "Fail to delete Ranger service %s, exception: %s", rangerServiceName, rse.getMessage());
+    }


Why do we need the automated delete Reanger service?
Is the RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT only used by ITs?

No. It's not only used by UT.
If we have a Ranger server and Ranger HDFS or Hive services are not pre-created , we need the to create the by our Gravitino.
If these Ranger Hive services or HDFS services are created by Gravitino, Gravitino should delete them, too.

xunliu · 2025-03-04T04:28:32Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

-  /**
-   * 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();
-            });
-  }


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.

I can revert this change.

xunliu · 2025-03-04T04:29:04Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  /** 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);
+    }
+  }


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.

OK. I just put public methods together, protected methods together, private methods together. I can revert this change.

xunliu · 2025-03-04T04:33:09Z

...er/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerITEnv.java

+            RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT,
+            "true"));


I think it would be better to add a check to see if Ranger service success was created.

xunliu · 2025-03-04T04:35:57Z

core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java

-    // 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);


Why is the order of implementation being adjusted and what are the implications?

You can see the comment.

We should call the authorization plugin before dropping the catalog, because the dropping catalog will close the authorization plugi

xunliu · 2025-03-04T23:47:32Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+    if (conf.containsKey(BaseCatalog.CATALOG_BYPASS_PREFIX + key)) {
+      return conf.get(BaseCatalog.CATALOG_BYPASS_PREFIX + key);
+    }


Why do we need BaseCatalog.CATALOG_BYPASS_PREFIX?

Because properties like hadoop.security.xxx should use PYPASS_PREFIX. It's the common behaviour in our system.

Maybe getByPassConfValue() is a better function name?

xunliu · 2025-03-04T23:49:21Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+            "true");
+
+    metalake.createCatalog("test", Catalog.Type.RELATIONAL, provider, "comment", uuidProperties);
+


Do we need to add check catalog logic after createCatalog()?

Added a new assertion.

xunliu · 2025-03-05T09:25:04Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    Map<String, String> uuidProperties =
+        ImmutableMap.of(
+            HiveConstants.METASTORE_URIS,
+            HIVE_METASTORE_URIS,
+            IMPERSONATION_ENABLE,
+            "true",
+            AUTHORIZATION_PROVIDER,
+            "ranger",
+            RangerAuthorizationProperties.RANGER_SERVICE_TYPE,
+            "HadoopSQL",
+            RangerAuthorizationProperties.RANGER_ADMIN_URL,
+            RangerITEnv.RANGER_ADMIN_URL,
+            RangerAuthorizationProperties.RANGER_AUTH_TYPE,
+            RangerContainer.authType,
+            RangerAuthorizationProperties.RANGER_USERNAME,
+            RangerContainer.rangerUserName,
+            RangerAuthorizationProperties.RANGER_PASSWORD,
+            RangerContainer.rangerPassword,
+            RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT,
+            "true");


Do we need to set a BaseAuthorization.UUID in the uuidProperties?

No need. The uuid will be generated automatically.

xunliu · 2025-03-05T09:26:20Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+    if (conf.containsKey(BaseCatalog.CATALOG_BYPASS_PREFIX + key)) {
+      return conf.get(BaseCatalog.CATALOG_BYPASS_PREFIX + key);
+    }


Maybe getByPassConfValue() is a better function name?

jerqi · 2025-03-05T09:42:31Z

I renamed the method.

xunliu · 2025-03-06T07:31:11Z

...chain/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java

+              for (Map.Entry<String, String> entry : properties.entrySet()) {
+                if (!entry
+                    .getKey()
+                    .startsWith(ChainedAuthorizationProperties.CHAIN_PLUGINS_PROPERTIES_KEY)) {
+                  pluginConfig.put(entry.getKey(), entry.getValue());
+                }
+              }
+              pluginConfig.put(
+                  BaseAuthorization.UUID, pluginConfig.get(BaseAuthorization.UUID) + pluginName);
+


If you went pass hadoop.security.authentication properties to the plugin, you can use authorization.chain.hdfs-xxx.hadoop.security.authentication.
Another thing is, I found you added some new properties key in the ranger-hive-plugin and ranger-hdfs-plugin, please update relation doc. thanks.

xunliu · 2025-03-06T11:32:07Z

...va/org/apache/gravitino/authorization/chain/integration/test/TestChainedAuthorizationIT.java

+    Catalog catalogTest =
+        metalake.createCatalog("test", Catalog.Type.RELATIONAL, "hive", "comment", autoProperties);
+    Map<String, String> newProperties = catalogTest.properties();
+    Assertions.assertTrue(
+        newProperties.containsKey("authorization.chain.hdfs1.ranger.service.name"));
+    Assertions.assertTrue(
+        newProperties.containsKey("authorization.chain.hive1.ranger.service.name"));


I think you need to check if test833 and test899 are in the Ranger service repo.

xunliu · 2025-03-06T11:43:14Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  protected abstract Map<String, String> getServiceConfigs(Map<String, String> config);
+
+  protected int getPrefixLength() {
+    return prefixLength;


I think we can remove prefixLength variable.

protected int getPrefixLength() { String.format("%s.", rangerAuthorizationProperties.getPropertiesPrefix()).length(); }

rangerAuthorizationProperties isn't field variable. We need to add the field variable rangerAuthorizationProperities. Isn't ok?

xunliu

LGTM

[apache#6536] improvement: Create Ranger service if service is absent

3cdb335

jerqi marked this pull request as draft February 28, 2025 11:44

tengqm reviewed Mar 2, 2025

View reviewed changes

jerqi added 2 commits March 3, 2025 14:43

fix ut

5b018ce

address comment

52444df

jerqi self-assigned this Mar 3, 2025

jerqi marked this pull request as ready for review March 3, 2025 12:43

tengqm reviewed Mar 3, 2025

View reviewed changes

jerqi added 2 commits March 4, 2025 10:51

Address comment

9f0743f

fix ut

74b88fd

jerqi changed the title ~~[#6536] improvement: Create Ranger service if service is absent~~ [#6536] improvement(authz): Create Ranger service if service is absent Mar 4, 2025

xunliu reviewed Mar 4, 2025

View reviewed changes

jerqi added 2 commits March 4, 2025 14:48

add comment and revert change

d60cc87

remove the logic of deleting service

69e34b2

xunliu reviewed Mar 4, 2025

View reviewed changes

jerqi added 2 commits March 5, 2025 11:29

Add a new assert

69e261f

Merge branch 'main' into ISSUE-6536

384e6e4

xunliu reviewed Mar 5, 2025

View reviewed changes

rename method

531ec52

jerqi and others added 4 commits March 5, 2025 21:48

optimize the usage

2fb0d96

add ut

83a5f42

fix ut

2fdc026

fix ut

bef8a8f

xunliu reviewed Mar 6, 2025

View reviewed changes

Address comment

2793f78

xunliu reviewed Mar 6, 2025

View reviewed changes

jerqi added 2 commits March 6, 2025 20:37

address comments

2b07cef

address comment

bdbca22

xunliu approved these changes Mar 7, 2025

View reviewed changes

jerqi merged commit 1297713 into apache:main Mar 7, 2025
28 checks passed

jerqi deleted the ISSUE-6536 branch March 7, 2025 09:14

		RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT,
		"true"));

		"true");

		metalake.createCatalog("test", Catalog.Type.RELATIONAL, provider, "comment", uuidProperties);

[#6536] improvement(authz): Create Ranger service if service is absent #6575

[#6536] improvement(authz): Create Ranger service if service is absent #6575

Conversation

jerqi commented Feb 28, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

xunliu left a comment

Choose a reason for hiding this comment

jerqi Mar 6, 2025 •

edited

Loading