BS: Add Keepalive sender #2573

oncilla · 2019-04-03T12:02:10Z

The keepalive sender periodically sends keepalive messages
on all interfaces.

Additionally, add one-hop creation function to spath.

fixes #2567

This change is

The keepalive sender periodically sends keepalive messages on all interfaces. fixes scionproto#2567

scrye

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

go/beacon_srv/internal/keepalive/sender_test.go, line 42 at r1 (raw file):

	Convey("Run sends ifid packets on all interfaces", t, func() {
		wconn, rconn := p2p.NewPacketConns()
		// Create sender.

This comment can be deleted.

go/beacon_srv/internal/onehop/sender.go, line 37 at r1 (raw file):

type Sender struct {
	// SrcIA is the ISD-AS of the local AS.
	SrcIA addr.IA

You can rename this as IA, it is clear from the context that it is the IA of the Sender. And the comment eliminates any possible confusion.

go/beacon_srv/internal/onehop/sender.go, line 41 at r1 (raw file):

	Addr *addr.AppAddr
	// Conn is the connection to sent the packets.
	Conn *snet.SCIONPacketConn

s/sent/send, and maybe rephrase it a bit as "Conn is used to send the packets". It is clear that it is a connection object.

go/beacon_srv/internal/onehop/sender.go, line 43 at r1 (raw file):

	Conn *snet.SCIONPacketConn
	// HFMacPool is the mac pool to issue hop fields.
	HFMacPool *sync.Pool

As discussed offline, I'm not sure if an elastic pool is the best approach to maintain cmac algorithm instantiations.

go/beacon_srv/internal/onehop/sender.go, line 48 at r1 (raw file):

// Send sends the payload on a one-hop path.
func (s *Sender) Send(dst snet.SCIONAddress, ifid common.IFIDType, nextHop *overlay.OverlayAddr,
	pld common.Payload, infoTime time.Time) error {

I would wrap ifid, pld, infoTime in a container type. They belong together, and it makes the signature of the function easier to read.

go/beacon_srv/internal/onehop/sender.go, line 59 at r1 (raw file):

// Pkt creates a scion packet with a one-hop path and the payload.
func (s *Sender) Pkt(dst snet.SCIONAddress, ifid common.IFIDType, pld common.Payload,
	now time.Time) (*snet.SCIONPacket, error) {

Rename now, since it is not strictly needed to be the current time. Some tests might use explicit times, etc.

go/beacon_srv/internal/onehop/sender.go, line 82 at r1 (raw file):

// CreatePkt creates a scion packet with a one-hop extension, and the
// provided path and payload.
func (s *Sender) CreatePkt(dst snet.SCIONAddress, path *Path,

This function does a pretty straightforward thing. You can move it to Pkt (and rename that one to CreatePkt), and just have that function look like:

path, err := s.CreatePath(...)
// err check
return &snet.SCIONPacket{
    // blah blah blah
}, nil

go/lib/spath/path_test.go, line 145 at r1 (raw file):

	mac, err := scrypto.InitMac(make(common.RawBytes, 16))
	xtest.FailOnErr(t, err)
	rawHop, err := hex.DecodeString("00000003000400000b00000000000000")

Add a comment explaining what the hexstring contains.

Also, I sometimes found it useful to use []byte, because it allows for cleaner alignment (like 8-byte lines) which makes stuff easier to parse for humans.

go/lib/spath/path_test.go, line 189 at r1 (raw file):

			SoMsg("hop", hop, ShouldResemble, &HopField{Mac: common.RawBytes{0, 0, 0}})
		})

remove extra newline

oncilla

Reviewable status: 5 of 10 files reviewed, 9 unresolved discussions (waiting on @scrye)

go/beacon_srv/internal/keepalive/sender_test.go, line 42 at r1 (raw file):