lasdf1234 commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4132234427
##########
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:
Agreed — force-appending `s3-secret-key` beside an explicit
`credential-providers` list (e.g. `s3-token`) is wrong and conflicts with the
`BaseCatalog` policy.
Glue/Paimon changes are out of scope for this PR and will be handled in a
follow-up (along with Aws/Dlf credential types). This PR restores Glue/Paimon
to match `main`, so the append is no longer part of the diff here.
##########
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:
Yes — with the current policy, an explicit `credential-providers` list does
not auto-list storage static providers (`s3-secret-key` / `oss-secret-key` / …).
For **this** PR that is intentional and safe: `getSecrets()` still returns
the cloud access-key pairs (with `USE_SECRET`), so connectors/humans are not
stranded when only `s3-token` is listed.
A follow-up that stops returning those keys from `getSecrets` will need a
scheme-aware recovery story (or require listing the static provider
explicitly). We are not changing that contract in this PR.
##########
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:
Fixed in `24f993509`.
`getSecrets()` `RESTException` (and other unexpected failures) fail-fast
again with a `TrinoException`, so a transient blip cannot leave a permanently
cached connector built from masked `******` properties. Only
`UnsupportedOperationException` / `NotFoundException` (older servers / stubs)
are skipped.
Covered by
`TestCatalogConnectorManager.testConnectorContextFailsFastOnSecretsRestException`.
The same fail-fast for `getSecrets` RESTException is applied in Spark / Flink
/ Iceberg REST for consistency; missing `/credentials` remains tolerated.
##########
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:
Fixed in `24f993509`.
Extracted `CredentialInfos.nonExpiringCredentialInfo(...)` in `api` and
wired Spark / Flink / Trino / Iceberg REST through it (skip nulls and
`expireTimeInMs != 0`). Unit coverage in `TestCredentialInfos`.
##########
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:
Same as the later Glue thread: agreed. Glue is out of scope for this PR and
restored to `main`; the force-append will be addressed in the Glue/Aws
follow-up.
--
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]
