From 6a8cd9d6751bf38c271c84d33787f3dc144d9ed5 Mon Sep 17 00:00:00 2001 From: Rob Shakir Date: Mon, 11 Oct 2021 18:51:30 -0700 Subject: [PATCH] [1/4] Compliance test fixes. (#80) * Add ccli parameters, update expected error response codes. * (M) client/client.go - Avoid panic when details are not populated. * (M) cmd/ccli/ccli_test.go - Add command line flag for skipping tests based on requiring FIB FIB ACK to allow skipping these tests where this is unimplemented. * (M) compliance/compliance.go - Add support for a caller to specify the election ID to be used. * Send non-empty NH, improve client error output. * (M) client/client.go - Add String() method for operation result's details such that there is additional information as to what the error corresponds to. * (M) compliance/compliance.go - Ensure that we do not send an empty next-hop when at least one field must be populated. See #79 which tracks adding a fix to the server side code. * Add support for allowing unimplemented in a check. * (M) chk/chk.go - Add options for checking errors that can be used to check whether something is unimplemented OR a specific error. * (M) compliance/compliance.go - Adopt this check for failed precondition vs. unimplemented for ALL_PRIMARY. * [2/4] Bug fixes in test code. (#81) * Bug fixes in test code. * (M) chk/chk.go - Add better error message formatting for debugging. * (M) client/client.go - Add a special case for a match used in compliance tests where the operation ID expected is unspecified. * (M) cmd/ccli/ccli_test.go - Rename some flags for consistency, allow tests that require reordering of transactions by the server to be skipped. * (M) compliance/compliance.go - Restructure the compliance tests to have a generator function that can be used to specify the ACK type, rather than needing to write wrappers manually. * Specify InstalledInRIB as an argument. * (M) demo/ipv4/ipv4_test.go * (M) device/device_test.go - Adopt renamed tests. * [3/4] Add compliance tests for metadata, different NI NHG. (#68) * Add compliance tests for IPv4 entries, fix server defect. * (M) client/client.go - Improve logging. * (M) compliance/compliance.go - Add test for adding IPv4 entry in a single ModifyRequest, and within separate requests. * (M) fluent/fluent.go - Add support for WithIPAddress to builder. * (M) gnmit/gnmit.go - Minor refactor to use a new GNMIServer container - no-op change for adding gNMI Set support. * (M) rib/rib.go - Improve logging. * (M) server/server.go * (M) server/server_test.go - Resolve a defect whereby the server called modifyEntry within a goroutine per operation within a ModifyRequest. In the case where a single ModifyRequest made multiple entries resolvable, then an entry could be duplicate ACK'd, by all the entries that made it resolvable. Whilst the RIB is safe to modify in this manner, the pending queue could result in this occurring - the duplicate install is a NOOP, but the duplicate ACK violated the API contract. Initially, resolve this issue by serially processing modify requests - added a TODO to cover a less naive solution. * Whitespace fixes. * Add compliance tests for deleting entries. * (M) client/client.go - Improve debugging output of an operation result. * (M) compliance/compliance.go - Add tests for deleting IPv4 entries, NHGs, NHs - both positive and negative tests. * (M) fluent/fluent.go - Add ability to reference a NHG NI from an IPv4Entry. * (M) go.sum - Tidy up go modules. * Fix tests to adopt new client stub argument. * Add compliance tests for metadata, different NI NHG. * (M) client/client.go - Improve string message for OpResults. * (M) compliance/compliance.go - Add IPv4Entry -> NHG in different NI test - currently skipped based on server configuration. - Add test for metadata on IPv4Entry. * (M) fluent/fluent.go - Add fluent function for adding metadata. * Fix support for comparing to unimplemented. * (M) chk/chk.go - Ensure that details are maintained when checking for unimplemented. * (M) compliance/compliance.go - Clean up compliance test. * (M) go.mod * (M) go.sum - Add golang gRPC genproto dependency. * Update test names. * (M) demo/ipv4/ipv4_test.go * (M) device/device_test.go - Specify RIB ACK as an argument rather than using the removed test. * [4/4] Handle nil responses when debugging. (#73) * Do not send election_id = 0. * (M) compliance/compliance.go - Ensure that the compliance tests do not send election_id = 0 which is an invalid value. * Add server handling for election ID = 0. * (M) server/server.go * (M) server/server_test.go - Ensure that election ID set to zero is responded to with an error. * Handle nil responses when debugging. * (M) client/client.go * (M) client/client_test.go - Ensure that a nil input value is cleanly handled when calling String() on a nil OpResult. * Fix string error reporting for partially specified messages. * (M) client/client.go * (M) client/client_test.go - Fix a panic in OpResult.String that could occur for partially populated messages. --- chk/chk.go | 63 ++++++-- client/client.go | 61 ++++++-- client/client_test.go | 30 ++++ cmd/ccli/ccli_test.go | 29 +++- compliance/compliance.go | 314 +++++++++++++++++++++++++++------------ demo/ipv4/ipv4_test.go | 2 +- device/device_test.go | 2 +- fluent/fluent.go | 9 +- go.mod | 1 + go.sum | 2 + 10 files changed, 380 insertions(+), 133 deletions(-) diff --git a/chk/chk.go b/chk/chk.go index a5509bcc..ceac4757 100644 --- a/chk/chk.go +++ b/chk/chk.go @@ -21,14 +21,19 @@ package chk import ( + "bytes" + "fmt" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/openconfig/gribigo/client" + "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" + + spb "google.golang.org/genproto/googleapis/rpc/status" ) // resultOpt is an interface implemented by all options that can be @@ -91,7 +96,13 @@ func HasResult(t testing.TB, res []*client.OpResult, want *client.OpResult, opt } } if !found { - t.Fatalf("results did not contain a result of value %s, got: %v", want, res) + buf := &bytes.Buffer{} + buf.WriteString(fmt.Sprintf("results did not contain a result of value %s\n", want)) + buf.WriteString("got:\n") + for _, r := range res { + buf.WriteString(fmt.Sprintf("\t%s\n", r)) + } + t.Fatalf(buf.String()) } } @@ -123,22 +134,54 @@ func HasNRecvErrors(t testing.TB, err error, count int) { } } +// ErrorOpt is an interface that is implemented by functions that examine errrors. +type ErrorOpt interface { + isErrorOpt() +} + +// allowUnimplemented is the internal representation of an ErrorOpt that allows for +// an error that is unimplemented or a specific error. +type allowUnimplemented struct{} + +// isErrorOpt marks allowUnimplemented as an ErrorOpt. +func (*allowUnimplemented) isErrorOpt() {} + +// AllowUnimplemented specifies that receive error with a particular status can be unimplemented +// OR the specified error type. It can be used to not return a fatal error when a server does +// not support a particular functionality. +func AllowUnimplemented() *allowUnimplemented { + return &allowUnimplemented{} +} + // HasRecvClientErrorWithStatus checks whether the supplied ClientErr ce contains a status with // the code and details set to the values supplied in want. -func HasRecvClientErrorWithStatus(t testing.TB, err error, want *status.Status) { +func HasRecvClientErrorWithStatus(t testing.TB, err error, want *status.Status, opts ...ErrorOpt) { t.Helper() + okMsgs := []*status.Status{want} + for _, o := range opts { + if _, ok := o.(*allowUnimplemented); ok { + uProto := proto.Clone(want.Proto()).(*spb.Status) + uProto.Code = int32(codes.Unimplemented) + unimpl := status.FromProto(uProto) + okMsgs = append(okMsgs, unimpl) + } + } + var found bool ce := clientError(t, err) for _, e := range ce.Recv { - s, ok := status.FromError(e) - if !ok { - continue - } - ns := s.Proto() - ns.Message = "" // blank out message so that we don't compare it. - if proto.Equal(ns, want.Proto()) { - found = true + for _, wo := range okMsgs { + s, ok := status.FromError(e) + if !ok { + continue + } + ns := s.Proto() + ns.Message = "" // blank out message so that we don't compare it. + + if proto.Equal(ns, wo.Proto()) { + found = true + } } } if !found { diff --git a/client/client.go b/client/client.go index 1aa7d487..3f6dc198 100644 --- a/client/client.go +++ b/client/client.go @@ -518,22 +518,12 @@ type OpResult struct { Details *OpDetailsResults } -// OpDetailsResults provides details of an operation for use in the results. -type OpDetailsResults struct { - // Type is the type of the operation (i.e., ADD, MODIFY, DELETE) - Type constants.OpType - - // NextHopIndex is the identifier for a next-hop modified by the operation. - NextHopIndex uint64 - // NextHopGroupID is the identifier for a next-hop-group modified by the - // operation. - NextHopGroupID uint64 - // IPv4Prefix is the IPv4 prefix modified by the operation. - IPv4Prefix string -} - // String returns a string for an OpResult for debugging purposes. func (o *OpResult) String() string { + if o == nil { + return "" + } + buf := &bytes.Buffer{} buf.WriteString("<") buf.WriteString(fmt.Sprintf("%d (%d nsec):", o.Timestamp, o.Latency)) @@ -544,7 +534,14 @@ func (o *OpResult) String() string { } if v := o.OperationID; v != 0 { - buf.WriteString(fmt.Sprintf(" AFTOperation { ID: %d, Type: %s, Status: %s }", v, o.Details.Type, o.ProgrammingResult)) + typ := "Unknown" + if o.Details != nil { + typ = o.Details.Type.String() + } + buf.WriteString(fmt.Sprintf(" AFTOperation { ID: %d, Type: %s, Status: %s }", v, typ, o.ProgrammingResult)) + } else if v := o.ProgrammingResult; v != spb.AFTResult_UNSET { + // Special case for input messages that are just matching on status. + buf.WriteString(fmt.Sprintf(" AFTOperation { Status: %s }", v)) } if v := o.SessionParameters; v != nil { @@ -559,6 +556,40 @@ func (o *OpResult) String() string { return buf.String() } +// OpDetailsResults provides details of an operation for use in the results. +type OpDetailsResults struct { + // Type is the type of the operation (i.e., ADD, MODIFY, DELETE) + Type constants.OpType + + // NextHopIndex is the identifier for a next-hop modified by the operation. + NextHopIndex uint64 + // NextHopGroupID is the identifier for a next-hop-group modified by the + // operation. + NextHopGroupID uint64 + // IPv4Prefix is the IPv4 prefix modified by the operation. + IPv4Prefix string +} + +// String returns a human-readable form of the OpDetailsResults +func (o *OpDetailsResults) String() string { + if o == nil { + return "" + } + buf := &bytes.Buffer{} + buf.WriteString(fmt.Sprintf("") + + return buf.String() +} + // Q enqueues a ModifyRequest to be sent to the target. func (c *Client) Q(m *spb.ModifyRequest) { if err := c.handleModifyRequest(m); err != nil { diff --git a/client/client_test.go b/client/client_test.go index f8b84e6a..fa642d5d 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1106,3 +1106,33 @@ func TestGet(t *testing.T) { }) } } + +func TestOpResultString(t *testing.T) { + tests := []struct { + desc string + inResult *OpResult + want string + }{{ + desc: "nil input", + inResult: nil, + want: "", + }, { + desc: "all fields nil", + inResult: &OpResult{}, + want: "<0 (0 nsec):>", + }, { + desc: "nil type in details", + inResult: &OpResult{ + OperationID: 42, + }, + want: "<0 (0 nsec): AFTOperation { ID: 42, Type: Unknown, Status: UNSET }>", + }} + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + if got := tt.inResult.String(); got != tt.want { + t.Fatalf("did not get expected string, got: %s, want: %s", got, tt.want) + } + }) + } +} diff --git a/cmd/ccli/ccli_test.go b/cmd/ccli/ccli_test.go index f1f727e4..7a3ff8b4 100644 --- a/cmd/ccli/ccli_test.go +++ b/cmd/ccli/ccli_test.go @@ -34,11 +34,14 @@ import ( ) var ( - addr = flag.String("addr", "", "address of the gRIBI server in the format hostname:port") - insecure = flag.Bool("insecure", false, "dial insecure gRPC (no TLS)") - skipverify = flag.Bool("skipverify", true, "allow self-signed TLS certificate; not needed for -insecure") - username = flag.String("username", os.Getenv("USER"), "username to be sent as gRPC metadata") - password = flag.String("password", "", "password to be sent as gRPC metadata") + addr = flag.String("addr", "", "address of the gRIBI server in the format hostname:port") + insecure = flag.Bool("insecure", false, "dial insecure gRPC (no TLS)") + skipVerify = flag.Bool("skip_verify", true, "allow self-signed TLS certificate; not needed for -insecure") + username = flag.String("username", os.Getenv("USER"), "username to be sent as gRPC metadata") + password = flag.String("password", "", "password to be sent as gRPC metadata") + initialElectionID = flag.Uint("initial_electionid", 0, "initial election ID to be used") + skipFIBACK = flag.Bool("skip_fiback", false, "skip tests that rely on FIB ACK") + skipSrvReorder = flag.Bool("skip_reordering", false, "skip tests that rely on server side transaction reordering") ) // flagCred implements credentials.PerRPCCredentials by populating the @@ -64,12 +67,16 @@ func TestCompliance(t *testing.T) { return // Test is part of CI, so do not fail here. } + if *initialElectionID != 0 { + compliance.SetElectionID(uint64(*initialElectionID)) + } + dialOpts := []grpc.DialOption{grpc.WithBlock()} if *insecure { dialOpts = append(dialOpts, grpc.WithInsecure()) - } else if *skipverify { + } else if *skipVerify { tlsc := credentials.NewTLS(&tls.Config{ - InsecureSkipVerify: *skipverify, + InsecureSkipVerify: *skipVerify, }) dialOpts = append(dialOpts, grpc.WithTransportCredentials(tlsc)) } @@ -87,6 +94,14 @@ func TestCompliance(t *testing.T) { stub := spb.NewGRIBIClient(conn) for _, tt := range compliance.TestSuite { + if skip := *skipFIBACK; skip && tt.In.RequiresFIBACK { + continue + } + + if skip := *skipSrvReorder; skip && tt.In.RequiresServerReordering { + continue + } + t.Run(tt.In.ShortName, func(t *testing.T) { c := fluent.NewClient() c.Connection().WithStub(stub) diff --git a/compliance/compliance.go b/compliance/compliance.go index 7d3d3eb8..03d021c4 100644 --- a/compliance/compliance.go +++ b/compliance/compliance.go @@ -44,6 +44,12 @@ func init() { // the last tests', and thus fail due to the state of the server. var electionID = &atomic.Uint64{} +// SetElectionID allows an external caller to specify an election ID to be used for +// subsequent calls. +func SetElectionID(v uint64) { + electionID.Store(v) +} + // Test describes a test within the compliance library. type Test struct { // Fn is the function to be run for a test. @@ -55,6 +61,16 @@ type Test struct { ShortName string // Reference is a unique reference to external data (e.g., test plans) used for the test. Reference string + + // RequiresFIBACK marks a test that requires the implementation of FIB ACK on the server. + // This is expected behaviour of a gRIBI server, but some implementations add this in + // later builds. + RequiresFIBACK bool + // RequiresServerReordering marks a test that requires the implementation of server-side + // reordering of transactions rather than an immediate NACK. Currently, some implementations + // immediately NACK forward references, which causes some tests to fail. The reference + // implementation handles reodering. + RequiresServerReordering bool } // TestSpec is a description of a test. @@ -86,67 +102,106 @@ var ( }, }, { In: Test{ - Fn: AddIPv4EntryRIBACK, + Fn: makeTestWithACK(AddIPv4Entry, fluent.InstalledInRIB), Reference: "TE-2.1.1.1", ShortName: "Add IPv4 entry that can be programmed on the server - with RIB ACK", }, }, { In: Test{ - Fn: AddIPv4EntryFIBACK, - Reference: "TE-2.1.1.2", - ShortName: "Add IPv4 entry that can be programmed on the server - with FIB ACK", + Fn: makeTestWithACK(AddIPv4Entry, fluent.InstalledInFIB), + Reference: "TE-2.1.1.2", + ShortName: "Add IPv4 entry that can be programmed on the server - with FIB ACK", + RequiresFIBACK: true, }, }, { In: Test{ - Fn: AddUnreferencedNextHopGroupFIBACK, + Fn: makeTestWithACK(AddUnreferencedNextHopGroup, fluent.InstalledInRIB), ShortName: "Add next-hop-group entry that can be resolved on the server, no referencing IPv4 entries - with RIB ACK", }, }, { In: Test{ - Fn: AddUnreferencedNextHopGroupRIBACK, - ShortName: "Add next-hop-group entry that can be resolved on the server, no referencing IPv4 entries - with FIB ACK", + Fn: makeTestWithACK(AddUnreferencedNextHopGroup, fluent.InstalledInFIB), + ShortName: "Add next-hop-group entry that can be resolved on the server, no referencing IPv4 entries - with FIB ACK", + RequiresFIBACK: true, + }, + }, { + In: Test{ + Fn: AddIPv4EntryRandom, + ShortName: "Add IPv4 entries that are resolved by NHG and NH, in random order", + RequiresServerReordering: true, }, }, { In: Test{ - Fn: AddIPv4EntryRandom, - ShortName: "Add IPv4 entries that are resolved by NHG and NH, in random order", + Fn: makeTestWithACK(AddIPv4ToMultipleNHsSingleRequest, fluent.InstalledInFIB), + ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (single ModifyRequest) - with FIB ACK", + Reference: "TE-2.1.2.1", + RequiresFIBACK: true, }, }, { In: Test{ - Fn: AddIPv4ToMultipleNHsSingleRequest, - ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (single ModifyRequest)", + Fn: makeTestWithACK(AddIPv4ToMultipleNHsSingleRequest, fluent.InstalledInRIB), + ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (single ModifyRequest) - with RIB ACK", Reference: "TE-2.1.2.1", }, }, { In: Test{ - Fn: AddIPv4ToMultipleNHsMultipleRequests, - ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (multiple ModifyRequests)", + Fn: makeTestWithACK(AddIPv4ToMultipleNHsMultipleRequests, fluent.InstalledInFIB), + ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (multiple ModifyRequests) - with FIB ACK", + Reference: "TE-2.1.2.2", + RequiresFIBACK: true, + }, + }, { + In: Test{ + Fn: makeTestWithACK(AddIPv4ToMultipleNHsMultipleRequests, fluent.InstalledInRIB), + ShortName: "Add IPv4 entries that are resolved to a next-hop-group containing multiple next-hops (multiple ModifyRequests) - with RIB ACK", Reference: "TE-2.1.2.2", }, }, { In: Test{ - Fn: DeleteIPv4Entry, - ShortName: "Delete IPv4 entry within default network instance", + Fn: makeTestWithACK(DeleteIPv4Entry, fluent.InstalledInRIB), + ShortName: "Delete IPv4 entry within default network instance - RIB ACK", + }, + }, { + In: Test{ + Fn: makeTestWithACK(DeleteReferencedNHGFailure, fluent.InstalledInRIB), + ShortName: "Delete NHG entry that is referenced - failure - RIB ACK", + }, + }, { + In: Test{ + Fn: makeTestWithACK(DeleteReferencedNHFailure, fluent.InstalledInRIB), + ShortName: "Delete NH entry that is referenced - failure - RIB ACK", }, }, { In: Test{ - Fn: DeleteReferencedNHGFailure, - ShortName: "Delete NHG entry that is referenced - failure", + Fn: makeTestWithACK(DeleteNextHopGroup, fluent.InstalledInRIB), + ShortName: "Delete NHG entry successfully - RIB ACK", }, }, { In: Test{ - Fn: DeleteReferencedNHFailure, - ShortName: "Delete NH entry that is referenced - failure", + Fn: makeTestWithACK(DeleteNextHopGroup, fluent.InstalledInFIB), + ShortName: "Delete NHG entry successfully - FIB ACK", + RequiresFIBACK: true, }, }, { In: Test{ - Fn: DeleteNextHopGroup, - ShortName: "Delete NHG entry successfully", + Fn: makeTestWithACK(DeleteNextHop, fluent.InstalledInRIB), + ShortName: "Delete NH entry successfully - RIB ACK", }, }, { In: Test{ - Fn: DeleteNextHop, - ShortName: "Delete NH entry successfully", + Fn: makeTestWithACK(DeleteNextHop, fluent.InstalledInRIB), + ShortName: "Delete NH entry successfully - FIB ACK", + RequiresFIBACK: true, + }, + }, { + In: Test{ + Fn: AddIPv4Metadata, + ShortName: "Add Metadata for IPv4 entry", + }, + }, { + In: Test{ + Fn: makeTestWithACK(AddIPv4EntryDifferentNINHG, fluent.InstalledInRIB), + ShortName: "Add IPv4 Entry that references a NHG in a different network instance", }, }} ) @@ -223,36 +278,12 @@ func ModifyConnectionSinglePrimaryPreserve(c *fluent.GRIBIClient, t testing.TB) WithReason(fluent.UnsupportedParameters). AsStatus(t) - chk.HasRecvClientErrorWithStatus(t, err, want) -} - -// AddIPv4EntryRIBACK adds a simple IPv4 Entry which references a next-hop-group -// to the gRIBI server, requesting a RIB-level ACK. -func AddIPv4EntryRIBACK(c *fluent.GRIBIClient, t testing.TB) { - addIPv4Internal(c, t, fluent.InstalledInRIB) -} - -// AddIPv4EntryFIBACK adds a simple IPv4 Entry which references a next-hop-group -// to the gRIBI server, requesting a FIB-level ACK. -func AddIPv4EntryFIBACK(c *fluent.GRIBIClient, t testing.TB) { - addIPv4Internal(c, t, fluent.InstalledInFIB) -} - -// AddUnreferencedNextHopGroupRIBACK adds an unreferenced next-hop-group that contains -// nexthops to the gRIBI server, requesting a FIB-level ACK. -func AddUnreferencedNextHopGroupRIBACK(c *fluent.GRIBIClient, t testing.TB) { - addNextHopGroupInternal(c, t, fluent.InstalledInRIB) -} - -// AddUnreferencedNextHopGroupFIBACK adds an unreferenced next-hop-group that contains -// nexthops to the gRIBI server, requesting a FIB-level ACK. -func AddUnreferencedNextHopGroupFIBACK(c *fluent.GRIBIClient, t testing.TB) { - addNextHopGroupInternal(c, t, fluent.InstalledInFIB) + chk.HasRecvClientErrorWithStatus(t, err, want, chk.AllowUnimplemented()) } -// addIPv4Internal is an internal test that adds IPv4 entries, and checks -// whether the specified FIB ack is received. -func addIPv4Internal(c *fluent.GRIBIClient, t testing.TB, wantACK fluent.ProgrammingResult) { +// AddIPv4Entry adds a fully referenced IPv4Entry and checks whether the specified ACK +// type (wantACK) is returned. +func AddIPv4Entry(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { c.Modify().AddEntry(t, fluent.NextHopEntry().WithNetworkInstance(server.DefaultNetworkInstanceName).WithIndex(1).WithIPAddress("192.0.2.1")) @@ -338,6 +369,93 @@ func AddIPv4EntryRandom(c *fluent.GRIBIClient, t testing.TB) { // TODO(robjs): add gNMI subscription using generated telemetry library. } +// AddIPv4Metadata adds an IPv4 Entry (and its dependencies) with metadata alongside the +// entry. +func AddIPv4Metadata(c *fluent.GRIBIClient, t testing.TB) { + ops := []func(){ + func() { + c.Modify().AddEntry(t, fluent.IPv4Entry(). + WithPrefix("1.1.1.1/32"). + WithNetworkInstance(server.DefaultNetworkInstanceName). + WithNextHopGroup(1). + WithMetadata([]byte{1, 2, 3, 4, 5, 6, 7, 8}), + ) + c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithID(1).WithNetworkInstance(server.DefaultNetworkInstanceName).AddNextHop(1, 1)) + c.Modify().AddEntry(t, fluent.NextHopEntry().WithIndex(1).WithNetworkInstance(server.DefaultNetworkInstanceName).WithIPAddress("2.2.2.2")) + }, + } + + res := doOps(c, t, ops, fluent.InstalledInRIB, false) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithIPv4Operation("1.1.1.1/32"). + WithOperationType(constants.Add). + WithProgrammingResult(fluent.InstalledInRIB). + AsResult(), + chk.IgnoreOperationID()) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithNextHopGroupOperation(1). + WithOperationType(constants.Add). + WithProgrammingResult(fluent.InstalledInRIB). + AsResult(), + chk.IgnoreOperationID()) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithNextHopOperation(1). + WithOperationType(constants.Add). + WithProgrammingResult(fluent.InstalledInRIB). + AsResult(), + chk.IgnoreOperationID()) +} + +// 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) { + // TODO(robjs): Server needs to be initialised with >1 VRF. + t.Skip() + ops := []func(){ + func() { + c.Modify().AddEntry(t, fluent.IPv4Entry(). + WithPrefix("1.1.1.1/32"). + WithNetworkInstance("NON-DEFAULT"). + WithNextHopGroup(1). + WithNextHopGroupNetworkInstance(server.DefaultNetworkInstanceName)) + c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithID(1).AddNextHop(1, 1)) + c.Modify().AddEntry(t, fluent.NextHopEntry().WithIndex(1).WithIPAddress("2.2.2.2")) + }, + } + + res := doOps(c, t, ops, wantACK, false) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithNextHopGroupOperation(1). + WithProgrammingResult(wantACK). + WithOperationType(constants.Add). + AsResult(), + chk.IgnoreOperationID()) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithNextHopOperation(1). + WithProgrammingResult(wantACK). + WithOperationType(constants.Add). + AsResult(), + chk.IgnoreOperationID()) + + chk.HasResult(t, res, + fluent.OperationResult(). + WithIPv4Operation("1.1.1.1/32"). + WithProgrammingResult(wantACK). + WithOperationType(constants.Add). + AsResult(), + chk.IgnoreOperationID()) +} + // doOps performs the series of operations in ops using the context // client c. wantACK specifies the ACK type to request from the // server, and randomise specifies whether the operations should be @@ -378,14 +496,12 @@ func doOps(c *fluent.GRIBIClient, t testing.TB, ops []func(), wantACK fluent.Pro return c.Results(t) } -// addNextHopGroupInternal is an internal implementation that checks that a -// next-hop-group can be added to the gRIBI server with the specified ACK mode. -// The tests does not install an IPv4Entry, so these NHGs are unreferenced. -// We still expect an ACK in this case. -func addNextHopGroupInternal(c *fluent.GRIBIClient, t testing.TB, wantACK fluent.ProgrammingResult) { +// AddUnreferencedNextHopGroup adds a NHG that is not referenced by any other entry. An ACK is expected, +// and is validated to be of the type specified by wantACK. +func AddUnreferencedNextHopGroup(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { - c.Modify().AddEntry(t, fluent.NextHopEntry().WithNetworkInstance(server.DefaultNetworkInstanceName).WithIndex(1)) + c.Modify().AddEntry(t, fluent.NextHopEntry().WithNetworkInstance(server.DefaultNetworkInstanceName).WithIndex(1).WithIPAddress("192.0.2.1")) }, func() { c.Modify().AddEntry(t, fluent.NextHopGroupEntry().WithNetworkInstance(server.DefaultNetworkInstanceName).WithID(42).AddNextHop(1, 1)) @@ -438,13 +554,13 @@ func baseTopologyEntries(c *fluent.GRIBIClient, t testing.TB) { // validateBaseEntries checks that the entries in the base topology are correctly // installed. -func validateBaseTopologyEntries(res []*client.OpResult, t testing.TB) { +func validateBaseTopologyEntries(res []*client.OpResult, wantACK fluent.ProgrammingResult, t testing.TB) { // Check for next-hops 1 and 2. for _, nhopID := range []uint64{1, 2} { chk.HasResult(t, res, fluent.OperationResult(). WithNextHopOperation(nhopID). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). WithOperationType(constants.Add). AsResult(), chk.IgnoreOperationID(), @@ -455,7 +571,7 @@ func validateBaseTopologyEntries(res []*client.OpResult, t testing.TB) { chk.HasResult(t, res, fluent.OperationResult(). WithNextHopGroupOperation(1). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). WithOperationType(constants.Add). AsResult(), chk.IgnoreOperationID(), @@ -465,17 +581,17 @@ func validateBaseTopologyEntries(res []*client.OpResult, t testing.TB) { chk.HasResult(t, res, fluent.OperationResult(). WithIPv4Operation("1.0.0.0/8"). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). WithOperationType(constants.Add). AsResult(), chk.IgnoreOperationID(), ) } -// AddIPv4ToMultipleNHsSingleRequest creates an IPv4 entry which references a NHG containing -// 2 NHs within a single ModifyRequest, validating that they are installed in the FIB. -func AddIPv4ToMultipleNHsSingleRequest(c *fluent.GRIBIClient, t testing.TB) { - +// AddIPv4ToMultipleNHsSingleRequest is the internal implementation of the single request installation +// of a IPv4Entry referencing a NHG that contains multiple NHs. It uses the wantACK parameter to determine the +// type of acknowledgement that is expected from the server. +func AddIPv4ToMultipleNHsSingleRequest(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { c.Modify().AddEntry(t, @@ -486,35 +602,41 @@ func AddIPv4ToMultipleNHsSingleRequest(c *fluent.GRIBIClient, t testing.TB) { }, } - validateBaseTopologyEntries(doOps(c, t, ops, fluent.InstalledInFIB, false), t) + validateBaseTopologyEntries(doOps(c, t, ops, wantACK, false), wantACK, t) } // AddIPv4ToMultipleNHsMultipleRequests creates an IPv4 entry which references a NHG containing -// 2 NHs within multiple ModifyReqests, validating that they are installed in the FIB. -func AddIPv4ToMultipleNHsMultipleRequests(c *fluent.GRIBIClient, t testing.TB) { - +// 2 NHs within multiple ModifyReqests, validating that they are installed in the specified RIB +// or FIB according to wantACK. +func AddIPv4ToMultipleNHsMultipleRequests(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, } - validateBaseTopologyEntries(doOps(c, t, ops, fluent.InstalledInFIB, false), t) + validateBaseTopologyEntries(doOps(c, t, ops, wantACK, false), wantACK, t) +} + +// makeTestWithACK creates a version of a test function that can be directly executed by the runner +// which has a specific ACK type. +func makeTestWithACK(fn func(*fluent.GRIBIClient, fluent.ProgrammingResult, testing.TB), wantACK fluent.ProgrammingResult) func(*fluent.GRIBIClient, testing.TB) { + return func(c *fluent.GRIBIClient, t testing.TB) { fn(c, wantACK, t) } } // DeleteIPv4Entry deletes an IPv4 entry from the server's RIB. -func DeleteIPv4Entry(c *fluent.GRIBIClient, t testing.TB) { +func DeleteIPv4Entry(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, func() { c.Modify().DeleteEntry(t, fluent.IPv4Entry().WithPrefix("1.0.0.0/8").WithNetworkInstance(server.DefaultNetworkInstanceName)) }, } - res := doOps(c, t, ops, fluent.InstalledInFIB, false) - validateBaseTopologyEntries(res, t) + res := doOps(c, t, ops, wantACK, false) + validateBaseTopologyEntries(res, wantACK, t) chk.HasResult(t, res, fluent.OperationResult(). WithIPv4Operation("1.0.0.0/8"). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID(), ) @@ -522,15 +644,15 @@ func DeleteIPv4Entry(c *fluent.GRIBIClient, t testing.TB) { // DeleteReferencedNHGFailure attempts to delete a NextHopGroup entry that is referenced // from the RIB, and expects a failure. -func DeleteReferencedNHGFailure(c *fluent.GRIBIClient, t testing.TB) { +func DeleteReferencedNHGFailure(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, func() { c.Modify().DeleteEntry(t, fluent.NextHopGroupEntry().WithID(1).WithNetworkInstance(server.DefaultNetworkInstanceName)) }, } - res := doOps(c, t, ops, fluent.InstalledInFIB, false) - validateBaseTopologyEntries(res, t) + res := doOps(c, t, ops, wantACK, false) + validateBaseTopologyEntries(res, wantACK, t) chk.HasResult(t, res, fluent.OperationResult(). @@ -543,7 +665,7 @@ func DeleteReferencedNHGFailure(c *fluent.GRIBIClient, t testing.TB) { // DeleteReferencedNHFailure attempts to delete a NH entry that is referened from the RIB // and expects a failure. -func DeleteReferencedNHFailure(c *fluent.GRIBIClient, t testing.TB) { +func DeleteReferencedNHFailure(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, func() { @@ -553,8 +675,8 @@ func DeleteReferencedNHFailure(c *fluent.GRIBIClient, t testing.TB) { c.Modify().DeleteEntry(t, fluent.NextHopEntry().WithIndex(2).WithNetworkInstance(server.DefaultNetworkInstanceName)) }, } - res := doOps(c, t, ops, fluent.InstalledInFIB, false) - validateBaseTopologyEntries(res, t) + res := doOps(c, t, ops, wantACK, false) + validateBaseTopologyEntries(res, wantACK, t) for _, i := range []uint64{1, 2} { chk.HasResult(t, res, @@ -568,8 +690,8 @@ func DeleteReferencedNHFailure(c *fluent.GRIBIClient, t testing.TB) { } // DeleteNextHopGroup attempts to delete a NHG entry that is not referenced and expects -// success. -func DeleteNextHopGroup(c *fluent.GRIBIClient, t testing.TB) { +// success. The ACK type expected is validated against wantACK. +func DeleteNextHopGroup(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, func() { @@ -580,14 +702,14 @@ func DeleteNextHopGroup(c *fluent.GRIBIClient, t testing.TB) { }, } - res := doOps(c, t, ops, fluent.InstalledInFIB, false) - validateBaseTopologyEntries(res, t) + res := doOps(c, t, ops, wantACK, false) + validateBaseTopologyEntries(res, wantACK, t) chk.HasResult(t, res, fluent.OperationResult(). WithNextHopGroupOperation(1). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID()) @@ -595,17 +717,14 @@ func DeleteNextHopGroup(c *fluent.GRIBIClient, t testing.TB) { fluent.OperationResult(). WithIPv4Operation("1.0.0.0/8"). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID()) } -// DeleteNextHop attempts to delete the NH entris within the base topology and expects -// success. -// -// TODO(robjs): When traffic and AFT validation is added, ensure that a partial delete -// scenario keeps traffic routed via the remaining NH. -func DeleteNextHop(c *fluent.GRIBIClient, t testing.TB) { +// DeleteNextHop attempts to delete the NH entries within the base topology and expects +// success. The ACK type returned is validated against wantACK. +func DeleteNextHop(c *fluent.GRIBIClient, wantACK fluent.ProgrammingResult, t testing.TB) { ops := []func(){ func() { baseTopologyEntries(c, t) }, func() { @@ -618,25 +737,24 @@ func DeleteNextHop(c *fluent.GRIBIClient, t testing.TB) { }, } - res := doOps(c, t, ops, fluent.InstalledInFIB, false) - validateBaseTopologyEntries(res, t) + res := doOps(c, t, ops, wantACK, false) + validateBaseTopologyEntries(res, wantACK, t) for _, i := range []uint64{1, 2} { chk.HasResult(t, res, fluent.OperationResult(). WithNextHopOperation(i). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID()) - } chk.HasResult(t, res, fluent.OperationResult(). WithNextHopGroupOperation(1). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID()) @@ -644,7 +762,7 @@ func DeleteNextHop(c *fluent.GRIBIClient, t testing.TB) { fluent.OperationResult(). WithIPv4Operation("1.0.0.0/8"). WithOperationType(constants.Delete). - WithProgrammingResult(fluent.InstalledInFIB). + WithProgrammingResult(wantACK). AsResult(), chk.IgnoreOperationID()) } diff --git a/demo/ipv4/ipv4_test.go b/demo/ipv4/ipv4_test.go index fe41b45d..77e042a9 100644 --- a/demo/ipv4/ipv4_test.go +++ b/demo/ipv4/ipv4_test.go @@ -38,6 +38,6 @@ func TestDemo(t *testing.T) { } c := fluent.NewClient() c.Connection().WithTarget(*addr) - compliance.AddIPv4EntryRIBACK(c, t) + compliance.AddIPv4Entry(c, fluent.InstalledInRIB, t) time.Sleep(2 * time.Second) } diff --git a/device/device_test.go b/device/device_test.go index 19d8470b..07f6796c 100644 --- a/device/device_test.go +++ b/device/device_test.go @@ -88,7 +88,7 @@ func TestDevice(t *testing.T) { case addr := <-devCh: c := fluent.NewClient() c.Connection().WithTarget(addr) - compliance.AddIPv4EntryRIBACK(c, t) + compliance.AddIPv4Entry(c, fluent.InstalledInRIB, t) _, cidr, err := net.ParseCIDR("1.1.1.1/32") if err != nil { diff --git a/fluent/fluent.go b/fluent/fluent.go index 448c280b..b4e3c043 100644 --- a/fluent/fluent.go +++ b/fluent/fluent.go @@ -464,7 +464,14 @@ func (i *ipv4Entry) WithNextHopGroupNetworkInstance(n string) *ipv4Entry { return i } -// opproto implements the GRIBIEntry interface, building a gRIBI AFTOperation. ID +// WithMetadata specifies a byte slice that is stored as metadata alongside +// the entry on the gRIBI server. +func (i *ipv4Entry) WithMetadata(b []byte) *ipv4Entry { + i.pb.Ipv4Entry.Metadata = &wpb.BytesValue{Value: b} + return i +} + +// opproto implements the gRIBIEntry interface, returning a gRIBI AFTOperation. ID // and ElectionID are explicitly not populated such that they can be populated by // the function (e.g., AddEntry) to which they are an argument. func (i *ipv4Entry) opproto() (*spb.AFTOperation, error) { diff --git a/go.mod b/go.mod index bb850846..2c638862 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/openconfig/grpctunnel v0.0.0-20210610163803-fde4a9dc048d // indirect github.com/openconfig/ygot v0.11.0 go.uber.org/atomic v1.7.0 + google.golang.org/genproto v0.0.0-20201214200347-8c77b98c765d // indirect google.golang.org/grpc v1.37.0 google.golang.org/protobuf v1.26.0 lukechampine.com/uint128 v1.1.1 diff --git a/go.sum b/go.sum index 4081ad08..feea5022 100644 --- a/go.sum +++ b/go.sum @@ -83,6 +83,7 @@ golang.org/x/crypto v0.0.0-20200302210943-78000ba7a073/go.mod h1:LzIPMQfyMNhhGPh golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= +golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3 h1:XQyxROzUlZH+WIQwySDgnISgOivlhjIEwaQaJEJrrN0= golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -118,6 +119,7 @@ golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGm golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY= golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= +golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135 h1:5Beo0mZN8dRzgrMMkDp0jc8YXQKx9DiJ2k1dkvGsn5A= golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=