Early Review of draft-ietf-netconf-restconf-13
review-ietf-netconf-restconf-13-opsdir-early-morand-2016-07-25-00

I have reviewed this document as part of the Operational directorate's ongoing
effort to review all IETF documents being processed by the IESG.  These
comments were written with the intent of improving the operational aspects of
the IETF drafts. Comments that are not addressed in last call may be included
in AD reviews during the IESG review.  Document editors and WG chairs should
treat these comments just like any other last call comments.

Document: draft-ietf-netconf-restconf-15

Summary:   This document describes an HTTP-based protocol that provides a
programmatic interface for accessing data defined in YANG (v1 or v1.1), using
the datastore concepts defined in NETCONF. Status: Ready with
issues/clarification

I'm (still) not (so) familiar with YANG and NETCONF and I've read the document
as the newbie that I am. Therefore some of my comments may be irrelevant if out
of context.

Summary:

The document is well written, the last version capturing most of the comments
received so far. The rational for this solution and the use of the protocol is
clearly explained. And all the protocol aspects are clear enough for
implementors. Comments: *Major comment: this solution defines an HTTP-based
solution but with HTTP/1.1 in mind. It is said that HTTP/2 can be used but
there is very few details on this possibility. More details are required if
HTTP/2-based implementations are really foreseen . *Major comment: the possible
colocation of RESTCONF and NETCONF server could be improved. There are a lot of
implied assumptions regarding the interactions between both servers that are
not so trivial IMHO. *Minor comment: Numerous cross-references between
RESTCONF, NETCONF, YANG and HTTP may make the life difficult to the reader.

Detailed review provided below.

Regards,

Lionel

*************************
1.  Introduction

   This document defines an HTTP [RFC7230] based protocol called
   RESTCONF, for configuring data defined in YANG version 1 [RFC6020] or
   YANG version 1.1 [I-D.ietf-netmod-rfc6020bis], using the datastore
   concepts defined in NETCONF [RFC6241].

LM: What about HTTP/2 that "enables a more efficient use of network resources
and a reduced perception of latency by introducing header field compression and
allowing multiple concurrent exchanges on the same connection.  It also
introduces unsolicited push of representations from servers to clients."?

1.1.3.  YANG

   The following terms are defined in [I-D.ietf-netmod-rfc6020bis]:

LM: It is missing, lately introduced in rfc6020bis:

   o  non-presence container

   o  presence container

1.3.  Data Model Driven API

   The RESTCONF protocol operates on a conceptual datastore defined with
   the YANG data modeling language.  The server lists each YANG module
   it supports using the "ietf-yang-library" YANG module, defined in
   [I-D.ietf-netconf-yang-library].  The server MUST implement the
   "ietf-yang-library" module, which MUST identify all the YANG modules
   used by the server.

LM: Could be clarified where it is listed: "and listing all implemented modules
in the "/modules-state/module" list"

   The conceptual datastore contents, data-model-specific operations and
   event notifications are identified by this set of YANG modules.

LM: either this sentence is part of the previous paragraph or the sentence
should be:

   "The conceptual datastore contents, data-model-specific operations and
    event notifications are identified by the set of YANG modules listed
    in the "/modules-state/module" list."

   The classification of data as configuration or non-configuration is
   derived from the YANG "config" statement.  Data ordering behavior is
   derived from the YANG "ordered-by" statement.

LM: "non-configuration data" is "state data" as per the definition of the
"config" statement:

   "When a node is tagged with "config false",
   its subhierarchy is flagged as state data.  If it is tagged with
   "config true", its subhierarchy is flagged as configuration data."

1.4.  Coexistence with NETCONF

   RESTCONF can be implemented on a device that supports NETCONF.

LM: is it "RESTCONF server + NETCONF server" or any combination "client/server"?
LM: "colocated" is only relevant if there is protocol interaction between
RESTCONF and NETCONF. And this interaction is described     in this section.
LM: when both servers are colocated, is it assummed that the RESTCONF server
acts as a NETCONF client and "normal" NETCONF operations are performed between
both?

   If the device supports :writable-running, all edits to configuration
   nodes in {+restconf}/data are performed in the running configuration
   datastore.  The URI template "{+restconf}" is defined in
   Section 1.1.6.

LM: Just for sake of clarity, "If the device supports" refers to the "if the
NETCONF server colocated with the RESTCONF server supports", right? Applies to
the rest of the section.

   Otherwise, if the device supports :candidate, all edits to
   configuration nodes in {+restconf}/data are performed in the
   candidate configuration datastore.  The candidate MUST be
   automatically committed to running immediately after each successful
   edit.

LM: "automatically" could be understood as "implicitly", that is not possible
with NETCONF. If I'm correct, what is meant is that the edit operations in the
candidate configuration datastore MUST be followed by a commit operation.

   Any edits from other sources that are in the candidate
   datastore will also be committed.  If a confirmed-commit procedure is
   in progress, then this commit will act as the confirming commit.

LM: s/If a confirmed-commit procedure is in progress/If a confirmed commit
procedure is in progress, LM: "in progress" means by another client?

   If
   the server is expecting a "persist-id" parameter to complete the
   confirmed commit procedure then the RESTCONF edit operation MUST fail
   with a "409 Conflict" status-line.

LM: s/If the server/If the NETCONF server
LM: this point is not clear for me. I understand from NETCONF that if a
Persist-Id is not expected, the confirming commit must be issued on the same
session that issued the confirmed commit. So even if the Persist-Id is not
expected, the operaion will still fail. But I have certainly missed something.
LM: Would it be useful to give the associated error-tag to use in this case?

   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.

LM: "automatically" here means "edit operation followed by a <copy-config>
operation from the <running> to the <startup>"?

   If a datastore that would be modified by a RESTCONF operation has an
   active lock from a NETCONF client, the RESTCONF edit operation MUST
   fail with a "409 Conflict" status-line.

LM: Would it be useful to give the associated error-tag to use in this case?

2.  Transport Protocol Requirements

2.1.  Integrity and Confidentiality

   HTTP/2 [RFC7540] MAY be used for RESTCONF.  The server MUST respond
   using a single HTTP/2 stream for all client requests from a stream.
   The server MAY respond using same HTTP/2 stream that was used for the
   corresponding request.

LM: the rest of the document focuses on HTTP/1.1. Is this section is enough to
know how to support RESTCONF over HTTP/2?

3.  Resources

   [...]

   A resource can be considered a collection of data and the set of
   allowed methods on that data.

LM: s/A resource can be considered a collection of data/A resource can be
considered as a collection of data

3.1.  Root Resource Discovery

   [...]

   If the XRD contains more than one link relation, then only the
   relation named "restconf" is relevant to this specification.

LM: please expand XRD (Extensible Reference Descriptor)

3.4.1.1.  Timestamp

   The last change time is maintained and the "Last-Modified"
   ([RFC7232], Section 2.2) header is returned in the response for a
   retrieval request.  The "If-Unmodified-Since" header can be used in
   edit operation requests to cause the server to reject the request if
   the resource has been modified since the specified timestamp.

LM: s/The "If-Unmodified-Since" header/The "If-Unmodified-Since" header
([RFC7232], Section 3.4)

3.5.1.  Timestamp

LM: the use of the timestamp regarding configuration and non-configuration data
is quite confusing. it said in the same section:

   For configuration data resources, the server MAY maintain a last-
   modified timestamp for the resource, and return the "Last-Modified"
   header when it is retrieved with the GET or HEAD methods.
   [...]
   This timestamp is only affected by configuration data resources, and
   MUST NOT be updated for changes to non-configuration data.

   For non-configuration data resources, the server MAY maintain a last-
   modified timestamp for the resource, and return the "Last-Modified"
   header when it is retrieved with the GET or HEAD methods.  The
   timestamps for non-configuration data resources are updated in an
   implementation-specific manner.

LM: what is the difference between the "two" last-modified timestamps?

3.5.2.  Entity tag

LM: same comment as above regarding the use of ETag for configuration and
non-configuration data.

3.5.3.  Encoding Data Resource Identifiers in the Request URI

   [...]

   A RESTCONF data resource identifier is not an XPath expression.

LM: what does it mean "is not"? Can be removed IMHO

3.5.3.1.  ABNF For Data Resource Identifiers

   [...]
   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].

LM: either it is a note and for information or it is a requirement. Cannot be
both. I guess that the note could be moved as body text with normative language.

3.5.4.  Defaults Handling

LM: should it be "Default Handling" or "Default handlings"? The name of the URI
is "defaults"

3.6.  Operation Resource

   [...]

   If the "rpc" or "action" statement has an "input" section then
   instances of these input parameters are encoded in the module
   namespace where the "rpc" or "action" statement is defined, in an XML
   element or JSON object named "input", which is in the module
   namespace where the "rpc" or "action" statement is defined.

   If the "rpc" or "action" statement has an "input" section and the
   "input" object tree contains any child data nodes which are
   considered mandatory nodes, then a message-body MUST be sent by the
   client in the request.

   [...]

LM: why not skip this text (and the following part) and describe the
input/output + message-body cases in the sections 3.6.1 and 3.6.2. This will
avoid some misalignements, especially regarding the mandatory presence of the
message-body. For instance, here it is said in section 3.6:

   If the "rpc" or "action" statement has an "input" section and the
   "input" object tree contains any child data nodes which are
   considered mandatory nodes, then a message-body MUST be sent by the
   client in the request.

   If the "rpc" or "action" statement has an "input" section and the
   "input" object tree does not contain any child nodes which are
   considered mandatory nodes, then a message-body MAY be sent by the
   client in the request.

LM: whereas in section 3.6.1:

   If the "rpc" or "action" statement has an "input" section, then the
   "input" node is provided in the message-body, corresponding to the
   YANG data definition statements within the "input" section.

LM: in which it is assumed that a message-body is always there when "input" is
present. LM: Moreover, in section 3.6, it is not said what is the payload body
carried in the message-body.

3.7.  Schema Resource

   [...]
   To retrieve a YANG module, a client first needs to get the URL for
   retrieving the schema, which is stored in the "schema" leaf.  Note
   that there is no required structure for this URL.  The URL value
   shown below is just an example.

LM: why do not recommend a default structure?

3.9.  Errors YANG Data Template

   [...]  It
   is not considered a resource type because no instances can be
   retrieved with a GET request.

LM: s/It is not considered a resource type/It is not considered as a resource
type

4.  Operations

   The RESTCONF protocol uses HTTP methods to identify the CRUD
   operation requested for a particular resource.

LM: CRUD operation/CRUD 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.

LM: as the table gives the mapping between RESCONF and NETCONF operations, it
could be explain why OPTIONS and HEAD as no equivalence in NETCONF, even if
obvious.

4.2.  HEAD

LM: it is not said if the server MUST implement this method (as for OPTIONS).
Same comment for all the other methods.

4.3. GET

LM: when relevant, the error-tag used with error indicated in the status-line
header would be useful to ensure a correct mapping of error cases with NETCONF
error cases. Comment valids for other methods where error cases are described.

4.5.  PUT

   The PUT method is sent by the client to create or replace the target
   data resource.  A request message-body MUST be present, representing
   the new data resource, or the server MUST return "400 Bad Request"
   status-line.

LM: for creation of data resource, are POST and PUT operations? If not, what is
the difference? If they are, is there any preference for creation of a data
resource?

LM: No specific comments on the rest of the document. Thank you.

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations
confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites
ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez
le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les
messages electroniques etant susceptibles d'alteration, Orange decline toute
responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged
information that may be protected by law; they should not be distributed, used
or copied without authorisation. If you have received this email in error,
please notify the sender and delete this message and its attachments. As emails
may be altered, Orange is not liable for messages that have been modified,
changed or falsified. Thank you.