Re: [tcpm] Robert Wilton's No Objection on draft-ietf-tcpm-yang-tcp-07: (with COMMENT)

["Scharf, Michael" <[e-mail]>]

Hi Rob,

Thanks a lot for all these detailed comments.

We have published a new, improved version -08 (cf. https://www.ietf.org/rfcdiff?url2=draft-ietf-tcpm-yang-tcp-08). Please have a look and let us know if this works for you.

More inline.

> -----Original Message-----
> From: Robert Wilton via Datatracker <[e-mail]>
> Sent: Thursday, June 30, 2022 3:29 PM
> To: The IESG <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Robert Wilton's No Objection on draft-ietf-tcpm-yang-tcp-07: (with
> COMMENT)
> 
> Robert Wilton has entered the following ballot position for
> draft-ietf-tcpm-yang-tcp-07: No Objection
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to
> https://www.ietf.org/about/groups/iesg/statements/handling-ballot-
> positions/
> for more information about how to handle DISCUSS and COMMENT
> positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-tcpm-yang-tcp/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Hi,
> 
> Thanks for another YANG module!
> 
> I have some non-blocking comments, I suspect that many of these have
> already
> been raised/discussed so really just want to check that they had been
> proactively considered.
> 
> 1)
>       TCP connection list: Access to status information for all TCP
>       connections.  Note, the connection table is modeled as a list that
>       is read-writeable, even though a connection cannot be created by
>       adding entries to the table.  Similarly, deletion of connections
>       from this list is implementation-specific.
> 
> I think that it would be helpful to further describe and perhaps constrain the
> behaviour here:
>  - I would suggest clarifying that the list is read/writable to allow
>  configuration to be associated with existing connections, or connections that
>  may be subsequently made. - It would be helpful to understand what
> happens if
>  the configuration for a connection changes for an existing connection.  Does
>  it force the connection to be closed and reopened?  Or does it depend on
> the
>  configuration change? - I don't think that deleting the configuration
>  properties should cause a connection to be closed (although it might cause
> the
>  connection to flap, as per my comment above.)  E.g., for BGP, isn't it the BGP
>  neighbour configuration that should control whether a connection logically
>  exists?

We have updated the description in -08 to:

            "List of TCP connections with their parameters.

              The list is modeled as writeable even though only some of
              the nodes are writeable, e.g. keepalive. Connections
              that are created and match this list SHOULD apply the
              writeable parameters. At the same time, implementations
              may not allow creation of new TCP connections simply by
              adding entries to the list. Furthermore, the behavior
              upon removal is implementation-specific. Implementations
              may not support closing or resetting a TCP connection
              upon an operation that removes the entry from the list.

              The operational state of this list SHOULD reflect
              connections that have configured, but not created, and
              connections that have been created. Connections in
              CLOSED state are not reflected on this list.";

Does this work for you?

> 2) With the ambiguity regarding YANG IP address and zones, can I clarify that
> in all cases, the intention is that zoned ip-addresses are allowed where the
> "inet:ip-address" type is used?

This excellent question came up in earlier reviews, too.

As a result, I did some research and found at least one "netstat" implementation that apparently uses zoned IP addresses in some cases (link-local IPv6 addresses, as far as I recall). If we want to allow the equivalent behavior in the YANG model, inet:ip-address would be the right choice.

Yet, to be honest, it is not really clear to me how widely zoned ip-addresses are used in IP stacks beyond the one example that I found. And I simply don't know why the basic "inet:ip-address" definition includes zones.

But I also don't see much harm in keeping the model as is. So far, we have not changed the model regarding that specific detail.

Having said this, personally I would agree to use "inet:ip-address-no-zone" instead, if somebody asks for this.

> 3) leaf enable-ao {
> I presume that there was a conscious decision not to use a presence
> container
> here (and hence avoid the need for the must statements)?

This is now a presence container.

> 4) leaf enable-md5 {
> I presume that implementations won't need to add extra MD5 specific
> parameters?
>  I.e., it doesn't make sense for this to be a presence container?

Same as for TCP-AO.

> 5) Connection is closed. Connections in this state
>                     may not appear in this list.";
> 
> In the operational state tree then what does the tcp/connections list
> represent?  Does it contain all connections that either exist or have some
> configuration associated with them?

See the updated description cited already. Please let us know if more is needed.

> 6) I think that leaf "state" (describing the connection state) should be marked
> as "config false".  A client should not be able to configure the current
> protocol "state".

Good catch. This is now "config false".

> 7)      leaf address {
>            type union {
>              type string;
>              type inet:ip-address;
>            }
> 
> Given the described behaviour, then I think that you should probably:
>  (i) List the inet:ip-address type first.
>  (ii) Add a length "0" restriction to the string type.

The definitions have been wapped to address comment (i).

Sorry, I don't fully understand comment (ii).

> 8)              For an application willing to accept both IPv4 and
>                  IPv6 datagrams, the value of this object must be
>                  ''h (a zero-length octet-string), with the value
>                  of the corresponding 'type' object being
>                  unknown (0).
> 
> What does the h mean?  I've got a nagging feeling that this is a known
> convention ...  I presumed that the WG considered and dismissed the option
> of
> having two separate listeners listed, one for v4 and one for v6 rather than
> the
> union with the string type?

This type and the description is inherited from TCP MIB. The TCP MIB uses only one list both for v4 and v6.

Well, somewhat unfortunately, the TCPM WG actually has not really discussed whether to use two listener lists instead. At first sight, it looks like a reasonable idea, too.

But note that other last call comments asked for feature parity with the TCP MIB. This listener list is actually included as a result of that, and in order to avoid issues, we decided to follow the TCP MIB as far as applicable.

> 9)
> list tcp-listeners {
>          key "type address port";
> This allows multiple listeners for the same port.  I just wanted to check that
> is allowed/expected?

Again, this is inherited from TCP MIB.

It may depend on internals of the operating system whether multiple listeners can exist on the same port, e.g., for different IP addresses. But at first sight I don't think the model should forbid this.

> 10) container statistics
> 
> These counters are global level, so there are not many of them.  Some of
> these
> counters are 32 bits, I presume that there is no reasonable risk that they
> would overflow over a reasonable time frame?

Implementers have reported that 32 bit counters for "in-segments" and "out-segments" overflow in running code, and this is why those two ones are 64 bit already.

For the other counters, overflow of 32 bit seems less likely. But the future is hard to predict...

In order to be on the safe side, we have modified the counters to 64 bit. 

Thanks for these excellent points!

Michael

> Nits:
> 
> included in TCP MIB => included in the TCP MIB
> TCP-AO TCP-AO [RFC5925] => Duplicate TCP-AO.
> 
> Thanks,
> Rob
> 
>