jerryshao commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4130270421
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalog.java:
##########
@@ -91,12 +93,17 @@ public Map<String, String>
propertiesWithCredentialProviders() {
// super() skips addCatalogSpecificCredentialProviders() when
credential-providers is already
// set, so the aws-* → s3-* key mapping never runs. Apply it
unconditionally here so that
// S3SecretKeyProvider.initialize() can read s3-access-key-id regardless
of how the catalog
- // was configured.
+ // was configured. Also ensure aws-secret-key is listed so Glue API keys
remain available via
+ // getCredentials after getSecrets stopped returning them.
String accessKeyId = props.get(GlueConstants.AWS_ACCESS_KEY_ID);
String secretAccessKey = props.get(GlueConstants.AWS_SECRET_ACCESS_KEY);
if (StringUtils.isNotBlank(accessKeyId) &&
StringUtils.isNotBlank(secretAccessKey)) {
props.putIfAbsent(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, accessKeyId);
props.putIfAbsent(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY,
secretAccessKey);
+ ensureCredentialProviderListed(props,
AwsSecretKeyCredential.AWS_SECRET_KEY_CREDENTIAL_TYPE);
+ // Remap creates s3-* keys; register s3-secret-key so getCredentials can
vend them even when
+ // credential-providers was already set (super skips auto-detect).
+ ensureCredentialProviderListed(props,
S3SecretKeyCredential.S3_SECRET_KEY_CREDENTIAL_TYPE);
Review Comment:
[Important] This appends `s3-secret-key` even when the operator set
`credential-providers` explicitly, which is exactly what
`core/.../BaseCatalog.java:538-540` says not to do ("do not auto-append
detected static providers (e.g. s3-secret-key beside s3-token)"). For a Glue
catalog configured the normal way — static `aws-*` keys for the Glue API plus
`credential-providers=s3-token` for data access — the provider list becomes
`s3-token,aws-secret-key,s3-secret-key`, and lines 101-102 have already
remapped the static keys into `s3-access-key-id` / `s3-secret-access-key` so
`S3SecretKeyProvider` can serve them.
The consequence is a widening of what is vended, not just a listing detail:
`CredentialOperationDispatcher.getCatalogCredentialContexts` builds one context
per listed provider and returns every non-null credential
(`CredentialOperationDispatcher.java:69-88`), `S3TokenGenerator:73-74` returns
`null` for a catalog context, so the client receives the long-lived static S3
pair where the operator asked for short-lived STS tokens — and each connector
merges it straight into engine config
(`spark-connector/.../BaseCatalog.java:759`,
`trino-connector/.../CatalogConnectorManager.java:1118`).
Line 103 (`aws-secret-key`) is all this PR's stated goal needs: it is the
Glue API credential, and it is scheme-neutral. Suggest dropping line 106 and
letting `addStorageCredentialProviders` keep owning `s3-secret-key` on the
auto-detect path only; if it really is needed when providers are explicit, gate
it on no other `s3-*` provider already being listed.
Verified by: read `GlueCatalog.propertiesWithCredentialProviders` and
`addCatalogSpecificCredentialProviders` in full at this head; traced
`ensureCredentialProviderListed` (`core/.../BaseCatalog.java:574-587`), the
early return it works around (`:536-541`), catalog-level fan-out in
`CredentialOperationDispatcher.java:69-113`, and `S3TokenGenerator.java:73`
returning null without a path context. `TestGlueCatalogCredentials.java:73-101`
pins this behaviour but with `custom-provider`, never a real s3 provider.
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/BaseCatalog.java:
##########
@@ -725,7 +732,43 @@ protected Table loadSparkTable(Identifier ident) {
private static Map<String, String> propsWithSecrets(Catalog catalog) {
Map<String, String> props =
new HashMap<>(catalog.properties() == null ? Collections.emptyMap() :
catalog.properties());
- props.putAll(catalog.supportsSecrets().getSecrets());
+ try {
+ Map<String, String> secrets = catalog.supportsSecrets().getSecrets();
+ if (secrets != null) {
+ props.putAll(secrets);
+ }
+ } catch (UnsupportedOperationException | NotFoundException e) {
+ // Stubs may not implement SupportsSecrets; older servers lack /secrets.
+ LOG.debug("Skipping getSecrets while resolving Spark catalog properties:
{}", e.toString());
+ } catch (RESTException e) {
+ LOG.warn(
+ "Failed to resolve getSecrets while building Spark catalog
properties; continuing with"
+ + " masked properties: {}",
+ e.toString());
+ }
+ try {
+ Credential[] credentials =
catalog.supportsCredentials().getCredentials();
+ if (credentials != null) {
+ for (Credential credential : credentials) {
+ // Skip expiring credentials: Spark catalog properties are fixed at
initialize time.
+ if (credential == null
+ || credential.expireTimeInMs() != 0
+ || credential.credentialInfo() == null) {
+ continue;
+ }
+ props.putAll(credential.credentialInfo());
+ }
+ }
Review Comment:
[Nit] This loop — null check, `expireTimeInMs() != 0` skip,
`putAll(credentialInfo())`, plus the same three-branch catch — is byte-for-byte
the same at four sites in this PR: here,
`flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/utils/PropertyUtils.java:101-130`,
`trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:1110-1140`,
and
`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:151-179`.
Any later change to the filtering policy (per the Question on
`core/.../BaseCatalog.java`) has to land in all four. A small shared helper —
`staticCredentialInfo(SupportsCredentials)` returning a merged map — would
collapse them and give the policy one test target instead of four.
Verified by: read all four sites at this head and diffed them by eye; the
only differences are the logger name and the log message wording.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -1058,33 +1061,86 @@ static Map<String, String> visibleProps(Catalog
catalog) {
}
/**
- * Overlays the secrets the Gravitino server vends for this catalog onto its
properties.
+ * Overlays secrets and credential info the Gravitino server vends for this
catalog onto its
+ * properties.
*
* <p>Resolved here, on the node that is about to build the connector,
rather than once at
* registration time: the registered definition travels through a CREATE
CATALOG statement that
* Trino persists as a catalog properties file, and a secret placed in it
would be readable there
- * for as long as the catalog exists.
+ * for as long as the catalog exists. Cloud/JDBC credential fields come from
{@code
+ * getCredentials()}; other secrets come from {@code getSecrets()}.
*/
private GravitinoCatalog withResolvedSecrets(
GravitinoCatalog catalog, GravitinoMetalake metalake) {
- Map<String, String> secrets;
+ Catalog loaded;
try {
- secrets =
metalake.loadCatalog(catalog.getName()).supportsSecrets().getSecrets();
+ loaded = metalake.loadCatalog(catalog.getName());
} catch (Exception e) {
- // Named explicitly: the caller's message only says the connector could
not be created, and
- // this step is the one that needs the Gravitino server reachable from
this node.
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_OPERATION_FAILED,
String.format(
"Failed to resolve the secrets of catalog %s in metalake %s: %s",
catalog.getName(), catalog.getMetalake(), toErrorMessage(e)),
e);
}
- if (secrets.isEmpty()) {
+ Map<String, String> properties = new HashMap<>(catalog.getProperties());
+ try {
+ Map<String, String> secrets = loaded.supportsSecrets().getSecrets();
+ if (secrets != null && !secrets.isEmpty()) {
+ properties.putAll(secrets);
+ }
+ } catch (UnsupportedOperationException | NotFoundException e) {
+ // Catalog may not support secrets, or older servers lack /secrets.
+ LOG.debug(
+ "Skipping getSecrets for catalog %s in metalake %s: %s",
+ catalog.getName(), catalog.getMetalake(), e.toString());
+ } catch (RESTException e) {
+ LOG.warn(
+ "Failed to resolve getSecrets for catalog %s in metalake %s;
continuing with masked"
+ + " properties: %s",
+ catalog.getName(), catalog.getMetalake(), e.toString());
Review Comment:
[Important] Swallowing `RESTException` from `getSecrets()` turns a fail-fast
path into a silently broken, cached connector. Before this change any failure
here became a `TrinoException` naming this step; now a transport failure or 5xx
only logs a WARN, and control falls through to
`createCatalogConnectorContextBuilder` /
`catalogConnectors.put(fullCatalogName, connectorContext)` at lines 997-1002.
The connector is therefore built with the masked values from
`catalog.getProperties()` — `jdbc-password=******` — and cached. Since
registration *succeeded*, the refresh loop will not rebuild it
(`testUnchangedCatalogIsNotReRegistered`), so one transient blip leaves that
Trino catalog failing every query with an authentication error until the
catalog definition changes or Trino restarts.
The back-compat case this is meant to cover is already handled by the
`NotFoundException` branch on line 1092. Suggest either rethrowing
`RESTException` for `getSecrets` (keeping the tolerant behaviour for
`getCredentials` at line 1136, where the endpoint genuinely may not exist), or,
if it must stay tolerant, not caching a context that was built from incomplete
properties.
Verified by: read `withResolvedSecrets` and `createCatalogConnectorContext`
in full at this head (the `catalogConnectors.put` on line 1002 is what makes it
sticky); `properties` starts from the masked `catalog.getProperties()` on line
1086. `LOG` is `io.airlift.log.Logger` (line 24/69), so the `%s` placeholders
are right. The new `testConnectorContextToleratesMissingCredentialsEndpoint`
asserts the build proceeds but not what the cached connector then contains.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -535,6 +535,9 @@ public Map<String, String>
propertiesWithCredentialProviders() {
props = Maps.newHashMap(secretManager.toPlaintextProperties(props));
}
if
(StringUtils.isNotBlank(props.get(CredentialConstants.CREDENTIAL_PROVIDERS))) {
+ // Explicit credential-providers wins: do not auto-append detected
static providers (e.g.
+ // s3-secret-key beside s3-token), which breaks path-based credential
selection. Catalogs that
+ // must keep jdbc/aws/dlf listed call ensureCredentialProviderListed in
their overrides.
Review Comment:
[Question] This policy means storage pairs are never listed once
`credential-providers` is set: the four catalogs that need it re-list `jdbc` /
`aws` / `dlf` in their own overrides, but nothing re-lists `s3-secret-key`,
`oss-secret-key`, `cos-secret-key` or `azure-account-key`. So for a catalog
carrying, say, `credential-providers=s3-token` plus a static S3 pair,
`getCredentials()` never returns that pair.
That is harmless in this PR, because the pair is still in `getSecrets()` —
`SecretPropertyUtils` is untouched here. But the stacked PR's job is to stop
`getSecrets()` returning these keys, and at that point the recovery path this
PR is supposed to establish does not exist for that configuration: the client
gets `******` and no credential. Is the intent that the stacked PR keeps
storage keys in `getSecrets()` whenever providers are explicit, or that such
catalogs are expected to drop `s3-token`? Worth writing down here, since this
comment is the only record of the decision.
Verified by: read `propertiesWithCredentialProviders` and
`addStorageCredentialProviders` in full at this head; confirmed the only
`ensureCredentialProviderListed` callers are `JdbcCatalog.java:168`,
`IcebergCatalog.java:123`, `PaimonCatalog.java:107,113` and
`GlueCatalog.java:103,106` (none for oss/cos/azure), and confirmed
`core/.../secret/SecretPropertyUtils.java` is not in this diff.
--
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]
