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

["Adrian Farrel" <[e-mail]>]

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.

> 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