Copilot commented on code in PR #11149:
URL: https://github.com/apache/gravitino/pull/11149#discussion_r3286572237
##########
core/src/main/java/org/apache/gravitino/Configs.java:
##########
@@ -521,6 +521,18 @@ private Configs() {}
+ " and must not contain '.' or the internal physical
separator (\\u0001)")
.createWithDefault(":");
+ public static final ConfigEntry<Boolean>
CATALOG_CREDENTIAL_BACKFILL_TO_PROPERTIES =
+ new ConfigBuilder("gravitino.catalog.credential.backfillToProperties")
+ .doc(
+ "If true, the server exposes hidden catalog credentials (such as
jdbc-user and "
+ + "jdbc-password) in the catalog properties response. Enable
only during a "
+ + "rolling upgrade while old connectors that do not support
credential vending "
+ + "are still in use. Enabling this is a security risk
because credentials "
+ + "become visible to anyone who can read catalog
properties.")
+ .version(ConfigConstants.VERSION_0_9_0)
Review Comment:
The new config entry `gravitino.catalog.credential.backfillToProperties` is
documented as introduced in 1.3.0 (see docs), but the
`ConfigBuilder.version(...)` here is set to `ConfigConstants.VERSION_0_9_0`.
This will incorrectly report the config’s availability/version (e.g., in
generated config docs or tooling). Update the version to the release that
actually introduces this config (likely `ConfigConstants.VERSION_1_3_0`).
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorContext.java:
##########
@@ -255,7 +258,13 @@ public CatalogConnectorContext build() throws Exception {
Preconditions.checkArgument(catalog != null, "catalog must not be null");
Preconditions.checkArgument(context != null, "context must not be null");
Preconditions.checkArgument(config != null, "config must not be null");
- Map<String, String> connectorConfig =
connectorAdapter.buildInternalConnectorConfig(catalog);
+ Catalog liveCatalog = metalake.loadCatalog(catalog.getName());
+ Credential[] credentials =
+ liveCatalog instanceof SupportsCredentials
+ ? ((SupportsCredentials) liveCatalog).getCredentials()
+ : new Credential[0];
+ Map<String, String> connectorConfig =
Review Comment:
Credential vending support is detected via `Catalog#supportsCredentials()`
(which may throw `UnsupportedOperationException`), not by `instanceof
SupportsCredentials`. Using `instanceof` couples this code to current client
implementations and can incorrectly skip credential vending if a `Catalog`
supports it via `supportsCredentials()` but doesn’t implement
`SupportsCredentials` directly. Prefer calling
`liveCatalog.supportsCredentials().getCredentials()` and catching
`UnsupportedOperationException` to fall back to an empty credential list.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/jdbc/GravitinoJdbcCatalog.java:
##########
@@ -74,4 +92,26 @@ protected AbstractCatalog realCatalog() {
public Optional<Factory> getFactory() {
return Optional.of(new JdbcDynamicTableFactory());
}
+
+ /**
+ * Overwrites the Flink JDBC user and password in {@code options} with
credentials obtained from
+ * the server via credential vending, if available. Falls back to the
existing options if the
+ * catalog does not support credential vending or no JDBC credential is
returned.
+ *
+ * @param catalog the Gravitino catalog client
+ * @param options the mutable Flink catalog options map to update
+ */
+ static void applyJdbcCredential(Catalog catalog, Map<String, String>
options) {
+ if (!(catalog instanceof SupportsCredentials)) {
+ return;
+ }
+ for (Credential credential : ((SupportsCredentials)
catalog).getCredentials()) {
+ if (credential instanceof JdbcCredential) {
Review Comment:
Credential vending support should be checked via
`Catalog#supportsCredentials()` rather than `instanceof SupportsCredentials`.
The intended capability check in the public API is `supportsCredentials()`;
using `instanceof` is tied to current implementations and can miss support if
the implementation only overrides `supportsCredentials()`. Prefer calling
`catalog.supportsCredentials().getCredentials()` and catching
`UnsupportedOperationException` to fall back to existing options.
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/jdbc/GravitinoJdbcCatalog.java:
##########
@@ -44,10 +48,34 @@ protected TableCatalog createAndInitSparkCatalog(
JDBCTableCatalog jdbcTableCatalog = new JDBCTableCatalog();
Map<String, String> all =
getPropertiesConverter().toSparkCatalogProperties(options, properties);
+ applyJdbcCredential(gravitinoCatalogClient, all);
jdbcTableCatalog.initialize(name, new CaseInsensitiveStringMap(all));
return jdbcTableCatalog;
}
+ /**
+ * Overwrites the Spark JDBC user and password in {@code sparkProperties}
with credentials
+ * obtained from the server via credential vending, if available.
+ *
+ * @param catalog the Gravitino catalog client; must implement {@link
SupportsCredentials} for
+ * credential vending to take effect
+ * @param sparkProperties the mutable Spark properties map to update
+ */
+ static void applyJdbcCredential(Catalog catalog, Map<String, String>
sparkProperties) {
+ if (!(catalog instanceof SupportsCredentials)) {
+ return;
+ }
+ for (Credential credential : ((SupportsCredentials)
catalog).getCredentials()) {
+ if (credential instanceof JdbcCredential) {
Review Comment:
Credential vending support should be checked via
`Catalog#supportsCredentials()` rather than `instanceof SupportsCredentials`.
The API contract for capability discovery is `supportsCredentials()` (throwing
`UnsupportedOperationException` when not supported); relying on `instanceof` is
more brittle if catalog implementations change. Consider using `try { for
(Credential c : catalog.supportsCredentials().getCredentials()) ... } catch
(UnsupportedOperationException e) { return; }`.
--
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]
