-- Section 3 --
As a YANG model can have several YANG modules, was there a discussion in the WG whether the SID range is assigned per module or per model ? The IANA section seems to refer to an allocation per RFC, i.e., per model and not per module.

The part of #37 not closed by #57

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

It is useful to assign a block of SIDs for experimental use.
The rules about permanent assignment for "real" SIDs do not apply to these SIDs.

SIDs are needed during development of modules when the details or even need for the module(s)
is not yet known. It is not desirable to fill the real SID allocations with obsolete or incorrect YANG modules.

Although any organization can allocate its own block for experimental use,
it is desirable for this range to be explicitly defined in the standard, so tools
can throw warnings and errors if experimental numbers appear in production code.

Found terminology that should be reviewed for inclusivity:

• Term "natively"; alternatives might be "built-in", "fundamental",
  "ingrained", "intrinsic", "original".
  See https://www.rfc-editor.org/part2/#inclusive_language for background and
  more guidance.

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 3.3. , paragraph 7, nit:

Section 6. The following examples shows the encoding of a 'hostname' leaf u
^^^^^
You should probably use "show".

Section 4. , paragraph 2, nit:

ta item (major type 5). A map is comprised of pairs of data items, with each
^^^^^^^^^^^^^^^
Did you mean "comprises" or "consists of" or "is composed of"?

Section 4.1. , paragraph 5, nit:

ection 3.3. The following examples shows the encoding of a 'system-state' co
^^^^^
You should probably use "show".

Section 5.2. , paragraph 7, nit:

e element -a byte string with a small number of bytes inside. For this case,
^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, use "a few", or use "some".

Section 6.8. , paragraph 2, nit:

ized-key/key-data" (SID 1734) for user name "bob" and authorized-key "admin"
^^^^^^^^^
It"s more common nowadays to write this noun as one word.

Section 6.9. , paragraph 6, nit:

user" (SID 1730) corresponding to user name "jack". CBOR diagnostic notation
^^^^^^^^^
It"s more common nowadays to write this noun as one word.

Document references draft-ietf-core-sid-15, but -16 is the latest available
revision.

ITS

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?

This should be very easy to resolve and I want to make sure that we understand the situation here better -

*Section 3 : says -

    "The creation of this new version of the ".sid" file
   SHOULD be performed using an automated tool"

If this is supposed to be the automation process written in appendix B then putting a reference here makes sense. If not, then as this is very important tool, more information need to be added here in this specification (like, where to find it, who create and maintains it, any reference to such an existing tools). Also I am missing what consequences one need to consider if the process is not automated. If this is same as written in the introduction -

   "Assignment of SIDs to YANG items can be automated.  For more details
   how this can be achieved, please consult Appendix B."

Then we have two kind of instructions for the same thing - "can be" and a normative "SHOULD". Hence it need to be clarified which one should prevail.

Work in progress

• Found bug in pyang where some SIDs are being omitted that should be generated!!
• Attempting to convert ietf-routing example to delta-SID encoding
• Diagnostic notation almost done (need missing SIDs not generated by pyang)
• Indefinite encoding example in progress (still learning so it is wrong)
• Example has a nested list, list with 2 keys, 2 augmenting modules
• Created 3 SID files using pyang 2.5.0

nested-list.txt
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]

Section 3 is not clear to me that the entire hierarchy from the target resource must be represented.

Even though SID or delta-SID encoding allows random nodes to be properly identified within
a CBOR representation, the YANG structure and complete set of YANG list keys cannot be omitted.

E.g. /foo[name='fred']/bar[name='test1'] must be represented as 2 lists

foo {
    name=fred
    bar  {
         name=test1
    }
}

The protocol request will identify all ancestor instances to the target resource.
In this case, list foo must be encoded to encode any descendant lists.
(Even though SID and delta-SID identifiers to not need foo, then bar to work.)

The complete hierarchy must be present to allow proper conversion to JSON or XML.

Even a hierarchy of containers (no YANG list keys at all) needs to be fully represented
to allow proper conversion and efficient encoding and decoding.

via: https://datatracker.ietf.org/doc/draft-ietf-core-sid/ballot/

(4) The policy in 7.4.2 for allocation a SID mega-range seems to aiming this towards organizations rather than individuals. The policy in 7.6 for the "IETF YANG SID Registry" requires an RFC. What is the mechanism if an individual or open source project wanted to get SIDs assigned for some of their YANG modules? I.e., should we be defining a separate mega-range, managed by IANA, with just Expert Review or Specification Required so that these modules could use SIDs allocated? Or do you envisage a separate entity taking up the responsibility for coordinating this?

Section 2 imports the term "schema tree" which is not used in this document.

In Section 9.1, "Required Parameters" should be "N/A", not "none". See RFC 6838 Section 5.6.

The layout of the table in Section 9.3 is a little confusing. For instance, the first two rows are collectively describing a single entry, I think, but that's not obvious given there are horizontal lines between them.

from Ben Kaduk via https://datatracker.ietf.org/doc/draft-ietf-core-sid/ballot/

(3) There may be another inconsistency to look into; Section 7.6.2 says
that:

• If another ".sid" file has already allocated SIDs for this YANG
  module (e.g. for older or newer versions of the YANG module), the
  YANG items are assigned the same SIDs as in the other ".sid" file.

But we are supposed to allocate a new SID for a YANG item if its
semantics change in a revision of the YANG module. Perhaps it's just
the "for older or newer versions of the YANG module" phrase that needs
tweaking?

One more clarification comment -

• Section 7.5.2: says -

    "  The
   maximum SID range size is 1000.  A larger size may be requested by
   the authors if this recommendation is considered insufficient.  It is
   important to note that an additional SID range can be allocated to an
   existing YANG module if the initial range is exhausted."

I have hard time understanding the mentioning of the maximum SID range here. does this mean this document sets the maximum range to 1000 but others can have more? please clarify.

SECDIR review is at:
https://datatracker.ietf.org/doc/review-ietf-core-yang-cbor-15-secdir-lc-emery-2021-03-10/

notes that it has nits.

(2) I think that this document should be clearer as to the relationship between 
SIDs and submodules (more details in the comment).

1. Regarding the relationship between sids and submodules, I think is best summed up by this comment in the appendix: "Note that ".sid" files can only be generated for YANG modules and not for submodules." I.e., I don't think that sids should be allocated for the name of the submodule, and any items within a submodule are effectively allocated sids as part of processing the module that includes them. This topic should be addressed early in the document, and probably the existing references to submodule in the introduction and the YANG module can be removed.

from Ben Kaduk, via: https://datatracker.ietf.org/doc/draft-ietf-core-sid/ballot/

(2) Per §7.4.2, YANG SID range registries with public ranges MUST
include a reference to the ".sid" file for such ranges, but the
IANA-managed YANG SID range registry established by §7.5 does not, in
and of itself, make such a provision. This function seems to be served
by the "IETF YANG SIG Registry" created by §7.6, so we may just need to
point to the one registry from the other in order to remain internally
consistent.

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.

The procedures for encoding SIDs as keys could be more precise.
How does a decoder tell if a SID as a key is a full SID or a delta from the parent?
They are encoding exactly the same so a large delta could be a full SID.

Apparent procedure:

1. first node must be a full SID (there is no parent yet)
2. each nested map or array must be encoded as a delta to the parent
   and this starts a new parent for delta calculation
3. the decoder never looks above the current map or array for the delta in the event
   a name is used instead of a SID for the current map or array
4. if a name is used for the key to start a map or array then none of descendants
   of this map or array can be identified with SID keys

There are no examples of mixing names and SIDs as keys.
The text as written seems to support the interpretation above.

There are long threads in 6man about SID, as related to SRH/Spring/etc.
I wonder if we might want to change our TLA.

Redundant, outdated, old format.

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.

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?

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

I installed all the packages.
Getting an error when I run make

andy@andy-i9-homedev:~/swdev/yang-cbor-wg$ make 
cat draft-ietf-core-sid.md  | sed -e 's/draft-ietf-core-sid-latest/draft-ietf-core-sid/g; s/draft-ietf-core-yang-cbor-latest/draft-ietf-core-yang-cbor/g;' | kramdown-rfc2629 --v3 | lib/add-note.py | xml2rfc -q -s 'Setting consensus="true" for IETF STD document' --rfc-base-url https://datatracker.ietf.org/doc/html/ --id-base-url https://datatracker.ietf.org/doc/html/ --cache=/home/andy/.cache/xml2rfc --v2v3 /dev/stdin -o /dev/stdout >draft-ietf-core-sid.xml
---
- No link definition for link ID '[rfc xxxx]' found on line 613
- No link definition for link ID 'rfc xxxx' found on line 613

I think it is complaining about this reference

* name:         ietf-sid-file
* namespace:    urn:ietf:params:xml:ns:yang:ietf-sid-file
* prefix:       sid
* reference:    [[RFC XXXX]]

Hello from the IPLD project ;)

For a few years we’ve been using CBOR tag 42 in dag-cbor. It’s actually the only tag we use, and we use it to note when something is a link to another piece of data. At the time we started using it there was nothing registered but we noticed that the IANA registry recently assigned it to this spec.

It would be very hard for us to migrate given the amount of data already created and persisted in the network.

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

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

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.

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.

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 probably need a comment to that effect, or possibly use the ugly RFC8792 approach.

this will take a fine-comb through document to verify needs.

schema mount allows YANG modules to be rooted within another subtree

The data that is mounted and where it is mounted is configurable so it is not possible
to define global SIDs for these mounted nodes.

It is possible (and part of the standard) that an XPath or JSON instance-identifier can represent any mounted nodes.
This is possible because the full path-to-root is specified.

One possible solution is to support a Local SID megarange to allow the server to
create a local SID file specific to its own schema-mount configuration.

Example

 module A {
    container top {
       container myroot {
          yangmnt:mount-point "myroot";
      }
   }
}
      
module B {
    container top {
        leaf name { type string; }
    }
}

If module B is mounted at mount-point "myroot" in module A then the following nodes are added

/A:top/myroot/B:top
/A:top/myroot/B:top/name

There is no practical way to define global SIDs for mounted objects.
It may be possible for some standard mount-points like RFC 8530

-- Section 7.5.2 --
AFAIK, many YANG modules are not originating at the IETF (Open Config, vendor specifics modules, ...). How can those non-IETF modules get a SID as the two ways to get a SID are based on RFC (if I understand correctly this section).

It caught my attention that the names of the editors in the module don't match the names of the document authors. Is there a reason for that?

I will let my OPS and Management AD to comment on the use of 'YANG schema' rather than 'YANG module' and about the word 'item' for many YANG-related concepts.

Section 1:

It's a bit weird to have requirements language in an Introduction.

Section 7, generally:

Thanks for providing advice for the Designated Experts for each of the created registries. This is a not-exactly-uncommon oversight.

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.

Yes, maybe according to Content-Type.
But there will be Content-Type which are SID-only and name-only.
This needs to be explained in yang-cbor document, with three Content-Types allocated.

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

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

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.

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.

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?

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

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

From Ben Kaduck via https://datatracker.ietf.org/doc/draft-ietf-core-sid/ballot/

(1) I think there is a new security consideration with this work that is
important to document clearly -- not only do we define a new type of
identifier, but we define a file format and other mechanisms for
disseminating that information. An entity that's processing
application/yang-data+cbor; id=sid information needs to ensure that the
.sid files (or other source of SID information) it uses for such
processing came from a trustworthy authority (or at least the same
source as the data file). It would be possible for malicious
manipulation of .sid file contents to cause a message recipient to
mis-interpret the received message without any indication of such
tampering.

Section 6.3.3
Please allocate a 50 SID range for draft-ietf-anima-constrained-voucher for the voucher-request, and a 50 SID range for the voucher (RFC8366) extension.
These could slide into 1400 and 1450 is you liked, or somewhere else, as you see fit.
Time is important here, as we have implementations that will need to change.

A generic comment about the operational issue of supporting TWO ways to encode a data node: either normal string or the SID. This means that either there is a 2-way negotiation mechanism or that all CORE nodes must support both encoding and have agreed on a common SID mappings. Section 7 only briefly touches this issue with "Content-Type" but not with "Accept".

-- Section 4.2.1 & 4.4.1 --
BTW, I like the idea of encoding a container with sequential SID and the delta CBOR encoding ;-)