Re: [Softwires] WGLC on draft-ietf-softwire-map-radius-14 - 2 of 2
Hi Ian, Please see my reply inline: Thanks for your comments Yu From: softwires-boun...@ietf.org [mailto:softwires-boun...@ietf.org] On Behalf Of ianfar...@gmx.com Sent: Friday, April 06, 2018 10:59 PM To: Yong Cui; Softwires WG Subject: Re: [Softwires] WGLC on draft-ietf-softwire-map-radius-14 - 2 of 2 Hi, Review part 2. Cheers, Ian - 4. Attributes 4.a) 'sub options' is used. Conventionally, this is hypenated as 'sub-options'. Please change throughout. [fuyu] Ok, I will change it throughout. 4.b) It would be more accurate to say: Different combinations of sub-options are required for each type of S46 Container... [fuyu] Ok, I will update this sentence. 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. [fuyu] I will add a paragraph in the Introduction. 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. [fuyu] OK 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. [fuyu] Ok, I will move the table from section 4.7 to section 4.2. 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. [fuyu] You have missed S46-DMR Sub Option. I think that one diagram will make the overall structure of the S46 Container Option more clearly than divided it into 3 diagrams. Do you think if I add the mandatory and optional for each sub-option in the figure 2 will be better? 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. [fuyu] Yes, sorry, it is a fault. 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./ [fuyu] I will update this sentence. 4.i) 4.3.3. S46-DMR Sub Option s/set to all zero./set to all zeros./ [fuyu] Done 4.j) 4.3.4. S46-V4V6Bind Sub Option s/There MUST be at most one/There MUST be exactly one/ [fuyu] Done 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. [fuyu] Done 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. [fuyu] OK, I will add a pointer to RFC7598. 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]./ [fuyu] Done. 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 co
Re: [Softwires] WGLC on draft-ietf-softwire-map-radius-14 - 2 of 2
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.
