Add Hyperdrive bindings by tmthecoder · Pull Request #1266 · cloudflare/workerd

tmthecoder · 2023-10-03T15:02:02Z

Adda a new binding type Hyperdrive as well as a corresponding Service definition ExternalTcpService that allows TCP connections to the designated host address & port

github-actions · 2023-10-03T15:02:19Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tmthecoder · 2023-10-03T15:08:57Z

I have read the CLA document and I hereby sign the CLA

kentonv · 2023-10-03T15:39:41Z

      # If `certificateHost` is not provided, then the certificate is checked against `address`.
    }

+    tcp :group {


Please add doc comments.

I assume this is something like:

Connect to the server over raw TCP. Bindings to this service will only support the connect() method; fetch() will throw an exception.

kentonv · 2023-10-03T15:41:40Z

+                     kj::HttpHeaderTable& headerTable,
+                     kj::Timer& timer, kj::EntropySource& entropySource)
+    : addr(kj::mv(addrParam)),
+      inner(kj::newHttpClient(timer, headerTable, *addr, {


Isn't the point of this class that it should form raw TCP connections, not HTTP? I'd expect request() throws an exception, and connect() calls addr->connect() without using an HttpClient.

kentonv · 2023-10-03T15:42:51Z

+                       kj::String user, kj::String password)
+    : clientIndex(clientIndex), database(kj::mv(database)),
+      user(kj::mv(user)), password(kj::mv(password)) {
+        randomHost = randomUUID(kj::none);


Does this actually need to be random? Could it instead just be a static fake string value?

Wouldn't that cause issues if there were 2+ Hyperdrive bindings?

Oh sorry, I missed that this is being used with setConnectOverride(). In that case, can we please generate the hostname the same way we do in the internal codebase? Matching behavior as much as possible is helpful for testing purposes.

randomHost = kj::str(kj::encodeHex(randomBytes), ".hyperdrive.local");

I didn't see a random byte generation function so I went with the UUID, should I port one over or use the UUID with the .hyperdrive.local appended?

I think we should match the formatting. UUIDs have a specific format which is not the same as a hex string.

Take a look at how randomUUID() itself generates random bytes -- it calls OpenSSL's RAND_bytes(). You can do the same here.

Ah alright. Should be good now

jasnell · 2023-10-03T16:59:22Z

Marking this with needs-internal-pr to ensure there is an internal CI run done. Feel free to remove that label once this has gone through at least one successful internal CI run.

tmthecoder · 2023-10-05T15:18:03Z

Marking this with needs-internal-pr to ensure there is an internal CI run done. Feel free to remove that label once this has gone through at least one successful internal CI run.

It passed internally, so I think the tag's set to be removed (I can't remove it though)

a-robinson

Looks good, just a couple minor things left. Thanks for confirming that it is actually able to successfully establish connections. Did you test what happens when trying to connect to an address that isn't listening?

a-robinson · 2023-10-12T17:51:30Z

@@ -0,0 +1,106 @@
+#include "hyperdrive.h"


Looks like we still need to add a copyright header here

a-robinson · 2023-10-12T17:52:41Z

+    try {
+      auto errorBody = co_await e->readAllText();
+      kj::throwFatalException(KJ_EXCEPTION(
+          FAILED, kj::str("unexpected error connecting to SQC from process sandbox: ", errorBody)));


Not quite -- we aren't connecting from a "process sandbox" here. I'd just remove the "from process sandbox" part of these log messages

a-robinson · 2023-10-12T17:55:29Z

    JSG_LAZY_READONLY_INSTANCE_PROPERTY(database, getDatabase);
    JSG_LAZY_READONLY_INSTANCE_PROPERTY(user, getUser);
    JSG_LAZY_READONLY_INSTANCE_PROPERTY(password, getPassword);
+    JSG_LAZY_READONLY_INSTANCE_PROPERTY(scheme, getScheme);


We don't want to be in the business of supporting more public fields/methods in workerd than in the production workers runtime. We should avoid exposing this for now.

a-robinson · 2023-10-12T18:03:06Z

+    f.fulfill(kj::none);
+    return kj::mv(stream);
+  }, [&f = *paf.fulfiller](kj::Exception e) {
+    KJ_LOG(WARNING, "failed to connect to local hyperdrive process", e);


Was it intentional that this was left as is?

a-robinson · 2023-10-12T18:04:20Z

+  kj::String password;
+  kj::String scheme;
+  bool registeredConnectOverride = false;
+  kj::Promise<kj::Own<kj::AsyncIoStream>> connectToDb();


Style nit, but it'd be nice to put an empty line in between the class's member variables and any private functions just to more cleanly visually separate them. When scanning over this I initially was trying to figure out why we needed a connectToDb variable before I stopped to take a closer look.

a-robinson · 2023-10-12T18:24:18Z

+  auto connectReq = kj::newHttpClient(*service)->connect(
+    kj::str(getHost(), ":", getPort()), headers, kj::HttpConnectSettings{});
+
+  // auto conectionRequest =


Forgot to delete this

a-robinson

Thanks @tmthecoder !

a-robinson · 2023-10-12T21:25:15Z

Just to be clear, I'll merge this as soon as the tests are green, but given that your branch is on your fork rather than the main repo, the builds are likely to take a few hours.

tmthecoder · 2023-10-12T21:39:55Z

@a-robinson alright, sounds good. Thanks!

a-robinson · 2023-10-12T21:57:04Z

Aaaand I forgot to squash the commits down :( I'm sorry for making the history messier, anyone who's reading this.

Support is already in progress (see cloudflare/workerd#1266) and should be coming in the next few weeks, but we've already had at least one user get very confused by this and cause us to spend time on debugging before realizing the problem was that they were talking about problems in wrangler dev rather than at the edge. This is an easy way to attempt to mitigate that for now.

…g does not work Description Hyperdrive is supported locally (see cloudflare/workerd#1266) and this warning is not longer accurate. It was added while the support was in progress to avoid confusion (cloudflare@54800f6), but now that local support for Hyperdrive bindings has been added, this log is incorrect and should be removed

…g does not work (#5812) * Remove Hyperdrive warning log indicating that Hyperdrive local binding does not work Description Hyperdrive is supported locally (see cloudflare/workerd#1266) and this warning is not longer accurate. It was added while the support was in progress to avoid confusion (54800f6), but now that local support for Hyperdrive bindings has been added, this log is incorrect and should be removed * Add changeset * Update .changeset/thirty-kings-serve.md Co-authored-by: Matt <granjef3@gmail.com> * Nit changeset fix --------- Co-authored-by: Matt <granjef3@gmail.com>

thomasgauvin · 2024-08-24T04:16:52Z

+kj::String Hyperdrive::getConnectionString() {
+  return kj::str(getScheme(), "://", getUser(), ":", getPassword(), "@", getHost(), ":", getPort(),
+                 "/", getDatabase(), "?sslmode=disable");
+}


@tmthecoder @a-robinson this hardcodes sslmode=disable. This prevents connection to remote databases such as Azure, Neon, etc. when developing locally. Can we have a way to connect to these databases which needs sslmode=require, while retaining the ability to connect to local Postgres with does need sslmode=disable?

Can we have a way to connect to these databases which needs sslmode=require, while retaining the ability to connect to local Postgres with does need sslmode=disable?

If you want to support remote databases that require SSL but also local DBs that don't support it, it sounds like what we want for local mode is sslmode=prefer, which allows SSL but doesn't require it.

It should be possible to vary the default sslmode in workerd compared to on the edge, but I'd suggest bringing it up internally either in chat or as a ticket so we don't lose track of it.

And I wouldn't expect Tejas to respond given that his internship ended last year :)

tmthecoder force-pushed the tejas/add-hyperdrive-bindings branch from 392a5ff to bef20bd Compare October 3, 2023 15:08

github-actions bot added a commit that referenced this pull request Oct 3, 2023

@tmthecoder has signed the CLA from Pull Request #1266

a993914