Re: [yang-doctors] Yangdoctors last call review of draft-ietf-mpls-ldp-yang-06

["Kamran Raza (skraza)" <[e-mail]>]

Hi Jan,

Thanks for your detailed review and apologies for the delayed addressing. 
We have also some other comments to address from other reviewers and are planning to spin a new rev right after new year.

Please see our reply inline below - tagged as [skraza]

On 2019-10-15, 8:07 AM, "Jan Lindblad via Datatracker" <[e-mail]> wrote:

    Reviewer: Jan Lindblad
    Review result: Ready with Issues
    
    Modules: ietf-mpls-ldp.yang, ietf-mpls-ldp-extended.yang
    Review result: Ready with Issues
    Reviewer: Jan Lindblad
    
    This is my YANG doctor LC review of draft-ietf-mpls-ldp-yang-06. I find the
    module ready with issues, even if there are a couple rather fundamental things
    to work out. It is evident that the authors have spent considerable time to
    make the module clean and tidy, which is why I can say it's more or less ready
    even when there are fundamentals to work out. In my judgement it may be
    possible to work out all remaining issues rather quickly.
    
    #1) Fitting mpls-ldp into the ietf-routing framework
    
    The modules at hand, ietf-mpls-ldp.yang and ietf-mpls-ldp-extended.yang, are
    augmenting into the ietf-routing module (RFC 8022). I find this highly
    appropriate. RFC 8022 has a section on how control plane protocols are meant to
    fit into the ietf-routing framework:
    
    5.3.2.  Defining New Control-Plane Protocols
    
       It is expected that future YANG modules will create data models for
       additional control-plane protocol types.  Such a new module has to
       define the protocol-specific configuration and state data, and it has
       to integrate it into the core routing framework in the following way:
    
       o  A new identity MUST be defined for the control-plane protocol, and
          its base identity MUST be set to "rt:control-plane-protocol" or to
          an identity derived from "rt:control-plane-protocol".
    ...
       o  Configuration parameters and/or state data for the new protocol
          can be defined by augmenting the "control-plane-protocol" data
          node under both "/routing" and "/routing-state".
    
    The current draft is doing neither of the above two points. IMHO, it should do
    both. Or if there is strong reason not to do that, incorporate a statement from
    the ietf-routing authors and a clear explanation of why the mpls-ldp related
    modules would be exempted. At present, this deviation from the RFC 8022
    architecture is never mentioned in the draft.
    
    Even if this issue falls under fundamental issues, the way I look at it, it
    would be quite easy to fix. Adjusting the two draft modules to comply with the
    points above requires changes to two dozen augment statement paths and a
    handful of leafref paths. Adding the identity and corresponding when derived
    from statement are a couple of lines.
    
    In principle, this change opens up for multiple instances of mpls-ldp. This
    might be useful if a system is used with two entirely separate networks (such
    as two separate customers), where each one might be running an mpls-ldp
    instance. If this is not desired, a must statement could be introduced to limit
    the number of instances to one.
    
[skraza]: We had discussed this in our prelim design mtgs at time of startup of this work and x-vendors
had agreed to limit our scope to single instance LDP/mLDP. So that’s why the current (non-list) hierarchy.
Having said, we are open to follow (list-based) hierarchy and limit to a single instance, as you suggested.
This is not most user friendly but we can do it.

    #2) Global level vs. local (e.g. interface or peer) level
    
    The module has several configurations that have a global and local level, such
    as configurations per peer or per interface. Presumably, these local level
    configurations are meant to override the global default when set and let the
    global default prevail when nothing specific is specified at the local level.
    
    The way this is being currently implemented in the modules, however, makes it
    impossible not to set a value at the local level, so the local setting will
    always prevail. The global configuration would have no effect at all (except in
    those cases where a local level configuration is marked with an if-feature and
    the server would not announce that feature).
    
    For example:
    
    On the global level, there is a graceful-restart/enable leaf:
    
          container global {
    ...
            container graceful-restart {
              leaf enable {
                type boolean;
                default false;
              }
    
    Further down, we have another graceful-restart/enable leaf, used per peer:
    
      grouping graceful-restart-attributes-per-peer {
    ...
        container graceful-restart {
          leaf enable {
            type boolean;
            default false;
          }
    
    I would assume this latter enable leaf should take precedence over the global
    graceful-restart when set on the peer. But if not set on the peer, the global
    graceful-restart value should be used. Since the peer graceful-restart/enable
    leaf has a default value, however, it will never happen that it is not set, and
    will thus render the global value completely irrelevant for all peers.
    
    The fix is simple: Do not have a default statement on the local leafs. Explain
    in the description statement what happens when the element has no value (the
    globally configured value will be used). Make sure the global leaf has a
    default (they all do already).
    
    The same applies to a rather long list of leafs in the module. I don't dare
    listing them here, because I would surely miss a few. My point is that the
    global/local relationships needs to be identified, local defaults removed. It
    would also make sense to mention how this is supposed to work in the
    descriptions.
    
[skraza]: Ack. We will remove “default” from local leafs and update the description of the local leaf.


    #3) Unlinked multikey leafrefs
    
    The grouping ldp-peer-ref has two leafrefs, one for each key in the list of ldp
    peers. Two leafrefs are exactly what you need to reference an entry in a list
    with two keys, so that's good. It is not enough to simply have two unlinked
    leafrefs, however. That would still allow them to have values that are valid
    separately, but not point to any entry in the list combined. If you have a copy
    of "Network Programmability with YANG", this is explained in detail on pp.
    451-454.
    
    To link the leafrefs, you'd need to update the path expression in the
    label-space-id leaf as follows:
    
      grouping ldp-peer-ref {
        description
          "An absolute reference to an LDP peer, by the LDP ID, which
           consists of the LSR ID and the Label Space ID.";
    
        leaf lsr-id {
          type leafref {
            path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
              + "ldp:peers/ldp:peer/ldp:lsr-id";
          }
          description
            "The LSR ID of the peer, as a portion of the peer LDP ID.";
        }
        leaf label-space-id {
          type leafref {
            path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
              + "ldp:peers/ldp:peer[lsr-id =
              current()/../lsr-id]/ldp:label-space-id";
          }
    
[skraza]: Looks like we missed this. Thanks for pointing out. We will fix this and x-chk others.

    The paths above would also have to be adjusted as part of fixing issue #1
    above. If this fixing results in allowing multiple mpls-ldp instances, the
    grouping above needs to be extended with one additional, linked leafref that
    points out which mpls-ldp instance is being targeted. Or if that is always the
    same instance as the reference is coming from, a reformulation of the paths
    above could ensure no mpls-ldp instance name would be required. In that case,
    the paths should be relative and never venture outside the mpls-ldp container.
    I would be happy to explain this further.
    
[skraza]: This is the unnecessary complexity that comes into play if we go with multi-instance. Isn't it?
But sure, we will address this as well.

    #4) Weakly defined types for neighbor-list-ref, prefix-list-ref, peer-list-ref
    
    These types are not leafrefs, but strings without any YANG substatements to
    define the format. The only thing the description does is to claim that the
    entities they refer to are outside the scope of this document. For an
    operator/programmer encountering this type, that isn't very helpful, and is not
    going to be interoperable.
    
      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.";
      }
    
    I'm not sure if this is fixable by sharpening the YANG module, but maybe more
    could be done to guide a confused reader. What would the user do to find out
    the format of this type and valid values? Add to the description.
    
[skraza]: At the time, there was no routing-policy model available for us – and hence the above.
Given that draft-ietf-rtgwg-policy-model now defines routing-policies with prefix-set, we would use those here and in all other places where we need to define a list of peers or list of prefixes.

    #5) Interfaces or mpls, who's on top?
    
    In the mpls-ldp modules, there are two interface lists and two leafrefs
    pointing to interfaces. This makes me wonder if some of the YANG structure
    would belong more naturally as an augment to ietf-interfaces instead? I mean,
    it doesn't sound fun to define the same interfaces in a lot of different
    places, just because a protocol needs some additional information. This is just
    a casual observation from an mpls-innocent guy, so you can disregard if you
    feel it's irrelevant.
    
[skraza]: This is a discussion about “interface-centric” vs “protocol-centric” config model.
We had also discussed this day1 of our modeling activity and had decided x-vendor to go with protocol-centric way.
We will keep it this way.

    #6) MD5 key
    
    There is a leaf md5-key of type string. Is this leaf sensitive from a security
    point of view? A plaintext string would not be ideal if that is the case.
    Choose a crypto type instead. Also, the format of this string is not defined.
    Hex without spaces? Explain the format in the description.
    
[skraza]:  Firstly, as far as we understand, the crypto does not define/cover “md5” authentication case.
Secondly, in our module we are dealing with the protocol/network message/connection authentications and not a user authentication. 
So not sure if the “crypto” define/procedures apply. Last but not least, the YANG for other routing protocols (BBGP, OSPF) seems to be doing/defining md5 authentication same way as us. 
Perhaps, we need a common solution/strategy at the RTG area level for routing protocols. If there already is one available, please let us know.


    ><