Hi Mahesh, Please see inline.
Cheers, Med De : Mahesh Jethanandani [mailto:[email protected]] Envoyé : jeudi 16 novembre 2017 00:59 À : BOUCADAIR Mohamed IMT/OLN Cc : [email protected]; [email protected]; [email protected]; NetMod WG Chairs; Benoit Claise Objet : Re: Yangdoctors early review of draft-ietf-softwire-dslite-yang-02 Med, See inline. On Nov 15, 2017, at 8:50 PM, [email protected]<mailto:[email protected]> wrote: Hi Mahesh, Thank you for the feedback. Please see inline. Cheers, Med -----Message d'origine----- De : Mahesh Jethanandani [mailto:[email protected]] Envoyé : mardi 14 novembre 2017 23:59 À : BOUCADAIR Mohamed IMT/OLN Cc : [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; draft-ietf-softwire- [email protected]<mailto:[email protected]>; NetMod WG Chairs; Benoit Claise Objet : Re: Yangdoctors early review of draft-ietf-softwire-dslite-yang-02 Mohamed, I realize that I am responding to an earlier thread than the one you asked your question on, so let me address the question that you asked on that thread first. Yes, the netmod tree diagrams draft is not an RFC, and yang-doctors do recommend that authors of drafts with YANG module reference the netmod tree diagram draft rather than cut and paste description of the tree symbols into their drafts. To the question of whether that reference can be informative or does it have to be normative, I would agree that it has to be normative. I realize that might impact the publication of the draft, so I am including the chairs of NETMOD WG(and one of the authors) to get an idea of when they think the draft could be published as an RFC. [Med] We need guidance how to proceed given that we do have two drafts in the softwire WG that are impacted (this one passed the WGLC, while the second one will be on the WGLC soon). BTW, an argument I heard recently is that the netmod document can be cited as informative given that the tree structure itself is not normative. I did bring up the question yesterday in NETMOD, and yes the consensus in NETMOD is to move the tree diagram document from Standards Track to Informational, where it can then be cited as informational. [Med] Thank you. But before you rush into publication, I took another look at the -08 version of the draft, and I have additional concerns about the draft and the YANG module in it, that I believe warrant another look. Minor comments - Please remove reference to chairs in the YANG module. All we recommend are the authors/editors. [Med] Done. - Please add a note for the RFC editor to replace “RFC XXXX” with the RFC number when the draft is published. [Med] There is already this note in the draft: === Editorial Note (To be removed by RFC Editor) Please update these statements with the RFC number to be assigned to this document: o "This version of this YANG module is part of RFC XXXX;" o "RFC XXXX: YANG Data Modules for Dual-Stack Lite (DS-Lite)"; o "reference: RFC XXXX" === - The indentations in the YANG module are still not consistent. Please fix them. [Med] OK. Additional comments: - The YANG module makes multiple references to subscriber-mask as a way to uniquely identity a subscriber. That, as you say is derived from the NAT module. But the NAT module has no subscriber-mask. It has subscriber-mask- v6. Is that what you meant to refer to? [Med] Yes. Please update the model/draft in that case. [Med] Done in my local copy. Does it mean you need a subscriber-mask for v6 only? [Med] Yes. The two use cases for this mask are DS-Lite and NAT64, which involve IPv6 addresses. The NAT YANG modules includes the following: "The subscriber mask is an integer that indicates the length of significant bits to be applied on the source IPv6 address (internal side) to ^^^^^^^^^^^^ unambiguously identify a user device (e.g., CPE). - Also, you augment the NAT module at the node nat:policy. But subscriber- mask-v6 is a child of nat:nat-policy. [Med] Is there any problem with this? Yes. To the extent that you cannot augment a node that does not exist in the tree. [Med] Not sure to understand this one. The augmented node is nat:policy. What is more troubling to me is that you say none of the tools have caught it. In the output you show below, it is not clear which NAT module you are using, and where it was fetched from. [Med] The datatracker shows all these details. Please check at: https://datatracker.ietf.org/doc/draft-ietf-softwire-dslite-yang/ - And, nat:nat-policy is a list with a key of policy-id. But you have no policy-id defined in your module. How do you reference the particular nat:nat-policy without a key? Was this model compiled/validated against the latest NAT module? [Med] This is derived from the nat:napt44, which is augmented with DS-Lite specifics. I confirm that the module was successfully validated: ========= xym 0.4: Extracting '[email protected]<mailto:[email protected]>' Removed 0 empty lines Extracting '[email protected]<mailto:[email protected]>' Removed 0 empty lines [email protected]<mailto:[email protected]>: pyang 1.7.3: pyang --verbose --ietf -p {libs} {model}: No validation errors yanglint 0.13.79: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} {model} -i: No validation errors [email protected]<mailto:[email protected]>: pyang 1.7.3: pyang --verbose --ietf -p {libs} {model}: No validation errors yanglint 0.13.79: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} {model} -i: No validation errors =========== I can add an explicit "when" statement that will look like: when "/nat:nat/nat:instances/nat:instance/nat:type='napt44'"; This will require updating the NAT YANG module to specify explicitly a NAT Type. - You augment nat:mapping-entry, which is also a list with a key of ‘index’. Where are you deriving ‘index’ from? [Med] Idem as above. - b4-ipv4-address has a default value of 192.0.0.2, and you have a note that the mask is /29. Are all ipv4 addresses configured with /29 mask? If not, how is netmask indicated to the device? [Med] This is about an IPv4 address. The mask is not required to be configured. This address can be picked from the /29 reserved block. See additional comments inline. Cheers. On Oct 9, 2017, at 3:34 PM, [email protected]<mailto:[email protected]> wrote: Dear Mahesh, Thank you very much for the detailed review. Much appreciated. It seems that you reviewed -02 of the draft, while the latest version is -06 (https://datatracker.ietf.org/doc/draft-ietf-softwire-dslite-yang/). As you can see below, almost all the comments do not apply for -06. Please see inline. Cheers, Med -----Message d'origine----- De : Mahesh Jethanandani [mailto:[email protected]] Envoyé : samedi 7 octobre 2017 02:17 À : [email protected]<mailto:[email protected]> Cc : [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]> Objet : Yangdoctors early review of draft-ietf-softwire-dslite-yang-02 Reviewer: Mahesh Jethanandani Review result: On the Right Track Document reviewed: draft-ietf-softwire-dslite-yang-02. Do not know why I was assigned -02 version when I now notice that there are several 03 versions submitted all the way up to -06. Status: On the Right Track. I am not an expert in DS-Lite. This review is looking at the draft from a YANG perspective. With that said, I have marked it as “On the Right Track” because of some of the points discussed below. The authors are encouraged to spend time looking at other models for examples on how to write the model, read RFC6087, Guidelines for YANG documents. Summary: This document defines a YANG data model for the DS-Lite Address Family Transition Router (AFTR) and Basic Bridging BroadBand (B4) elements . Overall Comments: The draft should refer to YANG 1.1 (RFC 7950) instead of RFC6020. [Med] Can fix this one. The draft is not NMDA compliant. It carries a separate -state container that needs to be collapsed into one single container. For example, it carries containers such as aftr-current-config which seems to be carrying state information for the leafs in dslite-config. [Med] There are no such containers in -06. Is it given that a server will implement both AFTR and B4? [Med] No. If not, then please define feature statements and use if-feature for each part (AFTR and B4), so implementors can choose what part of the model is implemented. [mj] I do not see this comment addressed. [Med] This is easy to fix. Actually, I interpreted no follow-up from your side as having separate modules is also fine with you. Features allow platforms to identify parts of the model that they are capable of supporting, while at the same time removing parts that are not supported. Platforms can deviate a model and say what they do not support parts of the model, but we usually reserve that for the case where there is no (hardware) support for a given feature. [Med] Fair enough. I updated the module accordingly. Thank you. Mahesh Jethanandani [email protected]<mailto:[email protected]>
_______________________________________________ Softwires mailing list [email protected] https://www.ietf.org/mailman/listinfo/softwires
