Peering links continued

acceptance/README.md

@@ -3,6 +3,9 @@
 This directory contains a set of integration tests.
 Each test is defined as a bazel test target, with tags `integration` and `exclusive`.
 
+Some integration tests use code outside this directory. For example, the
+`router_multi` acceptance test cases and main executable are in `tools/braccept`.
+
 ## Basic Commands
 
 To run all integration tests which include the acceptance tests, execute one of
@@ -18,6 +21,7 @@ Run a subset of the tests by specifying a different list of targets:
 
 ```bash
 bazel test --config=integration //acceptance/cert_renewal:all //acceptance/trc_update/...
+bazel test --config=integration //acceptance/router_multi:all --cache_test_results=no
 ```
 
 The following the flags to bazel test can be helpful when running individual tests:
@@ -27,8 +31,8 @@ The following the flags to bazel test can be helpful when running individual tes
 
 ## Manual Testing
 
-Some of the tests are defined using a common framework, defined in the
-bazel rules `topogen_test` and `raw_test`.
+Some of the tests are defined using a common framework, implemented by the
+bazel rules `topogen_test` and `raw_test` (in [raw.bzl](acceptance/common/raw.bzl)).
 These test cases allow more fine grained interaction.
 
 ```bash
@@ -41,5 +45,13 @@ bazel run //<test-package>:<target>_run
 bazel run //<test-package>:<target>_teardown
 ```
 
+For example:
+
+```bash
+bazel run //acceptance/router_multi:test_bfd_setup
+bazel run //acceptance/router_multi:test_bfd_run
+bazel run //acceptance/router_multi:test_bfd_teardown
+```
+
 See [common/README](common/README.md) for more information about the internal
 structure of these tests.

control/beaconing/extender.go

@@ -62,7 +62,7 @@ type DefaultExtender struct {
 	EPIC bool
 }
 
-// Extend extends the beacon with hop fields of the old format.
+// Extend extends the beacon with hop fields.
 func (s *DefaultExtender) Extend(
 	ctx context.Context,
 	pseg *seg.PathSegment,
@@ -85,11 +85,25 @@ func (s *DefaultExtender) Extend(
 	}
 	ts := pseg.Info.Timestamp
 
-	hopEntry, epicHopMac, err := s.createHopEntry(ingress, egress, ts, extractBeta(pseg))
+	hopBeta := extractBeta(pseg)
+	hopEntry, epicHopMac, err := s.createHopEntry(ingress, egress, ts, hopBeta)
 	if err != nil {
 		return serrors.WrapStr("creating hop entry", err)
 	}
-	peerBeta := extractBeta(pseg) ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2])
+
+	// The peer hop fields chain to the main hop field, just like any child hop field.
+	// The effect of this is that when a peer hop field is used in a path, both the
+	// peer hop field and its child are validated using the same SegID accumlator value:
+	// that originally intended for the child.
+	//
+	// The corrolary is that one cannot validate a hop field's MAC by looking at the
+	// parent hop field MAC when the parent is a peering hop field. This is ok: that
+	// is never done that way, it is always done by validating against the SegID
+	// accumulator supplied by the previous router on the forwarding path. The
+	// forwarding code takes care of not updating that accumulator when a peering hop
+	// is traversed.
+
+	peerBeta := hopBeta ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2])
 	peerEntries, epicPeerMacs, err := s.createPeerEntries(egress, peers, ts, peerBeta)
 	if err != nil {
 		return err
@@ -268,6 +282,10 @@ func (s *DefaultExtender) createHopF(ingress, egress uint16, ts time.Time,
 	}, fullMAC[path.MacLen:]
 }
 
+// extractBeta computes the beta value that must be used for the next hop to be added to
+// be added at the end of the segment.
+// FIXME(jice): keeping an accumulator would be just as easy to do as it is during
+// forwarding. What's the benefit of re-calculating the whole chain every time?
 func extractBeta(pseg *seg.PathSegment) uint16 {
 	beta := pseg.Info.SegmentID
 	for _, entry := range pseg.ASEntries {

pkg/slayers/path/scion/base.go

@@ -76,15 +76,21 @@ func (s *Base) IncPath() error {
 	}
 	if int(s.PathMeta.CurrHF) >= s.NumHops-1 {
 		s.PathMeta.CurrHF = uint8(s.NumHops - 1)
-		return serrors.New("path already at end")
+		return serrors.New("path already at end",
+			"curr_hf", s.PathMeta.CurrHF,
+			"num_hops", s.NumHops)
 	}
 	s.PathMeta.CurrHF++
 	// Update CurrINF
 	s.PathMeta.CurrINF = s.infIndexForHF(s.PathMeta.CurrHF)
 	return nil
 }
 
-// IsXover returns whether we are at a crossover point.
+// IsXover returns whether we are at a crossover point. This includes
+// all segment switches, even over a peering link. Note that handling
+// of a regular segment switch and handling of a segment switch over a
+// peering link are fundamentally different. To distinguish the two,
+// you will need to extract the information from the info field.
 func (s *Base) IsXover() bool {
 	return s.PathMeta.CurrHF+1 < uint8(s.NumHops) &&
 		s.PathMeta.CurrINF != s.infIndexForHF(s.PathMeta.CurrHF+1)

private/path/combinator/graph.go

@@ -239,7 +239,11 @@ func (g *dmg) GetPaths(src, dst vertex) pathSolutionList {
 }
 
 // inputSegment is a local representation of a path segment that includes the
-// segment's type.
+// segment's type. The type (up down or core) indicates the role that this
+// segment holds in a path solution. That is, in which order the hops would
+// be used for building an actual forwarding path (e.g. from the end in the
+// case of an UP segment). However, the hops within the referred PathSegment
+// *always* remain in construction order.
 type inputSegment struct {
 	*seg.PathSegment
 	Type proto.PathSegType
@@ -316,7 +320,11 @@ func (solution *pathSolution) Path() Path {
 		var pathASEntries []seg.ASEntry // ASEntries that on the path, eventually in path order.
 		var epicSegAuths [][]byte
 
-		// Go through each ASEntry, starting from the last one, until we
+		// Segments are in construction order, regardless of whether they're
+		// up or down segments. We traverse them FROM THE END. So, in reverse
+		// forwarding order for down segments and in forwarding order for
+		// up segments.
+		// We go through each ASEntry, starting from the last one until we
 		// find a shortcut (which can be 0, meaning the end of the segment).
 		asEntries := solEdge.segment.ASEntries
 		for asEntryIdx := len(asEntries) - 1; asEntryIdx >= solEdge.edge.Shortcut; asEntryIdx-- {
@@ -378,6 +386,9 @@ func (solution *pathSolution) Path() Path {
 			}
 		}
 
+		// Put the hops in forwarding order. Needed for down segments
+		// since we collected hops from the end, just like for up
+		// segments.
 		if solEdge.segment.Type == proto.PathSegType_down {
 			reverseHops(hops)
 			reverseIntfs(intfs)
@@ -487,15 +498,30 @@ func reverseEpicAuths(s [][]byte) {
 }
 
 func calculateBeta(se *solutionEdge) uint16 {
+
+	// If this is a peer hop, we need to set beta[i] = beta[i+1]. That is, the SegID
+	// accumulator must correspond to the next (in construction order) hop.
+	//
+	// This is because this peering hop has a MAC that chains to its non-peering
+	// counterpart, the same as what the next hop (in construction order) chains to.
+	// So both this and the next hop are to be validated from the same SegID
+	// accumulator value: the one for the *next* hop, calculated on the regular
+	// non-peering segment.
+	//
+	// Note that, when traversing peer hops, the SegID accumulator is left untouched for the
+	// next router on the path to use.
+
 	var index int
 	if se.segment.IsDownSeg() {
 		index = se.edge.Shortcut
-		// If this is a peer, we need to set beta i+1.
 		if se.edge.Peer != 0 {
 			index++
 		}
 	} else {
 		index = len(se.segment.ASEntries) - 1
+		if index == se.edge.Shortcut && se.edge.Peer != 0 {
+			index++
+		}
 	}
 	beta := se.segment.Info.SegmentID
 	for i := 0; i < index; i++ {
@@ -586,7 +612,8 @@ func validNextSeg(currSeg, nextSeg *inputSegment) bool {
 }
 
 // segment is a helper that represents a path segment during the conversion
-// from the graph solution to the raw forwarding information.
+// from the graph solution to the raw forwarding information. The hops should
+// be in forwarding order.
 type segment struct {
 	InfoField  path.InfoField
 	HopFields  []path.HopField

private/topology/json/json.go

@@ -107,11 +107,12 @@ type GatewayInfo struct {
 // BRInterface contains the information for an data-plane BR socket that is external (i.e., facing
 // the neighboring AS).
 type BRInterface struct {
-	Underlay Underlay `json:"underlay,omitempty"`
-	IA       string   `json:"isd_as"`
-	LinkTo   string   `json:"link_to"`
-	MTU      int      `json:"mtu"`
-	BFD      *BFD     `json:"bfd,omitempty"`
+	Underlay   Underlay        `json:"underlay,omitempty"`
+	IA         string          `json:"isd_as"`
+	LinkTo     string          `json:"link_to"`
+	MTU        int             `json:"mtu"`
+	BFD        *BFD            `json:"bfd,omitempty"`
+	RemoteIFID common.IFIDType `json:"remote_interface_id,omitempty"`
 }
 
 // Underlay is the underlay information for a BR interface.

private/topology/topology.go

@@ -243,6 +243,10 @@ func (t *RWTopology) populateBR(raw *jsontopo.Topology) error {
 				return err
 			}
 			ifinfo.LinkType = LinkTypeFromString(rawIntf.LinkTo)
+			if ifinfo.LinkType == Peer {
+				ifinfo.RemoteIFID = rawIntf.RemoteIFID
+			}
+
 			if err = ifinfo.CheckLinks(t.IsCore, name); err != nil {
 				return err
 			}

router/control/conf.go

@@ -50,10 +50,11 @@ type LinkInfo struct {
 	MTU      int
 }
 
-// LinkEnd represents on end of a link.
+// LinkEnd represents one end of a link.
 type LinkEnd struct {
 	IA   addr.IA
 	Addr *net.UDPAddr
+	IFID common.IFIDType
 }
 
 type ObservableDataplane interface {
@@ -174,10 +175,12 @@ func confExternalInterfaces(dp Dataplane, cfg *Config) error {
 			Local: LinkEnd{
 				IA:   cfg.IA,
 				Addr: snet.CopyUDPAddr(iface.Local),
+				IFID: iface.ID,
 			},
 			Remote: LinkEnd{
 				IA:   iface.IA,
 				Addr: snet.CopyUDPAddr(iface.Remote),
+				IFID: iface.RemoteIFID,
 			},
 			Instance: iface.BRName,
 			BFD:      WithDefaults(BFD(iface.BFD)),