jerryshao commented on code in PR #13569: URL: https://github.com/apache/gravitino/pull/13569#discussion_r4280330812
########## iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.iceberg.integration.test; + +import com.google.common.collect.ImmutableList; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.UUID; +import org.apache.gravitino.authorization.Privilege; +import org.apache.gravitino.authorization.Privileges; +import org.apache.gravitino.authorization.SecurableObject; +import org.apache.gravitino.authorization.SecurableObjects; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Integration test for Spark authorization through the Iceberg REST service loaded from the + * experimental classpath. + * + * <p>The inherited tests exercise table creation, loading, updates, deletion, listing, renaming, + * nested namespaces, and table privilege enforcement against the experimental Iceberg artifacts. + */ +@Tag("gravitino-docker-test") +public class IcebergExperimentalAuthorizationIT extends IcebergTableAuthorizationIT { + + private static final String EXPERIMENTAL_SERVER_DIRECTORY = "iceberg-rest-experimental-server"; + + @BeforeAll + @Override + public void startIntegrationTest() throws Exception { + String rootDirectory = System.getenv("GRAVITINO_ROOT_DIR"); + String experimentalServerHome = + Paths.get(rootDirectory, "iceberg", EXPERIMENTAL_SERVER_DIRECTORY).toString(); + customConfigs.put( + "gravitino.iceberg-rest.classpath", + String.join( + ",", + Paths.get(experimentalServerHome, "build", "libs").toString(), + Paths.get(experimentalServerHome, "build", "dependencies").toString())); Review Comment: [Important] In embedded test mode this classpath override is inert, and the suite still passes -- so it reports green while exercising stable Iceberg 1.11.0, not the experimental artifacts this class exists to cover. The override itself does reach the server config (`MiniGravitino.customizeConfigFile` applies `context.customConfig` after `getIcebergRestServiceConfigs`, `integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java:341-343`), but in embedded mode the server runs inside the test JVM, and `IsolatedClassLoader` treats every class that is not one of the listed `org.apache.gravitino.catalog.*` prefixes as *shared*: `isSharedClass` returns `!isCatalogClass(name)` (`core/src/main/java/org/apache/gravitino/utils/IsolatedClassLoader.java:268-273`, `283-300`), so `org.apache.iceberg.*` and `org.apache.gravitino.iceberg.*` are resolved from the base classloader first (line 242) and the configured directory is only consulted on `ClassNotFoundException`. The test JVM always has stable Iceberg on it -- `iceberg/iceberg-rest-server/build.gradle.kts:91` adds `org.apache.iceberg:iceberg-spark-runtime-3.4_2.12:1.11.0`, an unrelocated fat jar containing `org.apach e.iceberg.*` -- so nothing ever falls through. Even the isolated branch would not help: the `CustomURLClassLoader` parent is the same context classloader (lines 256-258), so delegation is parent-first. Embedded is the default mode (`build.gradle.kts:478`) and one of the two CI matrix values for `:iceberg:iceberg-rest-server:test` (`.github/workflows/backend-integration-test.yml:64`, `backend-integration-test-action.yml:69`), so half the CI runs of this class are silently testing the stable jars. Suggest making the fallback detectable rather than invisible: assert positively that the server is running the experimental distribution (e.g. check the Iceberg build version the aux service reports, or a behaviour only the read-restrictions fork provides) so the test fails instead of passing when isolation does not happen; and/or skip the class with `Assumptions.assumeTrue` outside deploy mode so it does not advertise coverage it cannot deliver. Verified by: reading `IsolatedClassLoader` (lines 230-258, 268-300) and `MiniGravitino` in full on this checkout, confirming `iceberg-spark-runtime` is on the module's test classpath, and confirming the embedded default and the CI matrix in the workflow files. ########## iceberg/iceberg-rest-experimental-server/build.gradle.kts: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +description = "iceberg-rest-experimental-server" + +plugins { + `maven-publish` + id("java") + id("idea") +} + +val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get() + +configurations.configureEach { + resolutionStrategy.eachDependency { + if ( + requested.name.startsWith("iceberg-") && + (requested.group == "org.apache.iceberg" || requested.group == "com.datastrato") + ) { + useTarget("com.datastrato:experimental-${requested.name}:$datastratoIcebergVersion") + because("Use the Datastrato Iceberg distribution in the experimental auxiliary service") + } Review Comment: [Important] The substitution brings the fork's own dependency versions with it, so this directory will ship third-party jars that are newer than the ones Gravitino pins -- including a major-version bump of Caffeine. I fetched `com.datastrato:experimental-iceberg-core:1.13.0`'s published POM from Maven Central: it imports `jackson-bom:2.22.2` and declares `jackson-core`/`jackson-databind` at 2.22.2, `com.github.ben-manes.caffeine:caffeine:3.2.4`, `avro:1.12.2`, `httpclient5:5.6.4` and `RoaringBitmap:1.6.23`. Gravitino pins `jackson = "2.15.2"` and `caffeine = "2.9.3"` (`gradle/libs.versions.toml:26`, `:69`), and `iceberg-rest-server` depends on `libs.caffeine` and the jackson libraries directly (`iceberg/iceberg-rest-server/build.gradle.kts:58` and `:67-70`). Gradle's default conflict resolution picks the highest version, so `copyDepends` will place Caffeine 3.2.4 and Jackson 2.22.2 into `iceberg-rest-experimental-server/libs` alongside `iceberg-rest-server` classes compiled against 2.9.3/2.15.2. Caffeine 2 to 3 is a deliberately binary-incompatible major release. Today this is masked at runtime, because `com.github.benmanes.*` and `com.fasterxml.*` are "shared" classes resolved from the server's own `libs` (`core/src/main/java/org/apache/gravitino/utils/IsolatedClassLoader.java:242`, `268-273`) -- i.e. the mismatched jars sitting in this directory are simply ignored. That is the same fragile coupling as the IT comment above, and it means the packaged set is not what it looks like. Suggest either forcing the catalog's versions for the non-Iceberg coordinates in this `resolutionStrategy` block, or documenting the intended resolved set and checking it (the PR description mentions verifying the packaged Iceberg jars -- the same check over jackson/caffeine would catch this). Verified by: `curl` of `https://repo1.maven.org/maven2/com/datastrato/experimental-iceberg-core/1.13.0/experimental-iceberg-core-1.13.0.pom` in this run, cross-read against `gradle/libs.versions.toml` and `iceberg/iceberg-rest-server/build.gradle.kts` on this checkout. ########## iceberg/iceberg-rest-experimental-server/build.gradle.kts: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +description = "iceberg-rest-experimental-server" + +plugins { + `maven-publish` + id("java") + id("idea") +} + +val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get() + +configurations.configureEach { + resolutionStrategy.eachDependency { + if ( + requested.name.startsWith("iceberg-") && + (requested.group == "org.apache.iceberg" || requested.group == "com.datastrato") + ) { + useTarget("com.datastrato:experimental-${requested.name}:$datastratoIcebergVersion") + because("Use the Datastrato Iceberg distribution in the experimental auxiliary service") + } + } +} + +dependencies { + implementation(project(":common")) { + exclude("*") + } + implementation(project(":core")) { + exclude("*") + } + implementation(project(":iceberg:iceberg-common")) { + exclude("*") + } Review Comment: [Nit] These three `exclude("*")` project dependencies look like no-ops. The module has no `src` directory, so nothing is compiled against them, and line 51's `implementation(project(":iceberg:iceberg-rest-server"))` already brings `:common`, `:core` and `:iceberg:iceberg-common` onto the runtime classpath *with* their own dependencies (exclusions declared on one dependency path do not apply to another). `compileJava.dependsOn(":iceberg:iceberg-rest-server:copyDepends")` on lines 62-64 is in the same category -- there are no sources to compile, and that task only populates the other module's `build/libs`. Dropping them would make the module's intent (repackage `iceberg-rest-server`'s runtime classpath with Iceberg substituted) clearer to the next reader. Verified by: `ls` of the new module directory on this checkout (build file only, no `src`), and reading `iceberg/iceberg-rest-server/build.gradle.kts:32-50` where the same `exclude("*")` pattern is used on a module that does have sources. ########## iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.iceberg.integration.test; + +import com.google.common.collect.ImmutableList; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.UUID; +import org.apache.gravitino.authorization.Privilege; +import org.apache.gravitino.authorization.Privileges; +import org.apache.gravitino.authorization.SecurableObject; +import org.apache.gravitino.authorization.SecurableObjects; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +import org.apache.iceberg.exceptions.ForbiddenException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Integration test for Spark authorization through the Iceberg REST service loaded from the + * experimental classpath. + * + * <p>The inherited tests exercise table creation, loading, updates, deletion, listing, renaming, + * nested namespaces, and table privilege enforcement against the experimental Iceberg artifacts. + */ +@Tag("gravitino-docker-test") +public class IcebergExperimentalAuthorizationIT extends IcebergTableAuthorizationIT { + + private static final String EXPERIMENTAL_SERVER_DIRECTORY = "iceberg-rest-experimental-server"; + + @BeforeAll + @Override + public void startIntegrationTest() throws Exception { + String rootDirectory = System.getenv("GRAVITINO_ROOT_DIR"); + String experimentalServerHome = + Paths.get(rootDirectory, "iceberg", EXPERIMENTAL_SERVER_DIRECTORY).toString(); + customConfigs.put( + "gravitino.iceberg-rest.classpath", + String.join( + ",", + Paths.get(experimentalServerHome, "build", "libs").toString(), + Paths.get(experimentalServerHome, "build", "dependencies").toString())); + super.startIntegrationTest(); + } + + @Override + protected TestDatabaseName getTestDatabaseName() { + return TestDatabaseName.PG_ICEBERG_EXPERIMENTAL_AUTHZ_IT; + } + + @Test + void testSparkCreateNamespaceAuthorization() { + String namespace = "experimental_authz_ns"; + revokeUserRoles(); + resetMetalakeAndCatalogOwner(); + + grantCatalogPrivileges(Privileges.UseCatalog.allow()); + sql("USE %s", SPARK_CATALOG_NAME); + + Assertions.assertThrowsExactly( + ForbiddenException.class, () -> sql("CREATE DATABASE %s", namespace)); + + grantCatalogPrivileges(Privileges.CreateSchema.allow()); + Assertions.assertDoesNotThrow(() -> sql("CREATE DATABASE %s", namespace)); + Assertions.assertDoesNotThrow(() -> sql("USE %s", namespace)); + } Review Comment: [Nit] This test duplicates existing coverage and sits in the wrong hierarchy. `IcebergNamespaceAuthorizationIT.testCreateNamespaceAuthorization` (lines 102-113 of that file) already does exactly this -- grant `UseCatalog`, assert `CREATE DATABASE` throws `ForbiddenException`, grant `CreateSchema`, assert it succeeds -- using the parent's `grantCreateSchemaRole` helper, so `grantCatalogPrivileges` here re-implements `grantUseCatalogRole`/`grantCreateSchemaRole` from that class (lines 510-545). Since this class extends `IcebergTableAuthorizationIT`, the extra method is also the only namespace-level case in a table-level subclass, and it is the only inherited-suite member that exists in the experimental variant but not the stable one -- so the two configurations no longer cover the same set. Either drop it and let the inherited table suite be the experimental coverage, or add a second experimental subclass of `IcebergNamespaceAuthorizationIT` and keep the namespace cases there. The namespace it creates is also never dropped; I checked the inherited assertions and none depend on an exact namespace list, so it is harmless today, just untidy. Verified by: reading `IcebergNamespaceAuthorizationIT` (lines 50-145, 510-556) and `IcebergTableAuthorizationIT` (lines 50-75, 360-385) on this checkout. ########## iceberg/iceberg-rest-server/build.gradle.kts: ########## @@ -205,7 +205,12 @@ tasks.test { // Exclude integration tests exclude("**/integration/test/**") } else { - dependsOn(tasks.jar) + dependsOn( + tasks.jar, + ":catalogs:catalog-lakehouse-iceberg:jar", + ":iceberg:iceberg-rest-experimental-server:copyDepends", + ":iceberg:iceberg-rest-experimental-server:jar" + ) Review Comment: [Nit] This makes every integration-test run of `:iceberg:iceberg-rest-server` build the experimental module and resolve the `com.datastrato` artifacts, even when the selected tests have nothing to do with the experimental classpath (e.g. `--tests IcebergRESTHiveCatalogIT`). That adds a network dependency and build time to all of them. If that is unwanted, the two experimental task dependencies could hang off a property or be moved to a dedicated test task, leaving `tasks.jar` and `:catalogs:catalog-lakehouse-iceberg:jar` as before. Verified by: reading `tasks.test` in this file on this checkout; the `else` branch applies to every non-`skipITs` invocation of the module's `test` task. ########## docs/iceberg-rest-service.md: ########## @@ -102,14 +102,27 @@ The server-level `gravitino.fetchFile.blockUnsafeRemoteUri` configuration contro #### Auxiliary Service -| Configuration item | Description | Default value | Required | -|------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------| -| `gravitino.auxService.names` | The auxiliary service name of the Gravitino Iceberg REST catalog service. Use **`iceberg-rest`**. | (none) | Yes | -| `gravitino.iceberg-rest.classpath` | The classpath of the Gravitino Iceberg REST catalog service; includes the directory containing jars and configuration. It supports both absolute and relative paths, for example, `iceberg-rest-server/libs, iceberg-rest-server/conf` | (none) | Yes | +| Configuration item | Description | Default value | Required | +|------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------| +| `gravitino.auxService.names` | The auxiliary service name of the Gravitino Iceberg REST catalog service. Use **`iceberg-rest`**. | (none) | Yes | +| `gravitino.iceberg-rest.classpath` | The classpath of the Gravitino Iceberg REST catalog service. Use `iceberg-rest-server/libs, iceberg-rest-server/conf` for the regular Iceberg dependencies or `iceberg-rest-experimental-server/libs, iceberg-rest-experimental-server/conf` for the experimental dependencies. It supports both absolute and relative paths. | (none) | Yes | These settings apply only to `gravitino.conf`. Do not add them to the standalone server configuration. +The auxiliary service name and all Iceberg REST configuration keys remain the same when using the +experimental dependencies. Only select the experimental classpath: + +```properties +gravitino.auxService.names = iceberg-rest +gravitino.iceberg-rest.classpath = iceberg-rest-experimental-server/libs, iceberg-rest-experimental-server/conf +``` + +The selected directory is loaded by the Iceberg REST auxiliary service's isolated classloader. The +experimental directory uses the Datastrato Iceberg distribution published under the +`com.datastrato` Maven group. Its version is pinned by `datastrato-iceberg` in +`gradle/libs.versions.toml`, independently of the regular Iceberg version. Review Comment: [Question] "The selected directory is loaded by the Iceberg REST auxiliary service's isolated classloader" overstates what the loader does, and users may read it as a guarantee that the stable and experimental dependency graphs cannot mix. `IsolatedClassLoader` shares, rather than isolates, every class outside a short list of `org.apache.gravitino.catalog.*` / `org.apache.gravitino.hive.*` prefixes: `isSharedClass` is `!isCatalogClass(name)` for `org.apache.iceberg.*` and `org.apache.gravitino.iceberg.*`, which are then taken from the base classloader and only fall back to the configured directory on `ClassNotFoundException` (`core/src/main/java/org/apache/gravitino/utils/IsolatedClassLoader.java:242`, `268-273`, `283-300`), and the class loader's parent is the server's own context classloader (lines 256-258). The selection works today only because `distribution/package/libs` happens to carry no Iceberg classes (`build.gradle.kts:1480` excludes `iceberg*` modules from `copySubprojectDependencies`). Would you be willing to soften this to say the directory is *prepended for the auxiliary service* and that it takes effect as long as no Iceberg classes are on the server classpath? And is it worth stating explicitly here that only one of the two directories can be active at a time, now that the service name is shared? Verified by: reading `IsolatedClassLoader` and `AuxiliaryServiceManager.registerAuxService` (`core/src/main/java/org/apache/gravitino/auxiliary/AuxiliaryServiceManager.java:155-184`) in full, and the distribution packaging filter in the root build file, on this checkout. ########## build.gradle.kts: ########## @@ -1168,6 +1168,7 @@ tasks { "copyJobsLib", ":authorizations:copyLibAndConfig", ":iceberg:iceberg-rest-server:copyLibAndConfigs", + ":iceberg:iceberg-rest-experimental-server:copyLibAndConfigs", Review Comment: [Important] The binary distribution now bundles non-ASF `com.datastrato:experimental-iceberg-*` jars, but `LICENSE.bin` / `NOTICE.bin` were not updated, and no CI check will catch it. This line pulls `iceberg-rest-experimental-server:copyLibAndConfigs` into `compileDistribution`, which syncs the resolved runtime classpath into `distribution/package/iceberg-rest-experimental-server/libs` (`iceberg/iceberg-rest-experimental-server/build.gradle.kts:70-75`), and the same task copies `LICENSE.bin`/`NOTICE.bin` into the package as the binary `LICENSE`/`NOTICE` (`build.gradle.kts:1213-1217`). `LICENSE.bin:323-331` still lists only the `Apache Iceberg*` artifacts; there is no entry for the Datastrato distribution. Its POM declares Apache-2.0 with `groupId com.datastrato` and `url https://github.com/datastrato/iceberg`, so this is an enumeration gap rather than a license conflict -- but it is a release-blocking one for a convenience binary, and `dev/release/check-license.sh:30` deliberately skips the `.bin` variants, so nothing fails before a release vote. Suggest adding the bundled `experimental-iceberg-*` artifacts to `LICENSE.bin` (and `NOTICE.bin` if the fork ships its own NOTICE), and confirming with the PMC that shipping a third-party Iceberg fork inside the ASF convenience binary is acceptable -- that call is above a code review. Verified by: reading `build.gradle.kts:1152-1240`, `LICENSE.bin:310-335` and `dev/release/check-license.sh` on this checkout, plus the fetched `experimental-iceberg-core-1.13.0.pom` license block. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
