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

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Wed, 5 Jan 2022 11:49:04 +0000

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 <[email-masked]> On Behalf Of Carsten Bormann
> Sent: 20 December 2021 03:51
> To: Rob Wilton (rwilton) <[email-masked]>
> Cc: [email-masked]; [email-masked]; The IESG
> <[email-masked]>; [email-masked]; Marco Tiloca <[email-masked]>
> 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 <[email-masked]>
> 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

---

From: Carsten Bormann <[email-masked]>
Date: Wed, 5 Jan 2022 17:08:49 +0100

Hi Rob,

thank you for this quick response!

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

The use of absolute SID values as map keys indeed requires the use of tag 47; the choice is only whether to actually opt for absolute SIDs or to keep with the (untagged) normal deltas.
The sentence you cited was intended as text leading in to a cross-reference, not a normative statement.  

However, that normative statement isn’t fully contained in Section 9.3, which just defines the CBOR tag, but somewhat smeared over 4.2.1 and the example in 4.5.1 (of all places).
So it seems that some editorial surgery would be beneficial here, making sure that the definition how to use tag 47 and an example for that is in a form that can be referenced here (and, to reduce may/MAY confusion, we probably should substitute “can”).

Now: https://github.com/core-wg/yang-cbor/issues/137

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

The intention was to say this in paragraph 2 of Section 7 — the cited text is just the summary of this section.  We could repeat the salient parts in the summary, but that has the usual risk of stating the same thing twice in subtly different ways.

> (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 und…

Indeed, the intention is for the mechanism to be permissive — I think that styles/policies on how to best use the mechanism in hybrid scenarios will need to evolve before we can write them up (possibly becoming part of an RFC-8407-like document at some point).

[Skipping items where no further work seems to be necessary:]

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

We didn’t want to put in examples showing good ways of using these feature SIDs, again for mechanism/policy reasons.

However, an example usage can be found in Page 8 of core-yang-library:
https://datatracker.ietf.org/doc/html/draft-ietf-core-yang-library-03#page-8

grouping implementation-parameters {
    description
      "Parameters for describing the implementation of a module.";

    leaf-list feature {
      type sid:sid;
      description
        "List of all YANG feature names from this module that are
[…]

So a YANG module needs to make use of sid:sid explicitly in order to make use of SIDs for features.

Enabling the use of SIDs automatically in the encoding of any SID-naive usage of yang:yang-identifier could widen the field of application.  However, as you note, at the base type level these are just YANG strings, so we would need a mechanism such as a CBOR tag that can be used in place of text strings anywhere in the encoding.  This would seem to be a rather invasive mechanism, for a somewhat limited gain.
Simply allocating SIDs for YANG features costs nearly nothing, and enables the above optimization in a module that will often be used in an id=sid environment.

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

Strictly speaking, the YANG-CBOR specification does not have to make this constraint; deltas from 64-bit unsigned integers do fit into CBOR 64-bit+sign integers.  However, given little compiler support for 64+sign, implementers are rather likely to get this wrong.  More so, as the usual source of SIDs to be used with YANG-CBOR is constrained to 63-bit integers, as you note, so there will be little testing of SID deltas that don’t fit into int64.
We can defuse this situation, with no practical loss, by insisting on 63-bit unsigned integers (0..9223372036854775807) for the SIDs in the YANG-CBOR specification, as well.

Now https://github.com/core-wg/yang-cbor/issues/138

[Thank you for the affirmative comments on our responses to the COMMENTs.]

Grüße, Carsten

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Wed, 5 Jan 2022 17:14:14 +0000

Hi Carsten,

Thanks for the answers, please see further comments inline ...

> Sent: 05 January 2022 16:09
> > 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?
> 
> The use of absolute SID values as map keys indeed requires the use of tag 47;
> the choice is only whether to actually opt for absolute SIDs or to keep with
> the (untagged) normal deltas.
> However, that normative statement isn’t fully contained in Section 9.3, which
> just defines the CBOR tag, but somewhat smeared over 4.2.1 and the
> example in 4.5.1 (of all places).
> So it seems that some editorial surgery would be beneficial here, making sure
> that the definition how to use tag 47 and an example for that is in a form that
> can be referenced here (and, to reduce may/MAY confusion, we probably
> should substitute “can”).
> 

Okay.

> > 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?
> 
> The intention was to say this in paragraph 2 of Section 7 — the cited text is
> just the summary of this section.  We could repeat the salient parts in the
> summary, but that has the usual risk of stating the same thing twice in subtly
> different ways.

Ah, that's fine.  I was only checking the diffs, so I had missed 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.
> 
> Indeed, the intention is for the mechanism to be permissive — I think that
> styles/policies on how to best use the mechanism in hybrid scenarios will
> need to evolve before we can write them up (possibly becoming part of an
> RFC-8407-like document at some point).

My comments was really because I was wondering if this permissiveness is perhaps non-obvious (although the file definitions provide a counter example), hence I was questioning whether making the permissiveness explicitly clear would be beneficial to the reader.

> > 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).
> >
> We didn’t want to put in examples showing good ways of using these feature
> SIDs, again for mechanism/policy reasons.
> 

Okay, I think that this was the bit that I was missing.

> 
> Enabling the use of SIDs automatically in the encoding of any SID-naive usage
> of yang:yang-identifier could widen the field of application.  However, as you
> note, at the base type level these are just YANG strings, so we would need a
> mechanism such as a CBOR tag that can be used in place of text strings
> anywhere in the encoding.  This would seem to be a rather invasive
> mechanism, for a somewhat limited gain.
> Simply allocating SIDs for YANG features costs nearly nothing, and enables
> the above optimization in a module that will often be used in an id=sid
> environment.

That's fine, I wasn't suggesting that - I was just not understanding how you were planning on using the allocated sids, and whether it needed any explicit text to describe how they are encoded, but I see that this just falls out because they are defined as YANG uint64s.

Note, possibly it would be 'nicer' if the YANG typedef for "sid" was defined as part of a separate typdefs YANG module (arguably ideally in this YANG CBOR draft) rather than the SID file draft, otherwise your CORE YANG library will end up with an import-only dependency on ietf-sid-file.yang.  Not the end of the world, but I thought that I would flag it, and leave it to you to decide whether you want to act on it (i.e., I won't hold my "discuss" for this point).  To give an example of where became a slight issue: OpenConfig wanted to reuse iana-types.yang, which had a dependency on a type defined in ietf-interfaces.yang but didn't want to use ietf-interfaces.yang.  In hindsight, having the type defined in a separate types YANG file would have been better.

> > 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.
> >
> Strictly speaking, the YANG-CBOR specification does not have to make this
> constraint; deltas from 64-bit unsigned integers do fit into CBOR 64-bit+sign
> integers.  However, given little compiler support for 64+sign, implementers
> are rather likely to get this wrong.  More so, as the usual source of SIDs to be
> used with YANG-CBOR is constrained to 63-bit integers, as you note, so there
> will be little testing of SID deltas that don’t fit into int64.
> We can defuse this situation, with no practical loss, by insisting on 63-bit
> unsigned integers (0..9223372036854775807) for the SIDs in the YANG-CBOR
> specification, as well.
> 

Thanks.

I'll clear my discuss.

Regards,
Rob