Cloudflare Workers support #2971
Conversation
The : and @ were the wrong way round
petebacondarwin
force-pushed
the
cf-workers
branch
3 times, most recently
from
af3721d
to
e1b088b
Compare
|
Converting back to draft while I refactor things on the back of the reviews. Thanks @brianc @jasnell and @charmander |
|
This is a lot of code for what looks to be a series of shims. Was mostly neutral on it until I saw it even adds a sha256 implementation to the repo. If you intend to support non-SASL auth we'd need an md5 implementation too. Doesn't feel right to expand the module to include all of that. Is it possible to instead add it as an external shim to the node environment before If it is going to be added, something like this should be externalized and injected at runtime similar to how we handled Promises before they were part of node core. That would involve pulling out the crypto and fs access functions and delegating them to something passed in the connection config object (defaulting to loading node crypto and fs). I don't really like expanding the module for that but at least it keeps the maintenance of the shim external. The contract for the module would be whatever set of functions we actually use. |
Can you expand on this? If this becomes an explicit config option, it becomes much higher friction for users & ORMs that depend on Note that this should allow node-postgres to work in other serverless environments, like Deno and Bun, once they also allow TCP socket creation. The crypto & FS shims are relevant there too. |
petebacondarwin
force-pushed
the
cf-workers
branch
from
46e9317
to
51681d9
Compare
|
@sehrope - thanks for your comments. That and the other reviews gave me enough motivation to go and do further refactoring to avoid the Node.js API shims altogether. Now the code is mostly using the more modern WebCrypto APIs. |
petebacondarwin
force-pushed
the
cf-workers
branch
from
9a50aa1
to
ddffc66
Compare
Of course it's higher friction if you're going to compare it to zero friction. But that's what you get for running on something that purports to be node but doesn't provide the entire userspace. The Cloudflare worker docs already instruct the end user they need to explicitly enable a flag to turn on nodejs compatibility for things like Buffer:
Ideally any special handling to make the environment node-like should be happening on Cloudflare's end. If need be, we can expand the configurability of this module to facilitate that. But whatever leverages those options to provide support for a specific provider should be owned by that provider.
If there's an actual standard or API then sure, but rolling our own sha256 or adding a service providers specific socket implementation (e.g. Again, I'm not against adding support for this use case, I just think it should be done in a way that minimizes code ownership for this project and allows multiple implementations. The PR for what I'm describing would not have any provider specific code. It'd be a series of commits to pull out internal references to things like crypto operations (for md5 and sha256), reading from the filesystem / pgpass, and creating the socket streams. The defaults would be using the nodejs built-ins for crypto, fs, and net so there would be no net change to the module. Anybody that wants to override it for a non-nodejs platform would shim those fields, either globally or in the config object passed to pg. A good analogy is the addition of dynamic passwords to this module. IIRC, the original request was someone wanting to authenticate with AWS IAM auth against an RDS database. Rather than adding in the AWS SDK and baking in a provider specific option for signing auth requests, we have a generic interface that allows specifying an async function to provide a password dynamically. That's applicable to N providers and it's up to each of them to support however they work internally. |
Swapping the deprecated Node.js API for the modern cross environment API.
petebacondarwin
force-pushed
the
cf-workers
branch
from
1d92c24
to
68b5b15
Compare
petebacondarwin
force-pushed
the
cf-workers
branch
2 times, most recently
from
8249ba2
to
cc35b46
Compare
|
@sehrope and @charmander - I have moved the |
|
Ideally we would have |
|
Oh no 😭 WebCrypto.subtle only became available in node 15. |
The only place we are stuck with node's original crypto API is for generating md5 hashes, which are not supported by WebCrypto.
petebacondarwin
force-pushed
the
cf-workers
branch
from
cc35b46
to
84611b7
Compare
petebacondarwin
force-pushed
the
cf-workers
branch
from
84611b7
to
8fcd6bd
Compare
|
All green @brianc - please take a look when you have some time. |
|
This is a huge PR - thank you so much for putting in the time and effort here from everyone involved. 😄 For a bit of background I've been meeting with cloudflare about this both in person here in Austin, TX and on video calls off and on for a month discussing this & so on. Really cool to see how little actual code change was involved. I hear the concern about making things more generic and accepting shims being injected in...there is still a bunch of churn on some of these web crypto and stream modules...hopefully they settle down & get "standardized" soon and we can look at making some of these injectables more dynamic. I'm cool w/ having them more bespoke when there are only node & CF workers being supported & can work towards more dynamic injectable things in the future if/when a 3rd supported platform wants to add their stuff. |
|
Thanks @brianc - are we good to merge this? |
|
|
||
| const debug = false | ||
|
|
||
| function dump(data: unknown) { | ||
| if (data instanceof Uint8Array || data instanceof ArrayBuffer) { | ||
| const hex = Buffer.from(data).toString('hex') | ||
| const str = new TextDecoder().decode(data) | ||
| return `\n>>> STR: "${str.replace(/\n/g, '\\n')}"\n>>> HEX: ${hex}\n` | ||
| } else { | ||
| return data | ||
| } | ||
| } | ||
|
|
||
| function log(...args: unknown[]) { | ||
| debug && console.log(...args.map(dump)) | ||
| } |
CI is green. Please take a look at the code. Probably easiest commit by commit