lasdf1234 commented on code in PR #13599:
URL: https://github.com/apache/gravitino/pull/13599#discussion_r4140915351
##########
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 / s3-secret-key stay listed
so Glue API keys and
+ // remapped storage keys remain available via getCredentials.
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:
Fixed in a95f083b5. When `credential-providers` is set explicitly, Glue now
only ensures `aws-secret-key` and no longer force-appends `s3-secret-key`, so
`s3-token` is preserved.
##########
catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogCredentials.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.catalog.glue;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.time.Instant;
+import java.util.Map;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.credential.AwsSecretKeyCredential;
+import org.apache.gravitino.credential.CredentialConstants;
+import org.apache.gravitino.credential.S3SecretKeyCredential;
+import org.apache.gravitino.meta.AuditInfo;
+import org.apache.gravitino.meta.CatalogEntity;
+import org.apache.gravitino.storage.S3Properties;
+import org.junit.jupiter.api.Test;
+
+public class TestGlueCatalogCredentials {
+
+ @Test
+ void testAwsKeysAutoDetectAwsAndS3Providers() {
+ Map<String, String> props =
+ Map.of(
+ GlueConstants.AWS_ACCESS_KEY_ID,
+ "AKIA",
+ GlueConstants.AWS_SECRET_ACCESS_KEY,
+ "secret",
+ GlueConstants.AWS_REGION,
+ "us-east-1");
+
+ CatalogEntity entity =
+ CatalogEntity.builder()
+ .withId(1L)
+ .withName("glue")
+ .withNamespace(Namespace.of("metalake"))
+ .withType(Catalog.Type.RELATIONAL)
+ .withProvider("glue")
+ .withProperties(props)
+ .withAuditInfo(
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
+ .build();
+
+ GlueCatalog catalog = new
GlueCatalog().withCatalogEntity(entity).withCatalogConf(props);
+ Map<String, String> withProviders =
catalog.propertiesWithCredentialProviders();
+
+ String providers =
withProviders.get(CredentialConstants.CREDENTIAL_PROVIDERS);
+
assertTrue(providers.contains(AwsSecretKeyCredential.AWS_SECRET_KEY_CREDENTIAL_TYPE));
+
assertTrue(providers.contains(S3SecretKeyCredential.S3_SECRET_KEY_CREDENTIAL_TYPE));
+ assertEquals("AKIA",
withProviders.get(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID));
+ assertEquals("secret",
withProviders.get(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY));
+ assertEquals("AKIA", withProviders.get(GlueConstants.AWS_ACCESS_KEY_ID));
+ }
+
+ @Test
+ void testExplicitCredentialProvidersStillGetsAwsAndS3() {
Review Comment:
Fixed in a95f083b5. Added
`testPropertiesWithCredentialProvidersPreserveS3Token`, which sets
`credential-providers=s3-token` and asserts the exact provider list
(`s3-token,aws-secret-key`) via split rather than `contains()`.
##########
api/src/main/java/org/apache/gravitino/credential/AwsSecretKeyCredential.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * Static AWS access-key credential for Glue (and similar) API authentication.
+ *
+ * <p>Distinct from {@link S3SecretKeyCredential}: credential-info keys are
{@code
+ * aws-access-key-id} / {@code aws-secret-access-key}, matching Glue catalog
properties.
+ */
+public class AwsSecretKeyCredential implements Credential {
+
+ /** AWS secret-key credential type. */
+ public static final String AWS_SECRET_KEY_CREDENTIAL_TYPE = "aws-secret-key";
+ /** AWS access key ID. */
+ public static final String GRAVITINO_AWS_ACCESS_KEY_ID = "aws-access-key-id";
+ /** AWS secret access key. */
+ public static final String GRAVITINO_AWS_SECRET_ACCESS_KEY =
"aws-secret-access-key";
+
+ private String accessKeyId;
+ private String secretAccessKey;
+
+ /**
+ * Constructs an {@link AwsSecretKeyCredential}.
+ *
+ * @param accessKeyId the AWS access key ID
+ * @param secretAccessKey the AWS secret access key
+ */
+ public AwsSecretKeyCredential(String accessKeyId, String secretAccessKey) {
+ validate(accessKeyId, secretAccessKey, 0);
+ this.accessKeyId = accessKeyId;
+ this.secretAccessKey = secretAccessKey;
+ }
+
+ /** Used by the credential factory. */
+ public AwsSecretKeyCredential() {}
+
+ @Override
+ public String credentialType() {
+ return AWS_SECRET_KEY_CREDENTIAL_TYPE;
+ }
+
+ @Override
+ public long expireTimeInMs() {
+ return 0;
+ }
+
+ @Override
+ public Map<String, String> credentialInfo() {
+ return new ImmutableMap.Builder<String, String>()
+ .put(GRAVITINO_AWS_ACCESS_KEY_ID, accessKeyId)
+ .put(GRAVITINO_AWS_SECRET_ACCESS_KEY, secretAccessKey)
+ .build();
+ }
+
+ @Override
+ public void initialize(Map<String, String> credentialInfo, long
expireTimeInMs) {
+ String accessKeyId = credentialInfo.get(GRAVITINO_AWS_ACCESS_KEY_ID);
+ String secretAccessKey =
credentialInfo.get(GRAVITINO_AWS_SECRET_ACCESS_KEY);
+ validate(accessKeyId, secretAccessKey, expireTimeInMs);
+ this.accessKeyId = accessKeyId;
+ this.secretAccessKey = secretAccessKey;
+ }
+
+ /**
+ * Returns the AWS access key ID.
+ *
+ * @return access key ID
+ */
+ public String accessKeyId() {
+ return accessKeyId;
+ }
+
+ /**
+ * Returns the AWS secret access key.
+ *
+ * @return secret access key
+ */
+ public String secretAccessKey() {
+ return secretAccessKey;
+ }
+
+ @Override
+ public String toString() {
Review Comment:
Fixed in a95f083b5. Dropped the custom `toString()` overrides on both
`AwsSecretKeyCredential` and `DlfSecretKeyCredential`, matching
`S3SecretKeyCredential` / `OSSSecretKeyCredential`.
##########
core/src/main/java/org/apache/gravitino/credential/AwsSecretKeyCredentialProvider.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Generates static AWS access-key credentials for Glue API authentication. */
+public class AwsSecretKeyCredentialProvider implements CredentialProvider {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(AwsSecretKeyCredentialProvider.class);
+
+ private String accessKeyId;
+ private String secretAccessKey;
+
+ @Override
+ public void initialize(Map<String, String> properties) {
+ if (properties == null) {
+ return;
+ }
+ this.accessKeyId =
properties.get(AwsSecretKeyCredential.GRAVITINO_AWS_ACCESS_KEY_ID);
+ this.secretAccessKey =
properties.get(AwsSecretKeyCredential.GRAVITINO_AWS_SECRET_ACCESS_KEY);
+ if (StringUtils.isNotBlank(accessKeyId) ^
StringUtils.isNotBlank(secretAccessKey)) {
+ LOG.warn(
Review Comment:
Fixed in a95f083b5. Both `AwsSecretKeyCredentialProvider` and
`DlfSecretKeyCredentialProvider` now use `Preconditions.checkArgument` for the
incomplete AK/SK pair case (still return null when both are absent), aligned
with `S3SecretKeyProvider`.
##########
api/src/main/java/org/apache/gravitino/credential/DlfSecretKeyCredential.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * Static Alibaba Cloud DLF (Data Lake Formation) access-key credential for
Paimon DLF catalogs.
+ *
+ * <p>Credential-info keys match Gravitino Paimon catalog properties: {@code
dlf-access-key-id},
+ * {@code dlf-access-key-secret}, and optionally {@code dlf-security-token}.
Connectors map these to
+ * Paimon REST keys {@code dlf.access-key-id}, {@code dlf.access-key-secret},
and {@code
+ * dlf.security-token}.
+ */
+public class DlfSecretKeyCredential implements Credential {
+
+ /** DLF secret-key credential type. */
+ public static final String DLF_SECRET_KEY_CREDENTIAL_TYPE = "dlf-secret-key";
+ /** DLF access key ID. */
+ public static final String GRAVITINO_DLF_ACCESS_KEY_ID = "dlf-access-key-id";
+ /** DLF access key secret. */
+ public static final String GRAVITINO_DLF_ACCESS_KEY_SECRET =
"dlf-access-key-secret";
+ /** Optional DLF security token. */
+ public static final String GRAVITINO_DLF_SECURITY_TOKEN =
"dlf-security-token";
+
+ private String accessKeyId;
+ private String accessKeySecret;
+ @Nullable private String securityToken;
+
+ /**
+ * Constructs a {@link DlfSecretKeyCredential} without a security token.
+ *
+ * @param accessKeyId the DLF access key ID
+ * @param accessKeySecret the DLF access key secret
+ */
+ public DlfSecretKeyCredential(String accessKeyId, String accessKeySecret) {
+ this(accessKeyId, accessKeySecret, null);
+ }
+
+ /**
+ * Constructs a {@link DlfSecretKeyCredential}.
+ *
+ * @param accessKeyId the DLF access key ID
+ * @param accessKeySecret the DLF access key secret
+ * @param securityToken optional security token
+ */
+ public DlfSecretKeyCredential(
+ String accessKeyId, String accessKeySecret, @Nullable String
securityToken) {
+ validate(accessKeyId, accessKeySecret, 0);
+ this.accessKeyId = accessKeyId;
+ this.accessKeySecret = accessKeySecret;
+ this.securityToken = securityToken;
Review Comment:
Fixed in a95f083b5. Documented on `expireTimeInMs()` that
`dlf-security-token` is passed through as-is and never refreshed; users who
need refreshable tokens should use `dlf-token-loader`.
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java:
##########
@@ -88,8 +89,35 @@ public PropertiesMetadata schemaPropertiesMetadata() throws
UnsupportedOperation
}
/**
- * Adds a JDBC credential provider when the backend is JDBC and credentials
are configured, then
- * delegates to the parent for storage (S3/OSS/Azure/GCS) credential
provider detection.
+ * Ensures JDBC / DLF providers stay listed even when {@code
credential-providers} was set
+ * explicitly. {@code super} skips {@link
#addCatalogSpecificCredentialProviders} in that case,
+ * and those keys are no longer recovered via {@code getSecrets}.
Review Comment:
Fixed in a95f083b5. Corrected the comment to explain that explicit
`credential-providers` skips auto-detection (so JDBC/DLF would not be vended
via `getCredentials`), and extracted the shared JDBC/DLF detection into
`detectJdbcAndDlfProviders`.
##########
catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestPaimonCatalog.java:
##########
@@ -390,11 +423,11 @@ void testJdbcBackendWithAzureCredentialProviders() {
}
@Test
- void testExplicitCredentialProvidersNotOverridden() {
+ void testExplicitCredentialProvidersStillGetsJdbc() {
Review Comment:
Fixed in a95f083b5. Restored an unchanged-list case (`assertEquals`), added
JDBC + DLF combined coverage, and updated the PR description to call out the
explicit-provider append behavior (aligned with `JdbcCatalog` /
`IcebergCatalog`).
--
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]
