SQC-652 Implement Hyperdrive TLS proxy in Miniflare#11219
Conversation
🦋 Changeset detectedLatest commit: d8209a2 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
0d78b14 to
d3980b4
Compare
7814814 to
3c93c1a
Compare
d2b0005 to
8868c95
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
A few questions and suggestions.
But it looks like this is a legitimate failure in CI?
https://github.com/cloudflare/workers-sdk/actions/runs/19313849941/job/55240551979?pr=11219#step:6:3822
packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts
Outdated
Show resolved
Hide resolved
8868c95 to
fd426a0
Compare
Yes this actually was a failure, I had missed this test but should be fixed now |
fdc7f10 to
48ef6d2
Compare
|
@petebacondarwin think I could get a re-review on this? I fixed some more tests and cleaned up some issues when testing more edge cases. I think currently the tests failing are flaky tests? |
|
Few minor comments, but LGTM! |
9880761 to
90a48f6
Compare
packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts
Outdated
Show resolved
Hide resolved
vicb
left a comment
There was a problem hiding this comment.
LGTM.
I have added mostly minor comments / suggestions.
If one thing, I feel like there is quite some code duplication across the PR.
7f890bc to
b4c59ce
Compare
b4c59ce to
7a976dd
Compare
dario-piotrowicz
left a comment
There was a problem hiding this comment.
I've had a pretty superficial review of this since this is already approved by two people and also because I don't have much context of this.
Generally looks good to me 😄
| Implement Hyperdrive binding TLS miniflare proxy. This will allow for wrangler dev hyperdrive bindings to connect to external | ||
| databases that require TLS. |
There was a problem hiding this comment.
nit:
| Implement Hyperdrive binding TLS miniflare proxy. This will allow for wrangler dev hyperdrive bindings to connect to external | |
| databases that require TLS. | |
| Implement Hyperdrive binding TLS miniflare proxy. This will allow for wrangler dev hyperdrive bindings to connect to external databases that require TLS. |
petebacondarwin
left a comment
There was a problem hiding this comment.
Cool. Let's give this a go.
allow for wrangler dev hyperdrive bindings to connect to external databases that require TLS. - Supports upgrading TLS connection to database by setting sslmode=prefer or sslmode=require or sslmode=disable in localConnectionString - Default sslmode is disable - Adds integration tests to miniflare test suite
7a976dd to
d8209a2
Compare
Fixes SQC-652.
This fixes the problem of when a user runs
npx wrangler devwith a Hyperdrive binding and
localConnectionStringpointing to a database that requires TLS. In such cases, the command will result in the following TLS error:This change adds a proxy layer to Miniflare and introduces a proxy service for Hyperdrive bindings. The proxy layer attempts to establish a TLS connection with the appropriate database based on the specified sslmode parameter.
This allows users to simply set
sslmode=requirein theirlocalConnectionString, enabling their Hyperdrive binding to automatically upgrade to TLS instead of requiring them to manually configure TLS settings in their database driver. (Note: this still will not work without additional changes to the binding service.)