openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

JSON format for gNMI Set on a specific list node vs. wildcard list node #139

Open wenovus opened 3 years ago

wenovus commented 3 years ago

Background

RFC7951 specifies the JSON format for a list node, but fails to specify the format for a specific element in a list node (e.g. /interfaces/interface[name="foo"]). The gNMI spec also fails to provide an example for this case.

My Questions

  1. Is gNMI Set on a specific list node allowed (e.g. SetRequest.replace.path is populated with /interfaces/interface[name="foo"])?
  2. If so, what should be its JSON format?
aashaikh commented 3 years ago

Since the RFC specifies the list encoding to be prefixed by the list name, wouldn't we end up having the data payload to be something like

"interface": [
  {
    "name": "foo",
    "config": {
      "description": "bar"
    }
  }
]

If this was a replace operation, it would of course clear the rest of the list element content back to empty/default.

robshakir commented 3 years ago

I don't quite agree with @aashaikh here.

If we have a path that refers to an object within a list - which is defined to be an array of objects - then the encoding for a specific entry in a list should be a JSON object. Essentially a list is treated as an indexable array/slice of containers - so using a JSON object means that we're consistent with that interpretation. This seems to be what RESTCONF does from a few examples elsewhere too - so we're consistent there.

So in the OpenConfig interfaces schema, I think we would have:

path: /interfaces/interface[name=eth0] value:

{
  "name": "eth0",
  "config": {
     "name": "eth0",
     "description": "ethernet zero"
  }
}
robshakir commented 3 years ago

This also seems more logical than having a replace to something more specific than the list node clearing what appear to be unrelated entries.

For example, replace /interfaces/interface[name=eth0] replacing /interfaces/interface[name=eth1] would be quite unexpected to me.

aashaikh commented 3 years ago

We should definitely do what is consistent with well-understood behavior here -- so the object rather than array SGTM.

But it seemed a little odd that the list is encoded as an array, but a single element in the list would be an object rather than a single item array containing the object.

robshakir commented 3 years ago

I think this is consistent with what you would expect in most languages though, right? For example if I write:

int n[5];

n = {0,1,2,3,4}; // refer to the array as an array
n[0] = 1; // refer to an element of the array as the included type

Or:

var x []int
x = []int{0,1,2,3,4} // refer to x as a slice
x[0] = 1 // refer to an element as a int

I think the distinction here is the data tree is essentially creating a virtual level here - the array itself is a parent of its children (members of the list), so the two paths refer to different things. This virtual level doesn't exist in the schema tree.

aashaikh commented 3 years ago

thanks , this explanation make sense. I think I was concerned that treating as a standalone object loses the context that it is part of a list. But for the data tree, it's actually not important.

wenovus commented 3 years ago

One follow-up question is what should be the RFC7951 JSON format?

RFC7951 always adds a context name for the JSON, and since the level is virtual, it's unclear to me what the context name should be. I think this is where Anee's suggestion might make sense, since if you make the prefix the name of the list, then it could be confusing that what's underneath is not an array, but an object.

kthex commented 3 years ago
So what JSON payload should be specified in the following cases? Path JSON payload
/interfaces/interface[name=eth1] an object?
/interfaces/interface an array?
/interfaces an object with an array inside?
robshakir commented 3 years ago

In my opinion:

sachinholla commented 3 years ago

{"name": "eth1", "config": { "description": "interface"}} and [{"name": "eth1"}, {"name": "eth2"}] are not a valid RFC7951 JSONs.

RFC clearly says list instance is encoded as a name/array pair. So, single interface instance should be encoded as {"interface": [{"name": "eth1", "config": { "description": "interface"}}]}. I feel gNMI should stick to the RFC standard for JSON_IETF encoding. This would enable easy transition from RFC7951 JSON based services (like RESTCONF) to gNMI.

robshakir commented 3 years ago

Can you point out where in the spec it says these? I didn't interpret it the same.

sachinholla commented 3 years ago

RFC7951, section 5.4:

A list instance is encoded as a name/array pair, and the array elements are JSON objects.

In fact all yang nodes, including leaf or anydata nodes, are encoded as name/value pairs. JSON values like {"name": "eth1", "config": { "description": "interface"}} do not fit into any of the RFC formats.

wenovus commented 3 years ago

RFC7951, section 3:

This document defines JSON encoding for YANG data trees and their subtrees.

Despite the above, I personally think the RFC failed to consider the full JSON RPC representation of a subtree, leading to the problem below:

A list instance is encoded as a name/array pair, and the array elements are JSON objects.

The above requirement makes sense when marshalling a full datatree (see section 4, which doesn't have a qualifying name because the JSON starts at the root node); however, the "name" part of the requirement becomes either redundant, or awkward, when marshalling a subtree. That is, either the path already specifies the name of the subtree node, or the path ends at the parent, which is confusing and probably why gNMI Set decided to include the node's name as part of the path. The question of how to use a path to identify which subtree a piece of JSON data represents is not addressed by the RFC. I'm going to guess this is how this problem came about.

sachinholla commented 3 years ago

I fully agree with you that context node is redundant and looks awkward when you have a path to point to a subtree. I guess RFC7951 authors tried to mimic YANG XML mapping rules. A leaf is encoded as <name>eth1</name> and a list instance would be <interface><name>eth1</name> ... </interface>. Maybe RFC7951 used the context node to have similar look and feel.

However, its is better to honor RFC7951 rules for JSON_IETF encoding. It is followed by many other standards, including YANG1.1 and RESTCONF. Will help easy transition from other management systems to gNMI.. In fact gNMI spec section 2.3.1 says:

JSON_IETF encoded data MUST conform with the rules for JSON serialisation described in RFC7951.

gsindigi commented 3 years ago

Considering the conversations happening in this regard, will it be any different for gnmi.Subscribe/Get with JSON encoding?

I had raised Should TogNMINotifications consider Encoding as well? in the past. This is applicable when the client requests the data to be JSON encoded.

anand-kumar-subramanian commented 2 years ago

@robshakir @aashaikh What is the final conclusion here? Are we going to stick to the standard set by RFC7951 for the gNMI spec as well?