knex: expirationChecker won't recreate connection object with Postgres client

Environment

Knex version: 0.20.3 Database + version: psql (PostgreSQL) 12.1 OS: macOS 10.14.6

Bug

The expirationChecker property on the connection object is not being called to recreate the connection object with new credentials for the database, even though the timeout was reached.

The first time that a request is being made, it works fine but after the timeout, it won’t allow to make subsequent connections to the database returning a permission_denined error (as it should). Here’s an example of the knex instance being made.

const database = knex({
  ...configuration,
  connection: async () => {
    const { timeout, ...credentials } = await vault.psqlCredentials('my-role');

    const connection = {
      ...configuration.connection,
      ...credentials,
      expirationChecker: () => timeout <= Date.now(),
      ssl: yn(get(configuration, 'connection.ssl', false))
    };

    return connection;
  }
});

It came to my attention that the psql config interface does not have an expirationChecker property in it but, I’m not sure if this has a direct impact with this situation since the function that would update the connection seems to be called on the first time.

About this issue

Original URL
State: open
Created 5 years ago
Reactions: 9
Comments: 35 (9 by maintainers)

Most upvoted comments

@julrich1 Sounds like a good approach then.

kibertoad on Aug 20, 2021

Resurrecting this thread again. Based on the discussion above it looks like the solution provided in this solution would be ideal fix. can we please get this reviewed and merged to the master, if its not already fixed in some other PR? This is indeed a blocker for a project I’m working on.

amarsiingh on Sep 19, 2022

The proposed change makes use of the existing expirationChecker field from the Knex configuration object (which currently does nothing) and only uses it if it is set, unless users are setting that expirationChecker property, there would be no behavior change for them.

julrich1 on Aug 20, 2021

Knex, people are doing quite heavy lifting in that async connection function, expecting it to be called very rarely or even only once. In that respect this could be considered a breaking change of sorts.

If there is too much heavy lifting done, then they should add cache to it.

In that respect this could be considered a breaking change of sorts.

I thought knex didn’t allow passing function to connection object yet. If it already is like that then yeah, it will be a breaking change, but still easy to fix with any memoize helper.

elhigu on Jun 30, 2021

Hey, digging this issue up again. I have pretty much the same use-case with rotating creds. Currently the only reliable way I’ve found to expire connections using stale creds is to .destroy() and .initialize() the connection pool.

I think the proposed solution three comments above is a very reasonable one, and shouldn’t introduce any unexpected new logic. If expirationChecker returns true, the implication there is that the configuration which was loaded is no longer valid, and I’m not sure if there is a situation in which that would not mean connections using that config are no longer valid.

Let’s turn this into a PR?

sprusr on Jun 30, 2021

Maybe… more people should read through those connection parts of the code base to make sure that it is a valid solution… I don’t know when I would have time to concentrate on that.

elhigu on Jan 24, 2020

Oh sorry, I thought you meant as if that could solve it.

Could this function be called upon the validation of a connection?

This way the connection could be destroyed and also the connection would be rebuilt at that point. This way it would prevent any type of timer.

I suppose the property connectionConfigExpirationChecker is set to true by default on initialisation, so maybe a variable that would contain the function for expirationChecker and will be checked and called upon validate, maybe something along these lines. I’ve tried some tests here and it worked as well.

dsbrgg on Jan 24, 2020

I can’t tell for every case but at least in my case, this property expirationChecker would be very handy because I need to get new credentials for the database after a set amount of time and I believe this way would be easier since knex could just end the resource on its own.

That would be as easy or even easier if knex would just always reload config if it is changed. Then there would be no need for that kind of timer. So to me that parameter still looks like a hack to me instead of feature. I remember that async knex configuration feature first implementation was not very versatile for many use cases either (IIRC it only made possible to get async config once and then there was no way to update it afterwards)…

elhigu on Jan 22, 2020

I actually don’t like or understand why there even is expirationChecker attribute in connection. To me would be better if connection could be declared as a function which then may return config directly or it may return promise, which resolves config.

Then it would be up to client code to implement logic when it wants to get fresh config and when it wants to for example return the same already resolved promise for the old config.

Could someone explain me better how that async / function getting of connection parameters works nowadays or what am I missing here?

elhigu on Jan 20, 2020

So, after a couple of digging, I think I was able to find the issue and at least provide some kind of solution, even if a naive one.

The connectionConfigExpirationChecker property is not being used except for the first time. That is happening because it’s only being called on the first create call on tarn.js.

From what I’ve investigated, tarn.js does not have a property or something like that to destroy resources based on expiration.

Basically, I’ve just added a setTimeout on the updatePoolConnectionSettingsFromProvider and acquiring the resource on the pool to properly close the connection. This also implies that the expirationChecker function should return a value representing the delay in milliseconds to be passed to setTimeout.

I’ve done some local tests here with the codebase I’m working with and I finally had the behaviour I was expecting without any side-effects from what I can see.

@kibertoad , @oranoran , @Ali-Dalal - maybe you could give some insight in this since you guys were involved in the last PR related to this functionality.

dsbrgg on Jan 15, 2020