Re: [Softwires] WGLC on draft-ietf-softwire-map-radius-14 - 1 of 2

[ianfarrer]

Hi,

Here’s my review of the draft. It’s rather long so I’ve had to submit it as two 
emails. It’s based on -15. 

Cheers,
Ian


Title
T.a)
 RADIUS Attribute for Softwire Address plus Port based Mechanisms

 To improve readability and as this defines 3 new RADIUS attributes, 
 'RADIUS Attributes for Address plus Port based Softwire Mechanisms'

-
Abstract:
A.a)
"IPv4-over-IPv6 transition mechanisms provide both IPv4 and IPv6
   connectivity services simultaneously during the IPv4/IPv6 co-existing
   period." 

This isn't really true. The transisition mechanisms don't provide
v6 connectivty, they rely on this being there and working. Suggested re-word:

IPv4-over-IPv6 transition mechanisms provide both IPv4 and IPv6
   connectivity services simultaneously during the IPv4/IPv6 co-existence
   period.
   
A.b)
"The Dynamic Host Configuration Protocol for IPv6 (DHCPv6)
   options have been defined to configure Customer Edge (CE) in MAP-E,
   MAP-T, Lightweight 4over6 and PREFIX64 option for Multicast Basic
   Bridging BroadBand (mB4) in multicast scenarios. "
   
Suggested re-word for readability: 
   DHCPv6 options have been defined for configuring clients to use MAP-E,
   MAP-T, Lightweight 4over6 and Multicast Basic Bridging BroadBand (mB4) in 
multicast scenarios.  
   

A.c)
The abstract has both the expanded and abbrieviated form of a few terms
(e.g. BNG). These definitions are also repeated later in the introduction.
As these terms are defined in the IETF acronyms list
https://www.rfc-editor.org/materials/abbrev.expansion.txt 

there's no need to provide these definitions, just the acronyms would
be OK. 
As a suggestion for readability - DHCPv6, RADIUS, AAA, and BNG are well
known so don't need the expanded form. mB4 is less well known, so
the expanded version is more helpful.
The repetitions of the definitions in the Introduction can be removed.

A.d)
"This document defines two new Remote Authentication Dial In User
   Service (RADIUS) attributes that carry CE or mB4 configuration
   information from an AAA server to BNG."

There's actually 3 new attributes in the draft (Softwire46-Priority
is missing).


--

1. Introduction
1.a)
It would simplify the overall readibility of the document
to define the MAP-E, MAP-T and lw4o6 (i.e. RFC7598 configured)
softwires as 'unicast' and the RFC8114 as multicast in the 
Intro. This would avoid the need to list MAP-E, MAP-T...
etc. throughout.

1.b)
s/Recently providers/Recently, providers/

1.c)
s/started to deploy IPv6 and consider how to transit to IPv6./
started deployment and transition to IPv6./

1.d)
"In particualr, the CE uses DHCPv6 options to discover the
   Border Relay (BR) and retrieve Softwire46 (S46) configurations."

This is one way that configuration can be done - there's also
a YANG model in development and DHCPv4o6 provisioning. It would be
more accurate to say:

"[RFC7598] defines DHCPv6 options for provisioning
CEs for unicast softwires."

1.e)
For the mutlicast section, all of the fields that are present
in OPTION_V6_PREFIX64 are described in full, but this is not
done for the unicast equivalents. It would make sense to be
uniform between the two, but if all of the options/sub-options
from RFC7598 get included, then it's going to get pretty long.

I think it would improve the consistency and readability
without brining in large amounts of duplication to remove
the text starting 'The following lists the multicast-related
information...' upto (and including) the Unicast Prefix64
description.

In it's place, a paragraph describing the relationship between
this document and RFC7598/8115 could be added, referencing the
relevant sections in those RFCs that the fields are defined.

1.f)
After the sentence "A DHCPv6 server function is
   assumed to be embedded in the BNG that allows it to locally handle 
   any DHCPv6 requests initiated by hosts." it would be
   worth adding that the term BNG is used througout the document
   to describe a device which functions as both the AAA client
   and DHCPv6 server. 

1.g)
s/MAP-E[RFC7597], MAP-T[RFC7599] and Lightweight 4over6[RFC7596]/
MAP-E [RFC7597], MAP-T [RFC7599] and Lightweight 4over6 [RFC7596]/

1.h)
s/options[RFC7598]/options [RFC7598]/

1.i)
Last paragraph: It would also be worth saying how the structure of the 
DHCP options and field namings are preserved so they can 
easily be mapped between DHCP and RADIUS.



3. Configuration Process with RADIUS
3.a)
s/CE with MAP/CE with softwire/

3.b)
s/how the RADIUS protocol and DHCPv6 co-operate/how the RADIUS and
DHCPv6 protocols co-operate/

3.c)
The figure is easier to follow if it is space out a bit more and 
clafifies the first step:

  CE BNG AAA Server
  |   |   |
  |---1.DHCPv6 Solicit--->|   |
  |(ORO with 

Re: [Softwires] WGLC on draft-ietf-softwire-map-radius-14 - 2 of 2

[ianfarrer]

Hi,

Review part 2.

Cheers,
Ian

-
4. Attributes 
4.a) 'sub options' is used. Conventionally, this is
hypenated as 'sub-options'. Please change throughout.

4.b) It would be more accurate to say:
Different combinations of sub-options are
   required for each type of S46 Container...

4.c)
"  The RADIUS attribute
   for Dual-Stack Lite [RFC6333] is defined in [RFC6519]."

This is the first time in the document that DSLite is referenced.
Would it be better to put a paragraph in the Introduction saying
that this document is not concerned with DSLite/RADIUS as it's 
already covered in RFC6519.

4.c)
The S46 Container Options section describes how the containers and
options are constructed. It would read better to have this overview text
before descrbing the structure of any of the attributes/options.

4.e)
In the description text around Fig.2 a pointer to the table in 4.7
would be helpful. It may actually be better to move the table from
section 4.7 into section 4.2 so they can be compared directly by
the reader.

4.f)
Figure 2 doesn't really make it clear that the relevant (and necessary)
sub-options (numbered 1-5) is decided by which on of the three
S46 container options is being used. This makes it confusing to understand.

As a suggestion, would 3 diagrams (one for each type) structured like this
be more clear showing what is necessary and what is optional?

   / (Mandatory)/1.Rule-IPv6-Prefix
  ||   sub-option
  | 1.S46-Rule + 2.Rule-IPv4-Prefix
  |sub-option  |   sub-option
  || 3.EA Length
 S46 MAP-E Container--+ \  sub-option
  Option  | 2.S46-BR Sub Option
- - - - - - - - - - - - - - - - - - - - - - - - - 
  | (Optional)   /1.PSID-offset
  | |   sub-option
  | 3.S46-PORTPARAMS ---+ 2.PSID-len
  | sub-option  |   sub-option
  | | 3.PSID
   \ \  sub-option

The paragraph begining 'There are three types of S46 Container Options...'
would need to be moved before this for readability.

4.g)
The paragraph begining 'There are three types of S46-Rule Sub Options...'
seems to be in the wrong section. It belongs in 4.3.1.

4.h)
s/The S46-BR Sub Option can only be encapsulated in the MAP-E Container
   Option or the Lightweight 4over6 Container Option./
   The S46-BR Sub Option can only be encapsulated in the MAP-E or 
   Lightweight 4over6 Container Options./

4.i)
4.3.3.  S46-DMR Sub Option
s/set to all zero./set to all zeros./

4.j)
4.3.4. S46-V4V6Bind Sub Option 
s/There MUST be at most one/There MUST be exactly one/

4.k)
4.3.5.  S46-PORTPARAMS Sub Option
Suggest that the first sentance is extended to say:
The S46-PORTPARAMS Sub Option specifies optional port set information
   that MAY be provisioned to CEs to configure sharing of an IPv4 address.

4.l)
Throughout this section, it would be a good idea to put in a pointer
to the section of RFC7598 that the option/sub-option corresponds to.

4.m)
s/The Softwire46-Multicast attribute conveys the IPv6 prefixes to be
   used in [RFC8114] to synthesize IPv4-embedded IPv6 addresses./
   The Softwire46-Multicast attribute conveys the IPv6 prefixes to be
   used to synthesize IPv4-embedded IPv6 addresses as per [RFC8114]./

4.n)
This attribute MAY be used in Access-Request packets as a hint to the
   RADIUS server.  For example, if the BNG is pre-configured with
   Softwire46-Multicast, these prefixes MAY be inserted in the
   attribute.

Is this saying that the attribute could be pre-configured with a
value in the BNG's AAA client meaning that it wouldn't be requested from
AAA, but would be supplied in the DHCP message? Please can you expand
the description. I also wonder if this statement is true for any of
the other attributes described in the document.

4.o)
The definitions of which RADIUS messages types the multicast 
attribute can appear in should also exist for the unicast and priority
attributes.
Update - having read section 4.10, the information is there, but
is duplicated for multicast. One common format for the information
and pointers would be cleaner.

4.p)
The description text and formatting between the 4.2-4.8 options is
inconsistent and should be aligned. Personally, I think the bullet
point list of what is and isn't permitted in section 4.9 is clear
and could be usefully used in 4.2-4.9.

4.q)
4.9.1.  ASM-Prefix64 TLV
The field in the diagram is called 'Prefix-Length', but in the
description, this field is 'Length'. This also needs fixing
in 4.9.2 and 4.9.3

4.r)
4.9.2 & 4.9.3
s/This fiel is reserved./This field is reserved./

4.s)
Section 4.9 uses TBD1 as a placeholder for an IANA assignment. 

[Softwires] Opsdir last call review of draft-ietf-softwire-map-mib-12

[Qin Wu]

Reviewer: Qin Wu
Review result: Ready

I have reviewed this document as part of the Operational directorate's ongoing
effort to review all IETF documents being processed by the IESG. These comments
were written with the intent of improving the operational aspects of the IETF
drafts. Comments that are not addressed in last call may be included in AD
reviews during the IESG review.  Document editors and WG chairs should treat
these comments just like any other last call comments. This draft defines MIB
for MAP-E for use with SNMP. It is well written and I have no concern on
operational aspects. Here are a few editorial comments as follows: 1. Please
remove unused reference RFC7598. 2. Section 4.1, the 1st paragraph, last
sentence Can you list which parts of the IF-MIB in more details here the MAP-E
depends on? 3. Section 4.1.1 two categories on mapping rules In MIB module
definition, it looks the mapping rule is divided into three categories, i.e.,
BMR, FMR and BMRandFMR,which is not consistent with two categories
classification defined in section 4.1.1, I am wondering whether we also have
fmrandbmr, i.e., Forwarding Mapping Rule can also be basic Mapping Rule, in
other words, is fmrandbmr same as bmrandfmr? Is fmrandbmr a set that belong to
both fmr and bmr? Try to understand this, would it be great to clarify this in
section 4.1.1. 4.Section 4.1.2 two kind of invalid packets In MIB module
definition, two MapSecurityCheckEntries are defined, one is
mapSecurityCheckInvalidv4, the other is mapSecurityCheckInvalidv4. I am
wondering whetherthese two entries are corresponding to two kind of invalid
packets described in section 4.1.2. also I am not sure I understand payload
source IPv4 address and port, are these payload source and port are referred to
received packets’ source IPv4 address port mentioned in section 4.1.2.
5.Section 6 does this document request IANA to assign new OID under mib-2 or
just use existing OID under mib-2?

___
Softwires mailing list
Softwires@ietf.org
https://www.ietf.org/mailman/listinfo/softwires