openconfig / gribi Goto Github PK

With recent go, when go modules are on by default, generating proto files are failing.

make
cd /home/user/gribi/proto/gribi_aft/enums && go generate
go: cannot find main module, but found .git/config in /home/user/gribi
	to create a module there, run:
	cd ../../.. && go mod init
make: *** [Makefile:5: generate] Error 1

Would the maintainers of this repo consider refactoring PR enabling the code generation with go modules?

Context is the below pull request:

• Clarify that when the election ID sent by a client is equal to the
  previous highest, the new client becomes the master.

Since election ID is the only field in AFTOperation that identifies that master client has sent the AFTOperation, how do we handle the situation if existing client which had the highest election ID lost it master role. Should that client's AFTOperation be failed even though the election ID sent in the AFToperation is equal to the last known election ID?

we are using gribi and aft to forward 5G GTP-U packets, the data plane destination look-up sequence uses the following network header fields:

• port (RAN facing)
• L2 (VSI & VLAN)
• IP-UDP tunnel (UDP port + outer IP source + destination) to identify a gNB-UPF tunnel
• GTP-U subscriber tunnel (TEID)
• Inner-IP routing (source + destination IP)

Using AFT model, each FT is logically separated and entry is added as oneof entry {...}; therefore grouping multiple FT as another FT is not feasible. The current implementation augments the gribi-aft.yang to support a gtp-u table as:
| +--ro afts
| | +--ro gtp-u
| | +--ro gtp-u-entry* [outer-ip-source outer-ip-destination teid]
| | +--ro outer-ip-source // -> ../state/outer-ip-source
| | +--ro outer-ip-destination // -> ../state/outer-ip-destination
| | +--ro teid // -> ../state/teid
| | +--ro state
| | | +--ro outer-ip-source? // -> ../../gtp-u-in-ip/state/outer-ip/ipv4/source-address
| | | +--ro outer-ip-destination? // -> ../../gtp-u-in-ip/state/outer-ip/ipv4/destination-address
| | | +--ro teid? // -> ../../gtp-u-in-ip/state/tunnel/teid
| | | +--ro traffic-class-mapping-profile? // -> ../../../../../../../qos/traffic-class-mapping-profiles/traffic-class-mapping-profile/config/name
| | | +--ro next-hop-group? // -> /oc-ni:network-instances/network-instance/afts/next-hop-groups/next-hop-group/state/id
| | | +--ro next-hop-group-network-instance? oc-ni:network-instance-ref
| | +--ro gtp-u-in-ip
| | | +--ro state
| | | +--ro l2
| | | | +--ro vlan? uint16
| | | +--ro outer-ip
| | | | +--ro ipv4
| | | | +--ro source-address? oc-inet:ipv4-prefix
| | | | +--ro destination-address? oc-inet:ipv4-prefix
| | | +--ro udp
| | | | +--ro port? oc-inet:port-number
| | | +--ro tunnel
| | | | +--ro teid? uint64
| | | | +--ro qfi? uint8
| | | +--ro inner-ip
| | | +--ro ip-address-set? ip-address-set. // enum for v4 or v6 ip
| | | +--ro ipv4
| | | | +--ro source-address? oc-inet:ipv4-prefix
| | | | +--ro destination-address? oc-inet:ipv4-prefix
| | | | +--ro dscp? oc-inet:dscp
| | | +--ro ipv6
| | | +--ro source-address? oc-inet:ipv6-prefix
| | | +--ro source-flow-label? oc-inet:ipv6-flow-label
| | | +--ro destination-address? oc-inet:ipv6-prefix
| | | +--ro destination-flow-label? oc-inet:ipv6-flow-label
| | | +--ro dscp? oc-inet:dscp

There are other augmentations like traffic-class-mapping-profile included here to enable per flow QoS, but ignore for now,, we will create a thread in QoS.

Looking forward to getting your feedback. Thanks

David

Wondering if there's any additional information on running fakeserver/fakeclient?
When trying to build fakeserver, had to make below change, and then go test works..
Am I missing anything? much appreciated.

$ go test github.com/openconfig/gribi/fakeserver
ok github.com/openconfig/gribi/fakeserver 0.192s

diff --git a/fakeserver/server.go b/fakeserver/server.go
index d6d1fd7..0eed829 100644
--- a/fakeserver/server.go
+++ b/fakeserver/server.go
@@ -20,13 +20,13 @@ import (
"context"
"sync"

•   "gob/gribi/oc"
  

•   "github.com/openconfig/gribi/oc"
  
    "github.com/openconfig/ygot/ygot"
    "google.golang.org/grpc/codes"
    "google.golang.org/grpc/status"
  

•   rpb "gob/gribi/proto/service"
  

•   rpb "github.com/openconfig/gribi/proto/service"
  
    gpb "github.com/openconfig/gnmi/proto/gnmi"
  

)
diff --git a/fakeserver/server_test.go b/fakeserver/server_test.go
index 46edb2e..61e3bfa 100644
--- a/fakeserver/server_test.go
+++ b/fakeserver/server_test.go
@@ -30,7 +30,7 @@ import (
"google.golang.org/grpc/reflection"
"google.golang.org/grpc/status"

•   rpb "gob/gribi/proto/service"
  

•   rpb "github.com/openconfig/gribi/proto/service"
  
    gpb "github.com/openconfig/gnmi/proto/gnmi"
  

)

In the specification.md both GetRequest message (section 4.2.1) and FlushRequest message (section 4.3.1) explicitly mention that network-instance name has to be specified; however, in proto files, we can see that network_instance is defined as oneof name/all:

  oneof network_instance {
    // The network instance from which the entries should be retrieved.
    // If `network_instance` is nil (`name` and `all` not specified),
    // or if `name` is specified and it is the empty string "",
    // then the request is considered invalid and the server should
    // respond with the INVALID_ARGUMENT error code in the status.proto
    // carried along with the RPC response.
    string name = 1;
    // all indicates that entries for all network instances for the
    // should be returned.
    Empty all = 2;
  }

Could you please clarify what is the expected behavior going forward, i.e. should the spec be updated to support "all" flag, or "all" is going to be eventually deprecated?

Thanks,

Reported by @nandanarista:

I did some digging and found at least one problem with reflection[1] for gRIBI (in general, not specific to EOS, problem occurs in gribigo[2] as well)

Focussing on the error below, I suspected it had something to do with reflection.

Method name not found
Failed to find method gribi.Flush in proto files.

I used

./grpcurl -H 'username: admin' -plaintext 127.0.0.1:6040 describe
Failed to resolve symbol "gribi.gRIBI": Symbol not found: gribi.gRIBI
caused by: File not found: github.com/openconfig/ygot/proto/ywrapper/ywrapper.proto

Some googling led me to a very helpful post [3] which says that

What that means is that the name (and relative path) used to compile a proto with protoc must exactly match how all other files will import it.

ywrapper.proto itself is compiled [4] using just the relative // source: ywrapper.proto, but gribi_aft.proto is importing it as import "github.com/openconfig/ygot/proto/ywrapper/ywrapper.proto”;

If my understanding of [3] is correct, the fix is to either regenerate ywrapper's go bindings to use the full github… path or change gribi_aft.proto to import ywrapper without the full github... path and regenerate the go bindings.

I tried both the grpcurl describe and grpc_cli call on gribigo after adding reflection to it, and I see the same problem.

My suggestion is that you file a bug and someone (maybe Nathaniel or Rob) fix this so that reflection (grpcurl describe) works with gribigo , update the generated go bindings on GitHub, and when that’s done, I can rebuild EOS’s gribi server to fix it in EOS and have it included in a later code drop.

[1] https://github.com/grpc/grpc-go/blob/master/Documentation/server-reflection-tutorial.md#grpc-server-reflection-tutorial
[2] https://github.com/openconfig/gribigo 
[3] https://github.com/fullstorydev/grpcurl/issues/22#issuecomment-375274465 
[4] https://github.com/openconfig/ygot/blob/master/proto/ywrapper/ywrapper.pb.go#L25
[5] https://github.com/openconfig/gribi/blob/master/v1/proto/gribi_aft/gribi_aft.proto#L13

Below is part of gribi.proto file. Ipv4Entry, ipv6Entry, and all appear to be missing key information. If my understanding is correct, they should be Ipv4EntryKey, Ipv6EntryKey, and so on.

    oneof entry {
      gribi_aft.Afts.Ipv4Entry ipv4 = 4;
      gribi_aft.Afts.Ipv6Entry ipv6 = 5;
      gribi_aft.Afts.LabelEntry mpls = 6;
      gribi_aft.Afts.NextHopGroup next_hop_group = 7;
      gribi_aft.Afts.NextHop next_hop = 8;
    }

In the HLD document in section TE-3 (Support for Hierarchical Lookups) it is mentioned that the NHGs and NHs will be sent in a single ModifyRequest. The test case and the requirement in the HLD seems to be contradictory. Can you please clarify?

1. In case of hardware error while programming a route, should gRIBI ModifyResponse contain AFTResult with FIB_FAILED or FAILED? (Comment suggests FAILED in line 286).
2. If Nexthop is programmed with interface in down state, should AFTResult be FIB_FAILED or RIB_PROGRAMMED.

Hi

the current set of error codes defined for Flush() RPC are input validation error codes. Flush as an operation can fail on the device and there are no error codes defined for that.

The failures could be due to server's inability to process flush() or due to misordered use of flush(), such as attempting to flush() network_instance X, when it still has objects being referenced by objects in network_instance Y.

I suggest two error codes be added
a) FAILED to indicate flush() as an operation failed on the device
b) INCOMPLETE to indicate device attempted to flush network_instance, but not all objects in network_instance could be flushed as they have references from other network instances.

thanks

The benchmark get test case takes seems to be taking more time for every get call for successive for loop iterations (https://github.com/openconfig/gribigo/blob/main/compliance/get.go#L345)

In the first for loop iteration, while trying to add 10 nhs each get call took 0.00043167 (averaged over 10 calls)
In the second for loop iteration, while trying to add 100 nhs each get call took 0.2018121 (averaged over 100 calls)
In the third for loop iteration, while trying to add 1000 nhs each get call took 0.40329421 (averaged over 49 calls)
All the 1000 nhs couldn’t be added because the test case timed out after one minute.

When I debugged this code I observed that we are losing on time waiting for the mutex:
https://github.com/openconfig/gribigo/blob/main/client/gribiclient.go#L1041
the time spent waiting for the mutex lock keeps increasing after every for loop iteration as can be seen above.

The analysis was done sometime back. So, I do not know if this is still seen in the latest code base.

PolicyForwardingEntry message defined in gribi/v1/proto/gribi_aft/gribi_aft.proto is missing source ip prefix.

For programming AFT nexthop with mac-address, are there any restrictions in using reserved mac-address , like 01:80:C2:XX:XX:XX, 00-00-5E-XX-XX-XX? Since these mac addresses are not meant for data packets and may be interpreted as PDU in pre-classification in receiving node, is it recommended to add restriction for such reserved mac address range?

It's observed that gRIBI should be preferred over other dynamic routing protocols, but that static should have higher precedence than gRIBI. However, the gRIBI motivation and protofile do not reference precedence, preference or static routes.

If election message is missing in the Flush request (neither id nor override is set), it will be good to send an error from server. A new error for this needs to be defined.

If override is set to FALSE, this should also be an error and a new value needs to be defined in FlushResponseError Reason enum.

// If the client redundancy mode is ALL_PRIMARY, but a client
// sends election_id, the network element closes the connection
// to the client and responds with FAILED_PRECONDITION in
// status.proto's code and sets ModifyRPCErrorDetails.reason
// to ELECTION_ID_IN_ALL_PRIMARY

When a client makes an invalid request via ModifyRequest message, is the server endpoint supposed to terminate the client connection after sending an error status?

In almost all the gRPC server examples I have looked, the server has only RPC specific context, don't see a mechanism to initiate disconnection of the client. Is there any gRPC example where the server initiates the client termination as well after sending error on the RPC, that would be helpful for reference.

Today in the FlushRequest, if election doesn't specify a id nor override in single primary mode, there's no Reason enum for this error. Does it make sense to add a specific enum like NO_ELECTION_ID_OR_OVERRIDE to ModifyResponseError.Reason ?

Message LabelEntryKey has both enum and oneof with same name but case is different.

This will result in redeclaration error for C stubs generated through protobuf-c, more details here protobuf-c/protobuf-c#389

timestamp field doesn't specify what units and reference - should be nanos since epoch.

Hi

Assuming session parameters has been set to AFTResultStatusType.RIB_AND_FIB_ACK.

The client programs the server a set of objects using a sequence of AFTOperations. It could be that some of these operations fail in FIB (or dataplane). What should GET() respond with when there are errors from FIB ?

a) If the server's response set consists of all objects programmed by the client, then client does not know which objects have been successfully programmed in FIB
b) If the server responds only with objects that have been programmed successfully in FIB (and dataplane), the client does not know the full set of objects previously programmed to the device, leaving device with potentially stale objects - if the object is missing/not needed in the new life of the client.

We suggest that the Get() response should consist of:
a) complete set of objects programmed through GRIBI that server has - for the requested AFTType
b) The response message per object should also carry the current status of RIB or FIB programming

If above is acceptable, it should also be clarified that with AFTPersistence.PRESERVE mode, the GET() would respond based on the last AFTResultStatusType used to program the object. Perhaps the GET() response should also carry the AFTResultStatusType if there can be uses where clients were to switch from one AFTResultStatusType to another while still maintaining AFTPersistence.PRESERVE.

thanks

Modifying ipv4 entry as defined in the ACL.YANG is not supported in current proto, same as v6 and L2 mac entries, will they be added or the intention is to use the protobuf from ACL.YANG as input to gRIBI. Thanks

David

 |           +--rw ipv4
 |           |  +--rw config
 |           |  |  +--rw source-address?        oc-inet:ipv4-prefix
 |           |  |  +--rw destination-address?   oc-inet:ipv4-prefix
 |           |  |  +--rw dscp?                  oc-inet:dscp
 |           |  |  +--rw dscp-set*              oc-inet:dscp
 |           |  |  +--rw protocol?              oc-pkt-match-types:ip-protocol-type
 |           |  |  +--rw hop-limit?             uint8

In a scenario where a client skips session parameters negotiation, and starts with a ModifyRequest with an AFToperation only, should the successful operation be acknowledged with a deprecated OK status as suggested by these lines in .proto

Or, as could be implied from the specification.md, we should use RIB_PROGRAMMED instead (since this is the default ack mode)