From 8583450e10db6dd9989b1d0e9c1c94f4b5dcf6af Mon Sep 17 00:00:00 2001 From: Rob Shakir Date: Thu, 26 Oct 2023 20:49:34 -0700 Subject: [PATCH 1/3] Add testing for reconcile function. * (M) rib/reconciler/*.go - Refactor to change return type to ReconcileOps that can be fed to another function that owns the client. - Make Ops publicly accessible. - Add unit testing for Reconcile function. - Add tests against a hanging gRIBI server. --- go.mod | 2 +- go.sum | 4 +- rib/reconciler/reconcile.go | 114 +++++---- rib/reconciler/reconcile_test.go | 409 +++++++++++++++++++++++++++---- rib/reconciler/remote_test.go | 31 +++ 5 files changed, 461 insertions(+), 99 deletions(-) diff --git a/go.mod b/go.mod index b2a13121..3c37f7d5 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ 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/v2 v2.100.1 lukechampine.com/uint128 v1.2.0 ) @@ -59,5 +60,4 @@ require ( golang.org/x/text v0.13.0 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/klog/v2 v2.90.1 // indirect ) diff --git a/go.sum b/go.sum index e707d59a..813cd26f 100644 --- a/go.sum +++ b/go.sum @@ -605,8 +605,8 @@ 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/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw= -k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= +k8s.io/klog/v2 v2.100.1 h1:7WCHKK6K8fNhTqfBhISHQ97KrnJNFZMcQvKp7gP/tmg= +k8s.io/klog/v2 v2.100.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= lukechampine.com/uint128 v1.2.0 h1:mBi/5l91vocEN8otkC5bDLhi2KdCticRiwbdB0O+rjI= lukechampine.com/uint128 v1.2.0/go.mod h1:c4eWIwlEGaxC/+H1VguhU4PHXNWDCDMUlWdIWl2j1gk= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= diff --git a/rib/reconciler/reconcile.go b/rib/reconciler/reconcile.go index 63c7f995..3f58d715 100644 --- a/rib/reconciler/reconcile.go +++ b/rib/reconciler/reconcile.go @@ -27,6 +27,7 @@ import ( "context" "fmt" "reflect" + "sync/atomic" "github.com/openconfig/gribigo/aft" "github.com/openconfig/gribigo/rib" @@ -38,13 +39,6 @@ type R struct { // intended and target are the mechanisms by which to access the intended // RIB (source of truth) and the target it is to be reconciled with. intended, target RIBTarget - - // intended is a RIB containing the AFT entries that are intended to be - // programmed by the reconciler. - lastIntended *rib.RIB - // lastTarget is a cache of the last RIB entries that were returned from - // the target RIB. - lastTarget *rib.RIB } // RIBTarget is an interface that abstracts a local and remote RIB in the @@ -65,6 +59,11 @@ type LocalRIB struct { r *rib.RIB } +// NewLocalRIB returns a new LocalRIB instance. +func NewLocalRIB(r *rib.RIB) *LocalRIB { + return &LocalRIB{r: r} +} + // Get returns the contents of the local RIB. func (l *LocalRIB) Get(_ context.Context) (*rib.RIB, error) { return l.r, nil @@ -86,39 +85,47 @@ func New(intended, target RIBTarget) *R { } } -// Reconcile performs a reconciliation operation between the intended and specified -// remote RIB. -func (r *R) Reconcile(ctx context.Context) error { +// Reconcile calculates the required gRIBI actions to institute a reconciliation +// operation between the intended and remote RIB. The specified ID is used as the +// base for the operation ID within gRIBI. Reconcile returns a set of operations +// in the form of a ReconcileOps struct. +// +// Within the returned ReconcileOps: +// - The Add field indicates entries that are to be newly added to the remote RIB. +// - The Replace field indicates entries that are replacing entries within the remote RIB, +// the replaces will be implicit (i.e., expressed as gRIBI ADD operations) unless the +// ExplicitReplace option is provided. +// - The Delete field indicates entries that are to be deleted from the remote RIB. +// +// Within each of these fields, entries are broken down into "top-level" entries which are from +// the IPv4, IPv6, or MPLS AFTs, and those that correspond to next-hop-group (NHG) or next-hop (NH) +// entries. This allows the client receiving these entries to enqueue them in the correct order +// to ensure that there are no forward references, and implement make-before-break. +func (r *R) Reconcile(ctx context.Context, id *atomic.Uint64) (*ReconcileOps, error) { // Get the current contents of intended and target. iRIB, err := r.intended.Get(ctx) if err != nil { - return fmt.Errorf("cannot reconcile RIBs, cannot get contents of intended, %v", err) + return nil, fmt.Errorf("cannot reconcile RIBs, cannot get contents of intended, %v", err) } tRIB, err := r.target.Get(ctx) if err != nil { - return fmt.Errorf("cannot reconcile RIBs, cannot get contents of target, %v", err) + return nil, fmt.Errorf("cannot reconcile RIBs, cannot get contents of target, %v", err) } // Perform diff on their contents. // TODO(robjs): Plumb through explicitReplace map. - diffs, err := diff(iRIB, tRIB, nil) + diffs, err := diff(iRIB, tRIB, nil, id) if err != nil { - return fmt.Errorf("cannot reconcile RIBs, cannot calculate diff, %v", err) + return nil, fmt.Errorf("cannot reconcile RIBs, cannot calculate diff, %v", err) } - _ = diffs - _, _ = r.lastIntended, r.lastTarget - - // Enqueue the operations towards target that bring it in-line with intended. - // TODO(robjs): Implement enqueuing in client. - return fmt.Errorf("reconciliation unimplemented") - + return diffs, nil } -// ops stores a set of operations with their corresponding types. Operations +// 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 { +// 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. @@ -127,24 +134,24 @@ type ops struct { TopLevel []*spb.AFTOperation } -// reconcile ops stores the operations that are required for a specific reconciliation +// ReconcileOps stores the operations that are required for a specific reconciliation // run. -type reconcileOps struct { +type ReconcileOps struct { // Add stores the operations that are explicitly adding new entries. - Add *ops + Add *Ops // Replace stores the operations that are implicit or explicit replaces of // existing entries. - Replace *ops + Replace *Ops // Delete stores the operations that are removing entries. - Delete *ops + Delete *Ops } -// newReconcileOps returns a new reconcileOps struct with the fields initialised. -func newReconcileOps() *reconcileOps { - return &reconcileOps{ - Add: &ops{}, - Replace: &ops{}, - Delete: &ops{}, +// NewReconcileOps returns a new reconcileOps struct with the fields initialised. +func NewReconcileOps() *ReconcileOps { + return &ReconcileOps{ + Add: &Ops{}, + Replace: &Ops{}, + Delete: &Ops{}, } } @@ -161,7 +168,7 @@ func newReconcileOps() *reconcileOps { // // 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) (*reconcileOps, error) { +func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Uint64) (*ReconcileOps, error) { if src == nil || dst == nil { return nil, fmt.Errorf("invalid nil input RIBs, src: %v, dst: %v", src, dst) } @@ -185,9 +192,8 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp return nil, fmt.Errorf("cannot copy destination RIB contents, err: %v", err) } - ops := newReconcileOps() + ops := NewReconcileOps() - var id uint64 for srcNI, srcNIEntries := range srcContents { dstNIEntries, ok := dstContents[srcNI] if !ok { @@ -205,7 +211,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp if ok && explicitReplace[spb.AFTType_IPV4] { opType = spb.AFTOperation_REPLACE } - id++ + id.Add(1) op, err := v4Operation(opType, srcNI, pfx, id, srcE) if err != nil { return nil, err @@ -227,7 +233,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp if ok && explicitReplace[spb.AFTType_MPLS] { opType = spb.AFTOperation_REPLACE } - id++ + id.Add(1) op, err := mplsOperation(opType, srcNI, lbl, id, srcE) if err != nil { return nil, err @@ -249,7 +255,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp if ok && explicitReplace[spb.AFTType_NEXTHOP_GROUP] { opType = spb.AFTOperation_REPLACE } - id++ + id.Add(1) op, err := nhgOperation(opType, srcNI, nhgID, id, srcE) if err != nil { return nil, err @@ -271,7 +277,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp if ok && explicitReplace[spb.AFTType_NEXTHOP] { opType = spb.AFTOperation_REPLACE } - id++ + id.Add(1) op, err := nhOperation(opType, srcNI, nhID, id, srcE) if err != nil { return nil, err @@ -290,7 +296,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp // Delete operations. for pfx, dstE := range dstNIEntries.GetAfts().Ipv4Entry { if _, ok := srcNIEntries.GetAfts().Ipv4Entry[pfx]; !ok { - id++ + id.Add(1) op, err := v4Operation(spb.AFTOperation_DELETE, srcNI, pfx, id, dstE) if err != nil { return nil, err @@ -301,7 +307,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp for lbl, dstE := range dstNIEntries.GetAfts().LabelEntry { if _, ok := srcNIEntries.GetAfts().LabelEntry[lbl]; !ok { - id++ + id.Add(1) op, err := mplsOperation(spb.AFTOperation_DELETE, srcNI, lbl, id, dstE) if err != nil { return nil, err @@ -312,7 +318,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp for nhgID, dstE := range dstNIEntries.GetAfts().NextHopGroup { if _, ok := srcNIEntries.GetAfts().NextHopGroup[nhgID]; !ok { - id++ + id.Add(1) op, err := nhgOperation(spb.AFTOperation_DELETE, srcNI, nhgID, id, dstE) if err != nil { return nil, err @@ -323,7 +329,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp for nhID, dstE := range dstNIEntries.GetAfts().NextHop { if _, ok := srcNIEntries.GetAfts().NextHop[nhID]; !ok { - id++ + id.Add(1) op, err := nhOperation(spb.AFTOperation_DELETE, srcNI, nhID, id, dstE) if err != nil { return nil, err @@ -339,13 +345,13 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool) (*reconcileOp // v4Operation builds a gRIBI IPv4 operation with the specified method corresponding to the // prefix pfx in network instance ni, using the specified ID for the operation. The contents // of the operation are the entry e. -func v4Operation(method spb.AFTOperation_Operation, ni, pfx string, id uint64, e *aft.Afts_Ipv4Entry) (*spb.AFTOperation, error) { +func v4Operation(method spb.AFTOperation_Operation, ni, pfx string, id *atomic.Uint64, e *aft.Afts_Ipv4Entry) (*spb.AFTOperation, error) { p, err := rib.ConcreteIPv4Proto(e) if err != nil { return nil, fmt.Errorf("cannot create operation for prefix %s, %v", pfx, err) } return &spb.AFTOperation{ - Id: id, + Id: id.Load(), NetworkInstance: ni, Op: method, Entry: &spb.AFTOperation_Ipv4{ @@ -357,13 +363,13 @@ func v4Operation(method spb.AFTOperation_Operation, ni, pfx string, id uint64, e // nhgOperation builds a gRIBI NHG operation with the specified method, corresponding to the // NHG ID nhgID, in network instance ni, using the specified ID for the operation. The // contents of the operation are the entry e. -func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID, id uint64, e *aft.Afts_NextHopGroup) (*spb.AFTOperation, error) { +func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID uint64, id *atomic.Uint64, e *aft.Afts_NextHopGroup) (*spb.AFTOperation, error) { p, err := rib.ConcreteNextHopGroupProto(e) if err != nil { return nil, fmt.Errorf("cannot create operation for NHG %d, %v", nhgID, err) } return &spb.AFTOperation{ - Id: id, + Id: id.Load(), NetworkInstance: ni, Op: method, Entry: &spb.AFTOperation_NextHopGroup{ @@ -375,13 +381,13 @@ func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID, id uint64 // 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) { +func nhOperation(method spb.AFTOperation_Operation, ni string, nhID uint64, id *atomic.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, + Id: id.Load(), NetworkInstance: ni, Op: method, Entry: &spb.AFTOperation_NextHop{ @@ -393,13 +399,13 @@ func nhOperation(method spb.AFTOperation_Operation, ni string, nhID, id uint64, // mplsOperation builds a gRIBI LabelEntry operation with the specified method corresponding to // the MPLS label entry lbl. The operation is targeted at network instance ni, and uses the specified // ID. The contents of the operation are the entry e. -func mplsOperation(method spb.AFTOperation_Operation, ni string, lbl aft.Afts_LabelEntry_Label_Union, id uint64, e *aft.Afts_LabelEntry) (*spb.AFTOperation, error) { +func mplsOperation(method spb.AFTOperation_Operation, ni string, lbl aft.Afts_LabelEntry_Label_Union, id *atomic.Uint64, e *aft.Afts_LabelEntry) (*spb.AFTOperation, error) { p, err := rib.ConcreteMPLSProto(e) if err != nil { return nil, fmt.Errorf("cannot create operation for label %d, %v", lbl, err) } return &spb.AFTOperation{ - Id: id, + Id: id.Load(), NetworkInstance: ni, Op: method, Entry: &spb.AFTOperation_Mpls{ diff --git a/rib/reconciler/reconcile_test.go b/rib/reconciler/reconcile_test.go index 32ce206a..8ccb8b93 100644 --- a/rib/reconciler/reconcile_test.go +++ b/rib/reconciler/reconcile_test.go @@ -2,7 +2,9 @@ package reconciler import ( "context" + "sync/atomic" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -44,7 +46,7 @@ func TestDiff(t *testing.T) { inSrc *rib.RIB inDst *rib.RIB inExplicitReplace map[spb.AFTType]bool - wantOps *reconcileOps + wantOps *ReconcileOps wantErr bool }{{ desc: "VRF NI in src, but not in dst", @@ -66,8 +68,8 @@ func TestDiff(t *testing.T) { return r.RIB() }(), inDst: rib.NewFake(dn).RIB(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ NH: []*spb.AFTOperation{{ Id: 3, NetworkInstance: "FOO", @@ -144,8 +146,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -160,7 +162,7 @@ func TestDiff(t *testing.T) { }, }}, }, - Delete: &ops{ + Delete: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 2, NetworkInstance: dn, @@ -207,8 +209,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ NHG: []*spb.AFTOperation{{ Id: 2, NetworkInstance: dn, @@ -228,7 +230,7 @@ func TestDiff(t *testing.T) { }, }}, }, - Replace: &ops{ + Replace: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -278,8 +280,8 @@ func TestDiff(t *testing.T) { inExplicitReplace: map[spb.AFTType]bool{ spb.AFTType_IPV4: true, }, - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ NHG: []*spb.AFTOperation{{ Id: 2, NetworkInstance: dn, @@ -299,7 +301,7 @@ func TestDiff(t *testing.T) { }, }}, }, - Replace: &ops{ + Replace: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -334,8 +336,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ NHG: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -375,8 +377,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Delete: &ops{ + wantOps: &ReconcileOps{ + Delete: &Ops{ NHG: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -428,8 +430,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ NHG: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -489,8 +491,8 @@ func TestDiff(t *testing.T) { inExplicitReplace: map[spb.AFTType]bool{ spb.AFTType_NEXTHOP_GROUP: true, }, - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ NHG: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -535,8 +537,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ NH: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -573,8 +575,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Delete: &ops{ + wantOps: &ReconcileOps{ + Delete: &Ops{ NH: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -608,8 +610,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ NH: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -646,8 +648,8 @@ func TestDiff(t *testing.T) { inExplicitReplace: map[spb.AFTType]bool{ spb.AFTType_NEXTHOP: true, }, - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ NH: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -693,8 +695,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Add: &ops{ + wantOps: &ReconcileOps{ + Add: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -737,8 +739,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Delete: &ops{ + wantOps: &ReconcileOps{ + Delete: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -790,8 +792,8 @@ func TestDiff(t *testing.T) { } return r.RIB() }(), - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -846,8 +848,8 @@ func TestDiff(t *testing.T) { inExplicitReplace: map[spb.AFTType]bool{ spb.AFTType_MPLS: true, }, - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -909,8 +911,8 @@ func TestDiff(t *testing.T) { inExplicitReplace: map[spb.AFTType]bool{ spb.AFTType_ALL: true, }, - wantOps: &reconcileOps{ - Replace: &ops{ + wantOps: &ReconcileOps{ + Replace: &Ops{ TopLevel: []*spb.AFTOperation{{ Id: 1, NetworkInstance: dn, @@ -977,7 +979,8 @@ func TestDiff(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - got, err := diff(tt.inSrc, tt.inDst, tt.inExplicitReplace) + id := &atomic.Uint64{} + got, err := diff(tt.inSrc, tt.inDst, tt.inExplicitReplace, id) if (err != nil) != tt.wantErr { t.Fatalf("did not get expected error, got: %v, wantErr? %v", err, tt.wantErr) } @@ -989,13 +992,13 @@ func TestDiff(t *testing.T) { // Keep the test input as pithy as possible whilst ensuring safe adds // in the real implementation. if tt.wantOps.Add == nil { - tt.wantOps.Add = &ops{} + tt.wantOps.Add = &Ops{} } if tt.wantOps.Replace == nil { - tt.wantOps.Replace = &ops{} + tt.wantOps.Replace = &Ops{} } if tt.wantOps.Delete == nil { - tt.wantOps.Delete = &ops{} + tt.wantOps.Delete = &Ops{} } if diff := cmp.Diff(got, tt.wantOps, @@ -1008,3 +1011,325 @@ func TestDiff(t *testing.T) { }) } } + +func TestReconcile(t *testing.T) { + dn := "DEFAULT" + tests := []struct { + desc string + inIntended RIBTarget + inTarget RIBTarget + inID *atomic.Uint64 + wantOps *ReconcileOps + wantErr bool + }{{ + desc: "local-local: one entry in intended, not in target", + inIntended: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectIPv4(dn, "1.0.0.0/24", 1); err != nil { + t.Fatalf("cannot add prefix to intended, %v", err) + } + return NewLocalRIB(r.RIB()) + }(), + inTarget: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + return NewLocalRIB(r.RIB()) + }(), + inID: &atomic.Uint64{}, + wantOps: &ReconcileOps{ + Add: &Ops{ + TopLevel: []*spb.AFTOperation{{ + Id: 1, + NetworkInstance: dn, + Op: spb.AFTOperation_ADD, + Entry: &spb.AFTOperation_Ipv4{ + Ipv4: &aftpb.Afts_Ipv4EntryKey{ + Prefix: "1.0.0.0/24", + Ipv4Entry: &aftpb.Afts_Ipv4Entry{ + NextHopGroup: &wpb.UintValue{Value: 1}, + }, + }, + }, + }}, + }, + Replace: &Ops{}, + Delete: &Ops{}, + }, + }, { + desc: "local-local: no differences between intended and target", + inIntended: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectNHG(dn, 100, map[uint64]uint64{ + 1: 10, + 2: 20, + 3: 30, + }); err != nil { + t.Fatalf("cannot add NHG, err: %v", err) + } + return NewLocalRIB(r.RIB()) + }(), + inTarget: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectNHG(dn, 100, map[uint64]uint64{ + 1: 10, + 2: 20, + 3: 30, + }); err != nil { + t.Fatalf("cannot add NHG, err: %v", err) + } + return NewLocalRIB(r.RIB()) + }(), + inID: &atomic.Uint64{}, + wantOps: &ReconcileOps{ + Add: &Ops{}, + Replace: &Ops{}, + Delete: &Ops{}, + }, + }, { + desc: "local-local: entry in target that is not in intended", + inIntended: NewLocalRIB(rib.NewFake(dn, rib.DisableRIBCheckFn()).RIB()), + inTarget: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectNH(dn, 1, "eth0"); err != nil { + t.Fatalf("cannot add NH, err: %v", err) + } + return NewLocalRIB(r.RIB()) + }(), + inID: &atomic.Uint64{}, + wantOps: &ReconcileOps{ + Add: &Ops{}, + Replace: &Ops{}, + Delete: &Ops{ + NH: []*spb.AFTOperation{{ + Id: 1, + NetworkInstance: dn, + Op: spb.AFTOperation_DELETE, + Entry: &spb.AFTOperation_NextHop{ + NextHop: &aftpb.Afts_NextHopKey{ + Index: 1, + NextHop: &aftpb.Afts_NextHop{ + InterfaceRef: &aftpb.Afts_NextHop_InterfaceRef{ + Interface: &wpb.StringValue{Value: "eth0"}, + }, + }, + }, + }, + }}, + }, + }, + }, { + desc: "local-local: non-zero starting ID", + inIntended: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectIPv4(dn, "1.0.0.0/24", 1); err != nil { + t.Fatalf("cannot add prefix to intended, %v", err) + } + return NewLocalRIB(r.RIB()) + }(), + inTarget: func() RIBTarget { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + return NewLocalRIB(r.RIB()) + }(), + inID: func() *atomic.Uint64 { + u := &atomic.Uint64{} + u.Store(42) + return u + }(), + wantOps: &ReconcileOps{ + Add: &Ops{ + TopLevel: []*spb.AFTOperation{{ + Id: 43, + NetworkInstance: dn, + Op: spb.AFTOperation_ADD, + Entry: &spb.AFTOperation_Ipv4{ + Ipv4: &aftpb.Afts_Ipv4EntryKey{ + Prefix: "1.0.0.0/24", + Ipv4Entry: &aftpb.Afts_Ipv4Entry{ + NextHopGroup: &wpb.UintValue{Value: 1}, + }, + }, + }, + }}, + }, + Replace: &Ops{}, + Delete: &Ops{}, + }, + }} + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + rr := New(tt.inIntended, tt.inTarget) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + got, err := rr.Reconcile(ctx, tt.inID) + + if (err != nil) != tt.wantErr { + t.Fatalf("did not get expected error, got: %v, wantErr? %v", err, tt.wantErr) + } + + if diff := cmp.Diff(got, tt.wantOps, + protocmp.Transform(), + cmpopts.EquateEmpty(), + ); diff != "" { + t.Fatalf("did not get expected RIB, diff(-got,+want):\n%s", diff) + } + }) + } +} + +func TestReconcileRemote(t *testing.T) { + dn := "DEFAULT" + tests := []struct { + desc string + inIntendedServer func(*testing.T, *rib.RIB) (string, func()) + inTargetServer func(*testing.T, *rib.RIB) (string, func()) + inInjectedIntendedRIB *rib.RIB + inInjectedRemoteRIB *rib.RIB + inID *atomic.Uint64 + wantOps *ReconcileOps + wantErr bool + }{{ + desc: "no routes to add", + inIntendedServer: newServer, + inTargetServer: newServer, + inInjectedIntendedRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inInjectedRemoteRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inID: &atomic.Uint64{}, + wantOps: &ReconcileOps{ + Add: &Ops{}, + Delete: &Ops{}, + Replace: &Ops{}, + }, + }, { + desc: "route to add from intended to target", + inIntendedServer: newServer, + inTargetServer: newServer, + inInjectedIntendedRIB: func() *rib.RIB { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + for i, a := range []string{"84.18.210.139/32", "142.250.72.174/32"} { + if err := r.InjectIPv4(dn, a, uint64(i+1)); err != nil { + t.Fatalf("cannot inject %s to intended, %v", a, err) + } + } + return r.RIB() + }(), + inInjectedRemoteRIB: func() *rib.RIB { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectIPv4(dn, "84.18.210.139/32", 1); err != nil { + t.Fatalf("cannot inject IPV4 prefix to intended, %v", err) + } + return r.RIB() + }(), + inID: func() *atomic.Uint64 { + u := &atomic.Uint64{} + u.Store(41) + return u + }(), + wantOps: &ReconcileOps{ + Add: &Ops{ + TopLevel: []*spb.AFTOperation{{ + Id: 42, + NetworkInstance: dn, + Op: spb.AFTOperation_ADD, + Entry: &spb.AFTOperation_Ipv4{ + Ipv4: &aftpb.Afts_Ipv4EntryKey{ + Prefix: "142.250.72.174/32", + Ipv4Entry: &aftpb.Afts_Ipv4Entry{ + NextHopGroup: &wpb.UintValue{Value: 2}, + }, + }, + }, + }}, + }, + Replace: &Ops{}, + Delete: &Ops{}, + }, + }, { + desc: "delete from target", + inIntendedServer: newServer, + inTargetServer: newServer, + inInjectedIntendedRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inInjectedRemoteRIB: func() *rib.RIB { + r := rib.NewFake(dn, rib.DisableRIBCheckFn()) + if err := r.InjectIPv4(dn, "84.18.210.139/32", 1); err != nil { + t.Fatalf("cannot inject IPV4 prefix to intended, %v", err) + } + return r.RIB() + }(), + inID: func() *atomic.Uint64 { + u := &atomic.Uint64{} + u.Store(41) + return u + }(), + wantOps: &ReconcileOps{ + Add: &Ops{}, + Replace: &Ops{}, + Delete: &Ops{ + TopLevel: []*spb.AFTOperation{{ + Id: 42, + NetworkInstance: dn, + Op: spb.AFTOperation_DELETE, + Entry: &spb.AFTOperation_Ipv4{ + Ipv4: &aftpb.Afts_Ipv4EntryKey{ + Prefix: "84.18.210.139/32", + Ipv4Entry: &aftpb.Afts_Ipv4Entry{ + NextHopGroup: &wpb.UintValue{Value: 1}, + }, + }, + }, + }}, + }, + }, + }, { + desc: "badly behaving target", + inIntendedServer: newServer, + inTargetServer: newBadServer, + inInjectedIntendedRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inInjectedRemoteRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inID: &atomic.Uint64{}, + wantErr: true, + }, { + desc: "sleepy intended server", + inIntendedServer: newHangingServer, + inTargetServer: newServer, + inInjectedIntendedRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inInjectedRemoteRIB: rib.New(dn, rib.DisableRIBCheckFn()), + inID: &atomic.Uint64{}, + wantErr: true, + }} + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + intendedAddr, intendedStop := tt.inIntendedServer(t, tt.inInjectedIntendedRIB) + defer intendedStop() + + targetAddr, targetStop := tt.inTargetServer(t, tt.inInjectedRemoteRIB) + defer targetStop() + + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + intendedTarget, err := NewRemoteRIB(ctx, dn, intendedAddr) + if err != nil { + t.Fatalf("cannot create intended RIBTarget, %v", err) + } + + targetTarget, err := NewRemoteRIB(ctx, dn, targetAddr) + if err != nil { + t.Fatalf("cannot create target RIBTarget, %v", err) + } + + rr := New(intendedTarget, targetTarget) + + got, err := rr.Reconcile(ctx, tt.inID) + if (err != nil) != tt.wantErr { + t.Fatalf("(reconciler.Reconcile()): cannot calculate difference, got unexpected err: %v", err) + } + + if diff := cmp.Diff(got, tt.wantOps, protocmp.Transform()); diff != "" { + t.Fatalf("reconciler.Reconcile(): did not get expected ops, diff(-got,+want):\n%s", diff) + } + }) + } +} diff --git a/rib/reconciler/remote_test.go b/rib/reconciler/remote_test.go index c3efb5c3..1feb8e04 100644 --- a/rib/reconciler/remote_test.go +++ b/rib/reconciler/remote_test.go @@ -101,7 +101,33 @@ func newBadServer(t *testing.T, r *rib.RIB) (string, func()) { go srv.Serve(l) return l.Addr().String(), srv.Stop +} + +type hangingGRIBI struct { + *spb.UnimplementedGRIBIServer +} +func (h *hangingGRIBI) Get(_ *spb.GetRequest, _ spb.GRIBI_GetServer) error { + time.Sleep(600 * time.Second) + return nil +} + +func newHangingServer(t *testing.T, r *rib.RIB) (string, func()) { + creds, err := credentials.NewServerTLSFromFile(testcommon.TLSCreds()) + if err != nil { + t.Fatalf("cannot load TLS credentials, got err: %v", err) + } + srv := grpc.NewServer(grpc.Creds(creds)) + s := &hangingGRIBI{} + + l, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("cannot create listener, got err: %v", err) + } + spb.RegisterGRIBIServer(srv, s) + + go srv.Serve(l) + return l.Addr().String(), srv.Stop } func TestGet(t *testing.T) { @@ -145,6 +171,11 @@ func TestGet(t *testing.T) { return r }(), }, + }, { + desc: "hanging gRIBI", + inDefName: dn, + inServer: newHangingServer, + wantErr: true, }} for _, tt := range tests { From 6f64549a3898be317ae79e7244c963f5b60fa702 Mon Sep 17 00:00:00 2001 From: Rob Shakir Date: Thu, 26 Oct 2023 21:46:31 -0700 Subject: [PATCH 2/3] Tidy go.mod. --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 3c37f7d5..df56d0d3 100644 --- a/go.mod +++ b/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/v2 v2.100.1 lukechampine.com/uint128 v1.2.0 ) @@ -60,4 +59,5 @@ require ( golang.org/x/text v0.13.0 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/klog/v2 v2.100.1 // indirect ) From 505a70cd856ffee9a49ddd2acedee45516aacc47 Mon Sep 17 00:00:00 2001 From: Rob Shakir Date: Mon, 30 Oct 2023 17:48:58 -0700 Subject: [PATCH 3/3] Remove additional argument. --- rib/reconciler/reconcile.go | 44 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/rib/reconciler/reconcile.go b/rib/reconciler/reconcile.go index 3f58d715..cbcac6ee 100644 --- a/rib/reconciler/reconcile.go +++ b/rib/reconciler/reconcile.go @@ -212,7 +212,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui opType = spb.AFTOperation_REPLACE } id.Add(1) - op, err := v4Operation(opType, srcNI, pfx, id, srcE) + op, err := v4Operation(opType, srcNI, id, srcE) if err != nil { return nil, err } @@ -234,7 +234,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui opType = spb.AFTOperation_REPLACE } id.Add(1) - op, err := mplsOperation(opType, srcNI, lbl, id, srcE) + op, err := mplsOperation(opType, srcNI, id, srcE) if err != nil { return nil, err } @@ -256,7 +256,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui opType = spb.AFTOperation_REPLACE } id.Add(1) - op, err := nhgOperation(opType, srcNI, nhgID, id, srcE) + op, err := nhgOperation(opType, srcNI, id, srcE) if err != nil { return nil, err } @@ -278,7 +278,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui opType = spb.AFTOperation_REPLACE } id.Add(1) - op, err := nhOperation(opType, srcNI, nhID, id, srcE) + op, err := nhOperation(opType, srcNI, id, srcE) if err != nil { return nil, err } @@ -297,7 +297,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui for pfx, dstE := range dstNIEntries.GetAfts().Ipv4Entry { if _, ok := srcNIEntries.GetAfts().Ipv4Entry[pfx]; !ok { id.Add(1) - op, err := v4Operation(spb.AFTOperation_DELETE, srcNI, pfx, id, dstE) + op, err := v4Operation(spb.AFTOperation_DELETE, srcNI, id, dstE) if err != nil { return nil, err } @@ -308,7 +308,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui for lbl, dstE := range dstNIEntries.GetAfts().LabelEntry { if _, ok := srcNIEntries.GetAfts().LabelEntry[lbl]; !ok { id.Add(1) - op, err := mplsOperation(spb.AFTOperation_DELETE, srcNI, lbl, id, dstE) + op, err := mplsOperation(spb.AFTOperation_DELETE, srcNI, id, dstE) if err != nil { return nil, err } @@ -319,7 +319,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui for nhgID, dstE := range dstNIEntries.GetAfts().NextHopGroup { if _, ok := srcNIEntries.GetAfts().NextHopGroup[nhgID]; !ok { id.Add(1) - op, err := nhgOperation(spb.AFTOperation_DELETE, srcNI, nhgID, id, dstE) + op, err := nhgOperation(spb.AFTOperation_DELETE, srcNI, id, dstE) if err != nil { return nil, err } @@ -330,7 +330,7 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui for nhID, dstE := range dstNIEntries.GetAfts().NextHop { if _, ok := srcNIEntries.GetAfts().NextHop[nhID]; !ok { id.Add(1) - op, err := nhOperation(spb.AFTOperation_DELETE, srcNI, nhID, id, dstE) + op, err := nhOperation(spb.AFTOperation_DELETE, srcNI, id, dstE) if err != nil { return nil, err } @@ -342,13 +342,13 @@ func diff(src, dst *rib.RIB, explicitReplace map[spb.AFTType]bool, id *atomic.Ui return ops, nil } -// v4Operation builds a gRIBI IPv4 operation with the specified method corresponding to the -// prefix pfx in network instance ni, using the specified ID for the operation. The contents +// v4Operation builds a gRIBI IPv4 operation with the specified method corresponding to a +// prefix in the network instance ni, using the specified ID for the operation. The contents // of the operation are the entry e. -func v4Operation(method spb.AFTOperation_Operation, ni, pfx string, id *atomic.Uint64, e *aft.Afts_Ipv4Entry) (*spb.AFTOperation, error) { +func v4Operation(method spb.AFTOperation_Operation, ni string, id *atomic.Uint64, e *aft.Afts_Ipv4Entry) (*spb.AFTOperation, error) { p, err := rib.ConcreteIPv4Proto(e) if err != nil { - return nil, fmt.Errorf("cannot create operation for prefix %s, %v", pfx, err) + return nil, fmt.Errorf("cannot create operation for prefix %s, %v", e.GetPrefix(), err) } return &spb.AFTOperation{ Id: id.Load(), @@ -360,13 +360,13 @@ func v4Operation(method spb.AFTOperation_Operation, ni, pfx string, id *atomic.U }, nil } -// nhgOperation builds a gRIBI NHG operation with the specified method, corresponding to the -// NHG ID nhgID, in network instance ni, using the specified ID for the operation. The +// nhgOperation builds a gRIBI NHG operation with the specified method, corresponding to a +// NHG in network instance ni, using the specified ID for the operation. The // contents of the operation are the entry e. -func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID uint64, id *atomic.Uint64, e *aft.Afts_NextHopGroup) (*spb.AFTOperation, error) { +func nhgOperation(method spb.AFTOperation_Operation, ni string, id *atomic.Uint64, e *aft.Afts_NextHopGroup) (*spb.AFTOperation, error) { p, err := rib.ConcreteNextHopGroupProto(e) if err != nil { - return nil, fmt.Errorf("cannot create operation for NHG %d, %v", nhgID, err) + return nil, fmt.Errorf("cannot create operation for NHG %d, %v", e.GetId(), err) } return &spb.AFTOperation{ Id: id.Load(), @@ -378,13 +378,13 @@ func nhgOperation(method spb.AFTOperation_Operation, ni string, nhgID uint64, id }, 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 +// nhOperation builds a gRIBI NH operation with the specified method, corresponding to a +// NH 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 uint64, id *atomic.Uint64, e *aft.Afts_NextHop) (*spb.AFTOperation, error) { +func nhOperation(method spb.AFTOperation_Operation, ni string, id *atomic.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 nil, fmt.Errorf("cannot create operation for NH %d, %v", e.GetIndex(), err) } return &spb.AFTOperation{ Id: id.Load(), @@ -399,10 +399,10 @@ func nhOperation(method spb.AFTOperation_Operation, ni string, nhID uint64, id * // mplsOperation builds a gRIBI LabelEntry operation with the specified method corresponding to // the MPLS label entry lbl. The operation is targeted at network instance ni, and uses the specified // ID. The contents of the operation are the entry e. -func mplsOperation(method spb.AFTOperation_Operation, ni string, lbl aft.Afts_LabelEntry_Label_Union, id *atomic.Uint64, e *aft.Afts_LabelEntry) (*spb.AFTOperation, error) { +func mplsOperation(method spb.AFTOperation_Operation, ni string, id *atomic.Uint64, e *aft.Afts_LabelEntry) (*spb.AFTOperation, error) { p, err := rib.ConcreteMPLSProto(e) if err != nil { - return nil, fmt.Errorf("cannot create operation for label %d, %v", lbl, err) + return nil, fmt.Errorf("cannot create operation for label %d, %v", e.GetLabel(), err) } return &spb.AFTOperation{ Id: id.Load(),