openconfig / gnmi

gRPC Network Management Interface
Apache License 2.0
459 stars 196 forks source link

Subscribe() requires a target while the spec says it shouldn't #151

Closed vincentbernat closed 11 months ago

vincentbernat commented 12 months ago

Hey!

Subscribe() requires a target, while the spec says it should not. In 2.2.2.1:

This field is optional for clients.

Commercial implementations do not require a target.

For context, I am attempting to use it to be able to test a gNMI client implementation into some opensource client, but I am unable to find a somewhat valid implementation of a gNMI server for tests. Lemming is requiring a target due to reusing this piece of code, gNXI is triggering errors when subscribing to paths not existing yet (while it should just ignore them), gnxi-simulators is relying on private repositories.

hellt commented 12 months ago

pinging @robshakir and @wenovus I'd also be curious to know if target could've been made optional in lemming, if we would want it to be used as a reference gnmi server impl.

gcsl commented 12 months ago

This should be an issue under Lemming. This particular Subscribe implementation is for a collector that hosts data from multiple targets and thus target is required to disambiguate the request.

wenovus commented 12 months ago

I agree that since the target field means something very different between the gNMI specification (essentially just associating a name with a stream of updates) vs. openconfig/gnmi's implementation, that lemming should modify this field prior to calling into this particular Subscribe's implementation. e.g. a server stream interceptor that modifies the field in order to always call a fixed target.

@vincentbernat feel free to open an issue in lemming.

Since a SubscribeRequest can subscribe to multiple paths, then this means it's also possible to have different paths within the same subscription to have different targets, such that we would expect different subscription updates to have different target values when streamed back. If/when we decide to support this in lemming as well, then we'll need to decide what happens when there are overlapping subscription paths with different targets.

@DanG100 for visibility

robshakir commented 11 months ago

@wenovus In practice, for a single device, I don't expect that you ever see >1 value of target in a subscription request, or any notification. The field is there to disambiguate multiple downstream targets when speaking to some gNMI endpoint that has knowledge of >1 downstream. In the case of lemming and an individual device, it's not really ever proxying for another device - but rather just returning values for itself. Thus each network device, and hence each lemming, would probably just use one value here.

wenovus commented 11 months ago

@wenovus In practice, for a single device, I don't expect that you ever see >1 value of target in a subscription request, or any notification. The field is there to disambiguate multiple downstream targets when speaking to some gNMI endpoint that has knowledge of >1 downstream. In the case of lemming and an individual device, it's not really ever proxying for another device - but rather just returning values for itself. Thus each network device, and hence each lemming, would probably just use one value here.

Thanks for the clarification. I think I'll convert any empty targets to the single assigned gNMI target for each lemming instance to fix this.

wenovus commented 11 months ago

Resolved at https://github.com/openconfig/lemming/pull/291