Re: [Taps] Genart last call review of draft-ietf-taps-interface-20

[Michael Welzl <[e-mail]>]

Dear Thomas,

I want to thank you very much for this extremely detailed review, and also apologize for not answering this for so long - it has somehow fallen through the cracks as we dealt with all the reviews, in fact because it arrived so early!  We merged your PR but I forgot that there was also a much longer email somewhere in one of my mail folders…  I take this blame!

I have now opened issues in our github repository on these matters:
https://github.com/ietf-tapswg/api-drafts/issues

... and we’ll deal with them one by one, there. Please feel free to comment there - and many thanks again!

Cheers,
MIchael



> On Apr 11, 2023, at 1:18 AM, Thomas Fossati via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Thomas Fossati
> Review result: Ready with Issues
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-taps-interface-??
> Reviewer: Thomas Fossati
> Review Date: 2023-04-10
> IETF LC End Date: 2023-04-14
> IESG Telechat date: Not scheduled for a telechat
> 
> The document describes an API that abstracts the transport services
> available on the platform and aims at replacing the BSD socket
> interface.
> 
> The intended status is standards track which looks appropriate.
> 
> The document makes no requests to IANA.
> 
> The document is generally well written and easy to follow.
> 
> Internationalisation-related concerns are minimal.  Though maybe
> something could be explicitly said about the parts of the API that can
> become visible to human eyes, e.g., Message Contexts, and SendError
> reasons.
> 
> Not a blocking issue - because in general the meaning can be inferred
> quite easily - but I've found the typographic conventions to be not
> super clear / consistent.  Some times input parameters are exemplary
> (e.g., "https", 443), other times they are given generic names (e.g.,
> GroupAddress).  It'd be probably neater if the API were described more
> formally in terms of their (abstract) input and output types to prevent
> the reader from inferring the wrong signatures.  Also, examples could
> use a more self-consistent pseudo-code convention, but as per premise,
> this is not blocking.  However, I'd suggest the authors to do a pass on
> the whole document and see if they see opportunities to improve the API
> presentation language.
> 
> The API objects can be quite complex, and there is no explicit
> validation on construction.  Question: is the assumption that
> implementations will do the same?  Or are they are free to, for example,
> use, say, a builder pattern to provide a safer experience for their
> users?
> 
> Question: should Appendix A be moved to draft-ietf-taps-impl?
> 
> Note: some examples have too long lines which will look suboptimally
> when rendered in text and pdf.
> 
> I was curious about the implementation status but I couldn't find
> further info.  If you happen to have implementations already, it'd be
> nice to give them visibility in a BCP205 "Implementation Status"
> section.
> 
> Eight authors are currently listed, but RFC7322 seems to allow at most
> five [1].
> 
> [1] https://www.rfc-editor.org/rfc/rfc7322.html#section-4.1.1
> 
> I have made a PR that fixes a few typos I stumbled upon while reading:
> https://github.com/ietf-tapswg/api-drafts/pull/1128
> 
> # 1.1
> 
> IPv4 and IPv6 addresses should be also listed among the basic types?
> Also, strings (e.g., used for services, interfaces, hostnames).
> 
> # 3.1.1
> 
> HTTPS with raw public keys is not a great match ;-)
> What about using a PK cert rather than raw PK?
> 
> # 3.1.2
> 
> I have found this example to be rather puzzling.  It looks to be sending
> the same message on two different connections to the same HTTPS endpoint
> at roughly the same time.  What is the use case for this?  Also, I
> couldn't tell whether the same output buffer is used for both responses,
> which added to my confusion.
> 
> The in last para:
> 
>  "Preconnections are reusable after being used to initiate a
>   Connection.  Hence, for example, after the Connections were closed,
>   the following would be correct:"
> 
> the positioning of "for example" is slightly ambiguous: it is not clear
> whether it is necessary that all connections derived from a given
> preconnection are closed before one is able to call Initiate() again, or
> not.
> Could you please clarify this aspect?
> 
> # 3.1.3
> 
> Apologies in advance, but my OCD is triggered by typographic
> inconsistencies :-)
> 
> In this example, are the "..."-prefixed comments supposed to have
> different semantics from those that don't?  If so, please explain,
> otherwise remove the "...".
> 
> It was not immediately clear to me whether "sending the ResolvedLocal
> list to peer via signalling channel" is taken care of by
> Preconnection.AddRemote(RemoteCandidates).
> Could you please clarify this?
> 
> Also, please remove the empty line before
> Preconnection.AddRemote(RemoteCandidates), unless there's a good reason
> for deviating from the rest of the examples, which I have missed.
> 
> # 4
> 
> The five paragraphs are slightly patchwork-y.  For example, the second
> para fluctuates from Selection to Connection and then again to Selection
> props.  
> 
> I suggest grouping more tightly around the different properties:
> 
> NEW
> 
> 4.  Transport Properties
> 
>   Each application using the Transport Services API declares its
>   preferences for how the Transport Services system should operate.
>   This is done by using Transport Properties, as defined in
>   [I-D.ietf-taps-arch], at each stage of the lifetime of a connection.
> 
>   Transport Properties are divided into Selection, Connection, and
>   Message Properties.
> 
>   Selection Properties (see Section 6.2) can only be set during pre-
>   establishment.  They are only used to specify which paths and
>   protocol stacks can be used and are preferred by the application.
>   Calling Initiate on a Preconnection creates an outbound Connection or
>   a Listener, and the Selection Properties remain readable from the
>   Connection or Listener, but become immutable.  Selection Properties
>   can be set on Preconnections, and the effect of Selection Properties
>   can be queried on Connections and Messages.
> 
>   Connection Properties (see Section 8.1) are used to inform decisions
>   made during establishment and to fine-tune the established
>   connection.  They can be set during pre-establishment and they may be
>   changed later.  Connection Properties can be set on Connections and
>   Preconnections; when set on Preconnections, they act as an initial
>   default for the resulting Connections.
> 
>   Message Properties (see Section 9.1.3) control the behavior of the
>   selected protocol stack(s) when sending Messages.  Message Properties
>   can be set on Messages, Connections, and Preconnections; when set on
>   the latter two, they act as an initial default for the Messages sent
>   over those Connections.
> 
>   Note that configuring Connection Properties and Message Properties on
>   Preconnections is preferred over setting them later.  Early
>   specification of Connection Properties allows their use as additional
>   input to the selection process.  Protocol-specific Properties, which
>   enable configuration of specialized features of a specific protocol,
>   see Section 3.2 of [I-D.ietf-taps-arch], are not used as an input to
>   the selection process, but only support configuration if the
>   respective protocol has been selected.
> 
> If you find the proposal above OK, I can PR this:
> https://github.com/thomas-fossati/taps-api-drafts/tree/s4-reflow
> 
> 
> I am not sure the following is entirely correct:
> 
>  "Calling Initiate on a Preconnection creates an outbound Connection or
>   a Listener"
> 
> since a Listener is created with a call to Listen(), isn't it?
> 
> # 4.1
> 
> In this sentence:
> 
>  "alphanumeric strings in which words may be separated by hyphens."
> 
> It is not clear what is meant by "words"?  Also, s/hyphens/hyphens and
> underscores"?
> 
> Note: wouldn't allowing only underscores as separators help uniformity
> across languages?
> 
> In this sentence:
> 
>  "[...] and are defined in an RFC."
> 
> I am not sure why being defined in an RFC is relevant here?  Maybe
> adding an example would help to understand the rationale..
> 
> The sentence:
> 
>  "Avoid using any [...]"
> 
> should probably use BCP14 language.
> 
> # 4.2
> 
> I got confused by the use of "Ignore" to mean "I have no preference, do
> whatever".  Not a real problem, just a bit of cognitive dissonance.
> 
> # 6
> 
> 5th para: why SHOULD and not MUST? Specifically, when is it OK for
> multiple remote endpoints to represent different services?
> 
> # 6.1
> 
> Maybe use 4443 for WithPort(): I guess one would normally use
> WithService("https") if they want to use the standard port for the
> service, and WithPort(...) only for alternative/non-default.
> 
> # 6.1.1
> 
> The three options:
> * Initiate() - send only
> * Listen() - receive only
> * Rendezvous() - send and receive
> are well described.
> 
> One source of confusion for me though is the very first para.  Does it
> apply to all three scenarios?  The sentence starts with the very generic
> "To use multicast [...]" so it'd seem so, but then reading the second
> para it looks like that preamble is specific to Initiate().
> 
> Could you please clarify?
> 
> # 6.1.3
> 
> Third para: It'd be probably more meaningful for:
> 
>  RemoteSpecifier.WithProtocol(QUIC)
> 
> to be
> 
>  AlternateRemoteSpecifier.WithProtocol(QUIC)
> 
> so that it chains up with the preceeding AddAlias() statement.
> 
> 
> # 6.1.4
> 
> Same comment as before with regards to WithPort().
> 
> 
> # 6.2.18
> 
> The reference to draft-ietf-taps-impl seems a bit too vague.  I have
> scanned it but couldn't find anything unambiguously applicable.
> 
> # 6.3.1
> 
> Is there a reason for not providing a user-friendly way for configuring
> X.509 verification?
> 
> # 6.3.2
> 
> The case of the trust callback does not match.
> 
> I think it should be one of:
> 
> SecurityParameters.SetTrustVerificationCallback(TrustCallback)
> 
> or 
> 
> trustCallback := NewCallback({
>  // Handle trust, return the result
> })
> 
> 
> Ditto for the challenge callback.
> 
> # 8.1.5
> 
> Is this SCTP specific or could it apply more generally?
> 
> # 8.1.10
> 
> You could use "{{Section 4.2.3 of I-D.ietf-taps-arch}}" to make the
> reference for connection contexts separation more precise: 
> 
> # 13
> 
> 5th para: not clear to me what is meant for "a protocols or paths" to be
> "raised"?  Same para: not clear what kind of "re-establishment is
> supported"?
> 
> Last para, the bit on visibility, tracing and logging:
> 
>  "Further, a Transport Services system should provide an interface to
>   poll information about which protocol and path is currently in use as
>   well as provide logging about the communication events of each
>   connection."
> 
> I reckon this is a very important point which is maybe worth BCP14 language.
> 
> # 15
> 
> I was a bit surprised that the TCP, SCTP and QUIC RFCs were not
> referenced.  But more generally, I am unsure about the criteria used to
> tell normative references from informative.  To me, BCP14 and
> draft-ietf-taps-arch are the only two normative things, the rest could
> be made informative.
> 
> 
> 
> 
> _______________________________________________
> Taps mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/taps