Re: [mpls] Benjamin Kaduk's No Objection on draft-ietf-mpls-base-yang-15: (with COMMENT)

[Tarek Saad <[e-mail]>]

Hi Benjamin,
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/8/20, 6:43 PM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:

    [External Email. Be cautious of content]


    Benjamin Kaduk has entered the following ballot position for
    draft-ietf-mpls-base-yang-15: No Objection

    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://urldefense.com/v3/__https://www.ietf.org/iesg/statement/discuss-criteria.html__;!!NEt6yMaO-gk!ShQX5naGtiolElLjw1rNiM8siw7Iun8IyU2y_afo_a_mmGiaIiIqjIicHsT7ow$<https://urldefense.com/v3/__https:/www.ietf.org/iesg/statement/discuss-criteria.html__;!!NEt6yMaO-gk!ShQX5naGtiolElLjw1rNiM8siw7Iun8IyU2y_afo_a_mmGiaIiIqjIicHsT7ow$>
    for more information about IESG DISCUSS and COMMENT positions.


    The document, along with other ballot positions, can be found here:
    https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-mpls-base-yang/__;!!NEt6yMaO-gk!ShQX5naGtiolElLjw1rNiM8siw7Iun8IyU2y_afo_a_mmGiaIiIqjIgJouG_tg$<https://urldefense.com/v3/__https:/datatracker.ietf.org/doc/draft-ietf-mpls-base-yang/__;!!NEt6yMaO-gk!ShQX5naGtiolElLjw1rNiM8siw7Iun8IyU2y_afo_a_mmGiaIiIqjIgJouG_tg$>



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

    Before getting into the section-by-section comments, I'll note that
    there are a few things in the YANG module itself that seem awry to my
    untrained eye.

    Section 2.2

       The 'ietf-mpls' module defines the following identities:
       [...]
       label-block-alloc-mode:

    (nit?) We also define two identities based on this one; is the list of
    "defines the following identities" supposed to be comprehensive (vs.
    "high-level")?
[TS]: the style we follow is to keep it high-level and include descriptions of the base identity only.

       nhlfe-multiple-contents:

          A YANG grouping that describes a set of NHLFE(s) and their
          associated parameters as described in the MPLS architecture
          document [RFC3031].

    (editorial) Perhaps we could say a few more words about how these NHLFEs
    are related/why they are being grouped together?  (I do not recall such
    grouping being specifically covered in 3031.)

[TS]: added.

       rib-mpls-properties:

          A YANG grouping for the augmentation of MPLS label forwarding data
          to the generic RIB as defined in [RFC3031].

    nit(?): the "as defined in <reference>" seems more applicable to "MPLS
    label forwarding data" than "generic RIB".
[TS]: rephrased.
NEW:
    ¦ A YANG grouping for the augmentation of the generic RIB with MPLS
    ¦ label forwarding data as defined in [RFC3031].

    Section 2.3

       o  routes that cross-connect an MPLS local label to a Layer-3
          adjacency or interface - such as MPLS Segment-Routing (SR)
          Adjecency Segments (Adj-SIDs), SR MPLS Binding SIDs, etc. as
          defined in [RFC8402].

    nit(?): is there a semantic difference between "MPLS SR" and "SR MPLS"
    for the two types of route mentioned?
[TS]: no, they are same and we adopted SR MPLS for both.

    Section 2.5

      identity label-block-alloc-mode {
        description
          "Base identity label-block allocation mode.";
      }

    nit: missing "for".
[TS]: fixed.

      grouping nhlfe-multiple-contents {
        description
          "MPLS NHLFE contents.";

    nit: this description feels a little terse; is this the "generic" case
    or something similar?
[TS]: updated.
NEW:
description
¦ "A YANG grouping that describes a set of NHLFE(s) and their
¦  associated parameters as described in the MPLS architecture.
¦  This grouping is used when multiple next-hops are associated
¦  with the route.";

        leaf index {
          type string;
          description
            "A user-specified identifier utilised to uniquely
             reference the next-hop entry in the next-hop list.
             The value of this index has no semantic meaning
             other than for referencing the entry.";
        }
        leaf backup-index {
          type string;
          description
            "A user-specified identifier utilised to uniquely
             reference the backup next-hop entry in the NHLFE list.
             The value of this index has no semantic meaning
             other than for referencing the entry.";
        }

    I'm not sure I understand the semantics, here -- does 'index'
    authoritatively name the current entry with 'backup-index' being
    effectively a pointer to a different entry?  I don't see any leafs of
    type string in, e.g., rt-types:mpls-label-stack that this would be
    referring to.  If the backup-index is indeed just a pointer to a
    different entry, why is the string name used instead of a YANG
    reference?
 [TS]: correct. The backup-index serves as a pointer to another forwarding entry that serves as backup path. Every entry has a primary "index"and a "backup-index". The "backup-index" has a value of another entry "index".

    Also, what's a good reference for "backup" MPLS next-hops?  I don't see
    the string "backup" in either RFC 3031 or 3032.
[TS]: I found RFC4090 and RFC5714 as related.

      grouping interfaces-mpls {
          [...]
          leaf name {
            type if:interface-ref;
            description
              "The name of a configured MPLS interface.";
          }

    pedantic nit: is this really a "name"?
[TS]: updated as:
NEW:
description
¦ "A reference to the name of a interface in the system that is
¦  to be enabled for MPLS.";


            leaf start-label {
              type rt-types:mpls-label;
              must '. >= ../end-label' {
                error-message "The start-label must be less than or equal "
                            + "to end-label";
              }

    I think the sense of the YANG comparator is reversed, for both
    start-label (this) and end-label (not quoted).  (Also, IIUC, it's
    redundant to specify both, but harmless.)
[TS]: corrected the check:
NEW:
leaf start-label {
¦ type rt-types:mpls-label;
¦ must '. <= ../end-label' {
¦   error-message "The start-label must be less than or equal "
¦   ¦   ¦   ¦   + "to end-label";
¦ }

            leaf free-labels-count {
              when "derived-from-or-self(../block-allocation-mode, "
                 + "'mpls:label-block-alloc-mode-manager')";
              type yang:counter32;
              config false;
              description
                "Label-block free labels count.";

    I think that counter32 is semantically the wrong type, here (and for
    inuse-labels-count as well) -- IIRC the 'counter' types are for thigns
    like counting a particular type of event, and only ever monotonically
    increase.  Even if the free label count behaves monotonically (which is
    far from clear to me), wouldn't it be decreasing, not increasing?
[TS]: we updated to use gauge32 instead.

    Also, won't it always be true that free-labels-count +
    inuse-labels-count == end-label - start-label + 1?  I'm not sure why
    there's a need to introduce such redundant fields and the corresponding
    risk of internal inconsistency among them.

[TS]: yes, we removed since it was redundant.

        uses rib-mpls-properties {
          /* MPLS AF aaugmentation to native MPLS RIB */

    nit: s/aaugmentation/augmentation/

        uses rib-active-route-mpls-input {
          /* MPLS AF aaugmentation to native MPLS RIB */

    (ditto)
[TS]: thanks for spotting, corrected.

    Section 4

    Why is the rt:simple-next-hop augmentation listed but not the
    rt:next-hop-list augmentation?  IIUC they are functionally identical in
    terms of the sensitivity of information content.

    Also, it seems like the augmented RPC output is similarly sensitive to
    the readable nodes that are already mentioned.
[TS]: added.

    It looks like the main writeable nodes are the global
    configuration/state, and while it's perhaps defensible to say that these
    particular nodes are not sensitive, I did want to check what the
    behavior would be if (e.g.) the label-blocks subtree was overwritten.
    Would things just silently start using the new label block and keep
    working?
[TS]: Updated for RPCs and writable data nodes.

    Perhaps it would be too banal, but should we reference the security
    considerations of 3031/3032 as applying to MPLS usage?
[TS]: added.

    Section 7.1

    It's not clear that we reference RFC 8402 in a normative manner
    anywhere.
[TS]: section 2.3 lists Adj-SID and Binding-SID label(s) defined in RFC8402.

    Section 7.2

    I agree with the document shepherd that RFCs 3031 and 3032 naturally
    would be classified as normative references.  Similarly, RFC 7424 is
    refrenced by the YANG module itself, which usually indicates a normative
    reference.

[TS]: ok, moved to normative.

Regards,
Tarek