snet: Simplify the api

[kormat]

There are a number of places where snet's API has multiple similar functions where it would be simpler to use and understand if there was one:

• SCIONNetwork constructors:
  • NewCustomNetwork
  • NewCustomNetworkWithPR
  • NewNetwork
  • NewNetworkWithPR
• SCIONNetwork Dial methods:
  • DialSCION
  • DialSCIONWithBindSVC
  • and the package-level functions
• SCIONNetwork Listen methods:
  • ListenSCION
  • ListenSCIONWithBindSVC
  • and the package-level functions

Proposal: it would be a lot simpler to use config structs for each of these cases.

For SCIONNetwork constructors:

type SCIONNetworkCfg struct {
    IA addr.IA

    // Only one of these may have a non-zero-value. If neither are set, snet
    // will not use sciond.
    SciondPath string
    PR pathmgr.Resolver

    // One (and only one) of these must be set.
    Dispatcher reliable.DispatcherService
    PktDispatcher PacketDispatcherService
}

For Dial*:

type DialSCIONCfg struct {
    Network string
    Laddr *Addr
    Baddr *Addr
    RAddr *Addr
    Svc addr.HostSVC
    Timeout time.Duration
}

And for Listen*:

type ListenSCIONCfg struct {
    Network string
    Laddr *Addr
    Baddr *Addr
    Svc addr.HostSVC
    Timeout time.Duration
}

[lukedirtwalker]

Instead of timeout being in the config struct it should be passed in as a parameter in form of a context.

[karampok]

@scrye I up-vote this issue. Touching the snet pkg was very confusing because of all the different APIs.
A cleanup, and well-defined interface would have been great.

[scrye]

I agree, I've discussed it with @sustrik as well. A lot of the APIs are no longer needed (bind addresses, path managers, multiple network constructors), and some of the semantics are unclear. The documentation is also lacking.

I feel like after a few rounds of removing stuff, we can have something that is as straightforward as the standard library net (or, at the very least, that's why we should aim for).

After cleaning up the addresses this is definitely something that we need to do, and once we do that, we should start moving down the parser stack and clean stuff down there too.

[scrye]

Regarding the suggestions in the first comment, we might be able to simplify even further.

Type by type breakdown below. Note that sometimes the following yet-to-be-implemented address types are used:

type UDPAddr struct { // snet.UDPAddr
  IA addr.IA
  UDP *net.UDPAddr
  Path Path
  Underlay net.Addr
}

type SVCAddr struct { // snet.SVCAddr
  IA addr.IA
  SVC addr.HostSVC
  Path Path
  Underlay net.Addr
}

type Network

Current API:

type Network interface {
    ListenSCIONWithBindSVC(network string,
        laddr, baddr *Addr, svc addr.HostSVC, timeout time.Duration) (Conn, error)
    DialSCIONWithBindSVC(network string,
        laddr, raddr, baddr *Addr, svc addr.HostSVC, timeout time.Duration) (Conn, error)
}

Simplified API:

type Network interface {
    Listen(ctx context.Context, laddr *snet.UDPAddr, svc addr.HostSVC) (*SCIONConn, error)
    Dial(ctx context.Context, laddr, raddr *snet.UDPAddr) (*SCIONConn, error)
}

type SCIONNetwork (implements Network)

Current API:

type SCIONNetwork struct {
    // contains filtered or unexported fields
}

func NewCustomNetworkWithPR(ia addr.IA, pktDispatcher PacketDispatcherService) *SCIONNetwork
func NewNetworkWithPR(ia addr.IA, dispatcher reliable.DispatcherService,
    querier PathQuerier, revHandler RevocationHandler) *SCIONNetwork

Suggested API:

type DefaultNetwork struct { // implements interface Network
    AddressFamily string // udp4, udp6, etc., thus no longer need during Dial/Listen
    Dispatcher PacketDispatcherService
    PathQuerier PathQuerier
    RevocationHandler RevocationHandler
}

// constructors deleted; objects built using fields directly

// delete methods that are not part of interface Network

type PacketDispatcherService

Current API:

type PacketDispatcherService interface {
    RegisterTimeout(ia addr.IA, public *addr.AppAddr, bind *net.UDPAddr,
        svc addr.HostSVC, timeout time.Duration) (PacketConn, uint16, error)
}

Suggested API:

type Dispatcher interface {
    Register(ctx context.Context, registration *snet.UDPAddr, svc addr.HostSVC) (PacketConn, *snet.UDPAddr, error)

type Conn

Current API:

type Conn interface {
    Read(b []byte) (int, error)
    ReadFrom(b []byte) (int, net.Addr, error)
    ReadFromSCION(b []byte) (int, *Addr, error)
    Write(b []byte) (int, error)
    WriteTo(b []byte, address net.Addr) (int, error)
    WriteToSCION(b []byte, address *Addr) (int, error)
    Close() error
    LocalAddr() net.Addr
    BindAddr() net.Addr
    SVC() addr.HostSVC
    // RemoteAddr returns the remote network address.
    RemoteAddr() net.Addr
    // SetDeadline sets read and write deadlines. Implements the net.Conn
    // SetDeadline method.
    SetDeadline(deadline time.Time) error
    SetReadDeadline(deadline time.Time) error
    SetWriteDeadline(deadline time.Time) error
}

Suggested API: Delete completely. net.PacketConn and net.Conn should be used instead.

type PacketConn

Renamed to DispatcherConn.

type SCIONConn

Renamed to Conn.

Remove methods BindSnetAddr, BindAddr.

type SCIONPacket

Renamed to Packet.

type SCIONPacketInfo

Renamed to PacketInfo

type SCIONPacketConn

Should be renamed to something that isn't as confusing (right now this clashes with net.PacketConn). No idea yet what the name should be.

[matzf]

This is great, even just cleaning up the names will be a very nice improvement! 🚀

@scrye Under "type Network", it might be even nicer if the Listen function could take svc as a pointer so that we can pass nil instead of SvcNone.

One alternative approach to this interface with a Network type, could be to do what net does: keep very simple package level Listen and Dial functions which just pick the default settings. For the dispatcher and sciond sockets paths, we could have a standard path and allow overriding over environment variables (similar to what net does with the name resolver settings).
Then, to allow specifically overriding advanced settings, we could have ListenConfig and Dialer types, analogous to net.
What do you think?

[lukedirtwalker]

@matzf Re taking svc as a pointer. I think it isn't a good idea. While it makes it easier for places where SvcNone is desired it will make it awkward for callers which want to pass and actual value, because in go you cannot take the address of a constant. So I think it makes more sense to keep the svc argument as a constant. Additionally IMO it's much more readable if I see a call Listen(..., addr.SvcNone, ..) vs Listen(...., nil, ...) I immediately see what the argument is about when using the constant, but when using nil I don't know what it means.

Regarding the Network interface: Since the IA is a property of the network the listen addresses can be *net.UDPAddrs.

[lukedirtwalker]

#3521 is a step towards the new Network interface

[matzf]

One thing I've observed while working with this (partially implemented) new API is that snet.Path includes the NextHop but the related spath.Path does not. The spath.Path and NextHop always come in pairs, e.g. in the UDPAddr type (both as described above, as "Underlay", and in the current implementation, as "NextHop" inherited from snet.Addr). This seems to complicate usage unnecessarily, as always both have to be passed/set, e.g.

scion/go/lib/infra/messenger/addr.go

Lines 138 to 139 in 8e6d4db

	ret.Path = p.Path()
	ret.NextHop = p.OverlayNextHop()

Proposal: Would it be feasible to integrate NextHop into spath.Path? Or, alternatively, have another wrapper type in snet that contains both spath.Path and NextHop?

[lukedirtwalker]

related #3675

[karampok]

As a part of this story, we need to have snet.Write/Read to return a meaningful error when the path is not set or the next hop is not set.

[scrye]

This is outdated by now. The current API is much simpler than what we had when this issue was created, but there is still room for cleaning it up further. We'll tackle those in smaller issues.