Re: [Idr] AD Review of draft-ietf-idr-te-pm-bgp-14

["Les Ginsberg (ginsberg)" <[e-mail]>]

Alvaro –

Thanx for the review.
I am fine with all of your comments – but I do have one concern.

You suggest replacing the reference to RFC7810 with a reference to draft-ietf-lsr-isis-rfc7810bis.
If we do that then the publication of this document will be blocked on the publication of draft-ietf-lsr-isis-rfc7810bis. But the changes being made in the bis draft do not affect the content of this document in any way. So it makes more sense to me to continue to reference RFC 7810.

I’ll go with whatever you decide, but wanted to raise this point before making the suggested change.
???

    Les


From: Alvaro Retana <[e-mail]>
Sent: Wednesday, November 28, 2018 1:01 PM
To: [e-mail]
Cc: Susan Hares <[e-mail]>om>; idr@ietf. org <[e-mail]>rg>; [e-mail]
Subject: AD Review of draft-ietf-idr-te-pm-bgp-14

Dear authors:

I just finished reading this document.  Please see the comments in-line below.

I found only one significant issue with the definition of Available Bandwidth (see my comment in §3.6).  This issue, along with my other comments (mostly nits), should be easy to address.  I am then starting the IETF Last Call.

Thanks!

Alvaro.


[Note: the line numbers come from idnits.]

...
17       Abstract

19         This document defines new BGP-LS TLVs in order to carry the IGP
20         Traffic Engineering Extensions defined in IS-IS and OSPF protocols.

[nit] s/defined in IS-IS and OSPF protocols/defined in the IS-IS and OSPF protocols

22       Requirements Language

24         The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
25         "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
26         "OPTIONAL" in this document are to be interpreted as described in BCP
27         14 [RFC2119] [RFC8174] when, and only when, they appear in all
28         capitals, as shown here.

[major] No Normative language is used -- which is not a bad thing.  Please remove the boilerplate text and the appropriate references.

...
90       2.  Link Attribute TLVs for TE Metric Extensions

92         The following new Link Attribute TLVs are defined:

94            TLV Name
95         ------------------------------------------
96          Unidirectional Link Delay

98          Min/Max Unidirectional Link Delay

100        Unidirectional Delay Variation

102        Unidirectional Link Loss

104        Unidirectional Residual Bandwidth

106        Unidirectional Available Bandwidth

108        Unidirectional Bandwidth Utilization

[nit] This is weird seemingly-stub section.  Consider renaming §3 "Link Attribute TLVs for TE Metric Extensions", moving the contents there and turning the list above into a table including the new TLV types...

110     3.  TLV Details

112     3.1.  Unidirectional Link Delay TLV

114       This TLV advertises the average link delay between two directly
115       connected IGP link-state neighbors.  The semantic of the TLV is
116       described in [RFC7810] and [RFC7471].

[nit] s/semantic/semantics

[minor nit] In rfc7810, the Type and Length fields are 1-byte each...so in reality rfc7810/rfc7471 describe the semantics of the Value field (not the whole TLV).  Maybe some short text in §3 about the TLV format following the specification in rfc7752 would help.


...
220     3.6.  Unidirectional Available Bandwidth TLV

222       This sub-TLV advertises the available bandwidth between two directly
223       connected IGP link-state neighbors.  The semantic of the TLV is
224       described in [RFC7810] and [RFC7471].

226        0                   1                   2                   3
227        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
228       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
229       |   Type                      |           Length                |
230       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
231       |                      Available Bandwidth                      |
232       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[major] AFAICT, Available Bandwidth is the only definition that is different between rfc7810/draft-ietf-lsr-isis-rfc7810bis and rfc7471.  The difference comes from the correction made to address this report [1].  Instead of trying to fix the definition here, I think that a similar report should be filed against rfc7471.  Please submit it and I will approve.

[1] https://www.rfc-editor.org/errata/eid5486


...
284     5.  IANA Considerations

286       This document requests assigning code-points from the registry "BGP-
287       LS Node Descriptor, Link Descriptor, Prefix Descriptor, and Attribute
288       TLVs" for the new Link Attribute TLVs defined in the table below:

290        TLV code-point                 Value
291       --------------------------------------------------------
292        1114              Unidirectional Link Delay

294        1115              Min/Max Unidirectional Link Delay

296        1116              Unidirectional Delay Variation

298        1117              Unidirectional Link Loss

300        1118              Unidirectional Residual Bandwidth

302        1119              Unidirectional Available Bandwidth

304        1120              Unidirectional Bandwidth Utilization

[major] This document doesn't request the assignments, but IANA has done the early allocation...  Please correct.


...
325     8.1.  Normative References
...
332       [RFC4271]  Rekhter, Y., Ed., Li, T., Ed., and S. Hares, Ed., "A
333                  Border Gateway Protocol 4 (BGP-4)", RFC 4271,
334                  DOI 10.17487/RFC4271, January 2006,
335                  <https://www.rfc-editor.org/info/rfc4271>.

[minor] I think this can be an Informative reference: just like rfc4272 and rfc6952 are.

...
348       [RFC7810]  Previdi, S., Ed., Giacalone, S., Ward, D., Drake, J., and
349                  Q. Wu, "IS-IS Traffic Engineering (TE) Metric Extensions",
350                  RFC 7810, DOI 10.17487/RFC7810, May 2016,
351                  <https://www.rfc-editor.org/info/rfc7810>.

[major] Please use draft-ietf-lsr-isis-rfc7810bis as the reference.