Skip to content

Commit

Permalink
fix JENKINS-69375: ensure properties are passed to Hikari (#504)
Browse files Browse the repository at this point in the history
* JENKINS-69375 ensure properties are passed to Hikari

The generic properties must be passed to Hikari in the constructor of `HikariConfig` otherwise they are not fully applied in the expected way. To maintain backward compatibility, I've also added the former `dsConfig.setDataSourceProperties(p)`, so that the properties are also still fully available as data source properties.

* add unit test

Co-authored-by: Benoit GUERIN <[email protected]>
  • Loading branch information
amandel and bguerin authored Aug 28, 2022
1 parent 13c90ec commit 3bad1be
Show file tree
Hide file tree
Showing 2 changed files with 219 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import javax.sql.DataSource;
import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
Expand Down Expand Up @@ -258,39 +257,9 @@ public synchronized PipelineMavenPluginDao getDao() {
jdbcPassword = Secret.toString(jdbcCredentials.getPassword());
}

HikariConfig dsConfig = new HikariConfig();
dsConfig.setJdbcUrl(jdbcUrl);
dsConfig.setUsername(jdbcUserName);
dsConfig.setPassword(jdbcPassword);
HikariConfig dsConfig = createHikariConfig(properties, jdbcUrl, jdbcUserName, jdbcPassword);
dsConfig.setAutoCommit(false);

Properties p = new Properties();
// todo refactor the DAO to inject config defaults in the DAO
if (jdbcUrl.startsWith("jdbc:mysql")) {
// https://github.com/brettwooldridge/HikariCP#configuration-knobs-baby
// https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration
p.setProperty("dataSource.cachePrepStmts", "true");
p.setProperty("dataSource.prepStmtCacheSize", "250");
p.setProperty("dataSource.prepStmtCacheSqlLimit", "2048");
p.setProperty("dataSource.useServerPrepStmts", "true");
p.setProperty("dataSource.useLocalSessionState", "true");
p.setProperty("dataSource.rewriteBatchedStatements", "true");
p.setProperty("dataSource.cacheResultSetMetadata", "true");
p.setProperty("dataSource.cacheServerConfiguration", "true");
p.setProperty("dataSource.elideSetAutoCommits", "true");
p.setProperty("dataSource.maintainTimeStats", "false");
} else if (jdbcUrl.startsWith("jdbc:postgresql")) {
// no tuning recommendations found for postgresql
} else if (jdbcUrl.startsWith("jdbc:h2")) {
// dsConfig.setDataSourceClassName("org.h2.jdbcx.JdbcDataSource"); don't specify the datasource due to a classloading issue
} else {
// unsupported config
}
if (StringUtils.isNotBlank(properties)) {
p.load(new StringReader(properties));
}
dsConfig.setDataSourceProperties(p);

// TODO cleanup this quick fix for JENKINS-54587, we should have a better solution with the JDBC driver loaded by the DAO itself
try {
DriverManager.getDriver(jdbcUrl);
Expand Down Expand Up @@ -324,7 +293,7 @@ public synchronized PipelineMavenPluginDao getDao() {
}
}

LOGGER.log(Level.INFO, "Connect to database {0} with username {1} and properties {2}", new Object[]{jdbcUrl, jdbcUserName, p});
LOGGER.log(Level.INFO, "Connect to database {0} with username {1}", new Object[]{jdbcUrl, jdbcUserName});
DataSource ds = new HikariDataSource(dsConfig);

Class<? extends PipelineMavenPluginDao> daoClass;
Expand All @@ -346,7 +315,7 @@ public synchronized PipelineMavenPluginDao getDao() {
}


} catch (RuntimeException | SQLException | IOException e) {
} catch (RuntimeException | SQLException e) {
LOGGER.log(Level.WARNING, "Exception creating database dao, skip", e);
dao = new PipelineMavenPluginNullDao();
}
Expand Down Expand Up @@ -452,20 +421,8 @@ public FormValidation doValidateJdbcConnection(
jdbcUserName = jdbcCredentials.getUsername();
jdbcPassword = Secret.toString(jdbcCredentials.getPassword());
}
HikariConfig dsConfig = new HikariConfig();
dsConfig.setJdbcUrl(jdbcUrl);
dsConfig.setUsername(jdbcUserName);
dsConfig.setPassword(jdbcPassword);

if (StringUtils.isNotBlank(properties)) {
Properties p = new Properties();
try {
p.load(new StringReader(properties));
} catch (IOException e) {
throw new IllegalStateException(e);
}
dsConfig.setDataSourceProperties(p);
}
HikariConfig dsConfig = createHikariConfig(properties, jdbcUrl, jdbcUserName, jdbcPassword);

try (HikariDataSource ds = new HikariDataSource(dsConfig)) {
try (Connection cnn = ds.getConnection()) {
Expand Down Expand Up @@ -579,6 +536,47 @@ public FormValidation doValidateJdbcConnection(

}

private static HikariConfig createHikariConfig(String properties, String jdbcUrl, String jdbcUserName, String jdbcPassword) {
Properties p = new Properties();
// todo refactor the DAO to inject config defaults in the DAO
if (jdbcUrl.startsWith("jdbc:mysql")) {
// https://github.com/brettwooldridge/HikariCP#configuration-knobs-baby
// https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration
p.setProperty("dataSource.cachePrepStmts", "true");
p.setProperty("dataSource.prepStmtCacheSize", "250");
p.setProperty("dataSource.prepStmtCacheSqlLimit", "2048");
p.setProperty("dataSource.useServerPrepStmts", "true");
p.setProperty("dataSource.useLocalSessionState", "true");
p.setProperty("dataSource.rewriteBatchedStatements", "true");
p.setProperty("dataSource.cacheResultSetMetadata", "true");
p.setProperty("dataSource.cacheServerConfiguration", "true");
p.setProperty("dataSource.elideSetAutoCommits", "true");
p.setProperty("dataSource.maintainTimeStats", "false");
} else if (jdbcUrl.startsWith("jdbc:postgresql")) {
// no tuning recommendations found for postgresql
} else if (jdbcUrl.startsWith("jdbc:h2")) {
// dsConfig.setDataSourceClassName("org.h2.jdbcx.JdbcDataSource"); don't specify the datasource due to a classloading issue
} else {
// unsupported config
}

if (StringUtils.isNotBlank(properties)) {
try {
p.load(new StringReader(properties));
} catch (IOException e) {
throw new IllegalStateException("Failed to read properties.", e);
}
}
LOGGER.log(Level.INFO, "Applied pool properties {0}", p);
HikariConfig dsConfig = new HikariConfig(p);
dsConfig.setJdbcUrl(jdbcUrl);
dsConfig.setUsername(jdbcUserName);
dsConfig.setPassword(jdbcPassword);
// to mimic the old behaviour pre JENKINS-69375 fix
dsConfig.setDataSourceProperties(p);
return dsConfig;
}

@Terminator
public synchronized void closeDatasource() {
if (dao != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
package org.jenkinsci.plugins.pipeline.maven;

import static com.cloudbees.plugins.credentials.CredentialsScope.GLOBAL;
import static java.util.Arrays.asList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.hamcrest.core.IsInstanceOf.instanceOf;

import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.sql.Connection;
import java.util.List;

import org.acegisecurity.Authentication;
import org.jenkinsci.plugins.pipeline.maven.dao.CustomTypePipelineMavenPluginDaoDecorator;
import org.jenkinsci.plugins.pipeline.maven.dao.MonitoringPipelineMavenPluginDaoDecorator;
import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginDao;
import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginH2Dao;
import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginMySqlDao;
import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginPostgreSqlDao;
import org.junit.ClassRule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.testcontainers.containers.MySQLContainer;
import org.testcontainers.containers.PostgreSQLContainer;

import com.cloudbees.plugins.credentials.Credentials;
import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
import com.mysql.cj.conf.RuntimeProperty;
import com.mysql.cj.jdbc.ConnectionImpl;
import com.zaxxer.hikari.HikariDataSource;
import com.zaxxer.hikari.pool.HikariProxyConnection;

import hudson.ExtensionList;
import hudson.model.ItemGroup;
import jenkins.model.Jenkins;

public class GlobalPipelineMavenConfigTest {

@ClassRule
public static MySQLContainer<?> MYSQL_DB = new MySQLContainer<>(MySQLContainer.NAME).withUsername("aUser")
.withPassword("aPass");

@ClassRule
public static PostgreSQLContainer<?> POSTGRE_DB = new PostgreSQLContainer<>(PostgreSQLContainer.IMAGE)
.withUsername("aUser").withPassword("aPass");

@ClassRule
public static JenkinsRule j = new JenkinsRule();

private static class FakeCredentialsProvider extends CredentialsProvider {
public FakeCredentialsProvider() {
}

@Override
public boolean isEnabled(Object context) {
return true;
}

@Override
public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup itemGroup,
Authentication authentication, List<DomainRequirement> domainRequirements) {
return (List<C>) asList(new UsernamePasswordCredentialsImpl(GLOBAL, "credsId", "", "aUser", "aPass"));
}

@Override
public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup itemGroup,
Authentication authentication) {
return getCredentials(type, itemGroup, authentication, null);
}
}

private GlobalPipelineMavenConfig config = new GlobalPipelineMavenConfig();

@Test
public void shouldBuildH2Dao() throws Exception {
PipelineMavenPluginDao dao = config.getDao();

assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
Object innerDao = getField(dao, "delegate");
assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
innerDao = getField(innerDao, "delegate");
assertThat(innerDao, instanceOf(PipelineMavenPluginH2Dao.class));
}

@Test
public void shouldBuildMysqlDao() throws Exception {
ExtensionList<CredentialsProvider> extensionList = Jenkins.getInstance()
.getExtensionList(CredentialsProvider.class);
extensionList.add(extensionList.size(), new FakeCredentialsProvider());
config.setJdbcUrl(MYSQL_DB.getJdbcUrl());
config.setJdbcCredentialsId("credsId");
config.setProperties("maxLifetime=42000");

PipelineMavenPluginDao dao = config.getDao();

assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
Object innerDao = getField(dao, "delegate");
assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
innerDao = getField(innerDao, "delegate");
assertThat(innerDao, instanceOf(PipelineMavenPluginMySqlDao.class));
Object ds = getField(innerDao, "ds");
assertThat(ds, instanceOf(HikariDataSource.class));
HikariDataSource datasource = (HikariDataSource) ds;
assertThat(datasource.getJdbcUrl(), equalTo(MYSQL_DB.getJdbcUrl()));
assertThat(datasource.getUsername(), equalTo("aUser"));
assertThat(datasource.getPassword(), equalTo("aPass"));
assertThat(datasource.getMaxLifetime(), is(42000L));
assertThat(datasource.getDataSourceProperties().containsKey("dataSource.cachePrepStmts"), is(true));
assertThat(datasource.getDataSourceProperties().getProperty("dataSource.cachePrepStmts"), is("true"));
assertThat(datasource.getDataSourceProperties().containsKey("dataSource.prepStmtCacheSize"), is(true));
assertThat(datasource.getDataSourceProperties().getProperty("dataSource.prepStmtCacheSize"), is("250"));
Connection connection = datasource.getConnection();
assertThat(connection, instanceOf(HikariProxyConnection.class));
connection = (Connection) getField(connection, "delegate");
assertThat(connection, instanceOf(ConnectionImpl.class));
RuntimeProperty<Boolean> cachePrepStmts = (RuntimeProperty<Boolean>) getField(connection, "cachePrepStmts");
assertThat(cachePrepStmts.getValue(), is(true));
}

@Test
public void shouldBuildPostgresqlDao() throws Exception {
ExtensionList<CredentialsProvider> extensionList = Jenkins.getInstance()
.getExtensionList(CredentialsProvider.class);
extensionList.add(extensionList.size(), new FakeCredentialsProvider());
config.setJdbcUrl(POSTGRE_DB.getJdbcUrl());
config.setJdbcCredentialsId("credsId");

PipelineMavenPluginDao dao = config.getDao();

assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
Object innerDao = getField(dao, "delegate");
assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
innerDao = getField(innerDao, "delegate");
assertThat(innerDao, instanceOf(PipelineMavenPluginPostgreSqlDao.class));
Object ds = getField(innerDao, "ds");
assertThat(ds, instanceOf(HikariDataSource.class));
HikariDataSource datasource = (HikariDataSource) ds;
assertThat(datasource.getJdbcUrl(), equalTo(POSTGRE_DB.getJdbcUrl()));
assertThat(datasource.getUsername(), equalTo("aUser"));
assertThat(datasource.getPassword(), equalTo("aPass"));
}

private Object getField(Object targetObject, String name) throws Exception {
Class<?> targetClass = targetObject.getClass();
Field field = findField(targetClass, name, null);
makeAccessible(field);
return field.get(targetObject);
}

private Field findField(Class<?> clazz, String name, Class<?> type) {
Class<?> searchType = clazz;
while (Object.class != searchType && searchType != null) {
Field[] fields = searchType.getDeclaredFields();
for (Field field : fields) {
if ((name == null || name.equals(field.getName())) && (type == null || type.equals(field.getType()))) {
return field;
}
}
searchType = searchType.getSuperclass();
}
return null;
}

private void makeAccessible(Field field) {
if ((!Modifier.isPublic(field.getModifiers()) || !Modifier.isPublic(field.getDeclaringClass().getModifiers())
|| Modifier.isFinal(field.getModifiers())) && !field.isAccessible()) {
field.setAccessible(true);
}
}
}

0 comments on commit 3bad1be

Please sign in to comment.