Early Review of draft-ietf-softwire-dslite-yang-02
review-ietf-softwire-dslite-yang-02-yangdoctors-early-jethanandani-2017-10-06-00

Request Review of draft-ietf-softwire-dslite-yang-06
Requested rev. 06 (document currently at 17)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2017-10-05
Requested 2017-09-07
Requested by Ian Farrer
Other Reviews Genart Last Call review of -14 by Russ Housley (diff)
Genart Telechat review of -15 by Russ Housley (diff)
Comments
Hi,

The authors of this draft have requested an early review of this draft. Please can a YANG doctor be assigned?

Many thanks,
Ian
Review State Completed
Reviewer Mahesh Jethanandani
Review review-ietf-softwire-dslite-yang-02-yangdoctors-early-jethanandani-2017-10-06
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/RYhNmmV5K_MMX4qamhdluSlU3YM
Reviewed rev. 02 (document currently at 17)
Review result On the Right Track
Draft last updated 2017-10-06
Review completed: 2017-10-06

Review
review-ietf-softwire-dslite-yang-02-yangdoctors-early-jethanandani-2017-10-06

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.

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.

Is it given that a server will implement both AFTR and B4? 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.

Section 1.1 Terminology

What terms are defined in RFC 6333 that are being used in this draft. Please be explicit. 

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.

Section 2.

s/can:/can
s/provisioned to the AFTR/provisioned on the AFTR/

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.

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.

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.

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

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?

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?

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?

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. 

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