Skip to main content

Early Review of draft-ietf-taps-interface-13
review-ietf-taps-interface-13-artart-early-sparks-2021-09-17-00

Request Review of draft-ietf-taps-interface
Requested revision No specific revision (document currently at 26)
Type Early Review
Team ART Area Review Team (artart)
Deadline 2021-09-17
Requested 2021-07-30
Requested by Reese Enghardt
Authors Brian Trammell , Michael Welzl , Reese Enghardt , Gorry Fairhurst , Mirja Kühlewind , Colin Perkins , Philipp S. Tiesel , Tommy Pauly
I-D last updated 2021-09-17
Completed reviews Secdir Telechat review of -22 by Sean Turner (diff)
Dnsdir Telechat review of -22 by Matt Brown (diff)
Intdir Telechat review of -22 by Tatuya Jinmei (diff)
Secdir Early review of -13 by Sean Turner (diff)
Artart Early review of -13 by Robert Sparks (diff)
Artart Last Call review of -20 by Robert Sparks (diff)
Genart Last Call review of -20 by Thomas Fossati (diff)
Secdir Last Call review of -20 by Sean Turner (diff)
Comments
This document is highly related to draft-ietf-taps-arch, for which we also request review.
Both documents are held until draft-ietf-taps-impl is finished as well, and the three docs are aligned.
Assignment Reviewer Robert Sparks
State Completed
Request Early review on draft-ietf-taps-interface by ART Area Review Team Assigned
Posted at https://mailarchive.ietf.org/arch/msg/art/g3nBK52CUcWroPMW2bZR15BjdRk
Reviewed revision 13 (document currently at 26)
Result On the Right Track
Completed 2021-09-17
review-ietf-taps-interface-13-artart-early-sparks-2021-09-17-00
This is an art-art early review of draft-ietf-taps-interface

This document reads fairly well. I have some structural concerns (see the early
review of the architecture document). In particular I think this document
contains the actual definition of the architecture and some of that is
inferential.

There are several places where the language in this document, once published as
an RFC, will not age well. Search for occurrences of the word "modern" and
consider writing less to motivate and more to define. Reconsider the use of
terms like "traditional".

Reconsider words like "atop" and "underneath" when talking about interface
boundaries. (Also, I don't know what it means to be atop an architecture.)

I cringe at systems that define Integers to be integers and some other
enumerated things ("special values"). Is it necessary to define things this
way? I understand it's currently typical to have this behavior in the APIs of
the protocols this system is attempting to abstract, but is it impossible to
stop propagating that kind of type confusion? This API could move the things
special values are being used for into separate arguments for example.

The definition of Numeric in 1.1 is circular.

The concept of cloning is used in the overview without introduction. I've
suggested an earlier discussion of it in the architecture document, which would
help, but even in this document, there should be an introduction or forward
reference.

As you can tell, I don't think the documents do what the first sentence of the
API summary claims. This document would not lose anything if you deleted the
sentence.

At the end of the API summary, there's a paragraph that looks like a "Structure
of this document" discussion - should it be in a different place? The list in
the paragraph is odd - why is Section 8, for instance, not in the list?

Consider showing an explicit context in the Send in the example in 3.1.1 - it
would make the reader guess less to wait to introduce implicit contexts later.

In the text version of the document, the last paragraph before 3.1.3 has some
markup (~~~) that has leaked through.

3.1.3 has a comment that's too long for the text version

The first sentence-paragraph of section 4 is complex. Please break it apart.

At the discussion of Transport Property Names at 4.1, I think you need to be
more specific than "alphanumeric". Explicitly list what characters are allowed.

The "reformatted where necessary" part of the requirement in the last paragraph
of 4.1 makes the MUST NOT almost impossible to conform to. I think I understand
what the paragraph is trying to say, but the current formulation leaves the
reader guessing. I think you are trying to say "Avoid using any of the terms
listed as keywords in the protocol numbers registry as any part of a vendor or
implementation-specific property name".

Do you really mean "host" at "(endpoints) SHOULD represent the same host" in
section 6, or do you mean something more like service, given that an endpoint
may configured with a DNS name?

I think there's a need for more discussion and clarity around the restriction
that "An Endpoint cannot have multiple identifiers of a same type set." Why
not? You effectively are letting the endpoint have (potentially) multiple IP
addresses when you create it with an identifier that is a DNS name. This feels
like an artificial, inconsistent, restriction given the rest of the document
(look at the 3rd paragraph of 6.1).

Would there be any benefit of having something like "WithServiceName" to
qualify a specifier for use with NAPTR/SRV? Or is the thinking that the
resolution of the thing handed down through WithHostname is restricted to
A/AAAA?

Why does the API have both the ability to say LocalSpecifier.WithInterface and
to set interface requirements on TransportProperties? Why not just keep the
latter and remove the former?

I would really like to see an example of how an application that relies on ICE
learns about new candidates as they are discovered - the callback pattern is
not clear to me yet.

There are several places where the document says "Changes to a <foo> after
func() has been called do not have any connection on existing <bar>s". This
feels like an opportunity for more clarity about what a <foo> really is. The
document uses the word Object, and maybe there's an assumption that there's a
shared understanding of what the properties of an Object are. Being explicit
with something like "An Object is a collection of properties that
implementations of the API read and use..." to avoid the "here's a chunk of
memory that can get passed into the API that it can operate on, and expect the
caller to see the effects of the operation" interpretation. I _think_ what the
current text is trying to say is that the user of the API should assume the
implementation of the API made its own private copy of the Object at the point
of the given func() call and that it will not see any changes to the copy the
user has at any later point. But then there's some text that looks like "Once a
<foo> has been used with a func(), it is invalid to modify any of its
properties." So...

I'll have to read the document yet another time, perhaps, but after the few
times I went through it for this review, I'm still not clear on how/whether
connections are grouped because of actions from the remote end(s). If so, how
does the user learn about the grouping? Does it have to poll
Connection.GroupedConnections() every time its told of a new Connection?

In section 8's third paragraph, the MUST NOT is not a good use of 2119/8174.
Are you trying to say protocol specific property _names_ must not be reused
across different protocols?

At the last bullet before section 8.1, are you RFC7556 being pointed to as an
example way one might derive properties? If so, adjust the language to make it
clear that its an example.

In 8.1.1 the second sentence is convoluted. Is there a way to restate it
without as many nots, nons, an different zeros?

At 8.1.2 you say lower values have higher priorities for connPrio. But later,
at msgPrio, higher values have higher priority, and in the examples where you
speak of the interaction of connPrio and msgPrio, you imply that higher means
higher for _both_ properties. Please check for consistency and clarify.

Is the "scope" used for message contexts (9.1.1) ever anything but a framer? If
not, could this section just say framer?

At the discussion of Send Events, I worry that the API is closing a door on
allowing the api to channel information about what's happening because of a
Send that may be available from future transports. Maybe such things would be
communicated as events from Connection instead (like Connection->SoftError),
even if they happened to be Send specific?