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

["Acee Lindem (acee)" <[e-mail]>]

Hi Stewart,

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

On 5/3/17, 10:54 AM, "Stewart Bryant" <[e-mail]> wrote:

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

There is no paucity of YANG reviewers for this document.
>
>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.

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


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

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

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

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

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

IP/IPv6 addresses are already defined in RFC 6991 and are imported by this
module. 
>
>========
>
>SB> In some protocols we use the NTP and the 1588 timer types, I
>assume
>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.

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

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.

>
>========
>
>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-num
>bers.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?

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.

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

It is on page 16. 

Thanks,
Acee 
>
>===========
>
>