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

improve: add support for database connection URIs #1168

Closed

Conversation

brontolosone
Copy link
Contributor

@brontolosone brontolosone commented Aug 1, 2024

Related: #377
Related: #394

  • Unifies connection configuration of Slonik/Knex
  • Allows connections to PostgreSQL over Unix domain sockets

I set out to "simply" add support for connecting to PostgreSQL over Unix domain sockets using peer authentication, as that's what I like to use. But it avalanched into this, as I figured the easiest way to make that happen would be to support connection URIs with which underlying libraries would hopefully do the right thing.

Along the way it turned out that the potential problem of Slonik gratuitously specifying a rejectUnauthorized parameter is no longer present in current versions, it now seems to only specify it when requested to (which can be done through query string parameters in the connection URI). For what it's worth central-backend uses Slonik v23.6.4 which is affected, the current 46.0.1 has bettered its behaviour but looks quite different, while the earliest released version that has the correct behaviour is v25.1.0.

As I currently understand it, the corollary is this: With this new code, until we upgrade Slonik to v25.1.0 (or later), people connecting to their databases over SSL will not be validating the server certificate fully. I tried a quick bump to v25.1.3 but that makes the unit tests fail; thus there'll be some work involved with that bump I suppose. For another day!

- Unifies connection configuration of Slonik/Knex
- Allows connections to PostgreSQL over Unix domain sockets
@brontolosone brontolosone force-pushed the postgres-unix-socket-support branch from a5a841a to 073450e Compare August 1, 2024 20:08
@matthew-white
Copy link
Member

matthew-white commented Aug 11, 2024

Thanks, @brontolosone! I can see the potential usefulness of this change, as it would open up additional options to connect to the database. It also seems consistent with the comment at #377 (comment). That said, as I mentioned when we were chatting, I think there are also dragons in this area. I think we should probably wait to continue investigating this until after you're done working on getodk/central#689. More details below!

As background, we're currently using old versions of both Slonik and Knex. We'd like to get off the old version of Slonik (#1119), either upgrading Slonik or replacing it altogether. As you mentioned, Slonik has had a lot of changes. We'd also like to get off Knex entirely (#744).

I see these comments in the code:

When an uri is supplied, we treat it as opaque and simply pass it on verbatim to Slonk/Knex etc, which both will in turn hopefully pass it on unmolested to node-postgres, which uses https://nodei.co/npm/pg-connection-string/ to parse it.

ideally we'd validate this uri for suitability for pg-connection-string.parse(). But:

  1. how would we target the exact version of pg-connection-string used by the pg module; it's not re-exported so that would require some acrobatics

I think the mechanics here might be more complicated than a simple passthrough. When I run npm ls pg-connection-string, it looks like Slonik and Knex both include pg-connection-string as a direct dependency. In our package-lock.json, Slonik and Knex use version 2.4.0 of pg-connection-string, while pg uses version 2.5.0. I took a quick look at the latest Slonik, and it doesn't seem to include pg-connection-string as a direct dependency.

Another thing that's changed with Slonik is connecting via SSL: see #433. Previously, Slonik just took a connection string, but now it also wants a separate object for SSL options. Mostly that sounds like a change for the better to me, but it makes me wonder about the approach in this PR, which is focused on connection strings. If/when we upgrade Slonik, how would users specify SSL options for Slonik, and would those same options also work with our version of Knex? My concern is that a user will try to specify an SSL option via the uri config in this PR, but the SSL option won't work, or it will work for Knex but not Slonik.

The advantage of our current approach is that we can limit the connection options to ones that we know will work. Users aren't able to supply a connection string that might work differently in Slonik vs. Knex. The downside is that there are options that we don't support.

I'm open to the change in this PR, and I think we should discuss further! That said, I think it's complicated enough that we should probably wait to circle back to it until after getodk/central#689.

@lognaturel
Copy link
Member

Given what @matthew-white has written, this feels risky and involved enough that I'd rather keep it for another time!

@lognaturel lognaturel closed this Oct 1, 2024
@brontolosone
Copy link
Contributor Author

brontolosone commented Oct 18, 2024

If all libs use bindings to libpq under the hood (knex seems to, via node-postgres, and slonik seems to, and pg too should we want to consider that in the future)... then perhaps we could try to defer to libpq's environment variable handling for everything, and cut out all these meddling middlemen.

@matthew-white
Copy link
Member

We've been talking a little more lately about how to get off Knex (#744). I feel like if we can wait to circle back to these questions until after we've removed Knex, a lot of things will be simpler.

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.

3 participants