FANNG1 commented on code in PR #10081: URL: https://github.com/apache/gravitino/pull/10081#discussion_r3224127160
########## api/src/main/java/org/apache/gravitino/credential/JdbcCredential.java: ########## @@ -0,0 +1,116 @@ +/* + * 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; + +/** JDBC credential for accessing JDBC backend services. */ +public class JdbcCredential implements Credential { + + /** JDBC credential type. */ + public static final String JDBC_CREDENTIAL_TYPE = "jdbc"; Review Comment: **[Naming] `JDBC_CREDENTIAL_TYPE = "jdbc"` is too generic.** The identifier `"jdbc"` will collide with any future JDBC credential variant (dynamic credentials, IAM-based auth, certificate-based, etc.). Consider something more specific like `jdbc-user-password` or `gravitino-jdbc` so the type name leaves room for additional JDBC credential kinds. ########## core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java: ########## @@ -445,6 +454,49 @@ public Map<String, String> properties() { return properties; } + /** + * 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. + * + * @return A map of properties including credential providers. + */ + public Map<String, String> propertiesWithCredentialProviders() { Review Comment: **[API stability] Missing stability annotation.** The helper `addStorageCredentialProviders` below is annotated `@Evolving` (good), but this new `public` extension method has no stability marker. Since it's a connector extension point that external catalog implementations will override, please mark it `@Evolving` (or `@Unstable`) to make the contract explicit. ########## catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalog.java: ########## @@ -77,4 +83,39 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio public PropertiesMetadata schemaPropertiesMetadata() throws UnsupportedOperationException { return SCHEMA_PROPERTIES_META; } + + @Override + public Map<String, String> propertiesWithCredentialProviders() { + Map<String, String> properties = Maps.newHashMap(super.propertiesWithCredentialProviders()); + return buildCredentialProvidersIfNecessary(properties); + } + + private Map<String, String> buildCredentialProvidersIfNecessary(Map<String, String> properties) { + // If credential providers already set, return as is + if (StringUtils.isNotBlank(properties.get(CredentialConstants.CREDENTIAL_PROVIDERS))) { + return properties; + } + + List<String> credentialProviders = new ArrayList<>(); + + // Add JDBC credential provider if backend is JDBC and jdbc-user/jdbc-password are set + String catalogBackend = properties.get(PaimonConstants.CATALOG_BACKEND); + if (catalogBackend != null Review Comment: **[Style] Redundant null check; inconsistent with `IcebergCatalog`.** `if (catalogBackend != null && PaimonCatalogBackend.JDBC.name().equalsIgnoreCase(catalogBackend))` — the explicit null check is unnecessary; `String#equalsIgnoreCase(null)` returns `false`. `IcebergCatalog.buildCredentialProvidersIfNecessary` uses the shorter form. Please align. ########## catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java: ########## @@ -81,4 +87,38 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio public PropertiesMetadata schemaPropertiesMetadata() throws UnsupportedOperationException { return SCHEMA_PROPERTIES_META; } + + @Override + public Map<String, String> propertiesWithCredentialProviders() { Review Comment: **[Design / breaking behavior] Implicit credential vending is opt-out.** The current implementation auto-injects `jdbc` (and S3 / OSS / Azure static-key) providers into `credential-providers` whenever the corresponding properties are present and the user has not explicitly set `credential-providers`. After upgrade, every existing Iceberg-JDBC / Paimon-JDBC / pure JDBC catalog will start vending its raw DB password to any caller holding credential-fetch privilege on the catalog. Unlike S3/OSS static keys (which are intentionally stored to be vended), the JDBC password historically was a server-side connection secret. Flipping it to user-retrievable by default is a semantic shift. Suggestion: make this **opt-in** — require the user to put `jdbc` explicitly in `credential-providers`, or add a switch such as `gravitino.credential.jdbc.vending.enabled`. At minimum, please document the behavior change in release notes and in the javadoc of `propertiesWithCredentialProviders()` / `buildCredentialProvidersIfNecessary` in all three catalogs (Iceberg / Paimon / JDBC). (Worth noting: `jdbc-password` is currently `hidden=false` in the property metadata, so it's already returned via `loadCatalog`. The two exposure paths now coexist — addressing both belongs in this PR or an immediate follow-up.) ########## catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogCredential.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.jdbc; + +import com.google.common.collect.Maps; +import java.time.Instant; +import java.util.Map; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.catalog.jdbc.config.JdbcConfig; +import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; +import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; +import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; +import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations; +import org.apache.gravitino.credential.CredentialConstants; +import org.apache.gravitino.credential.JdbcCredential; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.CatalogEntity; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** Tests for JdbcCatalog credential provider functionality. */ +public class TestJdbcCatalogCredential { + + /** A concrete implementation of JdbcCatalog for testing purposes. */ + private static class TestableJdbcCatalog extends JdbcCatalog { + + @Override + public String shortName() { + return "jdbc-test"; + } + + @Override + protected JdbcTypeConverter createJdbcTypeConverter() { + throw new UnsupportedOperationException("Not needed for credential test"); + } + + @Override + protected JdbcDatabaseOperations createJdbcDatabaseOperations() { + throw new UnsupportedOperationException("Not needed for credential test"); + } + + @Override + protected JdbcTableOperations createJdbcTableOperations() { + throw new UnsupportedOperationException("Not needed for credential test"); + } + + @Override + protected JdbcColumnDefaultValueConverter createJdbcColumnDefaultValueConverter() { + throw new UnsupportedOperationException("Not needed for credential test"); + } + } + + @Test + void testJdbcCatalogDefaultCredentialProviders() { + AuditInfo auditInfo = + AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build(); + + // Test JDBC catalog with jdbc-user and jdbc-password + Map<String, String> jdbcProps = Maps.newHashMap(); + jdbcProps.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:mysql://localhost:3306/test"); + jdbcProps.put(JdbcConfig.JDBC_DRIVER.getKey(), "com.mysql.cj.jdbc.Driver"); + jdbcProps.put(JdbcConfig.USERNAME.getKey(), "test-user"); + jdbcProps.put(JdbcConfig.PASSWORD.getKey(), "test-password"); + + CatalogEntity jdbcEntity = + CatalogEntity.builder() + .withId(1L) + .withName("jdbc-catalog") + .withNamespace(Namespace.of("metalake")) + .withType(TestableJdbcCatalog.Type.RELATIONAL) + .withProvider("jdbc-mysql") + .withAuditInfo(auditInfo) + .withProperties(jdbcProps) + .build(); + + TestableJdbcCatalog jdbcCatalog = new TestableJdbcCatalog(); + jdbcCatalog.withCatalogConf(jdbcProps).withCatalogEntity(jdbcEntity); + Map<String, String> properties = jdbcCatalog.propertiesWithCredentialProviders(); + + // Should have jdbc credential provider + String credentialProviders = properties.get(CredentialConstants.CREDENTIAL_PROVIDERS); + Assertions.assertNotNull(credentialProviders); + Assertions.assertEquals(JdbcCredential.JDBC_CREDENTIAL_TYPE, credentialProviders); + } + + @Test + void testJdbcCatalogNoCredentialProvidersWithoutPassword() { Review Comment: **[Tests] Add coverage for both `jdbc-user` and `jdbc-password` missing.** Current cases: (a) user + password present, (b) password missing, (c) explicit `credential-providers` not overridden. Missing: both `jdbc-user` and `jdbc-password` absent. Please add that negative case so the no-op default path is locked in by a test. ########## core/src/main/java/org/apache/gravitino/credential/JdbcCredentialProvider.java: ########## @@ -0,0 +1,62 @@ +/* + * 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; + +/** Generate JDBC user and password credentials to access JDBC backend services. */ +public class JdbcCredentialProvider implements CredentialProvider { + + private String jdbcUser; + private String jdbcPassword; + + @Override + public void initialize(Map<String, String> properties) { + if (properties == null) { + return; + } + // jdbcUser / jdbcPassword will be null when the catalog was not configured with JDBC + // credentials. + // getCredential() handles this by returning null (no credential available). + // Note: The property keys "jdbc-user" and "jdbc-password" match the configuration keys + // used by JDBC-based catalogs (e.g., Iceberg JDBC backend, Paimon JDBC backend). + this.jdbcUser = properties.get(JdbcCredential.GRAVITINO_JDBC_USER); Review Comment: **[Comments] Trim the explanatory comment block.** The multi-line comment above explaining what `jdbc-user` / `jdbc-password` mean repeats information already conveyed by the constant names. Suggest collapsing it to a single line noting why nulls are tolerated here (i.e. "unset when the catalog was not configured with JDBC credentials; `getCredential()` returns null in that case"). ########## catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java: ########## @@ -81,4 +87,38 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio public PropertiesMetadata schemaPropertiesMetadata() throws UnsupportedOperationException { return SCHEMA_PROPERTIES_META; } + + @Override + public Map<String, String> propertiesWithCredentialProviders() { + Map<String, String> properties = Maps.newHashMap(super.propertiesWithCredentialProviders()); + return buildCredentialProvidersIfNecessary(properties); + } + + private Map<String, String> buildCredentialProvidersIfNecessary(Map<String, String> properties) { Review Comment: **[Naming] `buildCredentialProvidersIfNecessary` is ambiguous.** The method mutates the passed-in map and short-circuits when `credential-providers` is already set. "IfNecessary" doesn't quite capture either behavior. Consider `buildCredentialProvidersIfAbsent` or `applyDefaultCredentialProviders`. Same comment applies to the equivalent helpers in `PaimonCatalog` and `JdbcCatalog`. -- 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]
