Re: [Idr] Alvaro's AD Comments on draft-ietf-idr-ls-distribution

["Acee Lindem (acee)" <[e-mail]>]

Hi Adrian, Alvaro, 

See one inline below…

On 10/3/15, 6:48 AM, "Idr on behalf of Adrian Farrel"
<[e-mail] on behalf of [e-mail]> wrote:

>Thanks, Alvaro, for your review of (I think) version -10 of the draft.
>
>Here are some answers to your questions.
>
>> Major:
>>
>> 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.
>
>You're right.
>I think we should add a new subsection 5.1...
>
>5.1  Guidance for Designated Experts
>
>In all cases of review by Designated Expert (DE) described here, the DE is
>expected to ascertain the existence of suitable documentation (a
>specification)
>as described in RFC5226, and to verify the permanent and publically
>readily
>available of the document. The DE is also expected to check the clarity of
>purpose and use of the requested code points. Lastly, the DE must verify
>that
>any specification produced in the IETF that requests one of these code
>points
>has been made available for review by the IDR working group, and that any
>specification produced outside the IETF does not conflict with work that
>is
>active or already published within the IETF.
>
>> "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.
>
>Yes, this text is a hang-over from the document template.
>
>> 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
>>know if
>you want to do it.
>
>I think the absence of an answer speaks for itself.
>
>> 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.
>
>There are several questions folded in here.
>
>Firstly, understand that the maximum rate described here is the default
>rate if
>not overridden by an operator or by an implementation that knows better.
>So we
>shouldn't get too hung up on it. Although, obviously, there is a
>reasonable
>expectation that if an implementation uses this default value the function
>should work.
>
>Why 200? Not sure where that came from. I have no evidence that 200 is a
>problem
>and given that IDR has done quite a bit of implementation and interop
>without
>anyone screaming, I assume this is OK.
>https://www.ietf.org/id/draft-ietf-idr-ls-distribution-impl-04.txt doesn't
>mention any problems. Are you aware of implementations where this is a
>problem?
>
>Could BGP-LS advertisements impact on "other BGP stuff"? Well, that
>really is an
>implementation and deployment issues? Will the BGP-LS speaker doing the
>advertisements also advertise "other BGP stuff" on the same session? Is
>the
>advertisement higher cost than the consolidation of IGP-TE information
>and the
>application of policy to determine what LS information to advertise.
>Could the
>advertisements be prioritised? This has been something of a recent thread
>on the
>IDR list and, although you are raising interesting questions, I don't
>believe we
>have any way to draw a line unless someone reports real problems that are
>not
>caused by implementation choices, yet we are required (by the IESG) to
>supply
>defaults for configurable parameters.
>
>Why "MAY" use a dedicated RR rather than "SHOULD"? Because there really
>doesn't
>appear to be any indication that this is a problem in all networks with
>all BGP
>implementations. 
>
>> 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?
>
>I don't think we are assuming corruption. So the malformed attribute
>comes as
>the result of an implementation difference or a bug at the sender.
>Requesting a
>refresh runs a significant risk of simply thrashing the advertisement
>since each
>time it is sent it will contain the same malformed attribute. This
>problem is
>considerably worsened if the session is reset because then we thrash the
>whole
>session.
>
>> Minor:
>>
>> 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?
>
>Isn't this standard behavior for unknown TLVs in BGP?
>E.g., 5512 section 4 "Unknown types are to be ignored and skipped upon
>receipt."
>That said, 5512 section 4 also says "When the TLV is being processed by a
>BGP
>speaker that will be performing encapsulation, any unknown sub-TLVs MUST
>be
>ignored and skipped."
>Since the behavior described here is new in as much as it describes
>processing
>of a new NLRI, I think using "MUST" would be OK.
>
>> 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.
>
>We could say "treating the entire value field an opaque hexadecimal
>string and
>comparing leftmost octets first regardless of the length of the string."
>
>> 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).
>
>Per Alia's comment replacement text is...
>  BGP-LS uses the Autonomous System (AS) Number and BGP-LS Identifier
>  (see Section 3.2.1.4) to disambiguate the Router-IDs, as described in
>  Section 3.2.1.1.
>
>> 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.
>
>I'm not sure the missing attributes should be "figured out". The job of
>BGP-LS
>is to collect and distribute information from the IGP. If the information
>is not
>there, it would not be invented by the
>BGP-LS speaker.
>
>The SRLG case has been fixed in -11: it was an error.
>
>> 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.
>
>Why?
>Certainly your proposal appears right: there are only four protocols
>listed in
>Table 2, but there is the question of future IDs. So we could write...
>
>   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 the other Protocol-IDs
>   listed in Table 2.
>
>I don't object to this change, but struggle to see the value.
>
>>  "LINK_STATE attribute" shows up in 3.3.2 and 3.3.3.  What is it?
>
>Thanks for this catch. Old text. Should now read "BGP-LS attribute with a
>link
>NLRI"
>
>> 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.
>
>Although no-one was asking for this and no-one will have implemented it,
>I can
>see no reason not to change to read...
>
>3.3.1.1.  Node Flag Bits TLV
>
>   The Node Flag Bits TLV carries a bit mask describing node attributes.
>   The value is a variable length bit array of flags, where each bit
>   represents a node capability.
>
>      0                   1                   2                   3
>      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
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |              Type             |             Length            |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |O|T|E|B|R|V| Rsvd|
>     +-+-+-+-+-+-+-+-+-+
>
>                   Figure 15: Node Flag Bits TLV format
>
>   The bits are defined as follows:
>
>            +----------+-------------------------+-----------+
>            |   Bit    | Description             | Reference |
>            +----------+-------------------------+-----------+
>            |   'O'    | Overload Bit            | [RFC1195] |
>            |   'T'    | Attached Bit            | [RFC1195] |
>            |   'E'    | External Bit            | [RFC2328] |
>            |   'B'    | ABR Bit                 | [RFC2328] |
>            |   'R'    | Router Bit              | [RFC5340] |
>            |   'V'    | V6 Bit                  | [RFC5340] |
>            | Reserved | Reserved for future use |           |
>            +----------+-------------------------+-----------+
>
>                    Table 8: Node Flag Bits Definitions
>
>
>And similarly no-one has asked for the DN-bit from 2328 or 5340 in section
>3.3.3.1. I am less sanguine about adding this as I am not quite sure that
>it is
>useful in BGP-LS, but it would certainly be easy to add. With this bit,
>and with
>other potential additions to both bit fields, I suggest we wait for the
>need/desire and folk can write new drafts.

The DN-bit is not really applicable to at the Node level in OSPF and
IS-IS. It is applicable to prefixes and is included in the flags in
section 3.3.3.1 (See the D-bit). For extra credit, you could add a
reference to OSPF and RFC 4576. This document started out with very few
references to the applicable OSPF RFCs but quite a few have been added by
Alia and me. 

Thanks,
Acee





>
>> 6.2.5.  (Performance Management)  It might be nice to include
>>updates/second
>as 
>> one of the performance indicators.
>
>I think that we add to the bottom of 6.2.5 as follows:
>
>"These statistics should be recorded as absolute counts since system or
>session
>start time. An implementation MAY also enhance this information by also
>recording peak per-second counts in each case."
>
>> 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?
>
>I think you have to be a recognised peer for this attack to have any
>significant
>value that is specific to *this* document.
>
>> Looking at the references, it seems to me that the following changes
>>could
>> be made: 
>> Make Informative: RFC1918
>Sure
>> Make Normative: I-D.ietf-idr-error-handling
>Yes, and this is now RFC7606
>
>> Curious question:  When encoding a "normal" size node using BGP-LS,
>>what is
>the
>> resulting size of the UPDATE?
>
>I'll leave this as an exercise for the reader.
>
>> Nits:
>> 3.2: s/'Static' protocol types/'Static configuration' protocol types
>> Again
>in 3.3.2.2.
>> 3.2.1 and 6.2.3 : What does "Paragraph 2" refer to?
>> 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.
>> 3.2.1.4: "IGP Router ID.mandatory TLV"  Should be sub-TLV.
>> 3.2.3.1: "The Route Type TLV allows to discrimination of these
>advertisements./The
>> Route Type TLV allows the discrimination of these advertisements.
>> 3.3.2.2: Please be consistent in the name.  Is it "MPLS Protocol Mask
>>TLV" or
>just
>> "MPLS Protocol TLV"?
>> 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.
>> 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.
>> 4.1 s/"see" the full topology of AS/"see" the full topology of AS2
>
>I'll work through the nits as well.
>New revision "soon".
>
>Thanks,
>Adrian
>
>
>_______________________________________________
>Idr mailing list
>[e-mail]
>https://www.ietf.org/mailman/listinfo/idr