Copilot commented on code in PR #11853:
URL: https://github.com/apache/gravitino/pull/11853#discussion_r3505843157
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -104,38 +108,50 @@ public Optional<IcebergConfig>
getIcebergCatalogConfig(String catalogName) {
"lakehouse-iceberg".equals(catalog.provider()),
String.format("%s.%s is not iceberg catalog", gravitinoMetalake,
catalogName));
- // Sensitive credentials (e.g. jdbc-password) are marked hidden in
PropertiesMetadata and
- // filtered out of catalog.properties(). We need two different strategies
to recover them:
+ // Sensitive credentials are marked hidden in PropertiesMetadata and
filtered out of
+ // catalog.properties(). We need two different strategies to recover them:
//
// Auxiliary mode: the catalog is a BaseCatalog running in the same JVM as
the Gravitino
// server. Call propertiesWithCredentialProviders() which returns the raw
entity properties
- // including all hidden fields.
+ // including all hidden fields, then generate credentials locally.
//
// Standalone mode: the catalog is a client-side object obtained via the
Gravitino REST API.
- // Call getCredentials() to retrieve vended credentials, then inject any
JdbcCredential
- // fields into the properties map so the JDBC backend can connect.
+ // Call supportsCredentials().getCredentials() to retrieve vended
credentials.
Map<String, String> catalogProperties;
if (catalog instanceof BaseCatalog) {
catalogProperties = ((BaseCatalog<?>)
catalog).propertiesWithCredentialProviders();
} else {
catalogProperties = new HashMap<>(catalog.properties());
- if (catalog instanceof SupportsCredentials) {
- Arrays.stream(((SupportsCredentials) catalog).getCredentials())
- .filter(c -> c instanceof JdbcCredential)
- .map(c -> (JdbcCredential) c)
- .findFirst()
- .ifPresent(
- jdbc -> {
- catalogProperties.putIfAbsent(
- IcebergConstants.GRAVITINO_JDBC_USER, jdbc.jdbcUser());
- catalogProperties.putIfAbsent(
- IcebergConstants.GRAVITINO_JDBC_PASSWORD,
jdbc.jdbcPassword());
- });
- }
}
+ CredentialPropertyUtils.applyIcebergCredentials(
+ getCatalogCredentials(catalog, catalogProperties), catalogProperties);
return
Optional.of(getIcebergConfigFromCatalogProperties(catalogProperties));
}
+ private static Credential[] getCatalogCredentials(
+ Catalog catalog, Map<String, String> catalogProperties) {
+ if (catalog instanceof BaseCatalog) {
+ return getInternalCatalogCredentials((BaseCatalog<?>) catalog,
catalogProperties);
+ }
+
+ return CredentialPropertyUtils.getCredentials(catalog);
+ }
+
+ private static Credential[] getInternalCatalogCredentials(
+ BaseCatalog<?> catalog, Map<String, String> catalogProperties) {
+ List<Credential> credentials = new ArrayList<>();
+ CatalogCredentialContext context =
+ new CatalogCredentialContext(PrincipalUtils.getCurrentUserName());
+ for (String credentialType :
+ CredentialUtils.getCredentialProvidersByOrder(() ->
catalogProperties)) {
Review Comment:
`CredentialUtils.getCredentialProvidersByOrder(...)` returns a `Set`, which
does not preserve the configured provider order (see
core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java:54-76).
Because later credentials can overwrite earlier properties during injection,
iterating an unordered set can make precedence nondeterministic when multiple
providers are configured. Consider iterating the ordered `List<String>` from
`CredentialConfig.CREDENTIAL_PROVIDERS` instead.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java:
##########
@@ -138,6 +144,22 @@ public void initialize(
this.icebergViewCatalogOperations = new
IcebergViewCatalogOperations(icebergCatalogWrapper);
}
+ private static void injectCredentialConfig(
+ Map<String, String> conf, String catalogName, Map<String, String>
resultConf) {
+ List<Credential> credentials = new ArrayList<>();
+ CatalogCredentialContext context =
+ new CatalogCredentialContext(PrincipalUtils.getCurrentUserName());
+ try (CatalogCredentialManager credentialManager =
+ new CatalogCredentialManager(catalogName, conf)) {
+ for (String credentialProvider :
CredentialUtils.getCredentialProvidersByOrder(() -> conf)) {
+ credentialManager.getCredential(credentialProvider,
context).ifPresent(credentials::add);
+ }
+ }
+
+ CredentialPropertyUtils.applyIcebergCredentials(
+ credentials.toArray(new Credential[0]), resultConf);
Review Comment:
`AwsIrsaCredential`/`S3TokenCredential` are time-bound session credentials,
but they’re injected into the catalog config once during `initialize(...)`. If
the Iceberg catalog wrapper (and its FileIO) is long-lived, these static
session keys will eventually expire and server-side S3 access can start
failing. Consider using a refreshable credentials mechanism (e.g., pass
web-identity inputs through so the AWS SDK refreshes automatically, or
invalidate/rebuild the catalog wrapper when `s3.session-token-expires-at-ms` is
near/over expiry).
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -104,38 +108,50 @@ public Optional<IcebergConfig>
getIcebergCatalogConfig(String catalogName) {
"lakehouse-iceberg".equals(catalog.provider()),
String.format("%s.%s is not iceberg catalog", gravitinoMetalake,
catalogName));
- // Sensitive credentials (e.g. jdbc-password) are marked hidden in
PropertiesMetadata and
- // filtered out of catalog.properties(). We need two different strategies
to recover them:
+ // Sensitive credentials are marked hidden in PropertiesMetadata and
filtered out of
+ // catalog.properties(). We need two different strategies to recover them:
//
// Auxiliary mode: the catalog is a BaseCatalog running in the same JVM as
the Gravitino
// server. Call propertiesWithCredentialProviders() which returns the raw
entity properties
- // including all hidden fields.
+ // including all hidden fields, then generate credentials locally.
//
// Standalone mode: the catalog is a client-side object obtained via the
Gravitino REST API.
- // Call getCredentials() to retrieve vended credentials, then inject any
JdbcCredential
- // fields into the properties map so the JDBC backend can connect.
+ // Call supportsCredentials().getCredentials() to retrieve vended
credentials.
Map<String, String> catalogProperties;
if (catalog instanceof BaseCatalog) {
catalogProperties = ((BaseCatalog<?>)
catalog).propertiesWithCredentialProviders();
} else {
catalogProperties = new HashMap<>(catalog.properties());
- if (catalog instanceof SupportsCredentials) {
- Arrays.stream(((SupportsCredentials) catalog).getCredentials())
- .filter(c -> c instanceof JdbcCredential)
- .map(c -> (JdbcCredential) c)
- .findFirst()
- .ifPresent(
- jdbc -> {
- catalogProperties.putIfAbsent(
- IcebergConstants.GRAVITINO_JDBC_USER, jdbc.jdbcUser());
- catalogProperties.putIfAbsent(
- IcebergConstants.GRAVITINO_JDBC_PASSWORD,
jdbc.jdbcPassword());
- });
- }
}
+ CredentialPropertyUtils.applyIcebergCredentials(
+ getCatalogCredentials(catalog, catalogProperties), catalogProperties);
return
Optional.of(getIcebergConfigFromCatalogProperties(catalogProperties));
Review Comment:
This injects vended session credentials directly into `catalogProperties`
and returns an `IcebergConfig` that may be cached and reused. For expiring
credentials like `AwsIrsaCredential`, this can lead to stale credentials being
used later unless there is an explicit refresh/invalidation mechanism tied to
`s3.session-token-expires-at-ms`. Consider adding a refresh strategy
(invalidate/reload when near expiry) or prefer passing web-identity inputs
through so the AWS SDK can refresh automatically.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/provider/TestDynamicIcebergConfigProvider.java:
##########
@@ -361,6 +363,52 @@ public void
testHttpCatalogFetcherUsedWhenAuthorizationDisabled() throws Illegal
Assertions.assertTrue(icebergConfig.isPresent());
}
+ @Test
+ public void testInjectsVendedStorageCredentialsIntoIcebergConfig() {
+ String metalakeName = "test_metalake";
+ String catalogName = "credential_catalog";
+
+ Catalog mockCatalog = Mockito.mock(Catalog.class);
+ SupportsCredentials supportsCredentials =
Mockito.mock(SupportsCredentials.class);
+ Mockito.when(mockCatalog.provider()).thenReturn("lakehouse-iceberg");
+ Mockito.when(mockCatalog.properties())
+ .thenReturn(
+ new HashMap<String, String>() {
+ {
+ put(IcebergConstants.CATALOG_BACKEND, "custom");
+ put(IcebergConstants.CATALOG_BACKEND_NAME, catalogName);
+ }
+ });
Review Comment:
Avoid double-brace initialization here; it creates an anonymous subclass and
can retain an implicit reference, making the test harder to read and
potentially leaking memory. Prefer constructing a normal map and returning it
from the stub.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java:
##########
@@ -138,6 +144,22 @@ public void initialize(
this.icebergViewCatalogOperations = new
IcebergViewCatalogOperations(icebergCatalogWrapper);
}
+ private static void injectCredentialConfig(
+ Map<String, String> conf, String catalogName, Map<String, String>
resultConf) {
+ List<Credential> credentials = new ArrayList<>();
+ CatalogCredentialContext context =
+ new CatalogCredentialContext(PrincipalUtils.getCurrentUserName());
+ try (CatalogCredentialManager credentialManager =
+ new CatalogCredentialManager(catalogName, conf)) {
+ for (String credentialProvider :
CredentialUtils.getCredentialProvidersByOrder(() -> conf)) {
+ credentialManager.getCredential(credentialProvider,
context).ifPresent(credentials::add);
Review Comment:
`CredentialUtils.getCredentialProvidersByOrder(...)` returns a `Set`
(core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java:54-76),
so provider order from configuration is not preserved. Since credential
injection can overwrite keys (e.g., S3/JDBC properties), iterating an unordered
set can make the effective credentials nondeterministic when multiple providers
are present. Prefer iterating the ordered `List<String>` from
`CredentialConfig.CREDENTIAL_PROVIDERS`.
--
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]
