-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: support CA_CERT env var override #194
Conversation
When connecting to remote databases that are SSL-required, the app will sometimes report cert errors. First, I tried to resolve this by pulling in `ca-certificates` in the container environment, but that wasn't sufficient. Turns out `node-postgres` in v8.x updated the ssl logic and that broke setups for many managed databases. As a workaround, one can now: 1. set the CA_CERT env var with the string contents of the db's CA 2. remove the `sslmode=require` from the connection auth starting The fact that 2 is necessary is a bit scary, but it's caused by the fact that the connectionString setting clobbers any manual `ssl` opts in the db config.
Built a container from this branch, and was able to connect to a DO pg database, provided both were true:
My intent in the diff was to add support for this setup, but it's important that the existing behavior not change. @ejmg I'd appreciate if you could take this branch for a spin locally, and ensure it still works just fine for you in your normal workflows. Otherwise, we should edit it. Also open to constructive criticism on the overall approach here. |
connectionString: process.env.DATABASE_URL, | ||
}); | ||
ssl: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In js/ts you can do in-place conditionals in objects like this:
ssl: {}, | |
...(process.env.CA_CERT && { | |
ssl: { | |
rejectUnauthorized: true, | |
ca: process.env.CA_CERT, | |
}, | |
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a style preference, your code works perfectly fine as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL! Great suggestion, added. (I also tested with a new ad-hoc container build to confirm I didn't break anything with my clumsy TS.)
Thanks for review! |
When connecting to remote databases that are SSL-required, the app will sometimes report cert errors. First, I tried to resolve this by pulling in
ca-certificates
in the container environment, but that wasn't sufficient. Turns outnode-postgres
in v8.x updated the ssl logic and that broke setups for many managed databases. As a workaround, one can now:sslmode=require
from the connection auth startingThe fact that 2 is necessary is a bit scary, but it's caused by the fact that the connectionString setting clobbers any manual
ssl
opts in the db config.Refs #193.