Add support for next-hop reconcilation and refactor return values.

  * (M) rib/helpers.go
  * (M) rib/helpers_test.go
    - Allow NH details to be injected into the fake RIB.
  * (M) rib/reconciler/reconcile.go
  * (M) rib/reconciler/reconcile_test.go
    - Restructure return values to allow a caller to stage their
      operations into the gRIBI server - e.g., perform adds to install
      new entries, then replaces, then deletes. Keep types separate to
      allow for more robust ordering.
    - Add support for NH diffing.
  * (M) rib/reconciler/remote_test.go
    - Adopt new fake RIB helper method.

go.mod

@@ -16,7 +16,6 @@ require (
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d
 	google.golang.org/grpc v1.58.0-dev
 	google.golang.org/protobuf v1.31.0
-	k8s.io/klog v1.0.0
 	lukechampine.com/uint128 v1.2.0
 )
 

go.sum

@@ -79,7 +79,6 @@ github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeME
 github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU=
 github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
 github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
-github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas=
 github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
 github.com/go-logr/logr v1.2.3 h1:2DntVwHkVopvECVRSlL5PSo9eG+cAkDCuckLubN+rq0=
 github.com/go-logr/logr v1.2.3/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
@@ -606,8 +605,6 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
-k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
-k8s.io/klog v1.0.0/go.mod h1:4Bi6QPql/J/LkTDqv7R/cd3hPo4k2DG6Ptcz060Ez5I=
 k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw=
 k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
 lukechampine.com/uint128 v1.2.0 h1:mBi/5l91vocEN8otkC5bDLhi2KdCticRiwbdB0O+rjI=

rib/helpers.go

@@ -143,15 +143,19 @@ func (f *fakeRIB) InjectNHG(ni string, nhgId uint64, nhs map[uint64]uint64) erro
 
 // InjectNH adds a next-hop entry to network instance ni, with the specified
 // index (nhIdx). An error is returned if it cannot be added.
-func (f *fakeRIB) InjectNH(ni string, nhIdx uint64) error {
+func (f *fakeRIB) InjectNH(ni string, nhIdx uint64, intName string) error {
 	niR, ok := f.r.NetworkInstanceRIB(ni)
 	if !ok {
 		return fmt.Errorf("unknown NI, %s", ni)
 	}
 
 	if _, _, err := niR.AddNextHop(&aftpb.Afts_NextHopKey{
-		Index:   nhIdx,
-		NextHop: &aftpb.Afts_NextHop{},
+		Index: nhIdx,
+		NextHop: &aftpb.Afts_NextHop{
+			InterfaceRef: &aftpb.Afts_NextHop_InterfaceRef{
+				Interface: &wpb.StringValue{Value: intName},
+			},
+		},
 	}, false); err != nil {
 		return fmt.Errorf("cannot add NH entry, err: %v", err)
 	}

rib/helpers_test.go

@@ -267,7 +267,7 @@ func TestFakeRIB(t *testing.T) {
 		desc: "nh only",
 		inBuild: func() *fakeRIB {
 			f := NewFake(dn)
-			if err := f.InjectNH(dn, 1); err != nil {
+			if err := f.InjectNH(dn, 1, "int42"); err != nil {
 				t.Fatalf("cannot add NH, err: %v", err)
 			}
 			return f
@@ -280,7 +280,12 @@ func TestFakeRIB(t *testing.T) {
 					r: &aft.RIB{
 						Afts: &aft.Afts{
 							NextHop: map[uint64]*aft.Afts_NextHop{
-								1: {Index: ygot.Uint64(1)},
+								1: {
+									Index: ygot.Uint64(1),
+									InterfaceRef: &aft.Afts_NextHop_InterfaceRef{
+										Interface: ygot.String("int42"),
+									},
+								},
 							},
 						},
 					},
@@ -348,7 +353,7 @@ func TestFakeRIB(t *testing.T) {
 		inBuild: func() *fakeRIB {
 			f := NewFake(dn)
 			// Discard the errors, since the test will check whether the entries are there.
-			f.InjectNH(dn, 1)
+			f.InjectNH(dn, 1, "int42")
 			f.InjectNHG(dn, 1, map[uint64]uint64{1: 1})
 			f.InjectIPv4(dn, "192.0.2.1/32", 1)
 			return f
@@ -363,6 +368,9 @@ func TestFakeRIB(t *testing.T) {
 							NextHop: map[uint64]*aft.Afts_NextHop{
 								1: {
 									Index: ygot.Uint64(1),
+									InterfaceRef: &aft.Afts_NextHop_InterfaceRef{
+										Interface: ygot.String("int42"),
+									},
 								},
 							},
 							NextHopGroup: map[uint64]*aft.Afts_NextHopGroup{

rib/reconciler/reconcile.go

@@ -30,7 +30,6 @@ import (
 
 	"github.com/openconfig/gribigo/aft"
 	"github.com/openconfig/gribigo/rib"
-	"k8s.io/klog"
 
 	spb "github.com/openconfig/gribi/v1/proto/service"
 )
@@ -116,6 +115,39 @@ func (r *R) Reconcile(ctx context.Context) error {
 
 }
 
+// ops stores a set of operations with their corresponding types. Operations
+// are stored as NH (nexthop), NHG (next-hop-group) and top-level (MPLS, IPv4,
+// IPv6). This allows a gRIBI client to sequence the ops suitably.
+type ops struct {
+	// NH stores the next-hop operations in the operation set.
+	NH []*spb.AFTOperation
+	// NHG stores the next-hop-group operations in the operation set.
+	NHG []*spb.AFTOperation
+	// TopLevel stores the IPv4, IPv6, and MPLS operations in the operation set.
+	TopLevel []*spb.AFTOperation
+}
+
+// reconcile ops stores the operations that are required for a specific reconciliation
+// run.
+type reconcileOps struct {
+	// Add stores the operations that are explicitly adding new entries.
+	Add *ops
+	// Replace stores the operations that are implicit or explicit replaces of
+	// existing entries.
+	Replace *ops
+	// Delete stores the operations that are removing entries.
+	Delete *ops
+}
+
+// newReconcileOps returns a new reconcileOps struct with the fields initialised.
+func newReconcileOps() *reconcileOps {
+	return &reconcileOps{
+		Add:     &ops{},
+		Replace: &ops{},
+		Delete:  &ops{},
+	}
+}
+
 // diff returns the difference between the src and dst RIBs expressed as gRIBI
 // AFTOperations. That is to say, for each network instance RIB within the RIBs:
 //
@@ -129,7 +161,10 @@ func (r *R) Reconcile(ctx context.Context) error {
 //
 // If an entry within the explicitReplace map is set to true then explicit, rather
 // than implicit replaces are generated for that function.
-func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) ([]*spb.AFTOperation, error) {
+func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOps, error) {
+	if src == nil || dst == nil {
+		return nil, fmt.Errorf("invalid nil input RIBs, src: %v, dst: %v", src, dst)
+	}
 	srcContents, err := src.RIBContents()
 	if err != nil {
 		return nil, fmt.Errorf("cannot copy source RIB contents, err: %v", err)
@@ -139,37 +174,16 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) ([]*spb.AFTOp
 		return nil, fmt.Errorf("cannot copy destination RIB contents, err: %v", err)
 	}
 
-	// Store the "top-level" operations (i.e., IPv4, IPv6, MPLS) and then the NHG and NHs
-	// separately. This allows us to return the operations separately so that they can be
-	// ordered in terms of programming. NHs need to be installed/replaced before NHGs, and
-	// then subsequently top-level entries.
-	topLevelOps, nhgOps, nhOps := []*spb.AFTOperation{}, []*spb.AFTOperation{}, []*spb.AFTOperation{}
+	ops := newReconcileOps()
+
 	var id uint64
 	for srcNI, srcNIEntries := range srcContents {
 		dstNIEntries, ok := dstContents[srcNI]
 		if !ok {
-			// The network instance does not exist in the destination therefore
-			// all entries are ADDs.
-			for pfx, e := range srcNIEntries.GetAfts().Ipv4Entry {
-				id++
-				op, err := v4Operation(spb.AFTOperation_ADD, srcNI, pfx, id, e)
-				if err != nil {
-					return nil, err
-				}
-				topLevelOps = append(topLevelOps, op)
-			}
-
-			for nhgID, e := range srcNIEntries.GetAfts().NextHopGroup {
-				id++
-				op, err := nhgOperation(spb.AFTOperation_ADD, srcNI, nhgID, id, e)
-				if err != nil {
-					return nil, err
-				}
-				nhgOps = append(nhgOps, op)
-			}
-
-			continue
+			dstNIEntries = &aft.RIB{}
+			dstNIEntries.GetOrCreateAfts()
 		}
+
 		// For each AFT:
 		//  * if a key is present in src but not in dst -> generate an ADD
 		//  * if a key is present in src and in dst -> diff, and generate an ADD if the contents differ.
@@ -185,7 +199,14 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) ([]*spb.AFTOp
 				if err != nil {
 					return nil, err
 				}
-				topLevelOps = append(topLevelOps, op)
+
+				// If this entry already exists then this is an addition rather than a replace.
+				switch ok {
+				case true:
+					ops.Replace.TopLevel = append(ops.Replace.TopLevel, op)
+				case false:
+					ops.Add.TopLevel = append(ops.Add.TopLevel, op)
+				}
 			}
 		}
 
@@ -200,18 +221,48 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) ([]*spb.AFTOp
 				if err != nil {
 					return nil, err
 				}
-				nhgOps = append(nhgOps, op)
+
+				// If this entry already exists then this is an addition rather than a replace.
+				switch ok {
+				case true:
+					ops.Replace.NHG = append(ops.Replace.NHG, op)
+				case false:
+					ops.Add.NHG = append(ops.Add.NHG, op)
+				}
 			}
 		}
 
+		for nhID, srcE := range srcNIEntries.GetAfts().NextHop {
+			if dstE, ok := dstNIEntries.GetAfts().NextHop[nhID]; !ok || !reflect.DeepEqual(srcE, dstE) {
+				opType := spb.AFTOperation_ADD
+				if ok && explicitReplace[spb.AFTType_NEXTHOP] {
+					opType = spb.AFTOperation_REPLACE
+				}
+				id++
+				op, err := nhOperation(opType, srcNI, nhID, id, srcE)
+				if err != nil {
+					return nil, err
+				}
+
+				// If this entry already exists then this is an addition rather than a replace.
+				switch ok {
+				case true:
+					ops.Replace.NH = append(ops.Replace.NH, op)
+				case false:
+					ops.Add.NH = append(ops.Add.NH, op)
+				}
+			}
+		}
+
+		// Delete operations.
 		for pfx, dstE := range dstNIEntries.GetAfts().Ipv4Entry {
 			if _, ok := srcNIEntries.GetAfts().Ipv4Entry[pfx]; !ok {
 				id++
 				op, err := v4Operation(spb.AFTOperation_DELETE, srcNI, pfx, id, dstE)
 				if err != nil {
 					return nil, err
 				}
-				topLevelOps = append(topLevelOps, op)
+				ops.Delete.TopLevel = append(ops.Delete.TopLevel, op)
 			}
 		}
 
@@ -222,20 +273,22 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) ([]*spb.AFTOp
 				if err != nil {
 					return nil, err
 				}
-				nhgOps = append(nhgOps, op)
+				ops.Delete.NHG = append(ops.Delete.NHG, op)
 			}
 		}
 
-		if srcN, dstN := len(srcNIEntries.GetAfts().NextHop), len(dstNIEntries.GetAfts().NextHop); srcN != 0 || dstN != 0 {
-			// TODO(robjs): Implement mapping of NH entries.
-			klog.Warningf("next-hop reconcilation unimplemented, NHG entries, src: %d, dst: %d", srcN, dstN)
+		for nhID, dstE := range dstNIEntries.GetAfts().NextHop {
+			if _, ok := srcNIEntries.GetAfts().NextHop[nhID]; !ok {
+				id++
+				op, err := nhOperation(spb.AFTOperation_DELETE, srcNI, nhID, id, dstE)
+				if err != nil {
+					return nil, err
+				}
+				ops.Delete.NH = append(ops.Delete.NH, op)
+			}
 		}
 	}
 
-	ops := append([]*spb.AFTOperation{}, nhOps...)
-	ops = append(ops, nhgOps...)
-	ops = append(ops, topLevelOps...)
-
 	return ops, nil
 }
 
@@ -274,3 +327,21 @@ func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID, id uint64
 		},
 	}, nil
 }
+
+// nhOperation builds a gRIBI NH operation with the specified method, corresponding to the
+// NH ID nhID, in network instance ni, using the specified ID for the operation. The contents
+// of the operation are the entry e.
+func nhOperation(method spb.AFTOperation_Operation, ni string, nhID, id uint64, e *aft.Afts_NextHop) (*spb.AFTOperation, error) {
+	p, err := rib.ConcreteNextHopProto(e)
+	if err != nil {
+		return nil, fmt.Errorf("cannot create operation for NH %d, %v", nhID, err)
+	}
+	return &spb.AFTOperation{
+		Id:              id,
+		NetworkInstance: ni,
+		Op:              method,
+		Entry: &spb.AFTOperation_NextHop{
+			NextHop: p,
+		},
+	}, nil
+}