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

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

Hi Kent, Michael,

Thanks for the comments.  As per inline below, I think that keepalives should remain as a presence container, and suggested tweaking the example further a bit.  Other than that I’ve checked the diff and I think that we are good to go.

Please see inline …

From: Kent Watsen <[e-mail]>
Date: Monday, 10 July 2023 at 16:13
To: Scharf, Michael <[e-mail]>, Rob Wilton (rwilton) <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] Extensions <[e-mail]>
Subject: Re: [netconf] AD review of draft-ietf-netconf-tcp-client-server-16
Hi Michael, thanks for jumping in.

All, please see my comment below.  Hopefully this is the end of the AD-review on this draft...

Kent



On Jul 6, 2023, at 5:23 AM, Scharf, Michael <[e-mail]> wrote:

Hi all,

Please see inline some comments on the pending issues. I have removed the points that are apparently resolved already.


Moderate level comments:

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

Michael (co-author), I believe that you wrote this text.  Can you guide us here?

<snip>
  Given the cost of keep-alives, parameters have to be configured
  carefully:

   *  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.
</snip>

Good catch. Out of my head, these values have been used in the draft since day one, i.e., before the reference to RFC 1122 was added.

It makes sense to use more conservative values in the example. A proposal would be:

<tcp-common xmlns="urn:ietf:params:xml:ns:yang:ietf-tcp-common">
 <keepalives>
   <idle-time>7200</idle-time>
   <max-probes>9</max-probes>
   <probe-interval>75</probe-interval>
 </keepalives>
</tcp-common>

These are the defaults in the Linux kernel 6.1.0 (tested using Debian 12), i.e., I guess that order of magnitude is somewhat common.


Perfect.  I updated all of the examples to use these values.

This item is considered resolved.

Below I have suggested that we keep the YANG default values, but also keep “keepalives” as a presence container (which would align to the requirement in RFC 1122 that keepalives are off by default.

With this, to enable keepalives you only need to configure the keepalives presence container and not the values underneath.  So, I would suggest perhaps changing some of the examples (if there is more than one) to only include the presence containers, and if going to include values then perhaps include not default values.





Minor level comments:

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

Good questions for Michael (my co-author), who worked on this section of
the draft.

Well, to be honest, I don't recall why we have not assigned default values. When the draft was discussed in TCPM, there has been some pushback regarding use of keep-alive messages in general. Also, different applications may have different timing requirements. One key requirement in RFC 1122 is that keep-alives should be off by default. No assigning default values is somewhat consistent with that.

Yet, the reality is that TCP stacks have default values. As long as the guidance in RFC 1122 is taken into account, I don't believe adding a default value to the YANG model would be harmful, e.g. the ones used by Linux (see above).

To be on the safe side, I have added the TCPM list in CC, given past TCPM discussions.

Rob, I also see no harm in specifying default values.  Applications can still refine the groupings with app-specific defaults.  This being the case, I’ve now set Michael’s values for the defaults, and removed the “mandatory true”, as well as removed the “presence” statement on the parent container.

I think that the presence container should be kept.  I.e., so, then you have to create the keepalive container if you want to enable keepalives but you don’t need to specify the values for the keepalives, the default values will be used.


Regarding “pushback” on TCP-keepalives, it is notable that keepalive may alternatively (and likely preferably) be configured at the SSH and TLS layers.  That said, keepalives are a real thing at the TCP-layer, and thus it seems proper to allow them to be configured.  Also, note that the TCP-layer may be used outside of a SSH/TLS context.

This item is considered resolved.




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

Michael, can you look at the “description” statement in the "ietf-tcp-common"
YANG module too?

The "description" statement already summarizes the most important constraints from Section 2.1.5, albeit in very few words and without much background.

I don't know if the whole text from 2.1.5 needs to be added to the description in the YANG model.

In my point of view, in the YANG model a reference to section 2.1.5 would be sufficient, e.g., along the lines of:

"Further guidance can be found in Section 2.1.5 of RFC DDDD."

This is my personal view. As the normative guidance in Section 2.1.5 was discussed in TCPM, it makes sense to have TCPM in the loop.

Added "Further guidance can be found in Section 2.1.5 of RFC DDDD.” to the description statement.

Yes, that looks good.


This item is considered resolved.




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

Generally, I want examples to populate every field, so to they’re visible to
readers.  But you’re right that configuring the defaults is lame.  I changed the
examples (both this one and the w/o proxy example) to configure non default
values:

 <remote-port>8443</remote-port>
 <local-address>192.0.2.2</local-address>
 <local-port>12345</local-port>

This item is considered resolved.

Just to add: The keepalive example values in this example should probably be updated, too.

Good catch, I did that too.

Regards,
Rob



Michael

Kent