Copilot commented on code in PR #11694:
URL: https://github.com/apache/gravitino/pull/11694#discussion_r3421841663
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalogFactory.java:
##########
@@ -122,35 +122,60 @@ Map<String, String> toIcebergCatalogOptions(Map<String,
String> catalogOptions)
Map<String, String> icebergCatalogOptions =
Maps.newHashMap(catalogOptions);
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
- // Only infer `catalog-type` from the backend when neither `catalog-type`
nor `catalog-impl` is
- // already set, otherwise an explicitly provided `catalog-impl` would
conflict with it.
+ // catalogBackend is only present here on the CREATE CATALOG path (raw
user SQL options). On
+ // the USE CATALOG path (loading a catalog already persisted in Gravitino),
+ // GravitinoCatalogStore.getCatalog() has already renamed catalog-backend
-> catalog-type
+ // before this method runs, so catalogBackend is null even for a REST/JDBC
catalog. This
+ // normalizes the CREATE CATALOG case so both paths converge on the same
catalog-type key.
+ String catalogType = normalizeCatalogType(icebergCatalogOptions,
catalogBackend);
+ // catalogType (not catalogBackend, which is unreliable as explained
above) must be used for
+ // every backend check from here on, or it silently no-ops on the USE
CATALOG path -- this was
+ // the root cause of #11601 (REST auth never propagated when loading a
persisted catalog).
+ propagateRestAuthIfNeeded(icebergCatalogOptions, catalogType);
+ translateJdbcBackendToCatalogImpl(icebergCatalogOptions, catalogType);
Review Comment:
`normalizeCatalogType(...)` returns `null` when `catalog-backend` is present
but `catalog-impl` is also set (because it intentionally skips writing
`catalog-type`), which will then prevent REST auth propagation / JDBC
translation from running on the CREATE CATALOG path even though the backend was
provided. Consider returning an “effective catalog type” (e.g., existing
`catalog-type` if present, otherwise `catalog-backend`) for downstream gating
checks, while still only *writing* `catalog-type` into the map when it’s safe
(i.e., no conflicting `catalog-impl`). This avoids silently skipping behavior
in valid configurations.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalogFactory.java:
##########
@@ -122,35 +122,60 @@ Map<String, String> toIcebergCatalogOptions(Map<String,
String> catalogOptions)
Map<String, String> icebergCatalogOptions =
Maps.newHashMap(catalogOptions);
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
- // Only infer `catalog-type` from the backend when neither `catalog-type`
nor `catalog-impl` is
- // already set, otherwise an explicitly provided `catalog-impl` would
conflict with it.
+ // catalogBackend is only present here on the CREATE CATALOG path (raw
user SQL options). On
+ // the USE CATALOG path (loading a catalog already persisted in Gravitino),
+ // GravitinoCatalogStore.getCatalog() has already renamed catalog-backend
-> catalog-type
+ // before this method runs, so catalogBackend is null even for a REST/JDBC
catalog. This
+ // normalizes the CREATE CATALOG case so both paths converge on the same
catalog-type key.
+ String catalogType = normalizeCatalogType(icebergCatalogOptions,
catalogBackend);
+ // catalogType (not catalogBackend, which is unreliable as explained
above) must be used for
+ // every backend check from here on, or it silently no-ops on the USE
CATALOG path -- this was
+ // the root cause of #11601 (REST auth never propagated when loading a
persisted catalog).
+ propagateRestAuthIfNeeded(icebergCatalogOptions, catalogType);
+ translateJdbcBackendToCatalogImpl(icebergCatalogOptions, catalogType);
+ // The outer Flink factory is `gravitino-iceberg`, but the nested Iceberg
factory still expects
+ // `catalog-type=iceberg` when building the native Iceberg catalog
instance.
+ icebergCatalogOptions.put(CommonCatalogOptions.CATALOG_TYPE.key(),
"iceberg");
+ return icebergCatalogOptions;
+ }
+
+ // Copies catalog-backend into catalog-type so the CREATE CATALOG path lines
up with the
+ // USE CATALOG path, where the rename already happened upstream. Skipped
when catalog-type or
+ // catalog-impl is already set, otherwise an explicitly provided
catalog-impl would conflict
+ // with it. Returns the resulting catalog-type, which is guaranteed to be
set on both paths.
+ private static String normalizeCatalogType(
+ Map<String, String> icebergCatalogOptions, String catalogBackend) {
if (catalogBackend != null
&&
!icebergCatalogOptions.containsKey(IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE)
&&
!icebergCatalogOptions.containsKey(IcebergPropertiesConstants.ICEBERG_CATALOG_IMPL))
{
icebergCatalogOptions.put(IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE,
catalogBackend);
}
- // A REST backend connects directly to the Iceberg REST service, bypassing
the Gravitino
- // server's auth proxy, so propagate the Gravitino client's authentication
to the REST client,
- // unless the user has already configured REST auth explicitly.
- if
(IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST.equalsIgnoreCase(catalogBackend)
+ return
icebergCatalogOptions.get(IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE);
+ }
Review Comment:
`normalizeCatalogType(...)` returns `null` when `catalog-backend` is present
but `catalog-impl` is also set (because it intentionally skips writing
`catalog-type`), which will then prevent REST auth propagation / JDBC
translation from running on the CREATE CATALOG path even though the backend was
provided. Consider returning an “effective catalog type” (e.g., existing
`catalog-type` if present, otherwise `catalog-backend`) for downstream gating
checks, while still only *writing* `catalog-type` into the map when it’s safe
(i.e., no conflicting `catalog-impl`). This avoids silently skipping behavior
in valid configurations.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalogFactory.java:
##########
@@ -122,35 +122,60 @@ Map<String, String> toIcebergCatalogOptions(Map<String,
String> catalogOptions)
Map<String, String> icebergCatalogOptions =
Maps.newHashMap(catalogOptions);
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
- // Only infer `catalog-type` from the backend when neither `catalog-type`
nor `catalog-impl` is
- // already set, otherwise an explicitly provided `catalog-impl` would
conflict with it.
+ // catalogBackend is only present here on the CREATE CATALOG path (raw
user SQL options). On
+ // the USE CATALOG path (loading a catalog already persisted in Gravitino),
+ // GravitinoCatalogStore.getCatalog() has already renamed catalog-backend
-> catalog-type
+ // before this method runs, so catalogBackend is null even for a REST/JDBC
catalog. This
+ // normalizes the CREATE CATALOG case so both paths converge on the same
catalog-type key.
+ String catalogType = normalizeCatalogType(icebergCatalogOptions,
catalogBackend);
+ // catalogType (not catalogBackend, which is unreliable as explained
above) must be used for
+ // every backend check from here on, or it silently no-ops on the USE
CATALOG path -- this was
+ // the root cause of #11601 (REST auth never propagated when loading a
persisted catalog).
+ propagateRestAuthIfNeeded(icebergCatalogOptions, catalogType);
+ translateJdbcBackendToCatalogImpl(icebergCatalogOptions, catalogType);
+ // The outer Flink factory is `gravitino-iceberg`, but the nested Iceberg
factory still expects
+ // `catalog-type=iceberg` when building the native Iceberg catalog
instance.
+ icebergCatalogOptions.put(CommonCatalogOptions.CATALOG_TYPE.key(),
"iceberg");
+ return icebergCatalogOptions;
+ }
+
+ // Copies catalog-backend into catalog-type so the CREATE CATALOG path lines
up with the
+ // USE CATALOG path, where the rename already happened upstream. Skipped
when catalog-type or
+ // catalog-impl is already set, otherwise an explicitly provided
catalog-impl would conflict
+ // with it. Returns the resulting catalog-type, which is guaranteed to be
set on both paths.
Review Comment:
The comment states the returned `catalog-type` is “guaranteed to be set on
both paths,” but the implementation can return `null` (e.g., when
`catalog-backend` is null, or when `catalog-impl` is set and no `catalog-type`
exists). Please adjust the comment to match actual behavior, or change the
method to uphold the guarantee.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalogFactory.java:
##########
@@ -122,35 +122,60 @@ Map<String, String> toIcebergCatalogOptions(Map<String,
String> catalogOptions)
Map<String, String> icebergCatalogOptions =
Maps.newHashMap(catalogOptions);
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
- // Only infer `catalog-type` from the backend when neither `catalog-type`
nor `catalog-impl` is
- // already set, otherwise an explicitly provided `catalog-impl` would
conflict with it.
+ // catalogBackend is only present here on the CREATE CATALOG path (raw
user SQL options). On
+ // the USE CATALOG path (loading a catalog already persisted in Gravitino),
+ // GravitinoCatalogStore.getCatalog() has already renamed catalog-backend
-> catalog-type
+ // before this method runs, so catalogBackend is null even for a REST/JDBC
catalog. This
+ // normalizes the CREATE CATALOG case so both paths converge on the same
catalog-type key.
+ String catalogType = normalizeCatalogType(icebergCatalogOptions,
catalogBackend);
Review Comment:
`normalizeCatalogType(...)` returns `null` when `catalog-backend` is present
but `catalog-impl` is also set (because it intentionally skips writing
`catalog-type`), which will then prevent REST auth propagation / JDBC
translation from running on the CREATE CATALOG path even though the backend was
provided. Consider returning an “effective catalog type” (e.g., existing
`catalog-type` if present, otherwise `catalog-backend`) for downstream gating
checks, while still only *writing* `catalog-type` into the map when it’s safe
(i.e., no conflicting `catalog-impl`). This avoids silently skipping behavior
in valid configurations.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/iceberg/TestGravitinoIcebergCatalogFactory.java:
##########
@@ -115,4 +119,26 @@ void testRestBackendKeepsCatalogType() {
Assertions.assertFalse(result.containsKey(IcebergPropertiesConstants.ICEBERG_CATALOG_IMPL));
Assertions.assertEquals("iceberg",
result.get(CommonCatalogOptions.CATALOG_TYPE.key()));
}
+
+ @Test
+ void testRestAuthPropagatedWhenLoadedFromCatalogStore() {
+ // Simulates the catalog-store load path (`USE CATALOG ...`): by the time
options reach this
+ // factory, GravitinoCatalogStore.getCatalog has already renamed
`catalog-backend` ->
+ // `catalog-type`, so only `catalog-type=rest` is present, no
`catalog-backend` key.
+ Map<String, String> options =
+ ImmutableMap.of(
+ IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE,
+ IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST,
+ IcebergPropertiesConstants.ICEBERG_CATALOG_URI,
+ "http://localhost:9001/iceberg/";);
+
+ // Regression check for #11601: the REST auth-propagation branch must be
gated on the
+ // normalized `catalog-type`, not the renamed-away `catalog-backend` key,
otherwise it would
+ // silently skip auth propagation on this path. GravitinoCatalogManager
isn't initialized in
+ // this unit test, so reaching it (and failing there) proves the gate now
fires correctly.
+ IllegalStateException exception =
+ Assertions.assertThrows(
+ IllegalStateException.class, () ->
factory.toIcebergCatalogOptions(options));
+
Assertions.assertTrue(exception.getMessage().contains("GravitinoCatalogManager"));
Review Comment:
This assertion is brittle: `exception.getMessage()` can be `null` (causing
an NPE), and matching a substring ties the test to a specific exception
message. Prefer asserting in a way that’s resilient to message changes (e.g.,
first assert the message is non-null, or assert on a more stable signal such as
the exception type/cause/stack trace containing `GravitinoCatalogManager.get`,
depending on what `GravitinoCatalogManager.get()` throws in this codebase).
--
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]
