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

[<[e-mail]>]

Hi Mahesh,

Please see inline.

Cheers,
Med

De : Mahesh Jethanandani [mailto:[e-mail]]
Envoyé : jeudi 16 novembre 2017 00:59
À : BOUCADAIR Mohamed IMT/OLN
Cc : [e-mail]; [e-mail]; [e-mail]; 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, [e-mail]<mailto:[e-mail]> wrote:

Hi Mahesh,

Thank you for the feedback.

Please see inline.

Cheers,
Med


-----Message d'origine-----
De : Mahesh Jethanandani [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.

[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 '[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]<mailto:[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]<mailto:[e-mail]>
Cc : [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>;
[e-mail]<mailto:[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.

[Med] Fair enough. I updated the module accordingly. Thank you.


Mahesh Jethanandani
[e-mail]<mailto:[e-mail]>