[#11501] test(iceberg-rest): Add Trino IT for Iceberg REST authorization in auxMode by roryqi · Pull Request #11502 · apache/gravitino

roryqi · 2026-06-08T12:29:41Z

What changes were proposed in this pull request?

Add a new test-only module iceberg/iceberg-rest-trino-it that verifies Gravitino's authorization is enforced when an Iceberg REST Catalog served in auxiliary mode (auxMode) is accessed through Trino's native iceberg REST connector. This is the Trino analog of the Spark-based IcebergAuthorizationIT.

Specifically:

build.gradle.kts + settings.gradle.kts — scaffold the module on a Java 24 toolchain with trino-iceberg:478 / trino-testing:478, mirroring trino-connector-473-478 (Error Prone / JaCoCo disabled for JDK 24, JUnit aligned to Trino's 6.0.0 BOM, jjwt api+impl only).
MockOAuthTokenServer (+ TestMockOAuthTokenServer) — a minimal RS256 client-credentials OAuth2 token endpoint bound to 127.0.0.1, used by the IRC dynamic config provider's service identity. Covered by a unit test asserting a valid signed JWT is returned.
TrinoIcebergRestAuthorizationITBase / TrinoIcebergRestAuthorizationIT / TrinoIcebergRestSmokeIT — boot the full Gravitino server in deploy mode with the iceberg-rest aux service + authorization + OAuth authenticator, and drive an in-process Trino query runner to assert privilege grant/deny.

Why are the changes needed?

The existing IcebergAuthorizationIT only covers the Spark path. Trino accesses the IRC over a different authentication path (OAuth2 bearer only, no Gravitino Basic auth), which was previously untested. This adds end-to-end coverage that Gravitino authorization is enforced for Trino IRC clients.

Fix: #11501

Does this PR introduce any user-facing change?

No. Test-only changes; no new APIs or property keys.

How was this patch tested?

This change is an integration test. Run with the built distribution in deploy mode:

./gradlew compileDistribution
./gradlew :iceberg:iceberg-rest-trino-it:test -PskipDockerTests=false -DtestMode=deploy

MockOAuthTokenServer is additionally covered by TestMockOAuthTokenServer.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Add a minimal RS256 client-credentials OAuth2 token endpoint for the Iceberg REST Catalog Trino IT, bound to 127.0.0.1 so a deploy-mode Gravitino server subprocess can reach it. Covered by a unit test that asserts the POST returns 200 with a valid signed JWT (subject + audience). Also adjust the module build for the JDK 24 toolchain: disable Error Prone 2.10.0 and JaCoCo 0.8.9 (both incompatible with JDK 24), align JUnit to the 6.0.0 BOM that Trino forces and add the matching platform-launcher, and pull only jjwt api+impl (the 0.13.x runtime uses jjwt-jackson from Trino; the stale jjwt-gson:0.11.x service breaks signing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Run the trino-iceberg-rest deploy-mode IT in its own job: it needs a Java 24 toolchain (Trino 478) and the built distribution, so it does not fit the JDK 17 catch-all backend IT. Gated by paths-filter on push/PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…t -> REST The catch-all backend IT runs every module's tests in embedded mode, where this Java 24 / Trino module has no GravitinoServer on its classpath and fails with NoClassDefFoundError. Guard startIntegrationTest with ITUtils.isEmbedded() so the IT self-skips outside deploy mode, keeping it runnable from all deploy workflows. Also rename the new test classes Rest -> REST to match the codebase convention (IcebergRESTServer, IcebergRESTUtils, GravitinoIcebergRESTServer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The backend and cron integration-test pipelines run every module's tests via the catch-all `./gradlew test`. Exclude :iceberg:iceberg-rest-trino-it:test from both so the Trino IRC IT is not run redundantly; it already has its own dedicated workflow (Java 24 / Trino, deploy only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

# Conflicts: # .github/workflows/backend-integration-test-action.yml

github-actions · 2026-06-09T05:00:58Z

Code Coverage Report

Overall Project	66.96%	🟢
Files changed	No Java source files changed	-

Module	Coverage
aliyun	1.72%	🔴
api	46.82%	🟢
authorization-common	85.96%	🟢
aws	3.66%	🔴
azure	2.47%	🔴
catalog-common	10.4%	🔴
catalog-fileset	80.23%	🟢
catalog-glue	66.91%	🟢
catalog-hive	79.44%	🟢
catalog-jdbc-clickhouse	80.02%	🟢
catalog-jdbc-common	44.22%	🟢
catalog-jdbc-doris	80.28%	🟢
catalog-jdbc-hologres	54.03%	🟢
catalog-jdbc-mysql	79.23%	🟢
catalog-jdbc-oceanbase	78.38%	🟢
catalog-jdbc-postgresql	82.29%	🟢
catalog-jdbc-starrocks	78.51%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	58.53%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.87%	🟢
catalog-lakehouse-paimon	79.15%	🟢
catalog-model	77.72%	🟢
cli	44.51%	🟢
client-java	78.01%	🟢
common	49.9%	🟢
core	82.38%	🟢
filesystem-hadoop3	77.27%	🟢
flink	0.0%	🔴
flink-common	45.72%	🟢
flink-runtime	0.0%	🔴
gcp	14.12%	🔴
hadoop-common	10.39%	🔴
hive-metastore-common	53.9%	🟢
iceberg-common	57.41%	🟢
iceberg-rest-server	74.0%	🟢
idp-basic	86.18%	🟢
integration-test-common	0.0%	🔴
jobs	66.17%	🟢
lance-common	20.79%	🔴
lance-rest-server	60.54%	🟢
lineage	53.02%	🟢
optimizer	82.95%	🟢
optimizer-api	21.95%	🔴
server	85.73%	🟢
server-common	73.28%	🟢
spark	28.57%	🔴
spark-common	41.66%	🟢
trino-connector	40.13%	🟢

diqiu50 · 2026-06-10T07:05:07Z

+  // incompatible with the 0.13.x runtime and makes signWith().compact() fail.
+  testImplementation(libs.jwt.api)
+  testRuntimeOnly(libs.jwt.impl)
+  testRuntimeOnly("io.jsonwebtoken:jjwt-jackson:0.13.0")


Should this be placed in the libs directory?

Good catch. It turned out the explicit declaration was redundant: jjwt-jackson:0.13.0 is already provided transitively by trino-iceberg/trino-main/trino-testing (verified with dependencyInsight that it stays on the test runtime classpath). So I removed the line entirely — no hardcoded version and no new catalog entry needed. Separately, the remaining version literals in this module (testcontainers, docker-java, and the Trino version) are now centrally managed in gradle/libs.versions.toml.

diqiu50 · 2026-06-10T07:11:07Z

+  @Test
+  @Order(1)
+  public void testNormalUserDeniedWithoutPrivileges() {
+    assertThrows(RuntimeException.class, () -> sql(NORMAL_CATALOG, "SELECT * FROM db1.shared"));


Can you check the specific error message instead of throwing a RuntimeException?

Done. Added assertAuthorizationDenied, which walks the exception cause chain and asserts the IRC 403 message "is not authorized to perform operation" (produced by BaseMetadataAuthorizationMethodInterceptor), applied to both denial tests.

diqiu50 · 2026-06-10T07:20:25Z

+      try {
+        super.stopIntegrationTest();
+      } finally {
+        containerSuite.close();


Closing ContainerSuite might affect other tests

Agreed. Removed containerSuite.close() from teardown — it is a process-wide singleton whose PostgreSQL container may be reused by other tests in the same JVM. This matches the Spark analog IcebergAuthorizationIT, which does not close it either.

diqiu50 · 2026-06-10T07:29:37Z

+
+// Test against the highest supported Trino version.
+val minTrinoVersion = 473
+val maxTrinoVersion = 478


Do we need to support multiple versions here?

No — this is a test-only module that boots an in-process Trino query runner, so it only needs a single version. Removed the min/max range + -PtrinoVersion machinery and pinned the version, now sourced from the catalog (libs.versions.trino-iceberg-it).

diqiu50 · 2026-06-10T07:30:33Z

+  testImplementation(project(":common"))
+  testImplementation(project(":clients:client-java"))
+  testImplementation(project(":core"))
+  testImplementation(project(":server-common"))


Some dependencies may not be used.

Some dependencies are required for BaseIT.

Removed awaitility and commons-lang3 (no references). I also checked :common, but it is required at compile time (ConfigEntry/MetalakeDTO show up in the signatures of Configs/createMetalake), so it is kept.

Correction: CI showed awaitility and commons-lang3 are actually not unused — the inherited BaseIT/ITUtils harness (from the integration-test-common testArtifacts dep) uses them at runtime, and testArtifacts does not carry transitive runtime deps, so removing them failed the ITs with NoClassDefFoundError: org/awaitility/Awaitility. I restored both (as testRuntimeOnly, since this module's own sources do not reference them) in 63ac498. :common remains required at compile time. Net result: the declared dependencies are all needed.

diqiu50 · 2026-06-10T07:35:15Z

+tasks.test {
+  useJUnitPlatform()
+  // These ITs require the built distribution (deploy mode) and a server subprocess.
+  val skipITs = providers.gradleProperty("skipITs").map(String::toBoolean).orElse(false)
+  val skipTests = providers.gradleProperty("skipTests").map(String::toBoolean).orElse(false)
+  onlyIf { !skipITs.get() && !skipTests.get() }
+}


Skipping here is inconsistent with other modules, which use project.hasProperty("skipITs").

Done. Switched to project.hasProperty("skipITs") + exclude("**/integration/test/**"), consistent with iceberg-rest-server. This also fixes a latent issue: the old onlyIf skipped these ITs under -PskipTests, when they should still run.

- Pin a single Trino version instead of the multi-version range machinery (this module boots an in-process query runner, unlike trino-connector). - Drop the redundant explicit jjwt-jackson dependency; Trino already provides it transitively at 0.13.x. - Remove unused awaitility and commons-lang3 test dependencies. - Use project.hasProperty("skipITs") + exclude, consistent with other modules (also fixes ITs being wrongly skipped under -PskipTests). - Assert the IRC 403 "not authorized" message in denial tests instead of a bare RuntimeException. - Do not close the shared ContainerSuite singleton in teardown, since its PostgreSQL container may be reused by other tests in the same JVM. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… in the catalog Move the testcontainers, docker-java, and Trino version literals out of the module build file into gradle/libs.versions.toml so they are centrally managed, keeping the resolutionStrategy.force block (which is required to override the newer testcontainers/docker-java that Trino's test stack would otherwise pull). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

These are not unused: the inherited BaseIT/ITUtils harness (pulled via the integration-test-common testArtifacts dependency) uses Awaitility and commons-lang3 at runtime. testArtifacts only carries the compiled classes, not their transitive runtime deps, so the ITs failed in CI with NoClassDefFoundError: org/awaitility/Awaitility. Declared as testRuntimeOnly since this module's own sources do not reference them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ertion Two issues surfaced when the ITs actually ran in CI: - TrinoIcebergRESTSmokeIT failed with "schema db1 already exists": dropping the Gravitino metalake does not remove namespaces from the shared PostgreSQL-backed Iceberg catalog, and removing containerSuite.close() (which previously reset it) left them behind. Now drop the catalog's schemas in teardown instead of tearing down the shared ContainerSuite. - testNormalUserDeniedWithoutPrivileges failed because Trino masks the REST 403 body on the read path and reports "Failed to load view" rather than Gravitino's authorization message (which it does surface for CREATE TABLE). Accept either form as evidence of a denial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

TrinoIcebergRESTSmokeIT and TrinoIcebergRESTAuthorizationIT share one PostgreSQL-backed Iceberg catalog (the shared ContainerSuite is intentionally not reset between classes). Both used schema "db1", so the second class to run failed with "schema db1 already exists". Give the smoke test its own schema ("smoke_db") so the classes cannot collide, and drop the unreliable teardown schema cleanup in favor of this deterministic isolation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

diqiu50

LGTM

… for Iceberg REST authorization in auxMode (#11502) (#11651) **Cherry-pick Information:** - Original commit: 88d3996 - Target branch: `branch-1.3` - Status: ✅ Clean cherry-pick (no conflicts) Co-authored-by: roryqi <roryqi@datastrato.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

roryqi and others added 4 commits June 8, 2026 12:29

build: scaffold trino-iceberg-rest-it module (Trino 478, Java 24)

47928e0

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

test: add Trino authorization coverage for Iceberg REST

e19eec5

roryqi requested a review from diqiu50 June 8, 2026 14:31

roryqi and others added 3 commits June 9, 2026 03:33

Merge remote-tracking branch 'upstream/main' into iceberg-irc-trino-it

2b5e97d

# Conflicts: # .github/workflows/backend-integration-test-action.yml

roryqi self-assigned this Jun 9, 2026

roryqi added the branch-1.3 Automatically cherry-pick commit to branch-1.3 label Jun 9, 2026

diqiu50 reviewed Jun 10, 2026

View reviewed changes

roryqi and others added 2 commits June 11, 2026 08:05

roryqi requested a review from diqiu50 June 11, 2026 08:41

roryqi and others added 3 commits June 11, 2026 09:18

diqiu50 approved these changes Jun 15, 2026

View reviewed changes

diqiu50 merged commit 88d3996 into apache:main Jun 15, 2026
33 checks passed

Uh oh!

Conversation

roryqi commented Jun 8, 2026

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?

Uh oh!

github-actions Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diqiu50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jun 9, 2026 •

edited

Loading