Subject: Re: [Netconf] AD review of draft-ietf-netconf-restconf-13

From: Benoit Claise <[email-masked]>
Date: Wed, 29 Jun 2016 15:13:47 +0200

Dear authors,

Thanks for this new draft. I removed the points that are integrated.
Note: I wished that the authors had replied with the points (not) 
addressed, instead of me spending a couple of hours reviewing all 
comments and diffs.

First of, there is an issue with the YANG module extraction

    xym.py draft-ietf-netconf-restconf-14.txt
    ERROR: 'draft-ietf-netconf-restconf-14.txt', Line 3930 - 'module'
    statement within another module
    Created the following models::
        example-ops.yang
        example-actions.yang
        example-jukebox.yang
        example-mod.yang

Not an issue with the draft, but with the xym.py that can't deal with a 
module example in a description:

    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.
                   For example, the 'system-restart' RPC operation defined
                   in the 'ietf-system' module would be represented as
                   an empty leaf in the 'ietf-system' namespace. This is
                   a conceptual leaf, and will not actually be found in
                   the module:

                      module ietf-system {                        <=====
                        leaf system-reset {
                          type empty;
                        }
                      }

Bug filed for xym.py
Testing https://github.com/donaldh/yang-extractor (which I learned of 
today), it works fine on this draft.
Currently testing the other drafts.

See in-line.
> 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?
I need to follow up with the I2RS chairs and AD on this one.
>
> - 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.
>
You have to say something about HTTP2
>
> - 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
>
The idea is to refer to the definitions in RFC6020bis, now that they 
have been introduced:

container: An interior data node that exists in at most one
       instance in the data tree.  A container has no value, but rather a
       set of child nodes.

    o  non-presence container: A container that has no meaning of its
       own, existing only to contain child nodes.

    o  presence container: A container where the presence of the
       container itself carries some meaning.

       own, existing only to contain child nodes.

>
> - "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 <https://tools.ietf.org/html/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.
This one has been addressed?
>
>
> Same remark for entity-tag and section 3.4.1.2
And this one?
>
> - 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.

    If the "rpc" or "action" statement has an "output" 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 "output".

Input or output?

>
> - 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?
And the answer is?
>
>
>
>   - 5.3.2 Json MetaData Encoding Example
>     Note thatRFC 6243 <https://tools.ietf.org/html/rfc6243>  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_"
And I still don't understand. Care to reply?
>    
>
> -
> 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?
care to reply?
>
> - Security Considerations.
> Please includehttps://trac.tools.ietf.org/area/ops/trac/wiki/yang-security-guidelines
>
>
>
>
> Regards, Benoit (OPS AD)
>
>        
>
>
> _______________________________________________
> Netconf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netconf

---

From: Benoit Claise <[email-masked]>
Date: Wed, 29 Jun 2016 15:38:49 +0200

Btw,

list access {
              key encoding;
              min-elements 1;
              description
                "The server will create an entry in this list for each
                 encoding format that is supported for this stream.
                 The media type 'application/yang-stream' is expected
                 for all event streams. This list identifies the
                 sub-types supported for this stream.";

Should this media type be registered?

Regards, B.
> Not an issue with the draft, but with the xym.py that can't deal with 
> a module example in a description:
>
>     container operations {
>                 description

---

From: Andy Bierman <[email-masked]>
Date: Thu, 30 Jun 2016 21:13:23 -0700

On Wed, Jun 29, 2016 at 6:13 AM, Benoit Claise <[email-masked]> wrote:

> addressed, instead of me spending a couple of hours reviewing all comments
> and diffs.
>
I did not get any YANG errors in idnits.
It is true that we validate the example modules without the --ietf option.
IMO, this MUST only be for modules enclosed in <CODE BEGINS> <CODE ENDS>

We never resolved the issue of <EXAMPLE-BEGINS> <EXAMPLE-ENDS>
There was no consensus that it was needed or how to do it.
So let's decide now.

> - https://github.com/netconf-wg/restconf/issues?q=is%3Aopen+is%3Aissue

OK

> background: https://www.ietf.org/mail-archive/web/netconf/current/msg08578.html
> I see in section 2.1

The WG needs to decide what to say, and what section needs to be updated.
There is a lot of text that cites HTTP 1.1.

I would prefer a small number of edits; MUST support HTTP 1.1; MAY support
HTTP/2

> The idea is to refer to the definitions in RFC6020bis, now that they have
> been introduced:
>

I removed terms that were not actually used in draft-13

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

You are right.  It needs more work.
I want to make sure we can support datastore-specific timestamps and entity
tags
for ephemeral and other datastores.

    RT                                   Last-Modified              ETag

    API                                       none                      none

    Operation                              none                    none

    Datastore                               SHOULD            MUST

    Data                                        MAY                   SHOULD

    restconf-state/capabilities      MAY                    SHOULD

(Examples show Last-Modified returned for API)

Section 3.4.1.1
>    the resource has been modified since the specified timestamp.
edit detection

>    header when it is retrieved with the GET or HEAD methods.
data resource

>    is retrieved with the GET or HEAD methods.

/restconf-state/capabilities  (this is MAY/SHOULD in draft-14)

>    is retrieved with the GET or HEAD methods.

this was removed in draft-14

>    when this data node is retrieved with the GET or HEAD methods.

this was removed in draft-14

> and potentially some more sections

I checked but cannot find more

> This one has been addressed?

done (in -15)

> Same remark for entity-tag and section 3.4.1.2

done  (in -15)

>    request message MUST NOT include a message-body.

this edit is done except exact replacement text not used.
Will be modified in -15 related to Dale's comments.

>    instances of these *input *parameters are encoded in the module

fixed cut-and-paste 'input' (in -15)

> So we would have to update this RESTCONF RFC-to-be with the pub/sub publication?

I certainly do not agree that RFC 5277 is going to be obsolete.
Adding new features should not make existing deployed functionality
obsolete.
IMO, the new work must update 5277, not replace it.

>    Note that RFC 6243 <https://tools.ietf.org/html/rfc6243> defines the "default" attribute with XSD, not
>    YANG, so *the YANG module name has to be assigned manually*.  The value
> I don't understand "*the YANG module name has to be assigned manually*"
>

fixed (in -15)
assigned instead of derived from a YANG module name.

> pyang --ietf example-jukebox.yang

I disagree that --ietf should be added to the validation of non-normative
modules.

> Please include https://trac.tools.ietf.org/area/ops/trac/wiki/yang-security-guidelines
>
>
this was done in -14

> Regards, Benoit (OPS AD)

Andy

---

From: Andy Bierman <[email-masked]>
Date: Thu, 30 Jun 2016 21:21:43 -0700

> Should this media type be registered?

This is a typo.  It should be text/event-stream. (fixed ni -15)

The references to the media type application/yang-stream+xml
and application/yang-stream+json in the "encoding" leaf will be removed in
-15.

Andy

> Netconf mailing [email-masked]://www.ietf.org/mailman/listinfo/netconf
>
>
>
>
> Netconf mailing [email-masked]://www.ietf.org/mailman/listinfo/netconf
>
>
>