Add way to change the credentials of a connection pool without recreating the whole pool object (secret rotation)

[jankatins]

We have the requirement to rotate the PG credentials every x days. Currently this means that we have to restart the service or at least replace the connection pool everywhere.

It would be nice if one could add a small helper thread which would look if the mounted credential have been changed and if so, would update the conninfo in the central connection pool and the pool would then drain the connection and replace them with new ones.

It would be even nicer, if one could give the pool a way to query for the conninfo and the pool would do that from time to time.

If there are already recommended ways: it would be nice to give some recipe in the docs, at least my google skills didn't let anything show up :-(

[dvarrazzo]

Hello @jankatins

the use case is not covered, but I can see its usefulness, thank you for detailing it.

I assume we can add a method like pool.set_conninfo(conninfo: str="", *, kwargs: dict | None = None) which would:

• replace conninfo and kwargs set up in the constructor
• remove every connection currently in the pool
• create an adequate number of connection to fill back the pool using the new conninfo
• upon old connection returned to the pool, they would be closed and replaced with a new connection
• reading the conninfo and kwargs attributes on the pool can be part of the documented and stable interface.

I'm not convinced to make the pool active with a callback to poll for infrastructure change. I think it's the program's responsibility to push for changes, furthermore providing and documenting a method to call is much easier than accepting a callback (libraries are easier than frameworks).

The work above is relatively simple... except closing the connections currently served when they are returned. It might require more changes than the rest, because at the moment the pool doesn't have a link to the connections that were given away and that therefore must be invalidated. We should either keep a map of them or annotate them in a way that will mark the need to close them on putconn.

All in all I am ok with introducing these changes but I'm not sure about the bandwidth I have, for other works I am doing at the moment and because soonish I would like to wrap up with psycopg 3.2. Are you able to provide a MR? Alternatively can your company fund this work?

[jankatins]

Ok, most of this looks straight forward. For the "expiring" of old connections:

I see that each connection is "marked" with the pool. One idea would be add a "version" in the pool which would simply increase in case the conninfo is updated. In a new _mark_connection(self, conn), we would both add a the conn._pool = self and a conn._pool_conninfo_version = self._conninfo_version. In _check_pool_putconn(conn) we would check the version, if it is different, we woul set conn._expire_at = monotonic()-1 and then the normal process would remove it.

Another idea would be simply not do anything: given that the default expiry of connections is about one hour, I would assume that old credentials are valid that long.

Re "callback to poll for changes": one nice thing is that the ConnectionPool infra already has all the things to do it, we would just need to supply a function and schedule it. But I guess letting the user do this is also fine.

Would any of these ideas be acceptable? If so I would create a MR for this.

[dvarrazzo]

I see that each connection is "marked" with the pool. One idea would be add a "version" in the pool which would simply increase in case the conninfo is updated

Yes, that's an option but, as we are refactoring this code, I'm considering whether adding a dictionary with the "given" connections (mapping id(conn) -> conn) wouldn't be a good refactoring, and to stop adding the pool reference to the connection. The mapping would be adequate to answer the question "does this connection belong to this pool" and I remember in the past someone finding the extra info added to the connection problematic (I think they were trying to use the psycopg 3 pool with psycopg 2 connection, which is not an awful idea).

Maintaining this mapping would be done in the same critical sections where _pool is manipulated. At a glance just:

• in getconn():

psycopg/psycopg_pool/psycopg_pool/pool_async.py

Line 296 in 558cbe7

conn = self._pool.popleft()

• in putconn():

psycopg/psycopg_pool/psycopg_pool/pool_async.py

Line 775 in 558cbe7

self._pool.append(conn)

Another idea would be simply not do anything: given that the default expiry of connections is about one hour, I would assume that old credentials are valid that long.

I don't know about that, for instance if someone uses this feature because they want to rotate credentials immediately to switch to a different server.

[jankatins]

Maintaining this mapping would be done in the same critical sections where _pool is manipulated.

Sounds good, let me try to build a PR for this.

(might take a week longer or so: I'm currently moving flats)