[netconf] AD review of draft-ietf-netconf-tcp-client-server-16

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

Hi Kent, Michael,

Thanks for another well written document in the set.  Comments below.

Moderate level comments:

(1) p 0, sec 

   This document defines three YANG 1.1 modules to support the
   configuration of TCP clients and TCP servers.  The modules include
   basic parameters of a TCP connection relevant for client or server
   applications, as well as client configuration required for traversing
   proxies.  The modules can be used either standalone or in conjunction
   with configuration of other stack protocol layers.

I suspect that we may get a DISCUSS for not supporting QUIC - but then this is a TCP model ;-).  But I guess that we should ask the QUIC WG if they are interested in helping construct an equivalent model in future.


(2) p 7, sec 2.2.  Example Usage

   <tcp-common xmlns="urn:ietf:params:xml:ns:yang:ietf-tcp-common">
     <keepalives>
       <idle-time>15</idle-time>
       <max-probes>3</max-probes>
       <probe-interval>30</probe-interval>
     </keepalives>
   </tcp-common>

I note that your example (and the subsequent ones) uses significantly shorter times than those recommended in the prose above.  Should the idle time be greatly increased in the example?  Or further text be included to justify or explain this example?


(3) p 20, sec 3.3.  YANG Module

       uses tcpcmn:tcp-common-grouping {
         augment "keepalives" {
           if-feature "tcp-client-keepalives";
           description
             "Add an if-feature statement so that implementations
              can choose to support TCP client keepalives.";
         }
       }

I think that this YANG is incorrect, or more precisely it has no effect, and to add an if-feature statement to the existing keepalives container it should be a refine statement instead of an augment statement.



Minor level comments:

(4) p 0, sec 

   The "Relation to other RFCs" section Section 1.1 contains a self-
   reference to this draft, along with a corresponding Informative
   Reference in the Appendix.

Please add more specific guidance, as per the other drafts.


(5) p 3, sec 1.  Introduction

   The modules focus on three different types of base TCP parameters
   that matter for TCP-based applications: First, the modules cover
   fundamental configuration of a TCP client or TCP server application,
   such as addresses and port numbers.  Second, a reusable grouping
   enables modification of application-specific parameters for a TCP
   connections, such as use of TCP keep-alives.  And third, client
   configuration for traversing proxies is included as well.  In each
   case, the modules have a very narrow scope and focus on a minimum set
   of required parameters.

Perhaps indicate that this could be extended in future, if required?


(6) p 5, sec 2.1.1.  Model Scope

   This document defines a common "grouping" statement for basic TCP
   connection parameters that matter to applications.  In some TCP
   stacks, such parameters can also directly be set by an application
   using system calls, such as the sockets API.  The base YANG model in
   this document focuses on modeling TCP keep-alives.  This base model
   can be extended as needed.

Perhps here, or in 2, or 2.1, indicate that this model is intended to be common to be TCP clients and servers (I know that is also stated in the intro.)


(7) p 6, sec 2.1.4.  Protocol-accessible Nodes

   The "ietf-tcp-common" module defines only "grouping" statements that
   are used by other modules to instantiate protocol-accessible nodes.

Maybe delete this section, or perhaps more explicitly indicate that it does not define/expose any protocol-accessible nodes?  This comment applies to the client and server modules also.


(8) p 6, sec 2.1.5.  Guidelines for Configuring TCP Keep-Alives

   *  The default idle interval (leaf "idle-time") MUST default to no
      less than two hours, i.e., 7200 seconds [RFC1122].  A lower value
      MAY be configured, but keep-alive messages SHOULD NOT be
      transmitted more frequently than once every 15 seconds.  Longer
      intervals SHOULD be used when possible.

Why not set the YANG default idle interval to 2 hours?  In fact, why not assign defaults to all of these parameters?  Or is the expectation that when these groupings are used, they may be refined with appropriate default values for the application?


(9) p 7, sec 2.1.5.  Guidelines for Configuring TCP Keep-Alives

   *  The maximum number of sequential keep-alive probes that can fail
      (leaf "max-probes") trades off responsiveness and robustness
      against packet loss.  ACK segments that contain no data are not
      reliably transmitted by TCP.  Consequently, if a keep-alive
      mechanism is implemented it MUST NOT interpret failure to respond
      to any specific probe as a dead connection [RFC1122].  Typically,
      a single-digit number should suffice.

Some of this guidance might be better in the description in the YANG model, or alternatively having a reference back to this section.


(10) p 8, sec 2.3.  YANG Module

     description
       "This module defines reusable groupings for TCP commons that
        can be used as a basis for specific TCP common instances.

Again, perhaps indicate that this is common to both TCP client and TCP server implementations.


(11) p 11, sec 3.1.2.1.  The "tcp-client-grouping" Grouping

   *  The "remote-address" node, which is mandatory, may be configured
      as an IPv4 address, an IPv6 address, a hostname.

I wanted to check that you wanted to allow binding to zoned addresses here.  Actually, I note that RFC 6991 (or 6991bis) don't currently define a hostname-no-zone.


(12) p 12, sec 3.1.2.1.  The "tcp-client-grouping" Grouping

   *  The "local-port" node, which is enabled by the "local-binding-
      supported" feature (Section 2.1.2), is not mandatory.  Its default
      value is '0', indicating that the operating system can pick an
      arbitrary port number.

Is using a default value the best approach here, rather than just not having any default statement?


(13) p 13, sec 3.2.  Example Usage

   <tcp-client xmlns="urn:ietf:params:xml:ns:yang:ietf-tcp-client">
     <remote-address>www.example.com</remote-address>
     <remote-port>443</remote-port>
     <local-address>0.0.0.0</local-address>
     <local-port>0</local-port>
     <proxy-server>
       <socks5-parameters>
         <remote-address>proxy.example.com</remote-address>
         <remote-port>1080</remote-port>
         <authentication-parameters>
           <username-password>
             <username>foobar</username>
             <cleartext-password>secret</cleartext-password>
           </username-password>
         </authentication-parameters>
       </socks5-parameters>
     </proxy-server>
     <keepalives>
       <idle-time>15</idle-time>
       <max-probes>3</max-probes>
       <probe-interval>30</probe-interval>
     </keepalives>
   </tcp-client>

Possibly for this example, it could elide the remote-port, local-address and local-port values to illustrate that they are not strictly requried to be populated.


(14) p 18, sec 3.3.  YANG Module

               container authentication-parameters {
                 presence
                   "Indicates that an authentication mechanism
                    has been configured.  Present so that the
                    mandatory descendant nodes do not imply that
                    this node must be configured.";

Another, arguably slightly simpler, choice here would be to remove the authentication-parameters presence-container and make the auth-type choice non mandatory.  gss-api and username-password would presumably become presence containers.



Nit level comments:

(15) p 6, sec 2.1.3.1.  The "tcp-common-grouping" Grouping

   Comments:
   *  The "keepalives" node is a "presence" node so that the mandatory
      descendant nodes do not imply that keepalives must be configured.

I suggest "presence container" rather than "presence" node.


(16) p 6, sec 2.1.5.  Guidelines for Configuring TCP Keep-Alives

   Keep-alive mechanisms exist in many protocols.  Depending on the
   protocol stack, TCP keep-alives may only be one out of several
   alternatives.  Which mechanism(s) to use depends on the use case and
   application requirements.  If keep-alives are needed by an
   application, it is RECOMMENDED that the aliveness check happens only
   at the protocol layers that are meaningful to the application.

s/aliveness check/liveness check/?


(17) p 9, sec 2.3.  YANG Module

         presence
           "Indicates that keepalives are enabled.  This statement is
            present so the mandatory descendant nodes do not imply that
            this node must be configured.";

I suggest removing the second sentence.

Regards,
Rob