YANG Data Model for Babel
draft-ietf-babel-yang-model-13

[[ nits ]]

[ appendix A ]

* "An Appendix" is cute.  :-)

  Consider perhaps "Example Configurations" or just "Examples",
  if you haven't already.

At the moment, the automated check is returning errors:

ietf-babel@2021-05-12.yang:148: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:157: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:175: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:176: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:186: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:187: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:206: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:207: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:214: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)
ietf-babel@2021-05-12.yang:215: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12)

Thank you to Loganaden Velvindron for the SECDIR review.

I'm clearing my discuss; I trust that it will be addressed.

Thank you Mahesh for the YANG syntax information, it is clear that the PYANG tool is plain wrong. I have cleared my DISCUSS position (but keeping the text below for archive purpose), I also noted that you replied/acted upon my original COMMENT 

Regards

-éric

--- Start of archive (to be ignored) ---

Thank you for the work put into this document. I really like the fact that the information model is built before the data model. Congratulations !

Please find below one blocking (but trivial to fix) DISCUSS point, some non-blocking COMMENT points (but replies would be appreciated), and one nit.

Thank you to Donald Eastlake for his shepherd's write-up (including the WG consensus).

I hope that this helps to improve the document,

Regards,

-éric

== DISCUSS ==

The YANG module does not compile correctly with PYANG, it should be easy to fix though :-) See:
https://yangcatalog.org/results/ietf-babel@2021-05-12_ietf.html
Or is it a PYANG error ?

== COMMENTS ==

The related links on should be updated. E.g., the YANG catalog entry should be:
https://www.yangcatalog.org/yang-search/module_details.php?module=ietf-babel@2021-05-12


-- Section 2.2 --
I usually use the expanded tree view rather the YANG module itself to get a global view. Is there any reason why the full tree view is not included?

-- Section 5 --
Is there any reason why the doc shepherd is not acknowledged ?

== NITS ==

-- Section 2.3 --
s/MAC based security/MAC-based security/ ?

§1: "It is based on the Babel Information Model [I-D.ietf-babel-information-model]."  The reference should be Normative.

Many thanks for the updates between the -10 and the -12; I'm happy to
report that they address my DISCUSS points (and all the COMMENT points
that I checked, as well)!

I'm terribly sorry to have failed to notice this previously, but when we
discuss the "cached_info" TLS extension, we mention its use in the
ClientHello and ServerHello messages.  However, for TLS 1.3 it seems
that this extension would probably not belong in the ServerHello, but
rather in the EncryptedExtensions message; unfortunately, this does not
actually seem to be formally specified anywhere (see
https://github.com/tlswg/tls13-spec/issues/1237).  If we're not
comfortable mentioning EncryptedExtensions by name, my suggestion would
be to make the change

OLD:
               "Indicates whether the cached_info extension is enabled.
                The extension is enabled for inclusion in ClientHello
                and ServerHello messages if the value is 'true'.";

NEW:
               "Indicates whether the cached_info extension is enabled.
                The extension is enabled for inclusion in TLS handshake
                messages if the value is 'true'.";

Appendix A

[Just noting that I did not review the new tree diagram or modified
examples, on the assumption that they were mechanically generated, and
the tool used to produce them is a more reliable cross-check against the
actual YANG model than a human (me).]

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 2.3, paragraph 4, nit:
-    connected to a wireless radio, the values can be overriden by setting
+    connected to a wireless radio, the values can be overridden by setting
+                                                            +

Section 2.1, paragraph 2, nit:
> model mandates the definition of some of the attributes, e.g. 'babel- implem
>                                  ^^^^^^^^^^^
If the text is a generality, 'of the' is not necessary.

Section 2.2, paragraph 6, nit:
> ddress. Finally, for security two subtree are defined to contain MAC keys an
>                                   ^^^^^^^
Possible agreement error. The noun 'subtree' seems to be countable, so consider
using: "subtrees".

Section 2.3, paragraph 4, nit:
> gorithm' to 'etx', and 'split-horizon' to false. Similarly, an interface that
>                                        ^^
Did you mean "too"?

Section 2.3, paragraph 49, nit:
> ects. Includes received and routes routes."; reference "RFC Z
>                             ^^^^^^^^^^^^^
Possible typo: you repeated a word

Section 2.3, paragraph 67, nit:
>  description "Indicates whether or not the split horizon optimization
>                         ^^^^^^^^^^^^^^
Consider shortening this phrase to just "whether". It is correct though if you
mean 'regardless of whether'.

Section 2.3, paragraph 86, nit:
>  the action is associated with the stats container inside an i
>                                    ^^^^^
An apostrophe may be missing.

Section 2.3, paragraph 92, nit:
>  "The multicast Hello history of whether or not the multicast Hello p
>                                  ^^^^^^^^^^^^^^
Consider shortening this phrase to just "whether". It is correct though if you
mean 'regardless of whether'.

Section 2.3, paragraph 93, nit:
>  "The unicast Hello history of whether or not the unicast Hello pac
>                                ^^^^^^^^^^^^^^
Consider shortening this phrase to just "whether". It is correct though if you
mean 'regardless of whether'.

Section 2.3, paragraph 105, nit:
>  MAC and include that MAC in the sent Babel packet. A MAC f
>                                  ^^^^
Did you mean "scent"?

Section 3, paragraph 1, nit:
> erations This document registers one URIs and one YANG module. 3.1. URI Regi
>                                  ^^^^^^^^
Don't use the number 'one' with plural words. Did you mean "one URI", "a URI",
or simply "URIs"?

Section 4, paragraph 8, nit:
> e whether packets are trusted. Some of the readable data or config false node
>                                ^^^^^^^^^^^
If the text is a generality, 'of the' is not necessary.

Section 4, paragraph 11, nit:
> ate credentials of the router. Some of the RPC operations in this YANG module
>                                ^^^^^^^^^^^
If the text is a generality, 'of the' is not necessary.

Section 4, paragraph 12, nit:
> nd their sensitivity/vulnerability from a RPC operation perspective: This
>                                         ^
Use "an" instead of 'a' if the following word starts with a vowel sound, e.g.
'an article', 'an hour'.

These URLs in the document did not return content:
 * http://tools.ietf.org/wg/babel/WG
 * https://www.rfc-editor.org/info/rfcXXXX

(2.3)

leaf received-metric {
           type uint16;
           description
             "The metric with which this route was advertised by the
              neighbor, or maximum value (infinity) to indicate the
              route was recently retracted and is temporarily
              unreachable. This metric will be 0 (zero) if the route
              was not received from a neighbor but was generated
              through other means. At least one of
              calculated-metric or received-metric MUST be non-NULL.";
           reference
             "RFC ZZZZ: Babel Information Model, Section 3.6,
              RFC 8966: The Babel Routing Protocol, Section 2.1.";
         }

         leaf calculated-metric {
           type uint16;
           description
             "A calculated metric for this route. How the metric is
              calculated is implementation-specific. Maximum value
              (infinity) indicates the route was recently retracted
              and is temporarily unreachable. At least one of
              calculated-metric or received-metric MUST be non-NULL.";
           reference
             "RFC ZZZZ: Babel Information Model, Section 3.6,
              RFC 8966: The Babel Routing Protocol, Section 2.1.";
         }

I don't understand the relationship between these two. If the metric was calculated rather than received, why would the value be zero instead of NULL? Isn't a zero metric dangerous in a routing algorithm?

(4) "config true perspective"?