pkg/snet: change snet.Topology to a struct

[lukedirtwalker]

Instead of having the snet.Topology as an interface, change it to a struct.
This makes the contract clearer, LocalIA and PortRange are only considered when
creating a connection. The Interface information however can dynamically change.

The daemon API now exposes an adapter to load the topology information once
or to load the information periodically.

[jiceatscion]

This change is 

[lukedirtwalker]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @JordiSubira)

---

a discussion (no related file):
This is more of a proposal of how we could deal with changing topologies for processes that have long standing connections. We could probably introduce some caching layer on top of snet.Topology. What do you think @JordiSubira ?

[JordiSubira]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

---

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

This is more of a proposal of how we could deal with changing topologies for processes that have long standing connections. We could probably introduce some caching layer on top of snet.Topology. What do you think @JordiSubira ?

Indeed, back on the dispatcherless PR we discussed about adding some wrapper around the snet.Daemon API for the caching topology related requests.

We decided to put on snet.SCIONPacketConn the interfaceMap as a "once"-cache when opening a connection, which is not ideal because as you said the interface<->ID mapping can dynamically change. We sacrificed this to avoid having to call the daemon API on each packet (because more of the applications use the daemon to implement n.Topology right now).

Do you think we can add the caching layer into this PR? We can have for the moment a simple one, even one shot and allow the application either to instantiate the wrapper or directly the snet.Daemon one.

[lukedirtwalker]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @JordiSubira)

---

a discussion (no related file):

Previously, JordiSubira wrote…

Indeed, back on the dispatcherless PR we discussed about adding some wrapper around the snet.Daemon API for the caching topology related requests.

We decided to put on snet.SCIONPacketConn the interfaceMap as a "once"-cache when opening a connection, which is not ideal because as you said the interface<->ID mapping can dynamically change. We sacrificed this to avoid having to call the daemon API on each packet (because more of the applications use the daemon to implement n.Topology right now).

Do you think we can add the caching layer into this PR? We can have for the moment a simple one, even one shot and allow the application either to instantiate the wrapper or directly the snet.Daemon one.

Sure, I can add a caching layer here.

[oncilla]

Reviewed 10 of 13 files at r3.
Reviewable status: 10 of 13 files reviewed, 3 unresolved discussions (waiting on @JordiSubira and @lukedirtwalker)

---

pkg/daemon/topology.go line 94 at r3 (raw file):

}

func (t *ReloadingTopology) Run(ctx context.Context, period time.Duration) {

We never clean up the entries that are no longer present.

Is that an issue, or do we assume interfaces are not removed?
If we assume that, I think we should add a corresponding comment

---

control/cmd/control/main.go line 957 at r3 (raw file):

		},
		Interface: func(ifID uint16) (netip.AddrPort, bool) {
			// XXX(lukedirtwalker): The amount of conversiones between

nit: Given we know when the data changes, I think it would be easy to convert it only once and store it.

[lukedirtwalker]

Reviewable status: 9 of 14 files reviewed, 3 unresolved discussions (waiting on @JordiSubira and @oncilla)

---

control/cmd/control/main.go line 957 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: Given we know when the data changes, I think it would be easy to convert it only once and store it.

yeah my issue is more that the topology loads as netip.AddrPort but then the API UnderlayNextHop exposes it as *net.UDPAddr, the snet API actually converts the netip.AddrPort back to *net.UDPAddr internally

---

pkg/daemon/topology.go line 94 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

We never clean up the entries that are no longer present.

Is that an issue, or do we assume interfaces are not removed?
If we assume that, I think we should add a corresponding comment

Good catch

The only issue I could think of is memory leak. For now I added a comment, but for good measure I'll fix the actual implementation tomorrow.

[oncilla]

Reviewed 3 of 13 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)

[oncilla]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira)

[JordiSubira]

Reviewed 12 of 13 files at r3, 3 of 3 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

[JordiSubira]

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

[lukedirtwalker]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)