Reloading database config with dynamic config #18358

guentherwieser · 2023-06-20T18:34:39Z

Note: Fails to work correctly as the config provided to Knex is based on a deep copy

This is to support #17466, yet it is far from being done - it's just to provide example code for better clarification

Hey, I just made a Pull Request!

✔️ Checklist

A changeset describing the change and affected packages. (more info)
Added or updated documentation
Tests for new functionality and regression tests for bug fixes
Screenshots attached (for UI changes)
All your commits have a Signed-off-by line in the message. (more info)

Note: Fails to work correctly as the config provided to Knex is based on a deep copy Signed-off-by: Günther Wieser <guenther.wieser@creative-it.com>

backstage-goalie · 2023-06-20T18:35:04Z

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/backend-common	packages/backend-common	minor	`v0.19.0`

Signed-off-by: Johannes Grumboeck <johannes.grumboeck@redbull.com>

jgrumboe · 2023-06-23T12:50:19Z

@guentherwieser wrote the following comment with questions to the issue #17466. I think it's better to have implementation discussions here in the PR.

Hi!

I've added a PR to show what we have tried so far.

We were able to detect config changes and invalidate existing Knex connections. Knex then creates new connections. Unfortunately, due to certain limitations which I will describe later, these new connections still are based on the original config.

Reasons:

The config supplied to the connection factory (connection.ts) from DatabaseManager.ts is a full copy of the config instance passed to the DatabaseManager - but it lacks some fundamental features like being able to subscribe to changes, and it won't get updated to the new config values when the config changes
This config is passed on to the database-specific connection factory, e.g. postgres.ts - thus, we can tell Knex that the connection it has created should be dropped and a new one should be created, but we cannot easily push the new config into this part of the code
The reason why we cannot push the changed config into the code easily is that DatabaseManager, connection factory, and database specific connection factory do A LOT OF STUFF with the connection settings
These code pieces create plugin-specific config copies if needed, cache the factories for these plugins, merge certain pre-defined properties with the ones from the user-created config, and on top of that execute database-specific code (e.g. to use a specific role with Postgres).

Long story short: to support the reload of the connection config, we would need to rewrite the DatabaseManager, the connection factory, and the database-specific factories so that all these property mergings and code executions that are needed to correctly setup a new connection, are decoupled from the Knex-specific > parts. So the factories wouldn't eventually create the Knex database object but would provide all means so that we can send a Knex config to the Knex factory that allows us to retrieve a new connection based on the current config.

How Knex can be set up to dynamically retrieve the current connection config, and how the invalidation of an existing connection works, is described here (below the database-specific examples): https://knexjs.org/guide/#configuration-options

We have added these already in the code of the PR, so the connection config for Knex is already an executed function instead of properties, and the expirationChecker() function is already working (currently, in the PR it is not based on config changes but more or less a timeout). But as said, the connection function will always create the "same" connection again and again as currently there is no practical way to have access (in a meaningful way with all the magic that needs to be applied) to the changed config object.

I know that's a lot :-) My question would be if anyone sees an easier way, or has any other valuable input on our approach?

@freben @benjdlambert @Rugvip @jhaals It would be great to get your opinion, if we are on the right track with this proposal, and what are your thoughts in this?

github-actions · 2023-06-30T12:59:51Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

jgrumboe · 2023-06-30T13:03:01Z

@freben @benjdlambert
We still need feedback/input if the proposal of @guentherwieser is the right way. Or are there other plans?

benjdlambert · 2023-06-30T14:49:58Z

Hey 👋 sorry it's just me this next few weeks and been a bit slammed! Will hopefully get to this over the weekend or monday 🙏

awanlin · 2023-07-03T18:54:59Z

Hi @guentherwieser, any chance you can update the title of this PR to something more descriptive? Makes it much easier when looking at all the PRs in a list to tell what this is about. Thanks in advance!

benjdlambert

I think that we're heading in a good direction - just some smaller things to work out I think. I'm also keeping an eye on #18507 as it looks like there's going to be some overlap here. I want to get some other maintainers feedback on the approaches for both this and the one over in #18507 when they get back from the holidays and work out a path forward here, so going to add the after-vacations label on this to pick up when they're back in a week or so. Does that sounds OK?

benjdlambert · 2023-07-04T08:06:28Z

packages/backend-common/src/database/connectors/postgres.ts

@@ -33,9 +34,17 @@ import { Client } from 'pg';
 export function createPgDatabaseClient(
  dbConfig: Config,
  overrides?: Knex.Config,
-) {
-  const knexConfig = buildPgDatabaseConfig(dbConfig, overrides);
+  deps?:  {


don't need these here right?

Rugvip

Thank you! 👍

Got an initial high level question on the approach

Rugvip · 2023-07-11T11:58:36Z

packages/backend-common/src/database/DatabaseManager.ts

+          console.log(`backend: ${JSON.stringify(config.getOptionalConfig('backend'))}`);
+        }
+        console.log('Invalidating the database cache...');
+        this.databaseCache.clear();


Do we need to tell knex that config has changed? Would it be enough to simply read the latest config whenever we want to create a new database connection? Reading config from the ConfigService is very fast, more or less property-access fast. With that I feel we would be able to avoid quite a lot of complicated coupling?

Of course that all hinges on whether knex will detect failed connections and try to re-establish new ones with new config. If it keeps trying to re-use the old config while failing to connect that approach wouldn't work.

To add to the above Q, I'm wondering what the behavior would be if we provide an expirationChecker that always returns true.

Do we need to tell knex that config has changed? Would it be enough to simply read the latest config whenever we want to create a new database connection? Reading config from the ConfigService is very fast, more or less property-access fast. With that I feel we would be able to avoid quite a lot of complicated coupling?

I don't think you can "just" read the latest config. In token-based access architecture, you will have to make a request to acquire a new token. Together with this token, you will get an expiration timestamp. Simple comparison current timestamp will not take long to mark the connection as expired.

There is a similar issue and PR with more accomplished implementation of the approach proposed here https://github.com/backstage/backstage/pull/18507/files

That's not what we're doing now though right? At the moment we are just reacting to changes in config.

Well, yeah. Token-based access is out of the scope of this PR. Sorry :)

So is it alright to flip around the approach in this PR? So that rather than subscribing and pushing config changes we have the knex client read the latest config every time it needs to create a new connection? We just gotta make sure that the expirationChecker behaves well with that approach. As in if we use a naive approach were it always returns true, we'd for example need to make sure that doesn't cause existing connections to be torn down. It could also be that we actually want the expirationChecker to read config itself, to check if it has changed, if that ends up being the best approach we'd also need to make sure it isn't called too often, i.e. on every statement.

github-actions · 2023-07-31T12:58:13Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

github-actions · 2023-08-08T12:41:17Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

github-actions · 2023-08-16T14:34:41Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

jgrumboe · 2023-08-23T14:17:05Z

We'll rethink the implementation and open a new PR when ready.

Initial draft for backstage#17466

08a539b

Note: Fails to work correctly as the config provided to Knex is based on a deep copy Signed-off-by: Günther Wieser <guenther.wieser@creative-it.com>

guentherwieser requested a review from a team as a code owner June 20, 2023 18:34

guentherwieser requested a review from freben June 20, 2023 18:34

chore: added changeset

4d081ac

Signed-off-by: Johannes Grumboeck <johannes.grumboeck@redbull.com>

jgrumboe force-pushed the 17466-dynamic-reload-of-database-config branch from d8b23be to 4d081ac Compare June 21, 2023 06:22

benjdlambert mentioned this pull request Jun 26, 2023

🚀 Feature: Add support to database connection for object or a function returning an object #18425

Closed

2 tasks

github-actions bot added the stale label Jun 30, 2023

github-actions bot removed the stale label Jun 30, 2023

benjdlambert reviewed Jul 4, 2023

View reviewed changes

benjdlambert added the after-vacations Some of the maintainers are away, let's look at this after the vacations label Jul 4, 2023

benjdlambert changed the title ~~Initial draft for #17466~~ Reloading database config with dynamic config Jul 4, 2023

Rugvip reviewed Jul 11, 2023

View reviewed changes

jgrumboe mentioned this pull request Jul 12, 2023

Added support for Azure AD authentication via Service principle #18507

Closed

5 tasks

Rugvip removed the after-vacations Some of the maintainers are away, let's look at this after the vacations label Jul 24, 2023

github-actions bot added the stale label Jul 31, 2023

freben removed the stale label Aug 1, 2023

github-actions bot added the stale label Aug 8, 2023

benjdlambert removed the stale label Aug 9, 2023

github-actions bot added the stale label Aug 16, 2023

github-actions bot closed this Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reloading database config with dynamic config #18358

Reloading database config with dynamic config #18358

guentherwieser commented Jun 20, 2023

backstage-goalie bot commented Jun 20, 2023 •

edited

jgrumboe commented Jun 23, 2023

github-actions bot commented Jun 30, 2023

jgrumboe commented Jun 30, 2023

benjdlambert commented Jun 30, 2023

awanlin commented Jul 3, 2023

benjdlambert left a comment

benjdlambert Jul 4, 2023

Rugvip left a comment

Rugvip Jul 11, 2023

Rugvip Jul 11, 2023

andriyun Jul 14, 2023

Rugvip Jul 14, 2023

andriyun Jul 14, 2023

Rugvip Jul 24, 2023

github-actions bot commented Jul 31, 2023

github-actions bot commented Aug 8, 2023

github-actions bot commented Aug 16, 2023

jgrumboe commented Aug 23, 2023

Reloading database config with dynamic config #18358

Reloading database config with dynamic config #18358

Conversation

guentherwieser commented Jun 20, 2023

Hey, I just made a Pull Request!

✔️ Checklist

backstage-goalie bot commented Jun 20, 2023 • edited

Changed Packages

jgrumboe commented Jun 23, 2023

github-actions bot commented Jun 30, 2023

jgrumboe commented Jun 30, 2023

benjdlambert commented Jun 30, 2023

awanlin commented Jul 3, 2023

benjdlambert left a comment

Choose a reason for hiding this comment

benjdlambert Jul 4, 2023

Choose a reason for hiding this comment

Rugvip left a comment

Choose a reason for hiding this comment

Rugvip Jul 11, 2023

Choose a reason for hiding this comment

Rugvip Jul 11, 2023

Choose a reason for hiding this comment

andriyun Jul 14, 2023

Choose a reason for hiding this comment

Rugvip Jul 14, 2023

Choose a reason for hiding this comment

andriyun Jul 14, 2023

Choose a reason for hiding this comment

Rugvip Jul 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2023

github-actions bot commented Aug 8, 2023

github-actions bot commented Aug 16, 2023

jgrumboe commented Aug 23, 2023

backstage-goalie bot commented Jun 20, 2023 •

edited