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

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

HI Kent, Michael,

Michael, thanks for these changes.

Kent, if you have time to merge in these changes then I can kick off IETF LC on Monday with these changes in.  Otherwise, they can be merged later, and since they are effectively editorial I can initiate the IETF LC regardless.

Regards,
Rob


From: Scharf, Michael <[e-mail]>
Date: Saturday, 27 January 2024 at 16:57
To: Kent Watsen <[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 all,

Sorry for the delayed reply.

After rereading RFC 9293, I think the references to RFC 1122 are not optimal, including the newest additions.

In fact, RFC 9293 now includes all relevant guidance and replaces these parts of RFC 1122. IMHP we should not reference the old text of RFC 1122.

My proposal would be the following bugfixes to draft-ietf-netconf-tcp-client-server-18, which basically replace all references to RFC 1122 by RFC 9293:


OLD Section 2.1.5.:

Network stacks may include "keep-alives" in their TCP implementations, although this practice is not universally accepted. If keep-alives are included, [RFC1122] mandates that the application MUST be able to turn them on or off for each TCP connection, and that they MUST default to off.

[… and several further paragraphs with similar references to RFC1122 ...]

NEW Section 2.1.5.:

Network stacks may include "keep-alives" in their TCP implementations, although this practice is not universally accepted. If keep-alives are included, [RFC9293] mandates that the application MUST be able to turn them on or off for each TCP connection, and that they MUST default to off.

[… and RFC 1122 is also replaced in all other cases in this section …]


OLD Section 2.3:

     presence
        "Indicates that keepalives are enabled, aligning to
         the requirement in Section 4.2.3.6 RFC 1122 that
         keepalives are off by default.";
      description
        "Configures the keep-alive policy, to proactively test the
         aliveness of the TCP peer.  An unresponsive TCP peer is
         dropped after approximately (idle-time + max-probes *
         probe-interval) seconds.  Further guidance can be found
         in Section 2.1.5 of RFC DDDD.";
      reference
        "RFC 1122:
           Requirements for Internet Hosts -- Communication Layers
         RFC 9293:
           Transmission Control Protocol (TCP), Section 3.8.4.";

NEW Section 2.3:

     presence
        "Indicates that keepalives are enabled, aligning to
         the requirement in Section 3.8.4 RFC 9293 that
         keepalives are off by default.";
      description
        "Configures the keep-alive policy, to proactively test the
         aliveness of the TCP peer.  An unresponsive TCP peer is
         dropped after approximately (idle-time + max-probes *
         probe-interval) seconds.  Further guidance can be found
         in Section 2.1.5 of RFC DDDD.";
      reference
        "RFC 9293:
           Transmission Control Protocol (TCP), Section 3.8.4.";


OLD Section 2.3:

     leaf idle-time {
        type uint16 {
          range "1..max";
        }
        units "seconds";
        default 7200;
        description
          "Sets the amount of time after which if no data has been
           received from the TCP peer, a TCP-level probe message
           will be sent to test the aliveness of the TCP peer.
           Two hours (7200 seconds) is safe value, per RFC 1122.";
        reference
          "RFC 1122:
            Requirements for Internet Hosts -- Communication Layers";
      }

NEW Section 2.3:

     leaf idle-time {
        type uint16 {
          range "1..max";
        }
        units "seconds";
        default 7200;
        description
          "Sets the amount of time after which if no data has been
           received from the TCP peer, a TCP-level probe message
           will be sent to test the aliveness of the TCP peer.
           Two hours (7200 seconds) is safe value, per RFC 9293.";
        reference
          "RFC 9293:
             Transmission Control Protocol (TCP), Section 3.8.4.";
      }


These changes are editorial only. And, well, we should have updated these legacy references to RFC 1122 earlier... Mea cupla.

Michael


From: Kent Watsen <[e-mail]>
Sent: Saturday, January 27, 2024 3:15 AM
To: Rob Wilton (rwilton) <[e-mail]>
Cc: Scharf, Michael <[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]<mailto:[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]<mailto:[e-mail]>>
Date: Monday, 10 July 2023 at 16:13
To: Scharf, Michael <[e-mail]<mailto:[e-mail]>>, Rob Wilton (rwilton) <[e-mail]<mailto:[e-mail]>>
Cc: [e-mail]<mailto:[e-mail]> <[e-mail]<mailto:[e-mail]>>, [e-mail]<mailto:[e-mail]> <[e-mail]<mailto:[e-mail]>>, [e-mail]<mailto:[e-mail]> Extensions <[e-mail]<mailto:[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]<mailto:[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.";


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?



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


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