-
Notifications
You must be signed in to change notification settings - Fork 75
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
improve: add support for database connection URIs #1168
Conversation
- Unifies connection configuration of Slonik/Knex - Allows connections to PostgreSQL over Unix domain sockets
a5a841a
to
073450e
Compare
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:
I think the mechanics here might be more complicated than a simple passthrough. When I run 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 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. |
Given what @matthew-white has written, this feels risky and involved enough that I'd rather keep it for another time! |
If all libs use bindings to libpq under the hood (knex seems to, via |
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. |
Related: #377
Related: #394
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 worthcentral-backend
uses Slonikv23.6.4
which is affected, the current46.0.1
has bettered its behaviour but looks quite different, while the earliest released version that has the correct behaviour isv25.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!