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

From: "Eric Voit (evoit)" <[email-masked]>
Date: Fri, 1 Jul 2016 17:44:11 +0000

Andy Bierman, July 01, 2016 12:13 AM
On Wed, Jun 29, 2016 at 6:13 AM, Benoit Claise <[email-masked]<mailto:[email-masked]>> wrote:

- 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

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

<Eric>  HTTP/2 needs to be allowed, even if no text examples are provided.

Is there any concern with sequencing of transactions here?  Section 2.1 says “Responses MUST be sent in the same order that requests are received.”   Different POST, PUT, PATCH Server responses might end up on different HTTP/2 streams and still meet this requirement.  We should preclude this possibility in the text.

Is it feasible to add requirement that all requests made from one HTTP/2 stream must be returned within a single HTTP/2 stream?   This would enable pipelining for an initiating application.   It would also allow parallelism for RESTCONF Server initiated messages which can safely be multiplexed.

Eric

---

From: Benoit Claise <[email-masked]>
Date: Sat, 2 Jul 2016 10:31:50 +0200

Hi Andy,

>     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
>
>
> I did not get any YANG errors in idnits.
Investigating with Henrik.
> 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>
>
I don't complain about the examples.
The issue is https://github.com/xym-tool/xym/issues/5
> 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.
>
>
>
>
>
>     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.
>
>
> OK
>
>>     - section 1
>>     background:https://www.ietf.org/mail-archive/web/netconf/current/msg08578.html
>>     I see in section 2.1
>>     - 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.
>
>>
>
>
> I removed terms that were not actually used in draft-13
ok.
>
>
>
>>     - "Last-Modified"
>>     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 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.
>
>
> edit detection
>
>>     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.
>
>
> data resource
>
>>     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.
>
>
>
> /restconf-state/capabilities  (this is MAY/SHOULD in draft-14)
>
>>     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.
>
>
>
> this was removed in draft-14
>
>>     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.
>
>
>
> this was removed in draft-14
>
>>     and potentially some more sections
>
>
>
> I checked but cannot find more
>
>>     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?
>
> done (in -15)
>
>>     Same remark for entity-tag and section 3.4.1.2
>     And this one?
>
>
>
> done  (in -15)
>
>>     - 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.
>
>
>
> this edit is done except exact replacement text not used.
> Will be modified in -15 related to Dale's comments.
>
>         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".
>
>
>
> fixed cut-and-paste 'input' (in -15)
>
>
>     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?
>
>
>
> 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.
ok, thanks.
>
>
>>       - 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?
>
>
>
>
> fixed (in -15)
> assigned instead of derived from a YANG module name.
>
>>        
>>
>>     -
>>     Since we have many examples around around example-jukebox, this should pass compilation, right?
>>
>>     pyang --ietf example-jukebox.yang
>
>
>
>
> I disagree that --ietf should be added to the validation of 
> non-normative modules.
>
>>     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
>
>
> this was done in -14
Ok, I guess that none of the readable data nodes in this YANG module may 
be considered sensitive or vulnerable in some network environments

Btw, should we update 
https://trac.tools.ietf.org/area/ops/trac/wiki/yang-security-guidelines 
now that we will have RESTCONF next to NETCONF. I guess so.

Regards, Benoit
>>     Regards, Benoit (OPS AD)
> Andy

---

From: Andy Bierman <[email-masked]>
Date: Sat, 2 Jul 2016 09:15:31 -0700

On Sat, Jul 2, 2016 at 1:31 AM, Benoit Claise <[email-masked]> wrote:

> Hi Andy,
>
>
>
> On Wed, Jun 29, 2016 at 6:13 AM, Benoit Claise <[email-masked]> wrote:
>
>> Dear authors,
>>
>> addressed, instead of me spending a couple of hours reviewing all comments
>> and diffs.
>>
>> ERROR: 'draft-ietf-netconf-restconf-14.txt', Line 3930 - 'module'
>> statement within another module
> 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
>> Testing https://github.com/donaldh/yang-extractor (which I learned of
>> today), it works fine on this draft.
>> - https://github.com/netconf-wg/restconf/issues?q=is%3Aopen+is%3Aissue
>> The idea is to refer to the definitions in RFC6020bis, now that they have
>> been introduced:
>>
>     API                                       none
>  none
>     Data                                        MAY
> SHOULD
>>    ([RFC7232], Section 2.2 <https://tools.ietf.org/html/rfc7232#section-2.2>) header is returned in the response for a
>>    instances of these *input *parameters are encoded in the module
>>    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*"
>>
> 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
>

Kent and I discussed whether the list of operation names retrieved under
/restconf/operations
was a threat, but we decided it wasn't because all the information can be
derived from
the YANG library contents.

Andy

---

From: Andy Bierman <[email-masked]>
Date: Tue, 5 Jul 2016 11:04:42 -0700

On Fri, Jul 1, 2016 at 10:44 AM, Eric Voit (evoit) <[email-masked]> wrote:

> Is there any concern with sequencing of transactions here?  Section 2.1
> says “Responses MUST be sent in the same order that requests are
> received.”   Different POST, PUT, PATCH Server responses might end up on
> different HTTP/2 streams and still meet this requirement.  We should
> preclude this possibility in the text.
>
>
>
> Is it feasible to add requirement that all requests made from one HTTP/2
> stream must be returned within a single HTTP/2 stream?   This would enable
> pipelining for an initiating application.   It would also allow parallelism
> for RESTCONF Server initiated messages which can safely be multiplexed.
>
>
>

How about this text added to  sec 2.1?

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

I think this also covers the GET for notifications.

Eric

Andy

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Wed, 6 Jul 2016 17:24:18 +0000

From: Andy Bierman, July 05, 2016 2:05 PM

On Fri, Jul 1, 2016 at 10:44 AM, Eric Voit (evoit) <[email-masked]<mailto:[email-masked]>> wrote:
Andy Bierman, July 01, 2016 12:13 AM
On Wed, Jun 29, 2016 at 6:13 AM, Benoit Claise <[email-masked]<mailto:[email-masked]>> wrote:

- section 1

I see in section 2.1

Do you mean:

So:

<Eric>  I think this allows the main objective of allowing parallelism as a client can use different streams for those requests which can safely be done in parallel.  (This shouldn’t add anything surprising as connections to a RESTCONF server can come in from multiple clients.)

I think the wording could be tweaked a little bit as here are benefits from the HTTP/2 protocol if using the same steam-id for responses is not mandated.

My suggested text would be:

HTTP/2 [RFC7540] MAY be used for RESTCONF.  The server MUST respond using 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.

Eric
Eric

Andy