[Lsr] Benjamin Kaduk's Discuss on draft-ietf-ospf-yang-26: (with DISCUSS and COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-ospf-yang-26: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

(1) Can we check whether it's okay to use the yang "string" type for raw
cryptographic keys (e.g., ospfv2-key, ospfv3-key)?  My understanding was
that yang strings were limited to human-readable, but that the crypto
keys could be raw binary values.

(2) Do we need to say anything about how to indicate when there are
discontinuities for the various "counter" types?


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I'm happy to see the discussion around Roman's Discuss.

Section 1

   YANG [RFC6020][RFC7950] is a data definition language used to define
   the contents of a conceptual data store that allows networked devices
   to be managed using NETCONF [RFC6241].  YANG is proving relevant
   beyond its initial confines, as bindings to other interfaces (e.g.,
   ReST) and encodings other than XML (e.g., JSON) are being defined.

This text feels a bit stale at this point.

Section 2

   model varies among router vendors.  Differences are observed in terms
   of how the protocol instance is tied to the routing domain, how
   multiple protocol instances are be instantiated among others.

nit: the grammar here is a bit odd, with the comma suggesting the start
of a list but no "and" present.

Section 2.2

   The ospf module is intended to match to the vendor specific OSPF
   configuration construct that is identified by the local identifier
   'name'.

I don't really understand what this is intended to mean.

Section 2.7

hello-timer in the module claims to be a rt-types:timer-value-seconds16
but shows up in the tree(s) as a uint32.
Similarly, the wait-itmer is a rt-types:timer-value-seconds32, which
also shows up in the tree(s) as a uint32, which is perhaps more
reasonable but perhaps not entirely accurate.
Other 'timer' leafs seem to have similar issues.

Section 3

     feature ospfv2-authentication-trailer {
       description
         "Use OSPFv2 authentication trailer for OSPFv2
          authentication.";

Is the feature for "use" or "support for"?
(Similarly for the ospfv3 authentication features.)

     identity ospfv2-lsa-option {
       description
         "Baes idenity for OSPFv2 LSA option flags.";

nit: "Base"

     identity v2-p-bit {
       base ospfv2-lsa-option;
       description
         "Only used in type-7 LSA. When set, an NSSA
          border router should translate the type-7 LSA
          to a type-5 LSA.";

There seem to be a few "identity <foo>-bit" stanzas whose description do
not mention the named bit specifically (but many that do).  Do we want
to be consistent about doing so?  (Likewise for <foo>-flag.)

     grouping tlv {
       description
         "Type-Length-Value (TLV)";
       leaf type {
         type uint16;
         description "TLV type.";
       }
       leaf length {
         type uint16;
         description "TLV length (octets).";
       }
       leaf value {
         type yang:hex-string;
         description "TLV value.";

Is there a way to apply a constraint so the 'length' matches the
hext-string's length?

     grouping router-capabilities-tlv {

The various descriptions hereunder could perhaps benefit from section
references, as, e.g., two nodes named "informational-capabilities" may
otherwise require some effort to distinguish.  Well, aside from the fact
that one is currently listed as "capabilitiess" with two esses, which
seems unlikely to be intended.

     grouping ospf-router-lsa-bits {
       container rputer-bits {

s/rputer/router/?

         container te-opaque {
           [...]
           container link-tlv {
             description "Describes a singel link, and it is constructed
             of a set of Sub-TLVs.";

s/singel/single/

     grouping ospfv3-lsa-external {
       [...]
       leaf referenced-link-state-id {
         type yang:dotted-quad;

RFC 5340 section 2.2 implies that the Link State ID is going to be a
32-bit identifier that need not be represented as dotted-quad, as it
does not have addressing semantics.  (dotted-quad seems to be used for
Link-State-ID-shaped things elsewhere, too, though the preferred form
may be the union of dotted-quad and uint32.)

     grouping lsa-common {
       description
           "Common fields for OSPF LSA representation.";
       leaf decode-completed {
         type boolean;
         description
           "The OSPF LSA body was successfully decoded other than
            unknown TLVs. Unknown LSAs types and OSPFv2 unknown
            opaque LSA types are not decoded. Additionally,
            malformed LSAs are generally not accepted and are
            not be in the Link State Database.";

nit: "are not be" is not grammatical.

     grouping lsa-key {
       description
         "OSPF LSA key.";

This could maybe benefit from a more descriptive description; is this a
sort or lookup key, for example?

       container database {
         description "Container for per AS-scope LSA statistics.";
         list as-scope-lsa-type {
           [...]
           leaf lsa-cksum-sum {
             type uint32;
             description
               "The sum of the LSA checksums of the LSA type.";

[It's not entirely clear to me why this sum-of-checksums is a useful
thing to track, but it may not be this document's role to do so.
Though, perhaps we do need to say if the sum is computed as integers
modulo 2**32.]

       leaf transmit-delay {
         type uint16;
         units seconds;
         description
           "Estimated time needed to transmit Link State Update
            (LSU) packets on the interface (seconds). LSAs have
            their age incremented by this amount on advertised
            on the interface. A sample value would be 1 second.";

nit: "on advertised on" does not seem grammatical.

       leaf lls {
       [...]
       container ttl-security {

Should these have a default value?

           case ospfv3-auth-ipsec {
             when "derived-from-or-self(../../../../../../rt:type, "
               +  "'ospfv3')" {
               description "Applied to OSPFv3 only.";
             }
             if-feature ospfv3-authentication-ipsec;
             leaf sa {
                 type string;

I don't see RFC 4552 talking about names for SAs; where would this be
discussed (and, are they guaranteed to be human-readable)?

           leaf poll-interval {
             type uint16;
             units seconds;
             description
               "Neighbor poll interval (seconds) for sending OSPF
                hello packets to discover the neighbor on NBMA
                networks. This interval dictates the granularity for
                discovery of new neighbors. A sample would be 2 minutes
                for a legacy Packet Data Network (PDN) X.25 network.";

Maybe "2 minutes (120 seconds)" since the units are seconds?

       container preference {
         description
           "Route preference configuration In many
            implementations, preference is referred to as
            administrative distance.";

nit: missing sentence break?

For the spf- and lsa-logs, do we require that the 'id' is assigned in
any particular order, or do we just rely on the included timestamp(s)
for any time-ordering required by the consumer?

     grouping notification-neighbor {
       [...]
       leaf neighbor-ip-addr {
         type yang:dotted-quad;
         description "Neighbor address.";

neighbors can only be identified by IPv4 addresses?

       leaf packet-source {
         type yang:dotted-quad;

packet sources, too? (multiple times)

       description
         "This notification is sent when aa neighbor
          state change is detected.";

nit: s/aa/a/

Section 4

   Additionally, local specificationn of OSPF authentication keys and
   the associated authentication algorithm is supported for legacy
   implementations that do not support key-chains [RFC8177] for legacy
   implementations that do not support key-chains.  It is RECOMMENDED
   that implementations migrate to key-chains due the seamless support
   of key and algorithm rollover, as well as, the encryption of key
   using the Advanced Encryption Standard (AES) Key Wrap Padding
   Algorithm [RFC5649].

(Roman caught the nits, so I won't duplicate that.)
I expected to see something about keeping the actual key material
secret, as well.