From 41a22c1792a277ce2ef08e0226b0112446c01045 Mon Sep 17 00:00:00 2001 From: Paul Latzelsperger <43503240+paullatzelsperger@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:40:10 +0200 Subject: [PATCH] feat: read database config from vault (#4129) * feat: read database config from vault * DEPENDENCIES * DEPENDENCIES * improve log message --- DEPENDENCIES | 6 +- .../sql-pool-apache-commons/build.gradle.kts | 1 + ...CommonsConnectionPoolServiceExtension.java | 84 +++++++++---------- ...onsConnectionPoolServiceExtensionTest.java | 81 +++++++++++++----- 4 files changed, 106 insertions(+), 66 deletions(-) diff --git a/DEPENDENCIES b/DEPENDENCIES index 1e4e2ca6c67..db2c108abbd 100644 --- a/DEPENDENCIES +++ b/DEPENDENCIES @@ -226,11 +226,11 @@ maven/mavencentral/org.apiguardian/apiguardian-api/1.1.2, Apache-2.0, approved, maven/mavencentral/org.assertj/assertj-core/3.25.3, Apache-2.0, approved, #12585 maven/mavencentral/org.awaitility/awaitility/4.2.1, Apache-2.0, approved, #14178 maven/mavencentral/org.bouncycastle/bcpkix-jdk18on/1.72, MIT, approved, #3789 -maven/mavencentral/org.bouncycastle/bcpkix-jdk18on/1.78, MIT, approved, #14235 +maven/mavencentral/org.bouncycastle/bcpkix-jdk18on/1.78, MIT, approved, #14434 maven/mavencentral/org.bouncycastle/bcprov-jdk18on/1.72, MIT AND CC0-1.0, approved, #3538 -maven/mavencentral/org.bouncycastle/bcprov-jdk18on/1.78, MIT AND CC0-1.0, approved, #14237 +maven/mavencentral/org.bouncycastle/bcprov-jdk18on/1.78, MIT AND CC0-1.0, approved, #14433 maven/mavencentral/org.bouncycastle/bcutil-jdk18on/1.72, MIT, approved, #3790 -maven/mavencentral/org.bouncycastle/bcutil-jdk18on/1.78, MIT, approved, #14238 +maven/mavencentral/org.bouncycastle/bcutil-jdk18on/1.78, MIT, approved, #14435 maven/mavencentral/org.ccil.cowan.tagsoup/tagsoup/1.2.1, Apache-2.0, approved, clearlydefined maven/mavencentral/org.checkerframework/checker-qual/3.12.0, MIT, approved, clearlydefined maven/mavencentral/org.checkerframework/checker-qual/3.42.0, MIT, approved, clearlydefined diff --git a/extensions/common/sql/sql-pool/sql-pool-apache-commons/build.gradle.kts b/extensions/common/sql/sql-pool/sql-pool-apache-commons/build.gradle.kts index 3bd4e464b48..42f4fbffdb7 100644 --- a/extensions/common/sql/sql-pool/sql-pool-apache-commons/build.gradle.kts +++ b/extensions/common/sql/sql-pool/sql-pool-apache-commons/build.gradle.kts @@ -30,6 +30,7 @@ dependencies { // required for statically mocking the JDBC DriverManager testImplementation(libs.mockito.inline) + testImplementation(project(":core:common:lib:boot-lib")) //in-mem vault } diff --git a/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/main/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtension.java b/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/main/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtension.java index b399b2afd34..ce88f2f3d2e 100644 --- a/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/main/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtension.java +++ b/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/main/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtension.java @@ -17,7 +17,9 @@ import org.eclipse.edc.runtime.metamodel.annotation.Extension; import org.eclipse.edc.runtime.metamodel.annotation.Inject; import org.eclipse.edc.runtime.metamodel.annotation.Setting; +import org.eclipse.edc.spi.EdcException; import org.eclipse.edc.spi.monitor.Monitor; +import org.eclipse.edc.spi.security.Vault; import org.eclipse.edc.spi.system.ServiceExtension; import org.eclipse.edc.spi.system.ServiceExtensionContext; import org.eclipse.edc.spi.system.configuration.Config; @@ -25,18 +27,20 @@ import org.eclipse.edc.sql.datasource.ConnectionFactoryDataSource; import org.eclipse.edc.sql.datasource.ConnectionPoolDataSource; import org.eclipse.edc.transaction.datasource.spi.DataSourceRegistry; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Properties; import java.util.function.BiFunction; import java.util.function.Consumer; +import java.util.function.Supplier; import javax.sql.DataSource; -import static java.lang.String.format; +import static java.util.Optional.ofNullable; @Extension(value = CommonsConnectionPoolServiceExtension.NAME) public class CommonsConnectionPoolServiceExtension implements ServiceExtension { @@ -44,50 +48,23 @@ public class CommonsConnectionPoolServiceExtension implements ServiceExtension { public static final String NAME = "Commons Connection Pool"; public static final String EDC_DATASOURCE_PREFIX = "edc.datasource"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_MAX_IDLE_CONNECTIONS = "pool.maxIdleConnections"; public static final String POOL_CONNECTIONS_MAX_IDLE = "pool.connections.max-idle"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_MAX_TOTAL_CONNECTIONS = "pool.maxTotalConnections"; public static final String POOL_CONNECTIONS_MAX_TOTAL = "pool.connections.max-total"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_MIN_IDLE_CONNECTIONS = "pool.minIdleConnections"; public static final String POOL_CONNECTIONS_MIN_IDLE = "pool.connections.min-idle"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_TEST_CONNECTION_ON_BORROW = "pool.testConnectionOnBorrow"; public static final String POOL_CONNECTION_TEST_ON_BORROW = "pool.connection.test.on-borrow"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_TEST_CONNECTION_ON_CREATE = "pool.testConnectionOnCreate"; public static final String POOL_CONNECTION_TEST_ON_CREATE = "pool.connection.test.on-create"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_TEST_CONNECTION_ON_RETURN = "pool.testConnectionOnReturn"; public static final String POOL_CONNECTION_TEST_ON_RETURN = "pool.connection.test.on-return"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_TEST_CONNECTION_WHILE_IDLE = "pool.testConnectionWhileIdle"; public static final String POOL_CONNECTION_TEST_WHILE_IDLE = "pool.connection.test.while-idle"; - @Deprecated(since = "0.3.1") - @Setting(required = false) - public static final String DEPRACATED_POOL_TEST_QUERY = "pool.testQuery"; public static final String POOL_CONNECTION_TEST_QUERY = "pool.connection.test.query"; - public static final Map CONFIGURATION_MAPPING = Map.of( - POOL_CONNECTIONS_MAX_IDLE, DEPRACATED_POOL_MAX_IDLE_CONNECTIONS, - POOL_CONNECTIONS_MIN_IDLE, DEPRACATED_POOL_MIN_IDLE_CONNECTIONS, - POOL_CONNECTIONS_MAX_TOTAL, DEPRACATED_POOL_MAX_TOTAL_CONNECTIONS, - POOL_CONNECTION_TEST_ON_BORROW, DEPRACATED_POOL_TEST_CONNECTION_ON_BORROW, - POOL_CONNECTION_TEST_ON_CREATE, DEPRACATED_POOL_TEST_CONNECTION_ON_CREATE, - POOL_CONNECTION_TEST_ON_RETURN, DEPRACATED_POOL_TEST_CONNECTION_ON_RETURN, - POOL_CONNECTION_TEST_WHILE_IDLE, DEPRACATED_POOL_TEST_CONNECTION_WHILE_IDLE, - POOL_CONNECTION_TEST_QUERY, DEPRACATED_POOL_TEST_QUERY); @Setting(required = true) public static final String URL = "url"; + + @Setting(value = "Username to be used for the JDBC connection. Can be omitted if not required, or if the user is encoded in the JDBC url.") + public static final String USER = "user"; + @Setting(value = "Username to be used for the JDBC connection. Can be omitted if not required, or if the password is encoded in the JDBC url.") + public static final String PASSWORD = "password"; + private final List commonsConnectionPools = new LinkedList<>(); @Inject private DataSourceRegistry dataSourceRegistry; @@ -98,6 +75,9 @@ public class CommonsConnectionPoolServiceExtension implements ServiceExtension { @Inject private ConnectionFactory connectionFactory; + @Inject + private Vault vault; + @Override public String name() { return NAME; @@ -127,6 +107,14 @@ public List getCommonsConnectionPools() { return commonsConnectionPools; } + private @NotNull Supplier<@Nullable String> readFromConfig(Config config, String value) { + return () -> { + var entry = EDC_DATASOURCE_PREFIX + "." + config.currentNode() + "." + value; + monitor.warning("Database configuration value '%s' not found in vault, will fall back to Config. Please consider putting database configuration into the vault.".formatted(entry)); + return config.getString(value, null); + }; + } + private void setIfProvidedString(String key, Consumer setter, Config config) { setIfProvided(key, setter, config::getString); } @@ -140,12 +128,7 @@ private void setIfProvidedInt(String key, Consumer setter, Config confi } private void setIfProvided(String key, Consumer setter, BiFunction getter) { - var oldKey = CONFIGURATION_MAPPING.get(key); - var oldValue = getter.apply(oldKey, null); - if (oldValue != null) { - monitor.warning(format("Configuration setting %s has been deprecated, please use %s instead", oldKey, key)); - } - var value = getter.apply(key, oldValue); + var value = getter.apply(key, null); if (value != null) { setter.accept(value); } @@ -165,11 +148,28 @@ private Map createConnectionPools(Config parent) } private DataSource createDataSource(Config config) { - var jdbcUrl = Objects.requireNonNull(config.getString(URL)); + var rootPath = EDC_DATASOURCE_PREFIX + "." + config.currentNode(); + + // read values from the vault first, fall back to config + var urlProperty = rootPath + "." + URL; + var jdbcUrl = ofNullable(vault.resolveSecret(urlProperty)).orElseGet(readFromConfig(config, URL)); + + if (jdbcUrl == null) { + throw new EdcException("Mandatory config '%s' not found. Please provide a value for the '%s' property, either as a secret in the vault or an application property.".formatted(urlProperty, urlProperty)); + } + + var jdbcUser = ofNullable(vault.resolveSecret(rootPath + "." + USER)) + .orElseGet(readFromConfig(config, USER)); + var jdbcPassword = ofNullable(vault.resolveSecret(rootPath + "." + PASSWORD)) + .orElseGet(readFromConfig(config, PASSWORD)); var properties = new Properties(); properties.putAll(config.getRelativeEntries()); + // only set if not-null, otherwise Properties#add throws a NPE + ofNullable(jdbcUser).ifPresent(u -> properties.put(USER, u)); + ofNullable(jdbcPassword).ifPresent(p -> properties.put(PASSWORD, p)); + return new ConnectionFactoryDataSource(connectionFactory, jdbcUrl, properties); } diff --git a/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/test/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtensionTest.java b/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/test/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtensionTest.java index 680d0c1b86c..84804734df1 100644 --- a/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/test/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtensionTest.java +++ b/extensions/common/sql/sql-pool/sql-pool-apache-commons/src/test/java/org/eclipse/edc/sql/pool/commons/CommonsConnectionPoolServiceExtensionTest.java @@ -15,12 +15,17 @@ package org.eclipse.edc.sql.pool.commons; import org.assertj.core.api.ThrowingConsumer; +import org.eclipse.edc.boot.vault.InMemoryVault; import org.eclipse.edc.junit.extensions.DependencyInjectionExtension; +import org.eclipse.edc.spi.EdcException; +import org.eclipse.edc.spi.security.Vault; import org.eclipse.edc.spi.system.ServiceExtensionContext; import org.eclipse.edc.spi.system.configuration.Config; import org.eclipse.edc.spi.system.configuration.ConfigFactory; +import org.eclipse.edc.sql.ConnectionFactory; import org.eclipse.edc.transaction.datasource.spi.DataSourceRegistry; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.jupiter.params.ParameterizedTest; @@ -33,14 +38,7 @@ import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_MAX_IDLE_CONNECTIONS; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_MAX_TOTAL_CONNECTIONS; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_MIN_IDLE_CONNECTIONS; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_TEST_CONNECTION_ON_BORROW; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_TEST_CONNECTION_ON_CREATE; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_TEST_CONNECTION_ON_RETURN; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_TEST_CONNECTION_WHILE_IDLE; -import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.DEPRACATED_POOL_TEST_QUERY; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.EDC_DATASOURCE_PREFIX; import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.POOL_CONNECTIONS_MAX_IDLE; import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.POOL_CONNECTIONS_MAX_TOTAL; @@ -51,6 +49,7 @@ import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.POOL_CONNECTION_TEST_QUERY; import static org.eclipse.edc.sql.pool.commons.CommonsConnectionPoolServiceExtension.POOL_CONNECTION_TEST_WHILE_IDLE; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -61,10 +60,13 @@ class CommonsConnectionPoolServiceExtensionTest { private static final String DS_1_NAME = "ds1"; private final DataSourceRegistry dataSourceRegistry = mock(); + private final ConnectionFactory connectionFactory = mock(); @BeforeEach void setUp(ServiceExtensionContext context) { context.registerService(DataSourceRegistry.class, dataSourceRegistry); + context.registerService(Vault.class, new InMemoryVault(mock())); + context.registerService(ConnectionFactory.class, connectionFactory); } @ParameterizedTest @@ -89,6 +91,55 @@ void initialize_withConfig(Map configuration, ThrowingConsumer { + assertThatThrownBy(pool::getConnection).isInstanceOf(EdcException.class); //we need this only to invoke the connection factory + verify(connectionFactory).create(eq("jdbc://whatever"), argThat(p -> + p.size() == 3 && + p.containsValue("test-pwd") && + p.containsValue("test-user"))); + }); + } + + @Test + void initialize_fromVault_shouldOverrideConfig(CommonsConnectionPoolServiceExtension extension, ServiceExtensionContext context) { + when(context.getConfig(EDC_DATASOURCE_PREFIX)) + .thenReturn(ConfigFactory.fromMap( + Map.of("ds1.name", "ds1", + "ds1.user", "this-should-be-ignored", + "ds1.password", "this-as-well"))); + var vault = context.getService(Vault.class); + + vault.storeSecret("edc.datasource." + DS_1_NAME + ".user", "test-user"); + vault.storeSecret("edc.datasource." + DS_1_NAME + ".password", "test-pwd"); + vault.storeSecret("edc.datasource." + DS_1_NAME + ".url", "jdbc://whatever"); + + extension.initialize(context); + + verify(dataSourceRegistry).register(eq(DS_1_NAME), any()); + assertThat(extension.getCommonsConnectionPools()).hasSize(1).first() + .satisfies(pool -> { + assertThatThrownBy(pool::getConnection).isInstanceOf(EdcException.class); //we need this only to invoke the connection factory + verify(connectionFactory).create(eq("jdbc://whatever"), argThat(p -> + p.size() == 3 && + p.containsValue("test-pwd") && + p.containsValue("test-user"))); + }); + } + static class ConfigProvider implements ArgumentsProvider { @@ -106,17 +157,6 @@ static class ConfigProvider implements ArgumentsProvider { DS_1_NAME + "." + POOL_CONNECTIONS_MAX_TOTAL, "10"); - private final Map deprecatedConfig = Map.of( - DS_1_NAME + ".url", DS_1_NAME, - DS_1_NAME + "." + DEPRACATED_POOL_TEST_CONNECTION_ON_CREATE, "false", - DS_1_NAME + "." + DEPRACATED_POOL_TEST_CONNECTION_ON_BORROW, "false", - DS_1_NAME + "." + DEPRACATED_POOL_TEST_CONNECTION_ON_RETURN, "true", - DS_1_NAME + "." + DEPRACATED_POOL_TEST_CONNECTION_WHILE_IDLE, "true", - DS_1_NAME + "." + DEPRACATED_POOL_TEST_QUERY, "SELECT foo FROM bar;", - DS_1_NAME + "." + DEPRACATED_POOL_MIN_IDLE_CONNECTIONS, "10", - DS_1_NAME + "." + DEPRACATED_POOL_MAX_IDLE_CONNECTIONS, "10", - DS_1_NAME + "." + DEPRACATED_POOL_MAX_TOTAL_CONNECTIONS, "10"); - @Override public Stream provideArguments(ExtensionContext context) { ThrowingConsumer checkDefault = this::checkDefault; @@ -128,8 +168,7 @@ public Stream provideArguments(ExtensionContext context) { return Stream.of( Arguments.of(defaultConfig, checkDefault, false), Arguments.of(configuration, checkWithConfig, false), - Arguments.of(envConfiguration, checkWithConfig, true), - Arguments.of(deprecatedConfig, checkWithConfig, false) + Arguments.of(envConfiguration, checkWithConfig, true) ); }