Thomas, thank you for your review and thank you all for the following discussion. I have entered a Discuss ballot for this document based on my own review. Lars > On Apr 11, 2023, at 02:18, Thomas Fossati via Datatracker <noreply@xxxxxxxx> 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. > > > > > -- > last-call mailing list > last-call@xxxxxxxx > https://www.ietf.org/mailman/listinfo/last-call
Attachment:
signature.asc
Description: Message signed with OpenPGP
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
