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

[Mahesh Jethanandani <[e-mail]>]

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.

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