allow hikari password changes at runtime #5308

berler · 2021-03-09T22:46:54Z

Goals (and why): Many customers have password policies that require regular password rotation for Oracle database users (often every 60 days). Currently since the password is in static config, this requires restarting services that use AtlasDB or rely on the Hikari pool. Ideally we should support being able to change user passwords without requiring services restart.

Implementation Description (bullets):

Hikari internally already supports changing the password of a pool at runtime without needing to shutdown or reinitialize the pool (it also supports changing the username, but we won't allow that for now).
Adds a setPassword method on the ConnectionManager interface which can be used to change the password of the pool. Note that the implementation here needs to do something different depending on if the pool is already initialized or not. Also note that the way hikari implements this, changing the password (or username) means that it would now copy the JDBC properties object every time a new connection is called. This hurts performance very slightly, but only for new connections (reusing an existing connection is not affected). The way I've implemented it here, you only pay this performance cost if you actually call setPassword at runtime (existing consumers will not be affected).
(NOT YET IMPLEMENTED) A future change can add a runtime config for postgres and oracle so that the hikari pool setPassword method gets called whenever the password changes in runtime config. This is not necessary for many of our internal products though, because internal products often do not use the normal atlas config (they have their own completely independent install/runtime configs).

Testing (What was existing testing like? What have you done to improve it?): Added tests against postgres. Also fixed the tests (HikariCpConnectionManagerTest was never being run because it was not in a suite, and the setup for that class was broken because it didn't actually wait for the database to be available which meant that even running it manually would have failed).

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday): normal

This adds a method to the hikari connection manager so that the password can be changed at runtime. This is a requirement to support being able to rotate database passwords without needing to restart services. Also fixes a bug that made HikariCpConnectionManagerTest not actually run (because it was not in a suite). The test itself was actually broken because it did not wait for the DB to be ready (the suite setup correctly makes sure that it can connect to the DB before running tests).

changelog-app · 2021-03-09T22:46:58Z

Generate changelog in `changelog/@unreleased`

Type

Description

allow hikari password changes at runtime

Goals (and why): Many customers have password policies that require regular password rotation for Oracle database users (often every 60 days). Currently since the password is in static config, this requires restarting services that use AtlasDB or rely on the Hikari pool. Ideally we should support being able to change user passwords without requiring services restart.

Implementation Description (bullets):

Hikari internally already supports changing the password of a pool at runtime without needing to shutdown or reinitialize the pool (it also supports changing the username, but we won't allow that for now).
Adds a setPassword method on the ConnectionManager interface which can be used to change the password of the pool. Note that the implementation here needs to do something different depending on if the pool is already initialized or not. Also note that the way hikari implements this, changing the password (or username) means that it would now copy the JDBC properties object every time a new connection is called. This hurts performance very slightly, but only for new connections (reusing an existing connection is not affected). The way I've implemented it here, you only pay this performance cost if you actually call setPassword at runtime (existing consumers will not be affected).
(NOT YET IMPLEMENTED) A future change can add a runtime config for postgres and oracle so that the hikari pool setPassword method gets called whenever the password changes in runtime config. This is not necessary for many of our internal products though, because internal products often do not use the normal atlas config (they have their own completely independent install/runtime configs).

Testing (What was existing testing like? What have you done to improve it?): Added tests against postgres. Also fixed the tests (HikariCpConnectionManagerTest was never being run because it was not in a suite, and the setup for that class was broken because it didn't actually wait for the database to be available which meant that even running it manually would have failed).

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday): normal

Check the box to generate changelog(s)

Generate changelog entry

Jolyon-S

I've done a preliminary review; @jeremyk-91 will give his own pass here (I'm trying to familiarise myself with parts of the codebase I don't know, so apologies if I've misunderstood this codepath at all).

Jolyon-S · 2021-03-11T16:58:55Z

atlasdb-dbkvs-hikari/src/main/java/com/palantir/nexus/db/pool/HikariCPConnectionManager.java

+        if (localState.type == StateType.NORMAL) {
+            // pool is initialized, need to set password directly on the pool
+            internalSetPassword(newPassword, localState.dataSourcePool);


I'm not that familiar with this codepath, but what happens if the state changes after this if check? Why don't you synchronise here?

Equally, what happens (and can it happen?) if there is a race to change the password - is it possible that the username gets set to null but the password is set, or vice versa?

This doesn't need to be synchronized because the only other possible state change is closing the pool. That is completely fine to happen concurrently because all this does is update a volatile field on the pool (it is fine to update that concurrently/after the pool is closed).

Also it is completely fine for the username to be null while the password is not null (the password field would just be ignored in that case which means it will use the original configured password). This would only happen with a specific execution order of concurrent calls where one setPassword passes in the original password and another passes in a different password.

So consider the following two concurrent calls where the static config has the password set as "originalPassword":

setPassword("originalPassword")

setPassword("newPassword")

The possible outcomes of this are as follows for the username/password override fields:

username = null; password = null; => "originalPassword" used for connections

username = null; password = "newPassword"; => "originalPassword" used for connections

username = "username"; password = "newPassword"; => "newPassword" used for connections (JDBC parameters copied for all new connections)

username = "username"; password = null; => "originalPassword" used for connections (although in this case the JDBC parameters are copied for all new connections)

Regardless of the outcome, the behavior is arguably correct (you can say one of the calls "wins" and that is what the end behavior is). There is also no point in time where any password would be used other than one of the passwords from the concurrent calls.

In practice there should not be concurrent calls to this. The only caller should be a subscription on the witchcraft Runtime config object. Witchcraft uses a single task on a schedule using scheduleWithFixedDelay to check the runtime config and calls the subscribers on the same thread (so no Runtime config subscription calls will ever overlap).

The only thing we need to really worry about is other threads that may call init() or close(), but that is handled correctly by the current code which checks the state.

Jolyon-S · 2021-03-11T17:01:50Z

atlasdb-dbkvs-hikari/src/main/java/com/palantir/nexus/db/pool/HikariCPConnectionManager.java

+        if (localState.type == StateType.ZERO) {
+            synchronized (this) {
+                // need to check again in case init just happened
+                localState = state;
+                if (localState.type == StateType.ZERO) {


While I believe that this is fine (since the only State with type set to ZERO has all nulls, and so checking the type is equivalent to checking the whole object), the method checkAndGetDataSourcePool performs a similar operation in the sense that it switches against the possible states that we can be in, and notably it compares the whole object vs just the type.

To err on the side of caution, can we follow the structure used there? In other words:

use a switch statement

compare state against localState directly

I thought about that, but that is actually suboptimal (and would result in needing to write duplicate code). When you examine the state again (while being synchronized) if the state happens to be the normal state, you would actually want to have the same behavior as what is outside of the synchronized block). It would also mean that you potentially need to add a third volatile read instead of what is written here where it is guaranteed a max of two volatile memory reads.

I'd hope that the perf of this method isn't that critical (though now that you've written this more efficient impl it makes sense, of course)!

Jolyon-S · 2021-03-11T17:03:22Z

atlasdb-dbkvs-hikari/src/main/java/com/palantir/nexus/db/pool/HikariCPConnectionManager.java

+     * <p>Hikari checks the username field for being null to determine if it needs to copy the JDBC parameters, so
+     * the username *must* be set for it to respect the password change.
+     */
+    private void internalSetPassword(String newPassword, HikariConfig configOrPool) {


As above, I'm a little concerned about concurrency (but I might be being paranoid here).

Jolyon-S · 2021-03-11T17:06:55Z

atlasdb-dbkvs-tests/src/test/java/com/palantir/nexus/db/pool/HikariCpConnectionManagerTest.java

@@ -141,6 +130,103 @@ public void testCloseIdempotent() throws SQLException {
        manager.close(); // shouldn't throw
    }

+    @Test
+    public void testRuntimePasswordChange() throws SQLException {


From looking at the base class and the tests, it seems rather difficult to write a test that confirms that attempts to concurrently edit the password don't cause an issue; still, if at all possible, it'd be quite interesting to see.

I'm not sure that a test would be able to confirm correctness unless you somehow can control the execution order on different threads and iterate through all possible execution orders.

I think it's fine to have undefined behavior as the only caller to this should be a witchcraft Runtime subscription which is guaranteed to never have multiple concurrent calls.

If someone does end up having concurrent calls and ends up in an unexpected state, either the password is correct or not. If the password is not correct, all connections would fail and they'd probably need to restart the service (or have something which triggers calling setPassword again later). For this case I would argue the bug is in the service which made concurrent calls to the setPassword method.

yeah, in sync here that trying to setPassword concurrently (as long as we're not doing it) shouldn't be allowed. Maybe we could stick a @NotThreadSafe or some javadocs there noting that that's a bad idea?

Jolyon-S · 2021-03-12T14:57:02Z

Seems reasonable in light of what you've said - thanks for providing additional context around these methods and changes.

I'll leave it to @jeremyk-91 for the +2 before we merge.

Thanks!

jeremyk-91

👍
Broadly looks good. I'd suggest some minor docs changes, mainly to tell people more explicitly to avoid calling setPassword() concurrently, but the impl looks solid.

jeremyk-91 · 2021-03-15T17:19:10Z

atlasdb-dbkvs-hikari/src/main/java/com/palantir/nexus/db/pool/HikariCPConnectionManager.java

+    private void internalSetPassword(String newPassword, HikariConfig configOrPool) {
+        if (newPassword.equals(connConfig.getDbPassword().unmasked())) {
+            configOrPool.setUsername(null);
+            configOrPool.setPassword(null);


thanks for the comment above, that explains this part well

jeremyk-91 · 2021-03-15T17:26:30Z

atlasdb-dbkvs-tests/src/test/java/com/palantir/nexus/db/pool/HikariCpConnectionManagerTest.java

@@ -141,6 +130,103 @@ public void testCloseIdempotent() throws SQLException {
        manager.close(); // shouldn't throw
    }

+    @Test
+    public void testRuntimePasswordChange() throws SQLException {


yeah, in sync here that trying to setPassword concurrently (as long as we're not doing it) shouldn't be allowed. Maybe we could stick a @NotThreadSafe or some javadocs there noting that that's a bad idea?

jeremyk-91 · 2021-03-15T17:26:48Z

commons-db/src/main/java/com/palantir/nexus/db/pool/ConnectionManager.java

+     * Change the password which will be used for new connections. Existing connections are not modified in any way.
+     *
+     * <p>Note that if the password is changed at runtime, there is a slight negative impact on pool performance
+     * because Hikari will copy the JDBC parameters every time a new connection is made.


yeah let's say concurrent calls to this method = UB

jeremyk-91 · 2021-03-15T17:30:26Z

atlasdb-dbkvs-hikari/src/main/java/com/palantir/nexus/db/pool/HikariCPConnectionManager.java

+        if (localState.type == StateType.ZERO) {
+            synchronized (this) {
+                // need to check again in case init just happened
+                localState = state;
+                if (localState.type == StateType.ZERO) {


I'd hope that the perf of this method isn't that critical (though now that you've written this more efficient impl it makes sense, of course)!

svc-autorelease · 2021-03-15T18:13:46Z

Released 0.309.3

berler requested review from jeremyk-91 and gsheasby March 9, 2021 22:47

Add generated changelog entries

1719a45

berler force-pushed the hikari-runtime-password-changes branch from 984bdb4 to 1719a45 Compare March 9, 2021 22:49

gsheasby removed their request for review March 10, 2021 08:50

make runtime password change test more reliable

d527dd5

Jolyon-S reviewed Mar 11, 2021

View reviewed changes

berler requested a review from Jolyon-S March 12, 2021 01:58

jeremyk-91 approved these changes Mar 15, 2021

View reviewed changes

add javadoc about concurrent calls

26e70b6

berler added merge when ready autorelease labels Mar 15, 2021

bulldozer-bot bot merged commit 33330e0 into develop Mar 15, 2021

bulldozer-bot bot deleted the hikari-runtime-password-changes branch March 15, 2021 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow hikari password changes at runtime #5308

allow hikari password changes at runtime #5308

berler commented Mar 9, 2021

changelog-app bot commented Mar 9, 2021 •

edited by berler

Loading

Jolyon-S left a comment

Jolyon-S Mar 11, 2021

Jolyon-S Mar 11, 2021

berler Mar 12, 2021

berler Mar 12, 2021

Jolyon-S Mar 11, 2021

berler Mar 12, 2021

jeremyk-91 Mar 15, 2021

Jolyon-S Mar 11, 2021

Jolyon-S Mar 11, 2021

berler Mar 12, 2021

jeremyk-91 Mar 15, 2021

Jolyon-S commented Mar 12, 2021

jeremyk-91 left a comment

jeremyk-91 Mar 15, 2021

jeremyk-91 Mar 15, 2021

jeremyk-91 Mar 15, 2021

jeremyk-91 Mar 15, 2021

svc-autorelease commented Mar 15, 2021

allow hikari password changes at runtime #5308

allow hikari password changes at runtime #5308

Conversation

berler commented Mar 9, 2021

changelog-app bot commented Mar 9, 2021 • edited by berler Loading

Generate changelog in changelog/@unreleased

Jolyon-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jolyon-S commented Mar 12, 2021

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-autorelease commented Mar 15, 2021

changelog-app bot commented Mar 9, 2021 •

edited by berler

Loading

Generate changelog in `changelog/@unreleased`