[Idr] AD Review of draft-ietf-idr-ls-distribution

["Alvaro Retana (aretana)" <[e-mail]>]

Authors:

Hi!

I just finished reading this draft.  There are a couple of things I would like to talk about before moving it forward (“Major” below).  I also have a number of minor comments and some nits.

Alia had already produced an AD review [https://www.ietf.org/mail-archive/web/idr/current/msg14146.html], but I didn’t see any follow up.  Our comments don’t fully overlap, so I expect both sets to be addressed.

Also, during the IETF Last Call some comments came in, but none of the authors replied, including comments from IANA.  Please address those as well.

Thanks!

Alvaro.


Major:

  1.  Section 5 (IANA Considerations)
     *   I didn’t see this comment as part of IANA’s review, but the text about the Designated Experts should be clarified.  There is some guidance in RFC5226.
     *   "Note to RFC Editor: this section may be removed on publication as an RFC.”  The IANA considerations section should remain because there are in fact considerations there.
     *   I would like to have Designated Experts in place soon.  It seems to me that having 2 of the authors volunteer (primary and backup) would be ideal.  Please let me knot if you want to do it.
  2.  6.1.2 (Installation and Initial Setup): [default] "maximum rate at which Link-State NLRIs will be advertised/withdrawn from neighbors is set to 200 updates per second.”  Where did the number come from?  It does look very big.  Are you assuming just changes or startup as well?  One of my concerns is the interaction with other BGP stuff (like regular routing!).  In fact, in 6.1.5 (Impact on Network Operation) you wrote:  "Frequency of Link-State NLRI updates could interfere with regular BGP prefix distribution.  A network operator MAY use a dedicated Route-Reflector infrastructure to distribute Link-State NLRIs.”  Why not use SHOULD?  If there may be significant operational impact I think you should be more direct with the guidance.
  3.  6.2.2 (Fault Management). "If an implementation of BGP-LS detects a malformed attribute, then it SHOULD use the 'Attribute Discard’ action..”  Doesn't this mean that the information may be useless, completely missing, or in the best case incomplete?  Aren’t we better off just resetting the session or at least requesting a route refresh?

Minor:

  1.  3.1 (TLV Format): "Unrecognized types are preserved and propagated.”  This statement sounds necessary for interoperability and the correct propagation of the information.  Should it have a “MUST” in it?
  2.  3.1 (TLV Format): “If there are more TLVs of the same type, then the TLVs MUST be ordered in ascending order of the TLV value within the TLVs with the same type.”  Ordered by value?  How is that done?  Some TLVs contain sub-TLVs in the value field, but others don’t.
  3.  3.2.1 (Node Descriptors):  The BGP-LS Identifier is used, but it is defined later in 3.2.1.4.  Please add a reference so that it is not mistaken for the “Identifier” (in the NLRIs themselves in Section 3.2).
  4.  3.3.2 (Link Attribute TLVs):  The attributes used are defined for ISIS, some are easily derived from OSPF, but other don’t map to anything.  For example, “administrative group (color)”.  How are these types of undefined parameters in OSPF figured out?  I suspect the easy answer is to just say that it is out of scope, but the fact remains that there can be information there that doesn’t exist in the original protocol.  It should at least be clarified what is expected.  Note that in 3.3.2.5 (Shared Risk Link Group TLV) a different case of this shows up: the text seems to indicate that because there is no SRLG TLV in OSPD-TE, then this information wouldn’t be available.  Again, please clarify if there may be procedures outside the scope of the document that could apply.
  5.  3.3.2.2 (MPLS Protocol Mask TLV)
     *   OLD>
   Generation of the MPLS Protocol Mask TLV is only valid for
   originators which have local link insight, like for example Protocol-
   IDs 'Static' or 'Direct' as per Table 2.  The 'MPLS Protocol Mask'
   TLV MUST NOT be included in NLRIs with protocol-IDs 'IS-IS L1', 'IS-
   IS L2', 'OSPFv2' or 'OSPFv3' as per Table 2.
     *   NEW>
     *
   Generation of the MPLS Protocol Mask TLV is only valid for
   originators which have local link insight, like for example Protocol-
   IDs 'Static' or 'Direct' as per Table 2.  The 'MPLS Protocol Mask'
   TLV MUST NOT be included in NLRIs with other protocol-Ids.
  6.  "LINK_STATE attribute” shows up in 3.3.2 and 3.3.3.  What is it?
  7.  In 3.3.1.1 (Node Flag Bits TLV) you included the ISIS Overload Bit, but didn’t say anything about OSPFv3’s R-bit or v6-bit; and in 3.3.3.1 (IGP Flags TLV) you included the IS-IS Up/Down Bit in this TLV, but I didn’t see the OSPF DN-bit anywhere.  Are these opportunities to reuse the same bit with a more generic definition while covering a similar option in OSPF?  I know you can’t cover all the options, but these two are well-known and seem low hanging fruit.
  8.  6.2.5.  (Performance Management)  It might be nice to include updates/second as one of the performance indicators.
  9.  Section 8 (Security Considerations) "The principal attack a consumer may apply is to attempt to start multiple sessions either sequentially or simultaneously.”  Isn’t this an attack that any other node can try?  Why limit the discussion only to consumers?
  10. Looking at the references, it seems to me that the following changes could be made:
     *   Make Informative: RFC1918
     *   Make Normative: I-D.ietf-idr-error-handling
  11. Curious question:  When encoding a “normal” size node using BGP-LS, what is the resulting size of the UPDATE?

Nits:

  1.  3.2: s/'Static' protocol types/‘Static configuration' protocol types     Again in 3.3.2.2.
  2.  3.2.1 and 6.2.3 : What does "Paragraph 2” refer to?
  3.  3.2.1.2: “mandatory TLV in all three types of NLRIs”  I assume you’re referring to Node, Link and Prefix NLRIs, right?  However, it is confusing because there are in fact 4 types defined.  Please correct.
  4.  3.2.1.4: “IGP Router ID…mandatory TLV”  Should be sub-TLV.
  5.  3.2.3.1: "The Route Type TLV allows to discrimination of these advertisements./The Route Type TLV allows the discrimination of these advertisements.
  6.  3.3.2.2: Please be consistent in the name.  Is it “MPLS Protocol Mask TLV” or just “MPLS Protocol TLV”?
  7.  The description of the example in 3.7 is easier to read (because of the formatting) than the one in 3.6.  It would be nice to be consistent.
  8.  3.8 seems to be a better example of simply ships in the night than migration as the emphasis is put on identifying the common links of the two processes.
  9.  4.1 s/"see" the full topology of AS/"see" the full topology of AS2