Repository: scionproto/scion (go)

Issue:
topology: allow peering links between core-ASes
Currently, the topology loader forbids core ASes from having peering links (i.e. links with `"link_to": "peer"`). The validation check is here: https://github.com/scionproto/scion/blob/4a8bc98ec7d969dd622c5a90fe677e3c980aaa88/private/topology/topology.go#L544-L550.
As far as I'm aware, there is no technical reason why core ASes cannot have peering links. 
Core ASes should be able to have peering links to any other AS, i.e. to both core or non-core ASes in both the same or different ISDs.


Pull request:
topology: allow peering links between core ASes
Fixes #4484 

Patch:
diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml
index 72c9c14cd..766833d3c 100644
--- a/.buildkite/pipeline.yml
+++ b/.buildkite/pipeline.yml
@@ -116,7 +116,7 @@ steps:
         - ./scion.sh run
         - tools/await-connectivity
         - ./bin/scion_integration || ( echo "^^^ +++" && false )
-        - ./bin/end2end_integration || ( echo "^^^ +++" && false )
+        - ./bin/end2end_integration --attempts=3 || ( echo "^^^ +++" && false )
       plugins: &scion-run-hooks
         - scionproto/metahook#v0.3.0:
             pre-command: .buildkite/cleanup-leftovers.sh
@@ -142,7 +142,7 @@ steps:
         - ./scion.sh topology -c topology/default-no-peers.topo
         - ./scion.sh run
         - tools/await-connectivity
-        - ./bin/end2end_integration || ( echo "^^^ +++" && false )
+        - ./bin/end2end_integration --attempts=3 || ( echo "^^^ +++" && false )
         - ./tools/integration/revocation_test.sh
       plugins: *scion-run-hooks
       artifact_paths: *scion-run-artifact-paths
@@ -158,7 +158,7 @@ steps:
         - ./scion.sh run
         - tools/await-connectivity
         - echo "--- run tests"
-        - ./bin/end2end_integration -d || ( echo "^^^ +++" && false )
+        - ./bin/end2end_integration -d --attempts=3 || ( echo "^^^ +++" && false )
       plugins: *scion-run-hooks
       artifact_paths: *scion-run-artifact-paths
       timeout_in_minutes: 15
diff --git a/control/beaconing/originator.go b/control/beaconing/originator.go
index c35b4e903..3c7bf2067 100644
--- a/control/beaconing/originator.go
+++ b/control/beaconing/originator.go
@@ -32,6 +32,7 @@ import (
 	"github.com/scionproto/scion/pkg/private/serrors"
 	seg "github.com/scionproto/scion/pkg/segment"
 	"github.com/scionproto/scion/private/periodic"
+	"github.com/scionproto/scion/private/topology"
 )
 
 var _ periodic.Task = (*Originator)(nil)
@@ -95,6 +96,9 @@ func (o *Originator) originateBeacons(ctx context.Context) {
 		return
 	}
 
+	// Core ases can have peering too. Fetch the peering interfaces.
+	peers := sortedIntfs(o.AllInterfaces, topology.Peer)
+
 	// Only log on info and error level every propagation period to reduce
 	// noise. The offending logs events are redirected to debug level.
 	silent := !o.Tick.Passed()
@@ -109,6 +113,7 @@ func (o *Originator) originateBeacons(ctx context.Context) {
 			intf:       intf,
 			timestamp:  o.Tick.Now(),
 			summary:    s,
+			peers:      peers,
 		}
 		go func() {
 			defer log.HandlePanic()
@@ -154,6 +159,7 @@ type beaconOriginator struct {
 	intf      *ifstate.Interface
 	timestamp time.Time
 	summary   *summary
+	peers     []uint16
 }
 
 // originateBeacon originates a beacon on the given ifID.
@@ -206,7 +212,7 @@ func (o *beaconOriginator) createBeacon(ctx context.Context) (*seg.PathSegment,
 		return nil, serrors.Wrap("creating segment", err)
 	}
 
-	if err := o.Extender.Extend(ctx, beacon, 0, o.intf.TopoInfo().ID, nil); err != nil {
+	if err := o.Extender.Extend(ctx, beacon, 0, o.intf.TopoInfo().ID, o.peers); err != nil {
 		return nil, serrors.Wrap("extending segment", err)
 	}
 	return beacon, nil
diff --git a/private/topology/topology.go b/private/topology/topology.go
index ef1234a60..4677a6159 100644
--- a/private/topology/topology.go
+++ b/private/topology/topology.go
@@ -593,7 +593,7 @@ func (m IDAddrMap) copy() IDAddrMap {
 func (i IFInfo) CheckLinks(isCore bool, brName string) error {
 	if isCore {
 		switch i.LinkType {
-		case Core, Child:
+		case Core, Child, Peer:
 		default:
 			return serrors.New("Illegal link type for core AS",
 				"type", i.LinkType, "br", brName)
diff --git a/tools/await-connectivity b/tools/await-connectivity
index 4f0d1f2ae..e0876f150 100755
--- a/tools/await-connectivity
+++ b/tools/await-connectivity
@@ -61,7 +61,7 @@ check_connectivity() {
   for as in $cores; do
     missing=$(comm -23 \
       <(printf "%s\n" $cores | grep -v $as | sort) \
-      <(curl -sfS $(cs_api_url_segments "$as") | jq '.[].start_isd_as' -r | sort -u)
+      <(curl --connect-timeout 5 -sfS $(cs_api_url_segments "$as") | jq '.[].start_isd_as' -r | sort -u)
     )
     if [ -n "$missing" ]; then
       echo "$as: waiting for" $missing
@@ -71,7 +71,7 @@ check_connectivity() {
 
   # non-core ASes: wait for at least one (up-)segment
   for as in $noncores; do
-    curl -sfS $(cs_api_url_segments "$as") | jq -e 'length > 0' > /dev/null || { 
+    curl --connect-timeout 5 -sfS $(cs_api_url_segments "$as") | jq -e 'length > 0' > /dev/null || { 
       echo "$as waiting for up segment"
       ret=1
     }
@@ -88,12 +88,12 @@ main() {
   local noncores=$(sed -n '/Non-core/,${s/^- //p}' gen/as_list.yml)
 
   for i in $(seq 1 "$QUIET"); do
-    check_connectivity "$cores" "$noncores" > /dev/null && exit 0
+    check_connectivity "$cores" "$noncores" > /dev/null && return 0
     sleep 1
   done
   for i in $(seq "$QUIET" $((TIMEOUT-1))); do
     echo "Check after ${i}s"
-    check_connectivity "$cores" "$noncores" && exit 0
+    check_connectivity "$cores" "$noncores" && return 0
     sleep 1
   done
   echo "Check after ${TIMEOUT}s"
@@ -101,3 +101,6 @@ main() {
 }
 
 main "$@"
+
+# that is not enough. Down segment registrations don't seem to happen fast.
+sleep 10


Test patch:
diff --git a/control/beaconing/originator_test.go b/control/beaconing/originator_test.go
index e7d64673a..63d625bfe 100644
--- a/control/beaconing/originator_test.go
+++ b/control/beaconing/originator_test.go
@@ -104,6 +104,9 @@ func TestOriginatorRun(t *testing.T) {
 						hopF := b.ASEntries[b.MaxIdx()].HopEntry.HopField
 						// Check the interface matches.
 						assert.Equal(t, hopF.ConsEgress, egIfID)
+						// Check that the expected peering entry is there too.
+						peering := b.ASEntries[b.MaxIdx()].PeerEntries[0]
+						assert.Equal(t, peering.HopField.ConsIngress, uint16(4242))
 						// Check that the beacon is sent to the correct border router.
 						br := net.UDPAddrFromAddrPort(interfaceInfos(topo)[egIfID].InternalAddr)
 						assert.Equal(t, br, nextHop)
diff --git a/control/beaconing/testdata/topology-core.json b/control/beaconing/testdata/topology-core.json
index 11284ccd2..eaae64d62 100644
--- a/control/beaconing/testdata/topology-core.json
+++ b/control/beaconing/testdata/topology-core.json
@@ -45,6 +45,16 @@
           "isd_as": "1-ff00:0:111",
           "link_to": "CHILD",
           "mtu": 1280
+        },
+        "4242": {
+          "underlay": {
+            "local": "127.0.0.5:0",
+            "remote": "127.0.0.4:0"
+          },
+          "isd_as": "2-ff00:0:220",
+          "link_to": "PEER",
+          "remote_interface_id": 2424,
+          "mtu": 1280
         }
       }
     }
diff --git a/private/path/combinator/combinator_test.go b/private/path/combinator/combinator_test.go
index 15bb1b6d4..af67f0ebf 100644
--- a/private/path/combinator/combinator_test.go
+++ b/private/path/combinator/combinator_test.go
@@ -96,10 +96,12 @@ func TestBadPeering(t *testing.T) {
 	}
 }
 
-func TestMultiPeering(t *testing.T) {
+func TestMiscPeering(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 	g := graph.NewDefaultGraph(ctrl)
+	// Add a core-core peering link. It can be used in some cases.
+	g.AddLink("1-ff00:0:130", 4001, "2-ff00:0:210", 4002, true)
 
 	testCases := []struct {
 		Name     string
@@ -111,7 +113,7 @@ func TestMultiPeering(t *testing.T) {
 		Downs    []*seg.PathSegment
 	}{
 		{
-			Name:     "two peerings between same ases",
+			Name:     "two peerings between same ases and core core peering",
 			FileName: "00_multi_peering.txt",
 			SrcIA:    addr.MustParseIA("1-ff00:0:112"),
 			DstIA:    addr.MustParseIA("2-ff00:0:212"),
@@ -125,8 +127,27 @@ func TestMultiPeering(t *testing.T) {
 				g.Beacon([]uint16{graph.If_210_X_211_A, graph.If_211_A_212_X}),
 			},
 		},
+		{
+			// In this case, the 130-210 peering link should not be used (the router would reject)
+			// because the hop through 210 would be assimilated to a valley path: one of the
+			// joined segments is a core segment, not a down segment.
+			Name:     "core to core peering forbidden",
+			FileName: "00_core_core_invalid_peering.txt",
+			SrcIA:    addr.MustParseIA("1-ff00:0:131"),
+			DstIA:    addr.MustParseIA("2-ff00:0:221"),
+			Ups: []*seg.PathSegment{
+				g.Beacon([]uint16{graph.If_130_A_131_X}),
+			},
+			Cores: []*seg.PathSegment{
+				g.Beacon([]uint16{
+					graph.If_220_X_210_X, graph.If_210_X_110_X, graph.If_110_X_130_A}),
+			},
+			Downs: []*seg.PathSegment{
+				g.Beacon([]uint16{graph.If_220_X_221_X}),
+			},
+		},
 	}
-	t.Log("TestMultiPeering")
+	t.Log("TestMiscPeering")
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
 			result := combinator.Combine(tc.SrcIA, tc.DstIA, tc.Ups, tc.Cores, tc.Downs, false)
diff --git a/private/path/combinator/testdata/00_core_core_invalid_peering.txt b/private/path/combinator/testdata/00_core_core_invalid_peering.txt
new file mode 100644
index 000000000..6b712da65
--- /dev/null
+++ b/private/path/combinator/testdata/00_core_core_invalid_peering.txt
@@ -0,0 +1,25 @@
+Path #0:
+  Weight: 5
+  Fields:
+    IF ..
+      HF InIF=1613 OutIF=0
+      HF InIF=0 OutIF=1316
+    IF ..
+      HF InIF=1311 OutIF=0
+      HF InIF=1121 OutIF=1113
+      HF InIF=2122 OutIF=2111
+      HF InIF=0 OutIF=2221
+    IF C.
+      HF InIF=0 OutIF=2224
+      HF InIF=2422 OutIF=0
+  Interfaces:
+    1-ff00:0:131#1613
+    1-ff00:0:130#1316
+    1-ff00:0:130#1311
+    1-ff00:0:110#1113
+    1-ff00:0:110#1121
+    2-ff00:0:210#2111
+    2-ff00:0:210#2122
+    2-ff00:0:220#2221
+    2-ff00:0:220#2224
+    2-ff00:0:221#2422
diff --git a/private/path/combinator/testdata/00_multi_peering.txt b/private/path/combinator/testdata/00_multi_peering.txt
index d0aafe069..e6191e236 100644
--- a/private/path/combinator/testdata/00_multi_peering.txt
+++ b/private/path/combinator/testdata/00_multi_peering.txt
@@ -31,6 +31,28 @@ Path #1:
     2-ff00:0:211#2325
     2-ff00:0:212#2523
 Path #2:
+  Weight: 5
+  Fields:
+    IF .P
+      HF InIF=1714 OutIF=0
+      HF InIF=1432 OutIF=1417
+      HF InIF=4001 OutIF=3214
+    IF CP
+      HF InIF=4002 OutIF=2123
+      HF InIF=2321 OutIF=2325
+      HF InIF=2523 OutIF=0
+  Interfaces:
+    1-ff00:0:112#1714
+    1-ff00:0:111#1417
+    1-ff00:0:111#1432
+    1-ff00:0:130#3214
+    1-ff00:0:130#4001
+    2-ff00:0:210#4002
+    2-ff00:0:210#2123
+    2-ff00:0:211#2321
+    2-ff00:0:211#2325
+    2-ff00:0:212#2523
+Path #3:
   Weight: 6
   Fields:
     IF ..