[#5775] feat(auth): Chain authorization plugin framework #5786

xunliu · 2024-12-06T07:30:13Z

What changes were proposed in this pull request?

Add Chain auth plugin module
Add auth common module
Add Chain authorization Ranger Hive and Ranger HDFS ITs

Why are the changes needed?

Fix: #5775

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Add ITs

jerqi · 2024-12-23T13:10:06Z

...n-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorizationPlugin.java

+                  ChainAuthorizationProperties.fetchAuthPluginProperties(pluginName, properties);
+              String authProvider =
+                  ChainAuthorizationProperties.getPluginProvider(pluginName, properties);
+              if ("ranger".equals(authProvider)) {


Do we only support Ranger plugin?

jerqi · 2024-12-23T13:10:49Z

...ization-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorization.java

  }

  @Override
  public AuthorizationPlugin newPlugin(
      String metalake, String catalogProvider, Map<String, String> config) {
-    return new TestMySQLAuthorizationPlugin();
+    switch (catalogProvider) {


Do we only support hive?

Currently, we only support the Hive catalog in this PR, But we can support more types in the future.
We need rigorous testing to enable this limit.

jerqi · 2024-12-24T06:13:33Z

...n-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorizationPlugin.java

+                  ChainAuthorizationProperties.fetchAuthPluginProperties(pluginName, properties);
+              String authProvider =
+                  ChainAuthorizationProperties.getPluginProvider(pluginName, properties);
+              if ("ranger".equals(authProvider)) {


Maybe we can have a common module to define the constant variable like Ranger.

The community discussed it before, just like Catalog shortName uses string variables (Hive, Hadoop), not uses constant variable, So I kept consistent.

jerqi · 2024-12-24T06:17:04Z

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

+    switch (metadataObject.type()) {
+      case METALAKE:
+      case SCHEMA:
+      case TABLE:


Is this right? Table can have a location, too.

This PR only supports Create_schema in the Catalog in the chain Ranger Hive and Ranger HDFS. I split other operations in the next PR.

Maybe you add TODO comment at least.

I already add todo integration test in the TestChainedAuthorizationIT.

We need add TODO in the code. Otherwise I will think this is an error.

jerqi · 2024-12-24T06:18:00Z

...thorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java

  public static final String RESOURCE_ALL = "*";
+  /** The `/` gives access to all path resources */
+  public static final String RESOURCE_ROOT_PATH = "/test/";


Why dow we need this?

rollback this change.

jerqi · 2024-12-24T06:22:11Z

integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java

+    } catch (Exception e) {
+      throw new IllegalStateException("Failed to set environment variable", e);
+    } finally {
+      setEnv(key, originalValue);


Do we need recover the property user.name?

OK, I improved this code.

jerqi · 2024-12-24T06:26:17Z

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

+                          gravitinoPrivilege.name(), securableObject.type());
+                  }
+                  break;
+                case SELECT_TABLE:


If HDFS Ranger plugin is used for Hive, we should have the SELECT_TABLE privilege.

I split SELECT_TABLE operations in the next PR.

Maybe you add TODO comment at least.

I already add todo integration test in the TestChainedAuthorizationIT.

jerryshao · 2024-12-24T08:52:54Z

authorizations/authorization-chain/build.gradle.kts

+  implementation(libs.javax.ws.rs.api)
+  implementation(libs.jettison)
+  compileOnly(libs.lombok)
+  implementation(libs.rome)


Can you make this alphabetically ordering?

jerryshao · 2024-12-24T08:55:47Z

...n-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorizationPlugin.java

+  public Boolean onMetadataUpdated(MetadataObjectChange... changes)
+      throws AuthorizationPluginException {
+    for (AuthorizationPlugin plugin : plugins) {
+      Boolean result = plugin.onMetadataUpdated(changes);


Will we throw exceptions here, what happens if the exception throws in the for loop?

In such a for loop, if the plugin calls the onMetadataUpdated function and throws an exception, the exception will propagate to the outer method ChainAuthorizaiton::onMetadataUpdated and will not be caught or handled. This means that the outer method also sell AuthorizationPluginException abnormalities, and the cycle will be interrupted, will not continue to deal with the rest of the plugin.

jerryshao · 2024-12-24T09:02:18Z

...n-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorizationPlugin.java

+                      ImmutableMap.of(Catalog.AUTHORIZATION_PROVIDER, authProvider))
+                  .ifPresent(libAndResourcesPaths::add);
+              IsolatedClassLoader classLoader =
+                  IsolatedClassLoader.buildClassLoader(libAndResourcesPaths);


Why do we need to build different classloaders for different plugins, can we just use catalog's classloader?

When we load ChainAuthorizationPlugin , We need add authorizaitons/chain/libs in the BaseCatalog IsolatedClassLoader class paths.
We cann't add authorizaitons/ranger/libs at same time. So we need an IsolatedClassLoader to separate load RangerAuthorizationPlugin here.

jerryshao · 2024-12-24T11:27:46Z

...n-chain/src/main/java/org/apache/gravitino/authorization/chain/ChainAuthorizationPlugin.java

+    }
+    return Boolean.TRUE;
+  }
+}


I think you can extract the common part and simplify the code like:

private Boolean chainedAction(Function<AuthorizationPlugin, Boolean> action) { for (AuthorizationPlugin plugin : plugins) { if (!action.apply(plugin)) { return false; } } return true; }

jerryshao · 2024-12-24T11:31:01Z

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

@@ -25,12 +25,12 @@
 import org.apache.gravitino.authorization.AuthorizationPrivilege;
 import org.apache.gravitino.authorization.AuthorizationSecurableObject;

-public class RangerPathBaseSecurableObject extends RangerPathBaseMetadataObject
+public class RangerHDFSSecurableObject extends RangerHDFSMetadataObject


Why do we change the class name here? I assume Ranger doesn't care whether it is the HDFS or not, it cares more about the path, am I right?

So if users have a Hadoop compatible file system, it can also be controlled by Ranger, but it is not a HDFS actually.

OK, I rollback this change.

jerryshao · 2024-12-24T11:35:56Z

...ization-common/src/main/java/org/apache/gravitino/authorization/AuthorizationProperties.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.authorization;


How about renaming the package to org.apache.gravitino.authorization.common for better understanding?

xunliu · 2024-12-24T14:31:35Z

@jerryshao I fixed all the problems, Please help me review it again. Thanks.

jerryshao · 2024-12-25T02:50:36Z

authorizations/authorization-chain/build.gradle.kts

+  testImplementation(project(":core"))
+  testImplementation(project(":clients:client-java"))
+  testImplementation(project(":server"))
+  testImplementation(project(":catalogs:catalog-common"))
+  testImplementation(project(":integration-test-common", "testArtifacts"))
+  testImplementation(project(":authorizations:authorization-ranger"))
+  testImplementation(project(":authorizations:authorization-ranger", "testArtifacts"))
+  testImplementation(libs.junit.jupiter.api)
+  testImplementation(libs.mockito.core)
+  testImplementation(libs.testcontainers)
+  testRuntimeOnly(libs.junit.jupiter.engine)
+  testImplementation(libs.mysql.driver)
+  testImplementation(libs.postgresql.driver)
+  testImplementation(libs.ranger.intg) {


Looks like we still have some alphabetical ordering issues, can you fix them all?

jerryshao · 2024-12-25T02:58:00Z

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

+import org.apache.gravitino.exceptions.AuthorizationPluginException;
+import org.apache.gravitino.utils.IsolatedClassLoader;
+
+/** Chain authorization operations plugin class. <br> */


Chained authorization operations...

jerryshao · 2024-12-25T02:59:24Z

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

+  }
+
+  private void initPlugins(String catalogProvider, Map<String, String> properties) {
+    ChainedAuthorizationProperties chainedAuthProperties =


"Authz" is short for "Authorization", it would be better change to "Authz".

Just minor suggestion, I'm OK with either.

jerryshao · 2024-12-25T03:08:59Z

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

+                      {
+                        String locationPath = getLocationPath(securableObject);
+                        if (locationPath != null && !locationPath.isEmpty()) {
+                          RangerPathBaseMetadataObject rangerHDFSMetadataObject =


Can you also rename the variable to "rangerPathXXX", not "rangerHDFSXXX"?

jerryshao · 2024-12-25T03:09:18Z

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

-      return rangerHDFSMetadataObject;
-    } else {
-      return new RangerPathBaseMetadataObject("", RangerPathBaseMetadataObject.Type.PATH);
+    RangerPathBaseMetadataObject rangerHDFSMetadataObject;


xunliu · 2024-12-25T04:52:25Z

@jerryshao I fixed all the problems, Please help me review it again. Thanks.

jerryshao · 2024-12-25T05:55:57Z

authorizations/authorization-common/build.gradle.kts

+tasks {
+  val runtimeJars by registering(Copy::class) {
+    from(configurations.runtimeClasspath)
+    into("build/libs")
+  }
+
+  jar {
+    dependsOn(runtimeJars)
+  }
+}


I guess this is not needed, am I right?

Ok, I removed it.

jerryshao · 2024-12-25T05:58:56Z

authorizations/build.gradle.kts

+  test {
+    subprojects.forEach {
+      dependsOn(":${project.name}:${it.name}:test")
+    }
+  }
+
+  register("copyLibAndConfig", Copy::class) {
+    subprojects.forEach {
+      if (!it.name.startsWith("authorization-common")) {
+        dependsOn(":${project.name}:${it.name}:copyLibAndConfig")
+      }
+    }
+  }
+}


Do we need these codes? I found that each module already has these tasks.

We can use it to optimize compileDistribution in the root build.gradle.kts.

jerryshao · 2024-12-25T06:00:53Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/RangerPathBaseMetadataObject.java

-      }
-      throw new IllegalArgumentException(
-          "No matching RangerMetadataObject.Type for " + metadataType);
-    }
  }

  private final String path;


Can we rename this class from RangerPathBaseMetadataObject to RangerPathBasedMetadataObject or RangerPathMetadataObject?

OK, I rename RangerPathBaseMetadataObject to PathBasedMetadataObject and moved it to authorization-common modul.

xunliu requested review from yuqi1129 and mchades December 6, 2024 07:30

xunliu self-assigned this Dec 6, 2024

xunliu requested a review from jerqi December 6, 2024 07:30

xunliu marked this pull request as draft December 7, 2024 04:43

xunliu mentioned this pull request Dec 16, 2024

[#5790] auth(chain): Chain authorization properties #5791

Merged

xunliu force-pushed the issue-5775 branch 2 times, most recently from 4b5dc53 to 4755a53 Compare December 23, 2024 03:24

xunliu changed the title ~~[#5775] feat(auth): Chain authorization plugin~~ [#5775] feat(auth): Chain authorization plugin framework Dec 23, 2024

xunliu marked this pull request as ready for review December 23, 2024 03:26

xunliu force-pushed the issue-5775 branch 4 times, most recently from 5a29c05 to a19a314 Compare December 23, 2024 10:37

jerqi reviewed Dec 23, 2024

View reviewed changes

jerqi reviewed Dec 24, 2024

View reviewed changes

xunliu added 4 commits December 24, 2024 16:27

[apache#5775] feat(auth): Chain authorization plugin

fe7b35b

address comments

35aba1f

Add license head

f5a7933

refactor JdbcProperties.java

b52e9e0

xunliu force-pushed the issue-5775 branch from 5e188c5 to b52e9e0 Compare December 24, 2024 08:38

jerryshao reviewed Dec 24, 2024

View reviewed changes

address comments

4a57e7d

jerryshao reviewed Dec 24, 2024

View reviewed changes

rename to chained

535bcd6

jerryshao reviewed Dec 25, 2024

View reviewed changes

xunliu added 2 commits December 25, 2024 11:43

address comments

d2944ca

improve code

5010cdb

jerryshao reviewed Dec 25, 2024

View reviewed changes

improve

c7d60b0

jerryshao approved these changes Dec 25, 2024

View reviewed changes

jerryshao merged commit 3e7e550 into apache:main Dec 25, 2024
26 checks passed

[#5775] feat(auth): Chain authorization plugin framework #5786

[#5775] feat(auth): Chain authorization plugin framework #5786

Conversation

xunliu commented Dec 6, 2024 • edited Loading

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

xunliu commented Dec 24, 2024

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

xunliu commented Dec 25, 2024

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

xunliu commented Dec 6, 2024 •

edited

Loading