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

Ivaylo Petrov <ivaylo@ackl.io> Tue, 07 April 2020 13:36 UTC

Return-Path: <ivaylo@ackl.io>
X-Original-To: core@ietfa.amsl.com
Delivered-To: core@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D7BB63A09EC for <core@ietfa.amsl.com>; Tue, 7 Apr 2020 06:36:52 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.898
X-Spam-Level:
X-Spam-Status: No, score=-1.898 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=ackl-io.20150623.gappssmtp.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Or74oI39uymq for <core@ietfa.amsl.com>; Tue, 7 Apr 2020 06:36:49 -0700 (PDT)
Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id EC4E23A09F1 for <core@ietf.org>; Tue, 7 Apr 2020 06:36:05 -0700 (PDT)
Received: by mail-wm1-x32b.google.com with SMTP id r26so1872646wmh.0 for <core@ietf.org>; Tue, 07 Apr 2020 06:36:05 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ackl-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=lbat/nid0XkgLgmugulQxduJIPCWodDW508b3fPW/2s=; b=FQ45laGivtYzV+cNdiJNCknxkD29eYT9uMjgfeg0bPsUvdLTTcnXehRuL1u3h8TX40 8cKoY/Dc83IdQ8PLaIXGdlAK0knoLTgZqpTH+nCgvOp4iIDE0dFpd66Er9yhdMJHRECG npyAM7E44Ap9+NodvpO9XPQ/nAn2mxk4bmc3ciSukteWUOebHbVkdF1XIM0ksJIrsosB 4kqElnGWwfdRHVKTwWBQWe/Z8KTqo+NMjQUOAPGqfQGVQeZLpMOkhUU24oNE9UIxCMgg wLJhxPdctyv7vaMLmTF5kHpV35RB4uF1tVhl1O4x9mkgy7GMhijaQFI37RznXtdCEw3I YVVg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=lbat/nid0XkgLgmugulQxduJIPCWodDW508b3fPW/2s=; b=JnU8cpsYv5Y4sOEjvjDzdJ9AMzZ7Co2deWQviw1C9W/rc5Sd8dYGcYNHW/s/JFhAUY /EVqLl/9wZB92kYhMz3TOKGsIzOntjxe9tTD2mkG8RK+F9jH6bjpLX487ZgrDorZbXMr RYTxJ6VABJoXapcnJF2c/2IqgDvkSWnWvUdKG9HfHQQWq0wkL9eJSUhVIKlZAVxOQk4X tR1jnRqAblGbG2TGgYHvBW1Ge0QDbBkShNR03DzhoDKM2wo50wYuaZL25Me/HGfLaQh8 O4vpsm2uKUPxAAK6W7ZrF1pG9FnTHqUupDyVbQpgRr2jwM1LC4Jn/z6NUETGlHHwNHJF VZ6g==
X-Gm-Message-State: AGi0PubNvUNSM7FTLYF2bnWBTpF5NKSd6L7IWR51blnYaRvRgF5CF1xp dA8LDREfm2rT2Ajz1HI00PJX4QUCG4bhf3EffBZ5z0qgUho=
X-Google-Smtp-Source: APiQypJAaSiaXelAodFi2dtYcjUP2tOZU1YTOCZv8CGw+jmCX15iaFZ9HALeZmgxPK/mfbSHUU/eOkyS6sO7R5VqxcE=
X-Received: by 2002:a1c:ab03:: with SMTP id u3mr2427013wme.86.1586266563938; Tue, 07 Apr 2020 06:36:03 -0700 (PDT)
MIME-Version: 1.0
References: <20200331110237.zbo3zw74xlccur3w@anna.jacobs.jacobs-university.de>
In-Reply-To: <20200331110237.zbo3zw74xlccur3w@anna.jacobs.jacobs-university.de>
From: Ivaylo Petrov <ivaylo@ackl.io>
Date: Tue, 07 Apr 2020 15:35:37 +0200
Message-ID: <CAJFkdRwhxa2T-LVoYfwbMcjjo-dWhwonf_q4B6vGGLuWy5K+BA@mail.gmail.com>
To: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>, core <core@ietf.org>, netmod@ietf.org
Content-Type: multipart/alternative; boundary="00000000000010200b05a2b37726"
Archived-At: <https://mailarchive.ietf.org/arch/msg/core/M6v_klWdcgckqfyGImnPogcYQ6s>
Subject: Re: [core] js review of draft-ietf-core-yang-cbor-12
X-BeenThere: core@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Constrained RESTful Environments \(CoRE\) Working Group list" <core.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/core>, <mailto:core-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/core/>
List-Post: <mailto:core@ietf.org>
List-Help: <mailto:core-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/core>, <mailto:core-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 07 Apr 2020 13:36:53 -0000

Hello Juergen,

Thank you for the detailed review and your comments! They truly help us
improve this document. Please find my answers below (prefixed by [IP]). Note
that the diff after handing your comments and those of Esko Dijk can be
found at [1].

Best regards,
Ivaylo

[1]:
https://tools.ietf.org/rfcdiff?url1=draft-ietf-core-yang-cbor&url2=http://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.txt
On Tue, Mar 31, 2020 at 1:03 PM Juergen Schoenwaelder <
j.schoenwaelder@jacobs-university.de> wrote:

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

[IP]: Fixed

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

[IP]: Fixed following your advice.


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

[IP]: From our discussions, I could say that that is desirable, but not
something these drafts can enforce.  (Also, for drafts that already are
well-advanced, one would expect a companion draft on a later timeline
instead of the original text-based (JSON/XML) draft.)


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

[IP]: Applied and added the reference to RFC7951 as that seems relevant.
Now it reads:

An additional set of encoding rules has been defined in {{RFC7951}} 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...
>

[IP]: This decision has been made before my personal involvement with this
document. I will let the other authors correct me if I am mistaken, but my
understanding is that while it is a good candidate, we did not necessarily
want to mandate the use of the SID draft in order to use the yang-cbor
draft. If the implementers want to have another way of deriving meaning for
the SIDs, that is fine. I will have to verify if the interoperability in
this case was a concern and how it was supposed to be handled.

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

[IP]: I modified the definition as you suggested, but I believe we should
keep it as it aims at avoiding confusion over choice and other schema nodes
that do not add any additional payload/information during the encoding.
They are not considered child nodes in the explicit definition as otherwise
we would have had to provide rules for their encoding.

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

[IP]: I reworded this definition as well, but similarly, a choice is not a
parent node in this context. Relevant text:

The container, list, case, notification, RPC input, RPC output, action
input or action output node in which a schema node is defined.



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

[IP]: I believe that my previous points provide the relevant answers, but do
not hesitate to let us know if you have more concerns about any of your
remarks.


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

[IP]: Fixed


> - avoid 'collection'?
>
>   s/A collection such as/A data node such as/
>

[IP]: RPCs, action inputs/outputs and notifications are not included in the
data node definition, therefore I changed it to 'nodes from the data tree',
which seems to be precise enough. Please let us know how that works for
you. Also do you think we should have a definition like `serializable node`
in order to avoid listing all the different node types each time?

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

[IP]:  Currently media types/content formats are defined in the CoMI draft,
which seems the appropriate place for me. If RESTCONF is to be used with
CBOR, I would expect that some specification should define the media type
along with uses of SIDs if needed.

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


[IP]: I rewrote this as

When schema node are serialized using the rules defined by this
specification as part of an application payload, the payload SHOULD include
information that would allow a stateless way to identify each node, such as
the SID number associated with each node, SID delta from another SID in the
application payload, the namespace qualified name or the
instance-identifier.


Please let us know if that is more clear.


> - s/section Section 4/Section 4/
>

[IP]: Fixed

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

[IP]: My understanding is that indeed there is the presumption that other
implementations that map YANG item identifiers to unsigned numbers could
exist in the future. If this is the case, I am not aware how one could
interoperate (I would guess based on the content format), but I will let my
coauthors comment on that.


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

[IP]: I mostly agree. It seems, however, that there are some nodes from
data trees that are not qualified as data nodes that are still of interest
to us here, so I used the term `nodes from data trees` instead of 'interior
data nodes'. Now the title reads

The 'container' and other nodes from the data tree

and the first sentence just misses `Interior data nodes such as ` and
starts listing the relevant nodes for which this section applies.


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

[IP]: Fixed


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

[IP]: Agreed, only interior nodes is replaced by for the nodes of the data
trees as follows:

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


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

[IP]: I added extra text that should make this much easier to understand.
While the relevant part could be communicated with a smaller example, I
prefer to have a more complete example as long as it does not bloat the
relevant part, which does not appear to be the case here.


> - typo
>
>   s/section defined the CBOR encoding/section defines the CBOR encoding/
>

[IP]: Fixed


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

[IP]: Fixed


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

[IP]: Fixed


> /js
>
> --
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>
>
> _______________________________________________
> core mailing list
> core@ietf.org
> https://www.ietf.org/mailman/listinfo/core
>