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 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: //?# ^ ^ ^ ^ ^ | | | | | 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 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 " message"? Is 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 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 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 , 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 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 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 element is adapted for use in RESTCONF, and an data tree information is returned for "4xx" and "5xx" class of status codes. -- [...] a mapping between the NETCONF 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, 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, 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