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

From: Robert Wilton via Datatracker <[email-masked]>
Date: Thu, 15 Jul 2021 06:35:22 -0700

Robert Wilton has entered the following ballot position for
draft-ietf-core-yang-cbor-16: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-core-yang-cbor/

----------------------------------------------------------------------
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.

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.

3. Did the WG consider supporting encoding YANG metadata (RFC 7952)? 
Presumably this would be expected to be covered as future work?

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).

5. I also support Ben's second discuss point.  I think that as written, this
draft needs a normative reference to the SID draft.

----------------------------------------------------------------------
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.

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.

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?

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.  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.

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).

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).

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.

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.

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.

Regards,
Rob