Re: [mpls] Robert Wilton's Discuss on draft-ietf-mpls-base-yang-15: (with DISCUSS and COMMENT)

[Tarek Saad <[e-mail]>]

Hi Rob,

Thanks for your thorough review and comments. We have posted rev -16 (https://tools.ietf.org/html/draft-ietf-mpls-base-yang-16)  of the document and have addressed all your comments. 
Please see inline for responses… [TS]

Regards,
Tarek

On 9/10/20, 7:38 AM, "Robert Wilton via Datatracker" <[e-mail]> wrote:

    Robert Wilton has entered the following ballot position for
    draft-ietf-mpls-base-yang-15: 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-mpls-base-yang/



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

    Hi,

    Thank for you this YANG module.  It is great to see IETF progressing publishing
    more YANG configuration/management models, to thank you for the time and effort
    that you have put into this.

    I support Roman's discuss regarding the security considerations, but would also
    like to add one of my own:

    In my experience, having some instance data examples (e.g., see Appendix D of
    RFC 8022) greatly helps readers understand the structure of the YANG module and
[TS]:XXX

    get a good feel for how the YANG model is used.  Was adding instance data
    examples considered for this document?  Do the authors that think it would be
    possible to add some examples?

    I've also added some comments on the YANG model below that probably need to be
    addressed but I didn't want to overload the main discuss point.

    Regards,
    Rob


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

    I also have some comments on the YANG module itself:

      feature mpls {
        description
          "Indicates support for MPLS switching.";
        reference
          "RFC3031";
      }

    Is the MPLS feature really required?  I.e. is it plausible that a device would
    implement this YANG module without supporting MPLS switching?  If not, then
    just implementing the module should probably be sufficient without a separate
    feature.
[TS]: agreed and removed feature.

    Alternatively, it might be worth looking at the ISIS YANG module that define a
    feature to control with ISIS can be administratively controlled.  E.g.
    draft-ietf-isis-yang-isis-cfg-42, feature admin-control.  On a related point:

         augment /rt:routing:
           +--rw mpls {mpls}?
               ...
              +--rw interface* [name]
                 +--rw name                      if:interface-ref
                 +--rw enabled?                  boolean
                 +--rw maximum-labeled-packet?   uint32

    Is the "enabled" leaf definitely required here?  Isn't having the interface
    under the MPLS list sufficient to indicate that MPLS is enabled?  If you really
    want this leaf then I would change its name to "enable" rather than "enabled",
    change it's default to true, and consider putting it under a feature like
    admin-control from the ISIS YANG module.
[TS]: leave it - consistent with other modules.

         augment /rt:routing/rt:ribs/rt:rib/rt:routes/rt:route:
           +--ro mpls-enabled?         boolean {mpls}?
           +--ro local-label?          rt-types:mpls-label {mpls}?
           +--ro destination-prefix?   -> ../local-label {mpls}?
           +--ro route-context?        string {mpls}?

    Is "local-label" clearly enough associated with MPLS in the IP RIB?  E.g.
    should this be mpls-local-label, or under an mpls container? Presumably the
    mpls-enabled leaf shouldn't appear under the MPLS RIB?  It might be cleaner to
    split this into two augments, one for the MPLS RIB, and one for other RIBS.

[TS]: module-name will be always used as distinguisher - mpls:local-label (destination-prefix does not include ip-xx)

    The mpls-operations-type is defined, but not actually used by this module.  I
    wanted to check that this is expected.  I also note that the operations-type
    doesn't include a "no-operation" case statement which I thought might
    potentially be useful (but I'm not an MPLS expert).

    Regarding  grouping label-blocks:

    Ben's comment is right that the must statements associated with the
    start-label/end-label have the wrong sense.  There should also only be one of
    them (since they would always fail/pass in unison), and I would recommend just
    keeping the end-label must statement.
[TS]: we corrected the check.

    I wasn't sure whether the unique statement is really helpful.  Am I right in
    assuming that the blocks should be disjoint and non overlapping? If so, the
    unique statement doesn't achieve this, and although it could probably be done
    in a must statement, it would be tricky to express and get right.  However, if
    this is a constraint then it would certainly be helpful for the description of
    label-block to mention this.

    Regarding grouping rib-active-route-mpls-input:

    I know that this has been discussed with yang-doctors and the authors of the
    base RIB YANG module that is being extended but seeing the YANG I'm not quite
    sure we have the best outcome, since users of the RPC would be required to
    specify both destination-address and local-label as input parameters.  Possibly
    a better choice would to make this a choice so that a client could query either
    using a destination-address or a local-label (with both using the same type
    definition)?
[TS]: since mandatory is not set on it, I presume the user of RPC is ok to specify one.

Regards,
Tarek