Re: [Gen-art] Gen-ART Last Call review of draft-ietf-opsawg-capwap-alt-tunnel-10

[Paul Kyzivat <[e-mail]>]

On 12/13/17 3:56 AM, Duzongpeng wrote:
> Hi, Paul
> 
> 	Please see inline.
> 	Thank you very much for your careful review.
> 	We have updated the draft accordingly.
> 	If any problem, please connect us.
> 
> Best Regards
> Zongpeng Du
> 
> -----Original Message-----
> From: Paul Kyzivat [mailto:[e-mail]]
> Sent: Tuesday, December 12, 2017 3:48 AM
> To: [e-mail]
> Cc: General Area Review Team
> Subject: Gen-ART Last Call review of draft-ietf-opsawg-capwap-alt-tunnel-10
> 
> 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 <​http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
> 
> Document: draft-ietf-opsawg-capwap-alt-tunnel-10
> Reviewer: Paul Kyzivat
> Review Date: 2017-12-11
> IETF LC End Date: 2017-12-13
> IESG Telechat date: TBD
> 
> Summary:
> 
> This draft is on the right track but has open issues, described in the review.
> 
> (Thanks for fixing most of the issues I raised in the previous round.)
> 
> Issues:
> 
> Major: 0
> Minor: 7
> Nits:  1
> 
> (1) MINOR:
> 
> In Section 1.3, if this document is intended to serve as a *historical* reference, then why isn't then intended status "Historic" rather than "Experimental"?
> <zongpeng>There have been discussions about it among the authors, chairs, and the ADs. Finally, the "Experimental" type is decided.
> According to https://tools.ietf.org/html/rfc2026#section-4.2, the historical type means:
>     A specification that has been superseded by a more recent
>     specification or is for any other reason considered to be obsolete is
>     assigned to the "Historic" level.
> Our document is a new one, so it is not very proper for us to declare a historical type.
> 
> 	Also in RFC2026, it is said that
>     The "Experimental" designation typically denotes a specification that
>     is part of some research or development effort.  Such a specification
>     is published for the general information of the Internet technical
>     community and as an archival record of the work, subject only to
>     editorial considerations and to verification that there has been
>     adequate coordination with the standards process (see below).
> 
> 	So we consider that the "Experimental" type is more suitable here.
> 
> 	And to avoid ambiguity, we have changed the "This experimental document is intended to serve as a historical reference for any future work as to the operational and deployment requirements." To
> 	"This experimental document is intended to serve as **an archival record** for any future work as to the operational and deployment requirements."
> </zongpeng>

Now I am more confused. This is new, rather than documenting existing 
deployed practice. It is not standards track, so this is not an intent 
to define something that can be deployed But it is being published as an 
archive.

Was this once intended to be standards track, but without sufficient 
interest or support to complete it as a standard. Is this then 
reflecting that "we did a lot of work on this and want to publish it in 
case there is future interest in doing something like this"?

If so, that is fine. If it is something else, then it would be helpful 
to have further explanation.

>   (2) MINOR:
> 
> Section 3 contains:
> 
>      Since AC can configure a WTP with more than one AR available for the
>      WTP to establish the data tunnel(s) for user traffic, it may be
>      useful for the WTP to communicate the selected AR.  To enable this,
>      the IEEE 802.11 WLAN Configuration Response may contain the AR list
>      element containing the selected AR.
> 
> But "IEEE 802.11 WLAN Configuration Response" is not defined in this version of the document. Seems like this may be a dangling reference from a prior version.
> <zongpeng>Thanks for proposing the problem. We add some explanations for the problem.
> Firstly, change the sentence to " the IEEE 802.11 WLAN Configuration Response may contain the AR list
>      element containing the selected AR *as shown in Figure 5*."
> Secondly, change the Config. To Configuration in the Figure 5, and add the [ AR List Element ]  in the IEEE 802.11 WLAN Configuration Response message.</zongpeng>

I'm still confused about what message is used to convey this. Is it an 
existing message in another spec, in which the AR List Element may be 
inserted? If so, does that message already allow elements defined 
elsewhere, such as this one, to be included? How would it be deciphered?

> (3) MINOR:
> 
> In Section 3.1, Figure 6 shows Tunnel-Type1 occupying the first 16 bits of a 32 bit value, and Tunnel-Type (2..N) all occupying the 2nd 16 bits of that value. That doesn't work for N>2. I *presume* that the intent is that this be an array of 16 bit values in network order starting with Tunnel-Type1, but that is not what it says. Needs some work.
> 
> <zongpeng>Thanks for proposing the problem. We add some explanations for the problem.
> Firstly, add some explanations into the Tunnel-type Field.
> Tunnel-Type: This is identified by value defined in Section 3.2. *There may be one or more Tunnel-Types as shows in Figure 6.*
> Secondly, change the graph to:
> 	0                   1                   2                   3
>   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> |      Tunnel-Type 1            |      Tunnel-Type 2            |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> |            ...                |      Tunnel-Type N            |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> </zongpeng>

Looks good.

> (4) MINOR:
> 
> In Section 3.3 I find the wording of the usage unclear in the following:
> 
>      The Alternate Tunnel Failure Indication message element is sent by
>      the WTP to inform the AC about the status of the Alternate Tunnel.
>      It MAY be included in the CAPWAP Station Configuration Request
>      message.  ...
> 
> It is the way "MAY" is used here that causes me confusion, as if there is some other way to achieve this goal. Perhaps the following would be
> clearer:
> 
>      The WTP MAY include the Alternate Tunnel Failure Indication message
>      in a CAPWAP Station Configuration Request message to inform the AC
>      about the status of the Alternate Tunnel.
> 
> <zongpeng> Thanks for proposing the problem. We revise accordingly. </zongpeng>

Thanks.

> (5) MINOR:
> 
> In Section 4.2 I find the usage of the term "Access Router (LMA) Information Element" confusing. I find no definition of this as a distinct thing, so I gather the intent is that this is a particular usage of "Access Router Information Element". I think this would be clearer to remove "(LMA)" from Figure 10.
> 
> <zongpeng> Thanks for proposing the problem. We revise accordingly. </zongpeng>

Looks good.

> (6) MINOR:
> 
> Section 5.2 uses "ARs" and "ARs information" in ways that are unclear and improper grammar. IIUC "AR" in this document means "Access Router", and so "ARs" should mean "Access Routers". It is used that way once in section 3.3, and once in 5.2. But several other usages in 5.2 are different, and seem to be intended to refer to "Access Router Information Elements". I suggest the following change:
> 
> OLD
> 
>      ... If there are more than one ARs
>      information provided by the AC for reliability reasons, the same
>      Tunnel DTLS Policy (see Figure 14) is generally applied for all
>      tunnels associated with the ARs.  Otherwise, Tunnel DTLS Policy MUST
>      be bonding together with each of the ARs, then WTP will enforce the
>      independent tunnel DTLS policy for each tunnel with a specific AR.
> 
> NEW
> 
>      ... If, for reliability reasons, the AC has provided more than one
>      AR address in the Access Router Information Element, the same
>      Tunnel DTLS Policy (see Figure 14) is generally applied for all
>      tunnels associated with those ARs.  Otherwise, Tunnel DTLS Policy
>      MUST be bonded together with each of the Access Router Information
>      Elements, and the WTP will enforce the independent tunnel DTLS policy
>      for each tunnel with a specific AR.
> 
> In addition the mechanics of this "bonding" aren't entirely clear. This seems to be covered by:
> 
>      A: If A bit is set, there is an AR information associated with the
>      DTLS policy.  There may be an array of pairs binding DTLS policy
>      information and AR information contained in the Tunnel DTLS Policy
>      Element.  Otherwise, the same Tunnel DTLS Policy (see Figure 14) is
>      generally applied for all tunnels associated with the ARs configured
>      by the AC.
> 
> The above says "There may be an array of pairs". How is the array encoded and how is its length specified? I'm guessing you intend:
> 
> When A=0:
> 
>         0                   1                   2                   3
>         0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |Tunnel DTLS Policy Element Type|        Length                 |
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |                        Reserved                       |A|D|C|R|
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> When A=1:
> 
>         0                   1                   2                   3
>         0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |Tunnel DTLS Policy Element Type|        Length                 |
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |                        Reserved                       |A|D|C|R|
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        .                       AR Information                          .
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |                        Reserved                       |A|D|C|R|
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        .                       AR Information                          .
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        |                        Reserved                       |A|D|C|R|
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>        .                       AR Information                          .
>        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> 
> where the number of repeats is equal to the number of AR addresses previously specified.
> 
> This needs to be made much clearer. ISTM it would be cleaner to forget the flag, and simply say this is always a list, where the last element has no AR Information and provides options for any address not previously mentioned. (But this isn't an option if it is documenting existing usage.)
> 
> In Figure 9, I gather that "AR Information" means "Access Router Information Element", and in this context it must be restricted to a single address, and must be the address of one of previously specified AR addresses. If so, please say this explicitly.
> 
> <zongpeng> Thanks for proposing the problem. We revise accordingly.
> The change is a little complicated, and is described following the comments.
> Several people had edited the draft, so that there was some conflicts in the description.
> But the main opinion among the authors is the same. Thanks again for the suggestion.</zongpeng>

The fix looks good to me.

> (7) MINOR:
> 
> Section 5.3 has a similar construction to that in 5.2, with the same issues and should get a comparable fix.
> 
> (8) NIT:
> 
> IdNits reports that the reference to RFC2460 in section 5.6 is obsolete.
> <zongpeng> Thanks for proposing the problem. We revise accordingly. </zongpeng>

Looks right.

> About the (6) and (7), we choose to forget the flag as you have suggested. However, it is found that it is not enough to just modify section 5.2 and 5.3, so that section 5.4, 5.5, 5.6 are also modified to support information providing of more than one ARs.
> Among them, section 5.5 is a little different, because every GRE key should be independent, and needs not to be the same.
> Also for section 5.6, IPv6 MTU are not needed in the context of IPv4 environment, so there is no default value, neither.
> 
> For the detailed modifications, please refer to the new version of draft attached.
> Perhaps we will update the draft recently if it is ok for you.

	Thanks,
	Paul