Re: [Wish] Genart last call review of draft-ietf-wish-whip-09

[Sergio Garcia Murillo <[e-mail]>]

Hi Dale,

First of all thank you very much for your thorough feedback and sorry very
much for the late reply.

I will answer the topics inline and open relevant issues on github so we
can have better tracking.

As a general comment, while reviewing the feedback gathered from the
different directorates reviews, I have found conflicting requests regarding
to both simultaneously further document behaviours and add extra
clarifications for areas that are outside their main expertise area of the
reviewers, while requesting the opposite, remove clarifications to not be
duplicated//conflicting with what is already specified elsewhere, when it
fits in their expertise domain.

So while I will try to accommodate all the feedback, I think that it is
important to align the document with the target audience and the purpose of
spec when choosing areas should be more verbose documented vs the one that
we should just keep raw references.


On Tue, Aug 8, 2023 at 5:46 PM Dale Worley via Datatracker <[e-mail]>
wrote:

> Reviewer: Dale Worley
> 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-wish-whip-09
> Reviewer:  Dale R. Worley
> Review Date:  2023-08-08
> IETF LC End Date:  2023-08-08
> IESG Telechat date:  [not known]
>
> Summary:
>
>     This draft is on the right track but has open issues, described in
>     the review.
>
> The exposition could be clarified in several places.  A few of the
> clarifications could be considered technical issues.  I list the
> issues in textual order, with an initial summary of the most important
> issues.
>
> * Summary of the major issue:
>
> A very important aspect of this document is that it defines the usage
> of "ICE via SDP via HTTP", equivalently, "supporting ICE offer/answer
> over HTTP" (as opposed to the original "ICE via SDP via SIP").  That
> definition likely will have broader usage than just WHIP.  But the
> document suffers from the "expert's disease", in that the authors
> clearly have deep knowledge of ICE and consequently only document the
> details of the ICE processing without providing any of the framework.
> This leaves naive readers to reconstruct the big picture.  I was going
> to add "As far as I can tell, all of the needed details are listed
> somewhere in section 4", but as you can see below, once I wrote an
> outline of what is needed, there are a few points that don't seem to
> be specified, at least not to the point that I recognized it.)
>

I don't think I agree with that. The document defines a signaling protocol
for establishing a WebRTC session between a server and client.
ICE is just one of the protocol layers involved (DTLS and RTP/RTCP are the
other ones).  ICE has a more visible control layer for the applications,
and that's why it has some REST operations exposed.
I think that it is fair to require some knowledge about WebRTC (and HTTP)
as a precondition for reading the draft.
Nevertheless, I have done some editorial work on the introduction and
overview section that I hope makes it more easy to read.


>
> I suggest that section 4 be organized by:
>
> - stating that it is defining "supporting ICE offer/answer
>   over HTTP" (so that it can be clearly referenced by later
>   specifications)
>
> - specifying the abstract operations involved (with the mapping to
>   specific operations listed either in this list or a later list):
>
> - - starting the ICE negotiation (via POST carrying
>     application/sdp)
>
> - - updating the ICE when trickling (via PATCH carrying
>     application/trickle-ice-sdpfrag)
>
> - - restarting ICE (via PATCH carrying (via PATCH carrying
>     application/trickle-ice-sdpfrag, but it's not clear how this is
>     distinguished, RFC 8840 does not specify it; perhaps because it
>     carries ice-ufrag/ice-pwd attributes (see section 4.1)?)
>
> - - termination (via DELETE from the client; not clear how from the
>     server)


> - specifying how WHIP constrains the ICE abstraction (Media server
>   MUST not do trickle ICE updates, not clear if Media server MAY do
>   ICE restarts, Media server MAY not support trickle ICE or ICE
>   restart, etc.)
>

I will add a small introduction about the role of ICE protocol and the
operations involved before hard jumping into the spec section.
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/147


>
> - discussion of how out-of-order requests may arise (which isn't at
>   all clear to me, as requests seem to be generated only by the
>   server, and carried by TCP, so they seem to be inherently
>   serialized)
>

HTTP requests may not be sent in the same TCP connection, so they may be
received out of order by the media server (especially if they traverse
different proxies due to load balancing).


> - discussion of how ETags MAY/MUST be handled, as that is
>   comprehensible only when one looks at the serialization needs of all
>   of the operations together
>

I have received a feedback from the HTTPDIR which seems to be in the
opposite direction:
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/141

I will try to work on this and try to find a solution that works for both.


>
> - specifying the particular features of each of the operations,
>   including any particulars of request and response fields and what
>   response codes are to be used for what error situations
>

I don't think I have fully understood this feedback.


>
> * Summary of minor issues that might have technical content:
>
> At various points in section 4 the text refers to a device sending a
> request or generating a response but in many of them, the text doesn't
> state whether the device is on the client or server end, or might be
> both.  I assume that the nature of the operation implies what devices
> might perform it based on text elsewhere, but it's hard to fully
> understand on the first reading pass.  If possible, each sentence
> should make this clear. -- Then again, if section 4 is reorganized to
> describe generic ICE/SDP/HTTP usage, attention should be paid that the
> generic usage may define how e.g. the server does an operation, even
> if WHIP servers may not do that operation.
>

I have not found a concrete example of this issue in the draft, could you
provide one?
By definition, clients send the HTTP requests and servers respond to them,
so I am a bit lost on this one.


>
> Section 4 lists a collection of 4xx and 5xx response codes to be used
> in certain situations.  Are these set due to the specific semantics of
> the codes or just to ensure that the recipient can tell what the cause
> of the error was?  (HTTP suffers from having a large number of error
> responses but all of them have vague semantics.  Unfortunately, there
> doesn't seem to be an HTTP response header that can carry codes
> defined for ICE usage specifically.)
>

We are mapping the HTTP error codes semantics to the specific errors that
we can forese for WHIP operations in order to give guidance to the client
about what the root cause of the issue could be.


>
> Section 4 requires the WHIP server to reject certain specific request
> methods, but no others.  Verify that you intend this specific
> limitation.
>
>
Yes, that is the intend, we have received some further request to clarify
the specific error codes:
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/135


> Section 4.1 references sections 6.6.2, 2.3, and 3.1 of RFC 9110.  All
> of these references appear to be incorrect, in that those sections do
> not discuss the topics at hand.  Section 4.3 references section 6.4.7
> of RFC 9110, which does not exist.  Some of these appear to be
> referencing sections in RFC 7231, which is obsoleted by RFC 9110.
>

Good catch, I updated the rfc reference, but not the sections:
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/148



>
> Section 4.1 talks about out-of-order PATCH requests but is unclear
> about what may cause them, as the immediate implication is that
> somehow the client is sending overlapping PATCH requests.  Probably
> the authors fully understand the allowed sequences of operations that
> cause this but the text needs to clarify what is being described.
>

We are not specifying any requirement on HTTP transports (i.e. they could
go over different TCP connections) so if you send two independent HTTP
requests, they may be received out of order by the server.

Section 4.1 has inconsistent statements about a WHIP resource's
> response code if the client requests an ICE restart but the resource
> does not support ICE restart.
>

This was also raised by httpdir:
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/140


>
> Section 4.2 extends RFC 8842, and this document should be marked as
> doing so.
>

I will ask for chairs advice on this.
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/151


> In Section 4.4, I notice that there's no explicit differentiation
> between the STUN and the TURN server information in returned Link
> header fields.  Please verify that either (1) the presence of username
> and credential-type differentiates the two cases, or (2) the Media
> client/server do not need to differentiate the two cases.
>

The scheme part of the url of the server differentiates between a STUN and
a TURN server:
Link: <*stun*:stun.example.net>; rel="ice-server"
Link: <*turn*:turn.example.net?transport=udp>; rel="ice-server";
           username="user"; credential="myPassword";
credential-type="password"

Section 4.7 requires POST responses to include Link headers for any
> WHIP extensions supported by the server.  Perhaps OPTIONS responses
> should be required to do so also.
>

 The Link headers not only describe the presence of the extension, but also
the URL that can be used as an entry point for that extension. Those are
WHIP Session/Resource specific and are not known when the OPTIONS request
is received.


> The designators of WHIP extensions are described as URIs.  In fact,
> they are URNs, and it would be more precise to refer to them as URNs
> in the text.
>

 Will do
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/149


> * All issues (most of them exposition/editorial), in text order:
>

I have included all the changes below in the following PR:


> 1.  Introduction
>
>    It also describes how to negotiate media flows
>    using the Offer/Answer Model with the Session Description Protocol
>    (SDP) [RFC3264] as well as the formats for data sent over the wire
>    (e.g., media types, codec parameters, and encryption).
>
> I think "as well as" should be "including".
>

Done.


>
>    RTSP [RFC7826], which is based on RTP, is
>    not compatible with the SDP offer/answer model [RFC3264].
>
> This statement might be clarified, as the naive reader (me) considers
> offer/answer to also be based on RTP.  I suspect that the critical
> meaning is "RTSP, unlike SDP offer/answer, has no provisions for
> negotiating the characteristics of the media session."
>
> Changed to:

RTSP {{?RFC7826}}, which is based on RTP, does not support the SDP
offer/answer model {{!RFC3264}} for negotiating the characteristics of the
media session.

Would that work for you?


>    This document proposes a simple protocol for supporting WebRTC as
>    media ingestion method which:
>
> I would amend to "... a simple protocol, WHIP, for ..." to have a
> clear assertion in the introduction of what WHIP is.  I would also add
> "HTTP", as the preceding text describes what WHIP *isn't* based on:
>
>    This document proposes a simple protocol, WHIP, based on HTTP, for
>    supporting WebRTC as media ingestion method which:
>
> --
>    *  Allows for ingest both in traditional media platforms and in
>       WebRTC end-to-end platforms with the lowest possible latency.
>
> Verify that "ingest" is used in the field as shorthand for "ingestion"
> (rather than being a typo).  The text seems to use both words with the
> same meaning; it should probably use only one.
>

I am not an native english speaker, so I asked ChatGPT to rewrite it, would
this be better?

"Enables ingestion on both traditional media platforms and WebRTC
end-to-end platforms, achieving the lowest possible latency."


>    *  Is usable both in web browsers and in native encoders.
>
> I would say "standalone encoders", but perhaps this is the industry
> terminology.
>

Changed.

>
> 2.  Terminology
>
> For clarity, I would move Figure 1 to after paragraph 1 of section 2
> and amend it to something like this.  Or perhaps better, relocate the
> current section 3 before section 2, so it can provide the context for
> the glossary.
>
>  +--------------+  +-------------+    +---------------+
> +--------------+
>  | Media client |  | WHIP client |    | WHIP server   |            | Media
> server |
>  +------+-------+  +--+----------+    +---------+---+-+
> +------+-------+
>         |             |                         |   |                     |
>         |             |                         | +-+-------------+       |
>         |             |                         | | WHIP resource |       |
>         |             |                         | +--------|------+       |
>         |             |                         |          |              |
>         |             |HTTP POST (SDP Offer)    |          |              |
>         |             +------------------------>+          |              |
>         |             |201 Created (SDP answer) |          |              |
>         |             +<------------------------+          |              |
>         |             |          ICE REQUEST               |              |
>         +---------------------------------------------------------------->+
>         |             |          ICE RESPONSE              |              |
>         |<----------------------------------------------------------------+
>         |             |          DTLS SETUP                |              |
>         |<===============================================================>|
>         |             |          RTP/RTCP FLOW             |              |
>         +<--------------------------------------------------------------->+
>                       | HTTP DELETE                        |
>                       +----------------------------------->|
>                       | 200 OK                             |
>                       <------------------------------------+
>
> The reason to move it before the definitions is that definitions would
> be a lot clearer with the diagram already in mind.
>

This chapter has been rewritten based on different directorate feedback, I
think it would address also your feedback as well.


>
> The "WHIP client" is separated into two components, one dealing with
> HTTP and SDP and one dealing with RTP, with both being involved in
> ICE, in the same way that the server is divided into two components
> with the same division of responsibilities.
>
> I suspect that historically, the server-side is conceptualized that
> the "Media Server" exists a priori with the "WHIP endpoint" being
> added to it as an extra unit, but the client-side is conceptualized as
> being a unitary system.  That structure makes the specification harder
> to understand, though, and I think showing the client and server with
> more parallel structures would clarify the specification.
>

Traditionally, server side components have two different components, the
signaling one, (which deals with HTTP and signaling) and the media server
(which deals with ICE/DTLS/SRTP). While they can be co-located in the same
server/daemon, they are logically different entities.


>
> The "WHIP resource" is shown as being a dependent of the "WHIP
> server".
>
> The "WHIP endpoint" is renamed "WHIP server".  At least in the SIP
> world, "endpoint" is used to refer to *both* ends of one connection,
> and so the term "WHIP endpoint" would be expected to include the WHIP
> client as well.
>

I will remove the term WHIP server, as it is not defined anywhere and use
the proper terminology in each case.


>
> The capitalization of "Media server" is made consistent with the other
> terms.  (See also the entry in the glossary.)
>

Make it lower case always.


>
> Although conceptually the left two items (Media client and WHIP
> client) and the right three items (WHIP server, WHIP resource, and
> Media server) collectively form the two end-systems of the
> client/server relationship, I have not tried to invent names for the
> two collections because there doesn't seem to be a need to reference
> them as wholes.
>
> These changes do require adjusting the terminology in the rest of the
> document.
>
> 3.  Overview
>
> See comments on section 2.
>
> 4.  Protocol Operation
>
>    The SDP offer SHOULD use the "sendonly" attribute and the SDP answer
>    MUST use the "recvonly" attribute in any case.
>
> You may want to extend this to "SHOULD use the "sendonly" attribute
> but MAY use the "sendrecv" attribute", since the other possible values
> are useless.
>
> "in any case" seems redundant given the presence of "MUST".
>

Changed to:
The SDP offer SHOULD use the "sendonly" attribute or MAY use the "sendrecv"
attribute instead, "inactive" and "recvonly" attributes MUST NOT be used.
The SDP answer MUST use the "recvonly" attribute.


>
> POST /whip/endpoint HTTP/1.1
>
> I recommend preceding each of the HTTP messages with a description of
> what the endpoints are.  E.g. "POST request from WHIP client to WHIP
> server" and "201 Created response from WHIP server to WHIP client".
>
>               Figure 2: HTTP POST doing SDP O/A example
>
> Perhaps more informative as "Example WHIP POST executing SDP
> offer/answer".
>

There is not such thing as "WHIP POST", renamed to:

"Example of SDP offer/answer exchange done via an HTTP POST"
would that work for you?


>
>    Once a session is setup, ICE consent freshness [RFC7675] SHALL be
>    used to detect non graceful disconnection and DTLS teardown for
>    session termination by either side.
>
> I think the meaning is "Once a session is set up, ICE consent
> freshness [RFC7675] MUST be maintained.  Both endpoints MUST monitor
> freshness and MUST tear down the connection if freshness is lost."
> (Note I don't know how ICE consent freshness is done, so I am assuming
> that no further details need be specified to ensure interoperability.)
>

Consent Freshness is a term defined in RFC7656 as "Maintaining and renewing
consent over time.", and is the name of the whole procedure to apply. Will
remove the ICE part as this is not defined like that anywhere.




> Probably the 2nd following paragraph ("A Media Server terminating...")
> can be combined with this paragraph.  Also, expand it to place the
> same requirement on the Media client.
>

WHIP clients and media servers do not have the same requirements on session
termination, as the media server will only perform the shutdown in abnormal
cases and the WHIP client is mandated to terminate it by explicit sending
an HTTP DELETE command.


>    The WHIP endpoints MUST return an "405 Method Not Allowed" response
>    for any HTTP GET, HEAD or PUT requests on the endpoint URL in order
>    to reserve its usage for future versions of this protocol
>    specification.
>
> This and the 2nd following paragraph only forbid a few methods, and
> allow an implementation to implement any others as extensions.  Please
> verify that these are exactly what you need.  Also, the wording should
> be "reserve their usage" because the referent is "any ... requests".
>

 I have changed the wording to exclude all methods except the ones defined
in the document for each URL

The WHIP endpoints MUST return an "405 Method Not Allowed" response for any
HTTP request method different than OPTIONS and POST on the endpoint URL in
order to reserve their usage for future versions of this protocol
specification.

The WHIP sessions MUST return an "405 Method Not Allowed" response for any
HTTP request method different than PATCH and DELETE on the session URLs in
order to reserve their usage for future versions of this protocol
specification.


>    The WHIP endpoints MUST support OPTIONS requests for Cross-Origin
>    Resource Sharing (CORS) as defined in [FETCH] and it SHOULD include
>    an "Accept-Post" header with a mime type value of "application/sdp"
>    on the "200 OK" response to any OPTIONS request received as per
>    [W3C.REC-ldp-20150226].
>
> I think this would be less awkward as
>
>    The WHIP endpoints MUST support OPTIONS requests for Cross-Origin
>    Resource Sharing (CORS) as defined in [FETCH].  The "200 OK"
>    response to any OPTIONS request SHOULD include an "Accept-Post"
>    header with a mime type value of "application/sdp" as per
>    [W3C.REC-ldp-20150226].
>

Changed.


>
> 4.1.  ICE and NAT support
>
> Text might be added to the beginning of this section similar to the
> text at the beginning of section 4.2, as WHIP puts specific
> restrictions on ICE usage for simplicity's sake.  It seems like "NAT"
> should be replaced by something else, as "NAT" is not mentioned
> elsewhere in the document.  "STUN/TURN" seems to be natural, but there
> is a section for that (4.4).  Perhaps this section should be titled
> "ICE usage".
>

Changed title to ICE support, I will add an introductory paragraph talking
about ICE and NAT.


> Also, several subsections of section 4 seem to be about usage and
> restrictions of various protocols; perhaps they should have parallel
> names e.g. "ICE usage", "WebRTC usage", "STUN/TURN usage".
>

I am not sure if I like that better, will review it one we have the full
editorial changes merged.


>
>    In order to simplify the protocol, there is no support for exchanging
>    gathered trickle candidates from Media Server ICE candidates once the
>    SDP answer is sent.
>
> I think this would be clearer as follows, since that moves the fact we
> are talking about the Media server earlier in the sentence:
>
>    In order to simplify the protocol, there is no support for the
>    Media server sending further trickle candidates once the SDP answer
>    is sent.
>

I have simplified it further to:

In order to simplify the protocol, exchanging media server's gathered ICE
candidates after sending the SDP answer is not supported.


>
> --
>
>    The WHIP client MAY perform trickle ICE or ICE restarts as per
>    [RFC8838] by sending an HTTP PATCH request ...
>
> PATCH is an unusual method, defined in RFC 5789, and it seems like
> there should be a reference for it here.
>

Added


>
> Also, you probably want to insert here a statement whether the WHIP
> resource MAY *initiate* ICE restarts; as far as I can tell, the text
> does not address this.
>
> Also, you want to directly state that the semantics (meaning) of an
> ICE update/restart request and thus how it is to be processed, is
> determined by the body, as specified in RFC 8840.  The current text
> leaves that silently implied.
>

Will do that as part of
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/147


>    If the WHIP resource supports either Trickle ICE or ICE restarts, but
>    not both, it MUST return a "405 Not Implemented" response for the
>    HTTP PATCH requests that are not supported.
>
>    If the WHIP resource does not support the PATCH method for any
>    purpose, it MUST return a "501 Not Implemented" response, as
>    described in [RFC9110] Section 6.6.2.
>
> The canonical description for "405" is "Method not allowed".  But I
> would reorganize and combine these paragraphs along these lines:
>
>    If the WHIP resource supports either Trickle ICE or ICE restarts,
>    but not both, it MUST return a "405 Method not allowed" response
>    for HTTP PATCH requests for the feature that is not supported.  If
>    neither feature is supported, it MUST return a "501 Not
>    Implemented" response for such HTTP PATCH requests.
>
> This allows PATCH requests for other purposes, if any of those are
> ever defined.


Will do it alongside
https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/135




> BTW, the reference "[RFC9110] Section 6.6.2" ("Trailer") seems to be
> unrelated to the text; please verify it.
>

Yes, I updated the RFC reference, but not the section, should be 15.6.2


>
>    As the HTTP PATCH request sent by a WHIP client may be received out-
>    of-order by the WHIP resource, ...
>
> To clarify this, I would expand it to:
>
>    The WHIP client MAY send overlapping HTTP PATCH requests to one
>    WHIP resource.  As the HTTP PATCH request sent by a WHIP client may
>    be received out-of-order by the WHIP resource, ...
>

Reworded to:
The WHIP client MAY send overlapping HTTP PATCH requests to one WHIP
session. Consequently, as those HTTP PATCH requests may be received
out-of-order by the WHIP session, the WHIP session...

>
> However it is unclear to me why the client would want to do this.
> Perhaps the concept is that the server may also be sending PATCH
> requests for trickle ICE etc.?  In that case, you would want to phrase
> it more like:
>
>    As the WHIP client and the WHIP resource may be both be sending
>    HTTP PATCH requests without coordination, an HTTP PATCH request
>    sent by a WHIP client may be received out-of-order by the WHIP
>    resource, ...


Only the WHIP client sends HTTP PATCH requests to the WHIP server.


>
> In any case, you should ensure you understand the possible
> complications and explain them here to the reader.  Also, the final
> sentence of the paragraph is:
>
>    Note that including the ETag
>    in the original "201 Created" response is only REQUIRED if the WHIP
>    resource supports ICE restarts and OPTIONAL otherwise.
>
> I *think* this means that the entirety of the preceding paragraph is
> conditional "If the WHIP resource supports ICE restarts, the following
> complications are possible ... and so the WHIP resource must return
> ETag header fields ...".  Whatever the causation/conditions are, they
> should be put at the start of the paragraph.
>
> Added to https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/141


>    A WHIP client sending a PATCH request for performing trickle ICE MUST
>    include an "If-Match" header field with the latest known entity-tag
>    as per [RFC9110] Section 3.1.  When the PATCH request is received by
>    the WHIP resource, it MUST compare the indicated entity-tag value
>    with the current entity-tag of the resource as per [RFC9110]
>    Section 3.1 and return a "412 Precondition Failed" response if they
>    do not match.
>
> Beware that this paragraph appears to be a continuation of the
> preceding paragraph, in that the sentence "Note ..." appears to be a
> condition on it also.  If that is true, when you relocate the
> condition to before the first paragraph, the condition should itself
> be paragraph-level, so it is clear that its scope is not ended by the
> end of the first paragraph.
>

Made the conditonal requirement happen on the first phase instead.


>
>    A WHIP resource receiving a PATCH request with new ICE candidates,
>    but which does not perform an ICE restart, MUST return a "204 No
>    Content" response without body.
>
> According to the previous text, if a WHIP resource receives a PATCH
> requesting an ICE restart but it does not implement ICE restart, it
> MUST return either 405 or 501.  This needs to be clarified; perhaps
> there is a difference between "does not support" and "does not
> perform"?
>

The "does not perform an ICE restart" is about the PATCH request, will
clarify it.


>
>    If the Media Server does not support
>    a candidate transport or is not able to resolve the connection
>    address, it MUST accept the HTTP request with the "204 No Content"
>    response and silently discard the candidate.
>
> Note that the Media server does not generate HTTP responses in any
> case.  I assume the meaning is "the WHIP resource must accept ...".  I
> think the intended meaning is
>

Right, changed.


>
>    If a WHIP resource receives a PATCH request containing a new ICE
>    candidate with a transport that the Media Server does not support
>    or is unable to resolve the address, the resource must MUST
>    silently discard the candidate and process the remainder of the
>    request.  Typically, it will accept the HTTP request with the "204
>    No Content" response.
>
> The last sentence is written as it is because the resource may have
> some other reason for rejecting the PATCH.
>

Changed to this instead:
it MUST silently discard the candidate and continue processing the rest of
the request normally.


>
>    A WHIP client sending a PATCH request for performing ICE restart MUST
>    contain an "If-Match" header field with a field-value "*" as per
>    [RFC9110] Section 3.1.
>
> Naively, this seems questionable, as presumably all ICE operations
> need to be serialized.  Or is it that ICE restart causes all preceding
> ICE updates to become irrelevant?  Again, some explanation of the
> allowed sequencing of ICE operations and the necessary
> serialization/dependencies would be good introduction.
>

ICE restarts reset any previous state, so all pending UPDATES will be
irrelevant.

>
>    Also, the "200 OK" response for a successful ICE restart MUST contain
>    the new entity-tag corresponding to the new ICE session in an ETag
>    response header field and MAY contain a new set of ICE candidates for
>    the Media Server.
>
> This reads oddly because it combines "new ICE session" with the
> possibility that the triggering PATCH request may not include new ICE
> candidates.  What is the implied processing if there are no
> candidates?  There are two possibilities, one that the new ICE session
> starts with no candidates (in that direction), the other is that the
> previously specified set of candidates is retained.  The latter
> possibility implies that the new session depends on the previous
> session and introduces serialization requirements.
>

The new ICE session can be started without candidates from the WHIP client.
They may be sent in an follow up PATCH request, or could be not needed/sent
at all.


>
>    If the ICE request cannot be satisfied by the WHIP resource, the
>    resource MUST return an appropriate HTTP error code and MUST NOT
>    terminate the session immediately.
>
> You probably mean "If the ICE update or restart request ...".  Also,
> you probably mean "... and MUST continue the previous ICE session."
>

 It is only about ICE restarts, added it.


>    In either case, the session MUST be
>    terminated if the ICE consent expires as a consequence of the failed
>    ICE restart as per [RFC7675] Section 5.1.
>
> Probably intensify to "In any case, ...".
>

Changed


>
>    Because the WHIP client needs to know the entity-tag associated with
>    the ICE session in order to send new ICE candidates, it MUST buffer
>    any gathered candidates before it receives the HTTP response to the
>    initial POST request or the PATCH request with the new entity-tag
>    value.
>
> I think this could be clarified by putting more of the sentence in
> time order:
>
>    Because the WHIP client needs to know the entity-tag associated
>    with the ICE session in order to send a PATCH containing new ICE
>    candidates, it MUST wait until it receives the HTTP response to the
>    initial POST request or the previous PATCH request before it can
>    send any candidates that were gathered after the previous request
>    was sent.
>
> --
>

changed to:
Because the WHIP client needs to know the entity-tag associated with the
ICE session in order to send a PATCH request containing new ICE candidates,
it MUST wait and buffer any gathered candidates until it receives the HTTP
response with the new entity-tag value to either initial POST request or
the last PATCH request performing an ICE restart.


>
>    ... the STUN requests will contain invalid ICE information and will
>    be rejected by the server.  When this situation is detected by the
>    WHIP Client, it MUST ...
>
> Given that this is a MUST, we need a more rigid specification of the
> specific responses that compel this behavior in the client.  Also,
> "the server" is used, but apparently in the sense "the STUN server",
> not "the Media server" as elsewhere in the text.  Also, it seems to be
> implied that the Media server will not do ICE restart if it receives
> these errors from the STUN server.  Is that true?
>

Changed to:
If there is a mismatch between the ICE information at the WHIP client and
at the WHIP session (because of an out-of-order request), the STUN requests
will contain invalid ICE information and will be dropped by the receiving
side. If this situation is detected by the WHIP Client, it MUST send a new
ICE restart request to the server.


>
> 4.2.  WebRTC constraints
>
>    In the specific case of media ingestion into a streaming service,
>
> Given that the abstract says "ingestion of content into streaming
> services and/or CDNs", this appears to confine the paragraph to
> discussing a subset of WHIP implementations.  This clause should
> probably be revised to clarify that it covers all WHIP usages.
>

Removed the whole phrase as the draft-ietf-rtcweb-gateways-02 is only
relevant for adding ICE lite support, which we already done in previous
sections.

>
>    Both the WHIP client and the WHIP endpoint SHALL use SDP bundle
>    [RFC9143].  ...
>
> Please verify that all proper specifications are both "MUST support"
> and "MUST use".  E.g., there is "... will use RTP/RTCP multiplexing
> ..." which should say "SHALL"/"MUST".
>

Yes, it is MUST support and use, added clarification


>
>    ... bundled "m=" sections as per [RFC8858] i.
>
> The ending "i" seems like a typo, but please verify it isn't the
> truncation of intended text.
>

Removed

>
>    ... in a single MediaStream as defined in [[!RFC8830]] ...
>
> It appears that RFC 8830 is an intended reference but it is not listed
> in section 8.
>

fixed

>
>    When a WHIP client sends an SDP offer, it SHOULD insert an SDP
>    "setup" attribute with an "actpass" attribute value, as defined in
>    [RFC8842].  However, if the WHIP client only implements the DTLS
>    client role, it MAY use an SDP "setup" attribute with an "active"
>    attribute value.  If the WHIP endpoint does not support an SDP offer
>    with an SDP "setup" attribute with an "active" attribute value, it
>    SHOULD reject the request with a "422 Unprocessable Entity" response.
>
>    NOTE: [RFC8842] defines that the offerer must insert an SDP "setup"
>    attribute with an "actpass" attribute value.  However, the WHIP
>    client will always communicate with a Media Server that is expected
>    to support the DTLS server role, in which case the client might
>    choose to only implement support for the DTLS client role.
>
> Note this means that this document updates RFC 8842, and should be
> marked as such.
>

I will ask for chairs advice on this.


>
> This part seems to be incomplete or inconsistent.  Clearly, a WHIP
> server MUST support the DTLS server role, and so it MUST be able to
> accept "a=setup:active".  This means that the described 422 error
> response should never occur.
>

The media server must support the DTLS server role, but the WHIP endpoint
may not be able to parse the SDP containing an "setup:active" as the
standard is "setup:actpass".



> 4.3.  Load balancing and redirections
>
>    WHIP endpoints and Media Servers might not be colocated on the same
>    server, so it is possible to load balance incoming requests to
>    different Media Servers.
>
> In stead of the passive "it is possible ...", it would be clearer to
> say "the WHIP server may load balance incoming requests to multiple
> Media servers."
>

Being pedantic, the redirection could (and most probably will happen), in a
network layer before the WHIP endpoints (nginx reverse proxies for
example). We don't have the term "service provider" used before, so I think
that the passive voice is appropriate.



>    WHIP clients SHALL support HTTP redirection
>    via the "307 Temporary Redirect" response as described in [RFC9110]
>    Section 6.4.7.
>
> RFC 9110 does not contain a section 6.4.7.
>

  Updated section reference to 15.4.8.

>
> 4.4.  STUN/TURN server configuration
>
>    A reference to each STUN/TURN server will be returned using the
>    "Link" header field [RFC8288] with a "rel" attribute value of "ice-
>    server".
>
> I notice that there's no explicit differentiation between the STUN and
> the TURN server information.  Please verify that either (1) the
> presence of username and credential-type differentiates the two cases,
> or (2) the Media client/server do not need to differentiate the two
> cases.
>

As explained above, the differentiation is done by the url scheme "turn:"
vs "stun:"


>
>               Figure 5: Example ICE server configuration
>
> It seems the caption should be "Example STUN/TURN server
> configuration".
>

Changed to align to the section title.


>    The generation of the TURN server credentials may require performing
>    a request to an external provider, which can both add latency to the
>    OPTIONS request processing and increase the processing required to
>    handle that request.  In order to prevent this, the WHIP Endpoint
>    SHOULD NOT return the STUN/TURN server configuration if the OPTIONS
>    request is a preflight request for CORS, that is, if The OPTIONS
>    request does not contain an Access-Control-Request-Method with "POST"
>    value and the the Access-Control-Request-Headers HTTP header does not
>    contain the "Link" value.
>
> I think the normative structure can be made clearer as:
>
>    In order to avoid adding latency and processing to an OPTIONS
>    request, the WHIP server SHOULD NOT return return the STUN/TURN
>    server configuration in OPTIONS responses unless either (1) the
>    configuration can be determined without extra processing or latency
>    (e.g. by performing a request to an external provider), or (2) if
>    the OPTIONS request is an XXX, and contains an
>    Access-Control-Request-Method with "POST" value and the the
>    Access-Control-Request-Headers HTTP header contains the "Link"
>    value.
>
> "is an XXX" optional and is to clarify the situation that exception
> (2) supports.  From the current text, I think it should be "is not a
> preflight request for CORS".  If "preflight request for CORS" is
> retained, please add a reference to "[FETCH]" here, as the only other
> mention of CORS in the document which has the reference to "[FETCH]"
> is a long way away.
>
> The OPTIONS request for stun/turn server configuration is heavily
discouraged, so I would prefer not to change the mandatory language. I have
added the fetch reference.



>    It might be also possible to configure the STUN/TURN server URIs with
>    long term credentials provided by either the broadcasting service or
>    an external TURN provider on the WHIP client, overriding the values
>    provided by the WHIP endpoint.
>
> This should start with that it is the WHIP client that is being
> configured:
>
>    The WHIP client MAY be configured with the STUN/TURN server URIs
>    and long term credentials provided by either the broadcasting
>    service or an external TURN provider, overriding the values
>    provided by the WHIP endpoint.
>

I have rephrased as follows:
The WHIP clients MAY also support configuring the STUN/TURN server URIs
with long term credentials provided by either the broadcasting service or
an external TURN provider, overriding the values provided by the WHIP
endpoint.


>
> 4.7.  Protocol extensions
>
>    The WHIP endpoint MUST
>    return one "Link" header field for each extension, with the extension
>    "rel" type attribute and the URI for the HTTP resource that will be
>    available for receiving requests related to that extension.
>
> This seems awkward to me.  Perhaps
>
>    The WHIP endpoint MUST return one "Link" header field for each
>    extension that it supports, with the "rel" attribute having the
>    extension URN as its value and the URI being suitable for that
>    extension.
>
> The part "the URI being suitable for that extension" recognizes that
> we can't predict what the semantics of the URI will be for a future
> extension, but the extension will define the semantics.
>

The protocol extensions must be HTTP based, so  I don't think that the URI
part is needed. Reworded the rest as:
The WHIP endpoint MUST return one "Link" header field for each extension
that it supports, with the extension "rel" attribute value containing the
extension URN and the URI for the HTTP resource that will be available for
receiving requests related to that extension.

>
>    Protocol extensions supported by the WHIP server MUST be advertised
>    to the WHIP client in the "201 Created" response to the initial HTTP
>    POST request sent to the WHIP endpoint.
>
> It seems like a good idea to require protocol extension support
> declaration in OPTIONS responses as well.
>
As stated above, the extensions URIs are only known when the session is
created, so they are not available to OPTIONS requests.


>
> 5.  Security Considerations
>
> The writers should do a spelling-check pass over section 5, as there
> are enough misspellings to inconvenience the Editor.
>
>    On top of that, the WHIP protocol exposes a thin new attack surface
>    specific [sic] of the REST API methods used within it:
>

Yes, it was already risen and reviewed.


>
> It seems like the following three items could be condensed, as most of
> the text enumerates problems that are well-known or clear from the
> context.  Perhaps something like:
>
>    * HTTP flooding and resource exhaustion:  Both the WHIP server and
>      the WHIP resources SHOULD implement a rate limit and avalanche
>      control mechanism for incoming requests.
>
>    * Security of WHIP resource URLs:  Servers for WHIP resource URLs
>      SHOULD either implement authentication and authorization
>      similarly to WHIP servers (see section 4.5) or use URLs that are
>      capabilities, in that they are unguessable and the possession of
>      the URL shows the client has the right to access the resource.
>
> IIRC we received feedback on the opposite direction about being more
verbose about the security considerations, would prefer to leave it as is.


> 6.1.  Link Relation Type: ice-server
>
>    The link relation type below has been registered by IANA per
>    Section 4.2 of [RFC8288].
>
> I do not see "ice-server" in
> https://www.iana.org/assignments/link-relations/link-relations.xhtml,
> so it appears the wording should be "IANA is asked to register ...".


> 6.3.1.  Specification Template
>
>    *  The designated contact shall be responsible for reviewing and
>       enforcing uniqueness.
>
> Should this be "designated expert"?  Compare with sections 6.4.1 and
> 6.4.2.


Both texts above has been revised by IANA already, would prefer not to
change without their input.


>
> 6.4.1.  Registration Procedure
>
>    The RFC SHOULD include any attributes defined.
>
> If this means what I think it does, it should be expanded to something
> like
>
>    The RFC MUST include the syntax and semantics of any
>    extension-specific attributes that may be provided in a Link header
>    field advertising the extension.
>
> Changed


> [END]
>

This was intense... 😅

Thank you once again for your thorough review.
Sergio