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

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

Hi Kent,

Changes look good.  Thanks.  I think this concludes the AD review, but as per my next email, if you get a chance to merge Michaels editorial changes that would be great.

From: Kent Watsen <[e-mail]>
Date: Saturday, 27 January 2024 at 02:14
To: Rob Wilton (rwilton) <[e-mail]>
Cc: Scharf, Michael <[e-mail]>, [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 Rob,

Thanks for the follow-up.
Please find more comments below.

Kent



On Jan 26, 2024, at 6:46 AM, Rob Wilton (rwilton) <[e-mail]> wrote:

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.

Good catch!
Here’s the diff:


     container keepalives {

       if-feature "keepalives-supported";

+      presence

+        "Indicates that keepalives are enabled, aligning to

+         the requirement in Section 4.2.3.6 RFC 1122 that

+         keepalives are off by default.";

Okay.


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.

I didn’t make this change, but can if you think my reasoning is unsound.

Essentially, I generally try to have examples set everything (all values), so that it’s obvious to the reader what all is possible.   Anyone reading the YANG can see the leafs have default values and can conclude that only the presence container needs to be configured, assuming the defaults are acceptable.

Thoughts?

Okay.


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.

I have re-added the presence container (see diff above)


Thanks,
Kent

Regards,
Rob


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