diff --git a/client/client_test.go b/client/client_test.go index 6cead359..dd245648 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1062,7 +1062,10 @@ func TestGet(t *testing.T) { t.Fatalf("cannot load TLS credentials, got err: %v", err) } srv := grpc.NewServer(grpc.Creds(creds)) - s := server.NewFake(server.DisableRIBCheckFn()) + s, err := server.NewFake(server.DisableRIBCheckFn()) + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.InjectRIB(nr) @@ -1314,9 +1317,12 @@ func TestFlush(t *testing.T) { t.Fatalf("cannot load TLS credentials, got err: %v", err) } srv := grpc.NewServer(grpc.Creds(creds)) - s := server.NewFake( + s, err := server.NewFake( server.DisableRIBCheckFn(), ) + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.InjectRIB(rib.New(server.DefaultNetworkInstanceName)) if tt.inRIB != nil { diff --git a/compliance/compliance.go b/compliance/compliance.go index d0a45a9e..f7e1f8a8 100644 --- a/compliance/compliance.go +++ b/compliance/compliance.go @@ -50,17 +50,30 @@ func SetElectionID(v uint64) { electionID.Store(v) } -// defaultNetworkInstance name is the string name of the default network instance -// on the server. It can be overridden by tests that have pushed a configuration -// to a server where they have specified a value for the default network instance. -var defaultNetworkInstanceName = server.DefaultNetworkInstanceName +var ( + // defaultNetworkInstance name is the string name of the default network instance + // on the server. It can be overridden by tests that have pushed a configuration + // to a server where they have specified a value for the default network instance. + defaultNetworkInstanceName = server.DefaultNetworkInstanceName + + // vrfName is a name of a non-default VRF that exists on the server. It can be + // overriden by tests that have pushed a configuration to the server where they + // have created a name that is not the specified string. + vrfName = "NON-DEFAULT-VRF" +) -// SetDefaultNetworkInstanceName allows an external caler to specify a network +// SetDefaultNetworkInstanceName allows an external caller to specify a network // instance name to be used for the default network instance. func SetDefaultNetworkInstanceName(n string) { defaultNetworkInstanceName = n } +// SetNonDefaultVRFName allows an external caller to specify a network-instance +// name to be used as a non-default Layer 3 VRF. +func SetNonDefaultVRFName(n string) { + vrfName = n +} + // Test describes a test within the compliance library. type Test struct { // Fn is the function to be run for a test. Tests must not error if additional @@ -596,15 +609,12 @@ func AddIPv4Metadata(c *fluent.GRIBIClient, t testing.TB, _ ...TestOpt) { // AddIPv4EntryDifferentNINHG adds an IPv4 entry that references a next-hop-group within a // different network instance, and validates that the entry is successfully installed. func AddIPv4EntryDifferentNINHG(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB, _ ...TestOpt) { - // TODO(robjs): Server needs to be initialised with >1 VRF. - t.Skip() - defer flushServer(c, t) ops := []func(){ func() { c.Modify().AddEntry(t, fluent.IPv4Entry(). WithPrefix("1.1.1.1/32"). - WithNetworkInstance("NON-DEFAULT"). + WithNetworkInstance(vrfName). WithNextHopGroup(1). WithNextHopGroupNetworkInstance(defaultNetworkInstanceName)) c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithID(1).AddNextHop(1, 1)) diff --git a/compliance/compliance_test.go b/compliance/compliance_test.go index edd948fc..c82a8efc 100644 --- a/compliance/compliance_test.go +++ b/compliance/compliance_test.go @@ -22,7 +22,10 @@ import ( "github.com/openconfig/gribigo/device" "github.com/openconfig/gribigo/fluent" "github.com/openconfig/gribigo/negtest" + "github.com/openconfig/gribigo/ocrt" + "github.com/openconfig/gribigo/server" "github.com/openconfig/gribigo/testcommon" + "github.com/openconfig/ygot/ygot" ) func TestCompliance(t *testing.T) { @@ -35,8 +38,16 @@ func TestCompliance(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - d, err := device.New(ctx, creds) + cfg := &ocrt.Device{} + cfg.GetOrCreateNetworkInstance(server.DefaultNetworkInstanceName).Type = ocrt.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE + cfg.GetOrCreateNetworkInstance(vrfName).Type = ocrt.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_L3VRF + jsonConfig, err := ygot.Marshal7951(cfg) + if err != nil { + t.Fatalf("cannot create configuration for device, error: %v", err) + } + + d, err := device.New(ctx, creds, device.DeviceConfig(jsonConfig)) if err != nil { t.Fatalf("cannot start server, %v", err) } diff --git a/compliance/flush.go b/compliance/flush.go index 3ff2df43..cbc92833 100644 --- a/compliance/flush.go +++ b/compliance/flush.go @@ -119,12 +119,8 @@ func FlushFromNonMasterDefaultNI(c *fluent.GRIBIClient, wantACK fluent.Programmi // L3VRF. It subsequently issues a Flush RPC and ensures that entries that are within the // flushed network instance (the default) are removed, but the others are preserved. func FlushOfSpecificNI(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB, _ ...TestOpt) { - // TODO(robjs): we need to initialise the server with >1 network instance. - t.Skip() defer flushServer(c, t) - vrfName := "TEST-VRF" - addFlushEntriesToNI(c, defaultNetworkInstanceName, wantACK, t) addFlushEntriesToNI(c, vrfName, wantACK, t) diff --git a/device/device.go b/device/device.go index 07b56016..1308d56c 100644 --- a/device/device.go +++ b/device/device.go @@ -142,23 +142,29 @@ func New(ctx context.Context, opts ...DevOpt) (*Device, error) { d := &Device{} jcfg := optDeviceCfg(opts) + dev := &ocrt.Device{} switch jcfg { case nil: - dev := &ocrt.Device{} - dev.GetOrCreateNetworkInstance("DEFAULT").Type = ocrt.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE - sr, err := sysrib.NewSysRIB(dev) - if err != nil { - return nil, fmt.Errorf("cannot build system RIB, %v", err) - } - d.sysRIB = sr + dev.GetOrCreateNetworkInstance(server.DefaultNetworkInstanceName).Type = ocrt.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_DEFAULT_INSTANCE default: - sr, err := sysrib.NewSysRIBFromJSON(jcfg) - if err != nil { - return nil, fmt.Errorf("cannot build system RIB, %v", err) + if err := ocrt.Unmarshal(jcfg, dev); err != nil { + return nil, fmt.Errorf("cannot unmarshal JSON configuration, %v", err) } - d.sysRIB = sr } + networkInstances := []string{} + for name, ni := range dev.NetworkInstance { + if ni.Type == ocrt.NetworkInstanceTypes_NETWORK_INSTANCE_TYPE_L3VRF { + networkInstances = append(networkInstances, name) + } + } + + sr, err := sysrib.NewSysRIB(dev) + if err != nil { + return nil, fmt.Errorf("cannot build system RIB, %v", err) + } + d.sysRIB = sr + ribHookfn := func(o constants.OpType, ts int64, ni string, data ygot.GoStruct) { _, _, _ = o, ni, data // write gNMI notifications @@ -215,6 +221,7 @@ func New(ctx context.Context, opts ...DevOpt) (*Device, error) { gRIBIStop, err := d.startgRIBI(ctx, gr.host, gr.port, creds, server.WithPostChangeRIBHook(ribHookfn), server.WithRIBResolvedEntryHook(ribAddfn), + server.WithVRFs(networkInstances), ) if err != nil { return nil, fmt.Errorf("cannot start gRIBI server, %v", err) @@ -282,11 +289,14 @@ func optTLSCreds(opts []DevOpt) *tlsCreds { func (d *Device) startgRIBI(ctx context.Context, host string, port int, creds *tlsCreds, opt ...server.ServerOpt) (func(), error) { l, err := net.Listen("tcp", fmt.Sprintf("%s:%d", host, port)) if err != nil { - return nil, fmt.Errorf("cannot create gRIBI server, %v", err) + return nil, fmt.Errorf("cannot create gRPC server for gRIBI, %v", err) } s := grpc.NewServer(grpc.Creds(creds.c)) - ts := server.New(opt...) + ts, err := server.New(opt...) + if err != nil { + return nil, fmt.Errorf("cannot create gRIBI server, %v", err) + } spb.RegisterGRIBIServer(s, ts) d.gribiAddr = l.Addr().String() d.gribiSrv = ts diff --git a/server/server.go b/server/server.go index 6f4887ea..7fd274d0 100644 --- a/server/server.go +++ b/server/server.go @@ -198,10 +198,10 @@ func DisableRIBCheckFn() *disableCheckFn { return &disableCheckFn{} } // disableCheckFn is the internal implementation of DisableRIBCheckFn. type disableCheckFn struct{} -// isRIBOpt implements the RIBOpt interface +// isServerOpt implements the ServerOpt interface func (*disableCheckFn) isServerOpt() {} -// hasDisableCheckFn checks whether the RIBOpt slice supplied contains the +// hasDisableCheckFn checks whether the ServerOpt slice supplied contains the // disableCheckFn option. func hasDisableCheckFn(opt []ServerOpt) bool { for _, o := range opt { @@ -212,8 +212,33 @@ func hasDisableCheckFn(opt []ServerOpt) bool { return false } +// WithVRFs specifies that the server should be initialised with the L3VRF +// network instances specified in the names list. Each is created in the +// server's RIB such that it can be referenced. +func WithVRFs(names []string) *withVRFs { return &withVRFs{names: names} } + +// withVRFs is the internal implementation of WithVRFs that can be read by the +// server. +type withVRFs struct { + names []string +} + +// isServerOpt implements the ServerOpt interface. +func (*withVRFs) isServerOpt() {} + +// hasWithVRFs checks whether the ServerOpt slice supplied contains the withVRFs +// option and returns it if so. +func hasWithVRFs(opt []ServerOpt) []string { + for _, o := range opt { + if v, ok := o.(*withVRFs); ok { + return v.names + } + } + return nil +} + // New creates a new gRIBI server. -func New(opt ...ServerOpt) *Server { +func New(opt ...ServerOpt) (*Server, error) { ribOpt := []rib.RIBOpt{} if hasDisableCheckFn(opt) { ribOpt = append(ribOpt, rib.DisableRIBCheckFn()) @@ -234,7 +259,15 @@ func New(opt ...ServerOpt) *Server { s.masterRIB.SetResolvedEntryHook(v.fn) } - return s + if vrfs := hasWithVRFs(opt); vrfs != nil { + for _, n := range vrfs { + if err := s.masterRIB.AddNetworkInstance(n); err != nil { + return nil, fmt.Errorf("cannot create network instance %s, %v", n, err) + } + } + } + + return s, nil } // Modify implements the gRIBI Modify RPC. @@ -1054,9 +1087,12 @@ type FakeServer struct { // NewFake returns a new version of the fake server. This implementation wraps // the Server implementation with functions to insert specific state into // the server without a need to use the public APIs. -func NewFake(opt ...ServerOpt) *FakeServer { - s := New(opt...) - return &FakeServer{Server: s} +func NewFake(opt ...ServerOpt) (*FakeServer, error) { + s, err := New(opt...) + if err != nil { + return nil, err + } + return &FakeServer{Server: s}, nil } // InjectRIB allows a client to inject a RIB into the server as though it was diff --git a/server/server_test.go b/server/server_test.go index 6df6630d..a92fcc8a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -68,7 +68,11 @@ func TestNewClient(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - s := New(nil) + s, err := New(nil) + if err != nil { + t.Fatalf("cannot create server, got error: %v", err) + } + for i, c := range tt.inIDs { wantErr := tt.wantClientErrCode[i] gotErr := s.newClient(c) @@ -713,16 +717,25 @@ func TestDoModify(t *testing.T) { inOps []*spb.AFTOperation wantMsg []*expectedMsg }{{ - desc: "unknown client", - inServer: New(), - inCID: "unknown", + desc: "unknown client", + inServer: func() *Server { + s, err := New() + if err != nil { + t.Fatalf("cannot create server, %v", err) + } + return s + }(), + inCID: "unknown", wantMsg: []*expectedMsg{{ errCode: codes.Internal, }}, }, { desc: "unsupported default parameters", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{} return s }(), @@ -734,7 +747,10 @@ func TestDoModify(t *testing.T) { }, { desc: "not expecting election ID", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ ExpectElecID: false, @@ -750,7 +766,10 @@ func TestDoModify(t *testing.T) { }, { desc: "not expecting persist=false", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: false, @@ -766,7 +785,10 @@ func TestDoModify(t *testing.T) { }, { desc: "add to default network instance", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: true, @@ -803,7 +825,10 @@ func TestDoModify(t *testing.T) { }, { desc: "add to unknown network instance", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: true, @@ -842,7 +867,10 @@ func TestDoModify(t *testing.T) { }, { desc: "invalid operation", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: true, @@ -878,7 +906,10 @@ func TestDoModify(t *testing.T) { }, { desc: "two valid operations", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: true, @@ -933,7 +964,10 @@ func TestDoModify(t *testing.T) { }, { desc: "ipv4 to one next-hop group containing two next-hops", inServer: func() *Server { - s := New() + s, err := New() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } s.cs["testclient"] = &clientState{ params: &clientParams{ Persist: true, @@ -1553,7 +1587,10 @@ func TestDoGet(t *testing.T) { // populated. serverAllRIBs := func() *Server { // use a nil function to check the RIB so that addEntry always succeeds - s := New(DisableRIBCheckFn()) + s, err := New(DisableRIBCheckFn()) + if err != nil { + t.Fatalf("cannot create server, %v", err) + } if _, _, err := s.masterRIB.AddEntry(DefaultNetworkInstanceName, &spb.AFTOperation{ Id: 1, @@ -1632,12 +1669,13 @@ func TestDoGet(t *testing.T) { }, }, inServer: func() *Server { - s := New(DisableRIBCheckFn()) vrfNames := []string{"ONE", "EIGHT", "FOUR"} - for _, v := range vrfNames { - if err := s.masterRIB.AddNetworkInstance(v); err != nil { - panic(fmt.Sprintf("cannot build testcase, %v", err)) - } + s, err := New( + DisableRIBCheckFn(), + WithVRFs(vrfNames), + ) + if err != nil { + t.Fatalf("cannot create server, err: %v", err) } prefixes := []string{"1.1.1.1/32", "8.8.8.8/32", "8.8.4.4/32"} @@ -1770,7 +1808,11 @@ func TestDoGet(t *testing.T) { s := tt.inServer if tt.inServer == nil { - s = New() + var err error + s, err = New() + if err != nil { + t.Fatalf("cannot create server, %v", err) + } } go s.doGet(tt.inReq, msgCh, doneCh, stopCh, errCh) @@ -1978,7 +2020,10 @@ func TestFlush(t *testing.T) { // singleNI creates a server with the default network instance with one entry. singleNI := func() *Server { - s := NewFake() + s, err := NewFake() + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } r := rib.New(DefaultNetworkInstanceName) addEntry(r, DefaultNetworkInstanceName) s.InjectRIB(r) @@ -1997,16 +2042,15 @@ func TestFlush(t *testing.T) { // other network instances specified, it contains one entry per network // instance. multiNI := func(names []string) *Server { - s := NewFake() - r := rib.New(DefaultNetworkInstanceName) - addEntry(r, DefaultNetworkInstanceName) + s, err := NewFake(WithVRFs(names)) + if err != nil { + t.Fatalf("cannot create server, error: %v", err) + } + addEntry(s.masterRIB, DefaultNetworkInstanceName) for _, n := range names { - if err := r.AddNetworkInstance(n); err != nil { - t.Fatalf("cannot add network instance %s to server, %v", n, err) - } - addEntry(r, n) + addEntry(s.masterRIB, n) } - s.InjectRIB(r) + return s.Server } @@ -2048,6 +2092,7 @@ func TestFlush(t *testing.T) { }, wantEntriesInNI: map[string]int{ DefaultNetworkInstanceName: 3, + "one": 0, }, }, { desc: "don't remove any entries",