AD review of restconf-13

abierman commented 8 years ago

Dear all,

Here is my draft-ietf-netconf-restconf-13 AD review [sorry for the delay, it took longer than expected]. If some of the points have already been discussed, let me know.

https://github.com/netconf-wg/restconf/issues?q=is%3Aopen+is%3Aissue There are still open issues. I would have expected that all issues are closed. Should I be worried?
From the charter: The RESTCONF work will consider requirements suggested by the other working groups (for example I2RS).

Are we good in terms of I2RS requirements for RESTCONF, if any?

section 1 RESTCONF is based on HTTP1.1 [RFC7230] What about HTTP2 [RFC7540]? I've seen some discussions on the NETCONF mailing but I'm unclear if RESTCONF would work with HTTP2. A few words are necessary IMO. background: https://www.ietf.org/mail-archive/web/netconf/current/msg08578.html I see in section 2.1 No other versions of HTTP are supported for use with RESTCONF. Do you mean: No other versions than HTTP 1.1 are supported for use with RESTCONF. So: HTTP2.0 MUST NOT be used with RESTCONF. If this is the case, some sort of justification is required.
section 1, editorial: OLD: The YANG language defines the syntax and semantics of datastore content, state data, protocol operations, and event notifications.

NEW: The YANG language defines the syntax and semantics of datastore content, configuration, state data, protocol operations, and event notifications.

Justification, to be more in line with RFC6020bis, which says: A YANG module defines a hierarchy of data that can be used for NETCONF-based operations, including configuration, state data, Remote Procedure Calls (RPCs), and notifications.

section 1 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. The server MAY provide access to specific datastores using operation resources, as described in Section 3.6.

Terminology: RESTCONF server, NETCONF server, server. "Server" according to the terminology section is the RFC6241 definition, so the NETCONF server. I don't think that's right in this sentence. You should review all "server" instances in the doc, and/or potentially change the terminology. Maybe: Server is not specific, so NETCONF or RESTCONF server NETCONF server RESTCONF server

Same issue with "client".

section 1.1.3 non-presence container (or NP-container) presence container (or P-container)

As Lionel Morand mentioned in his OPS-DIR draft-ietf-netmod-rfc6020bis-11 review: LM: "non-presence container" is not defined anywhere in the document. I can assume that it refers to a container that does not have a "presence" statement. If it is, it could be good to: define the term in the section 3 and to extend the existing text in the section 7.5.5

section 1.1.4: Sometimes the terms in that section are difficult to grasp without context, as some definitions don't explain what this term is for. For example: o operation resource: a resource with the media type "application/ yang.operation+xml" or "application/yang.operation+json".

Except from the name itself, it doesn't tell me that this resource is linked to the "operations", meaning the RPC operations in YANG.

For example: OLD: o schema resource: a resource with the media type "application/ yang". The YANG representation of the schema can be retrieved by the client with the GET method. NEW: o schema resource: a resource with the media type "application/ yang". The schema resource is used to retrieve the YANG representation of the schema by the client with the GET method.

Section 1.1.5 OLD: URI Template NEW: URI Template and Examples

I would add that the examples throughout the document are based on a jukebox YANG data model, and that, even if the examples should be self contained, the full jukebox YANG data model is available in the appendix C

Just one of the many justifications. See section 4.8.3 where this example comes out of the blue For example, assume the target resource is the "album" list. To retrieve only the "label" and "catalogue-number" of the "admin" container within an album, use: "fields=admin(label;catalogue-number)".

Section 1.2.

From the abstract: This document describes an HTTP-based protocol that provides a programmatic interface for accessing data defined in YANG, using the datastores defined in NETCONF.

From section 1: 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 datastores defined in NETCONF [RFC6241]. ...

The following figure shows the system components if a RESTCONF server is co-located with a NETCONF server:

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

The following figure shows the system components if a RESTCONF server is implemented in a device that does not have a NETCONF server:

  +-----------+           +-----------------+
  |  Web app  | <-------> |                 |
  +-----------+   HTTP    | network device  |
                          |                 |
                          +-----------------+

So we don't use the datastores defined in NETCONF, but datastore concepts. See next point.

Section 1.2: RESTCONF does not need to mirror the full functionality of the NETCONF protocol, but it does need to be compatible with NETCONF. RESTCONF achieves this by implementing a subset of the interaction capabilities provided by the NETCONF protocol, for instance, by eliminating datastores and explicit locking.

The abstract says: This document describes an HTTP-based protocol that provides a programmatic interface for accessing data defined in YANG, using the datastores defined in NETCONF.

So on one side, we eliminated the datastore and on the other side, we use them. And the first figure in section 1.2 doesn't help. In case of HTTP and NETCONF access to the network device, does HTTP use the datastore? I guess you want to introduce the conceptual datastore concept. Maybe: This document describes an HTTP-based protocol that provides a programmatic interface for accessing data defined in YANG, using the datastore concepts defined in NETCONF.

Introduction needs to be updated as well.

section 1.4, editorial OLD: If a datastore that would be modified by a RESTCONF operation has an active lock, the RESTCONF edit operation MUST fail with a "409 Conflict" status-line. NEW: 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.

We want to stress that there is no lock in RESTCONF

Section 1.5, editorial why "for example" in this sentence: For example, this document defines several query parameters in Section 4.8.
section 3 All RESTCONF resource types are defined in this document except specific datastore contents, protocol operations, and event notifications. The syntax and semantics for these resource types are defined in YANG modules.

What do you mean with "protocol operations"?

RPC and action: RESTCONF protocol operation (as defined in section 1.1.1) Section 3.3.2 is clearer with "data-model specific protocol operations supported by the server"
Operation resource as in S 3.6
Operation (GET, POST) like in S 4.0 And the definition speaks of o operation: the conceptual RESTCONF operation for a message, derived from the HTTP method, request URI, headers, and message- body. The document should be clear and consistent with all instances of "(protocol) operations"

Which operation do we speak about here? It could be understood as NETCONF protocol operations...

section 3.x

The API resource contains the entry points for the RESTCONF datastore and operation resources.

And I see in 3.3.1 that querying {+restconf}/data content requires application/yang.data+xml (or json), as opposed to .api+xml (or json) What about the querying {+restconf}/operations? does it require application/yang.api+xml or application/yang.data+xml ? https://tools.ietf.org/html/draft-ietf-netconf-restconf-13#section-3.3.2 should provide any example. My first guess was: since operations = API, I would have used application/yang.api+xml. Re-reading the draft, I now guess it's application/yang.data+xml (or json). The point is: what are the entry points for the operations resources?

Section 3.3.3, Editorial [RFC Editor Note: Adjust the date for ietf-yang-library below to the date in the published ietf-yang-library YANG module, and remove this note.]

Make it clear that the date to be changed is 2016-04-09, as opposed to only:

Mon, 23 Apr 2012

3.4. Datastore Resource The "{+restconf}/data" subtree represents the datastore resource type, which is a collection of configuration data and state data nodes. Should be "{+restconf}/datastore", right ?

"Last-Modified" This is one of those topics whose requirements are all over the place. This is confusing (at least to me)

Section 3.4.1.1 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.

Section 3.5 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.

Section 9.1 The server MUST maintain a last-modified timestamp for this container, and return the "Last-Modified" header when this data node is retrieved with the GET or HEAD methods.

Section 10.1 The server MUST maintain a last-modified timestamp for this container, and return the "Last-Modified" header when this data node is retrieved with the GET or HEAD methods.

Section 10.1.1 The server SHOULD maintain a last-modified timestamp for each instance of this list entry, and return the "Last-Modified" header when this data node is retrieved with the GET or HEAD methods.

and potentially some more sections At the minimum, we should have forward references from section 3.4.1.1 as it looks like self-contained.

Same remark for entity-tag and section 3.4.1.2

Section 3.5.1, editorial:

container top { list list1 { key "key1 key2 key3"; ... list list2 { key "key4 key5"; ... leaf X { type string; } } } leaf-list Y { type uint32; } }

For the above YANG definition, a target resource URI for leaf "X" would be encoded as follows (line wrapped for display purposes only):

/restconf/data/example-top:top/list1=key1,key2,key3/ list2=key4,key5/X

....

Mentions that "container top" is part of the example-top YANG data model

section 3.5.2.

If the target of a GET method is a data node that represents a leaf that has a default value, and the leaf has not been configured yet, the server MUST return the default value that is in use by the server.

I guess that it depends on the default handing mode. What about trim? ref: https://tools.ietf.org/html/rfc6243#section-3.2

Section 3.6 OLD: If the "rpc" or "action" statement has an "input" section, then a message-body MAY be sent by the client in the request, otherwise the request message MUST NOT include a message-body. If the "input" objcet tree contains mandatory parameters, then a message-body MUST be sent by the client.

NEW:

If the "rpc" or "action" statement has an "input" section and the "input" object tree contains mandatory parameters, 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 doesn't contain mandatory parameters, then a message-body MAY be sent by the client in the request.

If the "rpc" or "action" statement has no "input" section, the request message MUST NOT include a message-body.

section 3.6 If the operation is invoked without errors, and if the "rpc" or "action" statement has an "output" section, then a message-body MAY be sent by the server in the response, otherwise the response message MUST NOT include a message-body in the response message, and MUST send a "204 No Content" status-line instead.

First, I don't understand the first MAY. I would have thought it would be a MUST. Second, "MUST NOT include a message-body in the response message, and MUST send a "204 No Content" status-line instead." So the errors are not part of the message-body? Section 7.1 says: then the server SHOULD send a response message-body containing the information described by the "errors" container definition within the YANG module Section 8 And third, message body or message-body. The document uses both.

RFC5277 is a normative reference. I guess that pub/sub will obsolete RFC5277. So we would have to update this RESTCONF RFC-to-be with the pub/sub publication?
Section 4, editorial OLD: The NETCONF "remove" operation attribute is not supported by the HTTP DELETE method. NEW The NETCONF "remove" operation attribute is not supported by the HTTP DELETE method.
Section 4.4.1, Editorial: OLD: The "insert" and "point" query NEW: The "insert" (Section 4.8.5) and "point" (Section 4.8.6) query
Section 4.8 RESTCONF Query Parameters, HEAD is missing. Section 4.2: The same query parameters supported by the GET method are supported by the HEAD method.
Section 4.8 I see: o Each parameter can appear at most once in a request URI. In section 4.8.2, I see: More than one "depth" query parameter MUST NOT appear in a request. If more than one instance is present, then a "400 Bad Request" status-line MUST be returned by the server. In section 4.8.3, I see: More than one "fields" query parameter MUST NOT appear in a request. If more than one instance is present, then a "400 Bad Request" status-line MUST be returned by the server. etc.

You could include the generic specifications, along with the RFC2119 language, in section 4.8, as opposed to every 4.8.x section.

section 4.8.4 The format of this parameter is an XPath 1.0 expression, and is evaluated in the following context: o The set of namespace declarations is the set of prefix and namespace pairs for all supported YANG modules, where the prefix is the YANG module name, and the namespace is as defined by the "namespace" statement in the YANG module.

o The function library is the core function library defined in XPath 1.0.

RFC 6241 says: o The function library is the core function library, plus any functions defined by the data model.

Should this be aligned?

Section 4.6.1 The plain patch mechanism merges the contents of the message body with the target resource. If the target resource is a datastore resource (see Section 3.4), the message body MUST be either application/yang.datastore+xml or application/yang.datastore+json. If then the target resource is a data resource (see Section 3.5), then the message body MUST be either application/yang.data+xml or application/yang.data+json.

Something I completely missed (and only understood when I spoke to Martin). To edit a resource, we have two ways:

PATCH on the datastore resource like in appendix D.2.3
PATCH on the data resource, with the full path. To map the example in D.2.3, something like PATCH /restconf/data HTTP/1.1 Host: example.com Content-Type: application/yang.data+xml "http://x.x.x.x:yyyy/restconf/data/jukebox:library" Both examples should be illustrated next to each others, and the text in 4.6 or 4.6.1 clear.
- Section 4.8.7 What is this "notification replay feature"? I searched for "notification replay" in the doc, without luck. I finally found it, in https://tools.ietf.org/html/rfc5277#section-3.3, from the reference link: 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."; } You should really add the references in section 4.8.7 and 8, to RFC 5277, Section 3.4 and to the replay-support
- Section 5.1, editorial //?#
```
^       ^        ^       ^         ^
|       |        |       |         |
```
method entry resource query fragment
```
M       M        O        O         I
```

The method points to a space. Indent the line starting with

section 5.2 Content is encoded in either JSON or XML format. There are no capabilities to discover this, right? So the only solution is for a client to test it, right? How does a client know this? Trying is the only solution? Do we want to explain this, and show an example, for example with the HEAD method? Btw, I hope that we request to support JSON or XML is for the entire series of resource, right? Do we want to make it clear? +-----------+---------------------------------+ | Resource | Media Type | +-----------+---------------------------------+ | API | application/yang.api+xml | | | application/yang.api+json | | Datastore | application/yang.datastore+xml | | | application/yang.datastore+json | | Data | application/yang.data+xml | | | application/yang.data+json | | [none] | application/yang.errors+xml | | | application/yang.errors+json | | Operation | application/yang.operation+xml | | | application/yang.operation+json | | Schema | application/yang | +-----------+---------------------------------+
- 5.3.2 Json MetaData Encoding Example Note that RFC 6243 defines the "default" attribute with XSD, not YANG, so the YANG module name has to be assigned manually. The value "ietf-netconf-with-defaults" is assigned for JSON meta-data encoding.

I don't understand "the YANG module name has to be assigned manually"

Also, In section 5.3:

The RESTCONF protocol needs to retrieve the same meta-data that is used in the NETCONF protocol. Information about default leafs, last- modified timestamps, etc. are commonly used to annotate representations of the datastore contents. This meta-data is not defined in the YANG schema because it applies to the datastore, and is common across all data nodes.

=> I don't understand: This meta-data is not defined in the YANG schema because it applies to the datastore, and is common across all data nodes. This meta-data relates to default leafs and last-modified timestamps. The last-modified timestamps apply to more than just the datastore.

Also: This information is encoded as attributes in XML. JSON encoding of meta-data is defined in [I-D.ietf-netmod-yang-metadata].

You want to express something like this: With the XML encoding, the metadata is encoded as attributes in XML With the JSON encoding, the metadata is encoded as specified in [I-D.ietf-netmod-yang-metadata].

btw, use a single convention throughout the doc: meta-data or metadata draft-ietf-netmod-yang-metadata uses metadata

5.4

Each message represents some sort of resource access. An HTTP "status-line" header line is returned for each request. If a 4xx or 5xx range status code is returned in the status-line, then the error information will be returned in the response, according to the format defined in Section 7.1.

Why only 4xx and 5xx?

6.3, editorial OLD: A RESTCONF client MAY request the server compress the events using the HTTP header field "Accept-Encoding". For instance: NEW: A RESTCONF client MAY request that the server compress the events using the HTTP header field "Accept-Encoding". For instance:-

Section 8 container operations { description "Container for all operation resources (application/yang.operation),

          Each resource is represented as an empty leaf with the
          name of the RPC operation from the YANG rpc statement.

          E.g.;

             POST /restconf/operations/show-log-errors

             leaf show-log-errors {
               type empty;
             }
         ";
     }

I've been confused by this example, as at first glance I didn't consider a show-something as an operation. You might be thinking of a more obvious example: Reset-xxx is one

Section 13. Acknowledgment and "contributions" term. acknowledged persons for their contributions: do we speak about contributors, which has got a special meaning in terms of IPR? If not, let's avoid the "contributions" term
Good job with the appendix C and D. The examples are very useful.
Appendix D.1, editorial

The client may start by retrieving the top-level API resource, using the entry point URI "{+restconf}".

If this is a typical operational example, I guess you want to start by retrieving: GET /.well-known/host-meta HTTP/1.1

Appendix D.1.2 An example of deviations would be welcome (even if this is ietf-yang-library, which is a different dra
D.3.3 I guess this request should not report the module-set-id?

GET /restconf/data?fields=ietf-yang-library:modules-state/ module(name;revision) HTTP/1.1

...

{ "ietf-yang-library:modules-state": { "module-set-id": "cb4c422111a779e1eed55bffc8d6b46a3a0999e2", "module": [
Since we have many examples around around example-jukebox, this should pass compilation, right?

pyang --ietf example-jukebox.yang example-jukebox.yang:1: error: expected keyword "namespace" as child to "module" example-jukebox.yang:1: error: expected keyword "prefix" as child to "module" example-jukebox.yang:1: warning: RFC 6087: 4.1: the module name should start with one of the strings "ietf-" or "iana-" example-jukebox.yang:1: error: RFC 6087: 4.7: statement "module" must have a "contact" substatement example-jukebox.yang:1: error: RFC 6087: 4.7: statement "module" must have a "organization" substatement example-jukebox.yang:1: error: RFC 6087: 4.7: statement "module" must have a "description" substatement example-jukebox.yang:1: error: RFC 6087: 4.7: statement "module" must have a "revision" substatement

What was the agreed convention for example modules that should pass compilation?

We advice YANG data models to include a YANG tree diagram, like you did for "ietf-restconf-monitoring" The RESTCONF module doesn't. Obviously, pyang -f tree doesn't produce anything, but make it clear with some text, so that the readers don't believe it's an omission.
Security Considerations. Please include https://trac.tools.ietf.org/area/ops/trac/wiki/yang-security-guidelines
idnit complains about a few things.
[] in json encoding of list. For example, PUT /restconf/data/example-jukebox:jukebox/ library/artist=Foo%20Fighters/album=Wasting%20Light HTTP/1.1 Host: example.com Content-Type: application/yang.data+json

{ "example-jukebox:album" : { "name" : "Wasting Light", "genre" : "example-jukebox:alternative", "year" : 2011 } }

Don't we need [] for the album list?

       list album {
          key name;

          description
            "Represents one album resource within one
             artist resource, within the jukebox library.";

          leaf name {
            type string {
              length "1 .. max";
            }
            description "The name of the album.";
          }

          leaf genre {
            type identityref { base genre; }
            description
              "The genre identifying the type of music on
               the album.";
          }

          leaf year {
            type uint16 {
              range "1900 .. max";
            }
            description "The year the album was released";
          }

See https://tools.ietf.org/html/draft-ietf-netmod-yang-json-10#section-5.4 list bar { key foo; leaf foo { type uint8; } leaf baz { type string; } }

the following is a valid JSON-encoded instance:

"bar": [ { "foo": 123, "baz": "zig" }, { "baz": "zag", "foo": 0 } ]

Regards, Benoit (OPS AD)

abierman commented 8 years ago

Are we good in terms of I2RS requirements for RESTCONF, if any?

A: There are strong objections to adding datastores to RESTCONF. All datastore support will be through operations in the /restconf/operations/ subtree

abierman commented 8 years ago

3.4. Datastore Resource The "{+restconf}/data" subtree represents the datastore resource type, which is a collection of configuration data and state data nodes. Should be "{+restconf}/datastore", right ?

No -- this was "datastore" in YANG-API but shortened to "data" in RESTCONF

abierman commented 8 years ago

RFC5277 is a normative reference. I guess that pub/sub will obsolete RFC5277. So we would have to update this RESTCONF RFC-to-be with the pub/sub publication?

No -- IMO SSE and RFC5277 functionality works fine. Additional functionality is a separate feature

abierman commented 8 years ago

Issue

  pyang --ietf example-jukebox.yang 
  example-jukebox.yang:1: error: expected keyword "namespace" as child to "module"
   .... errors and warnings

When I run pyang:

     pyang --ietf example-jukebox.yang 
     example-jukebox.yang:1: warning: RFC 6087: 4.1: the module name should start 
     with one of the strings "ietf-" or "iana-"
     example-jukebox.yang:3: warning: RFC 6087: 4.8: namespace value should be 
    "urn:ietf:params:xml:ns:yang:example-jukebox"

These are expected, since these IETF warnings are wrong for examples

abierman commented 8 years ago

change term 'server' to 'NETCONF server'
add term 'RESTCONF server' also called 'server'
change term 'client' to 'NETCONF client'
add term 'RESTCONF client' also called 'client'
remove unused YANG terms
clarified operation resource and schema resource terms
clarified abstract and intro: RESTCONF uses NETCONF datastore concepts
removed term 'protocol operation'; use 'RPC operation' instead
clarified edit operation from NETCONF as nc:operation
clarified retrieval of an operation resource
remove ETag and Last-Modified requirements for /modules-state and /modules-state/module objects, since these are not configuration data nodes
clarified Last-Modified and ETag requirements for datastore and data resources
clarified defaults retrieval for leaf and leaf-list target resources
clarified request message-body for operation resources
clarified query parameters for GET also allowed for HEAD
clarified error handling for query parameters
clarified XPath function library for "filter" parameter
added example for 'edit a data resource'
added term 'notification replay' from RFC 5277
clarified unsupported encoding format error handling
change term 'meta-data' to 'metadata'
clarified RESTCONF metadata definition
clarified error info not returned for 1xx, 2xx, and 3xx ranges
clarified operations description in ietf-restconf module
clarified Acknowledgements section
clarified some examples
update some references
update RFC 2119 boilerplace

abierman commented 8 years ago

Issues not addressed

I2RS requirements for RESTCONF

Proposal: RESTCONF defines how it can be extended. It should not contain any I2RS details
What about HTTP/2?

Proposal: Add text that says server MAY support HTTP/2

abierman commented 8 years ago

not sure we need to say anything about HTTP/2

abierman commented 8 years ago

closed in draft 15

netconf-wg / restconf

AD review of restconf-13 #62

Mon, 23 Apr 2012