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: support Cloudflare Workers #2289

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

Mini256
Copy link
Contributor

@Mini256 Mini256 commented Nov 21, 2023

try to close #2179

Summary

  • To fix the error window is not found, replace node Timers with web timers
  • To fix the error Code generation from strings disallowed for this context, using the static parser in PR Add "interpreting" parser #2099
  • To fix the error crypto.hash() is not a function, polyfill node crypto withwebcrypto, and modify the functions which directly/indirectly called the crypto library as async function.

TODOs:

  • mysql_native_password auth plugin
  • caching_sha2_password auth plugin
  • sha256_password auth plugin
  • static_text_parser (will be finished in PR Add "interpreting" parser #2099)
  • static_binary_parser (will be finished in PR Add "interpreting" parser #2099)
  • add integration tests
  • repalce pg-cloudflare with mysql2-cloudflare?

Usage

For users, they need to:

  1. Add node_compat = true flag to polyfill the missing node modules
  2. Add useStaticParser flag to force mysql2 to use static parser.

For example: https://github.com/Mini256/mysql2-cloudflare-test/blob/1f339a85f4457b696f122503051991326dbc23dc/src/index.ts#L30

@shiyuhang0 shiyuhang0 force-pushed the add-static-text-parser branch from 6954a22 to c4d5458 Compare November 21, 2023 11:10
@Mini256 Mini256 marked this pull request as ready for review December 14, 2023 09:13
@Mini256 Mini256 marked this pull request as draft December 15, 2023 07:35
@shiyuhang0
Copy link

This PR can run on Cloudflare Workers with the following database:

  • MySQL 5
  • MySQL 8
  • TiDB Serverless Cluster
  • PlanetScale

@sidorares could you please approve to run the GitHub action?

@shiyuhang0 shiyuhang0 force-pushed the add-static-text-parser branch from 4760d37 to 0b9245b Compare December 25, 2023 09:04
@sidorares
Copy link
Owner

@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?

@shiyuhang0
Copy link

shiyuhang0 commented Dec 25, 2023

@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?

cc3458f

This commit adds a step-by-step tutorial. Since it's user-facing, it needs a new release of MySQL2 including this PR.
it's better to follow the develop guide in the doc to test locally without a new release.

@shiyuhang0 shiyuhang0 force-pushed the add-static-text-parser branch 2 times, most recently from 8949aca to cc3458f Compare December 25, 2023 11:24
@shiyuhang0
Copy link

shiyuhang0 commented Dec 27, 2023

CI is green. Maybe we can ignore the failure of CodeQL action, because the error is reported in the code that originally existed (just the location has changed)

As for the static parser. We'd like to merge your PR #2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares
The static binary parser is also needed to make things work. Code generation from strings disallowed for this context still occurs when code goes through the binary parser.

@sidorares
Copy link
Owner

As for the static parser. We'd like to merge your PR #2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares The static binary parser is also needed to make things work. Code generation from strings disallowed for this context still occurs when code goes through the binary parser.

I can probably merge it first, together with added binary protocol non-jit parser.
I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ). disableGenFunction ( again, too focused on implementation ). Naming things is hard 🤷‍♂️

re testing - thanks a lot for dev documentation. Are you aware of any way to test workers locally or in actions runtime? something similar to localstack. Ideally with the same checks, e.i errors when code tries to eval. Any container we can use for that?

@Mini256
Copy link
Contributor Author

Mini256 commented Dec 28, 2023

Are you aware of any way to test workers locally or in actions runtime?

Cloudflare Workers provide an unstable_dev API for E2E / integration tests:

https://developers.cloudflare.com/workers/wrangler/api/#unstable_dev

Test code in node-postgres:

https://github.com/brianc/node-postgres/blob/6cd0aeb212d1672edd33499b2f4f858cf7ed9a79/packages/pg/test/worker/src/index.test.js#L13

@shiyuhang0
Copy link

I can probably merge it first, together with added binary protocol non-jit parser.
I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ). disableGenFunction ( again, too focused on implementation ). Naming things is hard 🤷‍♂️

I prefer disableEvals among these names. You can also use other names, not good at naming for me :(

@sidorares
Copy link
Owner

I prefer disableEvals among these names. You can also use other names, not good at naming for me :(

I like it more than useStaticParser, I think I'll update in my branch ( together with a version for no evals binary parser ) and comment here when ready

@advancedtw
Copy link

I have tested this pr on multiple MySQL server ranging from old 5.7 to more recent MySQL and MariaDB version locally and on deployed workers so far it works flawlessly. The only issue is having to set node_compat = true instead of compatibility_flags = [ "nodejs_compat" ] but that's a small price to pay.
Also it's currently not possible to use drizzle with msyql driver but kysely works ok.

@elithrar
Copy link

FYI:

  • Other drivers (like node-postgres and Postgres.js) also require node_compat = true today
  • We have plans to allow nodejs_compat to work: this will likely require us to better audit specific packages and conditionally load/exclude Node.js APIs that we don't have support for in Workers, or that don't make sense in a serverless environment like Workers.

Otherwise, we'd love to see this merged + we're bringing MySQL support to Hyperdrive: https://developers.cloudflare.com/hyperdrive/

@shiyuhang0
Copy link

I have resolved the conflict with master branch.
@sidorares Hi, what's the process of no evals parser? I'd like to remove the related codes in the PR after that.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 6, 2024

Hi, @shiyuhang0 🙋🏻‍♂️

If it's okay, I'd like to make a few simple suggestions in the documentation:

  • Change from .md to .mdx
  • Change the title from Run on Cloudflare Workers to Cloudflare Workers
  • I think it fits in the Examples section, what do you think?

@shiyuhang0
Copy link

Hi, @shiyuhang0 🙋🏻‍♂️

If it's okay, I'd like to make a few simple suggestions in the documentation:

  • Change from .md to .mdx
  • Change the title from Run on Cloudflare Workers to Cloudflare Workers
  • I think it fits in the Examples section, what do you think?

already doing this, will push a commit soon

@sidorares
Copy link
Owner

@shiyuhang0 what do you think about useStaticParser flag name? Should we rename it to disableEval before its too late and somebody else depends on this name?

@shiyuhang0
Copy link

shiyuhang0 commented Mar 7, 2024

@shiyuhang0 what do you think about useStaticParser flag name? Should we rename it to disableEval before its too late and somebody else depends on this name?

Both it is fine for me. But we shouldn't worry about modifying parameter names in an unmerged PR. For me, I think it can even be modified before release.

I will adjust this PR after the name is determined

@m430
Copy link

m430 commented Mar 27, 2024

I really need this feature. I need to use the node-mysql2 driver with Drizzle in Cloudflare Workers. If everything is confirmed to be correct, could you please merge this PR as soon as possible? Thanks

@sidorares

@advancedtw
Copy link

advancedtw commented Mar 27, 2024

@m430 last time I checked Drizzle/Mysql was not compatible with cloudflare even using this updated driver. That is because of some node compatibility issue.
If you really need a mysql driver for cf today! you can use a git dependecy or @nora-soderlund cloudflare-mysql

@shiyuhang0 disableEval makes more sense to me

@sidorares
Copy link
Owner

@shiyuhang0 missed your message, I also prefer disableEval. Are you able to continue or you need someone to take over your work?

@shiyuhang0
Copy link

shiyuhang0 commented May 6, 2024

@shiyuhang0 missed your message, I also prefer disableEval. Are you able to continue or you need someone to take over your work?

@sidorares I have pushed a new commit to use disableEval instead of useStaticParser . Is there anything else I need to do?

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.97%. Comparing base (a0c70a2) to head (d060077).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2289   +/-   ##
=======================================
  Coverage   88.97%   88.97%           
=======================================
  Files          86       86           
  Lines       13527    13527           
  Branches     1564     1564           
=======================================
  Hits        12035    12035           
  Misses       1492     1492           
Flag Coverage Δ
compression-0 88.97% <ø> (ø)
compression-1 88.97% <ø> (ø)
static-parser-0 86.54% <ø> (ø)
static-parser-1 87.32% <ø> (ø)
tls-0 88.38% <ø> (ø)
tls-1 88.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Mini256 Mini256 marked this pull request as ready for review May 6, 2024 06:58
@elithrar
Copy link

elithrar commented May 6, 2024

Add node_compat = true flag to polyfill the missing node modules

Is there a path to instead supporting our modern nodejs_compat mode by updating imports (conditionally) vs using the legacy polyfill mode?

https://developers.cloudflare.com/workers/runtime-apis/nodejs/

@Mini256
Copy link
Contributor Author

Mini256 commented Feb 5, 2025

I did a simple test with 3.12.1-canary.603c2463 and connecting mysql in a non-SSL way can work properly. 👍

However, when I connect in SSL, I will get an error:

[unenv] tls.createSecureContext is not implemented yet!

https://github.com/Mini256/mysql2-cloudflare-test/blob/main/src/index.ts

I will try to add this integration test to the node-mysql2 test and try to solve it.

@sidorares
Copy link
Owner

node compatibility table: https://workers-nodejs-compat-matrix.pages.dev/

@i18nsite
Copy link

i18nsite commented Feb 8, 2025

I did a simple test with 3.12.1-canary.603c2463 and connecting mysql in a non-SSL way can work properly. 👍

However, when I connect in SSL, I will get an error:

[unenv] tls.createSecureContext is not implemented yet!

https://github.com/Mini256/mysql2-cloudflare-test/blob/main/src/index.ts

I will try to add this integration test to the node-mysql2 test and try to solve it.

still can't use in cloudflare worker


✘ [ERROR] A request to the Cloudflare API (/accounts/xxx/workers/scripts/cfcron) failed.

  Uncaught Error: Dynamic require of "events" is not supported
    at null.<anonymous> (main.js:21:9)
    at null.<anonymous> (file:///Users/z/i18n/lib/state/cf/src/main.js:2989:50641)
    at null.<anonymous> (file:///Users/z/i18n/lib/state/cf/src/main.js:1:458)
    at null.<anonymous> (file:///Users/z/i18n/lib/state/cf/src/main.js:2989:53394)
   [code: 10021]


  If you think this is a bug, please open an issue at:
  https://github.com/cloudflare/workers-sdk/issues/new/choose


+ bun x esbuild ../lib/main.js --bundle --minify --outfile=src/main.js --format=esm '--external:node:*'
✘ [ERROR] Could not resolve "events"

    ../node_modules/mysql2/promise.js:4:29:
      4 │ const EventEmitter = require('events').EventEmitter;
        ╵                              ~~~~~~~~

  The package "events" wasn't found on the file system but is built into node. Are
  you trying to bundle for node? You can use "--platform=node" to do that, which will
  remove this error.

✘ [ERROR] Could not resolve "process"

    ../node_modules/mysql2/lib/pool_cluster.js:3:24:
      3 │ const process = require('process');
        ╵                         ~~~~~~~~~

  The package "process" wasn't found on the file system but is built into node. Are
  you trying to bundle for node? You can use "--platform=node" to do that, which will
  remove this error.

✘ [ERROR] Could not resolve "events"

    ../node_modules/mysql2/lib/pool_cluster.js:8:29:
      8 │ const EventEmitter = require('events').EventEmitter;
        ╵                              ~~~~~~~~

  The package "events" wasn't found on the file system but is built into node. Are
  you trying to bundle for node? You can use "--platform=node" to do that, which will
  remove this error.

✘ [ERROR] Could not resolve "events"

    ../node_modules/mysql2/lib/promise/pool.js:3:29:
      3 │ const EventEmitter = require('events').EventEmitter;

@thomasgauvin
Copy link
Contributor

thomasgauvin commented Feb 28, 2025

Hey folks! Are you using nodejs_compat flag? With the nodejs_compat flag in your wrangler.jsonc/wrangler.toml , and using the most recent version of the wrangler CLI which uses the most recent version of workerd with the missing pieces (cloudflare/workerd#3315), you should be able to use the static parser in a way that works on Cloudflare Workers

@thomasgauvin
Copy link
Contributor

thomasgauvin commented Feb 28, 2025

I went ahead and created a repo that you can use as a reference for a sample project that works @i18nsite @Mini256 @wellwelwel https://github.com/thomasgauvin/mysql2-on-cf-workers You'll have to excuse my quick code that does not make use of env variables for connection details. It does run on Cloudflare Workers now with disableEval: true if you use one of the recent Wrangler releases for local development (I'm using 3.111.0). It's also supported on the 'deployed' Cloudflare Workers https://mysql2-on-cf-workers.toms-projects.workers.dev/

Please let me know if there's anything else I can do to help!

@wellwelwel
Copy link
Collaborator

wellwelwel commented Feb 28, 2025

Please let me know if there's anything else I can do to help!

@thomasgauvin, thanks for the update! I suppose all that's missing is just documenting disableEval option. Also, in order to check the condition mentioned by @Mini256, I'll test your repository through an explcit SSL connection.

Once this is confirmed, we can officially say that MySQL2 supports Cloudflare Workers by release in fact the canary version 🚀

@thomasgauvin
Copy link
Contributor

thomasgauvin commented Mar 1, 2025

@wellwelwel got it! Yes, I can confirm that ✘ [ERROR] Uncaught (in response) Error: [unenv] tls.createSecureContext is not implemented yet! still occurs in the most recent workerd/wrangler releases, and we're working on this. I'll make sure to provide updates in this thread with our progress here

@wellwelwel wellwelwel added needs documentation needs rebase For internal organization purpose and removed needs tests labels Mar 5, 2025
@thomasgauvin
Copy link
Contributor

thomasgauvin commented Mar 6, 2025

@wellwelwel update on this thread: creating tls context is not something that can be done from within the Cloudflare Workers runtime because it is the Cloudflare runners (which are responsible for running Workers isolates) that establish the outbound connections

What we're thinking here is to make it possible to set ssl certs in a Hyperdrive configuration rather than in the code itself. Bindings, such as the Hyperdrive binding or the mTLS binding, is usually how we resolve these limitations. Then we'd recommend folks use mysql2 to connect using Hyperdrive, leaving the ssl option undefined in the driver itself. (Hyperdrive is a connection pooler that also does query caching, which we're going make available on the free plan for this)

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 6, 2025

What we're thinking here is to make it possible to set ssl certs in a Hyperdrive configuration rather than in the code itself. Bindings, such as the Hyperdrive binding or the mTLS binding, is usually how we resolve these limitations. Then we'd recommend folks use mysql2 to connect using Hyperdrive, leaving the ssl option undefined in the driver itself. (Hyperdrive is a connection pooler that also does query caching, which we're going make available on the free plan for this)

@Mini256, I believe we can remove network wrappers and update the "nodejs_compat" flag usage in the examples:

{
  "compatibility_flags": ["nodejs_compat"],
}

As Hyperdrive will be responsible for the secure TLS context with Cloudflare Workers, we can release the canary version with the documentation you have created. What do you think?

@Mini256
Copy link
Contributor Author

Mini256 commented Mar 6, 2025

@wellwelwel LGTM, let me rebase this PR and update the docs.

@Mini256 Mini256 force-pushed the add-static-text-parser branch from 5dd33a2 to a514556 Compare March 6, 2025 03:08
@wellwelwel wellwelwel added documentation and removed needs rebase For internal organization purpose labels Mar 6, 2025
@Mini256
Copy link
Contributor Author

Mini256 commented Mar 6, 2025

@thomasgauvin According to the Hyperdrive documentation, it currently only supports the Postgres protocol, right? Are there any plan to support the MySQL and the MySQL protocol-compatible database (like TiDB Serverless, PlanetScale)

@wellwelwel wellwelwel self-requested a review March 6, 2025 04:53
@wellwelwel wellwelwel merged commit a79253d into sidorares:master Mar 6, 2025
101 checks passed
@Mini256 Mini256 deleted the add-static-text-parser branch March 6, 2025 05:11
@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 6, 2025

Thanks to everyone for your patience and great teamwork. Version 3.13.0 is out now 🎉

@thomasgauvin
Copy link
Contributor

@thomasgauvin According to the Hyperdrive documentation, it currently only supports the Postgres protocol, right? Are there any plan to support the MySQL and the MySQL protocol-compatible database (like TiDB Serverless, PlanetScale)

@Mini256, yes, getting the runtime to work with MySQL drivers goes hand in hand with our work to support MySQL on Hyperdrive. We're still working on it but I should be able to get any of you early access if you email me (first initial last name at cloudflare.com) or drop a chat in our Discord channel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Cloudflare workers
10 participants