refactor: replace addr.HostAddr hierarchy with tagged union addr.Host #4346

matzf · 2023-06-01T08:34:25Z

Replace the addr.HostAddr type hierarchy, consisting of the HostAddr interface and the HostNone, HostIPv4, HostIPv6 and HostSVC implementations with a single type addr.Host, representing all the different host address types as a tagged union ("sum type"). This uses, and follows the spirit of, the net/netip.Addr types to represent IP addresses for both IPv4 and IPv6.

The addr.Host type is a simple value type, which can be created and copied without heap allocations and can be used in comparisons and as map keys.
In addition to replacing all uses of addr.HostAddr, the new addr.Host type is used in the slayers.SCION.Get/SetSrc/DstAddr interfaces, as well as in the DRKey infrastructure. Using a consistent representation of the addresses allows to get rid of duplicate functionality and a number of address conversions, e.g. in snet.
In contrast to the addr.HostAddr interface, the new addr.Host does not implement the net.Addr interface. In particular, service addresses were passed through layers requiring a net.Addr. Use snet.SVCAddr in these cases.

Additionally, add a new addr.Addr type representing a full SCION address (ISD, AS and host address), including parsing functionality. This definition is identical to the snet.SCIONAddress type, which is now only kept as a type alias for compatibility.

Closes #4149.

Future work:

pkg/addr is a public API and should have a more SCION-specific and less clash-likely name, e.g. saddr
replace snet.SCIONAddress by addr.Addr
adopt using addr.Addr in the ping, traceroute etc tools, in place of the ill-fitting use of snet.UDPAddr.
add addr.MustParse{ISD,AS,IA} for completeness sake (currently in xtest package)
get rid of the IP-slice to netip.Addr conversions by gradually increasing the adoption of netip.Addr and related APIs

This change is

marcfrei

Reviewed 110 of 116 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)

pkg/addr/host.go line 95 at r2 (raw file):

// IP address.
// TODO(matzf): return ok or ...?
func HostIPFromSlice(ip net.IP) Host {

Just HostFromSlice instead of HostIPFromSlice?
Re "return ok or ...?": Browsing through the existing code, I see the convenience argument, nevertheless I lean towards passing the ok value to the caller instead of returning an invalid address.

pkg/addr/svc.go line 40 at r2 (raw file):

// SVC is a SCION service address.
// A service address is a short identifier for the service type, and a
// flag-bit to for multicast.

"flag-bit for multicast"?

Code quote:

flag-bit to for multicast

matzf

Reviewable status: 95 of 125 files reviewed, all discussions resolved (waiting on @marcfrei and @oncilla)

pkg/addr/host.go line 95 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Just HostFromSlice instead of HostIPFromSlice?

Re "return ok or ...?": Browsing through the existing code, I see the convenience argument, nevertheless I lean towards passing the ok value to the caller instead of returning an invalid address.

Done. I've added the handling of the ok result by raising errors where appropriate. This looks a bit clunky in some places now, but as pointed out in the PR description, the plan is to get rid of these type conversions by adopting the netip.Addr etc more broadly.
When the caller needs to handle the ok result, this HostIPFromSlice is really no longer any more convenient than using netip.AddrFromSlice directly, and so I've removed the function entirely.

marcfrei

Reviewed 5 of 17 files at r3, 25 of 25 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

lukedirtwalker

Reviewed 95 of 116 files at r1, 5 of 17 files at r3, 25 of 25 files at r4, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @matzf and @oncilla)

pkg/addr/addr.go line 63 at r4 (raw file):

	return fmt.Sprintf("%s,%s", a.IA, a.Host)
}

I think it would be nice if we'd also implement TextMarshaler and TextUnmarshaler

pkg/addr/addr.go line 85 at r4 (raw file):

// EXPERIMENTAL: This API is experimental. It may be changed to return a
// combined AddrPort type instead.
func ParseAddrPort(s string) (Addr, uint16, error) {

should we also have an JoinAddrPort?

pkg/addr/addr.go line 88 at r4 (raw file):

	host, port, err := net.SplitHostPort(s)
	if err != nil {
		return Addr{}, 0, serrors.WrapStr("invalid address: split host:port", err, "addr", s)

I wonder if it would be desirable to get rid of the serrors dependency on this package and if it is I would suggest to use fmt.Errorf here.

pkg/addr/host.go line 51 at r4 (raw file):

//
// The zero value is a valid object with Host{}.Type() == HostTypeNone.
type Host struct {

I think we should sort the members by size to get the better alignment (and smaller struct size): ip, svc, t, I wonder why SVC needs to be a uint16 and not just a uint8 (but it doesn't matter much anyway)

pkg/addr/svc.go line 43 at r4 (raw file):

// The package's SVC constant values are defined without multicast.
// The Multicast and Base methods set/unset the multicast flag.
type SVC uint16

If we anyway rework, can we get rid of the SVC multicast ? :P

pkg/addr/svc.go line 85 at r4 (raw file):

}

// XXX(matzf): change this to the format accepted by ParseSVC?

hm I think it's not nice that we don't have a matching pair. I would have expected that whatever I print with String() I can also parse.

tools/braccept/cases/bfd.go line 106 at r4 (raw file):

		SrcIA:        remoteIA,
	}
	err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("192.168.13.3")))

why not use addr.MustParseHost() here and below?

tools/braccept/cases/child_to_child_xover.go line 112 at r4 (raw file):

	}

	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.5.1"))); err != nil {

MustParseHost ?

tools/braccept/cases/child_to_internal.go line 101 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {

MustParseHost? here and below?

tools/braccept/cases/child_to_parent.go line 109 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {

ditto

tools/braccept/cases/internal_to_child.go line 104 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.MustParseHost("192.168.0.51")); err != nil {

ditto

tools/braccept/cases/jumbo.go line 108 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.3.1")); err != nil {

ditto

tools/braccept/cases/onehop.go line 82 at r4 (raw file):

		Path:         ohp,
	}
	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.4.1")); err != nil {

ditto

tools/braccept/cases/parent_to_child.go line 108 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.3.1")); err != nil {

ditto

tools/braccept/cases/parent_to_internal.go line 95 at r4 (raw file):

		Path:         sp,
	}
	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.3.1")); err != nil {

ditto

matzf

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

pkg/addr/addr.go line 63 at r4 (raw file):