Skip to content
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

end host, router: dispatch UDP in router, remove dispatcher socket #4344

Merged
merged 1 commit into from
May 17, 2024

Conversation

JordiSubira
Copy link
Contributor

@JordiSubira JordiSubira commented May 17, 2023

This PR intents to implement the dispatcher removal with the workaround discussed in #4280. The dispatcher pkg 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. The reliable/sock packages has been removed.

Yet to be done (on this or other PRs):


This change is Reviewable

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/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.

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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

SCIONPacketConnMetrics

private/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_dicts

tools/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.

@JordiSubira JordiSubira marked this pull request as ready for review September 28, 2023 15:07
@JordiSubira JordiSubira requested review from a team and oncilla as code owners September 28, 2023 15:07
Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JordiSubira JordiSubira marked this pull request as draft November 9, 2023 14:59
@JordiSubira JordiSubira marked this pull request as ready for review January 4, 2024 08:40
Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

previousHop

dispatcher/dispatcher.go line 402 at r9 (raw file):

}

// validateNextHopAddr returnS true if the underlay address on the UDP/IP wrapper

returns

Code quote:

returnS

dispatcher/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 Register

pkg/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:

next

pkg/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

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

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 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().

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 FlagInterface requests the IfIndex to 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 Controller quite 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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.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.

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 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.

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 ResolveLocal function back to addrutil.

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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SCIONPacketConn and make getLastHopAddr a normal helper function instead of capturing ifAddrs implicitly 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.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

underlay is 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.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)?

Done. I actually changed everything for "dispatchedPort" which is more consistent with the the terms used somewhere else.

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@tzaeschke tzaeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 17 of 18 files at r34, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)

Copy link
Contributor

@tzaeschke tzaeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

@matzf matzf changed the title Remove dispatcher end host, router: dispatch UDP in router, remove dispatcher socket May 16, 2024
Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r41, 1 of 1 files at r42, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants