proto updates for PROBE (RFC8335)

[fenner]

This is a new RPC to request a remote PROBE session. This is heavily based on PingRequest/PingResponse, modified for the PROBE semantics ( https://datatracker.ietf.org/doc/html/rfc8335 ).

JunOS and Arista EOS have their own Probe clients; there's a WIP at https://github.com/aristanetworks/iputils/tree/probe-client to make the iputils "ping" client able to send PROBEs.

[coveralls]

Pull Request Test Coverage Report for Build 8918035572

Details

• 22 of 415 (5.3%) changed or added relevant lines in 2 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.02%) to 1.22%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
system/system_grpc.pb.go	0	46	0.0%
system/system.pb.go	22	369	5.96%

Files with Coverage Reduction	New Missed Lines	%
system/system.pb.go	1	2.82%

Totals
Change from base Build 8911885186:	0.02%
Covered Lines:	172
Relevant Lines:	14097

---

💛 - Coveralls

[LimeHat]

Is this really needed?
gNOI client can do the math, adding a summary to each response seems unnecessary

[fenner]

That's true - other than the errors, all of these values can be derived from the set of responses. I copied this concept from the "ping" response, which also includes summary values that can be derived from the set of responses. If it's useful there, then I thought it would be useful here.

[LimeHat]

Personally, I think the direct streaming of responses is a better approach, but if you want to provide an option to return the summary, I would suggest refactoring this to use the oneof model and splitting it into two message types.

[fenner]

As I said, I modeled this after "ping" to keep it similar since the protocols are similar. Should "ping" also use oneof?

[LimeHat]

Should "ping" also use oneof?

Maybe :-) But it's a bit too late to change that, and not the point.

In this case, the state machine is significantly more complex than in ping rpc. IMO it's easier to write a client that doesn't have to infer the meaning of the message (a summary vs an individual response) based on field values, especially considering that you mostly have independent fields in each case. That also automatically answers questions such as "what fields are expected or allowed to be set together, what are valid/invalid combinations".

Is there any concern about this?

[fenner]

I can make the change, I'm just concerned about creating a difference from the ping RPC. I figured that the ping definition established how this kind of RPC was desired to be expressed in GNOI. (For example, I used an int64 for the time, even though uint64 is a better fit for an absolute timestamp, because that's what ping did).

[LimeHat]

I would suggest mapping the v4/v6 bits directly to individual fields, like it's done in the RFC.

[fenner]

This structure was used based on the section A.1 that was added to the draft RFC update based on user feedback.

https://www.ietf.org/archive/id/draft-fenner-intarea-probe-clarification-00.html#name-information-display

While I think it's possible to create a tool that performs the GNOI transaction and presents the information to the user, I've seen users run the RPCs and just want to be able to interpret the raw json results, so thought it was better for the json results to be in a presentation format.

[LimeHat]

so thought it was better for the json results to be in a presentation format.

I'm not sure if the value 3 is more useful to the user than active true, ipv4 true, ipv6 false? :-)

[fenner]

I assumed that the gnmi client would translate enum values, so you would get ACTIVE_V4 - but I admit ignorance on this front.

Another reason to use this enum is for the histogram summary - a histogram of "5 active, 3 ipv4, 1 ipv6" doesn't really tell you whether ipv4+ipv6 were ever active together. (I know you don't see the point of the summary data, so this won't be a particularly convincing argument).

[robshakir]

Generally LGTM.

[robshakir]

For the ifindex, ifname and ifaddr -- is there some definition that we can use that ties into the ecosystem more generally? Is ifname equal to the OpenConfig /interfaces/interface/state/name for example?

[fenner]

Well, https://fenner.github.io/probe-clarification/draft-fenner-intarea-probe-clarification.html refers to interface name and ifindex from https://www.rfc-editor.org/rfc/rfc8343 - I'm pretty ignorant of the details of ietf netmod vs. openconfig models. I'd be happy to put in appropriate references, though, if you can help me find them.

ifaddr is expected to be the IP address assigned to some interface (presumably a state lookup, not a config one).

remote is expected to be an entry in the ARP or ND table.

The reference is also a little tricky because these are not values on the node that the RPC is being sent to, but instead, values on the destination that's being specified in the RPC.

[fenner]

@robshakir just to clarify what you're looking for here: do you want comments like

message ProbeRequest {
  string destination = 1;   // Proxy address to probe. required.
  string source = 2;        // Source address to probe from.
  oneof probe_target {
  oneof probe_target {
    // exactly one of the next 4 items must be present.
    // ifIndex to probe, if present.  Equal to the value of
    // /interfaces/interface/state/if-index for the requested
    // interface on the destination node.
    int32 ifindex = 3; 
    // ifName to probe, if present.  Equal to the value of
    // /interfaces/interface/state/name for the requested
    // interface on the destination node.
    string ifname = 4;
    // ifName to probe, if present.  Equal to the value of
    // /interfaces/interface/<... IP address assigned ...> for the requested
    // interface on the destination node.
    string ifaddr = 5;
    // Remote address to probe, if present.  Equal to the value of
    // /neighbors/something/<... ARP or ND table entry ...> for the requested
    // remote neighbor on the destination node.
    string remote = 6;
  }

where obviously I'd fill in the right paths for "interface IP address" and "ARP/ND table" before publishing a revision