Re: [core] Robert Wilton's Discuss on draft-ietf-core-yang-cbor-16: (with DISCUSS and COMMENT)

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Carsten, all,

Thanks for working through my comments, your updates look good, the only two discuss points that I think that we may need further discussion on relate to assigning SIDs to features, and a question arising from the normative reference.

Whilst checking the changes (between -16 and -18), I did notice a couple of things that I have flagged, in case you want to tweak the text further:

(1) Section 3.2:

                                                 Where absolute SIDs are	
 	   desired in map key positions where a bare integer implies a delta,	
 	   they may be encoded using CBOR tag 47 (as defined in Section 9.3).
       
Just to check, is may rather than MUST correct here?  I.e., if you want to use an absolute SID where a delta is expected then it must use CBOR tag 47, or otherwise the receiver would not know it is an absolute SID?

(2) 
	  *  application/yang-data+cbor; id=sid -- for use by applications that	
 	      need to be frugal with encoding space and text string processing	
 	      (e.g., applications running on constrained nodes [RFC7228], or	
 	      applications with particular performance requirements);	
 		
 	   *  application/yang-data+cbor; id=name -- for use by applications	
 	      that do not want to engage in SID management, and that have ample	
 	      resources to manage text-string based item identifiers (e.g.,	
 	      applications that directly want to substitute application/	
 	      yang.data+json with a more efficient representation without any	
 	      other changes);

I think that it is fairly obvious from their names, but the definitions don't technically restrict what type of ids are used in the content.  Should they be more explicit on this?

(3) Related to 2, I did wonder if the document could possible be clearer about what is allowed when mixing name and sids, perhaps as an extra sentence in the third paragraph of section 3?  As I understand it, barring the media-type, anything goes, e.g., a container or list entry could contain some entries encoded with name ids and some with sids, e.g., which could be required if handling an augmented module that doesn't have sids allocated.

I'm happy to leave it to your discretion to decide whether and how to act on the three comments above.

I've put some comments inline, the only two that I think require attention are the ones related to discuss points 4 and 5.



> -----Original Message-----
> From: iesg <[e-mail]> On Behalf Of Carsten Bormann
> Sent: 20 December 2021 03:51
> To: Rob Wilton (rwilton) <[e-mail]>
> Cc: [e-mail]; [e-mail]; The IESG
> <[e-mail]>; [e-mail]; Marco Tiloca <[e-mail]>
> Subject: Re: Robert Wilton's Discuss on draft-ietf-core-yang-cbor-16: (with
> DISCUSS and COMMENT)
> 
> Hi Rob,
> 
> here is finally the response to your feedback, based on yang-cbor-18:
> 
> https://www.ietf.org/rfcdiff?url2=draft-ietf-core-yang-cbor-17.txt
> https://www.ietf.org/rfcdiff?url2=draft-ietf-core-yang-cbor-18.txt
> 
> > On 2021-07-15, at 15:35, Robert Wilton via Datatracker <[e-mail]>
> wrote:
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > Thanks for this document, it is good work, and I think that there
> specification
> > is almost there, but that the text could be tightened up in a few places.
> >
> > 1. The document should be clearer on its use of terminology around
> schema
> > nodes.  Mostly the encoding related to YANG data nodes, not YANG schema
> nodes.
> > I've provided more information in the comments section.
> 
> Right.
> Neither schema node nor data node are the term we need most.
> We have introduced the term “representation node” for instances of schema
> nodes that turn up in representations.

Yes, that sounds good.  It would have been nice if the base spec was clearer on this.


> 
> > 2. As also raised by Ben, this document should probably cover the YANG
> data
> > structure extension in RFC 8791.  This could potentially be done in addition
> to
> > rc:yang-data, but perhaps better in its place.
> 
> For the internal usage of YANG in the two drafts:
> We changed the ietf-sid-files module to use sx:structure.
> https://github.com/core-wg/yang-cbor/pull/116
> This is about the documents’s *use* of rc:yang—data migrating to
> sx:structure.
> 
> As far as showing sx:structure examples with encoding from YANG-CBOR:
> We have introduced sx:structure as another representation node, equivalent
> with container.
> This should cover all aspects of representing sx:structure.
> 
> We do not plan to remove the support for rc:yang-data.

Sounds reasonable.


> 
> > 3. Did the WG consider supporting encoding YANG metadata (RFC 7952)?
> > Presumably this would be expected to be covered as future work?
> 
> The design team did; we did not discuss this on the WG list.
> Supporting RFC 7952 metadata is a non-trivial effort, and we wouldn’t want
> that as a late addition to the present document.
> We do plan to set up another document that defines the CBOR
> representation of metadata, but not to include this in the present document
> (or round of documents).

That sounds look a good approach.


> 
> > 4. How does the CBOR encoding of SIDs apply to YANG features?  This draft
> > references features and the SID draft allows SIDs for them, but I don't
> > understand how they are used in the encoding (since features don't appear
> in
> > the instance data, they are only at the schema level).
> 
> YANG features can occur in the data of a YANG module, e.g., yang-library
> (RFC 8525).  Using a SID instead of an identifier for a feature allows a
> compact representation.

I'm still not really understanding how this would work.

The YANG library data carries the feature as a "yang:yang-identifier", which has a string as its base type.

So, unless there was some special handling either for the 'feature' leaf-list in YANG library, or for the encoding of "yang:yang-identifier" to allow the use of SIDs, I think that this would naturally just end up using a string encoding of the feature name (along with all other yang:yang-identifiers).

I can see that allocating SIDs to features does no harm, but I still don't currently see how they can be used.

> 
> > 5. I also support Ben's second discuss point.  I think that as written, this
> > draft needs a normative reference to the SID draft.
> 
> I don’t necessarily agree, but this is an easy change without negative
> consequences.

Okay, I can probably see where you are coming from, in that the intention is that the SID draft just defines a file format and allocation strategy, but the core definition of a SID is contained in the CBOR draft.

The SID draft constraints SIDs to being 63 bits.  Should that text/constraint move to the CBOR draft instead?


> 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > 1. Abstract:
> > This document defines encoding rules for serializing configuration
> >  data, state data, RPC input and RPC output, action input, action
> >  output, notifications and the yang-data extension defined within YANG
> >  modules using the Concise Binary Object Representation (CBOR, RFC
> >  8949).
> >
> > When I read the abstract, it raises the question about what is left out.
> > Hence, I am wondering whether it wouldn't be better to just describe it as
> > encoding YANG data tree instance data.  However, I see that the
> description
> > effectively mirrors the abstract for the YANG JSON encoding (RFC 7951), so
> it
> > is at least consistent.
> 
> We discussed this and do want to stay consistent with RFC 7951.

Okay.

> 
> > 2. I find this document (along with the SID draft), somewhat confusing in
> that
> > it references YANG schema nodes, but in most cases, it probably only cares
> > about the subset of YAND schema nodes that represent itemsthat are
> present in
> > data tree instantiation.  RFC 7950 calls such 'schema nodes' as 'data nodes'.
> > For example, 'choice', 'case', 'input' and 'output' exist as nodes in the YANG
> > schema tree, but are not data nodes and don't appear as instantiated data
> nodes.
> 
> The representation may also include information about schema nodes for
> action, rpc, input, output, and notification.  We introduced a new term,
> representation node, for this subset of schema nodes (see above).

Okay.

> 
> > Where this causes problems are in the definitions of: 'child' - (e.g., 'case'
> > cannot be a child of a 'container' only a child of a 'choice', and equivalently
> > for the definition of 'parent'.  This definitions are not heavily used and
> > could perhaps be removed?
> 
> We removed the definition of “child”, as it was used in only one place and
> that could easily be substituted by “schema node defined within”.
> We left the term “parent”, as this is used seven times, and clarified it to
> mean The schema node of the closest enclosing representation node.
> 
> > Hence my suggestion to clarify the text are:
> > - potentially import and use data node rather than schema node.  Note that
> > data node, as defined in RFC 7950, is a subset of schema nodes, so you still
> > need the text saying an instance of a data node.
> 
> We do import data node and already used it occasionally in -16.
> By the way, in the whole of YANG, I can only find RFC 7952 that ever talks
> about an “instance of a data node”, so I’m a bit reluctant to use that
> terminology throughout — any problem with using “data tree node”?

I think that "data tree node" is clear and unambiguous.

> 
> > Arguably, the RFC 7950
> > definition of a data node is somewhat confusing. - please check
> everywhere
> > where 'schema node' or 'data node' turns up in the text to ensure that you
> are
> > referencing instances of that schema node rather than the schema node
> itself.
> > E.g., the second paragraph in section 3 states 'where each child schema
> node
> > is encoded ...', but this should be an instance of child schema node. -
> ensure
> > that if you are referring to the parent or child of a 'schema node' that the
> > logic skip out the schema nodes that don't get encoded in the data tree.
> > E.g., when calculating SIDs.
> 
> This was covered by introducing the term “representation node” discussed
> above; please see updated introduction to Section 3.

Looks good.


> 
> > 3. I think that some of the references to submodules are not quite right.
> > Basically, the CBOR encoding should not need to concern itself about
> submodules
> > at all, since logically, it works with the module schema (which logically
> > incorporates the submodules).  E.g., perhaps you could mention this in the
> > introduction, referencing section 4.2.1 (4th paragraph in particular) of RFC
> > 7950?
> >
> > Hence:
> > In section 2:
> >  *  item: A schema node, an identity, a module, a submodule, or a
> >     feature defined using the YANG modeling language.
> >
> > I think that this should exclude submodule (and possibly feature as well).
> 
> Submodule is mostly gone from -18, except for paragraphs such as:
> 
> >> Note that any structuring of modules into submodules is transparent to
> YANG-CBOR:
> >> SIDs are not allocated for the names of submodules, and any
> >> items within a submodule are effectively allocated SIDs as part of
> >> processing the module that includes them.
> 

Looks good.


> 
> > In section 3.2:
> >  YANG modules, submodules, and features
> >
> > I don't think that you want submodules in this list (and perhaps not
> features
> > either).
> 
> Indeed, we removed all instances of “submodule” in lists of nodes.
> (See also reply to “features” above.)
> 
> > And in:
> >       6.13.1.  SIDs as instance-identifier
> >
> >          SIDs uniquely identify a schema node.  In the case of a single
> >          instance schema node, i.e., a schema node defined at the root of a
> >          YANG module or submodule or schema nodes defined within a
> container,
> >          the SID is sufficient to identify this instance.
> >
> > I would remove "or submodule" from this text.  Effectively, the text about
> > modules already covers this.
> 
> (See above.)
> 
> > 4. Please check the text that describes how lists are encoded.  Section 4.2
> > seems to suggest that they are encoded as a CBOR Map, section 4.4 states
> that
> > they are encoded as an array.  I presume that the answer is that they
> encode
> > the list is an array, and each list entry is a Map.
> 
> We now consistently use “list entry” (Section 7.8 of RFC 7950) for
> representation nodes that form a list.
> 
> > 5.
> >  Values of 'bits' types defined in a 'union' type MUST be encoded
> >  using a CBOR text string data item (major type 3) and MUST contain a
> >  space-separated sequence of names of 'bits' that are set
> >
> > It might be helpful to have a quick sentence to justify why this is done.
> 
> We added references to the section 6.12 in 6.6 and 6.7.
> We added some explanation in 6.12.
> Fully explaining the unfortunate idiosyncrasy is a bit beyond the scope of this
> specification, though.

What you have added looks good.

Thanks,
Rob

> 
> Grüße, Carsten