Fwd: [yang-doctors] YANG doctor review of draft-ietf-rtgwg-yang-rip-02

FYI.

Regards, B.

-------- Forwarded Message --------
Subject: 	[yang-doctors] YANG doctor review of draft-ietf-rtgwg-yang-rip-02
Date: 	Wed, 07 Dec 2016 17:01:19 +0100
From: 	Ladislav Lhotka <lhotka@nic.cz>
To: 	draft-ietf-rtgwg-yang-rip.all@ietf.org
CC: 	yang-doctors@ietf.org

Hi,

I was assigned to be the YANG doctor for this document. Here is my
review:

**** General comments

Overall, the module is in a good shape and integrates nicely with the
core routing data model [RFC 8022].

The I-D text is rather scarce, apart from the module it essentially
contains only boilerplate stuff. Some information about the context in
which this module is supposed to be used, design decisions etc. would
be certainly useful.

A non-normative appendix with a configuration example (instance data)
would also help people that are not fluent in YANG to understand what
data are included in this model.

Some parts of the model – "distribute-list", "redistribute", "bfd",
"authentication" on interfaces (or at least the "key" list) – are
probably not specific to RIP and could be used by other protocols as
well. If it is so, it would be better to model them separately as
general mechanisms.

Documents containing YANG modules that are imported by ietf-rip should
appear among normative references. One way to achieve this (which is
also useful by itself) is to include a table of namespace prefixes,
see e.g.

https://tools.ietf.org/html/rfc8022#section-2.3

**** Specific comments

      - Matching identities is done in the old YANG 1.0 way, for
        example:

        when "rt:type = 'rip:ripv2' or rt:type = 'rip:ripng'"

        This is fragile because the identities are matched based on
        string equality, so the result depends on the choice of the
        namespace prefix. Unless there is a strong reason not to use
        YANG 1.1, this is much simpler and more rubust

        when "derived-from(rt:type, 'rip:rip')"

      - In "redistribution", the must expressions are outright broken,
        e.g. for IS-IS:

        must "../../../../../rt:control-plane-protocol"
        + "[rt:name = current()]/type = 'isis'" { ... }

        First, the ietf-rip module also needs to import ietf-isis, and
        then the must statement can be rewritten like so:

        must "derived-from-or-self("
        + "../../../../../rt:control-plane-protocol"
        + "[rt:name = current()]/type, 'isis:isis')"

      - Some descriptions could mention that the parameter (feature
        etc.) only applies to RIP. For example, in

        feature interface-statistics {
          description
            "This feature indicates that the system supports collecting
             per-interface statistic data.";
        }

        I would add "... related to RIP." at the end.

      - Maybe it's absolutely clear to experts but the name and
        description of the "neighbor-configuration" feature look like
        the feature can make neighbors remotely configurable. I had to
        look up where it is used to find out what's the real
        meaning. Perhaps something like "explicit-neighbors" might be
        easier to understand?

      - The name of "no-supply" leaf looks strange. I checked a couple
        of CLIs and they use "passive", "passive-interface" or
        "send-options" for preventing RIP updates from being sent.

      - In "split-horizon" leaf, I would use "poison-reverse" enum
        rather than "poison".

Lada

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C

_______________________________________________
yang-doctors mailing list
yang-doctors@ietf.org
https://www.ietf.org/mailman/listinfo/yang-doctors