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

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

Hi Kent,

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

I think that this means that you have a full set of AD reviews for all 9 drafts, so you should be able to process/update them together.

This draft is also very well written.  It is also, not surprisingly, structurally very similar to the RESTCONF draft, and hence where appropriate, I would like you to regard the comments that I made on the RESTCONF draft as equally applying to this draft and I will assume that they will have the same resolution and be updated in both unless you indicate in your replies otherwise.  Thus, I have not reflagged the same issues inline in the NETCONF draft but did try and include a summary (below) of the main points that I raised in the other reviews that should also be considered here.  Hopefully, this is pragmatic/sufficient, but let me know otherwise.

I've also flagged a couple of other minor comments that seemed to be specific to this draft.

Moderate level comments:

(1) p 0, sec 

   This document defines two YANG modules, one module to configure a
   NETCONF client and the other module to configure a NETCONF server.
   Both modules support both the SSH and TLS transport protocols, and
   support both standard NETCONF and NETCONF Call Home connections.

I'm assuming that the comments that I've made previously on the other drafts, in particular the RESTCONF draft, will also be taken into consideration for this document, and resolved similarly (rather than raising all my questions again).

- RFC editor guidance on self references
- Relationship diagram clarification.
- Empty groupings comment 
- Support for on-demand connections rather than just persistent and periodic
- No endpoints container under grouping netconf-client-app-grouping/listen
- "Both the "initiate" and "listen" subtrees must be enabled " is unclear comment
- Query about "central" being the best prefix.
- Use of top level "*-supported" features vs putting them in a separate module.
- Avoid describing the reason why containers and p-containers exist in the YANG descriptions.
- Question about use of p-containers with lists requiring 1 or more entries (could just use a np-container and list of 0 or )
- Security considerations should list paths from groupings.
- Include expanded groupings, e.g., in the appendix?



Minor level comments:

(2) p 32, sec 3.2.  Example Usage

           <netconf-server-parameters>
             <!-- nothing to configure -->
           </netconf-server-parameters>

Possibly elide this, otherwise it implies that the netconf-server-parameter container is needed ...


(3) p 43, sec 3.3.  YANG Module

               uses ncs:netconf-server-grouping {
                 refine "client-identity-mappings/cert-to-name" {
                   min-elements 1;
                   description
                     "The TLS transport requires a mapping.";
                 }
               }

I note that this refinement appears in the NETCONF module, but an equivalent refinement doesn't exist in the RESTCONF module.  Hence, I wanted to check that was intentional.



Nit level comments:

(4) p 43, sec 3.3.  YANG Module

                   description
                     "NETCONF/TLS servers MUST validate client
                      certificates.  This configures certificates
                      at the socket-level (i.e. bags), more
                      discriminating client-certificate checks
                      SHOULD be implemented by the application.";

s/, more/. More/?


(5) p 45, sec 3.3.  YANG Module

                 refine "client-identity-mappings" {
                   if-feature "sshcmn:ssh-x509-certs";
                   description
                     "Augments in an 'if-feature' statement
                      ensuring the 'client-identity-mappings'
                      descendant is enabled only when SSH
                      supports X.509 certificates.";
                 }

s/Augments in/Adds in/.  I would suggest steering clear of using the term augment when not adding in new data nodes.


(6) p 46, sec 3.3.  YANG Module

                   description
                     "NETCONF/TLS servers MUST validate client
                      certificates.  This configures certificates
                      at the socket-level (i.e. bags), more
                      discriminating client-certificate checks
                      SHOULD be implemented by the application.";

s/, more/. More/?

Regards,
Rob