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

[Robert Wilton via Datatracker <[e-mail]>]

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

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.

     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.

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.

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)?