Re: [radext] Review request for draft-ietf-softwire-map-radius

[Alan DeKok <[e-mail]>]

> There has been a lot of work on getting draft ready for publication from
> the opinion of softwire wg. However, because there are several new
> attributes and a lot of sub-TLVs, we think we need to get RADEXT to help
> review, before we advance this doc to IESG.

  At a high level:

* Please remove all ASCII art.  RFC 8044 (RADIUS data types) Section 2.1 suggests that this is no longer necessary.

* Please use the attribute definition format suggested in RFC 8044 Section 2.1.3.  Doing so will make the document clearer

* Most of the data types in this document fit the data types in RFC 8044.  If not, the considerations of RFC 6158 Section 3.2.3 on "Complex Data Types" need to be addressed.

* Please decrease the use of "TLV"  An attribute can be referred to by name. e.g. "S46-MAP-E".  It does not need to be referred to as "S46-MAP-E TLV"


  And a review in detail:

 3.  Extensions of RADIUS Attributes and TLVs

   I suggest just calling this "New RADIUS Attributes".  There already exists "RADIUS extensions" documents, which do radically different things.


      The Softwire46-Configuration Attribute MUST contain one or more of
      the following: S46-MAP-E TLV, S46-MAP-T TLV, and/or S46-
      Lightweight-4over6 TLV.

  "the following... " what?  I suggest:

      The Softwire46-Configuration Attribute MUST contain one or more of
      the following attributes: S46-MAP-E, S46-MAP-T, and/or S46-
      Lightweight-4over6.

  Similar comments apply to many other places in the document.

  Also, is there any reason to shorten the attribute names?  "Softwire46-MAP-E" would be clearer than "S46-MAP-E".


  Figure 1 is useful and informative.  NIT: I would remove the repeated use of "TLV" and "sub-TLV".  Which attribute contains what is clear from the diagram.


3.1.1.1.  S46-MAP-E TLV Format

  I would rename this section "S46-MAP-E Attribute".  The section describes the attribute, not just it's format.  If RFC 8044 is referenced, the formats are defined there.  And don't need to be copied here.

  The section also doesn't say what the attribute is *for*.  While that description is alluded to in the rest of the document, it it not clear.  Again, using the layout recommended by RFC 8044 would be useful.  That layout includes a portion for a description of what the attribute does.

  Similar comments apply to many other places in the document.


Table 2: Softwire46 Sub-TLVs

  The use of "M - Mandatory, O - Optional, N/P - Not Permitted" is unusual.  I suggest using a table format similar to RFC 2865 Section 5.44, as is done with other RADIUS RFCs.  e.g. use "0", "0-1", "0+". etc.


  For number assignment, the TLVs create a new number space, and can likely be assigned directly in the document.  e.g. if "Foo" contains "Bar" and "Baz", then you can assign "1" to "Bar", and "2" to "Baz".  This is what IANA will do anyways, and it helps clarify the document, and prevent mistakes.


3.1.4.3.  EA-Length Sub-TLV

  This section defines an attribute that contains a 16 bit number.  Such attributes are discouraged by RFC 6158.  This definition should be corrected to be a 32-bit integer, or an explanation should be given as to why 16 bits is required here.

  Similar comments apply for Sections 3.1.6.1, 3.1.6.2, and 3.1.6.3.

  Section 3.2.  defines an attribute that contains an array of 16-bit integers.  Please follow the guidelines of RFC 6158 here.