YANG Data Model for Segment Routing
draft-ietf-spring-sr-yang-30

[[ questions ]]

[ section 8.3 ]

* On the bottom of page 18 there's a definition of an IS-IS system-id.
  draft-ietf-isis-yang-isis-cfg has a definition of a system-id as well as
  an extended-system-id.  Should this document reference that document's
  {extended-,}system-id definitions?

  I just wonder if this other document is somehow "more authoritative" for
  this type of thing, and therefore wondering whether referencing it would
  be the right thing to do.

Thanks for addressing my DISCUSS and COMMENT items.

(1) I have a significant concern about the incomplete representation of the 
MSD in this document.  Even though the model is incomplete, I am not balloting
DISCUSS because this point should be easy to address.

     grouping max-sid-depth {
       description
         "Maximum SID Depth (MSD) operational state grouping.";
       leaf node-msd {
         type uint8;
         description
           "Node MSD is the lowest MSD supported by the node.";
       }
       container link-msds {
         description
           "MSD supported by an individual interface.";
         list link-msds {
           key "interface";
           description
             "List of link MSDs.";
           leaf interface {
             type if:interface-ref;
             description
               "Reference to device interface.";
           }
           leaf msd {
             type uint8;
             description
               "MSD supported by the interface.";
           }
         }
       }
     }


(a) While there is a "type" mentioned, that seems to correspond to the
MSD-Value.  The MSD-Type is not included above.

(b) Note that the MSD is really a pair of MSD-Type/MSD-Value.  The description
above, even if extended to also include the MSD-Type, seems to allow for a
single pair, where multiple could be advertised per node/link.

(c) The ERLD (entropy-readable-label-depth, for which references should be
included) is advertised in the IGPs using the same mechanism as the MSD: using
the ERLD-MSD type.  IOW, the separation of the ERLD from the MSD in the module
is redundant/incorrect.

(d) MSD is a good example of a feature that is common to both the MPLS and 
IPv6 dataplanes and should be in the common part of the model, not defined 
separately for each.

(2) §3: "Module ietf-segment-routing-common defines generic types and 
groupings that SHOULD be reused by IGP extension modules."  The SHOULD is out 
place because the module is either supported or not, if it isn't then there is 
no effect on interoperability because the IGP extension module presumably 
chose a different option.  s/that SHOULD/to

(3) §3: There should be a representation of the ietf-segment-routing-common
module in this section.

If "srgb" already stands for "segment routing global block", what does
the extra "global" in "global-srgb" mean?

I support Roman's Discuss.

Section 4

   The module ietf-segment-routing-mpls augments the "/rt:routing/
   sr:segment-routing:" with a sr-mpls container.  This container
   defines all the configuration parameters related to segment-routing
   MPLS data plane.

nit: missing "the" ("the segment-routing MPLS data plane").

      *  Connected prefixes : maps connected prefixes to a segment ID.
         Advertisement of the mapping will be done by IGP when enabled
         for segment routing (see Section 5).  The SID value can be
         expressed as an index (default), or an absolute value.  The
         "last-hop-behavior" configuration dictates the PHP behavior:
         "explicit-null", "php", or "non-php".

Is the penultimate hop here a SR hop (a la PSP) or an underlying routing
protocol hop?

Section 5.1.1.1

   (i.e., a bundle of adjacencies).  The "advertise-adj-group-sid"
   configuration controls whether or not an additional adjacency SID is
   advertised.

nit: I think it is better to say "controls for which group(s) an
additional adjacency SID is advertised" -- the current wording implies
that there would only be one additional SID, but IIUC it is one
additional SID per group.

   both interfaces L3 and L4.  This will result in R1 advertising an
   additional Adj-SID for each adjacency, for example a Adj-SID with S
   flag set and value of 400 will be added to L1 and L2.  A Adj-SID with
   S flag set and value of 500 will be added to L3 and L4.  As L1/L2 and
   L3/L4 does not share the same "group-id", a different SID value will
   be allocated.

I don't think I remember where the 'S flag' is specified (or the
'B-flag' in the following section).  This is still in the
protocol-independent part of the model/document, right?  (Also, nit,
please consistently hyphenate or don't hyphenate the "X flag"
construction.)

Section 8.2

     grouping prefix-sid {
       description
         "This grouping defines cfg of prefix SID.";

nit: I think we can write out "config" or even "configuration" here.

Section 8.3

     feature max-sid-depth {
       description
         "Support for signaling MSD (Maximum SID Depth) in IGP.";
         reference "RFC 8476: Signaling Maximum SID Depth (MSD)
                              Using OSPF
                    RFC 8491: Signaling Maximum SID Depth (MSD)
                              Using IS-IS
                    RFC 8814: Singaling Maximum SID Deppt (MSD)
                              Using the Border Gateway Protocol
                              (BGP) - Link State";
     }

I feel like a few more words are in order -- do I claim the feature if I
inplement only one IGP's signaling?  Or do I need to have all three?  Or
"just" have support for it in all the IGPs that I support?

               enum "dual" {
                 description
                   "Two Adj-SIDs will be associated with the adjacency
                    if the interface is protected. In this case, will
                    be advertised with backup flag set, the other will
                    be advertised with the backup flag clear. In case
                    protection is not configured, single Adj-SID will
                    be advertised with the backup flag clear.";

nit: some missing words, maybe "In this case, one will", ", and the
other will", "a single Adj-SID will".

         description
           "Node MSD is the lowest MSD supported by the node.";

nit(?): "lowest link MSD"?

         list label-blocks {
           [...]
           leaf lower-bound {
           [...]
           leaf upper-bound {
           [...]
           leaf size {
           [...]
           leaf free {
           [...]
           leaf used {

Is there really all the internal redundancy between these that it sounds
like there is?  Would we benefit from any YANG-level constraints or
other enforced consistency checks?

     notification segment-routing-global-sid-collision {
       [...]
       leaf received-target {
         type string;
         description
           "Target received in the router advertisement that caused
            the SID collision.";

I'm not entirely sure that string is the right type here (it cannot
handle arbitrary binary strings and must be UTF-8) ... if it's a SID,
don't we normally represent those as uint32?  Or if it's a proper
route-target structure that seems like it would need to allow arbitrary
binary content.  (The original-target leaf would be similarly affected,
of course.)

Section 9

I believe at this point we're converging on using RFC 8446 as the TLS
reference (even though the nominal boilerplate template has had RFC
5246).  This would not imply a requirement to actually use TLS 1.3; it
just reflects that RFC 8446 is the current IETF reference for TLS.

Section 12.1

I guess RFC 3688 could probably be just an informative reference.
So could NETCONF and RESTCONF (6241 and 8040).