end host, router: dispatch UDP in router, remove dispatcher socket #4344
Conversation
There was a problem hiding this comment.
Thank you or this contribution, great to see action on this topic!
This PR now contains multiple breaking changes and we'll need to figure out how to merge and deploy these with the least amount of friction. The implementation plan in the design document has some proposal for this, but its never been discussed much and there seem to be some concerns compatibility. As far as I can see, the main question is whether we should make provisions for running old routers (i.e. routers not supporting the port dispatching) together with new services / applications. By removing the dispatcher entirely (wow, -11k LoC 🎉), we wouldn't have that option.
Reviewable status: 0 of 183 files reviewed, 3 unresolved discussions (waiting on @JordiSubira)
pkg/snet/snet.go line 69 at r1 (raw file):
func (d *DefaultConnector) OpenUDP(addr *net.UDPAddr) (PacketConn, error) { pconn, err := conn.OpenConn(addr)
I don't think it's worth using the underlay/conn wrapper here. The main functionality provided by that wrapper is that it exposes a batched read/write call, but these are hidden away by the net.PacketConn interface. I'd use net.UDPConn directly here.
Setting the receive and send buffer sizes is not critical for the individual application sockets, and the same huge buffer sizes probably no longer make sense. In fact, they can relatively easily be set by the SetReadBuffer/SetWriteBuffer functions on the UDPConn. If we expose these functions and the SyscallConn from SCIONPacketConn, we can give the applications more direct control over these buffers -- in particular, quic-go could then set its desired buffer sizes.
private/topology/topology.go line 39 at r1 (raw file):
// TODO(JordiSubira): Yet to be defined. HostPortRangeLow = 0 HostPortRangeHigh = 1<<16 - 1
My understanding was that these ranges would be configurable in the topology configuration, maybe even allowing different ranges for different IP subnets.
router/dataplane.go line 1466 at r1 (raw file):
return &net.UDPAddr{IP: dst.IP, Port: topology.EndhostPort}, nil default: panic(fmt.Sprintf("Port rewriting not supported for protcol number %v", l4Type))
That's just for debugging, right? 😉
I think in any error cases it should just fall back to the default endhost port.
JordiSubira
force-pushed
the
dispatcher_off
branch
from
1a1a9a6
to
8940c4e
Compare
JordiSubira
force-pushed
the
dispatcher_off
branch
2 times, most recently
from
c3bb862
to
93b4afb
Compare
There was a problem hiding this comment.
I added this shim component (it is still under the package disptacher, we can call it differently if this would make things clearer).
Reviewable status: 0 of 190 files reviewed, 3 unresolved discussions (waiting on @matzf)
pkg/snet/snet.go line 69 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I don't think it's worth using the
underlay/connwrapper here. The main functionality provided by that wrapper is that it exposes a batched read/write call, but these are hidden away by thenet.PacketConninterface. I'd usenet.UDPConndirectly here.Setting the receive and send buffer sizes is not critical for the individual application sockets, and the same huge buffer sizes probably no longer make sense. In fact, they can relatively easily be set by the
SetReadBuffer/SetWriteBufferfunctions on the UDPConn. If we expose these functions and theSyscallConnfromSCIONPacketConn, we can give the applications more direct control over these buffers -- in particular, quic-go could then set its desired buffer sizes.
Done.
router/dataplane.go line 1466 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
That's just for debugging, right? 😉
I think in any error cases it should just fall back to the default endhost port.
Done.
There was a problem hiding this comment.
Reviewed 93 of 183 files at r1, 67 of 81 files at r2, 29 of 29 files at r3, all commit messages.
Reviewable status: 189 of 190 files reviewed, 20 unresolved discussions (waiting on @matzf)
dispatcher/dispatcher.go line 41 at r3 (raw file):
// ErrMalformedL4Quote common.ErrMsg = "malformed L4 quote" // ReceiveBufferSize is the size of receive buffers used by the dispatcher. ReceiveBufferSize = 1 << 20
Remove it is unused
dispatcher/dispatcher.go line 43 at r3 (raw file):
ReceiveBufferSize = 1 << 20 // SendBufferSize is the size of the send buffers used by the dispatcher. SendBufferSize = 1 << 20
Remove it is unused
dispatcher/dispatcher.go line 46 at r3 (raw file):
) // Server is the main object allowing to create new SCION connections.
Update description
dispatcher/cmd/dispatcher/main.go line 57 at r3 (raw file):
func realMain(ctx context.Context) error { path.StrictDecoding(false) topos, err := globalCfg.Topolgies(ctx)
Same as in other comment, give support for ignoring having access to the topology file, if we are not running on SVC environement (CS, DS, test environment...).
dispatcher/cmd/dispatcher/main_test.go line 16 at r3 (raw file):
// limitations under the License. package main
Keep or delete?
dispatcher/config/config.go line 113 at r3 (raw file):
} func (cfg *Config) Topolgies(ctx context.Context) (map[addr.AS]*topology.Loader, error) {
See previous comments.
dispatcher/internal/metrics/metrics.go line 15 at r3 (raw file):
// limitations under the License. package metrics
Redo?
gateway/gateway.go line 477 at r3 (raw file):
LocalIA: localIA, Connector: &snet.DefaultConnector{ // Enable transparent reconnections to the dispatcher
Remove
gateway/pathhealth/pathwatcher.go line 81 at r3 (raw file):
return create(remote) } // FIXME(JordiSubira): Keep or change the listening port, once we decide
Remove comment
private/app/path/pathprobe/paths.go line 106 at r3 (raw file):
// should not be set, unless you know what you are doing. LocalIP net.IP // Metrics injected into snet.SCIONPacketConnMetrics.
DefaultConnector
Code quote:
SCIONPacketConnMetricsprivate/env/logging.go line 36 at r3 (raw file):
func LogAppStarted(svcType, elemID string) error { inDocker := false if runtime.GOOS == "linux" {
Add comment to clarify. Right now RunsInDocker() only is compatible with Linux. If we are going to run apps in docker also in macOS (and potentially in Windows) we should make the method compatible.
private/service/statuspages.go line 151 at r3 (raw file):
info := env.VersionInfo() inDocker := false if runtime.GOOS == "linux" {
Same as in other code snippet.
router/dataplane.go line 1769 at r3 (raw file):
port = topology.EndhostPort } log.Debug("TBR XXXJ:", "udp port that will be rewritten", port)
Remove
router/dataplane.go line 1772 at r3 (raw file):
return &net.UDPAddr{IP: dst, Port: int(port)}, nil case slayers.L4SCMP: // TODO(JordiSubira): On-going discussion regarding SCMP dst port
Remove comment and log
tools/topology/docker_utils.py line 59 at r3 (raw file):
return self.dc_conf # TODO(JordiSubira): Remove container if not needed
Remove
tools/topology/go.py line 173 at r3 (raw file):
return raw_entry def generate_disp(self, topo_dicts):
Seems to be unused
Code quote:
topo_dictstools/topology/go.py line 203 at r3 (raw file):
'dispatcher': { 'id': name, "config_dir": config_dir,
The config_dir is the path were the topology.json file is located. This is only needed when the dispatcher is running to give support to the SVC services (e.g. SVC_CS). We have to add support for ignoring this and do not break at starting.
There was a problem hiding this comment.
Reviewed 11 of 12 files at r4, all commit messages.
Reviewable status: 189 of 190 files reviewed, 8 unresolved discussions (waiting on @matzf and @oncilla)
There was a problem hiding this comment.
Reviewable status: 183 of 190 files reviewed, 9 unresolved discussions (waiting on @matzf and @oncilla)
dispatcher/dispatcher.go line 146 at r5 (raw file):
s.outBuffer.PushLayer(s.scmpLayer.LayerType()) case slayers.LayerTypeSCIONUDP: dstAddrPort, err = s.getDstSCIONUDP()
We MAY want to check that the underlay IP address (in the UDP/IP layer) is the same IP as in the inner SCION/UDP header.
There was a problem hiding this comment.
Reviewed 87 of 183 files at r1, 37 of 81 files at r2, 13 of 29 files at r3, 4 of 12 files at r4, 1 of 7 files at r5, 2 of 2 files at r6, 2 of 3 files at r7, 62 of 73 files at r8, 20 of 20 files at r9, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @JordiSubira, @lukedirtwalker, @matzf, and @oncilla)
control/cmd/control/main.go line 688 at r9 (raw file):
}) cleanup.Add(func() error { tcpServer.GracefulStop(); return nil }) if hpInterASServer != nil {
We have three types of spelling in the error messages here:
- "Hidden path"
- "HP"
- "Hidden Path"
dispatcher/dispatcher.go line 100 at r9 (raw file):
s.buf = s.buf[:cap(s.buf)] n, nn, _, previousHop, err := s.conn.ReadMsgUDPAddrPort(s.buf, s.oobuf)
Or just prevHop?
Code quote:
previousHopdispatcher/dispatcher.go line 402 at r9 (raw file):
} // validateNextHopAddr returnS true if the underlay address on the UDP/IP wrapper
returns
Code quote:
returnSdispatcher/dispatcher_test.go line 40 at r9 (raw file):
func testRunTestCase(t *testing.T, tc testCase) {
Remove empty line
dispatcher/cmd/dispatcher/main_test.go line 16 at r3 (raw file):
Previously, JordiSubira wrote…
Keep or delete?
Imo, it doesn't make sense to keep fully commented out files, we can still look up this information in the git history.
dispatcher/internal/metrics/metrics.go line 15 at r3 (raw file):
Previously, JordiSubira wrote…
Redo?
What's the state of this file? Could it also be removed?
pkg/daemon/grpc.go line 86 at r9 (raw file):
func (c grpcConn) Interfaces(ctx context.Context) (map[uint16]*net.UDPAddr, error) {
Remove empty line?
pkg/experimental/hiddenpath/discovery.go line 67 at r9 (raw file):
// CSResolver resolves the address of a Control Service // server in an IA. This is needed to get the needed // certificates from the Register to verify the segments.
To what exactly does "Register" refer here?
Code quote:
from the Registerpkg/snet/packet_conn.go line 212 at r9 (raw file):
lastHop, err := c.lastHop(pkt) if err != nil { return serrors.WrapStr("extracting next hop based on packet path", err)
"last"?
Code quote:
nextpkg/snet/snet.go line 101 at r9 (raw file):
SCMPHandler: d.SCMPHandler, Metrics: d.Metrics, getLastHopAddr: func(id uint16) (*net.UDPAddr, error) {
Why not explicitly add a reference to the controller to SCIONPacketConn and make getLastHopAddr a normal helper function instead of capturing ifAddrs implicitly here?
pkg/snet/snet.go line 127 at r9 (raw file):
return pconn, nil } return nil, serrors.New("There are no UDP ports available in range", "start", start, "end", end)
Or just syscall.EADDRINUSE for consistency with bind(2), https://man7.org/linux/man-pages/man2/bind.2.html?
Code quote:
serrors.New("There are no UDP ports available in range", "start", start, "end", end)private/topology/json/json.go line 79 at r9 (raw file):
MTU int `json:"mtu"` EndhostStartPort int `json:"endhost_start_port"` EndhostEndPort int `json:"endhost_end_port"`
Did we also consider only having a single field `endhost_port_range" in the JSON with a port range format like, e.g., in Caddy: https://caddyserver.com/docs/conventions#network-addresses?
Code quote:
EndhostStartPort int `json:"endhost_start_port"`
EndhostEndPort int `json:"endhost_end_port"`router/dataplane.go line 1770 at r9 (raw file):
switch l4Type { case slayers.L4UDP: // TODO(JordiSubira): Treat this as a parameter problem?
Yes, imo.
Code quote:
Treat this as a parameter problem?router/dataplane.go line 1878 at r9 (raw file):
} return 0, serrors.New("Unknown SCION SCMP content")
Remove empty line.
router/config/config.go line 76 at r9 (raw file):
"EndHostEndPort is nil; EndHostStartPort isn't") } if *cfg.EndhostStartPort < 0 {
< 1, right?
Code quote:
< 0
There was a problem hiding this comment.
Reviewed 32 of 183 files at r1, 3 of 81 files at r2, 5 of 29 files at r3, 1 of 7 files at r5, 1 of 3 files at r7, 10 of 73 files at r8, 9 of 20 files at r9, all commit messages.
Reviewable status: all files reviewed, 40 unresolved discussions (waiting on @JordiSubira, @lukedirtwalker, and @oncilla)
acceptance/old_new_br/test.py line 43 at r9 (raw file):
/ ("%s.toml" % br_as_2_id) scion.update_toml({"router.endhost_start_port": 0}, [br_as_2_file]) scion.update_toml({"router.endhost_end_port": 0}, [br_as_2_file])
Nit: combine the two update_toml calls.
acceptance/old_new_br/test.py line 47 at r9 (raw file):
def setup_start(self): super().setup_start() time.sleep(10) # Give applications time to download configurations
Who downloads config (copy-pasta comment?)? Maybe just use self.await_connectivity() instead?
control/cmd/control/main.go line 586 at r9 (raw file):
// because the client side, uses the DS to discover the address of the remote // HP server, thus it should use whatever is written in the topology file. hpInterASServer, hpWriterCfg, err := hpCfg.Setup(globalCfg.PS.HiddenPathsCfg)
How does this relate to the dispatcher removal? Isn't this a problem that we could fix separately?
dispatcher/dispatcher.go line 98 at r9 (raw file):
func (s *Server) Serve() error { for { s.buf = s.buf[:cap(s.buf)]
Nit: this seems superfluous, we never resize s.buf anyway.
dispatcher/dispatcher.go line 118 at r9 (raw file):
// If the outgoing message is SCMP Informational message (Echo or Traceroute) // the dispatcher has already processed the response, and the nextHop address // belongs to the BR.
Move the validateNextHopAddr call into the processing function, so we don't need determine which case we're in here.
For this, perhaps it makes sense to parse the control message stuff first, and pass the destination address into the processMsgNextHop call.
dispatcher/dispatcher.go line 203 at r9 (raw file):
return nil, nil } s.outBuffer.PushLayer(s.udpLayer.LayerType())
There does not seem to be a reason to perform this packet re-serialization in the forwarding cases, we can use the unchanged input buffer.
Only the SCMP replies need to be assembled.
One idea to do this would be to restructure such that this method processMsgNextHop returns a []byte in addition to the address. For the forwarding cases, just return buf. For the SCMP reply case, assemble the packet and return s.outBuffer.Bytes().
dispatcher/dispatcher.go line 223 at r9 (raw file):
} func (s *Server) reverseSCMPInfo() error {
replyToSCMPInfoRequest?
dispatcher/dispatcher.go line 463 at r9 (raw file):
}() return <-errChan
Looks like this go routine is not needed?
Suggestion:
return dispServer.Serve()dispatcher/dispatcher.go line 500 at r9 (raw file):
} if udpAddr.AddrPort().Addr().Unmap().Is4() {
It seems a bit confusing how this interacts with dual stack IPv4/6. As far as I can tell, this should work, but I was rather confused by the setup here.
I believe, this IPv4 code path will only be taken on hosts that don't support IPv6 (because we listen on a wildcard address and with the "udp" network type). Otherwise, the IPv6 code path will take care of both 6 and 4 (with mapped addresses).
If I got this right, the structure does not need to be changed; but perhaps this could be documented in a code comment: IPv6 includes the dual stack case, IPv4 case is only fallback for IPv4-only host.
Btw. oerhaps the appropriate control message parser (ipv4ControlMessage or ipv6ControlMessage) could be initialized here too, so we don't need to figure this out again later.
dispatcher/dispatcher.go line 501 at r9 (raw file):
if udpAddr.AddrPort().Addr().Unmap().Is4() { err := ipv4.NewPacketConn(conn).SetControlMessage(ipv4.FlagDst|ipv4.FlagInterface, true)
As far as I understand, the FlagInterface requests the IfIndex to be set, but we don't actually use that.
dispatcher/config/config.go line 48 at r9 (raw file):
config.NoDefaulter // ID is the SCION element ID. This is used to choose the relevant // portion of the topology file for some services.
Nit: this does not apply here. The ID is used only for logging I think.
dispatcher/config/config.go line 56 at r9 (raw file):
// topologies for their respective ASes. Normally, in a developer/testing // environment this list may contain multiple ASes. Topologies []string `toml:"topologies,omitempty"`
If I understand correctly, from these topology files we only extract the information to resolve the service addresses.
I think I would prefer to directly list this information here instead, like this:
[dispatcher]
id = "dispatcher"
[dispatcher.service_addresses]
"1-ff00:0:1,CS" = "127.0.0.18:31006"
"1-ff00:0:5,CS" = "..."I think this would make it much clearer what is going on and what bits of information are relevant.
In the longer term, we want to change the organization of the configuration for the other services to such a style too (i.e. don't use a shared config dir and "topology.json" file, but include the relevant bits into the configuration files for the individual services).
pkg/snet/packet_conn.go line 210 at r9 (raw file):
// Get ingress interface internal address lastHop, err := c.lastHop(pkt)
Couldn't we bypass this step if the packet was received directly, i.e. not via the shim-dispatcher?
Perhaps we can look at the sender address and only do this interfaced based lookup if the packet came from localhost:endhostport?
Aside: I think it would complicate things too much to include this in the same PR, but in my opinion, we should restructure the library interface here; instead of passing this underlay address up and down through the stack, we should get rid of snet/UDPAddr.NextHop and do exactly this path-interface-based lookup when writing the packet.
Btw; maybe change this readFrom helper function to return a netip.AddrPort, instead of this somewhat strange output-parameter-passing with.
pkg/snet/snet.go line 52 at r9 (raw file):
// Controller provides local-IA control-plane information type Controller interface {
I find this name Controller quite non-descriptive. Non-blocking because I don't have a good alternative.
pkg/snet/snet.go line 175 at r9 (raw file):
} // Listen opens a PacketConn. The returned connection's ReadFrom and WriteTo methods
PacketConn (low level, raw SCION socket) and Conn (higher level, cooked UDP socket) are distinct.
Suggestion:
// Listen opens a Conn.pkg/snet/snet.go line 184 at r9 (raw file):
// connection. func (n *SCIONNetwork) Listen(ctx context.Context, network string, listen *net.UDPAddr, svc addr.SVC) (*Conn, error) {
As far as I can tell, this svc parameter no longer influences anything. Remove this (here and in Dial)?
Related: I wonder if we need to add checks to ensure that packets received are actually destined to us. If we add this, we might still need this svc -- or perhaps not, possibly this case is "filtered" out in ResolverPacketConn? If this resolverPacketConn deals with all the SVC address cases, we could get away with saying that the higher level snet.Conn does not deal with svc addresses.
pkg/snet/writer.go line 89 at r9 (raw file):
// control, the application should bind to a specific address only. if listenHostIP.Unmap().IsUnspecified() { resolvedLocal, err := ResolveLocal(nextHop.IP)
The overhead of this must be enormous. Note that previously, wildcard bind addresses have simply been forbidden. I'm not sure it makes sense to support this at all if it involves such an overhead. To do this "properly", we'd have to keep a copy of the system's routing table and keep this up to date by subscribing to changes to it -- in Linux this can be done with netlink, but it would be quite a project on it's own...
If you revert this, please also move the ResolveLocal function back to addrutil.
private/app/appnet/infraenv.go line 344 at r9 (raw file):
Dispatcher: dispatcherService, // Discard all SCMP propagation, to avoid read errors on the QUIC // client.
Please keep the comment
proto/daemon/v1/daemon.proto line 29 at r9 (raw file):
rpc Paths(PathsRequest) returns (PathsResponse) {} // Return information about an AS. // TODO(JordiSubira): Add PortRange() endpoint instead of reusing AS
This.
Code quote:
// TODO(JordiSubira): Add PortRange() endpoint instead of reusing AS
There was a problem hiding this comment.
Reviewed 1 of 20 files at r9.
Reviewable status: 198 of 232 files reviewed, 28 unresolved discussions (waiting on @lukedirtwalker, @marcfrei, @matzf, and @oncilla)
control/cmd/control/main.go line 586 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
How does this relate to the dispatcher removal? Isn't this a problem that we could fix separately?
Revisit after rebasing and move out to separate PR if applicable
control/cmd/control/main.go line 688 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
We have three types of spelling in the error messages here:
- "Hidden path"
- "HP"
- "Hidden Path"
Revisit after rebasing and move out to separate PR if applicable
dispatcher/dispatcher.go line 118 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Move the
validateNextHopAddrcall into the processing function, so we don't need determine which case we're in here.
For this, perhaps it makes sense to parse the control message stuff first, and pass the destination address into theprocessMsgNextHopcall.
Done.
dispatcher/dispatcher.go line 203 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
There does not seem to be a reason to perform this packet re-serialization in the forwarding cases, we can use the unchanged input buffer.
Only the SCMP replies need to be assembled.One idea to do this would be to restructure such that this method
processMsgNextHopreturns a[]bytein addition to the address. For the forwarding cases, just returnbuf. For the SCMP reply case, assemble the packet and returns.outBuffer.Bytes().
Done.
dispatcher/dispatcher.go line 223 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
replyToSCMPInfoRequest?
Done.
dispatcher/dispatcher.go line 463 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Looks like this go routine is not needed?
Done.
dispatcher/dispatcher.go line 500 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
It seems a bit confusing how this interacts with dual stack IPv4/6. As far as I can tell, this should work, but I was rather confused by the setup here.
I believe, this IPv4 code path will only be taken on hosts that don't support IPv6 (because we listen on a wildcard address and with the "udp" network type). Otherwise, the IPv6 code path will take care of both 6 and 4 (with mapped addresses).
If I got this right, the structure does not need to be changed; but perhaps this could be documented in a code comment: IPv6 includes the dual stack case, IPv4 case is only fallback for IPv4-only host.Btw. oerhaps the appropriate control message parser (ipv4ControlMessage or ipv6ControlMessage) could be initialized here too, so we don't need to figure this out again later.
Yes, this is what I thought. I add the documentation part.
Done.
dispatcher/dispatcher.go line 501 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
As far as I understand, the
FlagInterfacerequests theIfIndexto be set, but we don't actually use that.
Done.
dispatcher/dispatcher_test.go line 40 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Remove empty line
Done.
dispatcher/cmd/dispatcher/main_test.go line 16 at r3 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Imo, it doesn't make sense to keep fully commented out files, we can still look up this information in the git history.
Yes, I agree I wanted to decide whether to adapt this code for the shim dispatcher or directly remove it. I think if new tests are thought to be necessary for the shim dispatcher they can be added in the newly created dispatcher_test.go which already contains some test.
dispatcher/config/config.go line 56 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
If I understand correctly, from these topology files we only extract the information to resolve the service addresses.
I think I would prefer to directly list this information here instead, like this:[dispatcher] id = "dispatcher" [dispatcher.service_addresses] "1-ff00:0:1,CS" = "127.0.0.18:31006" "1-ff00:0:5,CS" = "..."I think this would make it much clearer what is going on and what bits of information are relevant.
In the longer term, we want to change the organization of the configuration for the other services to such a style too (i.e. don't use a shared config dir and "topology.json" file, but include the relevant bits into the configuration files for the individual services).
Done.
dispatcher/internal/metrics/metrics.go line 15 at r3 (raw file):
Previously, marcfrei (Marc Frei) wrote…
What's the state of this file? Could it also be removed?
I will remove it at the moment. In the future we can add metrics to the shim dispatcher if this is desirable.
pkg/experimental/hiddenpath/discovery.go line 67 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
To what exactly does "Register" refer here?
Done. It was referring to the HP Registry.
pkg/snet/snet.go line 52 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
I find this name
Controllerquite non-descriptive. Non-blocking because I don't have a good alternative.
I changed it by CPInfoProvider, not the best name, but more descriptive I guess...
pkg/snet/snet.go line 175 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
PacketConn (low level, raw SCION socket) and Conn (higher level, cooked UDP socket) are distinct.
Done.
private/app/appnet/infraenv.go line 344 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Please keep the comment
Done.
private/topology/topology.go line 39 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
My understanding was that these ranges would be configurable in the topology configuration, maybe even allowing different ranges for different IP subnets.
Done.
router/config/config.go line 76 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
< 1, right?
I am allowing 0 (on the BR, not on the CS) to implicitly disable the port rewriting for all port, i.e., one can configure the border router toml file with portRange = [0,0]. Then all packets will be forwarded to 30041. Port 0 shouldn't be carried in any packet by standard, so it shouldn't be a conflict.
There was a problem hiding this comment.
Reviewable status: 198 of 232 files reviewed, 28 unresolved discussions (waiting on @lukedirtwalker, @marcfrei, @matzf, and @oncilla)
acceptance/old_new_br/test.py line 47 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Who downloads config (copy-pasta comment?)? Maybe just use
self.await_connectivity()instead?
Revisit after rebasing
pkg/snet/packet_conn.go line 210 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Couldn't we bypass this step if the packet was received directly, i.e. not via the shim-dispatcher?
Perhaps we can look at the sender address and only do this interfaced based lookup if the packet came from localhost:endhostport?Aside: I think it would complicate things too much to include this in the same PR, but in my opinion, we should restructure the library interface here; instead of passing this underlay address up and down through the stack, we should get rid of
snet/UDPAddr.NextHopand do exactly this path-interface-based lookup when writing the packet.Btw; maybe change this
readFromhelper function to return anetip.AddrPort, instead of this somewhat strange output-parameter-passing with.
Done. I agree with restructuring it, for me this is what makes more sense. As a temporary workaround I agree, here we can avoid the lookup.
pkg/snet/snet.go line 184 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
As far as I can tell, this
svcparameter no longer influences anything. Remove this (here and in Dial)?Related: I wonder if we need to add checks to ensure that packets received are actually destined to us. If we add this, we might still need this
svc-- or perhaps not, possibly this case is "filtered" out inResolverPacketConn? If this resolverPacketConn deals with all the SVC address cases, we could get away with saying that the higher levelsnet.Conndoes not deal with svc addresses.
Done. I followed your suggestion and moved it within ResolverPacketConn
pkg/snet/writer.go line 89 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
The overhead of this must be enormous. Note that previously, wildcard bind addresses have simply been forbidden. I'm not sure it makes sense to support this at all if it involves such an overhead. To do this "properly", we'd have to keep a copy of the system's routing table and keep this up to date by subscribing to changes to it -- in Linux this can be done with netlink, but it would be quite a project on it's own...
If you revert this, please also move the
ResolveLocalfunction back toaddrutil.
Done. Indeed, I just wanted to provide a workaround for unspecified addresses. Although, I agree if we do it, we should do it in a more sensible way.
There was a problem hiding this comment.
Reviewable status: 189 of 234 files reviewed, 25 unresolved discussions (waiting on @lukedirtwalker, @marcfrei, @matzf, and @oncilla)
pkg/snet/snet.go line 101 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Why not explicitly add a reference to the controller to
SCIONPacketConnand makegetLastHopAddra normal helper function instead of capturingifAddrsimplicitly here?
This is somehow aligned with the model discussed here: https://github.com/matzf/scion/blob/doc-endhost-overview/doc/dev/design/endhost-overview.rst?plain=1#L92-L102. The main reason is that we do not want to trigger the CPInfoProvider, which will in turn make a request to the Daemon, for every read operation. This getLastHopAddr callbacks basically hides this interface->Addr mapping within snet for the callers. If I misunderstood your point, please let me know :)
private/topology/json/json.go line 79 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Did we also consider only having a single field `endhost_port_range" in the JSON with a port range format like, e.g., in Caddy: https://caddyserver.com/docs/conventions#network-addresses?
Done
router/dataplane.go line 1770 at r9 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Yes, imo.
I will add the TODO for the future, since the parameter problem is not treated in the caller either for problems with extracting the destination address. I will give more priority to other modifications.
There was a problem hiding this comment.
Reviewed 4 of 34 files at r10, 1 of 20 files at r11, 1 of 20 files at r12.
Reviewable status: 178 of 234 files reviewed, 13 unresolved discussions (waiting on @JordiSubira, @lukedirtwalker, @marcfrei, and @oncilla)
dispatcher/dispatcher.go line 428 at r12 (raw file):
// UDP/SCION header. This refers to the safeguard for traffic reflection as discussed in: // https://github.com/scionproto/scion/pull/4280#issuecomment-1775177351 func (s *Server) parseUnderlayAddr(oobuffer []byte) *netip.Addr {
Nit: netip.Addr is designed to be passed by value (so it avoids heap allocations). Use .IsValid() instead of == nil.
dispatcher/config/config.go line 84 at r12 (raw file):
ID string `toml:"id,omitempty"` ServiceAddresses map[string]string `toml:"service_addresses,omitempty"` ParsedServiceAddresses map[addr.Addr]netip.AddrPort
With go-toml v2, this map can be marshalled directly. I'll merge #4342, then you can simplify this after rebasing. The custom parsing routine can go away too, only a check for a.Host.Type() == addr.HostTypeSvc in the Validate is left to do.
Suggestion:
ServiceAddresses map[addr.Addr]netip.AddrPort `toml:"service_addresses,omitempty"`
JordiSubira
force-pushed
the
dispatcher_off
branch
from
8b957f4
to
2082878
Compare
There was a problem hiding this comment.
Reviewable status: 155 of 242 files reviewed, 13 unresolved discussions (waiting on @lukedirtwalker, @marcfrei, @matzf, and @oncilla)
control/cmd/control/main.go line 586 at r9 (raw file):
Previously, JordiSubira wrote…
Revisit after rebasing and move out to separate PR if applicable
I took a look at #4376 and IIUC this PR would not work with that modification of the address binding for the resolver and the CS,DS service sockets. The initSVCredirect socket listens now on a dynamic port, which works fine, because it is registered into the "old" dispatcher, which then intercepts requests addresses to some SVC address and passes it through the socket that register for the concrete SVC. However, now the BR will send the messages with SVC addresses to the port specified in the topology.json. Therefore, this has to be changed in any case for this PR to work, while doing it first in a separate PR, in which the BR still behaves like the "old" BR seems a bit weird. My suggestion is that we merge this PR with the current solution and we discuss in a new one how to improve this. As you guys have pointed out previously, in any case, the SVC resolution mechanism should be refactor, so I guess those changes can be part of the future PR.
There was a problem hiding this comment.
Reviewable status: 155 of 242 files reviewed, 13 unresolved discussions (waiting on @JordiSubira, @lukedirtwalker, @marcfrei, and @oncilla)
control/cmd/control/main.go line 586 at r9 (raw file):
Previously, JordiSubira wrote…
I took a look at #4376 and IIUC this PR would not work with that modification of the address binding for the resolver and the CS,DS service sockets. The
initSVCredirectsocket listens now on a dynamic port, which works fine, because it is registered into the "old" dispatcher, which then intercepts requests addresses to some SVC address and passes it through the socket that register for the concrete SVC. However, now the BR will send the messages with SVC addresses to the port specified in thetopology.json. Therefore, this has to be changed in any case for this PR to work, while doing it first in a separate PR, in which the BR still behaves like the "old" BR seems a bit weird. My suggestion is that we merge this PR with the current solution and we discuss in a new one how to improve this. As you guys have pointed out previously, in any case, the SVC resolution mechanism should be refactor, so I guess those changes can be part of the future PR.
One way to make this work is to unify the addresses; use the same (underlay) port for the SVC CS address, and QUIC (both UDP and UDP/SCION port). We only open a single underlay socket for both the QUIC server and the SVC redirector. As you've already created (or kept) the "ResolverPacketConn` as a wrapper around the normal connection, we can just plug this under the QUIC server. (To keep backwards compat, we can optionally open two separate sockets, if quic.Address != public address. Unsure if this is useful, possibly this is the right time to cut out the quic.address config option instead.)
This may look different, but on the underlay this is very close to what we had before; previously, this was demuxed in the dispatcher, now the application has to do it manually.
Generally, I think it makes sense to allow binding the hidden path service on a separate address, but it seems like this is not strictly necessary here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @lukedirtwalker, @marcfrei, @matzf, @oncilla, and @tzaeschke)
pkg/snet/reader.go line 102 at r32 (raw file):
"local host", c.local.Host, "pkt destination IA", pkt.Destination.IA, "pkt destination host", pktAddrPort,
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r33, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @marcfrei, @matzf, @oncilla, and @tzaeschke)
There was a problem hiding this comment.
Reviewed 3 of 183 files at r1, 4 of 34 files at r10, 5 of 20 files at r11, 1 of 20 files at r12, 11 of 38 files at r13, 8 of 19 files at r14, 7 of 7 files at r15, 5 of 10 files at r16, 11 of 17 files at r18, 4 of 19 files at r19, 8 of 27 files at r20, 10 of 17 files at r21, 2 of 4 files at r22, 3 of 4 files at r23, 16 of 22 files at r24, 1 of 3 files at r25, 20 of 25 files at r27, 2 of 6 files at r28, 1 of 1 files at r29, 10 of 14 files at r30, 7 of 7 files at r31, 3 of 3 files at r32, 1 of 1 files at r33, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @JordiSubira, @matzf, @oncilla, and @tzaeschke)
daemon/internal/servers/grpc.go line 356 at r31 (raw file):
} // AS serves the AS request.
Don't really understand this comment.
dispatcher/dispatcher.go line 95 at r33 (raw file):
server.parser = parser // if the dispatcher feature is enabled, we enable IP_PKTINFO; see // description of setIPPktInfo for more information.
Pretty redundant with the actual code. Could be removed, imo.
Code quote:
// if the dispatcher feature is enabled, we enable IP_PKTINFO; see
// description of setIPPktInfo for more information.dispatcher/dispatcher.go line 151 at r33 (raw file):
// a byte array corresponding to the packet that has to be forwarded. // The input byte array *buf* is the raw incoming packet; the *underlay* address MUST NOT // be nil and corresponds to the IP address in the encapsulation UDP/IP header; *prevHop*
underlay is a struct and can't be nil
Suggestion:
the *underlay* address MUST NOT be nildispatcher/dispatcher.go line 151 at r33 (raw file):
// a byte array corresponding to the packet that has to be forwarded. // The input byte array *buf* is the raw incoming packet; the *underlay* address MUST NOT // be nil and corresponds to the IP address in the encapsulation UDP/IP header; *prevHop*
It's not really "encapsulated", right? Remove the word?
Code quote:
encapsulationdispatcher/dispatcher.go line 157 at r33 (raw file):
// It returns a non-nil error for non-recoverable errors, only. // If the incoming packet couldn't be processed due to a recoverable error or due to // incorrect address validation the returned buffer and address will be nil.
"buffer will be nil and the address empty"?
Code quote:
buffer and address will be nildispatcher/dispatcher.go line 171 at r33 (raw file):
} if len(s.decoded) < 2 { log.Error("Unexpected decode packet", "layers decoded", len(s.decoded))
remove?
Code quote:
decodedispatcher/dispatcher.go line 455 at r33 (raw file):
// parseUnderlayAddr returns the underlay destination address on the UDP/IP wrapper. // It returns nil, if the control message information is not present or cannot be parsed.
"an empty address" or "an invalid address"? Or should we just stick with "nil" even if it's not strictly true?
Code quote:
nildispatcher/cmd/dispatcher/main.go line 137 at r33 (raw file):
func RunDispatcher( isDispatcher bool,
Looks a little funny, don't have a good suggestion though. Could we maybe at least just not export the function?
Code quote:
func RunDispatcher(
isDispatcher bool,pkg/snet/conn.go line 78 at r31 (raw file):
} if local.Host == nil || local.Host.IP.IsUnspecified() { return nil, serrors.New("nil or unspecified address is not for the raw connection")
is not supported
Code quote:
is notpkg/snet/snet.go line 91 at r31 (raw file):
var err error if addr == nil || addr.IP.IsUnspecified() { return nil, serrors.New("nil or unspecified address is not permitted")
supported
Code quote:
permittedprivate/topology/topology.go line 71 at r31 (raw file):
MTU int EndhostStartPort uint16 EndhostEndPort uint16
A detail and probably also a bit late, but since we already have underlay.EndhostPort and topology.EndhostPort plus json.Topology.EndhostPortRange, shouldn't it then be EndhostPortStart and EndhostPortEnd, here and everywhere else (daemon.proto, router.Connector, router.DataPlane, snet.scionConnWriter)?
Code quote:
EndhostStartPort uint16
EndhostEndPort uint16router/dataplane.go line 2060 at r33 (raw file):
// XXX(JordiSubira): This implementation is far too slow for the dataplane. // We should reimplement this with fewer helpers and memory allocations, since // our solely goal is to parse the L4 port or identifier in the offending packets.
sole
Code quote:
solelytools/topology/net.py line 179 at r31 (raw file):
class PortGenerator(object): # XXX(JordiSubira): We keep this in the default range. If the configured ranged,
range
Code quote:
ranged
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @matzf, @oncilla, and @tzaeschke)
daemon/internal/servers/grpc.go line 356 at r31 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Don't really understand this comment.
Done. It was a leftover vv'.
dispatcher/dispatcher.go line 151 at r33 (raw file):
Previously, marcfrei (Marc Frei) wrote…
underlayis a struct and can't be nil
Done. I changed the code but not the doc, thank you for pointing this out.
dispatcher/dispatcher.go line 157 at r33 (raw file):
Previously, marcfrei (Marc Frei) wrote…
"buffer will be nil and the address empty"?
Done.
private/topology/topology.go line 71 at r31 (raw file):
Previously, marcfrei (Marc Frei) wrote…
A detail and probably also a bit late, but since we already have
underlay.EndhostPortandtopology.EndhostPortplusjson.Topology.EndhostPortRange, shouldn't it then beEndhostPortStartandEndhostPortEnd, here and everywhere else (daemon.proto, router.Connector, router.DataPlane, snet.scionConnWriter)?
Done. I actually changed everything for "dispatchedPort" which is more consistent with the the terms used somewhere else.
There was a problem hiding this comment.
Reviewed 1 of 18 files at r34.
Reviewable status: 235 of 252 files reviewed, 9 unresolved discussions (waiting on @lukedirtwalker, @matzf, @oncilla, and @tzaeschke)
private/topology/topology.go line 71 at r31 (raw file):
Previously, JordiSubira wrote…
Done. I actually changed everything for "dispatchedPort" which is more consistent with the the terms used somewhere else.
Perfect, thanks!
There was a problem hiding this comment.
Reviewed 17 of 18 files at r34, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)
There was a problem hiding this comment.
Reviewed 9 of 17 files at r18, 4 of 19 files at r19, 7 of 27 files at r20, 10 of 17 files at r21, 2 of 4 files at r22, 1 of 4 files at r23, 15 of 22 files at r24, 18 of 25 files at r27, 1 of 6 files at r28, 9 of 14 files at r30, 3 of 7 files at r31, 1 of 3 files at r32, 1 of 1 files at r33, 1 of 18 files at r34.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)
JordiSubira
force-pushed
the
dispatcher_off
branch
from
870ffc2
to
568dfc0
Compare
There was a problem hiding this comment.
Reviewed 2 of 7 files at r31, 1 of 3 files at r32, 1 of 1 files at r33, 10 of 18 files at r34, 64 of 64 files at r35, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira, @lukedirtwalker, and @oncilla)
dispatcher/config/config.go line 83 at r28 (raw file):
Previously, JordiSubira wrote…
Yes, indeed it is some hassle renaming. What about chaging the flag meaning and have an "onlySCMPInfoResponder"?
I'd flip the sign on this, to say what it does; something like "local_udp_forwarding" or "local_udp_dispatch", "enable_local_packet_dispatch", ....
pkg/snet/packet_conn.go line 353 at r35 (raw file):
return nil, serrors.New("interface number not found", "interface", id) } return &net.UDPAddr{
Use net.UDPAddrFromAddrPort(addrPort); mostly the same thing but more concise and also copies the zone, if any.
There was a problem hiding this comment.
Reviewable status: 253 of 256 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @marcfrei, @matzf, @oncilla, and @tzaeschke)
pkg/snet/packet_conn.go line 353 at r35 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
net.UDPAddrFromAddrPort(addrPort); mostly the same thing but more concise and also copies the zone, if any.
Done.
There was a problem hiding this comment.
Reviewed 8 of 18 files at r34, 61 of 64 files at r35, 3 of 3 files at r36, 2 of 2 files at r37, 3 of 3 files at r38, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r39, 2 of 2 files at r40, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)
JordiSubira
force-pushed
the
dispatcher_off
branch
from
47bbfc4
to
8189bb5
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r36, 2 of 2 files at r37, 4 of 4 files at r39, 2 of 2 files at r40, 3 of 3 files at r41, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @oncilla)
matzf
changed the title
There was a problem hiding this comment.
Reviewed 7 of 18 files at r34, 57 of 64 files at r35, 3 of 3 files at r36, 2 of 2 files at r37, 4 of 4 files at r39, 2 of 2 files at r40, 3 of 3 files at r41, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira and @oncilla)
pkg/snet/snet.go line 153 at r41 (raw file):
return nil, err } log.FromCtx(ctx).Debug("UDP socket openned on", "addr", packetConn.LocalAddr(), "to", remote)
opened
Code quote:
openned
JordiSubira
force-pushed
the
dispatcher_off
branch
from
8189bb5
to
b20fb4d
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r41, 1 of 1 files at r42, all commit messages.
Reviewable status:
change last-mile router port forwarding removing dispatcher from snet and infra libraries intermediate commit remove dispatcher fix dataplane port parsing adapt end2end braccept UTs integration no probe add port range fix port range remove ref to dispatcher & reliable socket fix epic failing test remove dispatcher and reliable: - still integration test failing due to lack of support for SCMP handling and more comments, leftovers, small fixes more minor after rebasing pass add stateless dispatcher intermediate commit - Still things missing, e.g., update topology to include stateless dispatcher update topology with reduced dispatcher config add error handling and debug verbose to endHost resolution in BR add reduced forwarding dispatcher integration and utils integration tests lint + chown container fix docker check, only for linux dev lint modify HP and tests lint fix broken rebase lint pass change dispatcher configuration bugfix: QUIC address for client with :0 port slayers: unmap IPv4-mapped IPv6 addresses (scionproto#4377) The Go standard library can produce IPv4-mapped IPv6 addresses when resolving IP addresses. These IP addresses need to be unmapped before putting them on the wire. Before this patch, we could observe the following with tshark: Len=1304 SCION 1-ff00:0:110,[::ffff:172.20.2.2] -> 1-ff00:0:111,[::ffff:172.20.3.2] UDP 32769 -> 32768 1208 The regression was introduced in scionproto#4346, which removed the unmapping behavior in slayers.PackAddr. This patch restores the behavior to ensure only unmapped IPv4 addresses make it on the wire. Handling the unmapping in the code that generates the addresses and only checking this in slayers would seem ideal, but these calls are often very far away from the place that would then trigger an error. Thus handling this in slayers seems like a compromise that saves us and the users of the slayers package a lot of trouble. add packet reflection safeguard in shim add compatible IP_PKTINFO code for windows fix validateNextHopAddr fix isSCMPInfo add endhost port range configuration port range comment udpportRange add fixme allow unspecified address fon SCIONNetwork.Listen retriving nextHop from path and local Interface information add dispatching logic for SCMP at BR remove dispatcher shim support for Windows remove br dispatcher configuration from integration tests add test for old and new br configuration with(out) shim dispatcher comments and minor fixes remove utils_chown container remove docker utils from script pass pass pass refactor topology endhost_port_range comment for router fix dispatcherless docker and integration tests ignore SCMP errors messages on initSvcRedirect() adapt HP test adapt integration tests error string HP control/main dispatcher pass return addresses in helper function by value fix rebase upgrade dispatcher shim config to toml v2 add PortRange() RPC in daemon Revert "modify HP and tests" This reverts commit 1c82e9c. remove leftover CSResolver leftover in HP discovery open a single underlay socket for both the QUIC server and the SVC redirector revert acceptance/hiden_paths test await connectivity in old_br acceptance test pass pass pass pass pass + lint pass changes to snet API + refactor pass + allow for using snet outside the defined port range changes in isShimDispatcher() add destination safeguard to snet.scionConnReader.read() add TODOs lint change dispatched_ports name in topo add dispatched_ports all|ALL option range for services in topology PortGenerator dynamic ports refactoring add isDispatcher flag fix clientNet SCMPHandler add default value for shim underlay addr fix dispatcher port + cleaning isShimDispatcher add dstPort check reader remove leftover + TODO revert destination type in ResolverPacketConn replace UnderlayAddr comment comments + TODOs + refactoring add options pattern NewCookedConn improve error message pass fix rebase rename dispatcher flag mocks pass update sig_short_exp_time docker file fix dialer constructor fix docker image references for sig adapt end2end test to use Dial/Listen API remove debug logs add comment for snet.Dial typo
This PR intents to implement the dispatcher removal with the workaround discussed in #4280. The
dispatcherpkg has been completely refactor and pruned. It now holds a stateless shim that forwards UDP Datagrams to the right underlay port, based on the SCION_UDP.dst_port. Thereliable/sockpackages has been removed.Yet to be done (on this or other PRs):
This change is