yuqi1129 commented on code in PR #11264:
URL: https://github.com/apache/gravitino/pull/11264#discussion_r3377663489
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueConnectorAdapter.java:
##########
@@ -101,6 +91,19 @@ public Map<String, String> buildInternalConnectorConfig(
return config;
}
+ static void applyS3Credential(Credential[] credentials, Map<String, String>
config) {
+ for (Credential credential : credentials) {
+ if (credential instanceof S3SecretKeyCredential) {
+ S3SecretKeyCredential s3 = (S3SecretKeyCredential) credential;
+ config.put(HIVE_METASTORE_GLUE_ACCESS_KEY, s3.accessKeyId());
+ config.put(HIVE_METASTORE_GLUE_SECRET_KEY, s3.secretAccessKey());
+ config.put(HIVE_S3_ACCESS_KEY, s3.accessKeyId());
+ config.put(HIVE_S3_SECRET_KEY, s3.secretAccessKey());
+ return;
Review Comment:
Does `Glue` not support `Azure` credentials as follows?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueConnectorAdapter.java:
##########
@@ -101,6 +91,19 @@ public Map<String, String> buildInternalConnectorConfig(
return config;
}
+ static void applyS3Credential(Credential[] credentials, Map<String, String>
config) {
+ for (Credential credential : credentials) {
+ if (credential instanceof S3SecretKeyCredential) {
+ S3SecretKeyCredential s3 = (S3SecretKeyCredential) credential;
+ config.put(HIVE_METASTORE_GLUE_ACCESS_KEY, s3.accessKeyId());
+ config.put(HIVE_METASTORE_GLUE_SECRET_KEY, s3.secretAccessKey());
+ config.put(HIVE_S3_ACCESS_KEY, s3.accessKeyId());
+ config.put(HIVE_S3_SECRET_KEY, s3.secretAccessKey());
+ return;
Review Comment:
S3 is the only supported type?
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/hive/GravitinoHiveCatalog.java:
##########
@@ -89,6 +97,43 @@ public HiveConf getHiveConf() {
return hiveCatalog.getHiveConf();
}
+ @Override
+ public void open() throws CatalogException {
+ try {
+ applyS3Credential(catalog(), hiveCatalog.getHiveConf());
+ } catch (NoSuchCatalogException e) {
+ LOG.warn(
+ "Catalog '{}' not found in Gravitino during open(); credential
injection skipped."
+ + " This is expected during CREATE CATALOG.",
+ catalogName(),
+ e);
+ }
+ super.open();
+ }
+
+ static void applyS3Credential(Catalog catalog, Configuration conf) {
Review Comment:
This one is almost the same as that in `GravitinoHiveCatalog` in spark
connector.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -451,30 +458,62 @@ public Map<String, String> properties() {
}
}
}
- return properties;
+ if (!shouldBackfillCredential()) {
+ return properties;
+ }
+ Map<String, String> result = Maps.newHashMap(properties);
+ result.putAll(propertiesWithCredentialProviders());
+ return result;
}
/**
- * Retrieves the properties of the catalog including credential providers.
Subclasses should
- * override this method to inject auto-detected credential provider names
into the properties map
- * before the {@link CatalogCredentialManager} is initialized. The default
implementation returns
- * {@link #properties()} unchanged.
+ * Retrieves the properties of the catalog including credential providers.
Detects storage and
+ * catalog-specific credential providers from the raw entity properties
(including hidden ones)
+ * and injects them before {@link CatalogCredentialManager} is initialized.
Subclasses may
+ * override {@link #addCatalogSpecificCredentialProviders} to add additional
providers.
*
- * @return A map of properties including credential providers.
+ * @return A map of raw properties with credential providers set.
*/
public Map<String, String> propertiesWithCredentialProviders() {
- return properties();
+ Map<String, String> props = Maps.newHashMap(entity().getProperties());
+ if
(StringUtils.isNotBlank(props.get(CredentialConstants.CREDENTIAL_PROVIDERS))) {
+ return props;
+ }
+ List<String> credentialProviders = new ArrayList<>();
+ addCatalogSpecificCredentialProviders(props, credentialProviders);
+ if (!credentialProviders.isEmpty()) {
+ props.put(CredentialConstants.CREDENTIAL_PROVIDERS, String.join(",",
credentialProviders));
+ }
+ return props;
}
/**
- * Detects storage credential providers (S3, OSS, Azure) from catalog
properties and appends them
- * to the provided list. Subclasses can call this method in their {@link
- * #propertiesWithCredentialProviders()} implementation to avoid duplicating
storage credential
- * detection logic.
+ * Detects credential providers for this catalog type and appends them to
{@code
+ * credentialProviders}. The default implementation calls {@link
+ * #addStorageCredentialProviders(Map, List)} to detect S3/OSS/Azure/GCS
credentials. Subclasses
+ * override this to add catalog-specific providers (e.g., JDBC).
*
- * @param properties The catalog properties map to scan for storage
credentials.
- * @param credentialProviders The list to append detected storage credential
providers to.
+ * @param properties the raw catalog properties
+ * @param credentialProviders the list to append detected provider names to
*/
+ protected void addCatalogSpecificCredentialProviders(
+ Map<String, String> properties, List<String> credentialProviders) {
+ addStorageCredentialProviders(properties, credentialProviders);
+ }
+
+ /**
+ * Returns whether hidden credentials should be backfilled into catalog
properties for backward
+ * compatibility with connectors that do not support credential vending.
Controlled by
+ * server-level config {@code
gravitino.catalog.credential.backfillToProperties}.
+ *
+ * @return true if backfill is enabled
+ */
+ protected boolean shouldBackfillCredential() {
+ Config serverConfig = GravitinoEnv.getInstance().config();
Review Comment:
When will it be null?
--
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]
