A File Format for YANG Instance Data
draft-ietf-netmod-yang-instance-file-format-21

Thanks for your work on this spec. Thanks also to Kent Watsen for his hard work shepherding it.

I have some comments below which I hope may be helpful.

1. Section 1, nit, s/In Appendix C describes/Appendix C describes/ (delete the "in")

2. Section 1.4:

   A YANG instance data set is created at a specific point of time.  If
   the data changes afterwards, this is not represented in the instance
   data set anymore.  The current values may be retrieved at run-time

I think "anymore" should be cut, for several reasons, the most important of which is that it seems to be objectively wrong. The cut would be the minimal fix, but did you mean something more like this? "If the data changes afterwards, the instance data will no longer represent the current data, unless it is updated."

3. Section 2, nit, s/constrains MAY be violated/constraints MAY be violated/

4. Section 2.1: I was amazed that the "external method" option, which is essentially the "simply don't bother" option, was acceptable to the WG and other reviewers. To my eye, the URI method option seems functionally just as good (it keeps the content of the file itself small) while providing a stronger (though still not very strong) assurance that the schema will actually be available. 
   Was "external method" discussed in the WG? Or am I simply in the rough for 
even thinking it surprising?
   
5. Appendix C: I'm inclined to agree with the shepherd's recommendation to remove this entire section (https://mailarchive.ietf.org/arch/msg/netmod/5DfEi8swOmLEPykBpFICJK6398s/) since readability is problematic and it doesn't seem to add to the usability of the spec. Since it is, as it says, only a non-normative appendix I don't insist, but I strongly encourage you to consider trimming or rewriting it.

The shepherd writeup is incomplete with respect to the first question.

All of the SHOULDs in Section 2 leave me wondering under what conditions one might legitimately deviate from what they are saying.  Since SHOULD offers a choice, I recommend making this more clear.

** Section 2.  
instance-data-set-name ['@' ( revision-date / timestamp ) ]
                     ( '.xml' / '.json' )

A syntax for an instance data file name is specified with normative language.  However, this format is not explained is cited.

** Section 2. Editorial.
OLD
If the leaf "name" is present in the instance data header, its value
   SHOULD be used for the "instance-data-set-name"

NEW
If the leaf "name" is present in the instance data header, its value
   SHOULD be used for the "instance-data-set-name" in the filename.

** Section 2.  

Description of the instance data set.  The description SHOULD
         contain information whether and how the data can change during
         the lifetime of the server

I found this definition of the description confusing as Figure 1 – 3 don’t seem to describe “whether and how the data” will change.

** Section 2.1.1.  Per “The inline-yang-library anydata data node carries instance data (conforming to ietf-yang-library@2019-01-04)”, please provide a reference to “ietf-yang-library@2019-01-04”.

** Section 4.  Please note the risk of using same-schema-as-file, especially if these configs are not integrity protected or received from outside sources.  Per https://, there are the risks of loading remote content.  Section 7 of RFC3986 is a good reference.  Per file://, there are things list directory traversal.

** Section 4.  Per “The header part is not security sensitive with one possible exception … the URI method”, I’m not sure that such a strong statement can be made given the lack of application context.  For example, the description leaf in the header could include sensitive information, say ‘Latest test router config for new super secret Aqua-Violet flying car project’.  This text needs to either have a caution that that this header is "unprotected so do not put in sensitive information unless this file is protected", or clarify that more in the header than the URI could be sensitive.

** Section 4.  Thanks for the language trying to create equivalency between the protections of the file and the YANG store that would house it on a live system.  Recommend making this text clear to say this applies to both at rest and in motion data.

OLD
The same kind of handling should be applied, that would be
   needed for the result of a read operation returning the same data.

NEW (roughly)
The same kind of handling should be applied to this file at rest and in transit that would be needed for the result of a read operation returning the same data.  These in-transit protection mechanisms will also mitigate integrity issues when transporting the file.

Thanks for the efforts on this document.

I have following comments, by addressing them I believe will improve the document quality

- Section 2.1 : Should it not say if the "content-schema" node exists then one of the methods MUST be used? as I see the specification of content schema is a SHOULD, hence may not be included for whatever reason.

- Section 4 : it says 

    Instance data files may contain sensitive data.

  OK, but what should be taken into consideration when putting the sensitive data in the data file. It feels like there should be more information provided to the user of the specification for actually materialize the statement made here.

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

Generic comment about the use of the word "file" (which means an object in a file system for me) rather than something more generic (no suggestion to offer though) ?

-- Section 1 --
The first 2 sentences are quite repetitive.

Is it about "offline delivery" or "exchange" ? At this point of reading the document, it is still unclear in my mind what it is about... The rest of the I-D made it clear.

Unclear which UC is either implemented or potential (even with the appendix); could also add forward references to the appendix UC). Should the implementation(s) be referenced if they are public ?


-- Section 1.1 --
Unsure why a "data set" should be named? The choice of words does not seem the best fit (even though if I have no suggestions).


-- Section 2 --
Like some other ADs, I wonder why "The context data part MUST... except" is not a "SHOULD" as there are exceptions.

What is the expected behaviour when the timestamp in the filename does not match the meta data ?

-- Section 2.1 --
There is a "SHOULD" so when are exceptions/deviations acceptable ?

The description of "simplified" is really too simple ;-)

I would also appreciate that the order of the list matches the following sub-sections order.

Thank you for using RFC 8792.

-- Section 4 --
Did the authors think about adding the party creating the file and adding an optional signature in the file itself?

== NITS ==

-- Section 1 --
The first 2 sentences are quite repetitive. Missing "." At the end of the 1st §

Why is "Factory Default Setting" capitalised ?

-- Section 1.2 --
Why using the future tense "shall be" rather than "are" ?

-- Section 2.1.1 and others --
Suggest to warn the reader that the examples are further in the text in a different section.

-- Section 6 --
A "," is missing.

Should we register media-types for the file formats being specified?

Section 2

   Two formats are specified based on the XML and JSON YANG encodings.
   Later, as other YANG encodings (e.g., CBOR) are defined, further
   instance data formats may be specified.

I don't remember seeing a clear description of the specifics of these
two specified formats.  (I assume it's just "use the XML/JSON encoding
for YANG structures", but I don't remember that stated anywhere.)

   The name of the instance data file SHOULD be of the form:

      instance-data-set-name ['@' ( revision-date / timestamp ) ]
                     ( '.xml' / '.json' )

This looks (almost?  Not sure about '@' vs. "@".) like valid ABNF.  Do
we want to say that and reference RFC 5234 for the interpretation of the
symbols?

   If the leaf "name" is present in the instance data header, its value
   SHOULD be used for the "instance-data-set-name".  If the "revision-
   date" is present in the filename it MUST conform to the format of the
   revision-date leaf in the YANG model.  [...]

This seems unenforcable, and contrary to the Unix ethos.  Why is it
necessary to try to have consistency betwen the contents of the file and
its name in the file system, as opposed to letting the type and contents
of a file speak for itself regardless of the name in the file system?

   Metadata, information about the data set itself SHOULD be included in
   the instance data set.  Some metadata items are defined in the YANG
   module "ietf-yang-instance-data", but other items MAY be used.

   Metadata MUST include:

      -  Version of the YANG Instance Data format

Doesn't the latter (MUST) effectively make the former (SHOULD) also into
a "MUST"?

Also, if this is actually mandatory, shouldn't that be reflected in the
YANG module?

Section 2.1.2

   import-only dependencies MAY be excluded from the leaf-list.  If they
   are excluded then the consumer of the instance data set has to apply
   the YANG language rules to resolve the imports.  An example of the

Do we want to say something like "Accordingly, recipients of the
instance data set must be prepared to perform this processing, absent
prior knowledge about the files they will be processing"?

Section 2.2.1

    <contact>info@acme.com</contact>

Unfortunately, acme.com is a real domain name; we should probably use a
BCP 32 name.  Likewise for urn:rdns:acme.com:oammodel:acme-system-ext,
etc.

Section 2.2.2

     <nacm xmlns="urn:ietf:params:xml:ns:yang:ietf-netconf-acm">
       <enable-nacm>true</enable-nacm>
       <read-default>deny</read-default>
       <exec-default>deny</exec-default>

Is there a <write-default> that should be set as well?  Or do we just
implicitly rely on the default from RFC 8341?

Section 3

         description
           "An arbitrary name for the YANG instance data set.  This
            value is primarily used for descriptive purposes.  However,
            when the instance data set is saved to a file, then the
            filename MUST encode the name's value, per Section 3
            of RFC XXXX.";

I think this requirement is currently stated in Section 2, not 3 (though
in my previous comment I suggest that the requirement should be
removed).

Section 4

(I wrote, then deleted as duplicate, essentially all of the same things
that Roman commented on.  Thanks for updating in response to his
comments.)

   The document does not specify any method to influence the behavior of
   a server.

A few of the listed use cases seem to involve loading configuration into
a server, which could perhaps be considered to influence the behavior of
the server in question.

   The header part is not security sensitive with one possible
   exception.  If the URI method is used for specification of the
   content schema and the URI includes a username and/or a password, the
   instance data file needs to be handled securely as mentioned below.

In the terminology of RFC 3986 this is the "userinfo subcomponent", as
in "the URI includes a userinfo subcomponent".

NITS

Section 2.2.1, 2.2.2

It's a bit challenging to get the <revision> of the file to be much
older than the <revision> of the YANG modules it uses.

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.2. , paragraph 4, nit:
> ance data; the instance data itself is is contained only in the 'content-data
>                                     ^^^^^
Possible typo: you repeated a word.

Section 3.2. , paragraph 5, nit:
> ove if the module gets changed // in anyway during reviews or RFC editor proc
>                                   ^^^^^^^^^
Did you mean "in any way"?

Section 3.2. , paragraph 19, nit:
>  data file needs to be handled in a secure way as mentioned below. The secur
>                                ^^^^^^^^^^^^^^^
Consider replacing this phrase with the adverb "securely" to avoid wordiness.

Section 5.2. , paragraph 1, nit:
>  v08 - v09 * Removed reference to similar to get reply * Introduced artwork
>                                ^^^^^^^^^^^^^
Typo. Did you mean "too similar to"?

These URLs in the document did not return content:
 * https://datatracker.ietf.org/wg/netmodf/