Last Call Review of draft-ietf-netconf-restconf-15
review-ietf-netconf-restconf-15-genart-lc-worley-2016-08-01-00

Request Review of draft-ietf-netconf-restconf
Requested rev. no specific revision (document currently at 18)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2016-08-03
Requested 2016-07-14
Other Reviews Genart Early review of -09 by Robert Sparks (diff)
Genart Last Call review of -15 by Robert Sparks (diff)
Genart Telechat review of -17 by Dale Worley (diff)
Secdir Early review of -09 by Liang Xia (diff)
Secdir Last Call review of -15 by Liang Xia (diff)
Opsdir Early review of -13 by Lionel Morand (diff)
Review State Completed
Reviewer Dale Worley
Review review-ietf-netconf-restconf-15-genart-lc-worley-2016-08-01
Posted at http://www.ietf.org/mail-archive/web/ietf/current/msg99132.html
Reviewed rev. 15 (document currently at 18)
Review result Ready with Nits
Draft last updated 2016-08-01
Review completed: 2016-08-01

Review
review-ietf-netconf-restconf-15-genart-lc-worley-2016-08-01

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at
<

http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: review-draft-ietf-netconf-restconf-15
Reviewer: Dale R. Worley
Review Date: 29 July 2016
IETF LC End Date: 3 August 2016
IESG Telechat date: "Not yet"

Summary:

This draft is basically ready for publication, but has nits that
should be fixed before publication.

* Technical nits

- In a few places, returned data can include URIs that are intended to
be usable by the client to retrieve information from the Restconf
server.  These places include the <location> element for accessing an
event stream, the Location header of POST responses, and the URLs for
retrieving schema resources (section 3.7).  The problem
is that implementing this assumes that the server knows a host
identifier (domain name or address) that the client can use to access
the server.  In general, given the ubiquity of NATs, the server
doesn't know this.

It seems to me that the two possible solutions are for the server to
be able to return a URI *relative reference* or for the server to use
as the host-part of the URL the value in the Host header of the
request (which perforce the server knows the client can use to access
the server).  Perhaps this is a known problem with a known solution
and so the draft doesn't bother to mention it.  Then again, perhaps in
practice the clients and the server are in region of the network
containing no internal NATS, and the problem does not arise.

But it seems to me that this could be a very general problem for
implementers and they should be instructed how to deal with it.  I
suspect that the authors know the intended resolution of this issue,
but it would help if the resolution was stated explicitly.

- Does XRD (RFC 6415) admit relative links as values of the href
attribute of the Link element?  Relative links are not usually
considered to be "URIs", but rather "URI-references" (RFC 3986 section
4.1), and RFC 6415 uses "URI" consistently, not "URI-reference".
Also, all examples in RFC 6415 show complete URIs.  If the href
attributes have to be full URIs, then the problem with host
identifiers described above also arises.

* General issues

- Might it be useful to consistently use a trailing-backslash
convention?  There are 17 places that state "lines wrapped for display
purposes only".  And there are places where lines are wrapped without
this message, e.g., section 6.2.  It might be better for the reader to
replace all of these messages with a single statement at the top:

   When a line ends with backslash as the last (non-whitespace)
   character, it is wrapped for display purposes.  It is to be
   considered to be joined to the next line by deleting the backslash,
   the following line break, and the leading whitespace of the next
   line.

- There is a general point regarding the interpretation of data resource
URLs.  Items in leaf-lists and lists that are not configuration data
need not have unique values/keys, and so a URL that selects nodes
within a leaf-list/list by providing a value may select multiple
target nodes.  The consequences of this do not seem to be explicitly
pointed out to the reader in any place.

In regard to retrieval requests (GET or HEAD), this is handled
correctly:  If the response is to be formatted in XML, the request
fails, because XML has no method to return multiple values.  If the
response is to be formatted in JSON, the request succeeds, because
JSON can return multiple values.  (As stated in section 4.3.)

All other requests methods modify the target resource(s), and
therefore cannot be directed to non-configuration data.  But
configuration data leaf-lists/lists must have unique values/keys, so a
URL that identifies modifiable resources must identify a unique
resource.  Most of the text conforms to this fact, but in section 4.7
(regarding DELETE) is:

   If the target resource represents a YANG leaf-list or list, then the
   DELETE method SHOULD NOT delete more than one such instance.

It seems to me that this situation cannot arise.

I don't think this issue requires much change to the text.  There is
the change to section 4.7 listed above, and it seems like it would be
useful to add a general warning in section 3.5.3:

    A URI that specifies list/leaf-list values may contain a value
    that identifies multiple data nodes.  Since duplicate values are
    not permitted in configuration data, any such URI must identify
    non-configuration data.  A request with such a URI is processed
    according to the specifications of the request method; in
    particular, since non-configuration data cannot be edited, a
    request with a method that changes data will necessarily be
    rejected if the URI specifies multiple data nodes.

- The term "resource type" is used frequently but not defined.  It
seems that the set of all resources, i.e., valid URLs in the Restconf
interface, is divided into classes, each of which has a distinct
"resource type", and a distinct set of behaviors.  It would be very
useful to the reader to state this explicitly and list the resource
types.  The text almost does this implicitly, as the resource types
correspond fairly well to the subsections of section 3, though only
sections 3.1, 3.4, 3.5, 3.6, 3.7, and 3.8 correspond to resource
types.

See also the chart listing three resource types (Datastore, Data, and
Operation) in section 4.4.

There seem to be a few places where "resource media type" is used to
mean the same thing as "resource type".  It seems like it should be
replaced by "resource type".

- YANG data template

I think there is an important concept buried in the term "YANG data
template", but it isn't stated exactly anywhere, and it's not clear to
me precisely what it is.  There is an entry in section 1.1.5, "Terms":

   o  YANG data template: a schema for modeling a conceptual data
      structure in an encoding-independent manner.  It is defined with
      the "yang-data" extension, found in Section 8.  It has a
      representation with the media-type "application/yang-data" or
      "application/yang-data+json".

I can't figure out the exact relationship between "YANG data
template", "something defined with the yang-data extension", "the
schema tree of a module", and "data that implicitly has
representations using XML and JSON".

The question of what is a "YANG data template" also interacts with the
question of what the purpose of the yang-data extension is.

I'm sure that the authors at least have an intuitive understanding of
how these concepts interrelate, but it doesn't seem to me that any of
this is laid out clearly for the naive reader.

I think this can be resolved by getting clear answers to these
questions:

-- Does the word "template" have a specific meaning?

The word "template" does not appear in
draft-ietf-netmod-rfc6020bis-14.  It doesn't seem to be equivalent to
"schema tree", because that's defined as "The definition hierarchy
specified within a module.", i.e., the entire tree defined by a
module.  Template seems to be equivalent to "the subtree rooted at a
first-level schema node".

It may be that the phrase "YANG data template" would be better
replaced by some other phrase.

-- What is the significance of the phrase "a schema for modeling a
conceptual data structure in an encoding-independent manner"?

By assumption, all of the data in the implemented modules exists in an
encoding-independent manner, and the mere fact that it's accessible
via RESTCONF defines that it can be represented in XML and JSON.

-- What exactly does the yang-data extension statement do?

It seems to me that the draft doesn't need the extension.  For
example, in section 7.1 regarding error responses, the current text
is:

   When an error occurs for a request message on any resource type, and
   the status code that will be returned is in the "4xx" range (except
   for status code "403 Forbidden"), then the server SHOULD send a
   response message-body containing the information described by the
   "yang-errors" YANG template definition within the "ietf-restconf"
   module, found in Section 8.

But it seems to me that it would suffice to say 'The response
message-body contains a representation of an instance of the "errors"
container in the "ietf-restconf" module find in Section 8 (using the
module name "ietf-restconf" and the namespace
"urn:ietf:params:xml:ns:yang:ietf-restconf").'

So what is the particular value of the yang-data extension and why is
it not needed for schema trees defined in ordinary implemented
modules?

-- While we're at it, where is the specification that all Yang data
has representations in XML and JSON?

Clearly there must be such a specification, as that's necessary for
Restconf to work at all, but I've overlooked it.

- There seem to be six statements in the draft that the fragment field
in RESTCONF resource URIs has no defined purpose, either for some set
of resources or for all resources.  (sections 3.4, 3.5, 3.6, 3.8, 5.1
(twice)  But of course, the fragment identifier is never presented to
the HTTP/HTTPS server for the resource -- see RFC 7230 section 5.1:

   The target URI excludes the reference's fragment component, if any,
   since fragment identifiers are reserved for client-side processing
   ([RFC3986], Section 3.5).

It seems like a single statement of this fact would be sufficient, and
that section 5.1 is a natural place to put it, along with removing the
"fragment" field from the illustration of an HTTP request's
request-line:

        <OP> /<restconf>/<path>?<query>#<fragment>

          ^       ^        ^       ^         ^
          |       |        |       |         |
        method  entry  resource  query    fragment

          M       M        O        O         I

However, the discussion of fragment identifiers in the media type
definitions is correct and proper.

- There seems to be a general inconsistency in indentation -- see the
end of the review for a list showing how each HTTP example is
indented.

- There should be only one space between the request URI and
"HTTP/1.1".  (RFC 7230 section 3.1.1)  This is done incorrectly in:

    3.1.  Root Resource Discovery
	  GET /top/restconf/operations  HTTP/1.1
    3.3.1.  {+restconf}/data
	   ?content=nonconfig  HTTP/1.1
    3.3.3.  {+restconf}/yang-library-version
       GET /restconf/yang-library-version  HTTP/1.1
    4.3.  GET
	  library/artist=Foo%20Fighters/album=Wasting%20Light  HTTP/1.1
    4.4.2.  Invoke Operation Mode
	  POST /restconf/operations/example-jukebox:play   HTTP/1.1
    4.5.  PUT
	      library/artist=Foo%20Fighters/album=Wasting%20Light   HTTP/1.1
	   library/artist=Foo%20Fighters/album=Wasting%20Light   HTTP/1.1
    D.1.1.  Retrieve the Top-level API Resource
       GET /restconf   HTTP/1.1
    D.1.3.  Retrieve The Server Capability Information
	   capabilities  HTTP/1.1
    D.2.1.  Create New Data Resources
	   library/artist=Foo%20Fighters  HTTP/1.1
    D.3.5.  "point" Parameter
	      Wasting%20Light%2Fsong%3DBridge%20Burning   HTTP/1.1

- Percent-encoding

There are some details of percent-encoding that aren't fully
specified.  Percent-encoding is described in sections 3.5.3 and 5.1,
so I've consolidated the discussion here.

One is that the Yang string character set is Unicode, so to fully
specify how percent-encoding is done, we have to specify a character
representation to map Unicode characters into octets, after which
percent-encoding can be done.  These days the expected encoding for
Unicode is UTF-8, and percent-encoding using the UTF-8 representation
is described in the final paragraph of section 2.5 of RFC 3986.  This
means that the item in section 3.5.3 should be changed from

   o  The key value is specified as a string, using the canonical
      representation for the YANG data type.  Any reserved characters
      MUST be percent-encoded, according to [RFC3986], section 2.1.

to

   o  The key value is specified as a string, using the canonical
      representation for the YANG data type.  Any reserved characters
      MUST be percent-encoded, according to [RFC3986], sections 2.1 and
      2.5.

and section 5.1 should be changed from

   Any reserved characters MUST be percent-encoded, according to
   [RFC3986], sections 2.1 and 2.5.

- Must "," be percent-encoded when it appears in the key value for a
leaf-list node, given that a leaf-list path component cannot have more
than one key values?  See the comments for section 3.5.3.

- There are a number of questions regarding the ABNF in section
3.5.3.1 and precisely which URLs must conform to that syntax.  See the
comments for that section.

- Is the ietf-restconf module listed in
{+restconf}/data/ietf-restconf-monitoring:modules-state/module?  I can
see that this could be argued either way.  See the comments for
section 10.1.1.

- Generally, a colon in JSON is formatted with one space before and
one space after.  But these lines are formatted differently:

            "@mtu": {
        "ietf-restconf:errors": {
              "error-type": "protocol",
              "error-tag": "lock-denied",
              "error-message": "Lock failed, lock already held"
        "ietf-restconf:restconf": {
        "ietf-yang-library:modules-state": {
          "module-set-id": "5479120c17a619545ea6aff7aa19838b036ebbd7",
          "ietf-yang-library:modules-state": {

* Nits

Table of Contents

   ...
   8.  RESTCONF module . . . . . . . . . . . . . . . . . . . . . . .  66

This should be "RESTCONF Module".

1.  Introduction

   If a RESTCONF server is co-located with a NETCONF server, then there
   are protocol interactions to be considered, as described in
   Section 1.4.

This is a bit vague, as it doesn't specify with *what* protocols there
are interactions.  But this is obvious to most readers, as it's
concerned with interaction with the Netconf protocol.  But it's not
obvious to readers who don't know Netconf, because the preceding text
only describes Netconf as a system of datastore semantics.  So I think
this could be clarified by modifying this sentence:

   NETCONF defines configuration datastores and a set of Create,
   Retrieve, Update, Delete (CRUD) operations that can be used to access
   these datastores.

to

   NETCONF defines configuration datastores; a set of Create,
   Retrieve, Update, Delete (CRUD) operations that can be used to access
   these datastores; and a protocol for invoking those operations.

and modifying the above sentence to

   If a RESTCONF server is co-located with a NETCONF server, then there
   are protocol interactions with the NETCONF protocol, which are described in
   Section 1.4.

and changing the first sentence/paragraph of 1.4 to

   RESTCONF can be implemented on a device that supports the NETCONF protocol.

--

   Data-model specific RPC operations defined with the YANG "rpc" or [...]

I think this should start "Data-model-specific", as all of that phrase
is an adjective that modifies "RPC".  Similarly in the following
sentence, and other places in the draft.

1.1.5.  Terms

The term "operation" is used in at least three senses:

- HTTP request methods (e.g., the title of section 4)
- abstract CRUD operations
- RPC operations, viz., from the Yang "rpc" and "action" statements.

and also within

- edit operation: a RESTCONF operation on a data resource using
  either a POST, PUT, PATCH, or DELETE method.

You need to check that you have ways to specify each of these, and
that all uses of "operation" are unambiguous.  It might be useful to
mention this ambiguity in the lexicon item for "operations".

   o  API resource: a resource that models the RESTCONF entry point and
      the sub-resources to access YANG-defined content.  It is defined
      with the YANG data template named "yang-api" in the
      "ietf-restconf" module.

This should probably start "the resource that ...", as there is only one.

   o  data resource: a resource that models a YANG data node.  It is
      defined with YANG data definition statements, and YANG containers,
      leafs, leaf-list entries, list entries, anydata and anyxml nodes
      can be data resources.

It seems redundant to say "and YANG containers, leafs, leaf-list
entries, list entries, anydata and anyxml nodes can be data
resources.", because that is the whole list of YANG entities that can
be data nodes.  Worse, if a new YANG data node type is added, this
list will have to be updated.  Would it be acceptable to delete the
second sentence?

   o  datastore resource: a resource that models a programmatic
      interface to NETCONF datastores.

This should probably start "the resource that ...", as there is only one.

I think "NETCONF datastores" should be "the NETCONF datastore", since a
datastore resource must model exactly one NETCONF datastore.

   o  event stream resource: This resource represents an SSE (Server-
      Sent Events) event stream.  The content consists of text using the
      media type "text/event-stream", as defined by the SSE
      [W3C.REC-eventsource-20150203] specification.  Each event
      represents one <notification> message generated by the server.  It
      contains a conceptual system or data-model specific event that is
      delivered within an event notification stream.  Also called a
      "stream resource".

What is a "<notification> message"?  Is <notification> a concept from the
NETCONF protocol?  It is not used elsewhere in the draft.

Also, an event does not represent a notification message, a
notification message represents an event.

The "It" in "It contains a conceptual system" seems ambiguous.  Do you
mean "Each message contains ..."?

Also, it seems that "system or data-model specific event" would be
clearer as "system event or data-model-specific event", to make it
clear that "system" is an adjective.

   o  patch: a generic PATCH request on the target datastore or data
      resource.  The media type of the message-body content will
      identify the patch type in use.

Does the "generic" add meaning here, or can it be omitted without
loss?

   o  plain patch: a specific PATCH request type, defined in
      Section 4.6.1, that can be used for simple merge operations.  It
      has a representation with the media-type "application/yang-data"
      or "application/yang-data+json".

I don't think that "PATCH request type" is defined.  Also the use of
"representation" is odd.  Perhaps:

   o  plain patch: a specific PATCH request type, defined in
      Section 4.6.1, that can be used for simple merge operations.  It
      is specified by a request Content-Type of "application/yang-data"
      or "application/yang-data+json".

--

   o  RESTCONF capability: An optional RESTCONF protocol feature
      supported by the server, which is identified by an IANA registered
      NETCONF Capability URI, and advertised with an entry in the
      "capability" leaf-list in Section 9.3.

Probably should be "[...] leaf-list defined in Section 9.3.".

   o  RESTCONF client: a client which implements the RESTCONF protocol.
      Also called "client".

It seems to me that the "Also called ..." clauses (for "client",
"server", and "stream resource") should be turned into stand-alone
entries, as otherwise it's difficult to find the definition of
"client", etc.

   o  schema resource: a resource that has a representation with the
      media type "application/yang".  The schema resource is used by the
      client to retrieve the YANG schema with the GET method.

Is the defining characteristic of a schema resource that it 'has a
representation with the media type "application/yang"'?  Or is its
defining characteristic the fact that "the client [can use it] to
retrieve the YANG schema"?  If the latter, I suggest reversing the
two sentences:

   o  schema resource: a resource that can be used by the client to
      retrieve a YANG schema.  It has a representation with the
      media type "application/yang".

--

1.2.  Subset of NETCONF Functionality

   The HTTP POST, PUT, PATCH, and DELETE methods are used to edit data
   resources represented by YANG data models.  These basic edit
   operations allow the running configuration to be altered in an all-
   or-none fashion.

What is the meaning of "all-or-none" here?  Initially, I though it
meant that the actions were atomic, in that they either succeeded
completely or had no effect, but it might be referring to the fact
that RESTCONF cannot assemble a transaction from several elementary
editing operations.

   RESTCONF is not intended to replace NETCONF, but rather provide an
   additional interface that follows Representational State Transfer
   (REST) principles [rest-dissertation], and is compatible with a
   resource-oriented device abstraction.

Given the first sentence of section 1, the major goal of RESTCONF
seems to be to be HTTP-based, so it might be more accurate to say
"... to provide an HTTP interface that follows ...".

Could "compatible with a resource-oriented device abstraction" be more
usefully expressed as "compatible with the NETCONF datastore model"?

      +-----------+           +-----------------+
      |  Web app  | <-------> |                 |
      +-----------+   HTTP    | network device  |
                              |                 |
      +-----------+           |   +-----------+ |
      |  NMS app  | <-------> |   | datastore | |
      +-----------+  NETCONF  |   +-----------+ |
                              +-----------------+

The figure does not show that the "Web app" is using RESTCONF -- it's
an illustration of the use of RESTCONF that doesn't specify where
RESTCONF is used!  Perhaps change to "RESTCONF over HTTP"?

"NMS" is used nowhere else in the draft.  I assume it means "network
management system".  Perhaps all readers are expected to know that.
Otherwise, "NETCONF client" would be better.  (And can't a network
management system use Restconf?)

1.3.  Data Model Driven API

   Using YANG, a client can predict all management resources, much like
   using URI Templates [RFC6570], but in a more holistic manner.

Better,

   Knowing the YANG modules describing the server's data model, a
   client can derive all management resource URLs and the proper
   structure of all RESTCONF requests and responses.

--

   RESTCONF provides the YANG module capability information supported by
   the server, in case the client wants to use it.  The URIs for data-
   model specific RPC operations and datastore content are predictable,
   based on the YANG module definitions.

I've folded the second sentence of this passage into the previous
edit.  It seems like the first sentence could be phrased more simply,
"RESTCONF allows the client to retrieve the list of YANG modules
supported by the server."  Actually, I'm not sure that's correct; what
is "the YANG module capability information"?  The only use of
"capability" in draft-ietf-netmod-rfc6020bis-14 is for the capability
identifier URI urn:ietf:params:netconf:capability:yang-library:1.0,
which is mandatory to implement (since there is no other version
defined).

You might want to note here that the server might provide the
definitions of the modules that it supports.  E.g., "The server can
optionally support retrieval of the YANG modules it supports; see
Section 3.7."

   The RESTCONF datastore editing model is simple and direct, similar to
   the behavior of the :writable-running capability in NETCONF.  Each
   RESTCONF edit of a datastore resource is activated upon successful
   completion of the transaction.

I don't know the exact terminology that is used in this field, but I
think the final word would better be "edit".  The problem is that
"transaction" is used in the database world to mean everything
starting with the first edit operation and ending with the commit to
permanent storage.  With that definition, the "completion of the
transaction" happens only after the commit is finished.  But in this
context, the commit is the same as "a datastore resource is
activated"!

1.4.  Coexistence with NETCONF

It seems to me that the section would more accurately be described as
"Datastore and transaction management".

   If the device supports :startup, the device MUST automatically update
   the non-volatile "startup datastore", after the running datastore has
   been updated as a consequence of a RESTCONF edit operation.

Is there a reason that there are double-quotes around "startup
datastore"?  I do not see that usage in RFC 6241.

1.5.  RESTCONF Extensibility

   This document defines version 1 of the RESTCONF protocol.  If a
   future version of this protocol is defined, then that document will
   specify how the new version of RESTCONF is identified.  It is
   expected that a different entry point {+restconf2} would be defined.

The symbol "{+restconf2}" has no defined meaning.  I think you want
"It is expected that a different entry point will be used, which will
be located using a different link relation (Section 3.1)."

      Response
      --------
      HTTP/1.1 200 OK
      Content-Type: application/xrd+xml
      Content-Length: nnn

   <XRD xmlns='

http://docs.oasis-open.org/ns/xri/xrd-1.0

'>
       <Link rel='restconf' href='/restconf'/>
       <Link rel='restconf2' href='/restconf2'/>
   </XRD>

The response message-body isn't indented to the same margin as the
headers.

   Typically this extension mechanism is used to identify optional query
   parameters but it is not limited to that purpose.

Should this say "optional query parameters that are supported"?  Also,
could "Typically" be improved as "Currently"?

2.2.  HTTPS with X.509v3 Certificates

   The use the X.509v3 based certificates is consistent with NETCONF
   over TLS [RFC7589].

There is a syntax error in the first few words of this sentence.

   The RESTCONF client MUST either use X.509 certificate path validation
   [RFC5280] to verify the integrity of the RESTCONF server's TLS
   certificate, or match the presented X.509 certificate with locally
   configured certificate fingerprints.

   The presented X.509 certificate MUST also be considered valid if it
   matches a locally configured certificate fingerprint.  If X.509
   certificate path validation fails and the presented X.509 certificate
   does not match a locally configured certificate fingerprint, the
   connection MUST be terminated as defined in [RFC5246].

The second sentence seems to be redundant with the second clause of
the first sentence.  (Or is it intended that the client MUST be
configurable with fingerprints?)  The last sentence seems to be
verbose; it should be possible to say "If the certificate cannot be
validated by either method, ...".

3.  Resources

   A resource has a representation associated with a media type
   identifier, as represented by the "Content-Type" header in the HTTP
   response message.

This seems awkward.  Perhaps:

   A resource has one or more representations, each associated with a
   different media type.  When a representation of a resource is sent
   in an HTTP response message, the associated media type is given in
   the "Content-Type" header.

--

   All RESTCONF resource types are defined in this document except
   specific datastore contents, RPC operations, and event notifications.

This depends on exactly what "resource type" means.  As discussed at
the beginning, that phrase seems to mean a small, finite number of
resource classes that are all defined in this document.  I think what
this sentence is trying to get at is that the specific resources of a
particular server that are in each of the resource type classes is
determined by the set of modules the server implements.  But that
isn't what the sentence says (assuming I'm correct about what
"resource type" means).

   The syntax and semantics for these resource types are defined in YANG
   modules.

Perhaps "... are defined in the YANG modules that the server
implements.".

   The RESTCONF protocol does not include a data resource discovery
   mechanism.  Instead, the definitions within the YANG modules
   advertised by the server are used to construct a predictable
   operation or data resource identifier.

It seems like "predictable" is redundant given the use of "construct".

3.1.  Root Resource Discovery

   After discovering the RESTCONF API root, the client MUST prepend it
   to any subsequent request to a RESTCONF resource.

It's not actually prepended to the *request*, it's the initial part of
the path of the request URI:

   The RESTCONF API root is used as the initial part of the path of
   the request URI of any request to a RESTCONF resource.

Because of the resource discovery system, any given host:port can
support only one RESTCONF server.  This means that a particular host
is not expected to support an arbitrarily large number of RESTCONF
servers, as each server must use a different port.  I doubt this is a
practical limitation for the intended usages, but it's probably worth
mentioning in the Introduction.

3.3.  API Resource

   +--rw restconf
      +--rw data
      +--rw operations
      +--ro yang-library-version

That seems to imply that the root resource has the name "restconf" (or
perhaps that the last component of its name is "restconf").  I think
you could be more accurate (with some abuse of notation) by:

   +--rw {+restconf}
      +--rw data
      +--rw operations
      +--ro yang-library-version

Section 1.7 says 'Ellipsis ("...") stands for contents of subtrees
that are not shown.', but ellipsis is not exploited here to clarify
where in the tree additional nodes will be:

   +--rw {+restconf}
      +--rw data
      |  ...
      +--rw operations?
      |  ...
      +--ro yang-library-version

Section 3.3.2 states that "operations" is optional, so there should be
a "?" after "operations".

   The "yang-api" YANG data template is defined with the "yang-data"
   extension in the "ietf-restconf" module, found in Section 8.  It is
   used to specify the structure and syntax of the conceptual child
   resources within the API resource.

I think this would be clearer if you said '... defined using the
"yang-data" extension', and "It specifies the structure and syntax
...".

3.4.  Datastore Resource

   The "{+restconf}/data" subtree represents the datastore resource
   type, which is a collection of configuration data and state data
   nodes.

Shouldn't this be "... represents the datastore, which is ..."?

   This resource type is an abstraction of the system's underlying
   datastore implementation.  It is used to simplify resource editing
   for the client.  The RESTCONF datastore resource is a conceptual
   collection of all configuration and state data that is present on the
   device.

The first sentence is extremely clear.  The second sentence is odd;
doesn't it mean "The client uses it to edit resources."?  The third
sentence seems to be a longer statement of the first sentence, given
what the datastore is.

   Configuration edit transaction management and configuration
   persistence are handled by the server and not controlled by the
   client.  A datastore resource can be written directly with the POST
   and PATCH methods.  Each RESTCONF edit of a datastore resource is
   saved to non-volatile storage by the server, if the server supports
   non-volatile storage of configuration data.

The second sentence makes sense in this context.  The first and third
sentences are datastore management, which is discussed in section 1.4.

3.4.1.  Edit Collision Detection

   Two "edit collision detection" mechanisms are provided in RESTCONF,
   for datastore and data resources.

Why are there double-quotes around "edit collision detection"?

There seems to be a third mechanism:  if the datastore is locked, the
request gets a 409 response (section 1.4).  Though maybe that's more
"collision prevention" than "collision detection".

3.4.1.1.  Timestamp

   If the RESTCONF server is colocated with a NETCONF server, then the
   last-modified timestamp MUST represent the "running" datastore.

"represent" isn't the right word here.  Perhaps "... MUST be that of
...".  Of course, this fact is implicit in the Restconf definition,
because Restconf provides access only to the running datastore, but
it's worth warning the implementer explicitly.

3.4.1.2.  Entity tag

   A unique opaque string is maintained and the "ETag" ([RFC7232],
   Section 2.3) header is returned in the response for a retrieval
   request.  The "If-Match" header can be used in edit operation
   requests to cause the server to reject the request if the resource
   entity tag does not match the specified value.

It seems you want 2119 language here:

   The server MUST maintain a unique opaque entity-tag for the
   datastore resource and MUST return it in the "ETag" ([RFC7232],
   Section 2.3) header in the response for a retrieval request.  The
   client MAY use an "If-Match" header in edit operation requests to
   cause the server to reject the request if the resource entity tag
   does not match the specified value.

An interesting point that you may want to warn the reader about is
that the entity-tag encodes not only the data in the resource but also
its representation; different representations (XML vs. JSON) must have
different entity-tags.  (RFC 7323 section 2.3)

   If the RESTCONF server is colocated with a NETCONF server, then this
   entity tag MUST represent the "running" datastore.

"represent" isn't the right word here.  Perhaps "... MUST be that of ...".

3.4.1.3.  Update Procedure

It seems like the substantial part of this section is "Changes to
configuration data resources affect the timestamp and entity tag to
... the datastore resource."  The rest of what it says is more fully
described in section 3.5.  It seems that the real point is to state
that changing any of the configuration resources changes the
timestamp/ETag of the datastore resource, which is more or less
understood, given that the datastore has a timestamp/ETag.  But it
seems that this could be more clearly said in 3.4.1:

   Two "edit collision detection" mechanisms are provided in RESTCONF
   for the datastore:  a timestamp and an ETag.  (These may also be
   provided for data resources; see Section 3.5.)  Any change to
   configuration data resources updates the timestamp and entity tag
   of the datastore resource.

3.5.  Data Resource

   A configuration data resource can be altered by the client with some
   or all of the edit operations, depending on the target resource and
   the specific operation.  Refer to Section 4 for more details on edit
   operations.

It seems like this paragraph is largely redundant.

3.5.3.  Encoding Data Resource Identifiers in the Request URI

   In YANG, data nodes are identified with an absolute XPath expression,
   defined in [XPath], starting from the document root to the target
   resource.  In RESTCONF, URI-encoded path expressions are used
   instead.

I think "are identified" should be "can be identified" -- relative
XPath is also allowed in Yang.

   If a node in the
   path is defined in another module than its parent node, then module
   name followed by a colon character (":") is prepended to the node
   name in the resource identifier.

That should start "If a node in the path is defined in another module
than its parent node, or its parent is the datastore, then module
...".

The following item appears in the list concerning list nodes:

   o  The key value is specified as a string, using the canonical
      representation for the YANG data type.  Any reserved characters
      MUST be percent-encoded, according to [RFC3986], section 2.1.

This fact also needs to be specified for leaf-list nodes, so either
this item should be duplicated into the list of items for leaf-list
nodes, or it should be pulled out as a top-level paragraph.

The reader needs to be warned that in the the character "," in a key
value must also be percent-encoded, despite that it is not considered
"reserved" in any definition of URL.

This leads to a technical nit:  Must "," be percent-encoded when it
appears in the key value for a leaf-list node, given that a leaf-list
path component cannot have more than one key values?  (Whether this is
specified as true or false is not important, but the standard needs to
fix the requirement.)

   Resource URI values returned in Location headers for data resources
   MUST identify the module name as specified in
   [I-D.ietf-netmod-yang-json], even if there are no conflicting local
   names when the resource is created.  This ensures the correct
   resource will be identified even if the server loads a new module
   that the old client does not know about.

It's not clear that this restriction adds anything.  Module names are
required in resource URIs already, by "If a node in the path is
defined in another module than its parent node, [or its parent is the
datastore,] then module name followed by a colon character (":") is
prepended to the node name in the resource identifier."

3.5.3.1.  ABNF For Data Resource Identifiers

       api-path = "/"  |
                  ("/" api-identifier
                    0*("/" (api-identifier | list-instance )))

I'm assuming there that <api-path> is the syntax for the path *within*
the API's section of the URL space, that is, the string which is
appended to the entry point {+restconf}.  This seems necessary, as
otherwise the segments of the entry point path would have to conform
to <api-identifier>, and that restriction seems pointless.  But this
point should probably be clarified in the text.

It seems that this ABNF only applies to the paths of data resource
identifiers, not other resource types.  But in that case, why does the
syntax allow "/" alone, which isn't the path of a data resource?
Indeed, a data resource path must start with "/data/" and be followed
by at least one segment.  ({+restconf}/data is the path of the
datastore resource, which is a different resource type.)

       key-value = string      ;; note 1

Of course, this is omitting the constraints about percent-encoding
reserved characters and comma.

   Note 1: The syntax for "api-identifier" and "key-value" MUST conform
   to the JSON identifier encoding rules in Section 4 of
   [I-D.ietf-netmod-yang-json].

Is this note actually correct?

3.6.  Operation Resource

   For example, if "module-A" defined a "reset" rpc operation, then
   invoking the operation from "module-A" would be requested as follows:

It seems like 'from "module-A"' is redundant in this context; we've
already stated that the operation is in module-A.

   All operation resources representing RPC operations supported by the
   server MUST be identified in the {+restconf}/operations subtree
   defined in Section 3.3.2.  Operation resources representing YANG
   actions are not identified in this subtree since they are invoked
   using a URI within the {+restconf}/data subtree.

Isn't this paragraph redundant given the discussion at the beginning
of the section?

3.6.2.  Encoding Operation Resource Output Parameters

   The request URI is not returned in the response.  This URI might have
   context information required to associate the output to the specific
   "rpc" or "action" statement used in the request.

Doesn't HTTP always allow the client to associate the response with
the request that generated it?  If so, this paragraph is not really
correct, as a returned request URI will not be "required" for the
client to perform the association.  Perhaps what is meant is
"knowledge of the request URI is needed to associate the output to the
specific "rpc" or "action" statement referenced in the request".

3.8.  Event Stream Resource

   The available streams can be retrieved from the stream list, which
   specifies the syntax and semantics of a stream resource.

Shouldn't that be "... the syntax and semantics of the stream resources"?

3.9.  Errors YANG Data Template

   An "errors" YANG data template models a collection of error
   information [...]

Presumably, 'The "errors" YANG data template ...'.

4.  Operations

   The following table shows how the RESTCONF operations relate to
   NETCONF protocol operations and edit operations, which are identified
   with the NETCONF "nc:operation" attribute.

For people who don't know NETCONF, it would be clearer to say
'... NETCONF protocol operations, and for the NETCONF edit operations,
the "nc:operation" attribute.'

   In particular, RESTCONF is compatible with the NETCONF
   Access Control Model (NACM) [RFC6536], as there is a specific mapping
   between RESTCONF and NETCONF operations, defined in Section 4.

Since this text is contains in Section 4, you could omit "defined in
Section 4".

   Implementation of all methods (except PATCH) are defined in
   [RFC7231].  This section defines the RESTCONF protocol usage for each
   HTTP method.

Why no RFC reference for the definition of PATCH?

4.2.  HEAD

   The HEAD method is sent by the client to retrieve just the headers
   that would be returned for the comparable GET method, without the
   response message-body.  It is supported for all resource types,
   except operation resources.

It seems a lot safer (and future-proof) to say "It is supported by all
resources that support GET."

Also, it might be helpful to expand the first sentence, "... just the
headers (which contain the metadata for a resource) ...".

4.3.  GET

   The request MUST contain a request URI that contains at least the
   entry point.

Uses of the term "entry point" are scattered through the document but
not listed in 1.1.5.

I assume that it is implicit how a JSON GET should return multiple
values.  It might help the reader to include an example of a GET that
returns multiple nodes (using JSON, of course).

BTW, what ETag and timestamp is returned by a GET whose URI identifies
more than one value?

4.5.  PUT

   If the target resource represents a YANG list instance, then the PUT
   method MUST NOT change any key leaf values in the message-body
   representation.

Shouldn't this be "... any key leaf values in the instance."?

4.6.1.  Plain Patch

   Please see
   [I-D.ietf-netconf-yang-patch] for an alternate media-type supporting
   the ability to delete child resources.  The YANG Patch Media Type
   allows multiple sub-operations (e.g., merge, delete) within a single
   PATCH operation.

It seems these two sentences would be clearer if they were combined:

   Please see [I-D.ietf-netconf-yang-patch] for an alternate
   media-type that supports the deleting child resources and
   specifying multiple sub-operations (e.g., merge, delete) within a
   single PATCH operation.

--

   If the target resource represents a YANG list instance, then the
   PATCH method MUST NOT change any key leaf values in the message-body
   representation.

Shouldn't that be "... any key leaf values in the instance"?

Section 4.6.1 needs to specify that the plain patch operation never
returns a response message-body.

4.7.  DELETE

   To delete a resource such as the "album" resource, the client might
   send:

This would be clearer if it was

   To delete the "album" resource with the key "Wasting Light", the client
   might send:

4.8.2.  The "depth" Query Parameter

   The "depth" parameter is used to specify the number of nest levels
   returned in a response for a GET method.

This would be better as 'Data nodes with a depth value exceeding the
"depth" parameter are not returned in a response for a GET method.'

   The first nest-level consists of the requested data node itself.

It seems that this would be more clear as "The requested data node
itself has depth value of 1."

   If the "fields" parameter (Section 4.8.3) is used to select
   descendant data nodes, these nodes all have a depth value of 1.

You want to augment this as:

   If the "fields" parameter (Section 4.8.3) is used to select
   descendant data nodes, these nodes and all their ancestors have a
   depth value of 1.

The following text could be clarified by adding parentheses around
this sentence:

   (This has the effect of including the nodes specified by the
   fields, even if the "depth" value is less than the actual depth
   level of the specified fields.)

--

   Any child nodes which are contained within a parent node have a
   depth value that is 1 greater than its parent.

Probably better as "Any other child node has a depth value that is ...".

   By default, the server will include all sub-resources within a
   retrieved resource, which have the same resource type as the
   requested resource.  The exception is the datastore resource.  If
   this resource type is retrieved then by default the datastore and all
   child data resources are returned.

This specification requires that "resource type" be well-defined.

4.8.3.  The "fields" Query Parameter

There should be a note that using "fields" does not select out a set
of nodes whose values are returned as a *set* of values, it returns a
*single* value, which is the value of the target data node, pruned by
the removal of all nodes that are not ancestors of or descendants of
one of the nodes specified in the "fields" parameter.  This is
particularly important when considering the interaction of "fields"
and "depth", and also is significant when the response representation
is XML.

4.8.4.  The "filter" Query Parameter

   This parameter is only allowed for GET methods on a text/event-stream
   data resource.

This is probably better put as "... on an event stream resource".

Similarly in section 4.8.7.

4.8.7.  The "start-time" Query Parameter

   If this query parameter is supported by the server, then the "replay"
   query parameter URI MUST be listed in the "capability" leaf-list in
   Section 9.3.  The "stop-time" query parameter MUST also be supported
   by the server.

For correctness, I think you need to join the two sentences:
'... leaf-list in Section 9.3, and the "stop-time" query parameter
MUST ...'.

Similarly in section 4.8.8.

4.8.9.  The "with-defaults" Query Parameter

   If the "with-defaults" parameter is set to "report-all-tagged" then
   the server MUST adhere to the defaults reporting behavior defined in
   Section 3.4 of [RFC6243].

This needs a note that section 5.3 provides additional information
about how default values are marked in responses when "with-defaults"
is "report-all-tagged".

5.2.  Message Encoding

   JSON encoding rules are defined in
   [I-D.ietf-netmod-yang-json].  JSON encoding of metadata is defined in
   [I-D.ietf-netmod-yang-metadata].

There needs to be some sort of juncture between these two sentences.
Does I-D.ietf-netmod-yang-json apply to metadata as well?  If so, the
second sentence should start "Additional JSON encoding rules for
metadata are ...".  If not, the subject of the first sentence should
be modified to show that metadata is excluded.  ("JSON encoding rules
for data are ..."?)

   Response output content
   encoding format is identified with the Accept header in the request.

This should be

   The response output content encoding formats that the client will
   accept are identified with the Accept header in the request.

--

   File extensions encoded
   in the request are not used to identify format encoding.

I don't know of any place where a file extension can be encoded in the
request.  But I get the unpleasant feeling that some server in the
wild has been seen with this behavior and this warning is needed!

5.3.  RESTCONF Metadata

   With the XML encoding, the metadata is encoded as attributes in XML.

There should be some reference here as to how it should be done.
Presumably this is a reference to RFC 6243.

5.3.2.  JSON MetaData Encoding Example

   [...] so the YANG module name has to be assigned instead of derived
   from the YANG module name.

I don't think this is can be correct; one use of "YANG module name" is
probably intended to be something else.

"MetaData" should be "Metadata".

6.3.1.  NETCONF Event Stream

   The server SHOULD support the "NETCONF" notification stream defined
   in [RFC5277].

It might help to note that the reference is to section 3.2.3 of RFC
5277.

Is "notification stream" the same as "event stream"?  It seems like
the draft should use one term or the other consistently.

There is a lot of information about query parameters here, but it
seems that it duplicates the discussion of query parameters elsewhere
in the draft.  Could it be abbreviated?

6.4.  Receiving Event Notifications

   The structure of the event data is based on the "notification"
   element definition in Section 4 of [RFC5277].  It MUST conform to the
   schema for the "notification" element in Section 4 of [RFC5277],
   except the XML namespace for this element is defined as:

   urn:ietf:params:xml:ns:yang:ietf-restconf

"this element" is somewhat ambiguous; probably better as "the event
data element".

   RESTCONF servers that do send the
   "id" field MUST still support the "startTime" query parameter as the
   preferred means for a client to specify where to restart the event
   stream.

It seems "startTime" should be "start-time".

But what is the significance of "MUST" here?  Implementing
"start-time" is always optional (section 6.3.1).  Perhaps it would
work to say

    RESTCONF servers that send the "id" field SHOULD support the
    "start-time" query parameter, as "start-time" is the preferred
    means for specifying where to restart fetching the event stream.

7.  Error Reporting

   The <rpc-error> element returned in NETCONF error
   responses contains some useful information.

I got confused by this sentence.  IIRC, this section is about error
responses generally, not just error responses for RPC operations.  It
seems the problem is that *all* Netconf operations are considered
RPCs, so <rpc-error> covers all Netconf errors.  I think this could be
made less confusing by combining this sentence with the following one
as:

   The error information that NETCONF error responses contain in the
   <rpc-error> element is adapted for use in RESTCONF, and an <errors>
   data tree information is returned for "4xx" and "5xx" class of
   status codes.

--

   [...] a mapping between the NETCONF <error-tag> value and the HTTP status
   code is needed.

Better to say "a mapping from the ... to the HTTP status code is
needed." since the reverse mapping is not unique.

   The semantics and syntax for RESTCONF error messages are defined with
   the "yang-errors" YANG data template extension, found in Section 8.

What is a "YANG data template extension"?  It probably means "YANG
data template extension statement", but I think it could be reduced to
'the "errors" YANG data template'.  (Of course, see my comments about
"YANG data template" at the beginning.)

7.1.  Error Response Message

   The Content-Type of this response
   message MUST be a subtype of application/yang-data (see example
   below).

This isn't the meaning of "subtype", which seems to be restricted (RFC
6838) to mean the part of the media type after "/".  This should work,
though:

   The Content-Type of this response message MUST be
   application/yang-data, plus optionally a structured syntax name
   suffix.

"structured syntax name suffix" is defined in RFC 6838 section 4.2.8.

   The client SHOULD specify the desired encoding for error messages by
   specifying the appropriate media-type in the Accept header.  If no
   error media is specified, [...]

The client will specify the desired encoding for all responses, which
perforce applies to error messages as well.  This can probably be
reduced to:

   The client SHOULD specify the desired encoding(s) for response
   messages by specifying the appropriate media-type(s) in the Accept
   header.  If the client did not specify an Accept header, [...]

--

   No response message-body content is expected by the
   client in this example.

Well, no content is expected on success, but the client was prepared
for failure by providing an Accept.  Better say

   There would be no response message-body content if this operation
   was successful.

The indentation of the DELETE example request and its response are
different, which should probably be fixed.

8.  RESTCONF module

        Note that the YANG definitions within this module do not
        represent configuration data of any kind.

Are there kinds of configuration data?  Seems like it would be better
to say "... are never configuration data."

And indeed, we could add 'config "false";' to the top-level objects to
specify that directly in Yang.

     ...

     extension yang-data {
      argument name {

Is there a reason that the substatements of this extension statement
are indented only 1 space?

        yin-element true;
      }
      description
        "This extension is used to specify a YANG data template which
         represents conceptual data defined in YANG. It is
         intended to describe hierarchical data independent of
         protocol context or specific message encoding format.
         Data definition statements within this extension specify
         the generic syntax for the specific YANG data template.

"this extension" is ambiguous, since it seems to me that it might be
referring to the immediately preceding extension-statement.  I think
the last sentence would be clearer as

         Data definition statements within a yang-data extension
         statement specify the generic syntax for the specific YANG
         data template, whose name is the argument of the yang-data
         extension statement.

--

         This extension is ignored unless it appears as a top-level
         statement. It SHOULD contain data definition statements
         that result in exactly one container data node definition.

It seems like you can assure that the extension will never be used in
an improper place, since only this document will use it.  Perhaps

         The yang-data extension MUST only be a top-level statement of
         a module.  It MUST contain only one schema node, which is a
         container.

--

         This allows compliant translation to an XML instance
         document for each YANG data template.

This needs to specify the source of the name of the top-level XML
element:

	 Instances of a YANG data template can thus be translated into
	 XML instances, whose top-level element corresponds to the
	 top-level container.

--

         The following data-def-stmt sub-statements have special
         meaning when used within a yang-data-resource extension
         statement.
         - The list-stmt is not required to have a key-stmt defined.
         - The if-feature-stmt is ignored if present.
         - The config-stmt is ignored if present.
         - The available identity values for any 'identityref'
           leaf or leaf-list nodes is limited to the module
           containing this extension statement, and the modules
           imported into that module.

It seems like poor practice to have the extension be described as
changing the semantics of Yang.  Better would be to turn these into
constraints, so that the valid contents of yang-data are a subset of
Yang, but that subset has the same semantics as Yang prescribes:

         - The if-feature-stmt must not be present.
         - If the config-stmt is present, its value must be 'false'.
         - The available identity values for any 'identityref'
           leaf or leaf-list nodes is limited to the module
           containing this extension statement, and the modules
           imported into that module. [unchanged!]

The item "The list-stmt is not required to have a key-stmt defined."
is redundant, since everything inside yang-data is not configuration
data, and non-configuration lists need not have keys.

This section contains the following lines which name media types which
do not exist.  Presumably this is a simple oversight and the correct
types are known.

          application/yang-api resource type.";

            application/yang-api resource type.";

             "Container representing the application/yang-datastore

              (application/yang-operation),

9.  RESTCONF Monitoring

   The "ietf-restconf-monitoring" module provides information about the
   RESTCONF protocol capabilities and event notification streams
   available from the server.  A RESTCONF server MUST implement the "/
   restconf-state/capabilities" container in this module.

Note that there is a line break in an incorrect place at the end of
the third line.  Presumably it is due to an extraneous space in the
XML2RFC file.

It seems like the second sentence would be more informative as "A
RESTCONF server MUST implement the ietf-restconf-monitoring module."
Since restconf-state/capabilities is mandatory within the module, its
existence need not be stated here.

   YANG Tree Diagram for "ietf-restconf-monitoring" module:

Shouldn't that be "YANG tree diagram ..."?

The nodes 'description' and 'replay-support' are shown as optional in
the tree diagram:

   +--ro restconf-state
      +--ro streams
         +--ro stream* [name]
            +--ro description?                string
            +--ro replay-support?             boolean
            +--ro replay-log-creation-time?   yang:date-and-time

but there is no 'mandatory "false";' for those leafs in the module
definition in section 9.3:

           leaf description {
             type string;
             description "Description of stream content";
             reference
               "RFC 5277, Section 3.4, <description> element.";
           }

           leaf replay-support {
             type boolean;
             description
               "Indicates if replay buffer supported for this stream.
                If 'true', then the server MUST support the 'start-time'
                and 'stop-time' query parameters for this stream.";
             reference
               "RFC 5277, Section 3.4, <replaySupport> element.";
           }

And the description should say that if replay-support is missing, its
assumed value is false[?].  Or should a 'default' statement be
employed?

9.1.2.  The "defaults" Protocol Capability URI

This section is awkwardly phrased.  Perhaps:

   This URI identifies the "basic-mode" defaults handling mode that is
   used by the server for processing default leafs in requests for
   data resources.  This protocol capability URI MUST be supported by
   the server, and MUST be listed in the "capability" leaf-list in
   Section 9.3.

      +----------+--------------------------------------------------+
      | Name     | URI                                              |
      +----------+--------------------------------------------------+
      | defaults | urn:ietf:params:restconf:capability:defaults:1.0 |
      +----------+--------------------------------------------------+

                     RESTCONF defaults capability URI

   This URI MUST have attached a query a parameter named "basic-mode"
   with one of the values listed below:

   +------------------+------------------------------------------------+
   | Value            | Description                                    |
   +------------------+------------------------------------------------+
   | report-all       | No data nodes are considered default           |
   | trim             | Values set to the YANG default-stmt value are  |
   |                  | default                                        |
   | explicit         | Values set by the client are never considered  |
   |                  | default                                        |
   +------------------+------------------------------------------------+

   The "basic-mode" behavior of the server is as specified for this
   value in "With-Defaults Capability for NETCONF" [RFC6243]:

   If the "basic-mode" is set to "report-all" then the server MUST
   adhere to the defaults handling behavior defined in Section 2.1 of
   [RFC6243].
   [...]

9.2.  restconf-state/streams

   This optional container provides access to the event notification
   streams supported by the server.  The server MAY omit this container
   if no event notification streams are supported.

The second sentence is redundant, given that "streams" is a
non-presence container, but it seems reasonable to warn the
implementer here.

10.1.  modules-state

   This mandatory container holds the identifiers for the YANG data
   model modules supported by the server.

This isn't how I'd phrase it.  I would say that modules-state/module
holds the identifiers.  Perhaps omit this section and number 10.1.1 as
"10.1"?

10.1.1.  modules-state/module

If I understand correctly, the resource tree under {+restconf} is
described by the ietf-restconf module.  All data resources not defined
by ietf-restconf are descendants of {+restconf}/data, including the
mandatory ietf-restconf-monitoring module.  But is ietf-restconf
listed in
{+restconf}/data/ietf-restconf-monitoring:modules-state/module?  I can
see that this could be argued either way.

12.  Security Considerations

   The "ietf-restconf-monitoring" YANG module defined in this memo is
   designed to be accessed via the NETCONF protocol [RFC6241].  The
   lowest NETCONF layer is the secure transport layer, and the
   mandatory-to-implement secure transport is Secure Shell (SSH)
   [RFC6242].  The NETCONF access control model [RFC6536] provides the
   means to restrict access for particular NETCONF users to a pre-
   configured subset of all available NETCONF protocol operations and
   content.

This reads oddly.  "ietf-restconf-monitoring" is designed to be
accessed via the RESTCONF protocol.  However, RESTCONF does use the
NETCONF access control model.  OTOH, this paragraph really isn't about
ietf-restconf-monitoring, it's very general.  It seems to me that you
want (and I may have the details wrong):

   The lowest RESTCONF layer is HTTPS, and the mandatory-to-implement
   secure transport is TLS [RFC5246].  The RESTCONF protocol uses the
   NETCONF access control model [RFC6536], which provides the means to
   restrict access for particular RESTCONF users to a preconfigured
   subset of all available RESTCONF protocol operations and content.

   This section provides security considerations for the resources
   defined by the RESTCONF protocol.  Security considerations for
   HTTPS are defined in [RFC7230].  RESTCONF does not specify which
   YANG modules a server needs to support, other than the
   "ietf-restconf-monitoring" module.  Security considerations for the
   other modules manipulated by RESTCONF can be found in the documents
   defining those YANG modules.

14.1.  Normative References

This draft has the longest normative references list I've ever seen!

14.2.  Informative References

   [rest-dissertation]
              Fielding, R., "Architectural Styles and the Design of
              Network-based Software Architectures", 2000.

Surely there is additional bibliographic information available for
this!

Appendix C.  Example YANG Module

   +---x play
      +--ro input
         +--ro playlist       string
         +--ro song-number    uint32

The "x" indicator is not listed in section 1.1.7.  Is it "standard"
for Yang tree diagrams?

The "+---x" before "play" should be "+--x".  This will also fix the
indenting problem.

C.1.  example-jukebox YANG Module

      ...
      contact "support at example.com";

Comparing with the examples of the contact statement in
draft-ietf-netmod-rfc6020bis-14, it seems that this should be
"support at example.com".

Appendix D.  RESTCONF Message Examples

   The examples within this document use the normative YANG module
   defined in Section 8 and the non-normative example YANG module
   defined in Appendix C.1.

This would flow better (for me) if it included the module names:

   The examples within this document use the normative YANG module
   "ietf-restconf" defined in Section 8 and the non-normative example
   YANG module "example-jukebox" defined in Appendix C.1.

D.1.1.  Retrieve the Top-level API Resource

      HTTP/1.1 200 OK
      Date: Mon, 23 Apr 2012 17:01:00 GMT
      Server: example-server
      Content-Type: application/yang-data+json

      {
        "ietf-restconf:restconf": {
          "data" : {},
          "operations" : {},
          "yang-library-version" : "2016-04-09"
        }
      }

   The server will return the same response either way, which might be
   as follows :

It's not really the "same response".  "conceptual data" seems better.

D.2.2.  Detect Resource Entity Tag Change

   In this example, the server just supports the datastore last-changed
   timestamp.  The client has previously retrieved the "Last-Modified"
   header and has some value cached to provide in the following request
   to patch an "album" list entry with key value "Wasting Light".  Only
   the "genre" field is being updated.

IIRC, the "value cached" is the URI
/restconf/data/example-jukebox:jukebox/library/artist=Foo%20Fighters/album=Wasting%20Light
for the album.  It seems this could be clearer as

   After the previous request, the client has cached the
   "Last-Modified" header and the Location header from the response to
   provide the following request to patch the "album" list entry with
   key value "Wasting Light".

And then change the If-Unmodified-Since header in the request to use
the value from the previous response, "Mon, 23 Apr 2012 17:03:00 GMT".

D.2.3.  Edit a Datastore Resource

   In this example, the client modifies two different data nodes by
   sending a PATCH to the datastore resource:

"different" might be redundant.

It might be worth pointing out which nodes are being modified.  As far
as I can see, the only modifiable nodes in the request data tree are
the "year" nodes, but the "year" for "Wasting Light" in this update is
the same as in its creation in D.2.1.  Should the request be modified?

D.2.4.  Edit a Data Resource

   In this example, the client modifies one data nodes by sending a
   PATCH to the data resource (URI wrapped for display purposes only):

   PATCH /restconf/data/example-jukebox:jukebox/library/
      artist=Nick%20Cave%20and%20the%Bad%20Seeds HTTP/1.1
   Host: example.com
   Content-Type: application/yang-data

   <artist xmlns="

http://example.com/ns/example-jukebox&quot

;>
     <name>Nick Cave and the Bad Seeds</name>
     <album>
       <name>The Good Son</name>
       <year>1990</year>
     </album>
   </artist>

Should be "one data node".

The example isn't very clear to me:  Which data node(s) is the PATCH
intended to modify?  Does "modify a data node" mean that one single
leaf is being modified, or does it include modification of an entire
subtree?  "the data resource" seems to be an "artist" node, but that
node per se isn't being modified.

I think it would help to write what changes the PATCH was intended to
cause, and adjust the wording accordingly.

D.3.2.  "depth" Parameter

   To determine if 1 or more resource instances exist for a given target
   resource, the value "1" is used.

Usually, this "1" would be spelled out as "one".

The following two sections probably should have short text identifying
what they illustrate.  E.g.,

D.3.7.  "start-time" Parameter

   The following URI shows an example of a start-time specification:

   // start-time = 2014-10-25T10:02:00Z
   GET /streams/NETCONF?start-time=2014-10-25T10%3A02%3A00Z

D.3.8.  "stop-time" Parameter

   The following URI shows an example of a stop-time specification:

   // stop-time = 2014-10-25T12:31:00Z
   GET /mystreams/NETCONF?stop-time=2014-10-25T12%3A31%3A00Z

This example is invalid, since it doesn't contain a start-time.
(section 4.8.8) Perhaps:

   The following URI shows an example of a stop-time specification
   (lines wrapped for display purposes only):

   // start-time = 2014-10-25T10:02:00Z
   // stop-time = 2014-10-25T12:31:00Z
   GET /streams/NETCONF?
      start-time=2014-10-25T10%3A02%3A00Z&
      stop-time=2014-10-25T12%3A31%3A00Z

D.3.9.  "with-defaults" Parameter

   The following YANG module is assumed for this example.

     module example-interface {
       prefix "exif";
       namespace "urn:example.com:params:xml:ns:yang:example-interface";

       container interfaces {
         list interface {
           key name;
           leaf name { type string; }
           leaf mtu { type uint32; }
           leaf status {
             config false;
             type enumeration {
               enum up;
               enum down;
               enum testing;
             }
           }
         }
       }
     }

As far as I can tell, module "example-interface" isn't used in this
section.  "example-interface" seems to be an abbreviated version of
module "example" in section A.1 of RFC 6243, but with the semantic
difference that "example-interface" omits the default statement that
drives the examples in this section.  It seems that you want to drop
"example-interface" entirely and start the section with:

   Assume the server implements the module "example" defined in
   Appendix A.1 of [RFC6243].  Assume the server's datastore is as
   defined in Appendix A.2 of [RFC6243].

----------------------------------------------------------------------
Indentation of HTTP examples

1.5.  RESTCONF Extensibility
      GET /.well-known/host-meta HTTP/1.1
      HTTP/1.1 200 OK
3.1.  Root Resource Discovery
      GET /.well-known/host-meta HTTP/1.1
      HTTP/1.1 200 OK
      GET /.well-known/host-meta HTTP/1.1
      HTTP/1.1 200 OK
      GET /top/restconf/operations  HTTP/1.1
      HTTP/1.1 200 OK
3.3.1.  {+restconf}/data
   GET /restconf/data/example-jukebox:jukebox/library
   HTTP/1.1 200 OK
3.3.3.  {+restconf}/yang-library-version
   GET /restconf/yang-library-version  HTTP/1.1
   HTTP/1.1 200 OK
3.6.  Operation Resource
   POST /restconf/operations/module-A:reset HTTP/1.1
   POST /restconf/data/module-A:interfaces/reset-all HTTP/1.1
3.6.1.  Encoding Operation Resource Input Parameters
   POST /restconf/operations/example-ops:reboot HTTP/1.1
   HTTP/1.1 204 No Content
      POST /restconf/operations/example-ops:reboot HTTP/1.1
   POST /restconf/data/example-actions:interfaces/interface=eth0
   HTTP/1.1 204 No Content
      POST /restconf/data/example-actions:interfaces/interface=eth0
3.6.2.  Encoding Operation Resource Output Parameters
   POST /restconf/operations/example-ops:get-reboot-info HTTP/1.1
      HTTP/1.1 200 OK
   HTTP/1.1 200 OK
   POST /restconf/data/example-actions:interfaces/interface=eth0
      HTTP/1.1 200 OK
3.6.3.  Encoding Operation Resource Errors
   POST /restconf/operations/example-ops:reboot HTTP/1.1
   HTTP/1.1 400 Bad Request
      HTTP/1.1 400 Bad Request
3.7.  Schema Resource
   GET /restconf/data/ietf-yang-library:modules-state/module=
      HTTP/1.1 200 OK
      HTTP/1.1
      HTTP/1.1 200 OK
4.3.  GET
   GET /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 200 OK
4.4.1.  Create Resource Mode
      POST /restconf/data HTTP/1.1
   HTTP/1.1 201 Created
4.4.2.  Invoke Operation Mode
      POST /restconf/operations/example-jukebox:play   HTTP/1.1
   HTTP/1.1 204 No Content
4.5.  PUT
      PUT /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 204 No Content
   PUT /restconf/data/example-jukebox:jukebox/
4.6.1.  Plain Patch
   PATCH /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 204 No Content
4.7.  DELETE
   DELETE /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 204 No Content
5.3.1.  XML MetaData Encoding Example
   GET /restconf/data/interfaces/interface=eth1
   HTTP/1.1 200 OK
5.3.2.  JSON MetaData Encoding Example
   GET /restconf/data/interfaces/interface=eth1
      HTTP/1.1 200 OK
6.2.  Event Streams
   GET /restconf/data/ietf-restconf-monitoring:restconf-state/
   HTTP/1.1 200 OK
6.3.  Subscribing to Receive Notifications
   GET /restconf/data/ietf-restconf-monitoring:restconf-state/
   HTTP/1.1 200 OK
   GET /streams/NETCONF HTTP/1.1
   GET /streams/NETCONF HTTP/1.1
7.1.  Error Response Message
   DELETE /restconf/data/example-jukebox:jukebox/
      HTTP/1.1 409 Conflict
   POST /restconf/data/example-jukebox:jukebox HTTP/1.1
   HTTP/1.1 409 Conflict
8.  RESTCONF module
                 POST /restconf/operations/ietf-system:system-restart
                 GET /restconf/operations
D.1.1.  Retrieve the Top-level API Resource
   GET /.well-known/host-meta HTTP/1.1
   HTTP/1.1 200 OK
   GET /restconf   HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf HTTP/1.1
   HTTP/1.1 200 OK
D.1.2.  Retrieve The Server Module Information
   GET /restconf/data/ietf-yang-library:modules-state HTTP/1.1
      HTTP/1.1 200 OK
D.1.3.  Retrieve The Server Capability Information
   GET /restconf/data/ietf-restconf-monitoring:restconf-state/
   HTTP/1.1 200 OK
D.2.1.  Create New Data Resources
      POST /restconf/data/example-jukebox:jukebox/library HTTP/1.1
   HTTP/1.1 201 Created
   POST /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 201 Created
D.2.2.  Detect Resource Entity Tag Change
      PATCH /restconf/data/example-jukebox:jukebox/
          HTTP/1.1
   HTTP/1.1 412 Precondition Failed
D.2.3.  Edit a Datastore Resource
   PATCH /restconf/data HTTP/1.1
D.2.4.  Edit a Data Resource
   PATCH /restconf/data/example-jukebox:jukebox/library/
D.3.1.  "content" Parameter
   GET /restconf/data/example-events:events?content=all
       HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf/data/example-events:events?content=config
       HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf/data/example-events:events?content=nonconfig
       HTTP/1.1
      HTTP/1.1 200 OK
D.3.2.  "depth" Parameter
   GET /restconf/data/example-jukebox:jukebox?depth=unbounded
       HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf/data/example-jukebox:jukebox?depth=1 HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf/data/example-jukebox:jukebox?depth=3 HTTP/1.1
      HTTP/1.1 200 OK
D.3.3.  "fields" Parameter
   GET /restconf/data?fields=ietf-yang-library:modules-state/
      HTTP/1.1 200 OK
D.3.4.  "insert" Parameter
      POST /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 201 Created
D.3.5.  "point" Parameter
      POST /restconf/data/example-jukebox:jukebox/
   HTTP/1.1 204 No Content
D.3.6.  "filter" Parameter
      GET /streams/NETCONF?filter=%2Fevent%2Fevent-class%3D'fault'
      GET /streams/NETCONF?filter=%2Fevent%2Fseverity%3C%3D4
      GET /streams/SNMP?filter=%2FlinkUp%7C%2FlinkDown
      GET /streams/NETCONF?
      GET /streams/critical-syslog?
      GET /streams/NETCONF?
      GET /streams/NETCONF?filter=(%2Fm1%3A*%20or%20%2Fm2%3A*)
D.3.7.  "start-time" Parameter
   GET /streams/NETCONF?start-time=2014-10-25T10%3A02%3A00Z
D.3.8.  "stop-time" Parameter
   GET /mystreams/NETCONF?stop-time=2014-10-25T12%3A31%3A00Z
D.3.9.  "with-defaults" Parameter
   GET /restconf/data/example:interfaces/interface=eth1 HTTP/1.1
      HTTP/1.1 200 OK
   GET /restconf/data/example:interfaces/interface=eth1
      HTTP/1.1 200 OK
----------------------------------------------------------------------

Dale