Merge pull request #312 from openconfig/gribi-route-del

Support gRIBI route deletion
openconfig · Oct 24, 2023 · e818bf5 · e818bf5
2 parents d404084 + 048d168
commit e818bf5
Show file tree

Hide file tree

Showing 9 changed files with 277 additions and 85 deletions.
diff --git a/bgp/tests/local_tests/BUILD b/bgp/tests/local_tests/BUILD
@@ -22,6 +22,11 @@ go_test(
         "//gnmi/oc/ocpath",
         "//proto/policyval",
         "@com_github_openconfig_gnmi//proto/gnmi",
+        "@com_github_openconfig_gribi//v1/proto/service",
+        "@com_github_openconfig_gribigo//chk",
+        "@com_github_openconfig_gribigo//client",
+        "@com_github_openconfig_gribigo//constants",
+        "@com_github_openconfig_gribigo//fluent",
         "@com_github_openconfig_ygnmi//ygnmi",
         "@com_github_openconfig_ygot//ygot",
         "@org_golang_google_grpc//:go_default_library",

diff --git a/bgp/tests/local_tests/route_propagation_test.go b/bgp/tests/local_tests/route_propagation_test.go
@@ -15,9 +15,15 @@
 package local_test
 
 import (
+	"context"
 	"fmt"
 	"testing"
+	"time"
 
+	"github.com/openconfig/gribigo/chk"
+	"github.com/openconfig/gribigo/client"
+	"github.com/openconfig/gribigo/constants"
+	"github.com/openconfig/gribigo/fluent"
 	"github.com/openconfig/lemming/bgp"
 	"github.com/openconfig/lemming/gnmi/fakedevice"
 	"github.com/openconfig/lemming/gnmi/oc"
@@ -33,6 +39,32 @@ func installStaticRoute(t *testing.T, dut *Device, route *oc.NetworkInstance_Pro
 	Await(t, dut, staticp.Static(*route.Prefix).State(), route)
 }
 
+// awaitTimeout calls a fluent client Await, adding a timeout to the context.
+func awaitTimeout(ctx context.Context, c *fluent.GRIBIClient, t testing.TB, timeout time.Duration) error {
+	subctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+	return c.Await(subctx, t)
+}
+
+func installGRIBIRoutes(ctx context.Context, t *testing.T, dut *Device, entries []fluent.GRIBIEntry, results []*client.OpResult, isDelete bool) {
+	if err := awaitTimeout(ctx, dut.gribic, t, time.Minute); err != nil {
+		t.Fatalf("Await got error during session negotiation: %v", err)
+	}
+
+	if !isDelete {
+		dut.gribic.Modify().AddEntry(t, entries...)
+	} else {
+		dut.gribic.Modify().DeleteEntry(t, entries...)
+	}
+	if err := awaitTimeout(ctx, dut.gribic, t, time.Minute); err != nil {
+		t.Fatalf("Await got error for entries: %v", err)
+	}
+
+	for _, wantResult := range results {
+		chk.HasResult(t, dut.gribic.Results(t), wantResult, chk.IgnoreOperationID())
+	}
+}
+
 func formatYgot(v any) string {
 	str, err := ygot.Marshal7951(v, ygot.JSONIndent("  "))
 	if err != nil {
@@ -42,12 +74,13 @@ func formatYgot(v any) string {
 }
 
 func awaitNotPresent[T any](t *testing.T, d *Device, q ygnmi.SingletonQuery[T]) {
+	t.Helper()
 	w := Watch(t, d, q, rejectTimeout, func(val *ygnmi.Value[T]) bool {
 		_, ok := val.Val()
 		return !ok
 	})
 	if _, ok := w.Await(t); !ok {
-		t.Fatalf("prefix (%v) was not rejected within timeout.", d)
+		t.Fatalf("prefix was not rejected within timeout at %v.", d)
 	}
 }
 
@@ -97,53 +130,122 @@ func TestRoutePropagation(t *testing.T) {
 	}
 	awaitDefaultPolicies()
 
-	prefix := "10.10.10.0/24"
-	installStaticRoute(t, dut1, &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),
-			},
-		},
-	})
-
 	staticp := ocpath.Root().NetworkInstance(fakedevice.DefaultNetworkInstance).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_STATIC, fakedevice.StaticRoutingProtocol)
-	v := GetAll(t, dut1, staticp.StaticAny().Config())
-	t.Logf("Installed static route: %s", formatYgot(v))
-
-	v4uni := ocpath.Root().NetworkInstance(fakedevice.DefaultNetworkInstance).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, fakedevice.BGPRoutingProtocol).Bgp().Rib().AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).Ipv4Unicast()
-	// Check route is in Adj-In of dut2.
-	Await(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
-	Await(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPost().Route(prefix, 0).Prefix().State(), prefix)
-	// Check route is in Loc-RIB of dut2.
-	Await(t, dut2, v4uni.LocRib().Route(prefix, oc.UnionString(dut1.RouterID), 0).Prefix().State(), prefix)
-	// Check route is in Adj-Out of dut2.
-	Await(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPre().Route(prefix, 0).Prefix().State(), prefix)
-	Await(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPost().Route(prefix, 0).Prefix().State(), prefix)
-
-	// Check route is in Adj-In of dut3.
-	Await(t, dut3, v4uni.Neighbor(dut2.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
-
-	// Check route is in Adj-In of dut4.
-	Await(t, dut4, v4uni.Neighbor(dut3.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
-
-	// -------------- Test deletion ----------------
-	Delete[*oc.NetworkInstance_Protocol_Static](t, dut1, staticp.Static(prefix).Config())
-
-	// Check route is not in Adj-In of dut2.
-	awaitNotPresent(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
-	awaitNotPresent(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPost().Route(prefix, 0).Prefix().State())
-	// Check route is not in Loc-RIB of dut2.
-	awaitNotPresent(t, dut2, v4uni.LocRib().Route(prefix, oc.UnionString(dut1.RouterID), 0).Prefix().State())
-	// Check route is not in Adj-Out of dut2.
-	awaitNotPresent(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPre().Route(prefix, 0).Prefix().State())
-	awaitNotPresent(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPost().Route(prefix, 0).Prefix().State())
-
-	// Check route is not in Adj-In of dut3.
-	awaitNotPresent(t, dut3, v4uni.Neighbor(dut2.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
-
-	// Check route is not in Adj-In of dut4.
-	awaitNotPresent(t, dut4, v4uni.Neighbor(dut3.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
+
+	checkpath := func(t *testing.T, prefix string, static bool) {
+		if static {
+			installStaticRoute(t, dut1, &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),
+					},
+				},
+			})
+
+			v := GetAll(t, dut1, staticp.StaticAny().Config())
+			t.Logf("Installed static route: %s", formatYgot(v))
+		} else {
+			installGRIBIRoutes(context.Background(), t, dut1,
+				[]fluent.GRIBIEntry{
+					fluent.NextHopEntry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithIndex(42).WithIPAddress("192.0.2.1"),
+					fluent.NextHopGroupEntry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithID(10).AddNextHop(42, 1),
+					fluent.IPv4Entry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithPrefix(prefix).WithNextHopGroup(10),
+				}, []*client.OpResult{
+					fluent.OperationResult().
+						WithNextHopOperation(42).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Add).
+						AsResult(),
+					fluent.OperationResult().
+						WithNextHopGroupOperation(10).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Add).
+						AsResult(),
+					fluent.OperationResult().
+						WithIPv4Operation(prefix).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Add).
+						AsResult(),
+				},
+				false,
+			)
+		}
+
+		v4uni := ocpath.Root().NetworkInstance(fakedevice.DefaultNetworkInstance).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, fakedevice.BGPRoutingProtocol).Bgp().Rib().AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).Ipv4Unicast()
+		// Check route is in Adj-In of dut2.
+		Await(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
+		Await(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPost().Route(prefix, 0).Prefix().State(), prefix)
+		// Check route is in Loc-RIB of dut2.
+		Await(t, dut2, v4uni.LocRib().Route(prefix, oc.UnionString(dut1.RouterID), 0).Prefix().State(), prefix)
+		// Check route is in Adj-Out of dut2.
+		Await(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPre().Route(prefix, 0).Prefix().State(), prefix)
+		Await(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPost().Route(prefix, 0).Prefix().State(), prefix)
+
+		// Check route is in Adj-In of dut3.
+		Await(t, dut3, v4uni.Neighbor(dut2.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
+
+		// Check route is in Adj-In of dut4.
+		Await(t, dut4, v4uni.Neighbor(dut3.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State(), prefix)
+
+		// -------------- Test deletion ----------------
+		if static {
+			Delete[*oc.NetworkInstance_Protocol_Static](t, dut1, staticp.Static(prefix).Config())
+		} else {
+			installGRIBIRoutes(context.Background(), t, dut1,
+				[]fluent.GRIBIEntry{
+					fluent.IPv4Entry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithPrefix(prefix).WithNextHopGroup(10),
+					fluent.NextHopGroupEntry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithID(10).AddNextHop(42, 1),
+					fluent.NextHopEntry().WithNetworkInstance(fakedevice.DefaultNetworkInstance).
+						WithIndex(42).WithIPAddress("192.0.2.1"),
+				}, []*client.OpResult{
+					fluent.OperationResult().
+						WithNextHopOperation(42).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Delete).
+						AsResult(),
+					fluent.OperationResult().
+						WithNextHopGroupOperation(10).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Delete).
+						AsResult(),
+					fluent.OperationResult().
+						WithIPv4Operation(prefix).
+						WithProgrammingResult(fluent.InstalledInFIB).
+						WithOperationType(constants.Delete).
+						AsResult(),
+				},
+				true,
+			)
+		}
+
+		// Check route is not in Adj-In of dut2.
+		awaitNotPresent(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
+		awaitNotPresent(t, dut2, v4uni.Neighbor(dut1.RouterID).AdjRibInPost().Route(prefix, 0).Prefix().State())
+		// Check route is not in Loc-RIB of dut2.
+		awaitNotPresent(t, dut2, v4uni.LocRib().Route(prefix, oc.UnionString(dut1.RouterID), 0).Prefix().State())
+		// Check route is not in Adj-Out of dut2.
+		awaitNotPresent(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPre().Route(prefix, 0).Prefix().State())
+		awaitNotPresent(t, dut2, v4uni.Neighbor(dut3.RouterID).AdjRibOutPost().Route(prefix, 0).Prefix().State())
+
+		// Check route is not in Adj-In of dut3.
+		awaitNotPresent(t, dut3, v4uni.Neighbor(dut2.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
+
+		// Check route is not in Adj-In of dut4.
+		awaitNotPresent(t, dut4, v4uni.Neighbor(dut3.RouterID).AdjRibInPre().Route(prefix, 0).Prefix().State())
+	}
+
+	t.Run("static", func(t *testing.T) {
+		checkpath(t, "10.10.10.0/24", true)
+	})
+	t.Run("gribi", func(t *testing.T) {
+		checkpath(t, "20.20.20.0/24", false)
+	})
 }
diff --git a/bgp/tests/local_tests/session_establish_test.go b/bgp/tests/local_tests/session_establish_test.go
@@ -24,6 +24,7 @@ import (
 	"sync/atomic"
 	"testing"
 
+	"github.com/openconfig/gribigo/fluent"
 	"github.com/openconfig/lemming"
 	"github.com/openconfig/lemming/bgp"
 	"github.com/openconfig/lemming/gnmi"
@@ -37,12 +38,14 @@ import (
 	"google.golang.org/grpc/credentials/local"
 
 	gpb "github.com/openconfig/gnmi/proto/gnmi"
+	spb "github.com/openconfig/gribi/v1/proto/service"
 )
 
 // TODO: Consolidate test helper code with integration and other unit tests.
 
 type Device struct {
 	yc       *ygnmi.Client
+	gribic   *fluent.GRIBIClient
 	ID       uint
 	AS       uint32
 	bgpPort  uint16
@@ -142,13 +145,29 @@ func newLemming(t *testing.T, id uint, as uint32, connectedIntfs []*AddIntfActio
 		configureInterface(t, intf, l.GNMI())
 	}
 
+	conn, err := grpc.Dial(gribiTarget, grpc.WithTransportCredentials(local.NewCredentials()))
+	if err != nil {
+		t.Fatalf("cannot dial gNMI server, %v", err)
+	}
+	gribistub := spb.NewGRIBIClient(conn)
+
+	c := fluent.NewClient()
+	c.Connection().WithStub(gribistub).
+		WithRedundancyMode(fluent.ElectedPrimaryClient).
+		WithPersistence().
+		WithFIBACK().
+		WithInitialElectionID(1, 0)
+	c.Start(context.Background(), t)
+	c.StartSending(context.Background(), t)
+
 	return &Device{
 		yc:       ygnmiClient(t, target, gnmiTarget),
+		gribic:   c,
 		ID:       id,
 		AS:       as,
 		bgpPort:  1111,
 		RouterID: routerID,
-	}, func() { l.Stop() }
+	}, func() { l.Stop(); c.Stop(t) }
 }
 
 type DevicePair struct {

diff --git a/go.mod b/go.mod
@@ -17,7 +17,7 @@ require (
 	github.com/openconfig/gnsi v1.2.2
 	github.com/openconfig/goyang v1.4.1
 	github.com/openconfig/gribi v1.0.0
-	github.com/openconfig/gribigo v0.0.0-20230902004455-c7aff9365bec
+	github.com/openconfig/gribigo v0.0.0-20231018021145-a1124db3ca83
 	github.com/openconfig/kne v0.1.14
 	github.com/openconfig/ondatra v0.2.7
 	github.com/openconfig/ygnmi v0.8.11

diff --git a/go.sum b/go.sum
@@ -955,8 +955,8 @@ github.com/openconfig/goyang v1.4.1/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+R
 github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f/go.mod h1:OoH46A2kV42cIXGyviYmAlGmn6cHjGduyC2+I9d/iVs=
 github.com/openconfig/gribi v1.0.0 h1:xMwEg0mBD+21mOxuFOw0d9dBKuIPwJEhMUUeUulZdLg=
 github.com/openconfig/gribi v1.0.0/go.mod h1:VFqGH2ZPFIfnKTimP4/AQB4OK0eySW5muJNFxXAwP6k=
-github.com/openconfig/gribigo v0.0.0-20230902004455-c7aff9365bec h1:Hz4EEMaBnNbumV+F+4snVI6FTwUmEogjgun/fY5F7F8=
-github.com/openconfig/gribigo v0.0.0-20230902004455-c7aff9365bec/go.mod h1:WLkZH98faNiIzeIIjCH9zaa41JOPXar+afoD7uUsRys=
+github.com/openconfig/gribigo v0.0.0-20231018021145-a1124db3ca83 h1:4H2eDRDWGYvbX8+cfHiEnB4tu0jhjZql7OSfLTXRdFY=
+github.com/openconfig/gribigo v0.0.0-20231018021145-a1124db3ca83/go.mod h1:D/+hEFq2bJzbn/T6WVjQfsIHMfKNOp24Yfg/VZc3nwI=
 github.com/openconfig/grpctunnel v0.0.0-20220819142823-6f5422b8ca70 h1:t6SvvdfWCMlw0XPlsdxO8EgO+q/fXnTevDjdYREKFwU=
 github.com/openconfig/kne v0.1.14 h1:3xHy2bP+rr+2/2uFqliWXGjMPR7umO6mvFXh/TA2aJE=
 github.com/openconfig/kne v0.1.14/go.mod h1:gMhrUKk6aveDaLSo2yi/25tDm9pSlmgRkG8IP45CGqs=

diff --git a/gribi/gribi.go b/gribi/gribi.go
@@ -116,25 +116,29 @@ func createGRIBIServer(gClient gpb.GNMIClient, target string, root *oc.Root) (*s
 		if aft != constants.IPv4 || !ok {
 			log.Errorf("Incompatible type of route receive, type: %s, key: %v", aft, key)
 		}
-		if optype != constants.Add {
-			// TODO(wenbli): handle replace and delete :-)
-			// For replace, just need to ensure Sysrib's gRPC supports it.
-			return
-		}
-		nhs, err := afthelper.NextHopAddrsForPrefix(ribs, netinst, prefix)
-		if err != nil {
-			log.Errorf("cannot add netinst:prefix %s:%s to the RIB, %v", netinst, prefix, err)
-			return
-		}
 		nhSum := []*afthelper.NextHopSummary{}
-		for _, nh := range nhs {
-			nhSum = append(nhSum, nh)
+		switch optype {
+		case constants.Add, constants.Replace:
+			nhs, err := afthelper.NextHopAddrsForPrefix(ribs, netinst, prefix)
+			if err != nil {
+				log.Errorf("cannot add netinst:prefix %s:%s to the RIB, %v", netinst, prefix, err)
+				return
+			}
+			for _, nh := range nhs {
+				nhSum = append(nhSum, nh)
+			}
+		case constants.Delete:
+		default:
+			return
 		}
 
 		routeReq, err := createSetRouteRequest(prefix, nhSum)
 		if err != nil {
 			log.Errorf("Cannot create SetRouteRequest: %v", err)
 		}
+		if optype == constants.Delete {
+			routeReq.Delete = true
+		}
 
 		resp, err := gzebraClient.SetRoute(context.Background(), routeReq)
 		if err != nil {

diff --git a/integration_tests/onedut_oneotg_tests/traffic_test/BUILD b/integration_tests/onedut_oneotg_tests/traffic_test/BUILD
@@ -21,5 +21,6 @@ go_test(
         "@com_github_openconfig_ondatra//gnmi",
         "@com_github_openconfig_ondatra//gnmi/otg/otgpath",
         "@com_github_openconfig_ygnmi//ygnmi",
+        "@org_golang_x_exp//slices",
     ],
 )