lasdf1234 commented on code in PR #13539:
URL: https://github.com/apache/gravitino/pull/13539#discussion_r4132236110
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -133,16 +132,20 @@ private static Map<String, String> resolveProps(Catalog
catalog) {
} catch (UnsupportedOperationException ignored) {
// Catalog does not support secret property operations.
}
- if (catalog instanceof SupportsCredentials) {
- Arrays.stream(((SupportsCredentials) catalog).getCredentials())
- .filter(c -> c instanceof JdbcCredential)
- .map(c -> (JdbcCredential) c)
- .findFirst()
- .ifPresent(
- jdbc -> {
- props.put(IcebergConstants.GRAVITINO_JDBC_USER,
jdbc.jdbcUser());
- props.put(IcebergConstants.GRAVITINO_JDBC_PASSWORD,
jdbc.jdbcPassword());
- });
+ try {
+ SupportsCredentials supportsCredentials = catalog.supportsCredentials();
+ if (supportsCredentials != null) {
+ Credential[] credentials = supportsCredentials.getCredentials();
+ if (credentials != null) {
+ for (Credential credential : credentials) {
+ if (credential != null && credential.credentialInfo() != null) {
+ props.putAll(credential.credentialInfo());
Review Comment:
Fixed earlier on this branch (skip `expireTimeInMs != 0`), and consolidated
in `24f993509` via shared `CredentialInfos.nonExpiringCredentialInfo` so the
four connector merge sites stay consistent.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -1058,33 +1059,64 @@ 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 (Exception e) {
+ 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);
+ }
+ try {
+ Credential[] credentials = loaded.supportsCredentials().getCredentials();
+ if (credentials != null) {
+ for (Credential credential : credentials) {
+ if (credential != null && credential.credentialInfo() != null) {
+ properties.putAll(credential.credentialInfo());
+ }
+ }
+ }
+ } catch (UnsupportedOperationException ignored) {
+ // Catalog does not support credential vending.
+ } catch (Exception e) {
Review Comment:
Aligned with Flink for `/credentials`: `UnsupportedOperationException` /
`NotFoundException` / `RESTException` are tolerated so older servers without
the endpoint still work.
Separately, `getSecrets()` `RESTException` is fail-fast again (see the later
Trino comment) — secrets failure must not produce a cached connector with
masked properties.
##########
api/src/main/java/org/apache/gravitino/credential/CredentialPropertyKeys.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.credential;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Catalog entity property keys that also appear in {@link
Credential#credentialInfo()} and are
+ * delivered via {@link SupportsCredentials#getCredentials()}, not {@code
getSecrets()}.
+ *
+ * <p>Omits fields that exist only in vended credential payloads and are never
catalog properties
+ * (for example {@code s3-session-token}, {@code oss-security-token}, {@code
cos-security-token},
+ * {@code adls-sas-token}, GCS {@code token}, and AWS IRSA {@code
access-key-id} / {@code
+ * secret-access-key} / {@code session-token}).
+ */
+public final class CredentialPropertyKeys {
+
+ private static final Set<String> KEYS;
+
+ static {
+ Set<String> keys = new HashSet<>();
+ // S3 static pair (also reused as session AK/SK field names in s3-token
payloads)
+ keys.add(S3SecretKeyCredential.GRAVITINO_S3_STATIC_ACCESS_KEY_ID);
+ keys.add(S3SecretKeyCredential.GRAVITINO_S3_STATIC_SECRET_ACCESS_KEY);
+ // OSS static pair
+ keys.add(OSSSecretKeyCredential.GRAVITINO_OSS_STATIC_ACCESS_KEY_ID);
+ keys.add(OSSSecretKeyCredential.GRAVITINO_OSS_STATIC_SECRET_ACCESS_KEY);
+ // COS static pair
+ keys.add(COSSecretKeyCredential.GRAVITINO_COS_STATIC_ACCESS_KEY_ID);
+ keys.add(COSSecretKeyCredential.GRAVITINO_COS_STATIC_SECRET_ACCESS_KEY);
+ // Azure account key pair
+ keys.add(AzureAccountKeyCredential.GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME);
+ keys.add(AzureAccountKeyCredential.GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY);
+ // JDBC
+ keys.add(JdbcCredential.GRAVITINO_JDBC_USER);
+ keys.add(JdbcCredential.GRAVITINO_JDBC_PASSWORD);
+ // Glue AWS API credentials
+ keys.add(AwsSecretKeyCredential.GRAVITINO_AWS_ACCESS_KEY_ID);
+ keys.add(AwsSecretKeyCredential.GRAVITINO_AWS_SECRET_ACCESS_KEY);
+ // Paimon DLF (dlf-security-token is an optional catalog property, not
vended-only)
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_ACCESS_KEY_ID);
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_ACCESS_KEY_SECRET);
+ keys.add(DlfSecretKeyCredential.GRAVITINO_DLF_SECURITY_TOKEN);
+ KEYS = Collections.unmodifiableSet(keys);
Review Comment:
`CredentialPropertyKeys` was dropped from this PR when we split Aws/Dlf /
getCredentials-only recovery into a follow-up. In the current model
`getSecrets()` still returns cloud access-key pairs for `USE_SECRET`, so the
hand-maintained exclusion set is no longer part of this change set. We can
revisit an SPI sync test if/when that exclusion returns.
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:
##########
@@ -184,9 +174,23 @@ public static Map<String, String> buildSecrets(
if (key == null || value == null) {
continue;
}
- if (isSecretProperty(key, value)) {
+ if (CredentialPropertyKeys.isCredentialPropertyKey(key)) {
+ continue;
+ }
Review Comment:
That finding applied to the intermediate `CredentialPropertyKeys` exclusion
model. This PR no longer excludes those keys from `getSecrets()` (cloud pairs
remain available with `USE_SECRET`), so schema/fileset static pairs are not
stranded here. Fileset-level `getCredentials` overlay stays in the 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]
