Re: [spring] Benjamin Kaduk's No Objection on draft-ietf-spring-sr-yang-29: (with COMMENT)

["Acee Lindem (acee)" <[e-mail]>]

Hi Ben, 
Thanks for your review. 

On 1/21/21, 4:08 AM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:

    Benjamin Kaduk has entered the following ballot position for
    draft-ietf-spring-sr-yang-29: 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://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-spring-sr-yang/



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

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

I agree that it is redundant. 

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

Fixed in a couple places. 

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

It is the routing next-hop for MPLS PHP but that router would support SR. I'll add "MPLS" to the text. 

    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.

Agreed. Fixed. 

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

I agree that S-flag is an IGP encoding and need not be included in the example since it isn't discussed elsewhere. 

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

    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?

Agreed. I'll make it at least one protocol.

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

Fixed. 

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

Agreed. 

    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?

Since this is operational data, it doesn't make sense to add constraints and we
Don't do this in other YANG modules.  

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

If it were a binary structure, it would need a lot more structure. Let me
Improve the description. 

    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.

I'll add RFC 8446 and keep RFC 5246 since they are separate TLS versions. 

    Section 12.1

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

These references are normative in other YANG model RFCs. For example, see https://datatracker.ietf.org/doc/rfc8695/

Thanks,
Acee