Re: [Netconf] IETF Last Call Gen-ART review of draft-ietf-netconf-restconf-15

[Andy Bierman <[e-mail]>]

Hi,

This will get added to the github issue list.

I am on vacation next week so edits will not be done for at least 2 weeks.


Andy


On Sun, Jul 31, 2016 at 5:19 PM, Dale R. Worley <[e-mail]> wrote:

> 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
> "[e-mail]".
>
> 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">
>      <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
>
> _______________________________________________
> Netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf
>