Subject: Re: [core] js review of draft-ietf-core-sid-12

From: Ivaylo Petrov <[email-masked]>
Date: Wed, 15 Apr 2020 15:27:21 +0200

Hello Juergen,

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

Best regards,
Ivaylo

[1]:
https://tools.ietf.org/rfcdiff?url1=draft-ietf-core-sid&url2=http://core-wg.github.io/yang-cbor/draft-ietf-core-sid-latest.txt

On Mon, Mar 30, 2020 at 11:32 PM Juergen Schoenwaelder <
[email-masked]> wrote:

> Hi,
>
> I have reviewed draft-ietf-core-sid-12. I will try to review yang-cbor
> tomorrow as well. (I am probably more interested in CBOR encoding for
> RESTCONF than the usage of CoAP itself, hence my priority on these two
> IDs under last call.)
>
> - Avoid the use of the word 'template', which is not a well defined
>   term and may be used with varying interpretations.
>
>    o  data nodes (Note: including those parts of a YANG template as
>       defined by the 'yang-data' extension.)
>
>   It is not clear to me what is meant with "those parts of a YANG
>   template as defined by the 'yang-data' extension." I think this
>   could benefit from a rewording.
>

[IP]: Reworded.

- The ID seems to assume that semantics of yang items never change.
>   This is true so far but NETMOD has chartered work that might change
>   this property. So what happens if the semantics of a YANG item
>   changes?
>
>    SIDs are assigned permanently, items introduced by a new revision of
>    a YANG module are added to the list of SIDs already assigned.
>
>   If a YANG module changes in a non-backwards compatible way, I assume
>   a new sid range must be allocated? Strictly speaking, this question
>   does not have to be answered today but it very likely needs an
>   answer in the future...
>

[IP]: We will not be able to clearly answer this before there is more
information how the YANG items semantics can change. For now it looks like
assigning new range would be a good solution, but maybe there will be some
other solutions that will be even more optimal. What looks logical is that
at least every semantic of an item should have a separate SID.

- The definition of 'path' is not very precise, i.e., it does not
>   spell out how module namespaces work, only by example.. I do not
>   know whether a definition can be imported from somewhere.
>

[IP]: I tried to improve it. Please let me know if now it is clear enough.

- s/RESCONF/RESTCONF/

[IP]: Fixed.

- Is it CoRECONF or CORECONF? And I find the term CORECONF confusing.
>   We have two protocols called NETCONF and RESTCONF and now we add
>   another protocol called CoMI and we call CoMI together with YANG
>   CBOR and SIDs CORECONF?
>
>   1) NETCONF  + YANG + XML      serialization + path naming -> ?
>   2) RESTCONF + YANG + XML|JSON serialization + path naming -> ?
>   3) CoMI     + YANG + CBOR     serialization + SID naming  -> CORECONF
>
>   We do not have a term for 1) and 2) and then we have a term for 3)
>   which, however, looks more like the protocol names used in 1) and
>   2). This comment is not specific to this ID, but the asymmetry
>   showed up while reading the SID document, I had to look at other IDs
>   to understand how things are named. And the SID document says
>
>    YANG is a language designed to model data accessed using one of the
>    compatible protocols (e.g.  NETCONF [RFC6241], RESCONF [RFC8040] and
>    CoRECONF [I-D.ietf-core-comi]).
>
>   Then I read the CoMI abstract. It first says CoMI is "a CoAP
>   Management Interface", it then says "The complete solution composed
>   of CoMI, [I-D.ietf-core-yang-cbor] and [I-D.ietf-core-sid] is called
>   CORECONF." and finally it states that "CORECONF extends the set of
>   YANG based protocols, NETCONF and RESTCONF, with the capability to
>   manage constrained devices and networks.". So I am confused, is
>   CORECONF a protocol as stated in this document? Or is CoMI a
>   protocol? (What is then the difference between a "Management
>   Interface" and a management protocol?) I am not sure whether I get
>   to review comi, hence I mention my confusion here as I hit it while
>   reviewing the sid document.
>

[IP]: Currently this is indeed somewhat confusing. The proposed change from
Michael Richardson was to at least have CORECONF in the title of the CoMI
document. I am wondering if that might still leave some of the confusion.
For me the simple solution is in this document to refer to CoMI, not
CORECONF and let CoMI draft define what CORECONF actually is. Unless you
think this will still not resolve the issue, this is going to be my way
forward.

- This description makes little sense to me:
>
>   typedef sid-file-version-identifier {
>     type uint64;
>     description
>       "Optional attribute that gives information about the .sid file
>        version.";
>   }
>
>   This is a type definition. Why does the description talk about an
>   optional attribute? The type should not state whether something
>   using the type is optional or not. (And I would prefer to avoid
>   'attribute', better use YANG defined terms or just describe that
>   this type represents a version number for a SID file.)
>

[IP]: I believe now it should be more clear.

- sid range - was it not said before that it is 63 bits? Is the idea
>   that sids with the highest bit set are legal but undefined or
>   reserved?  Or should there be a range restriction?
>
>   typedef sid {
>     type uint64;
>     description
>       "YANG Schema Item iDentifier";
>     reference
>       "[I-D.ietf-core-sid] YANG Schema Item iDentifier (SID)";
>   }
>

[IP]: I set the range accordingly.

- schema-node-path
>
>       "Identifies a schema-node path string for use in the
>        SID registry. This string format follows the rules
>        for an instance-identifier, as defined in RFC 7959,
>        except that no predicates are allowed.
>
>   RFC 7959 seems to be a typo, I assume you mean RFC 7951
>

[IP]: Yes, thank you for spotting this!

  s/Identifies a schema-node path string/A schema-node path"
>
>   It is a bit confusing to define a schema-node path by way of
>   reference to an instance identifier. I understand that you borrow
>   the namespace encoding from the way JSON encode instance identifiers
>   but this type really represents what RFC 7950 calls an absolute
>   schema node identifier, no? Is the term schema-node path actually
>   needed or is it the same as absolute schema node identifier? Or is
>   the difference between the two how namespaces are represented?
>

[IP]: I might have misunderstood something, but my understanding is that
the prefix related to a module could be changed during an import, whereas
here we really want to use the module name as a more stable identifier. The
difference between absolute schema node identifier and schema-node path is
that we mandate the use of module name and not prefix as defined in RFC
7950.

- dependency-revision
>
>         list dependency-revision {
>         key "module-name";
>
>         description
>           "Information about the revision of each YANG module that the
>           module in
>           'module-name' includes used during the .sid file generation.";
>
>    The sentence can probably be rewritten to make it clearer.
>
>    Are the SIDs assigned to a module dependent on the modules listed
>    in the 'dependency-revision'? (Is this a good name for the list?)
>    Why is it necessary to know which revisions were used to resolve
>    imports without revision?
>

[IP]: I reformulated it to

Information about the used revision during the .sid file generation
of each YANG module that the module in 'module-name' imported.

The idea is that if this information is not available later on and
different versions of the modules are used, slightly different results
might be produced due to augmented items for example.

- Please avoid wrapped entry point numbers in the table in 6.3.3.

[IP]: Fixed.

- The handling of early allocations sounds complex, I have some doubts
>   this process will work in practice...
>

[IP]: We are working on reformulating it in a more precise manner.

- Incomplete sentence:
>
>   RFC7120] also says
>
>   Or is the following paragraph a quote? In that case, add a colon.
>
> - There are likely more normative references, e.g., RFC 6991 and RFC
>   8040.
>

[IP]: Fixed.

- Why does the example in appendix A not have / need
>   dependency-revisions?

[IP]: Added now.

- I have not run any tools to validate things. I just read the I-D.

> /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
> [email-masked]
> https://www.ietf.org/mailman/listinfo/core
>

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 15 Apr 2020 18:20:54 +0200

> at least every semantic of an item should have a separate SID.

Yes and this will impact the SID document since SIDs are going to be
specific to a (module, path, version) triple.

> think this will still not resolve the issue, this is going to be my way

Avoiding CORECONF in this document helps to limit the problem. If CoMI
is the name of the protocol, I would hope we do not need CORECONF at
all. But then CORECONF is all over the place in
draft-ietf-core-comi-09.txt, it actually looks like the protocol is
called CORECONF and not CoMI. I really believe this terminology
confusion needs to be resolved in the WG so the WG actually knows and
agrees on the name of the technology they standardize.
 
> [IP]: I believe now it should be more clear.

Yes. I wonder though, is this a simple linear counter? Or can it be
anything as long as newer > older is satisfied? Or is this just a tag
that needs to match and it does not imply any order semantics?

> that we mandate the use of module name and not prefix as defined in RFC

Well, what you model here is an absolute schema node path, except that
prefixes are replaced by module names. Note that refering to
instance-identifier as defined in RFC 7951 has the problem, the RFC
7951 definition of an instance-identifier also includes prefixes
instead of module names.

/js