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

[Kent Watsen <[e-mail]>]

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.



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

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.

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.


> Michael


Kent