From a864a8c12e228cf0a891f6ab81ce5daf8f3673c8 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Mon, 9 Sep 2024 12:18:45 +0200 Subject: [PATCH] topology: allow peering links between core ASes (#4605) Fixes #4484 --- .buildkite/pipeline.yml | 6 ++--- control/beaconing/originator.go | 8 +++++- control/beaconing/originator_test.go | 3 +++ control/beaconing/testdata/topology-core.json | 10 +++++++ private/path/combinator/combinator_test.go | 27 ++++++++++++++++--- .../testdata/00_core_core_invalid_peering.txt | 25 +++++++++++++++++ .../combinator/testdata/00_multi_peering.txt | 22 +++++++++++++++ private/topology/topology.go | 2 +- tools/await-connectivity | 11 +++++--- 9 files changed, 102 insertions(+), 12 deletions(-) create mode 100644 private/path/combinator/testdata/00_core_core_invalid_peering.txt diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index 72c9c14cdb..766833d3ce 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 c35b4e9038..3c7bf2067c 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/control/beaconing/originator_test.go b/control/beaconing/originator_test.go index e7d64673a5..63d625bfe3 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 11284ccd2f..eaae64d627 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 15bb1b6d4e..af67f0ebf4 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 0000000000..6b712da657 --- /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 d0aafe0697..e6191e236c 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 .. diff --git a/private/topology/topology.go b/private/topology/topology.go index ef1234a602..4677a61592 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 4f0d1f2aee..e0876f1509 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