Subject: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Fri, 28 Feb 2020 04:39:14 +0000

Thanks for your detailed review, Ben. 
We have posted a new rev -08 that takes care almost all of your comments and comments from other IESG reviewers. 
On behalf of authors, Please see inline [skraza] for each of your comment/Q:

    ----------------------------------------------------------------------
    DISCUSS:
    ----------------------------------------------------------------------
    
    The YANG doctor last call review of the -06 notes that these modules are
    in violation of the expectations of RFC 8349 (at least w.r.t. defining a
    new identity for the control-plane protocol and possibly more).  This
    document should either be brought into compliance or explain why it
    diverges.

[skraza]: Fixed.  
    
    The YANG doctor's comments about default values for "local"
    configuration (e.g., graceful-restart configuration) causing the global
    configuration to never take effect should also be addressed.

[skraza]: Fixed. 
    
    Please include some justification for why LDP IPv6 is considered an
    "extended feature" (which is particularly surprising given that Section
    2 classifies "IP" to refer to both IPv4 and IPv6 together).
    
[skraza]:  This was an early decision taken and presented to WG and WG had agreed.
Moreover, Sec 1.1 does points out why LDP IPv6 is considered as an extended feature. The "base" features are the one most commonly implemented and deployed features.
The idea was that base must not contain any if-feature - given that LDPv6 is not widely implemented and was branded as if-feature, it is classified in the extended category then.
BTW, note that LDP has separate RFC for LDP IPv6. 

    We need to define the format/semantics of the md5-key string (e.g., is
    it hex? base64{url,}?) either directly or by reference (as the YANG
    doctor notes).  Using a crypto-type would probably be appropriate, as
    would adding a note that tcp-md5 is obsoleted by TCP-AO.
    
[skraza]: It is beyond our control as we are using it as-is from the RFC. Made the change anyways to add crypto-algo to widen the scope/type of key beyond MD5.

    In the peer-af-policy-container grouping's
    label-policy/advertise/prefix-list, we need to say if the filter is for
    inclusion or exclusion of outgoing label advertisements.
    Similarly for the incoming label advertisements in the 'accept'
    container (and most of the <foo>-list-ref usages?).
    
    In the policy-container grouping's
    label-policy/assign/independent-mode/prefix-list, the description
    suggests that the contents will provide not just a list of prefixes that
    act as a filter, but also a map from prefix to label.  This is a
    qualitatively different usage than the previous occurrences of
    prefix-list-ref, and it seems like a different type may be needed for
    it.  Similarly for ordered-mode.
    
[skraza]: Fixed as per latest changes that use rtgwg policy model constructs.

    (pro-forma) This document has 6 authors, and per RFC 7322, as this is
    more than five, we are supposed to consider whether that's appropriate.
    Seeing nothing in the shepherd writeup or similar, I mention it here.
    
[skraza]: We had taken an advise from Loa (MPLS WG chair) and would like to keep it to 6.
    
    ----------------------------------------------------------------------
    COMMENT:
    ----------------------------------------------------------------------
    
    Please also respond to the other YANG doctor, rtg-dir, and other
    directorate reviewers' comments.
    
    I support Roman's Discuss.
    
    I suggest making a reference to RFC 5920 at some point, perhaps from the
    security considerations.
[skraza]: Done.
    
    We have several instances of a "container ipv4" that's a "container with
    presence" in the jargon of RFC 7950.  As SEC AD one thing I look for is
    edge cases, such as when a subset of these containers are present but
    not all (and not none).  While for this specific case, it seems fairly
    natural to assume that IPv4 is enabled if any are present, it does make
    me wonder if the modeling is at an optimal state if there's even the
    possibility for such skewed signals.  (I didn't specifically audit for
    any other cases of "redundant" containers-with-presence.)
    
[skraza]: This is the same structure as RFC 8344. The edge cases are OK. The ipv4/ipv6 container may contain none, one, or more nodes in the container. ipv4/ipv6 address family is enabled as long as the container is “present”. Your assumption is correct, and the modeling (and the protocol as well) is in a good state, even with such possibilities. The signals should be clear enough.

    It's not entirely clear to me why we need (all of) the specialization of
    structures into using inet:ipv4-address and inet:ipv6-address as opposed
    to just inet:ip-address.  I do see that the latter is used in several
    places already, and so assume that this was already considered; perhaps
    some of the discussion/motivation could be included in the document,
    though.
    
[skraza]: When the type of an attribute is for a specific address family (ipv4 or ipv6), we use the address family specific type (inet:ipv4-address or inet:ipv6-address), so that user cannot provide a value of the wrong format.  inet:ip-address is simply a union of net:ipv4-address and inet:ipv6-address, and is used when the address family is not restricted. This is particular convenient when the user makes a query on a particular peer when the user does not have a particular address family in mind. The system will provide the address in a proper format.

    Section 1
    
       The data model is defined for following constructs that are used for
       managing the protocol:
    
    nit: "the following".
[skraza]: Fixed.    

    I share the YANG doctor's concern that separating the configuration and
    operational state constructs is not consistent with the NMDA model.
[skraza]: Fixed.
    
    Section 1.1
    
       The "base" category contains the basic and fundamental features that
       are covered in LDP base specification [RFC5036] and constitute the
       minumum requirements for a typical base LDP deployment.  Whereas, the
       "extended" category contains all other non-base features.  All the
    
    This statement ("all other") is not future-proof.
[skraza]:  Removed the "all".
    
    Section 3
    
       o  This module aims to address only the core LDP parameters as per
          RFC specification, as well as some widely deployed non-RFC
          features (such as label policies, session authentication etc).
          Any vendor specific feature should be defined in a vendor-specific
          augmentation of this model.
    
    [Even for widely-deployed non-RFC features we still need to provide a
    clear description of what semantics are being covered, whether
    in-document or via an external reference.]
    
[skraza]: The only non-RFC config was related to label policies - Reworked/enhanced the text.

       o  This model is a VPN Forwarding and Routing (VRF)-centric model.
          It is important to note that [RFC4364] defines VRF tables and
    
    side note: I would like the IETF to someday get to a point where "VPN"
    (the 'P' is for "private") is used only for protocols/deployments that
    provide confidentiality protection ("privacy") for the data being
    conveyed.  We are, of course, not there now, and I cannot insist on any
    change here.  But please consider whether an alternate phrasing would
    suffice, that includes just the "virtual" part but not "VPN".  Since we
    are mostly using "VRF" in the rest of the document, this is not as
    daunting as it sometimes is...
    
    Section 4
    
    It might be nice to have the tree show the ipv4 and ipv6 children in
    as similar an order as is possible (with the understanding that the
    semantics are not affected by the order of appearance, of course).
    
           |  +--rw address-families
           |  |  +--rw ipv4!
           |  |  |  +--rw enable?                           boolean
           |  |  |  +--ro label-distribution-controlmode?   enumeration
           |  |  |  +--ro bindings
           |  |  |  |  +--ro address* [address]
           |  |  |  |  |  +--ro address               inet:ipv4-address
           |  |  |  |  |  +--ro advertisement-type?   advertised-received
           |  |  |  |  |  +--ro peer
           |  |  |  |  |     +--ro lsr-id?           leafref
           |  |  |  |  |     +--ro label-space-id?   leafref
    
    Does this imply that there is only one peer per address?  Is that going
    to be a limitation for any deployment cases?

[skraza]: Not an issue. Note that a peer may have one or more bound addresses. 
    
              +--rw ldp-ext:session-downstream-on-demand
              |       {session-downstream-on-demand-config}?
              |  +--rw ldp-ext:enable?      boolean
    
    nit(?): missing '|' to connect down to ldp-ext:enable?
    Similarly for ldp-ext:max-wait a few lines later, and some other
    occurrences later in the document.

[skraza]: Some of these instances/typos were due to hand picked sub-tree. 
Not an issue anymore as we are printing the entire tree as generated by the tool.

    Section 5.1.2
    
    Am I reading this right that (e.g.) per-interface hello-holdtime is
    configurable as an extension and only the global hello-holdtime is
    present in the "base" module?

[skraza]: Yes.
    
    Section 7
    
    "NHLFE" is used only once in this document (and is not listed as
    "well-known" at
    https://www.rfc-editor.org/materials/abbrev.expansion.txt); please
    expand it out.
    
[skraza]: It is well known and listed in the above url. Please x-chk.

    Section 9
    
    Is 2018 still the right copyright year for the modules themselves?

[skraza]: 2020-02-25 rev now.

    Section 9.1
    
         typedef ldp-address-family {
           type identityref {
             base rt:address-family;
           }
    
    Why can't we use rt:address-family directly in the one container that
    uses this?
    
[skraza]: Fixed now.

           description
             "Duration represented as 32 bit seconds with infinite.";
    
    nits: "32-bit" (hyphenated), "with infinity".
[skraza]:  Fixed.    

         typedef downstream-upstream {
           type enumeration {
             enum downstream {
               description "Downstream information.";
             }
             enum upstream {
               description "Upstream information.";
             }
           }
           description
             "Received or advertised.";
         }
    
    nit: copy/paste on the description?
[skraza]:  Fixed.        

             leaf used-in-forwarding {
               type boolean;
               description
                 "'true' if the lable is used in forwarding.";
    
    nit: s/lable/label/
[skraza]:  Fixed.        
    
         grouping graceful-restart-attributes-per-peer {
           description
             "Per peer graceful restart attributes.
              On the local side, these attributes are configuration and
              operational state data. One the peer side, these attributes
              are operational state data reveiced from the peer.";
    
    nit: s/reveiced/received/
[skraza]:  Fixed.        
    
         grouping peer-state-derived {
           description
             "The peer state information derived from the LDP protocol
              operatoins.";
    
    nit: s/operatoins/operations/
[skraza]:  Fixed.            

         grouping peer-state-derived {
         [...]
           leaf next-keep-alive {
             type uint16;
             units seconds;
             config false;
             description "Time to send the next KeepAlive message.";
           }
    
    nit: this description text sounds like it's giving an absolute time, but
    given that it's a uint16 I presume it's "time [from now] until sending".
[skraza]:  Clarified.       

           container received-peer-state {
             config false;
             description
               "Operational state information learned from the peer.";
    
             uses graceful-restart-attributes-per-peer;
    
             container capability {
               description "Configure capability.";
    
    This is config-false; "Configure" in the description cannot be correct,
    though I suppose "Configured" could be.
    (Also applies to several of the child containers' descriptions.)
    
[skraza]:  Fixed all these.
  
           leaf up-time {
             type string;
             config false;
             description "Up time. The interval format in ISO 8601.";
    
    We probably should have some sort of reference for ISO 8601 (though of
    course it cannot actually *be* a <xref> in the YANG module itself)
    
[skraza]:  Changed to use timeticks64 now.

           leaf notification {
             type yang:counter64;
             description
               "The number of messages sent or received.";
    
    Is this the same as or different than "leaf total-messages"?
[skraza]:  Fixed.    

                   leaf label-distribution-controlmode {
    
    nit: any reason to not hyphenate control-mode as well?
[skraza]:  Fixed.        

               container interfaces {
                 description
                   "A list of interfaces for LDP Basic Descovery.";
    
    nit: s/Descovery/Discovery/
[skraza]:  Fixed.    
    
             container discovery {
               description
                 "Neibgbor discovery configuration and operational state.";
    
    nit: s/Neibgbor/Neighbor/
[skraza]:  Fixed.    
    
                       // ipv4
                       container hello-adjacencies {
                         config false;
                         description
                           "Containing a list of hello adjacencies.";
    
                         list hello-adjacency {
                           key "adjacent-address";
                           config false;
                           description "List of hello adjacencies.";
    
                           leaf adjacent-address {
                             type inet:ipv4-address;
                             description
                               "Neighbor address of the hello adjacency.";
                           }
    
    I'm not an expert here, but I'm not really seeing why we need to
    specialize this container for v4 vs v6 as opposed to just using
    inet:ip-address and having something that's applicable to both.

[skraza]: We could. There are pros and cons either way. The reasons to use separate containers in this model are:
1) Operators usually plan and configure IPv4 and IPv6 separately, rather than mixing the address family together. The separate containers provide more focus and clear separations.
2) Often times, there are attributes with different types for different address families. Mixing two together brings up some modeling difficulties to specifies differently for different address families.
3) Sometimes, there are attributes that are applicable to one address family but not the other. Mixing the two containers together would require some difficult conditions, which would complicate the model and make the model less usable.
4) Mixing the two containers can make the YANG file more compact, but not the configuration data and operational state data.
5) The existing RFCs use the style of separate containers, such as RFC 8344 and RFC 8349.
    
                       leaf adjacent-address {
                         type inet:ipv4-address;
                         description
                           "Configures a remote LDP neighbor and enables
                            extended LDP discovery of the specified
                            neighbor.";
    
    Given that there's a sibling leaf "enable", do we still want "and
    enables" here?
[skraza]: We don’t need “enables” here. Removed it.

    Should peer/capability have a comment in its description that the
    container exists to be augmented by other (extension) modules?
    
[skraza]: Yes. It is better. Added as suggested.

         notification mpls-ldp-fec-event {
    
    Am I reading this right that we only generate notifications on 0->1 and
    1->0 transitions of "number of prefixes active for a given FEC"?  Would
    we ever want to know about addition/removal of prefixes that do leave
    the FEC up?
    
[skraza]: Here, the prefix is the FEC. Addition/removal of a prefix is equivalent to the addition/removal of the FEC. Changed the leaf name from prefix to fec to avoid the confusion.

    Section 9.2
    
    I can't find a pattern into which if-feature statements go into
    containers used to augment things vs. the augment directives themselves.
    Can we be more consistent (or am I missing the pattern)?
    
[skraza]: It is not intended to set up a strict pattern between feature name and containers, while the purpose is to provide more descriptive feature names. That said, the parent container names are often used to provide the contexts for the feature name. e.g. In the case of “dual-stack-transport-pereference”, it seems that the parent container name “peers” is also beneficial, so we have the feature name “dual-stack-transport-preference” to “peers-dual-stack-transport-preference” to make the feature name fit better with the pattern and more descriptive.

         feature dual-stack-transport-pereference {
           description
             "This feature indicates that the system allows to configure
              the transport connection pereference in a dual-stack setup.";
    
    nit: s/pereference/preference/ (twice)
[skraza]: Fixed.
    
         feature policy-targeted-discovery-config {
           description
             "This feature indicates that the system allows to configure
              policies to control the acceptance of targeted neighbor
              discovery hello messages.";
    
    nit: s/hello/Hello/
    [skraza]: Fixed.

         typedef neighbor-list-ref {
           type string;
           description
             "A type for a reference to a neighbor address list.
              The string value is the name identifier for uniquely
              identifying the referenced address list, which contains a list
              of addresses that a routing policy can applied. The definition
              of such an address list is outside the scope of this
              document.";
    
    If I'm reading correctly between here and where this is used, the
    semantics of this are better described as "implementation-specific" --
    that is, this is a label that refers to some local configured state,
    specifically, a list of neighbor addresses used as an ACL.  (Emphasis on
    *local* -- it seems like it does not need to interact with anything
    else.)
    I do sympathize with the YANG doctor, though -- as-is, this feels
    underspecified.
    (BTW, IP ACLs are not considered to be a security measure by the broader
    security community.)
    Similarly for the subsequent <foo>-list-refs.

[skraza]: We do understand the concerns, and have fixed these two types by using the https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model, which was not available when we initially started to work on this model.
    
             container interfaces {
               description
                 "A list of interfaces on which forwarding is disabled.";
    
    nit: given the separate 'ldp-disable' leaf, perhaps s/is/can be/ is in
    order.
[skraza]: Understand the confusion. Changed as suggested.

    Also, the description of the list itself implies additional
    functionality, of listing the interfaces used for basic discovery.
    
[skraza]: The description was wrong. We don’t intend to use this list for discovery. Fixed.

         grouping graceful-restart-augment {
           description "Augmentation to graceful restart.";
    
           leaf helper-enable {
             if-feature graceful-restart-helper-mode;
             type boolean;
             default false;
             description
               "Enable or disable graceful restart helper mode.";
    
    Where is this "helper mode" documented?
[skraza]: This is not an RFC defined term but commonly known and used when GR timer is set to 0.
  Fixed the description in yang to highlight this.
    
         augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
           + "ldp:global" {
           description "Graceful forwarding nexthop augmentation.";
           uses global-forwarding-nexthop-augment;
         }
    
    nit: I don't think "Graceful" is correct, in the description.
    [skraza]: Removed it.

    Why is (the augmented)
    /rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:address-families/ipv6/label-distribution-controlmode
    'config false'?  Doesn't RFC 5036 say that an LSR might support either
    as a configuration option?
    
[skraza]: Yes, but no implementation supports such a cfg as change of such a mode is a massive disruptive operation - all implementation pick one mode as the default and only supported mode and stick to it.
  The LDP YANG design team had discussed this early enough and agreed to not provide this a cfg option.

               leaf adjacent-address {
                 type inet:ipv6-address;
                 description
                   "Configures a remote LDP neighbor and enables
                    extended LDP discovery of the specified
                    neighbor.";
    
    As for the ipv4 case, do we still want "and enables" here?
[skraza]: Fixed by removing it.    

         augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
           + "ldp:discovery/ldp:interfaces/ldp:interface/"
           + "ldp:address-families/ldp-ext:ipv6" {
           description "Interface address family IPv6 augmentation.";
           uses interface-address-family-ipv6-augment;
    
    Didn't we just create .../ldp:address-families/ldp-ext:ipv6 with an
    earlier augmentation in this module?  (Similarly for
    /rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:address-families/ldp-ext:ipv6
    which we augment with "uses peer-af-policy-container".)
    
[skraza]: Yes. It is ok to augment a node which is part of another augmentation. The result is to add them all together. Doing so is to avoid too much irregular patterns, so that we have more similar construction between IPv4 and IPv6.

    Appendix A
    
                     "is-router": [null],
    
    This looks weird to me ("array containing one element with value literal
    null") -- why does the array come into play?
    
    [Xufeng]: Here is a little explanation to the seemingly confusing notation: the pair of square brackets does not indicate a normal array. Instead, it is a special notation to indicate that the value of a “empty” type leaf is “present” (Section 6.9. of RFC 7951). Semantically the line above says that “(the neighbor) is a router.”