Re: [Netconf] Yangdoctors last call review of draft-ietf-netconf-restconf-client-server-04

[Kent Watsen <[e-mail]>]

Hi Andy,

Thanks for your review.  Below are my responses to your comments.

Thanks,
Kent


--

Reviewer: Andy Bierman
Review result: Ready with Nits

Review: draft-ietf-netconf-restconf-client-server-04

Modules:
 (M1) [e-mail]
 (M2) [e-mail]


YANG Usage:

 I did not find anything wrong in either module.
 pyang and yangdump-pro do not report any errors or warnings.

Comments:

C1:

 (M1) RESTCONF does not have sessions so the term "RESTCONF session"
 needs to be defined


<KENT> good point, in this case, it's actually the underlying transport (TLS) that has a session.  Fixed.  But what about the 'max-sessions' leaf, under 'listen', in both modules?  - should this also be changed to talk about the TLS session, or removed altogether?



C2:

 (M1) feature tls-initiate
 (M1) feature tls-listen
 (M2) feature tls-listen
 (M2) feature tls-call-home

 Are these features really needed since a RESTCONF server MUST support
 the TLS transport?

<KENT> the features themselves are needed, as we don't know what clients/servers may support.

<KENT> the "tls" in the name of these features is likely unnecessary but 1) maybe, someday, there will be another transport? and, 2) these names are consistent with the names in the ssh modules, where there are both the "tls" and "ssh" based feature names, and so make more sense there.



C3:

 (M1)  /restconf-client/initiate/restconf-server/persistent/

 RESTCONF has no session setup; It only has request and
 response interactions. The purpose of reconnect, idle timeouts etc.
 are completely up to the client application.


<KENT> similar to the above, this is really meant to reflect the TLS layer.  It may not make too much sense for /restconf-client/initiate/restconf-server/persistent.   That said, I think it's okay, in the sense that a real HTTPS client might keep the underlying TLS session open across many HTTP-level interactions as a performance optimization.  What do you think?

FWIW, it does *make* sense for /restconf-server/call-home/restconf-client/connection-type/persistent/.  For instance, a client may need to keep the call-home connection open in order to send messages to the server when it needs to...



C4:

 (M1)  /restconf-client/initiate/restconf-server/persistent/max-attempts

 What should the RESTCONF client do if a "Connection: close"
 header is received from the server (indicating the server is dropping
 the TCP connection) Should the reconnects proceed?

<KENT> I don't understand the question.  The 'max-attempts' regards 
testing the aliveness of a server that the client is already connected
to.  But, assuming the client was configured to try to maintain a
persisted TLS connection, the client would immediately try to reconnect
to one of the configured RESTCONF servers.



C5:

 (M1)  /restconf-client/initiate/restconf-server/periodic/


      "Periodically connect to the RESTCONF server, so that
       the RESTCONF server may deliver messages pending for
       the RESTCONF client.  The RESTCONF server must close
       the connection when it is ready to release it.

This is not how RESTCONF works.
The RESTCONF server requires a request message in order to
send content to a client.  I do not understand the use-case
for periodic RESTCONF sessions.

<KENT> Right, the server might be buffering data to send to
the client (e.g., logs).  Maybe it's a wording issue?

OLD

  server may deliver messages pending for the RESTCONF client

NEW

  client can collect data (e.g., logs) from the RESTCONF server

What do you think?




C6:

 (M1)  /restconf-client/initiate/restconf-server/periodic/
 (M2)  /restconf-server/call-home/restconf-client/connection-type/periodic/

 The RESTCONF notifications via SSE should be exempt from timeouts.
 The RESTCONF client should terminate an SSE request. The server should
 not timeout an SSE response connection


<KENT> Agreed, and the description statement for the 'idle-timeout' leaf
says "Sessions that have a notification subscription active are never 
dropped.  But here, under 'periodic', the whole point is to shed idle
connections.  For instance, imagine a client that wants to collect logs
from a low-powered server once a day.  What do you think?



C7:

Sec 1.2 Tree Diagrams

Old text should be replaced with reference to
draft-ietf-netmod-yang-tree-diagrams-01

<KENT> same comment as before.



C8:

 (M1) IETF copyright says 2014; change to 2017
 (M2) IETF copyright says 2014; change to 2017

<KENT> fixed.



Nits:

Some descriptions mention SSH. This should be removed from
this document.

<KENT> done.


The descriptions for choices that have only 1 case should
explain why a choice is being used.

<KENT> done.


The tree diagram shows the fully expanded groupings,
even many objects are in other drafts. I needed all 5 drafts
open in windows for searching, plus pyang tree output,
to really follow the data model structure.

<KENT> I know.  Do you have a proposal?



The examples do not show any usage of the hello-params.
This would be useful because the namespace for the
identityref objects (tls-version, cipher-suite) is not
the same as the module that uses the grouping.

<KENT> Point taken.  I sent Gary an email on this also.



Note that the 'map-type' identityref is shown correctly encoded
in examples on page 19 and 20.

<KENT> not sure which draft you mean, but it's okay, I know how to prefex identities.