[netmod] js review of draft-ietf-core-yang-cbor-12

[Juergen Schoenwaelder <[e-mail]>]

Hi,

here is my review of draft-ietf-core-yang-cbor-12. I have not checked
the examples in detail, I assume (hope?) the authors have used tools
to generate them or to validate them.

- Nit: There is no need to capitalize Action in the abstract (and
  elsewhere).

- You should perhaps write 'yang-data extension' instead of 'yang data
  template' since this is more concrete and less confusing. The use of
  'template' is a bit unfortunate in RFC 8040 since templates have
  been used with different meanings elsewhere and I believe people
  more commonly refer to the yang-data extension. I also checked
  draft-ietf-netmod-yang-data-ext-05.txt and it does not use the word
  "template" anymore.

  I see that you use 'YANG data template' later on in several places.
  This is not strictly incorrect but I find it unfortunate. I would
  prefer to talk specifically about the yang-data extensions. You
  actually import both terms, I believe only one is needed (and I
  prefer yang-data (extension) over YANG data template.

- If this work gets approved, will other specifications like
  draft-ietf-netmod-yang-data-ext-05.txt be expected to cover CBOR
  encoding in addition to XML and JSON? This is more a procedural
  question.

- Wording:

     A new set of encoding rules has been defined to allow the use of the
     same data models in environments based on the JavaScript Object
     Notation (JSON) Data Interchange Format [RFC8259].  This is
     accomplished in the JSON Encoding of Data Modeled with YANG
     specification [RFC7951].

  What is "environments based on the JavaScript Object Notation (JSON)
  Data Interchange Format". Perhaps do not even try to speculate about
  reasons. What about this?

     An additional set of encoding rules has been defined based on the
     JavaScript Object Notation (JSON) Data Interchange Format
     [RFC8259].

- Is there a reason why SID terminology is not imported from the SID
  specification? Is the reason to avoid a dependency? But then, can
  this dependency really be avoided? I reviewed the SID document first
  because I thought knowing what SIDs are is essential to understand
  this document...

- I am not sure I would call a notification or a container a
  collection. Note that RFC 7950 uses the phrase child node quite a
  bit in definitions, so this may do the same without using the
  (overloaded) word collection:

   o  child: A schema node defined as a child node of a
      container, a list, a case, a notification, an RPC input, an RPC
      output, an action input, an action output.

   I searched the text and 'child' only shows up once and I am not
   sure this deserves to define a term at all.

- Is 'parent' the same as an 'interior node' defined in RFC 7950? If
  so, why not use 'interior node'? Looking at the uses or parent, I am
  not even convinced this term needs to be defined at all, the context
  seems to be rather clear of what is meant. RFC 7950 does not define
  'parent' as term either but the meaning is clear in the contexts
  where the word is being used.

- To summarize the last few comments, I propose to import 'item' and
  'SID' from the SID document, to not define 'child' and 'parent'
  (following RFC 7950), and so the only term to defined here is
  'delta'. But see above concerning the relationship to the SID
  document; it is not clear to me what the goals and intentions are in
  terms of intended document dependencies.

- schema trees vs data trees

   This document defines CBOR encoding rules for YANG schema trees and
   their subtrees.

  You are not encoding schema trees, you are encoding data trees. See
  also the JSON document, which says:

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

  Why did you change this from data trees to schema trees?

- avoid 'collection'?

  s/A collection such as/A data node such as/

- If I were to use CBOR with RESTCONF, can I request (e.g. via a
  specific media-type) how I like the keys to be encoded? Or would
  such a mechanism have to be specified elsewhere, i.e., we would need
  another using CBOR with RESTCONF document that defines suitable
  media types?

- I do not understand this statement:

   Application payloads carrying a value serialized using the rules
   defined by this specification (e.g.  CoAP Content-Format) SHOULD
   include the identifier (e.g.  SID, namespace qualified name,
   instance-identifier) of this value.

  What is "the identifier of this value"? I am not getting what
  is being conveyed here.

- s/section Section 4/Section 4/

- SIDs other than [I-D.ietf-core-sid]?

     [...] If SIDs are to be used, the present specification is
     used in conjunction with a specification defining this management.
     One example for such a specification is [I-D.ietf-core-sid].

  This seems to indicate that there can be other kinds of SIDs or SIDs
  managed differently. Why is this? The SID I-D claims the entire
  number space, so how would a different 'specification defining the
  management of SIDs' ever work? Why not be specific that the usage of
  SIDs depends on [I-D.ietf-core-sid]? See my earlier comments about
  the unclear dependency relationship between this specification and
  the SID specification.

- Avoid 'collections'?

    4.2.  The 'container' and other collections

    Collections such as containers, list instances, notification

  Rewrite to:

    4.2.  The 'container' and other interior data nodes

    Interior data nodes such as containers, list instances, notification

  There are more uses of collections following that likely can be
  replaced with 'interior data nodes'. The phrase 'interior data node'
  is used in RFC 7950 (although not formally defined).

- Should

       77 : {                    / event (SID 60200) /

  be

       77 : {                    / example-port-fault (SID 60200) /

  Similarly

       47(60200) : {             / event (SID 60123) /

  be

       47(60200) : {             / example-port-fault (SID 60200) /

- Replace

  5.  Encoding of YANG data templates

   YANG data templates are data structures defined in YANG but not
   intended to be implemented as part of a datastore.  YANG data
   templates are defined using the 'yang-data' extension as described by
   [RFC8040].

   YANG data templates MUST be encoded using the encoding rules of a
   collection as defined in Section 4.2.

  with

  5.  Encoding of the 'yang-data' extension

   The yang-data extension [RFC8040] is used to define data structures
   in YANG that are not intended to be implemented as part of a
   datastore.

   The yang-data extension MUST be encoded using the encoding rules of
   interior nodes as defined in Section 4.2.

  to avoid the use of 'templates' and 'collections'. In the following
  text, there are a few more places where 'YANG template' should be
  replaced by the 'yang-data extension'.

- Unexpected ietf-comi module name showing up in an example

     "ietf-comi:error" : {

  The module prefix pops up out of the blue. You may want the put the
  yang-data definition into an example module, give it an example
  name, and then replace ietf-comi with that example name. Perhaps
  even simplify the yang-data schema to just 1-2 leafs.

- typo

  s/section defined the CBOR encoding/section defines the CBOR encoding/

- Improve wording

   To avoid overlap of 'value' defined in different 'enumeration'
   statements, 'enumeration' defined in a Leafs of type 'union' MUST be
   encoded using a CBOR text string data item (major type 3) and MUST
   contain one of the names assigned by 'enum' statements in YANG.

  This does not read well. Perhaps:

   Values of 'enumeration' types defined in a 'union' type MUST be
   encoded using a CBOR text string data item (major type 3) and MUST
   contain one of the names assigned by 'enum' statements in YANG.

  There is similar text on page 31 in the context of bits encoding
  that will also benefit from a rewrite.

- Third example Bob vs Jack

  Should the third example use

   "/ietf-system:system/authentication/user[name='jack']"

  instead of

   "/ietf-system:system/authentication/user[name='bob']"

  to line up with the third example using SIDs?

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
Fax:   [phone]         <https://www.jacobs-university.de/>