containers: Be able to send channel tokens to a container service #5939
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
gabivlj
force-pushed
the
gv/set-egress-binding
branch
7 times, most recently
from
bf34f06 to
608a51b
Compare
gabivlj
changed the title
…ccess to Workers from Containers
gabivlj
force-pushed
the
gv/set-egress-binding
branch
from
608a51b to
68718cb
Compare
| egressMappings.upsert(kj::mv(addr), kj::mv(subrequestChannel), | ||
| [](auto& existing, auto&& newValue) { existing = kj::mv(newValue); }); | ||
|
|
||
| // TODO: At some point we need to figure out how to make it so |
There was a problem hiding this comment.
I think we need to fully implement this for local testing now. If it's not available in local testing then nobody will be able to code against this feature.
There was a problem hiding this comment.
Responding to both comments about local dev here (@danlapid):
- I agree for the feature local dev shouldn't feel like an after-thought.
- That's the intent of the experimental flag, I think we can experiment the feature on our end and see if it's working as intended, and iterate on both production and local dev in the meantime.
There was a problem hiding this comment.
If you had local dev support you could iterate super fast 😄
There was a problem hiding this comment.
Independently of local-dev support being a must for this feature to be publicly available, I think it's not really equivalent the path of connectivity to workers in local dev and in prod.
There was a problem hiding this comment.
I don't really see the benefit of rushing to prod before implementing local dev.
It's not really in doubt whether or not it will work in prod. But it'll be hard to write code to test it in prod if we don't also have local dev.
| egressMappings.upsert(kj::mv(addr), kj::mv(subrequestChannel), | ||
| [](auto& existing, auto&& newValue) { existing = kj::mv(newValue); }); | ||
|
|
||
| // TODO: At some point we need to figure out how to make it so |
There was a problem hiding this comment.
I don't think this should be left as "at some point".
Local dev can't be an afterthought.
| void signal(jsg::Lock& js, int signo); | ||
| jsg::Ref<Fetcher> getTcpPort(jsg::Lock& js, int port); | ||
| jsg::Promise<void> setInactivityTimeout(jsg::Lock& js, int64_t durationMs); | ||
| void setEgressTcp(jsg::Lock& js, kj::String addr, jsg::Ref<Fetcher> binding); |
There was a problem hiding this comment.
Hmm, should we call this API setEgressHttp() considering that we're going to interpret it as HTTP?
In the future when we support connect handlers then we could actually have an option to support raw TCP, but it would have to be a separate method since we need the app to tell us whether to try parsing the input as HTTP or not.
There was a problem hiding this comment.
Another method name idea: interceptOutboundHttp
There was a problem hiding this comment.
This is a good idea. We could also instead have it be at a 'domain'/'hostname' level instead of IP port.
4 participants
When the user calls
setEgressTcp, they will be saying "I want all TCP requests to this IP port to be sent to this WorkerEntrypoint".For now, this feature is marked as 'experimental' as we test it out while its getting released.
The docker client just adds it to a local map of 'address mappings' and calls method to exchange the token for a subrequest channel. It does not really do much.
I used Claude Opus to generate some of this code with some guidance, and ofc reviewed by me.