Subject: [Last-Call] Genart last call review of draft-ietf-core-new-block-10

From: Pete Resnick via Datatracker <[email-masked]>
Date: Sat, 24 Apr 2021 14:33:42 -0700

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. 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? 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.

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".

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".

   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.)

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.

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.

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.

---

From: John C Klensin <[email-masked]>
Date: Sat, 24 Apr 2021 18:38:39 -0400

--On Saturday, April 24, 2021 14:33 -0700 Pete Resnick via
Datatracker <[email-masked]> wrote:

> 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.
>...
 
>    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".

If the first meaning is intended, isn't that tautologically
true?  If the token used is not a token, what would it be?

   john

---

From: Pete Resnick <[email-masked]>
Date: Sat, 24 Apr 2021 18:33:35 -0500

> true?  If the token used is not a token, what would it be?

You need to put it in the context of the example give. It could be:

     The token used MUST be a token that was received in a
     request using the same Request-Tag.

which means it can't be a token with a different Request-Tag, or it 
could be:

     The token used can be any token that was received in a
     request using the same Request-Tag.

which means that there might be different tokens that use the same 
Request-Tag, but you can use any of them.

pr
-- 
Pete Resnick https://www.episteme.net/
All connections to the world are tenuous at best

---

From: John C Klensin <[email-masked]>
Date: Sat, 24 Apr 2021 20:07:08 -0400

--On Saturday, April 24, 2021 18:33 -0500 Pete Resnick
<[email-masked]> wrote:

>>> 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.
>>> ...
>> 
>>> Nits/editorial comments:
>>> 
>>> In section 4.3:
>>> 
>>> token used MUST be a token" or you mean "The token used can
>>> be any token".
>> 
> which means it can't be a token with a different Request-Tag,
> or it could be:
> 
> which means that there might be different tokens that use the
> same Request-Tag, but you can use any of them.

Ack.  Thanks.
   john

---

From: <[email-masked]>
Date: Mon, 26 Apr 2021 10:31:27 +0000

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:[email-masked]]
> Envoyé : samedi 24 avril 2021 23:34
> À : [email-masked]
> Cc : [email-masked]; [email-masked]; last-
> [email-masked]
> Objet : Genart last call review of draft-ietf-core-new-block-10
> 
> Reviewer: Pete Resnick
> 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.
> 
> 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:
> 
> 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:
> 
> 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. 

> 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.

> Either change "neither...nor" to "either or", or change "does not
> include" to "includes".

[Med] Fixed. Thanks. 

> That sort of buries a MUST requirement for the Non-confirmable case
> inside this requirement for a Response Code. I suggest instead:
> 

[Med] Ack. 

> Maybe that's obvious, since the client does the requesting, but I
> think the non-passive form is easier to read.

[Med] OK.

> 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. 

> 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.

---

From: Pete Resnick <[email-masked]>
Date: Tue, 27 Apr 2021 18:11:23 -0500

Thanks for the followup. Just to close the loop on some of these:

On 26 Apr 2021, at 5:31, [email-masked] wrote:

> [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.

Excellent. Here's a purely editorial suggestion; entirely up to you 
whether to use it:

     The missing block numbers requested by the client MUST have an
     increasing block number in each additional Q-Block2 Option with no
     duplicates.

> [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.

Ah, OK, then what you have is correct as-is.

> [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.
>

I like it.

> [Med] Changed to: "If not all of the MAX_PAYLOADS payloads were 

Perfect.

All of the others look fine. Thanks again for the quick reply.

Cheers,

pr

---

From: <[email-masked]>
Date: Wed, 28 Apr 2021 04:51:54 +0000

Hi Pete,

OK with the proposed editorial change. Fixed in the local copy.

Thank you.

Cheers,
Med

De : last-call [mailto:[email-masked]] De la part de Pete Resnick
Envoyé : mercredi 28 avril 2021 01:11
À : BOUCADAIR Mohamed TGI/OLN <[email-masked]>
Cc : [email-masked]; [email-masked]; [email-masked]; [email-masked]
Objet : Re: [Last-Call] Genart last call review of draft-ietf-core-new-block-10

On 26 Apr 2021, at 5:31, [email-masked]<mailto:[email-masked]> wrote:

In section 4.4:

Excellent. Here's a purely editorial suggestion; entirely up to you whether to use it:

duplicates.

In section 4.3:

The token used MUST be any token that was received in a request
using
the same Request-Tag.

NEW:

I like it.

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...

Perfect.

Cheers,

pr

---

From: Francesca Palombini <[email-masked]>
Date: Wed, 28 Apr 2021 09:52:31 +0000

Hi Pete,

Thanks for the thoughtful review! Good catches, and happy to see the document improved.

Authors – please go ahead and submit the update including these changes, so I can push the button and move it forward.

Thanks,
Francesca

From: "[email-masked]" <[email-masked]>
Date: Wednesday, 28 April 2021 at 06:52
To: Pete Resnick <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: RE: [Last-Call] Genart last call review of draft-ietf-core-new-block-10
Resent from: <[email-masked]>
Resent to: <[email-masked]>, <[email-masked]>, Bormann <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, Francesca Palombini <[email-masked]>
Resent date: Wednesday, 28 April 2021 at 06:52

Hi Pete,

Thank you.

Cheers,
Med

In section 4.4:

duplicates.

In section 4.3:

NEW:

I like it.

In section 7.2:

Perfect.

Cheers,

pr

--
Pete Resnick https://www.episteme.net/<https://protect2.fireeye.com/v1/url?k=4f76cc35-10edf576-4f768cae-861fcb972bfc-1147fe9a31398186&q=1&e=ede0ae9f-ec03-476d-a29a-c0628cdb9626&u=https%3A%2F%2Fwww.episteme.net%2F>
All connections to the world are tenuous at best

Thank you.