[#4662] improve(IT): Add ranger authorization Hive E2E test #4651

xunliu · 2024-08-23T06:58:11Z

What changes were proposed in this pull request?

Use an isolated class loader to create an authorization Ranger plugin.
Add authorization for Hive E2E tests using Ranger via Gravitino RESTful access control.
Copy the authorization ranger jar to the distribution/package/authorizations/ranger/libs.
Add Apache Ranger to license.bin

Why are the changes needed?

#4662

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

CI

LICENSE.bin

yuqi1129 · 2024-08-25T12:53:46Z

.github/workflows/authorization-integration-test.yml

@@ -0,0 +1,110 @@
+name: Authorization Integration Test


Can you put authorization tests to the backend tests? I'm afraid the CI pipeline is already too large.

I think the authorization module is very important, and testing depends on many Docker containers (hive, ranger, MySQL, IAM, ...) , So I think better to split to a single CI pipeline.
Don't worry, I also disable the authorization IT test in the backend tests.

Could we reduce the number of the pipeline? The yaml will increase 6 pipelines.

Should the Authorization Integration Test include authentication tests? There are some AuthenticationIT in catalogIT

@jerqi Please create an issue to track and fix this problem. Thanks.

It's hard to test them with a single pipeline. They don't use a standalone module.

authorizations/authorization-ranger/build.gradle.kts

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

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java

core/src/main/java/org/apache/gravitino/utils/IsolatedClassLoader.java

yuqi1129 · 2024-08-25T13:13:45Z

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java

    String authorizationProvider =
        catalogPropertiesMetadata().containsProperty(AUTHORIZATION_PROVIDER)
            ? (String) catalogPropertiesMetadata().getOrDefault(conf, AUTHORIZATION_PROVIDER)
            : null;

    if (authorizationProvider == null) {
      LOG.info("Authorization provider is not set!");
-      return null;
+      return;


Do we only add logs if the authorization provider is not set?

Yes, the Authorization plugin is an option in the Catalog. It may or may not be configured.

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java

jerryshao · 2024-08-26T03:08:12Z

.github/workflows/authorization-integration-test.yml

+        # Integration test for AMD64 architecture
+        architecture: [linux/amd64]
+        java-version: [ 8, 11, 17 ]
+        test-mode: [ embedded, deploy ]


I'm afraid there are so many pipelines that ASF will not allow us to run so many runners. I would suggest we don't split them so many, maybe one "java-version" is enough, what do you think?

OK, If only one, I hope to use high version JDK to test.

Reduce to only use JDK-17 to test, Currently we only have two CI pipelines.

jerryshao · 2024-08-26T03:09:32Z

.github/workflows/authorization-integration-test.yml

+              - build.gradle.kts
+              - gradle.properties
+              - gradlew
+              - setting.gradle.kts


I think maybe only the code changes in catalog, core, and some other places is enough to trigger this test, some parts like client, web, etc maybe unrelated.

jerryshao · 2024-08-26T03:29:49Z

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

-    } else {
-      // In real environment, the catalog package is under the catalog directory.
-      pkgPath = String.join(File.separator, gravitinoHome, "catalogs", provider, "libs");
+      return String.join(File.separator, pkg, "libs");


How do we handle this case if Catalog.PROPERTY_PACKAGE is set? From the current code, if Catalog.PROPERTY_PACKAGE is set, then we'll directly return without handling authorization related packages.

I refactor these codes.

jerryshao · 2024-08-26T03:32:27Z

gradle/libs.versions.toml

@@ -79,6 +79,7 @@ flink = "1.18.0"
 cglib = "2.2"
 ranger = "2.4.0"
 javax-jaxb-api = "2.3.1"
+javax-ws-rs-api = "2.1.1"


Why do we need to add this library? I don't find there's a place using it.

I use it implementation(libs.javax.ws.rs.api) in the authorizations/authorization-ranger/build.gradle.kts:L56
Because JDK-17 removed Javax library internally, So we need to manually add it.

xunliu · 2024-08-26T05:17:48Z

@jerryshao Please help me review this PR, thanks.

jerryshao · 2024-08-26T06:33:47Z

...bc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java

+        ImmutableMap.<String, PropertyEntry<?>>builder()
+            .putAll(BASIC_CATALOG_PROPERTY_ENTRIES)
+            .putAll(Maps.uniqueIndex(propertyEntries, PropertyEntry::getName))
+            .build();


@yuqi1129 can you please check this part?

This part is fine with me.

jerryshao · 2024-08-26T06:34:08Z

...g-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogPropertiesMetadata.java

+                  false /* immutable */,
+                  false /* hidden */))
+          .putAll(BASIC_CATALOG_PROPERTY_ENTRIES)
+          .build();


@mchades can you please check this part?

This part LGTM

mchades

LGTM

xunliu added the branch-0.6 label Aug 23, 2024

xunliu requested review from yuqi1129 and mchades August 23, 2024 06:58

xunliu self-assigned this Aug 23, 2024

xunliu changed the title ~~[MINOR]: fix authorization ranger compile lib path~~ [MINOR]: fix authorization ranger jar conflict Aug 23, 2024

xunliu marked this pull request as draft August 23, 2024 09:44

jerqi reviewed Aug 23, 2024

View reviewed changes

LICENSE.bin Show resolved Hide resolved

[apache#4662] imporove(IT): Add ranger authorization Hive E2E test

323287c

xunliu force-pushed the minor-ranger-lib branch from d666855 to 323287c Compare August 23, 2024 14:28

xunliu changed the title ~~[MINOR]: fix authorization ranger jar conflict~~ [#4662] improve(IT): Add ranger authorization Hive E2E test Aug 23, 2024

Update notice.bin

09a1712

xunliu marked this pull request as ready for review August 23, 2024 14:33

xunliu force-pushed the minor-ranger-lib branch 2 times, most recently from eda989c to 974f71d Compare August 24, 2024 01:44

add CI yaml

d4641e2

xunliu force-pushed the minor-ranger-lib branch from 974f71d to d4641e2 Compare August 24, 2024 02:45

xunliu and others added 4 commits August 24, 2024 11:28

trigger HiveCatalog runtimeJar

19772ac

Fix jdbc driver problem.

2b3f503

use isolated class loader init auth plugin

fd24ddd

fix UT

f765ee5

yuqi1129 reviewed Aug 25, 2024

View reviewed changes

fix

d0c6ca0

FANNG1 mentioned this pull request Aug 26, 2024

[Bug report] Start the Gravitino Server failed in gravitino container due to java.lang.IllegalArgumentException: null (Daily pipeline 20240822) #4659

Closed

yuqi1129 reviewed Aug 26, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java Outdated Show resolved Hide resolved

jerryshao reviewed Aug 26, 2024

View reviewed changes

xunliu added 2 commits August 26, 2024 12:08

fix

7a94f17

fix

20a3df7

mchades mentioned this pull request Aug 26, 2024

[Bug report] CI: test containers initialization error on Cron Integration Test 20240822 #4640

Closed

Slightly improve the code

9dd3c5a

Fix the IT failure

1352ca2

jerryshao reviewed Aug 26, 2024

View reviewed changes

jerqi approved these changes Aug 26, 2024

View reviewed changes

mchades approved these changes Aug 26, 2024

View reviewed changes

jerryshao merged commit 3795285 into apache:main Aug 26, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4662] improve(IT): Add ranger authorization Hive E2E test #4651

[#4662] improve(IT): Add ranger authorization Hive E2E test #4651

xunliu commented Aug 23, 2024 •

edited

Loading

yuqi1129 Aug 25, 2024

xunliu Aug 26, 2024

jerqi Aug 26, 2024

mchades Aug 26, 2024

xunliu Aug 26, 2024

jerqi Aug 26, 2024

yuqi1129 Aug 25, 2024

xunliu Aug 26, 2024

jerryshao Aug 26, 2024

xunliu Aug 26, 2024

xunliu Aug 26, 2024

jerryshao Aug 26, 2024

xunliu Aug 26, 2024

jerryshao Aug 26, 2024

xunliu Aug 26, 2024

jerryshao Aug 26, 2024

xunliu Aug 26, 2024

xunliu commented Aug 26, 2024

jerryshao Aug 26, 2024

yuqi1129 Aug 26, 2024

yuqi1129 Aug 26, 2024

jerryshao Aug 26, 2024

mchades Aug 26, 2024

mchades left a comment

[#4662] improve(IT): Add ranger authorization Hive E2E test #4651

[#4662] improve(IT): Add ranger authorization Hive E2E test #4651

Conversation

xunliu commented Aug 23, 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

xunliu commented Aug 26, 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

mchades left a comment

Choose a reason for hiding this comment

xunliu commented Aug 23, 2024 •

edited

Loading