Comparison of Network Management Datastore Architecture (NMDA) Datastores
draft-ietf-netmod-nmda-diff-12

[S5] [question]

* In the output yang-patch response when differences are present, is it
  theoretically possible that 'create' or 'insert' operations will need
  to be represented?  If not, why not?

[S{7,9}] [observation]

* The last paragraph of Section 9 and the whole of Section 7 seem to be
  saying the same things?  Perhaps consider if it's better to just say
  them once in Section 9 or say everything in Section 7 and just have
  the last paragraph of Section 9 say, in effect, "see also Section 7".

In Section 7:

"... number of requests that is served to a client ..." -- s/is/are/

Also, it strikes me that some of what's in Section 7 is repeated in the last paragraph of Section 9.  I wonder if they could perhaps be merged, or 9 could reference 7, or something.

Thanks to Alexey Melnikov for the SECDIR review.

** Section 4.  “differences” bullet.  Per “… defined in RFC8072”, please make “RFC8072” an reference.

** Section 6.  Please review Alexey’s feedback on providing an empty line between the HTTP header and playload.

** Section 6.  “same request in RESTCONF (using JSON format)”.  Missing “:” making the JSON invalid.

OLD
   { "ietf-nmda-compare:input" {

NEW
   { "ietf-nmda-compare:input": {

** Section 9.  The primary new security issue is definitely the possibility of a denial of service as is documented.  I’m not sure what assumption are being made about the datastores -- would it be possible that a user doesn’t have read access to either the source and target of the comparison, but would be able to invoke the RPC?  If so, this might leak configuration information.

Thanks for the effort on this document.

Thanks to Alexey Melnikov for the secdir review.

I'm not experienced enough with YANG to know whether or how problematic
it is that the "anydata subtree-filter" node contents are described by
reference to the NETCONF specification, which has a particular (XML)
representation of YANG data and does not give a clear presentation of
the abstract YANG structure/semantics to be used.  Is it possible to use
the filter-spec choice option when, for example, RESTCONF is used with
JSON encoding?

Section 4

   o  report-origin: When set, this parameter indicates that origin
      metadata should be included as part of RPC output.  When this
      parameter is omitted, origin metadata in comparisons that involve
      <operational> is by default omitted.

Why is it important to complicate the semantics of this parameter with a
dependence on the datastore?  It seems like it would be simpler to get
this effect by having clients specify report-origin when the target is
not <operational>.  Note that changing the semantics would require text
changes in subsequent parts of the document for consistency.  (If
retaining the current semantics, please clarify whether "comparisons
that involve <operational>" applies when operational is source, target,
or either.)

Section 9

In addition to noting that the "compare" RPC is sensitive and should be
restricted to authorized parties, I suggest to reiterate that the
"compare" operation should not provide a mechanism to work around access
control on other nodes -- that is, a result should only be returned if
the requestor would be allowed to access both the "source" and "target"
trees independently of the RPC.  In particular, even a "no-matches"
output should not be returned, as that might provide a way to determine
the structure of the datastore even without accessing it.

We might also incorporate by reference the security considerations for
subtree filtering (RFC 6241) and xpath filtering (RFC 6991).

NITS

Section 1

   an unusually long time to do so.  This can be the case due to certain
   conditions not being met, certain parts of the configuration not
   propagating because considered inactive, resource dependencies not
   being resolved, or even implementation errors in corner conditions.

"because considered inactive" seems like an incomplete clause; maybe
"because they are considered inactive"?

Section 4

   o  differences: This parameter contains the list of differences.
      Those differences are encoded per YANG-Patch data model defined in

s/YANG-Patch/the YANG-Patch/
I'd also consider s/per/according to/, since this is not exactly a
logic-driven deduction but rather more of a new requirement.

Section 6

   for the management of interfaces defined in [RFC8343].  The excerpt
   of the data model whose instantiation is the basis of the comparison
   is as follows:

I feel like this phrasing is a little misleading, as not only is the
following snippet only a subset of the nodes contained within "container
interfaces" but the descriptions have been greatly abbreviated as well.
Perhaps we could say something about "for the purposes of understanding
the subsequent example, the following subset of the [RFC8343] data model
is provided".

   Accept: application/yang-d

(I believe this truncated header field was already noted by another
reviewer.)

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 1. , paragraph 2, nit:
> s both applied configuration data as well as status and statistics. As a resu
>                                   ^^^^^^^^^^
Probable usage error. Use "and" after "both".

Section 6. , paragraph 2, nit:
> ormat): HTTP/1.1 200 OK Date: Thu, 26 Jan 2019 20:56:30 GMT Server: example-s
>                               ^^^^^^^^^^^^^^^^
The date 26 Jan 2019 is not a Thursday, but a Saturday.

Uncited references: [RFC6991].

These URLs point to tools.ietf.org, which is being deprecated:
 * http://tools.ietf.org/wg/netconf/

These URLs in the document can probably be converted to HTTPS:
 * http://tools.ietf.org/wg/netconf/