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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 13 Jul 2021 15:33:19 -0700

Benjamin Kaduk 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:
----------------------------------------------------------------------

(1) The statement "Bytes with no bits set at the end of the byte string are
removed." in Section 6.7 seems confusing to the point of being
potentially harmful, and I'm not sure why it needs to be there.  In the
context it appears in, it seems to leave the value to be used for the
bit string offset in an ambiguous state.  If the intent is that such
strings should not be generated (and MAY/SHOULD/MUST be rejected by
recipients), that's okay, but having them silently ignored is very
surprising and may merit discussion.

(2) I think we should discuss the relationship between this document and
draft-ietf-core-sid, which are before the IESG at the same time.  This
document says that core-sid is "one example for" a specification
defining the management of SIDs, but draft-ietf-core-sid claims to be
the document that "defines the semantics, the registration, and
assignment processes of YANG SIDs".  I'm having a hard time seeing the
two statements as compatible with each other, but maybe I'm missing something.

(3) The second example of instance-identifier using SID (§6.13.1) seems
malformed, with "key name country" appearing under both "list user" and
"list authorized-key" and no "country" leaf within "list user" other
than the one under "list authorized-key".  (The actual identityref
example appears to correctly only use "name" as the key for "list
user" and not "list authorized-key".)

(4) Relatedly, the second example of instance-identifier by name (§6.13.2)
does not show a country for "authorized-key", and I'm not sure if that's
a valid way to represent the given YANG element.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I can see why it would not make sense to do so in this document that is
so tightly integrated with CORECONF, but is there any work to produce a
CBOR encoding for the "structure" extension from RFC 8791?

As a general note, I did not look particularly carefully at the example
CBOR encodings, on the assumption that they were automatically generated
as part of the document production process.

Section 4.2

   This specification supports two types of CBOR keys; SID as defined in
   Section 3.2 and names as defined in Section 3.3.

I suggest making an explicit statement about whether the two types of
keys can be comingled in the same container/etc, possibly just by
forward-reference to the media-type parameter.

Section 4.2.1

   In the context of containers and other nodes from the data tree, CBOR
   map keys within inner CBOR maps can be encoded using deltas or SIDs.
   In the case of deltas, they MUST be encoded using a CBOR unsigned
   integer (major type 0) or CBOR negative integer (major type 1),
   depending on the actual delta value.  [...]

It's a little surprising that we only get this extended discussion on
SID use in §4.2.1, when we already had §4.1.1 "Using SIDs in keys" that
said almost nothing about it.

Section 4.2.2

   [...] A namespace-qualified name
   MUST be used each time the namespace of a schema node and its parent
   differ.  In all other cases, the simple form of the name MUST be
   used.  [...]

We have a similar statement in §3.3 that is only "the simple form of the
name SHOULD be used" for the latter sentence.  It's not entirely clear
to me what causes the strength of requirement to be different between
the two cases.

Section 4.6

Do we want to explicitly acknowledge the mismatch between the "xml" in
"anyxml" and the actual CBOR contents?

Section 5.2

   The yang-data extensions encoded using names are carried in a CBOR
   map containing a single item pair.  The key of this item is set to
   the namespace qualified name of the yang-data extension container;
   the value is set to the CBOR encoding of this container as defined in
   Section 3.3.

I'm not sure if this is a nit or not, so putting it here: is §3.3 the
right reference for the encoding of the container (vs. §4.2)?

Section 6.6

Requiring the string form for enumeration constants in a union seems
like a big loss on encoded size.  Is it worth putting some discussion in
the document (or an appendix thereto) about why other options like
tagged integer are not usable?  (Similar discussion might be relevant
for the bits-in-union case.)

While it's banal, we might also put an example of the integer-encoded
form (as used by non-union contexts) so that the tagged-text-string
example isn't the only one given.

   Leafs of type enumeration MUST be encoded using a CBOR unsigned
   integer (major type 0) or CBOR negative integer (major type 1),
   depending on the actual value.  [...]

I think we should also mention the tagged-text-string case here.

Section 6.7

   In a number of cases the array would only need to have one element -
   a byte string with a small number of bytes inside.  For this case, it
   is expected to omit the array element and have only the byte array
   that would have been inside.  [...]

I think it is actually required (not just "expected") based on some
later text.

Section 6.10, 6.13

I strongly encourage explicit mention of the use of a CBOR tag when
these elements appear inside a union, analogous to the text in §6.6 and
6.7.  The list in §6.12 is pretty easy to miss, and the word "tag" does
not appear at all in these sections.

Section 7

   This specification defines the media-type "application/yang-
   data+cbor", which can be used without parameters or with the
   parameter "id=name" or "id=sid".

I think the media-type discussions should refer to the "id" parameter
and the two possible values for that parameter ('=' is not allowed in a
parameter name).

   This media-type represents a CBOR YANG document containing one or
   multiple data node values.  Depending on the presence and value of
   the media-type parameter "id", each data node is identified by its
   associated namespace qualified name as defined in Section 3.3
   ("id=name"), by its associated YANG SID (represented as a SID delta
   or via tag 47) as defined in Section 3.2 ("id=sid"), or either of
   these (no "id" parameter given).

I think that for identityref and instance-identifier we don't use the
tag 47 for absolute (non-delta) SIDs, at least outside of a union
context.

Section 8

It might be worth mentioning that when the 'id' parameter to the media
type is used, it's important to properly reject identifiers of the other
type, to avoid scenarios where different implementations interpret a
given content in different ways.

Section 10.1

It's not clear to me that RFC 6241 needs to be classified as normative,
at least based on the one place in the text where we reference it.

NITS

Section 2

   *  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, or an action output.

Is this enumeration of "parent" node types guaranteed to be fixed for
all time, or should we consider a more generic wording to describe it?
(Likewise for the "parent" definition.)

Section 4

The example from RFC 7317 may have truncated a little too much of the
content; "mandatory true" for choice transport, the "inet:" prefix for
"type inet:host" and "inet:port-number", etc.

Section 4.4.1

   Deltas of list members are equal to the SID of the current schema
   node minus the SID of the 'list'.

This seems redundant with the list of "Delta values are computed as
follows" from §4.2.1.

Section 4.5

   *  CBOR map keys of any inner schema nodes MUST be set to valid
      deltas or names.

   *  The CBOR array MUST contain either unique scalar values (as a
      leaf-list, see Section 4.3), or maps (as a list, see Section 4.4).

I think a parallel list structure would be "CBOR arrays MUST contain
[...]".

   *  CBOR map values MUST follow the encoding rules of one of the
      datatypes listed in Section 4.

(A recursive reference, though I mostly trust the reader to pick out the
relevant subsections.)

Section 4.5.1

   In some implementations, it might be simpler to use the absolute SID
   tag encoding for the anydata root element.  The resulting encoding is
   as follows:

Pedantically, what follows is diagnostic notation, not a CBOR encoding.

Section 4.6

   An anyxml schema node is used to serialize an arbitrary CBOR content,
   i.e., its value can be any CBOR binary object. anyxml value MAY
   contain CBOR data items tagged with one of the tags listed in
   Section 9.3.  The tags listed in Section 9.3 SHALL be supported.

The second sentence seems to both not start with a capital letter and
have a singular/plural mismatch, both of which could be resolved by
adding "An" at the start.

Section 6.13.1

   Single instance schema nodes MUST be encoded using a CBOR unsigned
   integer data item (major type 0) and set to the targeted schema node
   SID.

Should we say something about the lack of a delta encoding in this
context, analogous to §6.10.1?