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

feat: add connector factory method #1540

Merged

Conversation

ruyadorno
Copy link
Contributor

This changeset adds a new connector config option that may be used to define a custom socket factory method. Providing a much more flexible control of the socket connection creation.

Defining a custom connector config value allows Tedious to support a larger variety of environments/setups such as proxy servers using secure socket connections that are used by cloud providers such as GCP.

Linked below, the pg driver for PostgreSQL and the mysql2 driver for MySQL are prior art example of this pattern. Also linked below is the Cloud SQL Node.js Connector, which demonstrates how third-party libraries can leverage the custom socket factory method.

Refs: https://github.com/sidorares/node-mysql2/blob/ba15fe25703665e516ab0a23af8d828d1473b8c3/lib/connection.js#L63-L65
Refs: https://github.com/brianc/node-postgres/blob/b357e1884ad25b23a4ab034b443ddfc8c8261951/packages/pg/lib/connection.js#L20
Refs: https://github.com/GoogleCloudPlatform/cloud-sql-nodejs-connector
Signed-off-by: Ruy Adorno ruyadorno@google.com

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #1540 (0924240) into master (d075b9c) will increase coverage by 0.01%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
+ Coverage   79.67%   79.69%   +0.01%     
==========================================
  Files          91       91              
  Lines        4655     4659       +4     
  Branches      854      856       +2     
==========================================
+ Hits         3709     3713       +4     
+ Misses        680      678       -2     
- Partials      266      268       +2     
Impacted Files Coverage Δ
src/connection.ts 61.74% <42.85%> (+0.05%) ⬆️

... and 1 file with indirect coverage changes

src/connection.ts Outdated Show resolved Hide resolved
@arthurschreiber
Copy link
Collaborator

The changes look great, just a tiny nitpick. ❤️

How does this interact with encrypted connections? I assume the "Cloud SQL Node.js Connector" proxies via a TLS connection, and then tedious would open another TLS connection over that proxy connection?

Do you know what the perf impact of that setup would be? This doesn't sound great, but as it's only specific to GCP cloud connections, it's definitely no blocker. Just trying to understand if that's something we should document to make people aware of.

Thanks!

@ruyadorno
Copy link
Contributor Author

How does this interact with encrypted connections? I assume the "Cloud SQL Node.js Connector" proxies via a TLS connection, and then tedious would open another TLS connection over that proxy connection?

For GCP users using the Cloud SQL Node.js Connector the idea is to also set options.ecrypt=false (along with the connection factory method) by default in order to skip the driver encryption, so at the end of the day GCP users are not going to be running a doubly-encrypted communication system.

Every connection open by the factory method provided by the Cloud SQL Node.js Connector will use a TLS socket that uses an ephemeral certificate and keys that are rotated continuously (every hour or so). Essentially moving the encryption layer up a level so that our custom proxy handles the rotating certs logic.

This changeset adds a new `connector` config option that may be used to
define a custom socket factory method. Providing a much more flexible
control of the socket connection creation.

Defining a custom `connector` config value allows Tedious to support a
larger variety of environments/setups such as proxy servers using secure
socket connections that are used by cloud providers such as GCP.

Linked below, the `pg` driver for PostgreSQL and the `mysql2` driver for
MySQL are prior art example of this pattern. Also linked below is the
Cloud SQL Node.js Connector, which demonstrates how third-party libraries
can leverage the custom socket factory method.

Refs: https://github.com/sidorares/node-mysql2/blob/ba15fe25703665e516ab0a23af8d828d1473b8c3/lib/connection.js#L63-L65
Refs: https://github.com/brianc/node-postgres/blob/b357e1884ad25b23a4ab034b443ddfc8c8261951/packages/pg/lib/connection.js#L20
Refs: https://github.com/GoogleCloudPlatform/cloud-sql-nodejs-connector
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
@ruyadorno ruyadorno force-pushed the add-connector-factory-method branch from d947ee1 to 0924240 Compare April 27, 2023 20:30
@arthurschreiber
Copy link
Collaborator

Oh, I just thought of one more thing. The custom connector should eventually be able to take an AbortSignal so that we can properly forward hitting the connection timeout.

I think the PR can properly be changed as-is for now, but eventually that should be added. What do you think?

@ruyadorno
Copy link
Contributor Author

I think the PR can properly be changed as-is for now, but eventually that should be added. What do you think?

Sounds good to me!

I guess for a follow up PR then the only changes we'd need to land directly in Tedious would be an usage example of the AbortSignal param in examples/custom-connector.js and maybe also a unit test that uses a custom implementation that supports an AbortSignal? Am I missing any other changes that would need to follow up?

@arthurschreiber arthurschreiber merged commit e4eadf8 into tediousjs:master May 6, 2023
24 of 25 checks passed
@arthurschreiber
Copy link
Collaborator

Thanks for your contribution! ❤️

@github-actions
Copy link

github-actions bot commented May 7, 2023

🎉 This PR is included in version 16.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ruyadorno ruyadorno deleted the add-connector-factory-method branch May 25, 2023 17:52
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request May 30, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request May 30, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jun 1, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jun 1, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jun 2, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jun 5, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jun 5, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
AdamJohnSwan pushed a commit to AdamJohnSwan/tedious that referenced this pull request Jun 7, 2023
This adds a new `connector` config option that may be used to define a custom socket factory method, providing much more flexible control of the socket connection creation.
ruyadorno added a commit to ruyadorno/tedious that referenced this pull request Jul 24, 2023
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants