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

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

Hi Tom,

Thanks for your review. We've just replied to Jan's review. 
Please see further below inline [skraza]:

On 2019-10-15, 12:38 PM, "tom petch" <[e-mail]> wrote:

    Jan
    
    It would likely help if you could also look at
    draft-ietf-mpls-mldp-yang-06
    which augments
    draft-ietf-mpls-ldp-yang-06
    with such as
    [widescreen needed]
    module: ietf-mpls-mldp
      augment
    /rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:capab
    ility:
    and
         augment "/rt:routing/rt:control-plane-protocols/"
           + "ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:received-peer-state/"
           + "ldp:capability/mldp:mldp" {
    i.e. there are another 20 or so augment which are affected by the
    changes you propose.
    
    Note that the prefixes are ldp: and mldp: so the identities for the
    control plane protocols are probably better as those and not mpls-ldp or
    mpls-mldp.  But is mldp a separate protocol in RFC8349 terms or just
    part of ldp? I do not know.
    
[skraza]: We had discussed this day-1 of LDP/mLDP YANG design mtgs and had x-vendor consensus to make mLDP a child/dependent of LDP.
   So we do not need a separate identity for this. 

    The mldp draft has just completed WGLC. I looked at it but gave up on it
    because the line lengths were too long to display on my screen.

[skraza]: We will also fix the draft .txt format at next publication of mldp draft.

    Tom Petch
    
    
    ----- Original Message -----
    From: "Jan Lindblad via Datatracker" <[e-mail]>
    To: <[e-mail]>
    Cc: <[e-mail]>; <[e-mail]>;
    <[e-mail]>
    Sent: Tuesday, October 15, 2019 1:07 PM
    Subject: [mpls] Yangdoctors last call review of
    draft-ietf-mpls-ldp-yang-06
    
    
    > 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.
    >
    > #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.
    >
    > #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";
    >       }
    >
    > 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.
    >
    > #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.
    >
    > #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.
    >
    > #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.
    >
    > ><
    >
    > _______________________________________________
    > mpls mailing list
    > [e-mail]
    > https://www.ietf.org/mailman/listinfo/mpls