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

[Benoit Claise <[e-mail]>]

Mo,

As discussed in NETMOD, the intented status for 
draft-ietf-netmod-yang-tree-diagrams should be Informational and 
therefore the reference informative.

Regards, B.
> 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.
>
> 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.
> - Please add a note for the RFC editor to replace “RFC XXXX” with the RFC number when the draft is published.
> - The indentations in the YANG module are still not consistent. Please fix them.
>
> 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? Does it mean you need a subscriber-mask for v6 only?
> - Also, you augment the NAT module at the node nat:policy. But subscriber-mask-v6 is a child of nat:nat-policy.
> - 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?
> - You augment nat:mapping-entry, which is also a list with a key of ‘index’. Where are you deriving ‘index’ from?
> - 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?
>
> 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.
>
>>> Section 1.1 Terminology
>>>
>>> What terms are defined in RFC 6333 that are being used in this draft.
>>> Please be
>>> explicit.
>> [Med] Actually, all the terms defined in Section 3 of RFC 6333 are used in this document. I added an explicit pointer to Section 3.
>>
>>> Section 1.2 Tree Diagram
>>>
>>> Please refer to
>>> https://tools.ietf.org/html/draft-ietf-netmod-yang-tree-diagrams-01
>>> instead of
>>> defining the nomenclature for tree diagram in the document.
>> [Med] Done. Thanks.
>>
>>> Section 2.
>>>
>>> s/can:/can
>> [Med] There is no such occurrence in -06.
>>
>>> s/provisioned to the AFTR/provisioned on the AFTR/
>> [Med] There is no such occurrence in -06.
>>
>>> The document is very light in the architectural description of the YANG
>>> model
>>> itself. Statements like “model supports enabling one or more instances of
>>> the
>>> AFTR function on a device” are obvious looking at the model. A more
>>> reasonable
>>> description would be why multiple instances need to be supported. The same
>>> is
>>> true for “AFTR instance”. The fact that the instance has been defined as a
>>> list
>>> assure that each instance can be provisioned with dedicated configuration
>>> data,
>>> and maintain its own data table. Again why the multiple instances need to
>>> be
>>> maintained, what purpose do they serve, and the fact that they maintain
>>> their
>>> own data table is not clear.
>>>
>>> The document “assumes [RFC4787] [RFC5382][RFC 5508] are enabled”. What
>>> particular aspects of those documents is the draft assuming? Note, RFC4787
>>> is
>>> NAT Behavioral Requirements for Unicast UDP, and outlines some seven
>>> behaviors,
>>> all the way from NAT and Port Translation, Filtering, Hairpinning to
>>> Fragmentation of Outgoing Packets. Even if all the behaviors are assumed,
>>> it
>>> would be good to know how they impact this particular model.
>> [Med] What it meant is that all the recommendations are supported + all recommended values are the ones defined in those RFCs.
>>
>>> Section 3
>>>
>>> General Comments.
>>>
>>> Please follow [RFC6087], Guidelines for YANG documents, on how to write a
>>> YANG
>>> model. For example, the organization in the model should be IETF Softwire
>>> WG,
>>> not just Softwire WG.
>>>
>>> The indentation in the model is off in a few places (reference is a
>>> keyword
>>> that should be at the same indentation as description in all revision
>>> statements). Most models follow an indentation of 2 characters for every
>>> subsequent depth in the model. I see anywhere between 2 to 10 characters
>>> of
>>> indentation.
>>>
>> [Med] Will double check. Thanks.
> [mj] As mentioned above, the model still has indentation issues.
>
>>
>>> As far as naming conventions go, there is little value repeating names in
>>> a
>>> given hierarchy. For example, if the grouping is aftr-parameters, then it
>>> is a
>>> given that parameters inside the grouping are aftr parameters. No need to
>>> say
>>> dslite-aftr-ipv6-address, where both dslite and aftr are redundant.
>>>
>> [Med] Please note that this does not apply for -06.
>>
>>> There is little or no use of must statements to qualify the configuration.
>>> I am
>>> not an expert at DS-Lite, so I cannot suggest where a must statement may
>>> be
>>> helpful to have. But with all the parameters that are optional, there must
>>> be
>>> some relationship between the attributes that could be captured with must
>>> statements. For example, for the leaf max-softwire-per-subscriber, the
>>> leaf is
>>> trying to prevent a misbehaving subscriber. Could that behavior be somehow
>>> captured with a must statement for the resources it can/cannot consume. Or
>>> with
>>> port-presevation-enable and port-parity-preservation-enable? Is there a
>>> relationship between the two variables that could be captured in the
>>> model?
>>>
>>> All references to RFC should be moved under a reference section.
>> [Med] Please check -06. All references are under a reference section.
>>
>>
>> For
>>> example:
>>> OLD:
>>>       leaf udp-lifetime {
>>>           type uint32;
>>>           units "seconds";
>>>           default 120;
>>>           description
>>>               "UDP inactivity timeout [RFC4787].";
>>>       }
>>> NEW:
>>>       leaf udp-lifetime {
>>>           type uint32;
>>>           units "seconds";
>>>           default 120;
>>>           description
>>>               "UDP inactivity timeout.“;
>>>           reference
>>>             “[RFC4787].”;
>>>       }
>>>
>>> A note should be prepared for the RFC editor to indicate what revision the
>>> model should finally carry, and what the description/reference statement
>>> in the
>>> model should look like when the document is finally published. See
>>> examples in
>>> other drafts such as draft-ietf-netconf-zerotouch Editorial Note on what
>>> to add.
>>>
>>> Specific Comments
>>>
>>> The name of the file in the line with <CODE BEGINS> is missing the .yang
>>> suffix
>>> in the file. When extracted, the extracted file will be missing the
>>> suffix.
>>> Please add it. This leads to the following error from yanglint.
>> [Med] This was fixed in previous version.
>>
>>> ietf-dslite@2017-01-03" without file extension - unknown format.
>>>
>>> In grouping port-number, could the port-range be something as simple as
>>>
>>> container port-number {
>>>   must “starting-port”;
>>>   leaf starting-port {
>>>     type inet:port-number;
>>>   }
>>>   leaf ending-port {
>>>     type inet:port-number;
>>>   }
>>> }
>>>
>>> where if there is no ending-port it is deemed to be a single port, but if
>>> there
>>> is a ending-port is considered to be a range?
>> [Med] This does not apply for -06.
>>
>>> The list transport-protocol has one leaf which is the transport-protocol-
>>> id.
>>> Why does the single leaf, which is the key, need to be inside a list?
>> [Med] Idem as above.
>>
>>> Logging-info has a choice statement for protocol. Is it that there can be
>>> only
>>> one choice for the form of logging? Could it be possible that users might
>>> want
>>> to enable both syslog and ipfix?
>> [Med] Idem as above.
>>
>>> A pyang compilation of the model with —ietf and —lint option was clean.
>>>
>>> A idnits run of the draft reveals some issues with spacing and references.
>>> The
>>> one related to spacing are parts of the diagram, which confuses idnits.
>> [Med] Will double check those. Thanks.
>>
>>> Checking nits according to https://www.ietf.org/id-info/checklist :
>>>   ------------------------------------------------------------------------
>>> ----
>>>
>>>   ** There are 5 instances of too long lines in the document, the longest
>>> one
>>>      being 3 characters in excess of 72.
>>>
>>>   == There are 2 instances of lines with non-RFC6890-compliant IPv4
>>> addresses
>>>      in the document.  If these are example addresses, they should be
>>> changed.
>>>
>>>   Miscellaneous warnings:
>>>   ------------------------------------------------------------------------
>>> ----
>>>
>>>   == Line 162 has weird spacing: '...ocol-id    uin...'
>>>
>>>   == Line 207 has weird spacing: '...address    ine...'
>>>
>>>   == Line 215 has weird spacing: '...address    ine...'
>>>
>>>   == Line 238 has weird spacing: '...rvation    boo...'
>>>
>>>   == Line 261 has weird spacing: '...ocol-id    uin...'
>>>
>>>   == (5 more instances...)
>>>
>>>   Checking references for intended status: Proposed Standard
>>>   ------------------------------------------------------------------------
>>> ----
>>>
>>>      (See RFCs 3967 and 4897 for information about using normative
>>> references
>>>      to lower-maturity documents in RFCs)
>>>
>>>   == Unused Reference: 'RFC7753' is defined on line 1988, but no explicit
>>>      reference was found in the text
>>>
>>>   == Outdated reference: A later version (-04) exists of
>>>      draft-boucadair-pcp-yang-03
>>>
>>>      Summary: 1 error (**), 0 flaws (~~), 9 warnings (==), 1 comment (--).
>>>
> Mahesh Jethanandani
> [e-mail]
>
> .
>