Skip to content

Commit

Permalink
Merge pull request #463 from tiborschneider/main
Browse files Browse the repository at this point in the history
Export NextHop in BGP attribute map
  • Loading branch information
tiborschneider authored Aug 9, 2024
2 parents f8c9710 + 06516d0 commit 503e57f
Show file tree
Hide file tree
Showing 5 changed files with 199 additions and 9 deletions.
19 changes: 19 additions & 0 deletions bgp/gobgp.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ func (t *bgpTask) populateRIBAttrs(path *api.Path, rib *oc.NetworkInstance_Proto
}

var (
hasNextHop bool
hasCommunity bool
commIndex uint64
hasOrigin bool
Expand All @@ -635,6 +636,20 @@ func (t *bgpTask) populateRIBAttrs(path *api.Path, rib *oc.NetworkInstance_Proto
continue
}
switch m := m.(type) {
// When advertising routes, lemming sets the next-hop attribute.
case *api.NextHopAttribute:
hasNextHop = true
attrSet.nextHop = m.GetNextHop()
// Other BGP speakers (like the OTG) may use the MP_REACH_NLRI attribute.
case *api.MpReachNLRIAttribute:
// Some BGP speakers attach two next-hops to IPv6 afi-safi routes. In that case, the
// second next-hop is a link-local address (starting with fe80:). OpenConfig, however,
// can only represent single next-hops. We therefore just take the first next-hop
// address.
if len(m.NextHops) > 0 {
hasNextHop = true
attrSet.nextHop = m.NextHops[0]
}
case *api.CommunitiesAttribute:
if comms := m.GetCommunities(); len(comms) > 0 {
hasCommunity = true
Expand Down Expand Up @@ -672,6 +687,9 @@ func (t *bgpTask) populateRIBAttrs(path *api.Path, rib *oc.NetworkInstance_Proto
attrSetIndex := t.attrSetTracker.getOrAllocIndex(attrSet)
route.SetAttrIndex(attrSetIndex)
attrSetOC := rib.GetOrCreateAttrSet(attrSetIndex)
if hasNextHop {
attrSetOC.SetNextHop(attrSet.nextHop)
}
if hasOrigin {
attrSetOC.SetOrigin(attrSet.origin)
}
Expand All @@ -697,6 +715,7 @@ type ocRIBRoute interface {
}

type ribAttrSet struct {
nextHop string
origin oc.E_BgpTypes_BgpOriginAttrType
med uint32
localPref uint32
Expand Down
1 change: 1 addition & 0 deletions bgp/tests/local_tests/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ go_test(
srcs = [
"community_count_test.go",
"community_set_test.go",
"next_hop_attr_test.go",
"own_as_path_test.go",
"policy_test.go",
"prefix_set_test.go",
Expand Down
158 changes: 158 additions & 0 deletions bgp/tests/local_tests/next_hop_attr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package local_test

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/openconfig/lemming/bgp"
"github.com/openconfig/lemming/gnmi/oc"
"github.com/openconfig/ygot/ygot"
"google.golang.org/protobuf/testing/protocmp"
)

// This test case creates a new topology in which all IP addresses are fixed. The topology consists
// of only two connected devices. The second device sets the next-hop to itself.
func TestNextHopAttr(t *testing.T) {
prefix := "10.0.0.0/10"

dut1, dut2, stop := setupShortTopology(t)
defer stop()

// Install regular test route into DUT1.
route := &oc.NetworkInstance_Protocol_Static{
Prefix: ygot.String(prefix),
NextHop: map[string]*oc.NetworkInstance_Protocol_Static_NextHop{
"single": {
Index: ygot.String("single"),
NextHop: oc.UnionString("192.0.2.1"),
Recurse: ygot.Bool(true),
},
},
}
installStaticRoute(t, dut1, route)

testAttrsShortPath(t, prefix, dut1, dut2, testCaseShortPath{
Dut1AdjRibOutPreNextHop: "192.0.2.1",
Dut1AdjRibOutPostNextHop: "192.0.2.1",
Dut2AdjRibInPreNextHop: "192.0.2.1",
Dut2AdjRibInPostNextHop: "192.0.2.1",
Dut2LocalRibNextHop: "192.0.2.1",
})
}

func setupShortTopology(t *testing.T) (*Device, *Device, func()) {
t.Helper()

dut1, stop1 := newLemming(t, 1, 64500, []*AddIntfAction{{
name: "eth0",
ifindex: 0,
enabled: true,
prefix: "192.0.2.1/30",
niName: "DEFAULT",
}})
dut2, stop2 := newLemming(t, 2, 64500, nil)

// Remove any existing BGP config
Delete(t, dut1, bgp.BGPPath.Config())
Delete(t, dut2, bgp.BGPPath.Config())
Delete(t, dut1, bgp.RoutingPolicyPath.Config())
Delete(t, dut2, bgp.RoutingPolicyPath.Config())

establishSessionPairs(t, DevicePair{dut1, dut2})

// Clear the path for routes to be propagated.
Replace(t, dut1, bgp.BGPPath.Neighbor(dut2.RouterID).ApplyPolicy().DefaultExportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE)
Replace(t, dut2, bgp.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().DefaultImportPolicy().Config(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE)

// Wait until policies are installed.
Await(t, dut1, bgp.BGPPath.Neighbor(dut2.RouterID).ApplyPolicy().DefaultExportPolicy().State(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE)
Await(t, dut2, bgp.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().DefaultImportPolicy().State(), oc.RoutingPolicy_DefaultPolicyType_ACCEPT_ROUTE)

return dut1, dut2, func() {
stop1()
stop2()
}
}

type testCaseShortPath struct {
Dut1AdjRibOutPreNextHop string
Dut1AdjRibOutPostNextHop string
Dut2AdjRibInPreNextHop string
Dut2AdjRibInPostNextHop string
Dut2LocalRibNextHop string
}

func testAttrsShortPath(t *testing.T, prefix string, dut1, dut2 *Device, routeTest testCaseShortPath) {
dut1AttrSetMap := Lookup(t, dut1, bgp.BGPPath.Rib().AttrSetMap().State())
dut1AttrMap, _ := dut1AttrSetMap.Val()
dut2AttrSetMap := Lookup(t, dut2, bgp.BGPPath.Rib().AttrSetMap().State())
dut2AttrMap, _ := dut2AttrSetMap.Val()
updateAttrMaps := func() {
dut1AttrSetMap = Lookup(t, dut1, bgp.BGPPath.Rib().AttrSetMap().State())
dut1AttrMap, _ = dut1AttrSetMap.Val()
dut2AttrSetMap = Lookup(t, dut2, bgp.BGPPath.Rib().AttrSetMap().State())
dut2AttrMap, _ = dut2AttrSetMap.Val()
}

v4uni := bgp.BGPPath.Rib().AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).Ipv4Unicast()

if diff := awaitNoDiff(func() string {
attrs, err := getAttrs(t, dut1, dut1AttrMap, v4uni.Neighbor(dut2.RouterID).AdjRibOutPre().Route(prefix, 0).AttrIndex().State())
if err != nil {
return err.Error()
}
return cmp.Diff(routeTest.Dut1AdjRibOutPreNextHop, attrs.GetNextHop(), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Fatalf("DUT 1 AdjRibOutPre attribute difference (prefix %s) (-want, +got):\n%s", prefix, diff)
}
if diff := awaitNoDiff(func() string {
attrs, err := getAttrs(t, dut1, dut1AttrMap, v4uni.Neighbor(dut2.RouterID).AdjRibOutPost().Route(prefix, 0).AttrIndex().State())
if err != nil {
return err.Error()
}
return cmp.Diff(routeTest.Dut1AdjRibOutPostNextHop, attrs.GetNextHop(), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Fatalf("DUT 1 AdjRibOutPost attribute difference (prefix %s) (-want, +got):\n%s", prefix, diff)
}
if diff := awaitNoDiff(func() string {
attrs, err := getAttrs(t, dut2, dut2AttrMap, v4uni.Neighbor(dut1.RouterID).AdjRibInPre().Route(prefix, 0).AttrIndex().State())
if err != nil {
return err.Error()
}
return cmp.Diff(routeTest.Dut2AdjRibInPreNextHop, attrs.GetNextHop(), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Fatalf("DUT 2 AdjRibInPre attribute difference (prefix %s) (-want, +got):\n%s", prefix, diff)
}
if diff := awaitNoDiff(func() string {
attrs, err := getAttrs(t, dut2, dut2AttrMap, v4uni.Neighbor(dut1.RouterID).AdjRibInPost().Route(prefix, 0).AttrIndex().State())
if err != nil {
return err.Error()
}
return cmp.Diff(routeTest.Dut2AdjRibInPostNextHop, attrs.GetNextHop(), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Fatalf("DUT 2 AdjRibInPost attribute difference (prefix %s) (-want, +got):\n%s", prefix, diff)
}
if diff := awaitNoDiff(func() string {
attrs, err := getAttrs(t, dut2, dut2AttrMap, v4uni.LocRib().Route(prefix, oc.UnionString(dut1.RouterID), 0).AttrIndex().State())
if err != nil {
return err.Error()
}
return cmp.Diff(routeTest.Dut2LocalRibNextHop, attrs.GetNextHop(), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Fatalf("DUT 2 LocRib routeTest difference (prefix %s) (-want, +got):\n%s", prefix, diff)
}
}
29 changes: 20 additions & 9 deletions bgp/tests/local_tests/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,17 @@ func setDefaultAttrs(a *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet, rejected bo
if a.LocalPref == nil {
a.LocalPref = ygot.Uint32(100)
}
// reset the next-hop
a.NextHop = nil
return a
}

// Set the next-hop to nil, so that test-cases do not need to compare the next-hop.
func resetNextHop(a *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet) *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet {
if a == nil {
return nil
}
a.NextHop = nil
return a
}

Expand Down Expand Up @@ -430,7 +441,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.PrevAdjRibOutPreAttrs, false), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.PrevAdjRibOutPreAttrs, false), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibOutPre attribute difference (prefix %s) (-want, +got):\n%s", prevDUT.ID, prefix, diff)
}
Expand All @@ -439,7 +450,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.PrevAdjRibOutPostAttrs, false), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.PrevAdjRibOutPostAttrs, false), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibOutPost attribute difference (prefix %s) (-want, +got):\n%s", prevDUT.ID, prefix, diff)
}
Expand All @@ -448,7 +459,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibInPreAttrs, false), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibInPreAttrs, false), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibInPre attribute difference (prefix %s) (-want, +got):\n%s", currDUT.ID, prefix, diff)
}
Expand All @@ -457,7 +468,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibInPostAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibInPostAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibInPost attribute difference (prefix %s) (-want, +got):\n%s", currDUT.ID, prefix, diff)
}
Expand All @@ -466,7 +477,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.LocalRibAttrs, routeTest.ExpectedResult != policytest.RouteAccepted), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.LocalRibAttrs, routeTest.ExpectedResult != policytest.RouteAccepted), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v LocRib routeTest difference (prefix %s) (-want, +got):\n%s", currDUT.ID, prefix, diff)
}
Expand All @@ -475,7 +486,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibOutPreAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibOutPreAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibOutPre attribute difference (prefix %s) (-want, +got):\n%s\n%+v", currDUT.ID, prefix, diff, routeTest.AdjRibOutPreAttrs)
}
Expand All @@ -484,7 +495,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibOutPostAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.AdjRibOutPostAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibOutPost attribute difference (prefix %s) (-want, +got):\n%s", currDUT.ID, prefix, diff)
}
Expand All @@ -493,7 +504,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.NextAdjRibInPreAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.NextAdjRibInPreAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v AdjRibInPre attribute difference (prefix %s) (-want, +got):\n%s", nextDUT.ID, prefix, diff)
}
Expand All @@ -502,7 +513,7 @@ func testAttrs(t *testing.T, route policytest.TestRoute, routeTest *policytest.R
if err != nil {
return err.Error()
}
return cmp.Diff(setDefaultAttrs(routeTest.NextLocalRibAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), attrs, protocmp.Transform())
return cmp.Diff(setDefaultAttrs(routeTest.NextLocalRibAttrs, routeTest.ExpectedResult == policytest.RouteDiscarded), resetNextHop(attrs), protocmp.Transform())
}, updateAttrMaps); diff != "" {
t.Errorf("DUT %v LocRib attribute difference (prefix %s) (-want, +got):\n%s", nextDUT.ID, prefix, diff)
}
Expand Down
1 change: 1 addition & 0 deletions policytest/definitions.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type RoutePathTestCase struct {
NextAdjRibInPreCommunities []string
NextLocalRibCommunities []string

// The next-hop attribute will not be compared (the addresses are not stable between test runs)
PrevAdjRibOutPreAttrs *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet
PrevAdjRibOutPostAttrs *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet
AdjRibInPreAttrs *oc.NetworkInstance_Protocol_Bgp_Rib_AttrSet
Expand Down

0 comments on commit 503e57f

Please sign in to comment.