Re: [core] Genart last call review of draft-ietf-core-new-block-10

[[e-mail]]

Hi Pete, 

Thank you for the review. 

The changes to address your review can be tracked here: https://tinyurl.com/new-block-latest. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Pete Resnick via Datatracker [mailto:[e-mail]]
> Envoyé : samedi 24 avril 2021 23:34
> À : [e-mail]
> Cc : [e-mail]; [e-mail]; last-
> [e-mail]
> Objet : Genart last call review of draft-ietf-core-new-block-10
> 
> Reviewer: Pete Resnick
> 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-core-new-block-10
> Reviewer: Pete Resnick
> Review Date: 2021-04-24
> IETF LC End Date: 2021-04-28
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary:
> 
> The document looks pretty solid to me. There is one item I marked as
> a minor "issue", but it may simply be an editorial item that confused
> me; I figured I'd call it an issue just in case so it doesn't get
> left to the last minute to look at.
> 
> Do note that I have not reviewed the examples for correctness; I
> simply don't have the expertise to be convinced I'd do it right.
> 
> Major issues:
> 
> None.
> 
> Minor issues:
> 
> In section 4.4:
> 
> I find this paragraph confusing:
> 
>    The requested missing block numbers MUST have an increasing block
>    number in each additional Q-Block2 Option with no duplicates.  The
>    server SHOULD respond with a 4.00 (Bad Request) to requests not
>    adhering to this behavior.
> 
> So, given the SHOULD in the second sentence, it appears that the MUST
> in the first sentence doesn't apply to the server (i.e., to enforce
> this), but rather to the client doing the request. You should
> probably say it that way.

[Med] Yes. This text should be linked to the previous paragraph when the client issues the request for missing blocks. Anyway, I agree it is better to be explicit here. Fixed.

 Also, the SHOULD in the second sentence is
> not entirely clear to me: Are you saying that the server can choose
> to use some other response code, or are you saying that the server
> can accept the request and do something interesting with it?

[Med] The latter. Normally the server must discard such request but given that one of the target use cases for this spec is DDoS mitigation, servers may be tolerant.

 Below is
> an attempt to fix it, but might not be correct depending on what you
> mean:
> 
>    The client MUST use an increasing block number in each additional
>    Q-Block2 Option when requesting missing block numbers, and MUST
>    request no duplicates.  The server MUST reject requests  not
> adhering
>    to this behavior and SHOULD respond with a 4.00 (Bad Request) to
> such
>    requests.
> 
> There are other places in the document that use MUST with regard to
> what needs to be in a piece of data (see for example sections 4.5 and
> 4.6), but don't make it clear who is responsible for enforcing that
> MUST (the client or the server).
> You should read through the entire document for MUSTs (or SHOULDs)
> like that and make sure it's clear from the context.

[Med] Will double check and tweak the text when required. Thanks. 

> 
> Nits/editorial comments:
> 
> In section 4.3:
> 
> In several response code definitions:
> 
>    The token used MUST be any token that was received in a request
> using
>    the same Request-Tag.
> 
> That doesn't really parse well. I think you either mean "The token
> used MUST be a token" or you mean "The token used can be any token".
> 

[Med] The second para of this section specifies that all block requests must use the same Request-Tag. The 4th para indicates that each of these block requests will use a new token. The server must use one of these tokens when replying. 

Updated the text as follows: 

NEW: 
   The token used MUST be one of the tokens that were received in a request for this block-wise exchange.

> Specific response codes:
> 
>    4.00 (Bad Request)
> 
>       This Response Code MUST be returned if the request does not
>       include neither a Request-Tag Option nor a Size1 Option but
> does
>       include a Q-Block1 option.
> 
> Either change "neither...nor" to "either or", or change "does not
> include" to "includes".

[Med] Fixed. Thanks. 

> 
>    4.02 (Bad Option)
> 
>       Either this Response Code (in case of Confirmable request) or a
>       reset message (in case of Non-confirmable request) MUST be
>       returned if the server does not support the Q-Block Options.
> 
> That sort of buries a MUST requirement for the Non-confirmable case
> inside this requirement for a Response Code. I suggest instead:
> 
>       This Response Code MUST be returned for a Confirmable request
> if
>       the server does not support the Q-Block Options. (A reset
> message
>       is sent in case of Non-confirmable request.)

[Med] Ack. 

> 
> In section 4.4:
> 
> The passive here is not great form, particularly because it doesn't
> name the
> actor:
> 
>    It is permissible to set the M bit to request this...
> 
> How about instead:
> 
>    The client MAY set the M bit to request this...
> 
> Maybe that's obvious, since the client does the requesting, but I
> think the non-passive form is easier to read.

[Med] OK.

> 
> In the second to last paragraph:
> 
>    If the server transmits a new body of data (e.g., a triggered
> Observe
>    notification) with a new ETag to the same client as an additional
>    response, the client should remove any partially received body
> held
>    for a previous ETag for that resource as it is unlikely the
> missing
>    blocks can be retrieved.
> 
> I'm ambivalent about whether that "should" ought to be uppercased,
> but I just wanted to make sure you intended the lowercase.

[Med] That was on purpose. We wanted to leave it to the implementation, e.g., logging or application-specific policies. 

> 
> In section 7.2:
> 
>    For the server receiving NON Q-Block1 requests, it SHOULD send
> back a
>    2.31 (Continue) Response Code on receipt of all of the
> MAX_PAYLOADS
>    payloads to prevent the client unnecessarily delaying.
> Otherwise...
> 
> When you say "Otherwise", Do you mean, "For other payloads"? Either
> way, you should probably change that to make it clear.
> 
> 
[Med] Changed to: "If not all of the MAX_PAYLOADS payloads were received, ..."


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.