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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Thu, 21 Jan 2021 01:08:14 -0800

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

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 21 Jan 2021 22:19:35 +0000

Hi Ben, 
Thanks for your review. 

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

I agree that it is redundant. 

    Section 4

Fixed in a couple places. 

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

Agreed. Fixed. 

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

Fixed. 

    Section 8.3

         feature max-sid-depth {
           description

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

                   enum "dual" {
                     description

Fixed. 

             description

Agreed. 

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

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

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

    Section 9

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

    Section 12.1

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

Thanks,
Acee

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 14:41:45 -0800

Hi Acee,

>     https://datatracker.ietf.org/doc/draft-ietf-spring-sr-yang/

Thanks!  That was my suspicion, but it seems helpful to remove the
potential confusion.

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

I suspect that might lead to some awkwardness if something supports (for
example) both OSPF and IS-IS, but only supports MSD for one of them.  But I
didn't think it through very carefully, and trust you to pick something
reasonable.

> Don't do this in other YANG modules.  

Oops, my mistake -- I didn't notice that this was just the operational
data.

> If it were a binary structure, it would need a lot more structure. Let me

Thanks!

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

That all works for me.

Thanks for the updates and quick reply,

Ben

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 21 Jan 2021 23:04:55 +0000

Hi Ben, 
See one inline.

On 1/21/21, 5:42 PM, "Benjamin Kaduk" <[email-masked]> wrote:

    Hi Acee,

Let me talk to my co-authors on this one.

Thanks,
Acee

    Thanks!

    That all works for me.

    Ben