Refactor overlay addresses to always use net.IP

[scrye]

We currently do not support any other overlay L3. Also, the overlay
connection objects always expect IP addresses. This makes the
net.IP address requirement explicit in the overlay address constructor.
Also, the constructor can no longer error out.

---

This change is 

[sgmonroy]

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @scrye)

---

go/border/rpkt/route.go, line 118 at r1 (raw file):

	if onLastSeg && rp.dstIA.Equal(rp.Ctx.Conf.IA) {
		// Destination is a host in the local ISD-AS.
		dst := overlay.NewOverlayAddr(rp.dstHost.IP(), overlay.EndhostPort)

can you double check that the dstHost at his point is always an IP and not say SVC?

---

go/godispatcher/network/dispatcher.go, line 121 at r1 (raw file):

		return nil, common.NewBasicError("unable to construct UDP addr", err)
	}
	if network == "udp4" && listeningAddress.IP == nil {

should we check here that the IP is IPv4?

---

go/godispatcher/network/dispatcher.go, line 124 at r1 (raw file):

		listeningAddress.IP = net.IPv4zero
	}
	if network == "udp6" && listeningAddress.IP == nil {

should we check here that the IP is IPv6?

---

go/integration/end2end/main.go, line 189 at r1 (raw file):

	c.conn.SetWriteDeadline(getDeadline(ctx))
	if remote.NextHop == nil {
		remote.NextHop = overlay.NewOverlayAddr(remote.Host.L3.IP(), overlay.EndhostPort)

Same as previous comment, do we check somewhere that the Host address is an HostIP type?
might be a good place to put some asserts.

---

go/lib/hostinfo/hostinfo.go, line 74 at r1 (raw file):

func (h *Host) Overlay() (*overlay.OverlayAddr, error) {
	return overlay.NewOverlayAddr(h.Host().IP(), h.Port), nil

This reports no error if Host is HostSVC

---

go/lib/overlay/addr.go, line 25 at r1 (raw file):

type OverlayAddr struct {
	l3 net.IP

😍

---

go/lib/topology/addr.go, line 195 at r1 (raw file):

	}
	port := uint16(rpbo.Public.OverlayPort)
	if port == 0 && udpOverlay {

old code would complain if a port was configured but was not udpOverlay

---

go/lib/topology/addr_test.go, line 124 at r1 (raw file):

		{"Invaild Public IP Address", false, pubBad, nil, nil, ErrInvalidPub},
		{"Invaild Bind IP Address", false, pubIPv4, nil, bindBad, ErrInvalidBind},
		{"No UDP Overlay", false, pubUDPIPv4, nil, bindBad, ErrOverlayPort},

related to previous comment.

---

go/lib/topology/topology_test.go, line 40 at r1 (raw file):

	}
	if op == 0 {
		return overlay.NewOverlayAddr(l3.IP(), 0)

I think you can remove this branch code.

[scrye]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @sgmonroy)

---

go/border/rpkt/route.go, line 118 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

can you double check that the dstHost at his point is always an IP and not say SVC?

Done.

---

go/godispatcher/network/dispatcher.go, line 121 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

should we check here that the IP is IPv4?

There is no IP to check, it is a nil net.IP slice (or I'm misunderstanding what you mean).

---

go/godispatcher/network/dispatcher.go, line 124 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

should we check here that the IP is IPv6?

There is no IP to check, it is a nil net.IP slice (or I'm misunderstanding what you mean).

---

go/integration/end2end/main.go, line 189 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Same as previous comment, do we check somewhere that the Host address is an HostIP type?
might be a good place to put some asserts.

In an application I'd agree, but here it's an integration test. I'd rather this breaks, we might find a bug this way.

---

go/lib/hostinfo/hostinfo.go, line 74 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

This reports no error if Host is HostSVC

Very good point. Fixed.

Since the type is an interface, checking for nil IP() is safer I feel. If it's a valid IPv4 or IPv6 everything is fine; otherwise it's an unsupported address which will return nil and cause an error.

---

go/lib/overlay/addr.go, line 25 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

😍

Done.

---

go/lib/topology/addr.go, line 195 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

old code would complain if a port was configured but was not udpOverlay

Is that a valid error though? Or rather, is it safe to leave the behavior as undefined?

I'm struggling with what "port configured but not udpOverlay" would even mean. I'm thinking discarding the port at that point is valid, which is what this code would do if NewOverlayAddr actually constructed valid non-UDP/IP overlay addresses.

I'm now also curious, how was overlay.NewOverlayAddr supposed to build a pure IP overlay address? A port of 0 is a perfectly valid port if an application wants the OS to choose a free port, so it can't be signaled "in-band". I'm guessing a NullL4 layer 4 type would have been needed, but that's a bit awkward (or a new constructor, but that kind of defeats the purpose of having this interface one).

---

go/lib/topology/addr_test.go, line 124 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

related to previous comment.

Yes, I removed the test because it seemed overly strict for a reason that was not at all clear to me. I'm not convinced it should be added back.

---

go/lib/topology/topology_test.go, line 40 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

I think you can remove this branch code.

Done.

[scrye]

a discussion (no related file):
Full acceptance run: https://buildkite.com/scionproto/acceptance/builds/615.

---

[sgmonroy]

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kormat and @scrye)

---

go/godispatcher/network/dispatcher.go, line 121 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

There is no IP to check, it is a nil net.IP slice (or I'm misunderstanding what you mean).

not sure how I read this before, IP is nil as you point out.

---

go/godispatcher/network/dispatcher.go, line 124 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

There is no IP to check, it is a nil net.IP slice (or I'm misunderstanding what you mean).

not sure how I read this before, IP is nil as you point out.

---

go/lib/topology/addr.go, line 195 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Is that a valid error though? Or rather, is it safe to leave the behavior as undefined?

I'm struggling with what "port configured but not udpOverlay" would even mean. I'm thinking discarding the port at that point is valid, which is what this code would do if NewOverlayAddr actually constructed valid non-UDP/IP overlay addresses.

I'm now also curious, how was overlay.NewOverlayAddr supposed to build a pure IP overlay address? A port of 0 is a perfectly valid port if an application wants the OS to choose a free port, so it can't be signaled "in-band". I'm guessing a NullL4 layer 4 type would have been needed, but that's a bit awkward (or a new constructor, but that kind of defeats the purpose of having this interface one).

did you forget that originally l4 was an interface so it could be nil?

---

go/lib/topology/addr_test.go, line 124 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Yes, I removed the test because it seemed overly strict for a reason that was not at all clear to me. I'm not convinced it should be added back.

AFAICR @kormat wanted this check to detect invalid configurations.

[scrye]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kormat, @scrye, and @sgmonroy)

---

go/lib/topology/addr.go, line 195 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

did you forget that originally l4 was an interface so it could be nil?

Ah, right. I already forgot since it was always a port in there.

---

go/lib/topology/addr_test.go, line 124 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

AFAICR @kormat wanted this check to detect invalid configurations.

Is it even invalid? If I see something like "This is an IP only address, and here is its port" I'm thinking it's safe to discard the port. I don't think being stricter than absolutely needed yields any benefits.

[sgmonroy]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kormat and @scrye)

---

go/lib/topology/addr_test.go, line 124 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Is it even invalid? If I see something like "This is an IP only address, and here is its port" I'm thinking it's safe to discard the port. I don't think being stricter than absolutely needed yields any benefits.

Just saying that AFAICR that was the reason for the error.
I'm ok with discard/ignore, maybe a warning.

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved