BS, DISP: Use net.UDPAddr instead of addr.AppAddr

[karampok]

Contributes #3344

This change is 

[lukedirtwalker]

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @karampok, @oncilla, and @scrye)

---

go/beacon_srv/main.go, line 295 at r1 (raw file):

	topo := t.topoProvider.Get()
	topoAddress := topo.PublicAddress(addr.SvcBS, cfg.General.ID)
	bs := &net.UDPAddr{

do this after the nil check below.

[lukedirtwalker]

Reviewed 15 of 16 files at r1.
Reviewable status: 15 of 16 files reviewed, 8 unresolved discussions (waiting on @karampok, @oncilla, and @scrye)

---

go/examples/discovery_client/client.go, line 49 at r1 (raw file):

		"(form \"host:port\" or \"[host]:port\"")
	// FIXME(roosd): Use AppAddr parsing once code base does no longer assume L4 is UDP.
	dsAddr addr.AppAddr

why not create a net.UDPAddr variable for this? getTopo is executed multiple times, so I think it would be nicer if the error checking happened once in the validateFlags and set the variable. And afterwards just uses the parsed data.

---

go/examples/discovery_client/client.go, line 119 at r1 (raw file):

		return serrors.New("Both topology and discovery service address specified")
	}
	if _, err := url.Parse(*ds); err != nil {

Does that deal with ports?

---

go/lib/discovery/discoveryinfo/info.go, line 60 at r1 (raw file):

	h.mu.Lock()
	defer h.mu.Unlock()
	if !h.addr.IP.Equal(a.IP) {

Doesn't the port need to be checked as well?

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

	defer h.mu.Unlock()
	if !h.addr.IP.Equal(a.IP) {
		h.addr = a

Previously there was a copy, don't we need that anymore?

---

go/lib/discovery/discoveryinfo/info.go, line 77 at r1 (raw file):

	h.mu.Lock()
	defer h.mu.Unlock()
	return h.addr

Maybe we should also copy?

---

go/lib/discovery/discoverypool/pool.go, line 58 at r1 (raw file):

	for k, v := range svcInfo {
		x := v.PublicAddr(v.Overlay)
		y := &net.UDPAddr{

x and y are not very speaking names.
Maybe use svcAddr for y and x will hopefully be removed once PublicAddr returns the correct type.

---

go/lib/discovery/discoverypool/pool.go, line 67 at r1 (raw file):

			info.Update(y)
		}

Not needed.

[lukedirtwalker]

Reviewed 1 of 16 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karampok, @oncilla, and @scrye)

---

go/lib/discovery/discovery.go, line 120 at r1 (raw file):

// FetchTopo fetches the topology with the specified parameters from the
// discovery service. If client is nil, the default http client is used.
func FetchTopo(ctx context.Context, params FetchParams, ds *addr.AppAddr,

please check with @oncilla whether we can remove this.

[scrye]

Reviewed 13 of 16 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)

---

go/examples/discovery_client/client.go, line 131 at r2 (raw file):

		defer cancelF()

		ip, sport, err := net.SplitHostPort(*ds)

net.ResolveUDPAddr might be useful do simplify this (I think it does what the code here wants for IP:port pairs.

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Previously there was a copy, don't we need that anymore?

Yes, we should keep copies in the initial refactors (i.e., try to have as few semantic changes as possible). I got bitten by a copy I missed and they are extremely hard to debug.

---

go/lib/discovery/discoveryinfo/info.go, line 77 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Maybe we should also copy?

I'd say yes, for the first version.

Our libraries in general don't do a good job of communicating address ownership, so it's hard to say when passing by reference is legitimate use. Until then, extra copies are the safer option.

[karampok]

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Previously there was a copy, don't we need that anymore?

To my understanding no, because inside the *net.UDPAddr we have no other pointer reference or interface. Therefore we are safe.

If not, we have to find a unit-test which proves otherwise.

[scrye]

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

To my understanding no, because inside the *net.UDPAddr we have no other pointer reference or interface. Therefore we are safe.

If not, we have to find a unit-test which proves otherwise.

There is a reference though, the net.IP slice. If a.IP changes after Update is called (e.g., a.IP[2] = 42), it will change h.addr.IP[2] to 42.

[karampok]

Reviewable status: 14 of 16 files reviewed, 9 unresolved discussions (waiting on @lukedirtwalker, @oncilla, and @scrye)

---

go/beacon_srv/main.go, line 295 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

do this after the nil check below.

Done.

---

go/examples/discovery_client/client.go, line 49 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

why not create a net.UDPAddr variable for this? getTopo is executed multiple times, so I think it would be nicer if the error checking happened once in the validateFlags and set the variable. And afterwards just uses the parsed data.

This is an example, and in an example I want to see snippet of code how to use something that I can input something I know (string) and a copy paste of that part to work.
Nevertheless, I am not sure how/why we have this example (and why here and not inside the doc of discovery) so I have not strong opinion.

---

go/examples/discovery_client/client.go, line 119 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Does that deal with ports?

This checks if the url is valid (if no port then it is valid) (if invalid port then fails)

---

go/examples/discovery_client/client.go, line 131 at r2 (raw file):

Previously, scrye (Sergiu Costea) wrote…

net.ResolveUDPAddr might be useful do simplify this (I think it does what the code here wants for IP:port pairs.

done, but I cannot really test this stuff here.
Do we use it somewhere, do you know if i can try it out somehow?

---

go/lib/discovery/discovery.go, line 120 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

please check with @oncilla whether we can remove this.

I have already done that

---

go/lib/discovery/discoveryinfo/info.go, line 60 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Doesn't the port need to be checked as well?

Done.

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

There is a reference though, the net.IP slice. If a.IP changes after Update is called (e.g., a.IP[2] = 42), it will change h.addr.IP[2] to 42.

I am not not super convinced but done.

---

go/lib/discovery/discoveryinfo/info.go, line 77 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

I'd say yes, for the first version.

Our libraries in general don't do a good job of communicating address ownership, so it's hard to say when passing by reference is legitimate use. Until then, extra copies are the safer option.

same as above, done

---

go/lib/discovery/discoverypool/pool.go, line 58 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

x and y are not very speaking names.
Maybe use svcAddr for y and x will hopefully be removed once PublicAddr returns the correct type.

correct, I am not sure I can change the PublicAddr to return the correct type as part of this PR

---

go/lib/discovery/discoverypool/pool.go, line 67 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Not needed.

Done.

[lukedirtwalker]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla and @scrye)

[lukedirtwalker]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla and @scrye)

[scrye]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

---

go/examples/discovery_client/client.go, line 131 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

done, but I cannot really test this stuff here.
Do we use it somewhere, do you know if i can try it out somehow?

(I just realized it's weird that this is an UDP address, since I think the discovery protocol works over TCP. But it's not something for this PR.)

I don't think we use it anywhere, so manual testing would be the only way (but I'm not that familiar with discovery).

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I am not not super convinced but done.

It's a bit hard to reason if copy(h.addr.IP, a.IP) does the copy correctly (one needs to think: is h.addr.IP allocated? what if it was, but a.IP is nil)?

To not have to deal with reasoning about ^, you can use the copy one-liner from the next comment.

---

go/lib/discovery/discoveryinfo/info.go, line 77 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

same as above, done

This call to copy will silently not behave as expected. After the allocation ret := &net.UDPAddr{}, ret.IP is a nil slice of type net.IP. Copy does not affect length or capacity, meaning copy(ret.IP, h.addr.IP) will copy 0 bytes and leave ret.IP as a nil (see https://play.golang.org/p/T-JGMpxdAHb).

The cleanest way to do a one-liner copy is:

ret.IP = append(h.addr.IP[:0:0], h.addr.IP...)

(see https://github.com/golang/go/wiki/SliceTricks).

[scrye]

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

[karampok]

Reviewable status: 14 of 17 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @oncilla, and @scrye)

---

go/lib/discovery/discoveryinfo/info.go, line 61 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It's a bit hard to reason if copy(h.addr.IP, a.IP) does the copy correctly (one needs to think: is h.addr.IP allocated? what if it was, but a.IP is nil)?

To not have to deal with reasoning about ^, you can use the copy one-liner from the next comment.

Done.

---

go/lib/discovery/discoveryinfo/info.go, line 77 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This call to copy will silently not behave as expected. After the allocation ret := &net.UDPAddr{}, ret.IP is a nil slice of type net.IP. Copy does not affect length or capacity, meaning copy(ret.IP, h.addr.IP) will copy 0 bytes and leave ret.IP as a nil (see https://play.golang.org/p/T-JGMpxdAHb).

The cleanest way to do a one-liner copy is:

ret.IP = append(h.addr.IP[:0:0], h.addr.IP...)

(see https://github.com/golang/go/wiki/SliceTricks).

okay, I am convinced. I have added unit-tests to make sure the above behavior.
(nevertheless it feels that if the reference is actually being modified by more parts in the code, that is is not optimal)

[scrye]

Reviewed 1 of 2 files at r3, 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[lukedirtwalker]

Reviewed 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved