[netconf] AD review of draft-ietf-netconf-restconf-client-server-29

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Kent,

Here is my AD review of draft-ietf-netconf-restconf-client-server-29.

Another draft that is also well written and a pleasure to review - thank you.  Review comments:

Moderate level comments:

(1) p 0, sec 

                   RESTCONF Client and Server Models
              draft-ietf-netconf-restconf-client-server-29

I've not reflagged issues that have been flagged previously, and I presume that you will handle generically/consistently on all drafts (e.g., RFC editor guidance, relationship text, whether to highlight presence containers in the description).


(2) p 10, sec 2.1.3.  Protocol-accessible Nodes

   *  The reason for why "restconf-client-app-grouping" exists separate
      from the protocol-accessible nodes definition is so as to enable
      instances of restconf-client-app-grouping to be instantiated in
      other locations, as may be needed or desired by some modules.

In theory, this could be achieved by whether the module is "import-only" or "implemented" although I have to say that I don't really like the distinction - it seems to add complexity.  An alternative, maybe cleaner, way of solving this could be to put the groupings into a separate "ietf-restconf-client-types" or "ietf-restconf-client-groupings" module separate from the module that defines the protocol nodes for the central client or server.  E.g., I think that it would be a shame if all modules started to follow this module of having top-level if-feature statements.


(3) p 21, sec 2.3.  YANG Module

             choice connection-type {
               mandatory true;
               description
                 "Selects between available connection types.";
               case persistent-connection {

Should there be an option for an "on-demand" connection?  I.e., the details are stored centrally, but a connection is made if needed, and then it is closed (possibly after an idle timeout), if the connection is no longer needed.  This comment also equivalently applies to the server part.


(4) p 23, sec 2.3.  YANG Module

                 enum last-connected {
                   description
                     "Indicates that reconnections should start
                      with the endpoint last connected to.  If
                      no previous connection has ever been
                      established, then the first endpoint
                      configured is used.   RESTCONF clients
                      SHOULD be able to remember the last
                      endpoint connected to across reboots.";
                 }

The SHOULD be able to remember the last endpoint feels like somewhat of a (possibly unnecessary) burden to the client.  I would prefer this to be a MAY, or something softer, or to better understand where this requirement is coming from.


(5) p 47, sec 4.1.  The "ietf-restconf-client" YANG Module

   None of the writable data nodes in this YANG module are considered
   sensitive or vulnerable in network environments.  The NACM "default-
   deny-write" extension has not been set for any data nodes defined in
   this module.

I think that this section (and the one below) must list the paths that are security sensitive in the groupings that it uses, i.e., just deferring the grouping definition is probably not sufficient to make readers sufficiently aware.



Minor level comments:

(6) p 6, sec 2.1.2.1.  The "restconf-client-grouping" Grouping

   *  This grouping does not define any nodes, but is maintained so that
      consuming modules can augment nodes into it if needed.

This probably should be reworded since the grouping doesn't help with augmentations.  There is part of me that wonders whether the grouping is really helpful at all.


(7) p 9, sec 2.1.2.4.  The "restconf-client-app-grouping" Grouping

   *  Both the "initiate" and "listen" subtrees must be enabled by
      "feature" statements.

I find this text slightly unclear, i.e., I presume that this means that they are both predicated by feature statements rather than both features must be enabled when using this grouping.


(8) p 10, sec 2.1.3.  Protocol-accessible Nodes

   *  The top-level node "restconf-client" is additionally constrained
      by the feature "central-restconf-client-supported".

I wasn't sure whether 'central' is the best adjective here, possible alternative suggestions could be 'default' or 'common'.


(9) p 16, sec 2.3.  YANG Module

     feature https-listen {
       description
         "The 'https-listen' feature indicates that the RESTCONF client
          supports opening a port to listen for incoming RESTCONF
          server call-home connections.  This feature exists as not
          all RESTCONF clients may support RESTCONF call home.";
       reference
         "RFC 8071: NETCONF Call Home and RESTCONF Call Home";
     }

To differentiate between these two features, the descriptions should probably clarify that they are listening for HTTPS vs HTTP connections.


(10) p 17, sec 2.3.  YANG Module

     grouping restconf-client-initiate-stack-grouping {
       description
         "A reusable grouping for configuring a RESTCONF client
          'initiate' protocol stack for a single connection.";

Maybe 'single outbound connection', or 'single client connection', to differentiate from call-home?


(11) p 17, sec 2.3.  YANG Module

       choice transport {
         mandatory true;
         description
           "Selects between available transports. This is a
            'choice' statement so as to support additional
            transport options to be augmented in.";

I'm not convinced that the second sentence should be in the YANG model (e.g., if it turns up in UI documentation), but is great in the prose that accompanies the module.  If you agree to change in the module definition then it would make sense to change this in other places as well.


(12) p 17, sec 2.3.  YANG Module

         case https {
           if-feature "https-initiate";
           container https {
             must 'tls-client-parameters/client-identity
                   or http-client-parameters/client-identity';
             description
               "Specifies HTTPS-specific transport
                configuration.";
             container tcp-client-parameters {
               description
                 "A wrapper around the TCP client parameters
                  to avoid name collisions.";
               uses tcpc:tcp-client-grouping {
                 refine "remote-port" {
                   default "443";
                   description
                     "The RESTCONF client will attempt to
                      connect to the IANA-assigned well-known
                      port value for 'https' (443) if no value
                      is specified.";
                 }
               }
             }
             container tls-client-parameters {
               description
                 "A wrapper around the TLS client parameters
                  to avoid name collisions.";
               uses tlsc:tls-client-grouping;
             }

Looking at these further and thinking about how they would get displayed in a UI, I wonder whether these containers would just be better described as "TLS client parameters" rather than mentioning what is really an implementation detail.  If you agree to change this then I presume that you would update in other places and drafts to be consistent.


(13) p 18, sec 2.3.  YANG Module

             container http-client-parameters {
               description
                 "A wrapper around the HTTP client parameters
                  to avoid name collisions.";
               uses httpc:http-client-grouping;
             }
             container restconf-client-parameters {
               description
                 "A wrapper around the RESTCONF client parameters
                  to avoid name collisions.
                  This container does not define any nodes.  It
                  exists as a potential augmentation target by
                  other modules.";

I wasn't sure that the last paragraph is helpful here (and is already stated as part of the grouping), since if anyone does augment in extra nodes then it becomes misleading.


(14) p 22, sec 2.3.  YANG Module

                      The RESTCONF client SHOULD gracefully close
                      the underlying TLS connection upon completing
                      planned activities.

For a periodic connection, how are the planned activities specified, initiated, or controlled?  Having the periodic connection specified in this way seems a bit strange to me, and I'm struggling to understand exactly how it will be used and how everything dovetails together.


(15) p 26, sec 3.1.1.  Features

   Features:
     +-- http-listen
     +-- https-listen
     +-- https-call-home
     +-- central-restconf-server-supported

Similar to the client, I wasn't sure whether 'central' is the best adjective here, possible alternative suggestions could be 'default' or 'common'.


(16) p 26, sec 3.1.2.1.  The "restconf-server-grouping" Grouping

   *  The "client-identity-mappings" node, which must be enabled by
      "feature" statements, defines a mapping from certificate fields to
      RESTCONF user names.

The comment about needing to be enabled via "feature" statements looks to be inaccurate, or otherwise I'm confused by it.


(17) p 28, sec 3.1.2.3.  The "restconf-server-callhome-stack-grouping" Grouping

     grouping restconf-server-callhome-stack-grouping:
       +-- (transport)
          +--:(https) {https-listen}?
             +-- https
                +-- tcp-client-parameters
                |  +---u tcpc:tcp-client-grouping
                +-- tls-server-parameters
                |  +---u tlss:tls-server-grouping
                +-- http-server-parameters
                |  +---u https:http-server-grouping
                +-- restconf-server-parameters
                   +---u rcs:restconf-server-grouping

I just wanted to check, is 'https-listen' the right name for the feature statement?  I had assumed that this configuration would be for an outbound connection rather than an inbound one, and to me listen implies inbound.


(18) p 29, sec 3.1.2.4.  The "restconf-server-app-grouping" Grouping

     grouping restconf-server-app-grouping:
       +-- listen! {http-listen or https-listen}?
       |  +-- endpoint* [name]
       |     +-- name?                                    string
       |     +---u restconf-server-listen-stack-grouping
       +-- call-home! {https-call-home}?
          +-- restconf-client* [name]
             +-- name?                 string
             +-- endpoints
             |  +-- endpoint* [name]
             |     +-- name?                                      string
             |     +---u restconf-server-callhome-stack-grouping
             +-- connection-type
             |  +-- (connection-type)
             |     +--:(persistent-connection)
             |     |  +-- persistent!
             |     +--:(periodic-connection)
             |        +-- periodic!
             |           +-- period?         uint16
             |           +-- anchor-time?    yang:date-and-time
             |           +-- idle-timeout?   uint16
             +-- reconnect-strategy
                +-- start-with?     enumeration
                +-- max-wait?       uint16
                +-- max-attempts?   uint8

I've raised this in a previous review, but I wonder whether it would be helpful to also include the fully expanded groupings for your top-level groupings intended to be used by implementors, and maybe the top level "central" definition as well.  These could end up being fairly long, but assuming that they do not wrap too much (to the point that they become unreadable), then these could just be included in an appendix.


(19) p 32, sec 3.2.  Example Usage

               <http-server-parameters>
                 <server-name>foo.example.com</server-name>
               </http-server-parameters>

The remote-address field and server-name fields differ.  Is this valid (and for the second endpoint below)?


(20) p 37, sec 3.3.  YANG Module

     feature https-listen {
       description
         "The 'https-listen' feature indicates that the RESTCONF server
          supports opening a port to listen for incoming RESTCONF over
          TLS client connections, whereby the TLS connections are
          terminated by the server itself.";
       reference
         "RFC 8040: RESTCONF Protocol";
     }

As per my previous comment, this description doesn't cover that it is also used in the call home stack.


(21) p 42, sec 3.3.  YANG Module

     grouping restconf-server-app-grouping {
       description
         "A reusable grouping for configuring a RESTCONF server
          application that supports both 'listen' and 'call-home'
          protocol stacks for a multiplicity of connections.";
       container listen {
         if-feature "http-listen or https-listen";
         presence
           "Identifies that the server has been configured to
            listen for incoming client connections.  This statement
            is present so the mandatory descendant nodes do not
            imply that this node must be configured.";
         description
           "Configures the RESTCONF server to listen for RESTCONF
            client connections.";
         list endpoint {
           key "name";
           min-elements 1;
           description
             "List of endpoints to listen for RESTCONF connections.";
           leaf name {
             type string;
             description
               "An arbitrary name for the RESTCONF listen endpoint.";
           }
           uses restconf-server-listen-stack-grouping;
         }
       }

Rather than having a presence container here, you could have just had endpoint be a list of 0 or more elements.  A similar comment applies to the call-home container below.

I also note that there is an endpoints container wrapping the endpoints under call-home but not listen.  Presumably this has been done because there are other leaves under call-home/restconf-client, but I'm not sure that you can guarantee that wouldn't ever be the case under listen as well.  I.e., would having the containers in both places be more consistent?


(22) p 46, sec 3.3.  YANG Module

                 enum last-connected {
                   description
                     "Indicates that reconnections should start with
                      the endpoint last connected to.  If no previous
                      connection has ever been established, then the
                      first endpoint configured is used.   RESTCONF
                      servers SHOULD be able to remember the last
                      endpoint connected to across reboots.";

Note, same comment about remembering the last endpoint applies here.



Nit level comments:

(23) p 19, sec 2.3.  YANG Module

             container tcp-server-parameters {
               description
                 "A wrapper around the TCP client parameters
                  to avoid name collisions.";

s/client/server/?


(24) p 39, sec 3.3.  YANG Module

               description
                 "Identifies contact information for the external
                  system that terminates connections before passing
                  them thru to this server (e.g., a network address
                  translator or a load balancer).  These values have
                  no effect on the local operation of this server,
                  but may be used by the application when needing to
                  inform other systems how to contact this server.";

s/thru/through/

Thanks,
Rob