Support datasource password change in runtime #1599
Conversation
| @@ -22,5 +22,6 @@ | |||
| @MicronautTest(transactional = false) | |||
| @Property(name = "jpa.default.properties.hibernate.connection.db-type", value = "mysql") | |||
| @Property(name = "jpa.default.reactive", value = "true") | |||
| @Property(name = "test-resources.containers.mysql.image-name", value = "mysql:8.4.5") | |||
There was a problem hiding this comment.
Unrelated to this PR, mysql:latest docker image was failing to start in tests so needed to update it to 8.x
| } | ||
| return dbcpDataSourcePoolMetadata; | ||
| } | ||
|
|
||
| @Override | ||
| public void onApplicationEvent(DataSourcePasswordChangedEvent event) { |
There was a problem hiding this comment.
I think instead of a new event you should extend RefreshEventListener and override getObservedConfigurationPrefixes() to get the data which will make it work in other contexts like the refresh endpoint and periodic monitoring.
For UCP you should call:
public void reconfigureDataSource(Properties configuration) throws SQLException {
For Hikari you should call:
hikariDataSource.getHikariConfigMXBean().setPassword("newPassword");
hikariDataSource.getHikariPoolMXBean().softEvictConnections();
For DBCP:
// set the properties
// and then call
pool.evict();
There was a problem hiding this comment.
I wasn't sure if I am able to populate all Properties for UCP or can only set password to properties?
Btw, I think this is not needed, since existing connections will have old password and it should work until connections are evicted by the pool and then regenerated with the new password.
As for the event, I thought this will be more convenient because users might have their own logic/events for password change and this might be simpler and more straightforward.
There was a problem hiding this comment.
it isn't about being easier it is because this change won't work from the refresh management endpoint or wither periodic config refresh mechanisms
There was a problem hiding this comment.
Ok, I guess will wait this PR micronaut-projects/micronaut-core#11798 to be merged and then for which drivers needed listen to refresh event and I suppose datasources.*.password and extract datasource name and update password if needed.
There was a problem hiding this comment.
Changed logic in refresh event listener to get new value like
String password = applicationContext.getRequiredProperty(property, String.class);
where property will be datasources.{datasourcename}.password
|
Can we test this with two datasource and validate that rerouting works? |
jdbc-ucp/src/main/java/io/micronaut/configuration/jdbc/ucp/DatasourceFactory.java
Outdated
Show resolved
Hide resolved
I have updates tests for DBCP, Hikari and Tomcat by altering default user in the test and then executing query. Something is not working as expected for UCP, investigating now. |
jdbc/src/main/java/io/micronaut/jdbc/BaseDatasourceFactory.java
Outdated
Show resolved
Hide resolved
radovanradic
added
the
type: improvement
|
Need to wait this PR micronaut-projects/micronaut-core#11798 and micronaut-core version with it be propagated before can merge this |
radovanradic
changed the title
… not set some properties
|
Is this ready now? |
UCP is behaving differently after upgrade to core 4.8.17 so I made some changes. However, currently native UCP tests failing and I am investigating how to fix it. |
radovanradic
changed the title
| @@ -51,7 +52,7 @@ | |||
| public class DatasourceConfiguration implements BasicJdbcConfiguration { | |||
| private static final Logger LOG = LoggerFactory.getLogger(DatasourceConfiguration.class); | |||
|
|
|||
| @ConfigurationBuilder(allowZeroArgs = true, excludes = {"connectionFactoryProperties"}) | |||
| @ConfigurationBuilder(allowZeroArgs = true, excludes = {"connectionFactoryProperties", "URL", "username", "password"}) | |||
There was a problem hiding this comment.
@graemerocher This is what needed to change after upgrade to core 4.8.17 (containing fix for config refresh). So, when bean.inject is called from RefreshScope since this is ConfigurationBuilder it would call setter for all PoolDataSourceImpl bean and one of these are setURL, setUsername, setPassword. But, since we have these already in DatasourceConfiguration and are calling this delegate methods I think we don't need these.
Btw, when changed password in the database and refresh event is fired, calling delegate.setURL would stop and start connection pool but password has not been propagated yet. So, excluding these resolves the issue.
And in DatasourceConfiguration.setUrl() and other methods has now check to not call delegate if property has not changed since that would also trigger connection pool stop/start.
|
github-project-automation
bot
moved this from Ready for Review
to Done
in 4.9.0 Release
This PR adds support for changing datasource password without need to restart the app. It might address some reported issues like this one.
The idea is that users publish event with datasource name and changed password so listeners would be able to update datasource passwords. Current connections created with old password are validated (tested with hikari and ucp) and only when connection expires and being recreated then new password will be used.
@sdelamo Suggested using bean refresh, but don't know how it would work - tried refreshing
DataSourceConfigurationbut that didn't have effect sinceDataSourcebeans are already created and injected with the old password and refreshingDataSourceConfigurationwon't rebuild existing datasources. Or maybe I was not aware how it could be done.