router: make room for multiple underlay impls phase 1 #4658

jiceatscion · 2024-11-29T18:58:39Z

The goal is to eventually remove all underlay-specific code from the main router code and place it in a plugin, such that there can then be multiple underlay plugins. This PR isn't attempting to fully accomplish that. It avoids moving some code, so that the diff is easier to read. The price to pay is a few extra spaghetti left between main code and plugin.

The code that should eventually move to the underlay is:

runReceiver and runForwarder.
likely some of the BFD code.
opening of connections (currently done by connector.go)

Other changes being planned:

Stop reusing the internal connection for sibling links (so we can take advantage of bound connections).
Add knowledge of multiple underlay into the configurator.
Make underlay addresses opaque to the router.
Demultiplex to links on ingest, so ifID and srcAddress (when they are defined by the link) are obtained in the most efficient way (directly from the link's fields for example).

Contributes to: #4593

jiceatscion · 2024-11-29T18:58:46Z

This change is

The underlay code is now the udpip underlay provider. The refactoring is not complete. The assumption that underlay addresses are udp/ip addresses is still part of the router. Also, the main receiver and sender tasks are still where they are and use the underlay connections via a temporary API. The reason for that is to make the diff somewhat more reviewable by not moving large swaths of code from one file to another.

tzaeschke

Iḿ not really familiar with this module and I'm probably missing a lot of context, so my review is almost exclusively about comments and naming.

Reviewed 1 of 11 files at r2.
Reviewable status: 1 of 11 files reviewed, 22 unresolved discussions (waiting on @jiceatscion)

-- commits line 16 at r3:
Since it is incomplete, maybe add some points about what exactly still needs to be done?

router/underlay.go line 33 at r3 (raw file):

// Link embodies the router's idea of a point to point connection. A link associates the underlay
// connection, with a bfdSession, a destination address, etc. It also allows the concrete send

connection with

Code quote:

connection, with

router/underlay.go line 48 at r3 (raw file):

	IsUp() bool
	GetIfID() uint16
	GetRemote() netip.AddrPort // incremental refactoring: using code will move to underlay.

TODO?

router/underlay.go line 59 at r3 (raw file):

//
// Incremental refactoring: addresses are still explicitly IP/port. In the next step, we have to
// make them opaque; to be interpreted only by the underlay implementation.

TODO?

router/underlayproviders/udpip.go line 27 at r3 (raw file):

// This is currently the only implementation. The goal of splitting out this code from the router
// is to enable other implementations. However, as a first step, we continue assuming that the
// batchConn is given to us and is a Udp socket and that, in the case of externalLink, it is bound.

UDP

Code quote:

Udp

router/underlayproviders/udpip.go line 49 at r3 (raw file):

func (u *provider) GetConnections() map[netip.AddrPort]router.UnderlayConnection {
	// a map of interfaces and a map of concrete implementations aren't compatible.

A

Code quote:

router/underlayproviders/udpip.go line 50 at r3 (raw file):

func (u *provider) GetConnections() map[netip.AddrPort]router.UnderlayConnection {
	// a map of interfaces and a map of concrete implementations aren't compatible.
	// For the same reason, we cannot have the map of concrete as our return type; it

concrete implementations as

Code quote:

concrete as

router/underlayproviders/udpip.go line 55 at r3 (raw file):

	// Since we do not want to store our own things as interfaces, we have to translate.
	// Good thing it doesn't happen often.
	m := make(map[netip.AddrPort]router.UnderlayConnection)

How often is map() called? Is it worth caching it?

router/underlayproviders/udpip.go line 90 at r3 (raw file):

}

// Incremental refactoring: The following implements UnderlayConnection so some of the code

TODO?

router/underlayproviders/udpip.go line 202 at r3 (raw file):

// to erase the separation between link and connection for this implementation. Side effect
// of moving the address:link here: the router does not know if there is an existing link. As
// a result it has to give us a bfdSession in all cases and if we might throuw it away (there

throw

Code quote:

throuw

router/underlayproviders/udpip.go line 204 at r3 (raw file):

// a result it has to give us a bfdSession in all cases and if we might throuw it away (there
// are no permanent resources attached to it). This will be fixed by moving some bfd related code
// in-here; but later.

TODO? Maybe a ticket?

router/underlayproviders/udpip.go line 219 at r3 (raw file):

	c, exists := u.allConnections[netip.AddrPort{}]
	if !exists {
		// That doesn't actually happen and we'll get rid of this soon.

TODO?

router/dataplane.go line 232 at r3 (raw file):

	unsupportedUnspecifiedAddress = errors.New("unsupported unspecified address")
	noBFDSessionFound             = errors.New("no BFD session was found")
	// noBFDSessionConfigured        = errors.New("no BFD sessions have been configured")

Maybe remove this?

router/dataplane.go line 356 at r3 (raw file):

	}

	l := d.underlay.NewInternalLink(conn, d.RunConfig.BatchSize)

rename to "link" or something longer than "l" (which I first read as "1")?

router/dataplane.go line 392 at r3 (raw file):

	}
	lk := d.underlay.NewExternalLink(conn, d.RunConfig.BatchSize, bfd, dst.Addr, ifID)
	d.interfaces[ifID] = lk

lk -> link?

router/dataplane.go line 648 at r3 (raw file):

	}

	for a, l := range d.underlay.GetLinks() {

Use slightly more descriptive variable names?

router/dataplane.go line 741 at r3 (raw file):

		pkt.rawPacket = pkt.rawPacket[:size] // Update size; readBatch does not.
		// Incremental refactoring: We should begin with finding the link and get the ifID

Is it common in scionproto to insert a TODO here?

router/dataplane.go line 1023 at r3 (raw file):

// Big issue with metrics and ifID. If an underlay connection must be shared between links
// (for example, libling links), then we don't have a specific ifID in the connection per se. It

sibling

Code quote:

libling

router/dataplane.go line 1035 at r3 (raw file):

//     metrics... might not be much cheaper than the naive way.
//   - Use one fw queue per ifID in each connection... but then have to round-robin for fairness....
//     smaller batches?

That's also a TODO, isn't it?

router/dataplane.go line 1203 at r3 (raw file):

	// If this is an inter-AS BFD, it can via an interface we own. So the ifID matches one link
	// and the ifID better be valid. In the future that will be checked upstream from here.
	l, exists := p.d.interfaces[p.pkt.ingress]

l is an interface? Maybe rename to intf (or link) or something?

router/dataplane.go line 1528 at r3 (raw file):

// SrcIA checks by disguising packets as transit traffic.
//
// Incremental refactoring: All or part of this check should move to the underlay.

TODO?

router/dataplane.go line 1548 at r3 (raw file):

}

// Validates the egress interface referenced by the current hop. This  is not called for

This is not

Code quote:

This  is not

jiceatscion

Reviewable status: 1 of 11 files reviewed, 22 unresolved discussions (waiting on @tzaeschke)

-- commits line 16 at r3: