-
Notifications
You must be signed in to change notification settings - Fork 637
EW-9327 Implement connection string override support #6742
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||
| #include "worker-rpc.h" | ||||||
| #include "workerd/jsg/jsvalue.h" | ||||||
|
|
||||||
| #include <workerd/api/global-scope.h> | ||||||
| #include <workerd/io/features.h> | ||||||
| #include <workerd/io/io-context.h> | ||||||
| #include <workerd/jsg/ser.h> | ||||||
|
|
@@ -2067,6 +2068,33 @@ jsg::Ref<Socket> Fetcher::connect( | |||||
| return connectImpl(js, JSG_THIS, kj::mv(address), kj::mv(options)); | ||||||
| } | ||||||
|
|
||||||
| void Fetcher::setPort(uint16_t p) { | ||||||
| KJ_ASSERT(port == kj::none); | ||||||
| port = p; | ||||||
| } | ||||||
|
|
||||||
| jsg::Optional<kj::StringPtr> Fetcher::getHost(jsg::Lock& js) { | ||||||
| if (port == kj::none) { | ||||||
| return kj::none; | ||||||
| } | ||||||
| if (!registeredConnectOverride) { | ||||||
| kj::FixedArray<kj::byte, 16> randomBytes; | ||||||
| workerd::getEntropy(randomBytes); | ||||||
| randomHost = kj::str(kj::encodeHex(randomBytes), ".hyperdrive.local"); | ||||||
|
|
||||||
| // Returns the random hostname and ensures the connect override is registered on the | ||||||
| // ServiceWorkerGlobalScope for the Worker. This getter has a side effect: it registers (or | ||||||
| // re-registers) an entry in the ServiceWorkerGlobalScope's connectOverrides HashMap so that | ||||||
| // cloudflare:sockets's connect() will route connections to this magic hostname through the | ||||||
| // fetcher. | ||||||
| auto& globalScope = IoContext::current().getCurrentLock().getGlobalScope(); | ||||||
| globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()), | ||||||
| [self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", self->port), kj::none); }); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH]
Suggested change
|
||||||
| registeredConnectOverride = true; | ||||||
| } | ||||||
| return randomHost; | ||||||
| } | ||||||
|
|
||||||
| jsg::Promise<jsg::Ref<Response>> Fetcher::fetch(jsg::Lock& js, | ||||||
| kj::OneOf<jsg::Ref<Request>, kj::String> requestOrUrl, | ||||||
| jsg::Optional<kj::OneOf<RequestInitializerDict, jsg::Ref<Request>>> requestInit) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -263,10 +263,14 @@ class Fetcher: public JsRpcClientProvider { | |
| // URL that has no protocol or host. | ||
| // | ||
| // See pipeline.capnp or request-context.h for an explanation of `isInHouse`. | ||
| explicit Fetcher(uint channel, RequiresHostAndProtocol requiresHost, bool isInHouse = false) | ||
| explicit Fetcher(uint channel, | ||
| RequiresHostAndProtocol requiresHost, | ||
| bool isInHouse = false, | ||
| kj::Maybe<uint16_t> connectionStringOverridePort = kj::none) | ||
| : channelOrClientFactory(channel), | ||
| requiresHost(requiresHost), | ||
| isInHouse(isInHouse) {} | ||
| isInHouse(isInHouse), | ||
| port(connectionStringOverridePort) {} | ||
|
|
||
| // Create a Fetcher bound to an IoChannelFactory::SubrequestChannel object rather than a numeric | ||
| // channel. This Fetcher will inherently be bound to the current I/O context. | ||
|
|
@@ -440,6 +444,12 @@ class Fetcher: public JsRpcClientProvider { | |
| rpc::JsRpcTarget::Client getClientForOneCall( | ||
| jsg::Lock& js, kj::Vector<kj::StringPtr>& path) override; | ||
|
|
||
| void setPort(uint16_t port); | ||
| jsg::Optional<uint16_t> getPort() const { | ||
| return port; | ||
| } | ||
| jsg::Optional<kj::StringPtr> getHost(jsg::Lock& js); | ||
|
|
||
| JSG_RESOURCE_TYPE(Fetcher, CompatibilityFlags::Reader flags) { | ||
| // WARNING: New JSG_METHODs on Fetcher must be gated via compatibility flag to prevent | ||
| // conflicts with JS RPC methods (implemented via the wildcard property). Ideally, we do not | ||
|
|
@@ -451,6 +461,9 @@ class Fetcher: public JsRpcClientProvider { | |
| JSG_METHOD(fetch); | ||
| JSG_METHOD(connect); | ||
|
|
||
| JSG_LAZY_READONLY_INSTANCE_PROPERTY(host, getHost); | ||
| JSG_LAZY_READONLY_INSTANCE_PROPERTY(port, getPort); | ||
|
Comment on lines
+464
to
+465
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Missing compat flag gate — shadows JS RPC wildcard property. The comment right above (lines 453–459) warns:
This applies equally to properties. These should be gated behind a compat flag, or alternatively moved to a subclass that only the connection-string-override Fetcher uses (not all Fetchers globally). |
||
|
|
||
| if (flags.getServiceBindingExtraHandlers()) { | ||
| JSG_METHOD(queue); | ||
| JSG_METHOD(scheduled); | ||
|
|
@@ -527,6 +540,12 @@ class Fetcher: public JsRpcClientProvider { | |
| channelOrClientFactory; | ||
| RequiresHostAndProtocol requiresHost; | ||
| bool isInHouse; | ||
|
|
||
| // used for optional connection string override. port being non-none indicates that an override | ||
| // has been requested. | ||
| bool registeredConnectOverride = false; | ||
| jsg::Optional<kj::String> randomHost; | ||
| jsg::Optional<uint16_t> port = kj::none; | ||
| }; | ||
|
|
||
| // Type of the second parameter to Request's constructor. Also the type of the second parameter | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LOW] Consider using
getPort()rather than accessingportfield directly for consistency.The Hyperdrive implementation calls
getPort()for both thesetConnectOverridekey and the lambda body. Here you mixgetPort()(line 2091) and directself->portaccess (line 2092). Usingself->getPort()in the lambda would also need unwrapping but would be more consistent.Also: this line calls
getPort()which returnsjsg::Optional<uint16_t>—kj::str()with a Maybe will produce unexpected output here too. Should unwrap: