Update packages/pg/lib/connection-parameters.js. Fix SSL issue (#2723) #2994
Conversation
There was a problem hiding this comment.
I appreciate the effort and thought here! A couple of concerns
-
all code merged needs supporting tests. Having to support this code base long after you may move on from postgres or even node.js its the only way I can keep my sanity
-
don't use sync APIs - any file reading needs to be non-blocking.
dapeleg-dn
force-pushed
the
pr-2723
branch
2 times, most recently
from
775622b to
94599e9
Compare
|
@brianc, please take a look. I force-pushed the code. I walked in a slightly different way, allowing you to mix TLS definitions in both environment variables and connection string or config options. Tests have been added as well for your request. See my comment above regarding the sync/async discussion. |
|
@brianc, are you okay with the changes? |
| @@ -1,6 +1,7 @@ | |||
| 'use strict' | |||
|
|
|||
| var dns = require('dns') | |||
| const _ = require('lodash') | |||
There was a problem hiding this comment.
Undeclared dependency, and too heavy just to check if an object is empty (should pull in just https://www.npmjs.com/package/lodash.isempty if really necessary).
| @@ -3,6 +3,11 @@ var helper = require('../test-helper') | |||
| const Suite = require('../../suite') | |||
|
|
|||
| var assert = require('assert') | |||
| const fs = require('fs') | |||
|
|
|||
| const tmp = require('tmp') | |||
There was a problem hiding this comment.
Another undeclared dependency. Can the files just be part of the repo instead of written dynamically by test code?
| const fs = caFileName || crtFileName || keyFileName ? require('fs') : null | ||
| var result = {} | ||
| if (caFileName) { | ||
| result.ca = fs.readFileSync(caFileName).toString() |
There was a problem hiding this comment.
Implicit sync calls that happen on every connection might not be good (one good reason for user code to be responsible for this…)
There was a problem hiding this comment.
Regarding sync / async, read this: #2994 (comment)
| } | ||
| } else if (typeof this.ssl === 'object') { | ||
| var sslCandidate = fillInSSLVariablesFromEnvironment({}) | ||
| this.ssl = Object.assign(this.ssl, sslCandidate) |
There was a problem hiding this comment.
Potentially modifies user-provided config.ssl object in place.
| this.ssl = Object.assign(this.ssl, sslCandidate) | |
| this.ssl = {...this.ssl, ...sslCandidate} |
| @@ -33,6 +34,27 @@ var readSSLConfigFromEnvironment = function () { | |||
| return defaults.ssl | |||
| } | |||
|
|
|||
| var fillInSSLVariablesFromEnvironment = function(existingSsl) { | |||
There was a problem hiding this comment.
existingSsl is always {}?
|
Is there some workaround while this is being worked on? currently it seems I am blocked from using node-postgres with Google Cloud SQL. |
|
@adaboese You can pass your SSL configuration in the |
|
@adaboese here's the documentation - I hope its clear! I'm going to close this PR as it doesn't seem to be being moved forward. There's an open issue (#2723) so we can take another shot at it w/o loadash and soforth. |
No description provided.