Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection Pool reset, due to connection parameters change, during operation #66

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

joaoroscoe
Copy link

Monitors for changes in connection data/ database credentials at run time, and recreates the connection pool when changes are detected. This ensures that database connection will be kept alive, even when connection related parameters (coming from environment or context variables) are changed.

@joaoroscoe joaoroscoe closed this Mar 3, 2024
@joaoroscoe joaoroscoe reopened this Mar 3, 2024
postgresql.js Outdated
Comment on lines 173 to 187
let dbAccessCfgData = {};
dbAccessCfgData.user = getField(node, node.config.userFieldType, node.config.user);
dbAccessCfgData.password = getField(node, node.config.passwordFieldType, node.config.password);
dbAccessCfgData.host = getField(node, node.config.hostFieldType, node.config.host);
dbAccessCfgData.port = getField(node, node.config.portFieldType, node.config.port);
dbAccessCfgData.database = getField(node, node.config.databaseFieldType, node.config.database);
dbAccessCfgData.ssl = getField(node, node.config.sslFieldType, node.config.ssl);
dbAccessCfgData.application_name = getField(node, node.config.applicationNameType, node.config.applicationName);
dbAccessCfgData.max = getField(node, node.config.maxFieldType, node.config.max);
dbAccessCfgData.idleTimeoutMillis = getField(node, node.config.idleFieldType, node.config.idle);
dbAccessCfgData.connectionTimeoutMillis = getField(node, node.config.connectionTimeoutFieldType, node.config.connectionTimeout);

// Get previous db access configuration data
const nodeContext = node.context();
const previousDbAccessCfgData = nodeContext.get('previousDbAccessCfgData')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code introduces quite some overhead and therefore some performances penalties. This is on the "hot path", and will be called many many times. So we need to be careful and see how the negative consequences can be mitigated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the fact that this will run at all messages reception could become a real problem depending on the scenario considered. Quite simplistic, my initial idea, sorry!
In my specific scenario, the frequency of transactions in the database is relatively low, so I am unlikely to face performance issues, here. However, of course, we want a solution that is scalable.

Possibilities that occur to me:

  • Create flags in the endpoint configuration, to mark which parameters should be monitored for changes;
  • Leave the monitoring task out of the implementation, creating just a way to force the pool to restart (a message containing a specific flag?).
  • Check whether there have been changes to parameters/credentials only when errors occur.

Answering your question: in my specific scenario, only the database access password, which is obtained from an external system, will be changed periodically, as part of a security policy.

Copy link
Author

@joaoroscoe joaoroscoe Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another potential issue: considering that we could have several nodes acessing the database, what would happen if a node restarts the pool, and another one is caught right in the middle of trying to use it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are all good ideas. Exposing a possibility to reload the pool, for instance through a specific message, could be a simple option. Please check my question further down, so I can better understand the use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding concurrency, that will need to be tested when we have some a clearer scenario

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have just pushed a new commit, including:

  • Pool is reset if msg includes a property with key "reconnect" and any truthy value;
  • Pool is reset if a change is detected in any connection parameter that is flagged to be monitored.

Inputs (checkboxes) were added to the configuration node editor, to flag the connection parameters to be monitored for changes. Those are read at initialization, and pushed into a "watch list" array. Only parameters on that list will be checked at message reception, greatly minimizing overhead in the "hot path".

Additionally, I made changes in node-config-input-ssl input html definition, so that the Typed Input widget will show with the same width as the other ones, to make space for 'global' type input.

I haven't touched the documentation, so far.

Your tougths ?

@Alkarex
Copy link
Member

Alkarex commented Mar 4, 2024

Thanks for the PR 👍🏻
Could you please explain (again) what parameter is dynamically changing, how it is changed, and how you get the information?
We need to find an efficient way :-)

@joaoroscoe
Copy link
Author

Thanks for the PR 👍🏻 Could you please explain (again) what parameter is dynamically changing, how it is changed, and how you get the information? We need to find an efficient way :-)

Hi. I guess you've been waiting for a more detailed description of the situation I've been working on, which I provide below.

I'm working in a project in which the security requirements specify that having a hardcoded database access credentials - specially the password - is not acceptable. Those must be obtained from a corporate Key Vault solution, at runtime.

I already have an working flow implementation that periodically queries the Key Vault for database access credentials, and keeps that info up to date, in a global context object, where the postgresql access module can find it.

Now, the catch is:

  1. The node-red-contrib-postgresql code creates the connection pool only once, right at Node Red startup, when flows are loaded - that's tipically before the Key Vault access flow can do it's thing;
  2. In my particular situation, the Key Vault does* change the password periodically, also due to security requirements. 3. My Key Vault pooling does detect password changes and is able to send a "reconnect* message to node-red-contrib-postgresq nodes;
  3. In some other project, one could get such credentials from Environment variables instead, and those could change without Node Red knowing anything about it. In such situations, having credentials change detection performed within the node-red-contrib-postgresql module could be useful.

I hope the explanation above is clear.
Best regards,
Joao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants