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

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

Hi Ben, 

On 8/22/19, 1:46 AM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:

    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.

It does only include human-readable keys. Here is the RFC 7950 YANG ABNF definition.


   ;; any Unicode or ISO/IEC 10646 character, including tab, carriage
   ;; return, and line feed but excluding the other C0 control
   ;; characters, the surrogate blocks, and the noncharacters
   yang-char = %x09 / %x0A / %x0D / %x20-D7FF /
                               ; exclude surrogate blocks %xD800-DFFF
              %xE000-FDCF /    ; exclude noncharacters %xFDD0-FDEF
              %xFDF0-FFFD /    ; exclude noncharacters %xFFFE-FFFF
              %x10000-1FFFD /  ; exclude noncharacters %x1FFFE-1FFFF
              %x20000-2FFFD /  ; exclude noncharacters %x2FFFE-2FFFF
              %x30000-3FFFD /  ; exclude noncharacters %x3FFFE-3FFFF
              %x40000-4FFFD /  ; exclude noncharacters %x4FFFE-4FFFF
              %x50000-5FFFD /  ; exclude noncharacters %x5FFFE-5FFFF
              %x60000-6FFFD /  ; exclude noncharacters %x6FFFE-6FFFF
              %x70000-7FFFD /  ; exclude noncharacters %x7FFFE-7FFFF
              %x80000-8FFFD /  ; exclude noncharacters %x8FFFE-8FFFF
              %x90000-9FFFD /  ; exclude noncharacters %x9FFFE-9FFFF
              %xA0000-AFFFD /  ; exclude noncharacters %xAFFFE-AFFFF
              %xB0000-BFFFD /  ; exclude noncharacters %xBFFFE-BFFFF
              %xC0000-CFFFD /  ; exclude noncharacters %xCFFFE-CFFFF
              %xD0000-DFFFD /  ; exclude noncharacters %xDFFFE-DFFFF
              %xE0000-EFFFD /  ; exclude noncharacters %xEFFFE-EFFFF
              %xF0000-FFFFD /  ; exclude noncharacters %xFFFFE-FFFFF
              %x100000-10FFFD  ; exclude noncharacters %x10FFFE-10FFFF

However, this is the intent as this is support for implementations that don't support key-chains (RFC 8177). The "Security Considerations" recommend key chains. We'll add the hexadecimal specification as another reason for preferring key-chains. 

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

I've never heard any mention of counter discontinuities in the context of OSPF control plane counters. 
    
    
    ----------------------------------------------------------------------
    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.

Updated in -27.
    
    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.

Fixed in -27.
    
    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.

I removed this paragraph and augmented the next one.
    
    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.

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

The latter - fixed. 
    
         identity ospfv2-lsa-option {
           description
             "Baes idenity for OSPFv2 LSA option flags.";
    
    nit: "Base"

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

Made these consistent. This was added very late when we realized that YANG type bit is not extendable. 
    
         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?

Since this is returned operational data, it doesn't make a lot of sense to put constraints on it. It is basically what the router returns. If it is wrong, it wouldn't have been parsed correctly in the first place. 
    
         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.

I fixed the description on these to differential between the list uint32 flags and the identities.
    
         grouping ospf-router-lsa-bits {
           container rputer-bits {
    
    s/rputer/router/?

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

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

Good catch. This should be unit32 for OSPFv3. There were two instances of this. 
    
         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.

Fixed - "will not be".
    
         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?

Yes - I've improved.
    
           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.]

This is from the OSPF MIB (RFC 4750). It is not used functionally as one cannot guarantee differing LSDBs will not yield the same checksum. However, if the checksums are different, you can assure the databases are different. 
    
           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.

Fixed - "when advertised on".
    
           leaf lls {
           [...]
           container ttl-security {
    
    Should these have a default value?

I think 1 would be appropriate since only virtual-links and sham-links require more.
    
               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)?

If it is not human readable, how would it be configured? 
    
               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?

Fixed.
    
           container preference {
             description
               "Route preference configuration In many
                implementations, preference is referred to as
                administrative distance.";
    
    nit: missing sentence break?

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

I added ordering to the description of the spf-log and lsa-log. However, the id is a purely internal value to provide a uniqueness key for the YANG list. 
    
         grouping notification-neighbor {
           [...]
           leaf neighbor-ip-addr {
             type yang:dotted-quad;
             description "Neighbor address.";
    
    neighbors can only be identified by IPv4 addresses?

Changed to IP address type.
    
           leaf packet-source {
             type yang:dotted-quad;
    
    packet sources, too? (multiple times)
    
Changed to IP address type. 

           description
             "This notification is sent when aa neighbor
              state change is detected.";
    
    nit: s/aa/a/

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

Can you suggest some text?

Thanks,
Acee