Re: [Softwires] Yangdoctors early review of draft-ietf-softwire-dslite-yang-02

[Mahesh Jethanandani <[e-mail]>]

Med,

See inline.

> On Nov 15, 2017, at 8:50 PM, [e-mail] wrote:
> 
> Hi Mahesh, 
> 
> Thank you for the feedback. 
> 
> Please see inline. 
> 
> Cheers,
> Med 
> 
>> -----Message d'origine-----
>> De : Mahesh Jethanandani [mailto:[e-mail] <mailto:[e-mail]>]
>> Envoyé : mardi 14 novembre 2017 23:59
>> À : BOUCADAIR Mohamed IMT/OLN
>> Cc : [e-mail] <mailto:[e-mail]>; [e-mail] <mailto:[e-mail]>; draft-ietf-softwire-
>> [e-mail] <mailto:[e-mail]>; 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.

> 
>> 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.

> 
> 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. 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.

> 
>> - 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 '[e-mail] <mailto:[e-mail]>'
>   Removed 0 empty lines
> Extracting '[e-mail] <mailto:[e-mail]>'
>   Removed 0 empty lines
> 
> 
> [e-mail] <mailto:[e-mail]>:
> 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
> 
> 
> [e-mail] <mailto:[e-mail]>:
> 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, [e-mail] 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:[e-mail]]
>>>> Envoyé : samedi 7 octobre 2017 02:17
>>>> À : [e-mail]
>>>> Cc : [e-mail]; [e-mail];
>>>> [e-mail]
>>>> 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. 

Mahesh Jethanandani
[e-mail]