Subject: [Lsr] AD Review for draft-ietf-ospf-yang-21

From: Alvaro Retana <[email-masked]>
Date: Wed, 5 Jun 2019 15:26:23 -0700

Dear authors:

I just finished reading this document.  I have some comments in-line,
including a couple that I consider major -- I'll wait for whose to be
addressed before moving forward.

The datatracker reports 4 errors in the model, mostly related to
ietf-bfd-types.  Please take a look.

Thanks!

Alvaro.

[Line numbers from id-nits.]

...
81 1.  Overview
...
92   This document defines a YANG data model that can be used to configure
93   and manage OSPF and it is an augmentation to the core routing data
94   model.  If fully conforms to the Network Management Datastore
95   Architecture (NDMA) [RFC8342].  A core routing data model is defined
96   in [RFC8349], and it provides the basis for the development of data
97   models for routing protocols.  The interface data model is defined in
98   [RFC8343] and is used for referencing interfaces from the routing
99   protocol.  The key-chain data model used for OSPF authentication is
100   defined in [RFC8177] and provides both a reference to configured key-
101   chains and an enumeration of cryptographic algorithms.

[nit] s/If fully conforms/It fully conforms

103   Both OSPFv2 [RFC2328] and OSPFv3 [RFC5340] are supported.  In
104   addition to the core OSPF protocol, features described in other OSPF
105   RFCs are also supported.  These includes demand circuit [RFC1793],
106   traffic engineering [RFC3630], multiple address family [RFC5838],
107   graceful restart [RFC3623] [RFC5187], NSSA [RFC3101], and OSPF(v3) as
108   a PE-CE Protocol [RFC4577], [RFC6565].  These non-core features are
109   optional in the OSPF data model.

[nit] s/OSPF(v3) as a PE-CE Protocol/use as a PE-CE Protocol    Using
"OSPF(v3)" is confusing because it is not clear whether OSPFv3-only is
meant or both, but v2 is not mentioned...

...
138 2.1.  OSPF Operational State

140   The OSPF operational state is included in the same tree as OSPF
141   configuration consistent with Network Management Datastore
142   Architecture [RFC8342].  Consequently, only the routing container in
143   the ietf-routing model [RFC8349] is augmented.  The routing-state
144   container is not augmented.

[nit] s/consistent with Network/consistent with the Network

146 2.2.  Overview
...
154   module: ietf-ospf
155     augment /rt:routing/rt:control-plane-protocols/
156              rt:control-plane-protocol:
157       +--rw ospf
158             .
159             .
160          +--rw operation-mode?          identityref
161          +--rw af?                      identityref
162             .
163             .
164          +--rw areas
165          |  +--rw area* [area-id]
166          |     +--rw area-id                   area-id-type
167          |        .
168          |        .
169          |     +--rw virtual-links
170          |     |  +--rw virtual-link* [transit-area-id router-id]
171          |     |     .
172          |     |     .
173          |     +--rw sham-links {pe-ce-protocol}?
174          |     |  +--rw sham-link* [local-id remote-id]
175          |     |     .
176          |     |     .
177          |     +--rw interfaces
178          |        +--rw interface* [name]
179          |           .
180          |           .
181          +--rw topologies {multi-topology}?
182             +--rw topology* [name]
183                .
184                .

186   The ospf module is intended to match to the vendor specific OSPF
187   configuration construct that is identified by the local identifier
188   'name'.  The field 'version' allows support for OSPFv2 and OSPFv3.

[minor] The 'version' field doesn't appear in the tree above.  §2.3 says
the same thing with more details.  Perhaps take the sentence out of this
paragraph.

190   The ospf container includes one OSPF protocol engine instance.  The
191   instance includes OSPF router level configuration and operational
192   state.

[??] One of the things that is confusing me is the instance concept in the
model.  I understand instances in OSPF, and I see how the module is
organized around instances ("grouping instance-..."), including even
per-instance Router-ID.  But I don't see anywhere the definition of an
instance-id (except attached to an interface (§2.7) and specific to OSPFv3
(§3))...if wanting to configure (or read from) a specific instance, how
does the client differentiate?  [YANG is not my thing, so this may be
obvious to everyone else.  If it is, a quick pointer would be very
appreciated.]

...
209 2.4.  Optional Features
...
221   3.   explicit-router-id: Support explicit per-instance Router-ID
222        specification.

[minor] Is there a reference?  Should the document point to rfc5340/rfc6549
here?

...
234   8.   ttl-security: Support OSPF Time to Live (TTL) security check
235        suppression [RFC5082].

[minor] s/suppression/support    Right?

237   9.   nsr: Support OSPF Non-Stop Routing (NSR).

[minor] Reference?

...
242   11.  admin-control: Support Administrative control of the protocol
243        state.

[minor] This seems like a "basic" feature: enabling/disabling the
protocol.  Why was it decided to make it optional?  Are there
implementations that don't support enabling/disabling?

...
261   17.  ospfv2-authentication-trailer: Support OSPFv2 Authentication
262        trailer as specified in [RFC5709] or [RFC7166].

[minor] s/RFC7166/RFC7474

...
267   19.  ospfv3-authentication-trailer: Support OSPFv3 Authentication
268        trailer as specified in [RFC7474].

[minor] s/RFC7474/RFC7166

...
300 2.5.  OSPF Router Configuration/Operational State
...
308   module: ietf-ospf
...
336          +--rw enable?               boolean {admin-control}?

[nit] Perhaps the admin-control should be moved closer to the start of this
branch so it is clear what is being enabled/disabled.

...
464 2.6.  OSPF Area Configuration/Operational State
...
570          |     |     +--rw hello-interval?       uint16
571          |     |     +--rw dead-interval?        uint32
572          |     |     +--rw retransmit-interval?  uint16

[minor] Why aren't rt-types:timer-value-* used for the *-timer/interval
definitions?  I would have thought this was exactly the reason for the
definitions in rfc8294.

...
1054 2.9.  OSPF RPC Operations
...
1061   o  clear-neighbor: restart a particular set of OSPF neighbor.

[minor] Should the description be something like: "reset a set of OSPF
neighbors on a particular interface"?

1063     rpcs:
1064       +---x clear-neighbor
1065       |  +---w input
1066       |     +---w routing-protocol-name
1067       |     +     -> /rt:routing/control-plane-protocols/
1068       |     +         control-plane-protocol/name
1069       |     +---w interface?               if:interface-ref

...
1076 3.  OSPF YANG Module
...
1143        Editor:   Derek Yeung
1144                  <mailto:[email-masked]>
1145        Author:   Acee Lindem
1146                  <mailto:[email-masked]>
1147        Author:   Yingzhen Qu
1148                  <mailto:[email-masked]>
1149        Author:   Jeffrey Zhang
1150                  <mailto:[email-masked]>
1151        Author:   Ing-Wher Chen
1152                  <mailto:[email-masked]>
1153        Author:   Dean Bogdanovic
1154                  <mailto:[email-masked]>
1155        Author:   Kiran Agrahara Sreenivasa
1156                  <mailto:[email-masked]";

[minor] This list is not the same as what is in the front page of this
document.

...
1685     typedef if-state-type {
1686       type enumeration {
...
1712         enum backup {
1713           value "6";
1714           description
1715             "Interface Backup Designated Router (BDR) state.";
1716         }

[nit] s/backup/bdr   To match the other instances where a BDR is referenced.

...
2729       container network {
2730         when "derived-from-or-self(../../header/type, "
2731            + "'ospfv3-network-lsa')" {
2732           description
2733             "Only applies to Network LSAs.";
2734         }
2735         description "Network LSA.";

[nit] Perhaps network-lsa could be a better name.

...
3053     grouping lsa-common {
3054       description
3055           "Common fields for OSPF LSA representation.";
3056       leaf decoded-completed {
3057         type boolean;
3058         description
3059           "The OSPF LSA body is fully decoded.";
3060       }

[minor] What is decoded-completed indicating?  Does it indicate that no
errors where found, IOW, "successful decoding"?  Does it mean that the LSA
has been decoded (but not an indication of success, or not)?  Is it a
time-based indication; IOW, is there a chance that an LSA has not been
processed when retrieving the information...would that result in false??

...
3271     grouping instance-fast-reroute-state {
3272       description "IPFRR state data grouping";

[nit] s/IPFRR/IP-FRR   That is how the term is used everywhere else in this
document.

...
3541       leaf hello-interval {
3542         type uint16 {
3543           range "1..65535";
3544         }
3545         description
3546           "Interval between hello packets (seconds). It must
3547            be the same for all routers on the same network.
3548            Different networks, implementations, and deployments
3549            will use different hello-intervals. A sample value
3550            for a LAN network would be 10 seconds.";
3551       }

[minor] Some implementations support having a sub-second
hello-interval...but with a timer in seconds that can't be done.

...
3664                 leaf ospfv2-key {
3665                   type string;
3666                   description
3667                     "OSPFv2 authentication key. The
3668                      length of the key may be dependent on the
3669                      cryptographic algorithm. In cases where it is
3670                      not, a key length of at least 32 octets should
3671                      be supported to allow for interoperability
3672                      with strong keys.";

[major] Is there a reference for the part about the "key length of at least
32 octets should be supported to allow for interoperability with strong
keys"?

...
3719                 leaf ospfv3-key {
3720                   type string;
3721                   description
3722                     "OSPFv2 authentication key. The
3723                      length of the key may be dependent on the
3724                      cryptographic algorithm. In cases where it is
3725                      not, a key length of at least 32 octets should
3726                      be supported to allow for interoperability
3727                      with strong keys.";

[major] The same comment from ospfv2-key applies here.

[nit] s/OSPFv2 authentication key/OSPFv3 authentication key

...
3856           leaf priority {
3857             type uint8 {
3858               range "1..255";
3859             }
3860             description "Neighbor priority for DR election.";
3861           }

[minor] A range is included in this definition, but not in other priority
statements before...should a range be included in those other definitions?

[minor] Why is 0 not applicable here?

...
3936       leaf dead-timer {
3937         type uint32;
3938         units "seconds";
3939         config false;
3940         description "This timer tracks the remaining time before
3941                      the neighbor is declared dead.";
3942       }

[major] For *-timer: Is tracking the remaining time in seconds enough?  I
would assume that ms would be the right unit.  Why seconds?

...
4390       container preference {
4391         description "Route preference config state.";

[minor] Maybe it's just me, but I don't understand what this piece of the
model does.  It seems like it can be used to configure which type of route
is preferred: external over intra-area, for example...but that doens't make
too much sense to me.  Is that the intended use?  Can you please give me an
example of when this functionality could be used?

4392         choice scope {
4393           description
4394             "Options for expressing preference
4395              as single or multiple values.";
4396           case single-value {
4397             leaf all {
4398               type uint8;
4399               description
4400                 "Preference for intra-area, inter-area, and
4401                  external routes.";
4402             }
4403           }
4404           case multi-values {
4405             choice granularity {
4406               description
4407                 "Options for expressing preference
4408                  for intra-area and inter-area routes.";
4409               case detail {
4410                 leaf intra-area {
4411                   type uint8;
4412                   description
4413                     "Preference for intra-area routes.";
4414                 }
4415                 leaf inter-area {
4416                   type uint8;
4417                   description
4418                     "Preference for inter-area routes.";
4419                 }
4420               }
4421               case coarse {
4422                 leaf internal {
4423                   type uint8;
4424                   description
4425                     "Preference for both intra-area and
4426                      inter-area routes.";
4427                 }
4428               }
4429             }
4430             leaf external {
4431               type uint8;
4432               description
4433                 "Preference for AS external routes.";
4434             }
4435           }

4437         }
4438       }

...
4551       container stub-router {
4552         if-feature stub-router;
4553         description "Set maximum metric configuration";

4555         choice trigger {
4556           description
4557             "Specific triggers which will enable stub
4558              router state.";
4559           container always {
4560             presence
4561               "Enables unconditional stub router support";
4562             description
4563               "Unconditional stub router state (advertise
4564                transit links with max metric";

[minor] s//MaxLinkMetric [rfc6897]

...
5537 4.  Security Considerations
...
5552   There are a number of data nodes defined in ietf-ospf.yang module
5553   that are writable/creatable/deletable (i.e., config true, which is
5554   the default).  These data nodes may be considered sensitive or
5555   vulnerable in some network environments.  Write operations (e.g.,
5556   edit-config) to these data nodes without proper protection can have a
5557   negative effect on network operations.  For OSPF, the ability to
5558   modify OSPF configuration will allow the entire OSPF domain to be
5559   compromised including peering with unauthorized routers to misroute
5560   traffic or mount a massive Denial-of-Service (DoS) attack.  The
5561   security considerations of OSPFv2 [RFC2328] and [RFC5340] apply to
5562   the ietf-ospf.yang module as well.

[minor] s/OSPFv2 [RFC2328] and [RFC5340]/OSPFv2 [RFC2328] and OSPFv3
[RFC5340]

[major] How do the "security considerations of OSPFv2 [RFC2328] and
[RFC5340] apply to the ietf-ospf.yang module"?  rfc2328/rfc5340 only talk
about authentication of the protocol exchange...

...
5626 7.1.  Normative References
...
5679   [RFC4750]  Joyal, D., Ed., Galecki, P., Ed., Giacalone, S., Ed.,
5680              Coltun, R., and F. Baker, "OSPF Version 2 Management
5681              Information Base", RFC 4750, DOI 10.17487/RFC4750,
5682              December 2006, <https://www.rfc-editor.org/info/rfc4750>.
...
5731   [RFC5643]  Joyal, D., Ed. and V. Manral, Ed., "Management Information
5732              Base for OSPFv3", RFC 5643, DOI 10.17487/RFC5643, August
5733              2009, <https://www.rfc-editor.org/info/rfc5643>.
...
5745   [RFC5880]  Katz, D. and D. Ward, "Bidirectional Forwarding Detection
5746              (BFD)", RFC 5880, DOI 10.17487/RFC5880, June 2010,
5747              <https://www.rfc-editor.org/info/rfc5880>.

5749   [RFC5881]  Katz, D. and D. Ward, "Bidirectional Forwarding Detection
5750              (BFD) for IPv4 and IPv6 (Single Hop)", RFC 5881,
5751              DOI 10.17487/RFC5881, June 2010, <https://www.rfc-
5752              editor.org/info/rfc5881>.

[minor] I think these references can be Informative.

...
5880 7.2.  Informative References

[major] All the references in this section (except maybe rfc905) point at
functionality in the model...as the references above, these should be
Normative as well.

These documents are already in the Downref registry: rfc5309, rfc5443,
rfc5714, rfc6987.

The others are not, but I don't expect any issues in the IETF Last Call.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Sat, 22 Jun 2019 23:03:53 +0000

Hey Alvaro, 

Thanks for the review - we will address your comments in the -22 version. See inline. 

On 6/5/19, 6:26 PM, "Alvaro Retana" <[email-masked]> wrote:

    Dear authors:

    
    Thanks!
    
    Alvaro.

    ...
    81 1.  Overview
    ...

Fixed. 

Sure "OSPFv2 or OSPFv3" 

    ...

Fixed

    146 2.2.  Overview
    ...
    154   module: ietf-ospf

Yeah - I removed the sentence. I was thinking of adding it to the tree but it is not there is the full tree. 

I have removed all instances of "engine" from the text and model. Also, I've just added:

The ietf-ospf model defines a single instance of OSPF which may be instantiated as an OSPFv2 or OSPFv3 instance. Multiple instances are instantiated as multiple control-plane protocols instances.

At one time, the model did include multiple instances. However, this was the manifestation of a poor implementation choice for a particular implementation. 

    ...

No - explicit protocol instance router-id configuration is not standardized. Apparently, there are implementations that don't allow protocol instance configuration but rely on global router-id explicit configuration. We got input from implementations beyond the affiliations of the authors. 

    ...

Right - fixed. 

    [minor] Reference?

No IETF reference for OSPF NSR. However, I have a few patents in this area - should the model reference these? __

    ...

I'm happy to remove it. 

    ...

Yup. Fixed. 
    
    ...

How could I missed this... Fixed

    ...

Ok

    ...

I'd like to use these types universally. However, YANG seems to have a restriction that I can't further restrict the range. This really sucks. Will do what I can and make the best trade-off. 

    ...

Yes. Fixed. 
    
    1063     rpcs:

    ...
    1076 3.  OSPF YANG Module
    ...
    1143        Editor:   Derek Yeung

Sure. Fixed.

    ...
    
    [nit] s/backup/bdr   To match the other instances where a BDR is referenced

Good catch. Fixed. 

Thanks,
Acee

---

From: Alvaro Retana <[email-masked]>
Date: Mon, 24 Jun 2019 16:52:03 -0400

On June 22, 2019 at 7:03:58 PM, Acee Lindem (acee) ([email-masked]) wrote:

Acee:

Hi!

Thanks for the review - we will address your comments in the -22 version.
See inline.

I have a couple of replies.  Please take a look at the bottom because it
looks like you didn’t get (or missed) part of the review.

Thanks!

Alvaro.

...

[minor] Reference?

<acee> No IETF reference for OSPF NSR. However, I have a few patents in
this area - should the model reference these? __

If they are applicable to this document, then yes, please disclose them.
But NSR is not described here (so I wouldn’t think the patents apply),
which is why I was asking for a reference.   Right now the model says NSR
can be enabled/disabled, but it doesn’t explain what is being
enabled/disabled.  Given that, it is not clear what vendors can claim to
support, leading to potential incompatibilities and/or unintended behavior.

I would like to either see a description (but this document is probably not
the right place for that), a pointer to a description (which doesn’t have
to be an IETF document), or to have the support taken out.  If the WG
agreed to what NSR means, then I would be fine with transcribing some of
that here (from an existing mail thread, for example).

...

<acee> I'm happy to remove it.

I was not asking you to remove it…I was simply wondering why it is optional.

You seem to have removed it completely, but it is still in §2.5.

...

...

<acee> I'd like to use these types universally. However, YANG seems to have
a restriction that I can't further restrict the range. This really sucks.
Will do what I can and make the best trade-off.

I see you changed all, except retransmit-interval and restart-interval….
Just pointing it out in case you meant to.

...

...

<acee> Good catch. Fixed.

Thanks,
Acee

What about the rest of the comments??

https://mailarchive.ietf.org/arch/msg/lsr/LKvR2rfkPHJui_GMRerIdDNHkug

I don’t know if it’s me, but several people seem to not receive the full
length of my comments…but they are complete in the archive…so I’m not sure
that the problem is.  Please take a look at the archive — I have pasted
comments after line 1716 below.  I am changing the state to Revised ID
Needed.

Thanks!

Alvaro.

...

2729       container network {

2732           description

2734         }

...

3054       description

3057         type boolean;

3058         description

3060       }

...

...

3542         type uint16 {

3543           range "1..65535";

3544         }

3545         description

3551       }

...

3664                 leaf ospfv2-key {

3665                   type string;

3666                   description

3672                      with strong keys.";

...

3719                 leaf ospfv3-key {

3720                   type string;

3721                   description

3727                      with strong keys.";

...

3856           leaf priority {

3857             type uint8 {

3858               range "1..255";

3859             }

3861           }

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

...

4392         choice scope {

4393           description

4396           case single-value {

4397             leaf all {

4398               type uint8;

4399               description

4401                  external routes.";

4402             }

4403           }

4404           case multi-values {

4406               description

4409               case detail {

4410                 leaf intra-area {

4411                   type uint8;

4412                   description

4414                 }

4415                 leaf inter-area {

4416                   type uint8;

4417                   description

4419                 }

4420               }

4421               case coarse {

4422                 leaf internal {

4423                   type uint8;

4424                   description

4427                 }

4428               }

4429             }

4430             leaf external {

4431               type uint8;

4432               description

4434             }

4435           }

4437         }

4438       }

...

4555         choice trigger {

4556           description

4558              router state.";

4559           container always {

4560             presence

4562             description

...

...

...

...

...

...

...

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Wed, 26 Jun 2019 15:53:24 +0000

Hi Alvaro,
This may take a couple iterations. See inline.

From: Alvaro Retana <[email-masked]>
Date: Monday, June 24, 2019 at 4:52 PM
To: Acee Lindem <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, Stephane Litkowski <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: AD Review for draft-ietf-ospf-yang-21

On June 22, 2019 at 7:03:58 PM, Acee Lindem (acee) ([email-masked]<mailto:[email-masked]>) wrote:

Acee:

Hi!

I have a couple of replies.  Please take a look at the bottom because it looks like you didn’t get (or missed) part of the review.

Thanks!

Alvaro.

...

[minor] Reference?

<acee> No IETF reference for OSPF NSR. However, I have a few patents in this area - should the model reference these? __

If they are applicable to this document, then yes, please disclose them.  But NSR is not described here (so I wouldn’t think the patents apply), which is why I was asking for a reference.   Right now the model says NSR can be enabled/disabled, but it doesn’t explain what is being enabled/disabled.  Given that, it is not clear what vendors can claim to support, leading to potential incompatibilities and/or unintended behavior.

I would like to either see a description (but this document is probably not the right place for that), a pointer to a description (which doesn’t have to be an IETF document), or to have the support taken out.  If the WG agreed to what NSR means, then I would be fine with transcribing some of that here (from an existing mail thread, for example).

As you know, Non-Stop routing doesn’t require protocol extensions. I’ll look for a good definition .

...

Yes – the YANG tree. I’ve removed it in -23.

...
...

<acee> I'd like to use these types universally. However, YANG seems to have a restriction that I can't further restrict the range. This really sucks. Will do what I can and make the best trade-off.

I see you changed all, except retransmit-interval and restart-interval…. Just pointing it out in case you meant to.

These data nodes didn’t have a range of 1…65535 like the rtgwg type and the types don’t support a second range.  I may remove these types though since part of the type union for these types is a value that we don’t support (infinity).

...
...

Thanks,
Acee

I don’t know if it’s me, but several people seem to not receive the full length of my comments…but they are complete in the archive…so I’m not sure that the problem is.

Please take a look at the archive — I have pasted comments after line 1716 below.  I am changing the state to Revised ID Needed.

Thanks!

Alvaro.

...

2729       container network {

2732           description

2734         }

Fixed in -23.

...

3054       description

3057         type boolean;

3058         description

3060       }

[minor] What is decoded-completed indicating?  Does it indicate that no errors where found, IOW, "successful decoding"?  Does it mean that the LSA has been decoded (but not an indication of success, or not)?

I clarified this.

@@ -1955,10 +1955,14 @@ module ietf-ospf {

   grouping lsa-common {

     description

         "Common fields for OSPF LSA representation.";

-    leaf decoded-completed {

+    leaf decode-completed {

       type boolean;

       description

-        "The OSPF LSA body is fully decoded.";

+        "The OSPF LSA body was successfully decoded other than

+         unknown TLVs. Unknown LSAs types and OSPFv2 unknown

+         opaque LSA types are not decoded. Additionally,

+         malformed LSAs are generally not accepted and are

+         not be in the Link State Database.";

Is it a time-based indication; IOW, is there a chance that an LSA has not been processed when retrieving the information...would that result in false??

This wouldn’t be possible in any sensible implementation. Only LSDB only includes the binary form of the data and the API would decode the individual fields on an atomic version of the LSA.

...

[nit] s/IPFRR/IP-FRR   That is how the term is used everywhere else in this document.

Fixed.

...

3542         type uint16 {

3543           range "1..65535";

3544         }

3545         description

3551       }

[minor] Some implementations support having a sub-second hello-interval...but with a timer in seconds that can't be done.

This is a good thing. Sub-second hellos were a misguided proprietary feature implemented and then regretted by several prominent vendors.

...

3664                 leaf ospfv2-key {

3665                   type string;

3666                   description

3672                      with strong keys.";

[major] Is there a reference for the part about the "key length of at least 32 octets should be supported to allow for interoperability with strong keys"?

No. We should remove this and use what we finally settled on the key-chain (RFC 8177). As a co-author, I wanted to defer to key-chains but there are implementations that allow key specificatons in the protocols (actually most of them support this at least for backward compatibility

...

3719                 leaf ospfv3-key {

3720                   type string;

3721                   description

3727                      with strong keys.";

Same response applies.

Fixed in -23

...

3856           leaf priority {

3857             type uint8 {

3858               range "1..255";

3859             }

3861           }

[minor] A range is included in this definition, but not in other priority statements before...should a range be included in those other definitions?

This will be removed in -23.

It is applicable and indicates the router is not eligible for DR/BDR election. The range will be removed.

Refer to appendix C.5 in RFC 2328. All neighbors on NBMA network are to be configured (eligible and non-eligible).

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

[major] For *-timer: Is tracking the remaining time in seconds enough?  I would assume that ms would be the right unit.  Why seconds?

Because sub-second hellos was a bad idea – three words: B-F-D…

...

[minor] Maybe it's just me,

Yeah – I think you need to refresh your CCIE certification.

but I don't understand what this piece of the model does.  It seems like it can be used to configure which type of route is preferred: external over intra-area, for example...but that doens't make too much sense to me.  Is that the intended use?  Can you please give me an example of when this functionality could be used?

I’m going to add a reference to RFC 8349 route-preference and also add to the description that this is “also known as, Administrative Distance”.

4392         choice scope {

4393           description

4396           case single-value {

4397             leaf all {

4398               type uint8;

4399               description

4401                  external routes.";

4402             }

4403           }

4404           case multi-values {

4406               description

4409               case detail {

4410                 leaf intra-area {

4411                   type uint8;

4412                   description

4414                 }

4415                 leaf inter-area {

4416                   type uint8;

4417                   description

4419                 }

4420               }

4421               case coarse {

4422                 leaf internal {

4423                   type uint8;

4424                   description

4427                 }

4428               }

4429             }

4430             leaf external {

4431               type uint8;

4432               description

4434             }

4435           }

4437         }

4438       }

...

4555         choice trigger {

4556           description

4558              router state.";

4559           container always {

4560             presence

4562             description

Fixed in -23 – RFC 6987.

...

...

[minor] s/OSPFv2 [RFC2328] and [RFC5340]/OSPFv2 [RFC2328] and OSPFv3 [RFC5340]

[major] How do the "security considerations of OSPFv2 [RFC2328] and [RFC5340] apply to the ietf-ospf.yang module"?  rfc2328/rfc5340 only talk about authentication of the protocol exchange...

Well, whoever would want fewer security considerations… However, I tend to agree and will remove this. I also plan to add the text regarding keys.

...

...

...

...

5752              editor.org/info/rfc5881<http://editor.org/info/rfc5881>>.

I agree.

...

[major] All the references in this section (except maybe rfc905) point at functionality in the model...as the references above, these should be Normative as well.

I agree.

These documents are already in the Downref registry: rfc5309, rfc5443, rfc5714, rfc6987.

I don’t why a standards track document can’t have normative references to informational RFCs – especially YANG models.

If there are, hopefully they will be truncated.

Thanks,
Acee

---

From: Alvaro Retana <[email-masked]>
Date: Wed, 26 Jun 2019 15:26:10 -0400

On June 26, 2019 at 11:53:28 AM, Acee Lindem (acee) ([email-masked]) wrote:

Acee:

Hi!

I only have one response. :-)

...

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

<acee> Because sub-second hellos was a bad idea – three words: B-F-D…'

This question is not about sub-second Hellos…it’s about the *remaining
time*.  Even if Hellos are x seconds apart, the “remaining time before the
neighbor is declared dead” can still be in ms, right?  Why not?  Note that
there are other places in the model that are characterized as tracking the
remaining time.

Thanks!!

Alvaro.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 27 Jun 2019 01:31:02 +0000

Hi Alvaro,

Date: Wednesday, June 26, 2019 at 3:26 PM

On June 26, 2019 at 11:53:28 AM, Acee Lindem (acee) ([email-masked]<mailto:[email-masked]>) wrote:

Acee:

Hi!

...

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

This question is not about sub-second Hellos…it’s about the *remaining time*.  Even if Hellos are x seconds apart, the “remaining time before the neighbor is declared dead” can still be in ms, right?  Why not?  Note that there are other places in the model that are characterized as tracking the remaining time.

I don’t feel that strongly. However, it would seem that one would use the same granularity as the configuration. No?

We found that the RFC 8294 timer types aren’t good for “config true” values since the values “infinity” and “not-set” are included in the union. Hence, they lend themselves better to operational state than configurable values.

Thanks,
Acee

Thanks!!

Alvaro.

---

From: Alvaro Retana <[email-masked]>
Date: Thu, 27 Jun 2019 07:54:59 -0400

On June 26, 2019 at 9:31:05 PM, Acee Lindem (acee) ([email-masked]) wrote:

...

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

I don’t feel that strongly. However, it would seem that one would use the
same granularity as the configuration. No?

I wouldn’t think so.  If I was an operator I would like to know if there
are 500ms left before my neighbor dies, and not just 1 or 0.  I think this
may also be useful for troubleshooting.

But I’m not an operator…

Alvaro.

We found that the RFC 8294 timer types aren’t good for “config true” values
since the values “infinity” and “not-set” are included in the union. Hence,
they lend themselves better to operational state than configurable values.

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Thu, 27 Jun 2019 15:11:21 +0000

I am firmly on the side of Acee on this one – and I think more attention needs to be paid to his initial answer: “B-F-D”.

The implications of this are that we do not expect control plane to have finer granularity than seconds – which is why routing protocol hold times are expressed in seconds (both adjacency hold times and LSA hold times).
Which means that even if you had the ability to display “X.mmm seconds remaining” this would not mean that the actual reaction to the timeout would occur within milliseconds of the timer expiration.

I would also argue that operationally it does not matter if an adjacency times out in N seconds or N.5 seconds. This is not used as a fast failure detection mechanism.

   Les

From: Lsr <[email-masked]> On Behalf Of Alvaro Retana
Sent: Thursday, June 27, 2019 4:55 AM
To: Acee Lindem (acee) <[email-masked]>; [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Re: [Lsr] AD Review for draft-ietf-ospf-yang-21

On June 26, 2019 at 9:31:05 PM, Acee Lindem (acee) ([email-masked]<mailto:[email-masked]>) wrote:

...

...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3942       }

I wouldn’t think so.  If I was an operator I would like to know if there are 500ms left before my neighbor dies, and not just 1 or 0.  I think this may also be useful for troubleshooting.

But I’m not an operator…

Alvaro.