Re: [Taps] Artart early review of draft-ietf-taps-impl-12

[Michael Welzl <[e-mail]>]

Dear Bron,

Many thanks for this, this was extremely useful!

Two of your comments relate to normative language, and I think they may require more discussion. To this end, I just opened issue #1037:
https://github.com/ietf-tapswg/api-drafts/issues/1037

To address the others, I just created PR #1036, please see:
https://github.com/ietf-tapswg/api-drafts/pull/1036 <https://github.com/ietf-tapswg/api-drafts/pull/1036>


Detailed answers below:

> On 31 May 2022, at 07:15, Bron Gondwana via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Bron Gondwana
> Review result: Ready with Nits
> 
> Apologies for the really delayed review - I clicked "accept" on this thinking
> I'd get to it immediately, and then was unable to before I went on vacation.
> Thanks Barry for prodding me to get back to it.
> 
> Overall, this document looks fine.  I reviewed it in isolation, without reading
> all the specs to which it refers.
> 
> Thanks for the detailed work.  If I was to criticize the structure at all, I'd
> say it's a bit repetative in some places - but I'm also aware that as a
> reference, it's necessary to repeat the same information!
> 
> Having said that - he's some notes of things I noticed or that confused me on
> the read through - take what you will if it's useful to you!
> 
> Cheers,
> 
> Bron.
> 
> -----
> 
>   [...] During the process of
>   establishment (Section 4), the Connection will not be bound to a
>   specific transport protocol instance, since multiple candidate
>   Protocol Stacks might be raced.
> 
> It took me a while to understand "raced" in this context - but I guess it's
> well known to anyone in the field.

I decided against changing anything here because this is elaborated upon in the architecture draft (draft-ietf-taps-arch), where it says, at the beginning of a section called “Candidate Racing":
"Connection establishment attempts for a set of candidates may be performed simultaneously, synchronously, serially, or using some combination of all of these. We refer to this process as racing, borrowing terminology from Happy Eyeballs [RFC8305].”

Repeating these kinds of statements from the architecture draft here would just make the implementation draft longer; I’d expect implementers who might have trouble with the language to go back to the architecture doc and read it.


> There's a couple of English language things:
> 
>   Once the Connection is established, Transport Services implementation
>   maps actions and events to the details of the chosen Protocol Stack.
> 
> "the Transport Services"?

Done.


>   [...] The properties held by a Connection or Listener is
>   independent of other connections that are not part of the same
>   Connection Group.
> 
> "the properties held by ... are"?

Done.


>   Connection establishment is only a local operation for a Datagram
>   transport (e.g., UDP(-Lite)), which serves to simplify the local
>   send/receive functions and to filter the traffic for the specified
>   addresses and ports [RFC8085].
> 
> That's a mess.  I rewrote it as:
> 
> "For a Datagram transport (e.g. UDP(-Lite)), connection establishment is only a
> local operation, where it serves to simplify the local send/receive functions
> and ..."

Done.


>   The implementation stores these properties as a part of the
>   Preconnection object for use during connection establishment.  For
>   Selection Properties that are not provided by the application, the
>   implementation must use the default values specified in the Transport
>   Services API ([I-D.ietf-taps-interface]).
> 
> Normative MUST?  I guess this is just a guidance document so doesn't need
> anything normative.  There's some later "should" as well which I would
> generally ask the same question for.

This went into the issue mentioned above.


>   This document structures the candidates for racing as a tree as
>   terminological convention.  While a a tree structure is not the only
>   way in which racing can be implemented, it does ease the illustration
>   of how racing works.
> 
> Apart from the 'a a', the first sentence is also clumsy.  Maybe:
> 
> "For ease of illustration, this document structures the candidates for racing
> as a tree, however a tree structure is not the only way in which racing can be
> implemented".

Done.


>   *  "Interface Instance or Type": If the application specifies an
>      interface type to be preferred or avoided, implementations should
>      accordingly rank the paths.  If the application specifies an
>      interface type to be required or prohibited, an implementation is
>      expeceted to not include the non-conforming paths.
> 
> Typo "expected".

Fixed.


>   The set of possible Local Endpoints is gathered.  In the simple case,
>   this merely enumerates the local interfaces and protocols, and
>   allocates ephemeral source ports.  For example, a system that has
>   WiFi and Ethernet and supports IPv4 and IPv6 might gather four
>   candidate Local Endpoints (IPv4 on Ethernet, IPv6 on Ethernet, IPv4
>   on WiFi, and IPv6 on WiFi) that can form the source for a transient.
> 
> I don't recognise the term "transient" as a noun in this context.  Maybe this
> is lack of familiarity.

I don’t recognise this term in this context either. I believe it is meant to stress that the port allocation can be ephemeral, which is already said in the preceding sentence that you quote here. So I just removed “that can form the source for a transient.” - I think this is clear enough without it.


>   The timing algorithms for racing should remain independent across
>   branches of the tree.  Any timers or racing logic is isolated to a
>   given parent node, and is not ordered precisely with regards to other
>   children of other nodes.
> 
> "Any timers or racing logic are"?  Or "Any timer or racing logic is". 
> Something to align the plurals!  Probably the later since the rest of the
> paragraph remains in the singular.

I think that “timers” could be “timer” here because of the “Any” - so I changed this to “Any timer or racing logic is isolated…"


> 4.7.2.  Implementing listeners for Connectionless Protocols
> 
>   Connectionless protocols such as UDP and UDP-lite generally do not
>   provide the same mechanisms that connected protocols do to offer
>   Connection objects.  Implementations should wait for incoming packets
>   for connectionless protocols on a listening port and should perform
>   four-tuple matching of packets to either existing Connection objects
>   or the creation of new Connection objects.  On platforms with
>   facilities to create a "virtual connection" for connectionless
>   protocols implementations should use these mechanisms to minimise the
>   handling of datagrams intended for already created Connection
>   objects.
> 
> This is really fuzzy.  There's either a missing comma or some missing words.

Here’s what I did.
Sentence 1: this reads ok to me, I left it unchanged.
Sentence 2: this is too long and hard to read, IMO, and so I changed it.
Sentence 3: this is confusing because, IMO, it’s not clear what is meant with “minimise the handling of datagrams” - is this talking about the application using the API, or is it about “handling" inside the Transport Services system? How would “virtual connection” functionality minimize this “handling” there? To me, this is really quite unclear, and I’m also not sure that it does add a lot of value. So, I suggest to delete this sentence - if such facilities exist in a system, and they do help, I don’t think that implementers need a hint to make use of them.

As a result, the new paragraph is:

NEW:
  Connectionless protocols such as UDP and UDP-lite generally do not
  provide the same mechanisms that connected protocols do to offer
  Connection objects.  Implementations should wait for incoming packets
  for connectionless protocols on a listening port and should perform
  four-tuple matching of packets to existing Connection objects if possible.
  If a matching Connection object does not exist, an incoming packet from
  a connectionless protocol should cause a new Connection object to be created.


>   The amount of data that can be sent as 0-RTT data varies by protocol
>   and can be queried by the application using the Maximum Message Size
>   Concurrent with Connection Establishment Connection Property.  An
>   implementation can set this property according to the protocols that
>   it will race based on the given Selection Properties when the
>   application requests to establish a connection.
> 
> The capitalisation in there is very confusing to me - I would expect "With" to
> also be capitalised if the whole thing is the name of something, and
> "Concurrent" not to be capitalised otherwise.

There was a time when we referred to Properties from draft-ietf-taps-interface by their long name (really, the heading). See here:
https://ietf-tapswg.github.io/api-drafts/draft-ietf-taps-interface.html#name-read-only-connection-proper <https://ietf-tapswg.github.io/api-drafts/draft-ietf-taps-interface.html#name-read-only-connection-proper>
and this statement must come from this time (hence the capitalization style). Now, we use the actual name of the property - in this case, it should be zeroRttMsgMaxLen.

Accordingly, I changed the first sentence here to:

NEW:
  The amount of data that can be sent as 0-RTT data varies by protocol
  and can be queried by the application using the zeroRttMsgMaxLen
  Connection Property.

NOTE:
=====
This comment made me look for similar mistakes in the document. I did a search for “propert” and it made me do the following additional fixes:

* the list of Message Properties in section 5.1.1: I changed all names here, and I discovered that “noSegmentation" was missing in the list, and the text around fragmentation covered both fragmentation and segmentation. We must have missed to update the implementation draft along with the interface draft here!

So, I did the following change, in line with the interface draft:

OLD:
No Fragmentation: When set, this property limits the message size to the Maximum Message Size Before Fragmentation or Segmentation (see Section 10.1.7 of [I-D.ietf-taps-interface]). Messages larger than this size generate an error. Setting this avoids transport-layer segmentation or network-layer fragmentation. When used with transports running over IP version 4 the Don't Fragment bit will be set to avoid on-path IP fragmentation ([RFC8304]).


NEW:
- noFragmentation: Setting this avoids network-layer fragmentation. Messages exceeding the transport’s current estimate of its maximum packet size (the singularTransmissionMsgMaxLen Connection Property) can result in transport segmentation when permitted, or generate an error. When used with transports running over IP version 4 the Don't Fragment bit will be set to avoid on-path IP fragmentation ([RFC8304]).

- noSegmentation: When set, this property limits the message size to the transport’s current estimate of its maximum packet size (the singularTransmissionMsgMaxLen Connection Property). Messages larger than this size generate an error. Setting this avoids transport-layer segmentation and network-layer fragmentation. When used with transports running over IP version 4 the Don't Fragment bit will be set to avoid on-path IP fragmentation ([RFC8304]).


* I updated the property names mentioned in section 3.1 (“Configuration-time errors”).

* I updated the property name “Connection Priority” to “connPriority" under “Clone:” in section 10.1.

* I updated the property names in section 10.2.

* I updated the last sentence of section 10.3 as follows. NOTE, this is a semantic change, because I think it’s wrong as it stands, accidentally talking about UDP-Lite instead of UDP:

OLD:
Properties that require checksum coverage are not supported by UDP-Lite, such as "Corruption Protection Length", "Full Checksum Coverage on Sending", "Required Minimum Corruption Protection Coverage for Receiving", and "Full Checksum Coverage on Receiving".

NEW:
Properties that require adjusting checksum coverage are only supported by UDP-Liste, such as msgChecksumLen, fullChecksumSend, recvChecksumLen, and fullChecksumRecv.


* further down, for multicast UDP:

OLD:
The "unidirectional receive" transport property is required, and the Local Endpoint must be configured with a group IP address and a port.

NEW:
The “direction" Selection Property must be set to “unidirectional receive", and the Local Endpoint must be configured with a group IP address and a port.

* a few more textual replacements below, for SCTP, referring to some Message Properties

* textual replacements in section B.1 for Bounds on Send or Receive Rate



>   Upon receiving this event, a framer implementation is responsible for
>   performing any necessary transformations and sending the resulting
>   data back to the Message Framer, which will in turn send it to the
>   next protocol.  Implementations SHOULD ensure that there is a way to
>   pass the original data through without copying to improve
>   performance.
> 
> but... here we have some NORMATIVE language.  That SHOULD should probably be
> non-capitalised to align with earlier usage.  (and so on later in the document,
> e.g. in the SCTP description)

This went into the issue mentioned above.


> ----
> 
> That's all the notes I took.

Many thanks again!

Cheers,
Michael