Surface UDP peer address and declare UDP listeners to workers - #7503
Open
guybedford wants to merge 2 commits into
Open
guybedford wants to merge 2 commits into
guybedford wants to merge 2 commits into
Conversation
The UDP connect() handler received no remoteAddress: the listener's Flow knew the peer but it was dropped before reaching setupDatagramSocket(). Thread it through UdpConnectCustomEvent (and the udpConnect RPC params) so socket.opened.remoteAddress reports the peer, and set clientAddress on the request metadata as the TCP listener does. The local address handed to the handler was the raw config string (e.g. "*:0") rather than the bound endpoint; use the same host:boundPort authority the TCP listener uses. UDP sockets are now registered in inboundListeners with protocol "udp", so getInboundListeners() declares them alongside TCP ones.
Contributor
|
LGTM |
The peer address reaches the UDP connect event the way it reaches TCP connect(): via SubrequestMetadata.clientAddress into WorkerEntrypoint, which now stores it on IoContext_IncomingRequest for custom events to read, rather than as a udpConnect RPC parameter. For workerd-to-workerd RPC, startEvent() carries the client address alongside the cf blob.
Contributor
|
I don't have the permission to formally approve anymore, but everything looks good to me! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fills in two gaps in the inbound UDP
connect()handler from #7130 that are prerequisites for routing UDP flows through anode:dgramlayer the waynode:netconsumes TCP listeners.socket.opened.remoteAddressnow reports the peeraddress:port. The listener'sFlowalready knew the peer but it was dropped beforesetupDatagramSocket(); it is now threaded throughUdpConnectCustomEventand theudpConnectRPC params, andclientAddressis set on the request metadata as the TCP listener does.socket.opened.localAddressis the boundhost:portauthority rather than the raw config string (e.g.*:0), matching TCP.inboundListenerswith protocol"udp", sogetInboundListeners()declares them alongside TCP ones.Tests:
udp-connectgains a remote/local address round-trip;net-server-nodejs-testadds a UDP listener to its config to check it does not land in the TCP port table.