Subject: [Ice] Genart last call review of draft-ietf-ice-rfc5245bis-16

From: Stewart Bryant <[email-masked]>
Date: Thu, 25 Jan 2018 13:56:45 -0800

Reviewer: Stewart Bryant
Review result: Ready with Nits

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-ice-rfc5245bis-16
Reviewer: Stewart Bryant
Review Date: 2018-01-25
IETF LC End Date: 2018-01-26
IESG Telechat date: Not scheduled for a telechat

Summary: This is a well written document and I am sure it will serve its target
audience well. However Genart reviews take the perspective of someone new to
the field, and although I am sure it is probably correct and complete when
taken together with its references the learning curve is perhaps a little
steeper than it needs to be due to the extent of assumed knowledge. In the nits
section of this review I make a few simple suggestions that I think would make
it easier for the new reader.

Major issues: None

Minor issues: None

Nits/editorial comments:

   "in the XOR-RELAYED-ADDRESS attribute. "

SB> As far as I can see this not yet been defined or a reference provided in
the document.

   The table in Figure 8 illustrates an example.

SB> There is something wierd going on here.
SB> Figure 8 seems malformed possibly spread over a page break.

SB> You introduce Ta, but it would be so much kinder to the reader to give it a
real name.

SB> DSCP is not well known so needs to defined

SB> You introduce FINGERPRINT without a pointer to where it is defined

SB> The 487 error comes out of a hat without a pointer to where it is defined

SB> ICE-CONTROLLED comes out of the same hat without a pointer/definition, same
with PRIORITY, MESSAGE-INTEGRITY, ALTERNATE-SERVER, XOR_MAPPED_ADDRESS,
USE-CANDIDATE, CHECK-LIST

Section 7.3.1.4, the agent sets the nominated flag of the pair to
SB> should that be nominated or NOMINATED?

In section 8.3.1 it says: " The procedures in Section 8" which is true but
strangely self referencing

7.3.1.4.  Triggered Checks

   Next, the agent constructs a pair....

SB> Next after what? and a pair of what?

You say "Let HTO" again a user friendly name would be helpful to the new reader

Appendix B is great, particularly from section B5 onwards. It would be great to
forward reference this to help the reader understand the normative text earlier
in the document.

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 26 Jan 2018 10:33:14 +0000

Hi Stewart,

Thank You for the review! Please see inline.

> Summary: This is a well written document and I am sure it will serve its
>target audience 
> well. However Genart reviews take the perspective of someone new to the
>field, and 
> although I am sure it is probably correct and complete when taken
>together with its
> references the learning curve is perhaps a little steeper than it needs
>to be due to the 
> extent of assumed knowledge. In the nits section of this review I make a
>few simple 
> suggestions that I think would make it easier for the new reader.

Thanks! :)

...

Nits/editorial comments:

>SB> As far as I can see this not yet been defined or a reference
>SB> provided in the document.

I will add a reference to RFC 5766.

---

I¹ll check that.

---

> SB> You introduce Ta, but it would be so much kinder to the reader to
> SB> give it a real name.

Ta was defined in RFC 5245, and it's commonly used in ICE-related
discussions, so I think it would cause confusion to change the name at
this point.

---

> SB> DSCP is not well known so needs to defined

I will expand to "DiffServe Codepoint", and add a reference to RFC 2474,
on first occurrence.

---

I will add a reference to RFC 5389 on first occurrence.

---

SB> The 487 error comes out of a hat without a pointer to where it is
SB> defined

It's defined in section 16.2. I can add a reference to that section on
first occurrence.

---

>SB> ICE-CONTROLLED comes out of the same hat without a
>SB> pointer/definition,
>same
>USE->CANDIDATE, CHECK-LIST

Some are defined in section 16.1. I will a reference to that section on
first occurrence.

Some are defined in RFC 5389 and RFC 5766. I will add references on first
occurrence.

----

>SB> should that be nominated or NOMINATED?

"nominated"

---

>In section 8.3.1 it says: " The procedures in Section 8" which is true
>but strangely self referencing

The text actually references the text later in the section :) So, I
suggest to remove the first sentence:

"The procedures in Section 8 require that an ICE agent continue to
   listen for STUN requests and continue to generate triggered checks
   for a data stream, even once processing for that stream completes."

---

>SB> Next after what? and a pair of what?

"candidate pair"

I will fix it.

---

>You say "Let HTO" again a user friendly name would be helpful to the new

The name was provided by transport people that provided text. As it¹s
similar to RTO, I¹d like to keep it.

---

>Appendix B is great, particularly from section B5 onwards. It would be
>great to forward reference this to help the reader understand the
>normative text earlier in the document.

Any particular place where you would like to have the reference? In the
Introduction?

---

Regards,

Christer

---

From: Stewart Bryant <[email-masked]>
Date: Fri, 26 Jan 2018 10:47:11 +0000

Hi Christer

Thanks for your reply

A few follow-up comments inline.

> discussions, so I think it would cause confusion to change the name at

I understand. Maybe some words to explain it a little. Perhaps "the foo 
timer known in this technology as Ta" of something similar.

> similar to RTO, I¹d like to keep it.

I see what you are doing. You could however give it a name and say known 
as HTO. A name just
makes it easier to remember what it does.

> Any particular place where you would like to have the reference? In the
  Yes a heads up to to Appendix in the Intro would be useful, the a 
pointer to the
specific section when you first introduce a parameter doing something in 
the protocol.

Thanks

Stewart

---

From: Christer Holmberg <[email-masked]>
Date: Sun, 28 Jan 2018 13:28:37 +0000

Hi Steward,

Please see inline.

> I understand. Maybe some words to explain it a little. Perhaps "the foo timer known in this technology as Ta" of something similar.
>
> I see what you are doing. You could however give it a name and say known as HTO. A name just makes it easier to remember what it does.

First, I am not sure which generic timer Ta could be associated with. Ta is very ICE specific, as it controls the STUN/TURN transactions. 

Section 5.1.1.2 says:

   "The gathering process is controlled using a timer, Ta.  Every time Ta
   expires, the agent can generate another new STUN or TURN transaction."

Would it help if I added the timer descriptions to the Terminology section? Something like:

Timer Ta:	The timer for generating new STUN or TURN transactions.
Timer RTO:	The retransmission timer for a given STUN or TURN transaction.
Timer HTO:	The timeout timer for a given STUN or TURN transaction.	

---

>>> Appendix B is great, particularly from section B5 onwards. It would 
>>> be great to forward reference this to help the reader understand the 
>> Any particular place where you would like to have the reference? In the Introduction?
> Yes a heads up to to Appendix in the Intro would be useful, the a pointer to the specific section 
> when you first introduce a parameter doing something in the protocol.

What about adding the following new paragraph to the end of the Introduction.

"[REF-to-Appendix-B] provide background information and motivations regarding the design
decisions that were made when designing ICE."

Regards,

Christer

---

From: Stewart Bryant <[email-masked]>
Date: Sun, 28 Jan 2018 23:35:15 +0000

Yes, both would be excellent clarifications that would have helped me.

Thanks

Stewart

Sent from my iPad

> On 28 Jan 2018, at 13:28, Christer Holmberg <[email-masked]> wrote:
> 
> Hi Steward,
> 
> Please see inline.
> 
>> ---
>>