Subject: [Lsr] Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 01 Oct 2019 18:16:52 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-isis-yang-isis-cfg-40: 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-isis-yang-isis-cfg/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Perhaps the most minor thing that could be Discuss-level, and should be
trivial to resolve, but:

The "i-e" leaf in groupings prefix-ipv4-std and neighbor does not say
whether boolean value true corresponds to internal or external.

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

Section 1

   This document defines a YANG [RFC7950] data model for IS-IS routing
   protocol.

nit: "the IS-IS routing protocol"

Section 2.8

   This YANG module supports LFA (Loop Free Alternates) [RFC5286] and
   remote LFA [RFC7490] as IP Fast Re-Route (FRR) techniques.  The
   "fast-reroute" container may be augmented by other models to support
   other IP FRR flavors (MRT, TI-LFA, etc.).

If we're going to give examples of other flavors, do we want to have
informative references for them?  (This is particularly relevant since
we define enumeration values for them in the "alternate-type"
enumeration.)

Section 6

     identity lsp-attached-default-metric-flag {
         base lsp-flag;
         description "Set when originator is attached to
              another area using the referred metric.";

nit(?): I'm not sure whether the "referred" in the description is
appropriate given the "default" in the identity name.

     feature ldp-igp-sync {
       description
         "LDP IGP synchronization.";

nit: the surrounding context suggests that "Support for" would give a
more consistent style.  Maybe for auto-cost, te-rid, max-ecmp,
lsp-refresh, and admin-control as well.

     feature nlpid-control {
       description
         "This feature controls the advertisement
          of support NLPID within IS-IS configuration.";

nit: "support for"

     feature maximum-area-addresses {
       description
         "Support of maximum-area-addresses config.";

nit: s/of/for/

           leaf alternate-type {
             type enumeration {
               [...]
               enum other {
                 description
                   "Unknown alternate type.";

Do I remember correctly that enumerations are not extensible in the
future?  I don't know how relevant that would be here, though.

In the "protection-available" container, is there some sort of
constraint that two of the alternate-metrics add up to the third?

       container unprotected-routes {
         config false;
         list address-family-stats {

nit: the name/description of address-family-stats doesn't match up with
the parent container that just claims to be a list of unprotected
prefixes (no stats).

       list protection-statistics {

side note: I wonder whether the various uint32 leaves are better as
gauge32 than plain uint32.
Also, perhaps the description could mention a relationship bteween
total-routes/protected-routes+unprotected-routes, and/or
protected-routes/link-protected-routes+node-protected-routes.

     grouping route-content {
       [...]
       leaf-list tag {
         type uint64;
         description
           "List of tags associated with the route. The leaf
            describes both 32-bit and 64-bit tags.";

Are these the admin tags from RFC 5130?  Where is it specified that the
32- and 64-bit variants are just different views into a single
consolidated tag namespace?

     grouping authentication-global-cfg {

I see how "global" is in contrast to a smaller scope (hello, interface,
adjacency, etc.) both here and elsewhere, but when we have a global
defeault as well as per-level configuration that use the same grouping,
it ceases to be universally "global".  But, it's probably too late to be
worth changing so many names just for aesthetics...

       leaf poi-tlv {
         if-feature poi-tlv;
         type boolean;
         default false;
         description
           "Enable advertisement of IS-IS purge TLV.";

nit(?): I thought the purge origin identification TLV was an extension
to the generic purge capability but this description ("purge TLV") is
very generic.

Should any of the uint32 leaves in "packet-counters" instead be of type
counter32?

     grouping spf-log {
       [...]
           leaf id {
             type uint32;
             description
               "Event identifier -  purely internal value.";

Is there anything useful to say about the IDs being chronologically
ordered?  (Also applies to the other logs.)

       container delay-metric {
         leaf metric {
           type std-metric;
           description "IS-IS delay metric for IPv4 prefix";
         }
         leaf supported {
           type boolean;

Should the "metric" leaf be conditional on "supported" being true?
(Same for the other flavors of metric, as well as when they appear later
on in the "neighbor" grouping.)

       container expense-metric {
         leaf metric {
           type std-metric;
           description "IS-IS expense metric for IPv4 prefix";
         }
         leaf supported {
           type boolean;
           default "false";
           description
             "Indicates whether IS-IS delay metric is supported.";

nit: copy/paste-o delay vs. expense?

       container error-metric {
         [...]
         leaf supported {
           type boolean;
           default "false";
           description "IS-IS error metric for IPv4 prefix";

I think "Indicates whether [...] is supported" would be more consistent
with the sibling nodes.

     grouping prefix-ipv4-extended {
       [...]
       leaf up-down {
         type boolean;
         description  "Value of up/down bit.";

I assume true means "up", but we really ought to say...
(Also in prefix-ipv6-extended if not more)

       leaf ip-prefix {
         type inet:ipv4-address;
         description "IPv4 prefix address";
       }
       leaf prefix-len {
         type uint8;
         description "IPv4 prefix length (in bits)";

Doesn't RFC 6991 give us a combined ipv4-prefix type?  (I could imagine
that doing string parsing on it is less automation-friendly than the
representation here, of course...)

       leaf-list tag64 {
         type uint64;
         description
           "List of 32-bit tags associated with the IPv4 prefix.";

nit: s/32/64/

     grouping prefix-ipv6-extended {
       [...]
       leaf prefix-len {
         type uint8;
         description  "IPv4 prefix length (in bits)";

nit: s/4/6/

       leaf-list tag {
         type uint32;
         description
           "List of 32-bit tags associated with the IPv4 prefix.";
       }
       leaf-list tag64 {
         type uint64;
         description
           "List of 32-bit tags associated with the IPv4 prefix.";

s/IPv4/IPv6/; $s/32/64/

       container unidirectional-link-delay-variation {
         description
           "Container for the average delay variation
           from the local neighbor to the remote one.";
         leaf value {
           type uint32;
           units usec;
           description
             "Delay variation value expressed in microseconds.";

Is this a standard deviation (variance), mean absolute error, ...?

       container unidirectional-link-loss {
         [...]
         leaf value {
           type uint32;
           units percent;
           description
             "Link packet loss expressed as a percentage
             of the total traffic sent over a configurable interval.";

This document is all about specifying configuration.  How do I configure
the interval in question?

       container unidirectional-link-loss {
         [...]

Is there a relationship worth mentioning amongst the residual,
available, and utilized bandwidth?

           container expense-metric {
             leaf metric {
               type std-metric;
               description "IS-IS delay expense metric value";
             }
             leaf supported {
               type boolean;
               default "false";
               description "IS-IS delay expense metric supported";
             }
             description "IS-IS delay expense metric container";

Previously we just used "expense metric" instead of "delay expense
metric".

     notification lsp-too-large {
       uses notification-instance-hdr;
       uses notification-interface-hdr;

This is probably just for my education and not the document's sake, but
both these groupings include a leaf of type 'level' (though for
different names).  Are they just always going to have the same value?

       leaf reason {
         type string {
           length "1..255";
         }
         description
           "The system may provide a reason to reject the
            adjacency. If the reason is not available,
            an empty string will be returned.
            The expected format is a single line text.";

Wouldn't an empty string be zero-length (which is forbidden by the
length restriction)?

Section 7

I'm happy to see that several of the notifications mandate rate-limiting
their generation, in the description text, alleviating any concerns
about "spamminess" or DoS due to notification load!  It might be worth a
brief mention in the security considerations that that's why the
rate-limiting is in place.

   For IS-IS authentication, configuration is supported via the
   specification of key-chain [RFC8177] or the direction specification
   of key and authentication algorithm.  Hence, authentication

nit: s/direction/direct/

   configuration using the "auth-table-trailer" case in the
   "authentication" container inherits the security considerations of
   [RFC8177].  This includes the considerations with respect to the
   local storage and handling of authentication keys.

I'd consider also noting that the key-chain method is preferred (a
listing of why may not be needed and can probably be found in other
references).

Appendix A

Please use an address from the blocks reserved by RFC 5737 instead of
1.1.1.1, which is in actual use on the public Internet.

---

From: <[email-masked]>
Date: Tue, 8 Oct 2019 10:33:49 +0200

Hi Benjamin,

Thanks for your comment.
Please find some inline answers.
I'm doing the changes as part of the -41.

Stephane

-----Original Message-----
From: Benjamin Kaduk via Datatracker <[email-masked]> 
Sent: mercredi 2 octobre 2019 03:17
To: The IESG <[email-masked]>
Cc: [email-masked]; Yingzhen Qu <[email-masked]>; [email-masked]; [email-masked]; [email-masked]; [email-masked]
Subject: Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT)

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

Perhaps the most minor thing that could be Discuss-level, and should be trivial to resolve, but:

The "i-e" leaf in groupings prefix-ipv4-std and neighbor does not say whether boolean value true corresponds to internal or external.
[SLI] Right, I have added the following statement in the description:
"Set to false to indicate an internal metric"

Section 1

[SLI] Fixed

Section 2.8

If we're going to give examples of other flavors, do we want to have informative references for them?  (This is particularly relevant since we define enumeration values for them in the "alternate-type"
enumeration.)
[SLI] Sounds good, I have added some references.

Section 6

nit(?): I'm not sure whether the "referred" in the description is appropriate given the "default" in the identity name.

[SLI] Fixed

     feature ldp-igp-sync {
       description

nit: the surrounding context suggests that "Support for" would give a more consistent style.  Maybe for auto-cost, te-rid, max-ecmp, lsp-refresh, and admin-control as well.
[SLI] Fixed

     feature nlpid-control {
       description

nit: "support for"
[SLI] Fixed

nit: s/of/for/

[SLI] Fixed

           leaf alternate-type {
             type enumeration {
               [...]
               enum other {
                 description

Do I remember correctly that enumerations are not extensible in the future?  I don't know how relevant that would be here, though.

[SLI] Right, I have changed to identity 

In the "protection-available" container, is there some sort of constraint that two of the alternate-metrics add up to the third?
[SLI] It is not really a constraint from a YANG point of view, these are operational counters.

nit: the name/description of address-family-stats doesn't match up with the parent container that just claims to be a list of unprotected prefixes (no stats).

[SLI] I agree, I have changed the address-family-stats to "prefixes"

side note: I wonder whether the various uint32 leaves are better as
gauge32 than plain uint32.
Also, perhaps the description could mention a relationship bteween total-routes/protected-routes+unprotected-routes, and/or protected-routes/link-protected-routes+node-protected-routes.

     grouping route-content {
       [...]
       leaf-list tag {
         type uint64;
         description

Are these the admin tags from RFC 5130?  Where is it specified that the
32- and 64-bit variants are just different views into a single consolidated tag namespace?

[SLI] Right, I have added a better description.

I see how "global" is in contrast to a smaller scope (hello, interface, adjacency, etc.) both here and elsewhere, but when we have a global defeault as well as per-level configuration that use the same grouping, it ceases to be universally "global".  But, it's probably too late to be worth changing so many names just for aesthetics...

[SLI] I agree but yes it may be late to start changing this.

       leaf poi-tlv {
         if-feature poi-tlv;
         type boolean;
         default false;
         description

nit(?): I thought the purge origin identification TLV was an extension to the generic purge capability but this description ("purge TLV") is very generic.

[SLI] I have enhanced the description to tell that we are talking about the Purge Originator Identification TLV

Should any of the uint32 leaves in "packet-counters" instead be of type counter32?
[SLI] Agree

     grouping spf-log {
       [...]
           leaf id {
             type uint32;
             description

Is there anything useful to say about the IDs being chronologically ordered?  (Also applies to the other logs.)
[SLI] I have added a comment to tell that most recent events have the biggest id value.

       container delay-metric {
         leaf metric {
           type std-metric;

Should the "metric" leaf be conditional on "supported" being true?
(Same for the other flavors of metric, as well as when they appear later on in the "neighbor" grouping.)

[SLI] This is not a configuration leaf, so I don't think that there is a need for enforcement at YANG level.

[SLI] Fixed

       container error-metric {
         [...]
         leaf supported {
           type boolean;
           default "false";

I think "Indicates whether [...] is supported" would be more consistent with the sibling nodes.
[SLI] Fixed

[SLI] Fixed

       leaf ip-prefix {
         type inet:ipv4-address;

Doesn't RFC 6991 give us a combined ipv4-prefix type?  (I could imagine that doing string parsing on it is less automation-friendly than the representation here, of course...)
[SLI] Yes, but from a protocol point of view, IPv4 and IPv6 are not combined in the same datastructure. We wanted to keep them separated from a model point of view too.

       leaf-list tag64 {
         type uint64;
         description

nit: s/32/64/
[SLI] Fixed

nit: s/4/6/
[SLI] Fixed

       leaf-list tag {
         type uint32;
         description

s/IPv4/IPv6/; $s/32/64/
[SLI] Fixed

Is this a standard deviation (variance), mean absolute error, ...?
[SLI] We just mimic what is defined in RFC8570. As the reference is defined in the model. People should refer to it for more details.
Moreover this is a pure operational state.

This document is all about specifying configuration.  How do I configure the interval in question?
[SLI] This is an operational state.

Is there a relationship worth mentioning amongst the residual, available, and utilized bandwidth?
[SLI] Again, RFC8570 is the reference for additional details.

Previously we just used "expense metric" instead of "delay expense metric".

[SLI] Fixed to "expense metric"

This is probably just for my education and not the document's sake, but both these groupings include a leaf of type 'level' (though for different names).  Are they just always going to have the same value?

[SLI] The instance may be level-1-2, while the interface may be level-2 only.

       leaf reason {
         type string {
           length "1..255";
         }
         description

Wouldn't an empty string be zero-length (which is forbidden by the length restriction)?

[SLI] Fixed by allowing 0 length string

Section 7

I'm happy to see that several of the notifications mandate rate-limiting their generation, in the description text, alleviating any concerns about "spamminess" or DoS due to notification load!  It might be worth a brief mention in the security considerations that that's why the rate-limiting is in place.

[SLI] Sounds good.

nit: s/direction/direct/

[SLI] Fixed

I'd consider also noting that the key-chain method is preferred (a listing of why may not be needed and can probably be found in other references).

Appendix A

Please use an address from the blocks reserved by RFC 5737 instead of 1.1.1.1, which is in actual use on the public Internet.

[SLI] Fixed

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 8 Oct 2019 03:12:49 -0700

Hi Stephane,

Thanks for pulling in the fixes; just a few notes inline (and trimming)...

[...]
> [SLI] It is not really a constraint from a YANG point of view, these are operational counters.

Sorry, I shouldn't have used the word "constraint", since that's a YANG
verb.  I was just asking if the description should reiterate to the reader
that in normal circumstances the two will add up to the third (and "no" is
a fine answer).

[...]
> [SLI] This is not a configuration leaf, so I don't think that there is a need for enforcement at YANG level.

Oh, I had missed that it was config false; sorry about that.

[...]
> Moreover this is a pure operational state.

https://tools.ietf.org/html/rfc8570#section-4.3 still leaves me a little
unclear about what exactly is being reported (it sounds like it's just an
average link delay so the word "variation" confuses me), but I agree that
we should not say any more in this document.

> [SLI] This is an operational state.

The leaf value is operational state, yes, but it reports a valid computed
"over a configurable interval".  How do I configure that interval?

Thanks!

-Ben

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 8 Oct 2019 17:08:13 +0000

Hi Ben, 
Thanks for the excellent review - see one inline. 

On 10/8/19, 6:13 AM, "Benjamin Kaduk" <[email-masked]> wrote:

    Hi Stephane,

    [...]

    [...]

    [...]

    

I don't disagree that there is more configuration and state associated with these traffic metrics. However, these are not really under the purview of this model since IS-IS (and OSPF for that matter) are only serving as a mechanism to advertise these traffic metrics. 

Thanks,
Acee
    
    Thanks!
    
    -Ben