Subject: [RTG-DIR] Rtgdir early partial review of draft-ietf-rtgwg-routing-types-02

From: Stewart Bryant <[email-masked]>
Date: Wed, 03 May 2017 07:54:19 -0700

Review is partially done. Another review request has been registered
for completing it.

Reviewer: Stewart Bryant
Review result: Has Issues

Firstly I should say that I am not an expert in YANG, hence my
suggestion that you may wish to assign an additional reviewer.

However in reviewing this a number of questions and issues arose:

2.  Overview

   This document defines the following data types:

SB> Accessibility note - the order of types seems random. It might
SB> be more helpful to the reader if they were, in a systematic order
SB> for example alphabetical and/or dependency order.

=======

 router-id
      Router Identifiers are commonly used to identify a nodes in
SB> s/a nodes/nodes/

=======

   route-target-type
      This type defines the import and export rules of Route Targets,
as
      descibed in Section 4.3.1 of [RFC4364].  An example usage can
be
SB> s/descibed/described/
      found in [I-D.ietf-idr-bgp-model].

========

SB> I am surprised that IP multicast addresses are here, but IP
addresses are not.
SB> I would have thought that both should be in the same place.

========

SB> In some protocols we use the NTP and the 1588 timer types, I
assume
SB> they are defined elsewhere.

========

   mpls-label
      The 20 bits label values in an MPLS label stack entry,
specified
      in [RFC3032].  This label value does not include the encodings
of
      Traffic Class and TTL (time to live).  The label range
specified
      by this type covers the general use values and the
special-purpose
      label values.  An example usage can be found in
      [I-D.ietf-mpls-base-yang].

SB> I am surprised that you don't start with label and then define the

SB> other label definitions in terms of existing definitions.
SB> The obvious order being label, sp-label, gp-label, generalized

========

S/ "This identity represents IPv4 address family.";/"This identity
represents the IPv4 address family.";

========

     //The rest of the values deinfed in the IANA registry
SB> s/deinfed/defined/

SB> However a question arises, the list stops at mt-v6
SB> Why do you stop at this point in the IANA list?
SB>
https://www.iana.org/assignments/address-family-numbers/address-family-numbers.xhtml
SB> It cannot be because some of the later ones are less relevant, as
some of the 
SB> included ones are rather rare. One the other hand there are some
later ones that 
SB> seem modern and useful.
SB>
SB> Also why do you have types in this list that you do not later
define in detail?

===========

SB> You have generalized label in the list top, but not in
SB> the YANG model itself.

===========

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Wed, 3 May 2017 17:46:45 +0000

Hi Stewart,

Thanks for review. We need to publish a new version of this draft as we
have signification changes in our GitHub repository.

>suggestion that you may wish to assign an additional reviewer.

There is no paucity of YANG reviewers for this document.
>SB> for example alphabetical and/or dependency order.

They are grouped by functionality with functionally similar types being
grouped together. This is similar in organization to a C header file.

>      Router Identifiers are commonly used to identify a nodes in

Will fix in next revision (if not already fixed in the git repository).

>      found in [I-D.ietf-idr-bgp-model].

Will fix in next revision (if not already fixed in the git repository).

>SB> I would have thought that both should be in the same place.

IP/IPv6 addresses are already defined in RFC 6991 and are imported by this
module. 
>SB> they are defined elsewhere.

Nobody has suggested these heretofore. If you provide types and where they
would be used, we will strongly consider including them.

>SB> The obvious order being label, sp-label, gp-label, generalized

YANG Identities are very extendable but somewhat limited in semantics. For
example, a base identity is “a new globally unique, abstract, and untyped
identity.” So, the sp-label identities cannot have a base type of label
(RFC 7950). I have wondered why the can’t have a type myself.

>SB> Also why do you have types in this list that you do not later

I have included the rest of the IANA defined AFs in the GitHub version.
There will be no other YANG description of these address families.
However, I have embellished these a bit to at least have the full
expansion of acronyms. These will be moved to a separate
iana-routing-types module in the same draft.

It is on page 16. 

Thanks,
Acee