yuqi1129 commented on code in PR #14032:
URL: https://github.com/apache/gravitino/pull/14032#discussion_r4333727097
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalog.java:
##########
@@ -132,4 +169,44 @@ protected static AbstractCatalog asAbstractCatalog(Object
catalog) {
catalog.getClass().getName());
return (AbstractCatalog) catalog;
}
+
+ // Non-REST backends only receive the catalog-level credentials fetched once
in open(); a REST
+ // backend is exempt because the inner REST catalog obtains per-table,
refreshable credentials
+ // itself. aws-irsa is rejected: its credential is injected but never
refreshed, so it overrides
+ // the node's own credentials and then expires. Path-scoped token providers
vend nothing at
+ // catalog level, which leaves the node's own credentials in effect, so they
are only returned
+ // for the caller to warn about.
+ @VisibleForTesting
+ static List<String> checkCredentialProviders(
+ String catalogName, Map<String, String> gravitinoCatalogProperties) {
+ String backend =
+ gravitinoCatalogProperties.get(
+ IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
+ if
(IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST.equalsIgnoreCase(backend))
{
Review Comment:
[Important] Could this decision use the effective inner catalog type rather
than the persisted Gravitino backend?
`GravitinoIcebergCatalogFactory.toIcebergCatalogOptions()` respects an explicit
`catalog-type`, so the client can select REST while the server catalog still
has `catalog-backend=hive`.
I reproduced this by confirming that the factory produces
`catalog-type=rest`, then calling `open()` with server properties containing
`catalog-backend=hive` and `credential-providers=aws-irsa`: this check throws
before creating the inner catalog. That incorrectly rejects a REST client,
contrary to the intended REST exemption.
Please base the check on the normalized inner-catalog selection and add an
`open()` regression test for this combination; the new tests currently call
only the static helper.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalog.java:
##########
@@ -132,4 +169,44 @@ protected static AbstractCatalog asAbstractCatalog(Object
catalog) {
catalog.getClass().getName());
return (AbstractCatalog) catalog;
}
+
+ // Non-REST backends only receive the catalog-level credentials fetched once
in open(); a REST
+ // backend is exempt because the inner REST catalog obtains per-table,
refreshable credentials
+ // itself. aws-irsa is rejected: its credential is injected but never
refreshed, so it overrides
+ // the node's own credentials and then expires. Path-scoped token providers
vend nothing at
+ // catalog level, which leaves the node's own credentials in effect, so they
are only returned
+ // for the caller to warn about.
+ @VisibleForTesting
+ static List<String> checkCredentialProviders(
+ String catalogName, Map<String, String> gravitinoCatalogProperties) {
+ String backend =
+ gravitinoCatalogProperties.get(
+ IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
+ if
(IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST.equalsIgnoreCase(backend))
{
+ return Collections.emptyList();
+ }
+ String providers =
gravitinoCatalogProperties.get(CredentialConstants.CREDENTIAL_PROVIDERS);
+ if (StringUtils.isBlank(providers)) {
Review Comment:
[Important] An omitted `credential-providers` value does not necessarily
mean there is no token provider. The server's
`BaseCatalog.addStorageCredentialProviders()` infers `gcs-token` from
`gcs-service-account-file`, but that inference happens in
`propertiesWithCredentialProviders()` and is not normally included in the
client's `catalog.properties()` response.
For a Hive/JDBC catalog configured with `gcs-service-account-file` and no
explicit provider list, this early return suppresses the warning, leaving the
silent no-credential case that this PR is intended to diagnose. A local
reproduction confirms the helper returns an empty list for that configuration.
Please account for effective/inferred providers and add coverage for this
documented default configuration.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/IcebergPropertiesConstants.java:
##########
@@ -52,4 +52,8 @@ public class IcebergPropertiesConstants {
@VisibleForTesting
public static final String ICEBERG_CATALOG_BACKEND_REST =
CatalogUtil.ICEBERG_CATALOG_TYPE_REST;
+
+ public static final String GRAVITINO_ICEBERG_DATA_ACCESS =
IcebergConstants.DATA_ACCESS;
Review Comment:
[Nit] Please add Javadoc for both new public constants, as required by the
project agent guidelines.
--
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]
