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

[Kent Watsen <[e-mail]>]

Hi Rob,

Thank you for your review.
Please see below for responses to your comments.

Kent // author


> On Jun 29, 2023, at 9:36 AM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:
> 
> 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).

Ack.


> (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.


KENT - factor out top-level container to its own YANG module?
KENT - factor out top-level container to its own YANG module?
KENT - factor out top-level container to its own YANG module?

- Foo.yang
- Foo-stacks.yang
- Foo-app.yang


> (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.


This choice statement is in the "restconf-client-app-grouping” grouping.
This grouping has “description” statement:

    description
      "A reusable grouping for configuring a RESTCONF client
       application that supports both 'initiate' and 'listen'
       protocol stacks for a multiplicity of connections.";

Note the word “application”.   The intention of the “app” grouping is to
support NMS/Controller clients that wish to maintain long-lived
connections to devices (RESTCONF-servers in this case) that the 
NMS/Controller manages.

The two “connection-type” case statements: are “persistent” and “periodic”.
Please be aware that “periodic" doesn’t exclude "on-demand”, as an
NMS/controller can always connect ASAP.  Technically, “periodic” is more 
like “on-demand and periodic”, but that’s wordy (and not always true), so
I shortened it to just “periodic”.  The “description” statement could be 
improved to be clearer on this point...

Is this what you had in mind wrt “on-demand”?  (i.e., as an adjunct to “periodic”)

Or did you  mean just “on-demand” by itself?  If so, would this make sense for
an *app* that is trying to maintain long-lived connections to servers that may
need to push notifications over the client-initiated connections?



> (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.

The idea behind “across reboots" is that it might make use of caches that the "last-connected” endpoint may have created and possible reuse.  To what extent it matters I cannot say.   

Should I add this detail or remove the last sentence all together?

I’d rather remove the sentence than make it a MAY, since there is an implicit SHOULD in the first sentence…

Thoughts?



> (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.

I agree that the previous text insufficiently made readers aware.

But instead "carrying up” every NACM flag set in recursively used groupings, I added this statement.

          Please be aware that this YANG module uses groupings from
          other YANG modules that define nodes that may be considered
          sensitive or vulnerable in network environments.   Please
          review the Security Considerations for dependent YANG modules
          for information as to which nodes may be considered sensitive
          or vulnerable in network environments.

I added this to every module-specific Security Consideration section in all of the "*-client-server” drafts.  That is, all drafts except crypto-types, truststore, and keystore, as these drafts already “carried up” NACM flags set in imported modules.

Good enough?



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

Removed.  Also removed in the netconf-client-server draft.



> (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.

Agreed.    s/must be enabled by/are predicated by/

Fixed in the netconf-client-server draft also.



> (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'.

The “central” term is now used in the truststore, keystore, netconf, and restconf drafts.   I think that, before, the term “global” was used….  If you want to change the term again, perhaps “top-level”?

I personally wish these top-level containers didn’t exist in the netconf/restconf drafts, but the WG previously said they thought there were important.  :sigh:   FWIW, the top-level nodes *are* useful in the truststore/keystore drafts...


> (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.

Good catch.

For “http-listen”:  s/call-home connections/call-home connections using HTTP/
For “https-listen”: s/call-home connections/call-home connections using HTTPS/



> (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?

Fixed.  In netconf-client-server draft too.

Also fixed the other “stack” grouping in a similar way.
  - it now says “listen on a single port”.



> (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.

Second sentence is removed.  A total of four locations in the two restconf modules.

Interestingly, the second sentence was not present in the two netconf modules.


> (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.

Agreed.  In all drafts, I updated all of the description statements to NOT mention that it was a “wrapper...to avoid name collisions”.



> (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.

Agreed.  Removed not just second sentence, but second paragraph (also in the netconf-client-server draft)
I removed the third sentence also to be consistent with the other similar nodes in those modules.



> (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.

Let’s first think about a persistent connection:  The client sends RPCs when it wants, and the server sends notifications when it wants.  Now extend that to periodic connections: replace “when it wants” with “on the next periodic connection”.

Of course, in some systems, the server can still send “when it wants” by forcing the connection to be established ASAP.  For this reason some might call it an “on-demand/periodic” connections.  But since that’s too long, and not always the case, I shortened to just “periodic” connection.  

Thoughts?


> (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'.

Please see my response to your comment in the client module.



> (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.

Agreed.  Removed.  Shocking that such typos are still being found (thank you for being thorough!)



> (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.

Egads, absolutely not.  It was supposed to be "https-call-home”.   Fixed now.

Note: the feature was already correct in the netconf-client-server draft.


> (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.

The problem is that they do wrap so much as to become unreadable.

If we were to add fully-expanded tree diagrams to the appendix, I would want to see corresponding Datatracker update that unfolds the artwork for all document formats except the plain-text format.  Especially for web, a horizontal scrollbar would be excellent...



> (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)?

Correct.

“Remote-address” is for where the call-home connection goes.
“Server-name” is how the RESTCONF-server announces itself to the RESTCONF-client it connects to.

These are different things.



> (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.

Correct, nor was it intended to.

The next “feature” in the module is used:

110   feature https-call-home {
111     description
112       "The 'https-call-home' feature indicates that the RESTCONF
113        server supports initiating connections to RESTCONF clients.";
114     reference
115       "RFC 8071: NETCONF Call Home and RESTCONF Call Home";
116   }




> (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.

True, for this case, but leaving as is allows a consuming module to augment-in more “mandatory true” nodes outside the list.  Thoughts?


> 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?

Ah yes, I remember seeing this before…

I added the wrapping “endpoints” container (in both modules in this draft and also the netconf-client-server draft)

FWIW, as annoying as it is, I always try (in IETF drafts) to have a wrapping “container” for lists so that the entire list can be deleted in a single protocol operation. 


> (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.

Please see my response above to this issue.



> 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/?

Yup, found and fixed already  (also in the netconf-client-server draft)



> (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/

Fixed (also in two other locations where used)


> Thanks,
> Rob

K.