Re: [Idr] AD Review of draft-ietf-idr-eag-distribution-15

[Jeff Tantsura <[e-mail]>]

HI Alvaro,

Many thanks for your thoughtful review, please see inline (updated version has been published)

Cheers,
Jeff
On Mar 30, 2021, 1:25 PM -0700, Alvaro Retana <[e-mail]>, wrote:
> Dear authors:
>
> Here is my review of this document.  I have some major comments (see
> inline below) that should be easy to address and that I would like to
> see reflected in the text before starting the IETF Last Call.
>
> Thank you for your work!
>
> Alvaro.
>
>
> [Line numbers from idnits.]
>
>
> 11 Distribution of Traffic Engineering Extended Admin Groups using BGP-LS
>
> [minor] s/Admin/Administrative
[jeff] ack
>
>
> 12                   draft-ietf-idr-eag-distribution-15
>
> 14 Abstract
>
> 16   Administrative groups are link attributes (commonly referred to as
> 17   "colors" or "link colors") advertised by link state protocols (e.g.
> 18   ISIS or OSPF) and used for traffic engineering.  These administrative
> 19   groups were initially defined as 32 bit masks.  As network usage
> 20   grew, these 32 bit masks were found to constrain traffic engineering.
> 21   Therefore, link state protocols (ISIS, OSPF) were expanded to
> 22   advertise a variable length administrative group.This document
> 23   defines an extension to BGP-LS for advertisement of extended
> 24   administrative groups (EAGs) to allow to support a number of
> 25   administrative groups greater than 32, as defined in [RFC7308].
>
> [minor] A couple of changes to simplify.  The details can be found in
> the Introduction.
[jeff] ack
>
> Suggestion>
>    Administrative groups are link attributes advertised used for traffic
>    engineering.  This document defines an extension to BGP-LS for
>    advertisement of extended administrative groups (EAGs).
>
>
> ...
> 70 1.  Introduction
>
> 72   Administrative groups (commonly referred to as "colors" or "link
> 73   colors") are link attributes that are advertised by link state
> 74   protocols like IS-IS [RFC5305], OSPFv2 [RFC3630] and OSPFv3 [RFC5329]
> 75   for traffic engineering use-cases.  The BGP-LS advertisement of the
> 76   originally defined (non-extended) administrative groups is encoded
> 77   using the Administrative Group (color) TLV 1088 as defined in
> 78   [RFC7752].
[jeff] ack
>
> [minor] "IS-IS [RFC5305], OSPFv2 [RFC3630] and OSPFv3 [RFC5329]"
> These references should point at the protocols themselves, not the TE
> extensions.  Also, these references can be Informative.
[jeff] ack, changed
[jeff] Alvaro - this is the text from 7308, that specifically calls  out TE documents -
"This document defines the Extended Administrative Group (EAG) sub-TLVfor both OSPF [RFC3630] and IS-IS [RFC5305].” ?
>
>
> 80   These administrative groups are defined as a fixed-length 32-bit
> 81   bitmask.  As networks grew and more use-cases were introduced, the
> 82   32-bit length was found to be constraining and hence extended
> 83   administrative groups (EAG) were introduced in the IS-IS and OSPFv2
> 84   link state routing protocols [RFC7308].
>
> [minor] rfc7308 also includes OSPFv3.
>
> s/were introduced in the IS-IS and OSPFv2 link state routing protocols
> [RFC7308]./were introduced in [RFC7308].
[jeff] ack
>
>
> ...
> 97 2.  Advertising Extended Administrative Groups in BGP-LS
>
> 99   This document defines an extension that enable BGP-LS speakers to
> 100   signal the EAG of links in a network to a BGP-LS consumer of network
> 101   topology such as a centralized controller.  The centralized
> 102   controller can leverage this information in traffic engineering
> 103   computations and other use-cases.  When a BGP-LS speaker is
> 104   originating the topology learnt via link-state routing protocols like
> 105   OSPF or IS-IS, the EAG information of the links is sourced from the
> 106   underlying extensions as defined in [RFC7308].  The BGP-LS speaker
> 107   may also advertise the EAG information for the local links of a node
> 108   when not running any link-state IGP protocol e.g. when running BGP as
> 109   the only routing protocol.
>
> [minor] There is no defined mechanism to originate EAG information
> when not running a link-state protocol.  Let's take the last sentence
> out.
>
[jeff] ack
>
> 111   The EAG of a link is encoded in a new Link Attribute TLV [RFC7752]
> 112   using the following format:
>
> 114      0                   1                   2                   3
> 115      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
> 116     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 117     |              Type             |             Length            |
> 118     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 119     |    Extended Administrative Groups (variable)                 //
> 120     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> [minor] Maybe use the same representation as rfc7308 for the format.
[jeff] this is the standard way to represent encodings in BGP-LS documents, I think readers are more comfortable with this representation, please let me know what you think.
>
>
> ...
> 128   o  Length: variable length which represents the total length of the
> 129      value field.  The length value MUST be multiple of 4.  If the
> 130      length is not a multiple of 4, the TLV MUST be considered
> 131      malformed.
>
> [major] "total length of the value field"  What are the units?
[jeff ack]
>
>
> 133   o  Value: one or more sets of 32-bit bitmasks that indicate the
> 134      administrative groups (colors) that are enabled on the link when
> 135      those specific bits are set.
>
> [major] rfc7308 talks about an "Extended Admin Group" (singular!), and
> not a set of groups.  This TLV is then not in line with rfc7308.
>
> The whole document should be checked for (editorial) compliance with
> the concept of a single EAG.
>
[jeff] ack, i believe now it reads correct
>
> 137   The EAG TLV is an optional TLV.  The originally defined AG TLV 1108
> 138   and the EAG TLV 1173 defined in this document MAY be advertised
> 139   together.  The semantics of the EAG and the backward compatibility
> 140   aspects of EAG with respect to the AG are handled as described in the
> 141   Backward Compatibility section of [RFC7308], namely - If a node
> 142   advertises both AG and EAG, then the first 32 bits of the EAG MUST be
> 143   identical to the advertised AG.
>
> [minor] We don't really need this paragraph because any compatibility
> or checking is outside of what BGP-LS does (just transport).  Please
> remove it.
>
[jeff] ack
>
> ...
> 158 4.  Security Considerations
>
> 160   The extensions in this document advertise same administrative group
> 161   information specified via [RFC7752] but as a larger/extended value
> 162   and hence does not introduce security issues beyond those discussed
> 163   in [RFC7752] and [I-D.ietf-idr-rfc7752bis].
>
> [major] No need to mention rfc7752bis -- at all!
>
> [major] Please take a look at the Security Considerations in rfc8814
> and copy them here -- with appropriate modifications to talk about EAG
> instead of MSD, of course.
>
[jeff] ack
>
> [End of Review]