Subject: Re: [tsvwg] WGLC of draft-ietf-tsvwg-natsupp to CONCLUDE 22nd Aug 2020

From: Michael Tuexen <[email-masked]>
Date: Mon, 2 Nov 2020 03:22:55 +0100

> On 16. Aug 2020, at 19:41, Maksim Proshin <[email-masked]> wrote:
> 
> Hi,
> 
> Please find below my review comments. 
> I don’t have any major comments this time, only suggestions and a few typos.
Hi Maxim,

thanks for your review. See my comments in-line.

Best regards
Michael
> 
> 1.
> There are places with “must”, “should” and “may” in the text and I propose to avoid them like we did it in RFC4960bis.
Done for all non all-caps occurrences of MUST, REQUIRED, SHALL, SHOULD, RECOMMENDED, MAY, and OPTIONAL.
> 
> 2.
> Section 1:
> “ To date, specialized code for SCTP has not yet been added to most NAT functions so that only a translation of IP addresses is supported. The end result of this is that only one SCTP-capable host can successfully operate behind such a NAT function and this host can only be single-homed. The only alternative for supporting legacy NAT functions is to use UDP encapsulation as specified in [RFC6951].”
> 
> Perhaps the text above should be a separate paragraph.
Done.
> 
> 3.
> Section 3:
> “The VTag is a unique 32-bit tag that must accompany any incoming SCTP packet for this association to the Internal-Address.”
> I think it’s better to rephrase “this association” to “the current association” since it’s first time when it’s mentioned. Or maybe it’s better to say in the previous sentence that VTag is an association property.
I changed it to

The SCTP Verification Tag (VTag) (see Section 3.1 of
<xref target='RFC4960'/>) that the internal host has chosen for an association.
The VTag is a unique 32-bit tag that accompanies any incoming SCTP packet
for this association to the Internal-Address.
> 
> 4.
> Section 4.3:
> Perhaps it will be worth to rephrase “public Internet” and say simply “network” or so. I suppose the same feature can be used in other use cases like the mobile network.
Changed it to Network.
> There are a lot of other places where “Internet” is used. Not sure it’s easy to rephrase it in all places. Please consider it or maybe clarify somewhere in the beginning that it also applies to other cases and not just Internet. 
I think the IETF writes documents for the Internet. These can be used in other networks too.
So right now I use Internet in the document.
> 
> 5.
> Section 4.3, the first figure:
> What does exactly “RestartSupported” mean here? If it’s “Disable Restart” then it’s assumed that INIT has “Disable Restart” parameter included because Create() uses it. It could be worth to add it into INIT or simply remove it from Create() to avoid any confusion. Otherwise, if it means that “Disable Restart” isn’t supported, it would be better to clearly state it.
I changed the parameter to IsRestartDisabled. This better matches the text:

For the SCTP NAT processing the NAT function has to maintain a NAT binding table
of Internal-VTag, Internal-Port, Remote-VTag, Remote-Port, Internal-Address,
and whether the restart procedure is disabled or not.

Whether it is true or false depends on the INIT... It is true iff the INIT contains the DisableRestart
parameter. I guess this is now clearer.
> 
> 6.
> The figures don’t have a caption. It might be worth to add it.
Some figures do, some don't. This has been done intentionally. Packet formats don't have an
and the figures which are part of a sequence to show the handling of a packet exchange.
> 
> 7.
> Section 4.3:
> “In the case Lookup fails, the SCTP packet is dropped”
> It might be better to clarify/rephrase “Lookup fails”.
Changed to:
In the case where the Lookup function fails because it does not find an
entry, the SCTP packet is dropped.
> 8.
> Section 6.2.2:
> containign => containing
Done
> 
> 9.
> Section 6.3.1:
> “The NAT function, when processing the packet containing the INIT ACK chunk, should note in its NAT binding table that the association supports the disable restart extension.”
> It’s better to refer to a section (6.3.2?) where the disable restart extension is explained.
Not really, since this is the description of the endpoint considerations. The NAT should
only note if the restart is supported or not in this step.
> 
> BR, Maxim
> 
> -----Original Message-----
> From: tsvwg <[email-masked]> On Behalf Of Gorry Fairhurst
> Sent: Saturday, August 1, 2020 1:42 PM
> To: [email-masked]
> Subject: [tsvwg] WGLC of draft-ietf-tsvwg-natsupp to CONCLUDE 22nd Aug 2020
> 
> This email starts the WGLC for
> "Stream Control Transmission Protocol (SCTP) Network Address Translation Support".
> 
> The intended status is a Standards Track RFC.
> The current version of the document can be found at:
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-tsvwg-natsupp%2F&amp;data=02%7C01%7Cmaksim.proshin%40orioninc.com%7Cfeae23528d964a83e19d08d83607a4f1%7Cadbbbd8276e5495285313cc59f3c1fdd%7C0%7C0%7C637318753810432781&amp;sdata=%2BE%2FvZnKqN94XQWV6zsp33tIyXYywM2ZFSjgpoU6CE5o%3D&amp;reserved=0
> 
> Please review the latest version and please let us know any comments or suggestions. TSVWG needs reviews to ensure that the content is ready to move forward. Feedback supporting publication ("+1") is also very welcome.
> 
> Thanks
> 
> Gorry, on behalf of the TSVWG chairs
>

---

From: "Maksim Proshin [GMAIL]" <[email-masked]>
Date: Mon, 2 Nov 2020 10:18:45 +0300

Hi Michael,

I'm fine with all your changes and don't have any further comments. Thanks!

BR, Maxim

On Mon, Nov 2, 2020 at 5:23 AM Michael Tuexen <
[email-masked]> wrote:

> > There are places with “must”, “should” and “may” in the text and I
> propose to avoid them like we did it in RFC4960bis.
> > “ To date, specialized code for SCTP has not yet been added to most NAT
> functions so that only a translation of IP addresses is supported. The end
> result of this is that only one SCTP-capable host can successfully operate
> behind such a NAT function and this host can only be single-homed. The only
> alternative for supporting legacy NAT functions is to use UDP encapsulation
> as specified in [RFC6951].”
> >
> > “The VTag is a unique 32-bit tag that must accompany any incoming SCTP
> packet for this association to the Internal-Address.”
> > I think it’s better to rephrase “this association” to “the current
> association” since it’s first time when it’s mentioned. Or maybe it’s
> better to say in the previous sentence that VTag is an association property.
> I changed it to
>
> > Perhaps it will be worth to rephrase “public Internet” and say simply
> “network” or so. I suppose the same feature can be used in other use cases
> like the mobile network.
> Changed it to Network.
> > There are a lot of other places where “Internet” is used. Not sure it’s
> easy to rephrase it in all places. Please consider it or maybe clarify
> somewhere in the beginning that it also applies to other cases and not just
> Internet.
> > What does exactly “RestartSupported” mean here? If it’s “Disable
> Restart” then it’s assumed that INIT has “Disable Restart” parameter
> included because Create() uses it. It could be worth to add it into INIT or
> simply remove it from Create() to avoid any confusion. Otherwise, if it
> means that “Disable Restart” isn’t supported, it would be better to clearly
> state it.
> > “The NAT function, when processing the packet containing the INIT ACK
> chunk, should note in its NAT binding table that the association supports
> the disable restart extension.”
> > Please review the latest version and please let us know any comments or
> suggestions. TSVWG needs reviews to ensure that the content is ready to
> move forward. Feedback supporting publication ("+1") is also very welcome.
> >
> > Thanks
> >

-- 
BR, Maxim

---

From: Michael Tuexen <[email-masked]>
Date: Mon, 2 Nov 2020 16:38:42 +0100

> On 2. Nov 2020, at 08:18, Maksim Proshin [GMAIL] <[email-masked]> wrote:
> 
> Hi Michael,
> 
Thanks for checking and letting me know.

Best regards
Michael
> 
> BR, Maxim
> 
> On Mon, Nov 2, 2020 at 5:23 AM Michael Tuexen <[email-masked]> wrote:
> 
>