[Idr] Document shepherd review for draft-ietf-idr-rfc7752bis

[Jeffrey Haas <[e-mail]>]

Ketan and Working Group,

Here's my document shepherd review for RFC7752-bis.  

General assessment: Generally good shape, some concerns on error handling and
consistency on NLRI keying.

-- Jeff


---

NLRI keying consistency:
There are a number of TLVs that are optional.  Some of these optional fields may
be present in the NLRI.  These include:
- Autonomous System
- BGP-LS Identifier (deprecated)
- Multi-Topology ID

Additional optional TLVs may be distributed in the protocol in the future.

Consider two BGP-LS Producers.  Producer 1 emits no optional fields for a given
piece of link state.  Producer 2 emits all of the optional fields.

A BGP-LS Consumer may receive two distinct pieces of information for the same
topology data.  

There is no discussion in the document as to how such inconsistencies could be
handled by the Consumer.

---

BGP-LS Attribute consistency:

Consider two BGP-LS Producers.  Producer 1 emits no optional fields for a given
piece of link state.  Producer 2 emits all of the optional fields.
(This is a simplified example.  The critical detail is that the Path Attributes
are not identical.)

In any case where the BGP-LS NLRI is identical, an implementation will use BGP
route selection procedures to pick one of the possible paths and select it to
forward to its peers.  In the absence of overriding policy features such as
LOCAL_PREF or MED, this will often devolve to selecting the IGP distance to the
BGP NEXT_HOP.  (Choose closest point of Truth.)

There's no discussion about how inconsistent information may be lost by having
multiple producers that do not consistently produce their data.

---

The remainder of my comments have to do with Error Handling.  My intent is not
to re-litigate prior decisions, it's to note that the consequences of those
prior decisions are not adequately discussed in the document.

Section 7.2.2 does an admirable job in noting the usual things a BGP developer
should look for in a specification. But mostly it says, no matter how bad the
contents, move along:

 :    Each TLV MAY indicate the valid and permissible values and their
 :    semantics that can to be used only by a BGP-LS Consumer for its
 :    semantic validation.  However, the handling of any errors may be
 :    specific to the particular application and outside the scope of this
 :    document.  A BGP-LS Consumer should ignore unrecognized and
 :    unexpected TLV types in both the NLRI and BGP-LS Attribute portions
 :    and not consider their presence as an error.

While I must echo a common pithy comment from one of my protocol elders, 
"BGP is not a dump truck", in this case it is exactly that.  The expectation is
that if an implementation has a series of well-formed and nested TLVs that
follow their ordering rules, that implementation is to pass things on.

A significant portion of RFC 7606, BGP's Error Handling extensions, was intended
to deal with what was called at the time "Optional-Transitive Nonsense".  Much
of the motivation for the RFC was preventing a downstream BGP peer from
resetting its session because an ignorant upstream router was propagating bad
data.  Another portion of the motivation is that once you realize you have bad
data, you shouldn't pass it along.

The strict TLV structuring of BGP-LS makes the most types of syntactic errors
impossible.  However, BGP-LS describes a number of semantic validations using
RFC 2119/8174 keywords without adequately describing what to do when those
behaviors are violated at the BGP-LS Consumer.

Simple examples of such things are:
- TLVs of fixed length that are not of the proper length.
- TLVs that are mandatory that are absent.
- TLVs that are not appropriate for a given NLRI type present in the attributes;
  e.g. Node properties for a Link NLRI.
- Metrics that are of inappropriate size for their underlying protocol
  semantics.

While there may be an urge to not address such issues as "this is the Consumer's
problem", this misses a common bit of learned BGP wisdom: Such things are the
nightmare of interoperability.  If you have two applications that are BGP-LS
consumers that are intended to cooperate for implementing an application enabled
by BGP-LS, and they have different error handling semantics, you may have
incorrect operational behavior.

"Make the same mistakes everywhere" is not a great recipe.

I strongly recommend that this issue be addressed either in section 7.2.2 or in
a new section.

---------------------------------------------------------------------------------

Some comments on the draft text are noted below.  This is from the idnits output.

407	4.1.  TLV Format
[...]
435	   type MUST be in ascending order based on the value field.  Comparison
436	   of the value fields is performed by treating the entire field as an
437	   opaque hexadecimal string.  Standard string comparison rules apply.

I suspect what is intended here is opaque binary data rather than representing
it in hexadecimal format.  "Standard string comparison" might make sense if you
were doing strcmp() on an actual hexadecimal string.  The intent here is
probably lexicographically ordered since "standard string comparison" isn't
really defined here.

447	   The TLVs within the BGP-LS Attribute MAY be ordered in ascending
448	   order by TLV type.  BGP-LS Attribute with unordered TLVs MUST NOT be
449	   considered malformed.

Probably "SHOULD be ordered"?  If it's MAY be ordered and MUST NOT be malformed,
why mention it at all?

451	4.2.  The Link-State NLRI

453	   The MP_REACH_NLRI and MP_UNREACH_NLRI attributes are BGP's containers
454	   for carrying opaque information.  This specification defines three
455	   Link-State NLRI types that describe either a node, a link, and a
456	   prefix.

"either a node, a link, or a prefix"
[...]

495	      0                   1                   2                   3
496	      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
497	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
498	     |            NLRI Type          |     Total NLRI Length         |
499	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
500	     |                                                               |
501	     +                       Route Distinguisher                     +
502	     |                                                               |
503	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
504	     |                                                               |
505	     //                  Link-State NLRI (variable)                 //
506	     |                                                               |
507	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

509	         Figure 6: Link-State VPN AFI 16388 / SAFI 72 NLRI Format

I'd suggest mentioning the length of the route distinguisher in the diagram
rather than letting it be solely inferred by the text representation.

516	                   +------+---------------------------+
517	                   | Type | NLRI Type                 |
518	                   +------+---------------------------+
519	                   |  1   | Node NLRI                 |
520	                   |  2   | Link NLRI                 |
521	                   |  3   | IPv4 Topology Prefix NLRI |
522	                   |  4   | IPv6 Topology Prefix NLRI |
523	                   +------+---------------------------+

525	                            Table 1: NLRI Types

Note that the contents of this table do not match the IANA Considerations
section.


529	   The Node NLRI (NLRI Type = 1) is shown in the following figure.

531	      0                   1                   2                   3
532	      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
533	     +-+-+-+-+-+-+-+-+
534	     |  Protocol-ID  |
535	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
536	     |                           Identifier                          |
537	     |                            (64 bits)                          |
538	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
539	     //                Local Node Descriptors (variable)            //
540	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

542	                      Figure 7: The Node NLRI Format

The ASCII diagram covering Identifier isn't consistent with the multi-row
format.  Compare vs. Figure 6's Route Distinguisher.

This issue occurs multiple places further in the document.

The syntax of the Identifier isn't clearly discussed in the document.  Is the
intent to be a network-byte ordered uint64 number?

[...]

753	   Autonomous System:  Opaque value (32-bit AS Number).  This is an
754	      optional TLV.  The value SHOULD be set to the AS Number associated
755	      with the BGP process originating the link-state information.  An
756	      implementation MAY provide a configuration option on the BGP-LS
757	      Producer to use a different value.

Some text discussing private AS numbers would be helpful.  The item of concern
is when BGP-LS is used between cooperating ASes not under a single
administrative control.  In such cases, avoiding private AS collisions is an
edge case.

[...]

795	      There can be at most one instance of each sub-TLV type present in
796	      any Node Descriptor.  The sub-TLVs within a Node Descriptor MUST
797	      be arranged in ascending order by sub-TLV type.  This needs to be
798	      done to compare NLRIs, even when an implementation encounters an
799	      unknown sub-TLV.  Using stable sorting, an implementation can do a
800	      binary comparison of NLRIs and hence allow incremental deployment
801	      of new key sub-TLVs.

I believe this section is intended to be outdented one level?

[...]

895	   The TLVs/sub-TLVs corresponding to the interface addresses and/or the
896	   local/remote identifiers may not always be signaled in the IGPs
897	   unless their advertisement is enabled specifically.  In such cases,
898	   it is valid to advertise a BGP-LS Link NLRI without any of these
899	   identifiers.

If there are multiple BGP-LS producers, can this happen if one producer is
configured with this option and another is not?

If so, the same Link Descriptor may be generated with different NLRI
representation.

901	4.2.2.1.  Multi-Topology ID

903	   The Multi-Topology ID (MT-ID) TLV carries one or more IS-IS or OSPF
904	   Multi-Topology IDs for a link, node, or prefix.

906	   The semantics of the IS-IS MT-ID are defined in Section 7.1 and 7.2
907	   of RFC 5120 [RFC5120].  The semantics of the OSPF MT-ID are defined
908	   in Section 3.7 of RFC 4915 [RFC4915].  If the value in the MT-ID TLV
909	   is derived from OSPF, then the upper 5 bits of the MT-ID field MUST
910	   be set to 0.

912	   The format of the MT-ID TLV is shown in the following figure.

914	      0                   1                   2                   3
915	      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
916	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
917	     |              Type             |          Length=2*n           |
918	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
919	     |R R R R|  Multi-Topology ID 1  |             ....             //
920	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
921	     //             ....             |R R R R|  Multi-Topology ID n  |
922	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

924	                  Figure 12: Multi-Topology ID TLV Format

926	   where Type is 263, Length is 2*n, and n is the number of MT-IDs
927	   carried in the TLV.

929	   The MT-ID TLV MAY be present in a Link Descriptor, a Prefix
930	   Descriptor, or the BGP-LS attribute of a Node NLRI.  In a Link or
931	   Prefix Descriptor, only a single MT-ID TLV containing the MT-ID of
932	   the topology where the link or the prefix is reachable is allowed.
933	   In case one wants to advertise multiple topologies for a given Link
934	   Descriptor or Prefix Descriptor, multiple NLRIs MUST be generated
935	   where each NLRI contains a single unique MT-ID.  When used in the
936	   Link or Prefix Descriptor TLV for IS-IS, the Bits R are reserved and
937	   MUST be set to 0 (as per Section 7.2 of RFC 5120 [RFC5120]) when
938	   originated and ignored on receipt.

940	   In the BGP-LS attribute of a Node NLRI, one MT-ID TLV containing the
941	   array of MT-IDs of all topologies where the node is reachable is
942	   allowed.  When used in the Node Attribute TLV for IS-IS, the Bits R
943	   are set as per Section 7.1 of RFC 5120 [RFC5120].

The text above for OSPF MT-ID values (RFC 4915, §3.7) could be somewhat clearer.

The diagram above is structured for IS-IS.  The OSPF intent is BOTH that R is
set to zero (the IS-IS text has cases where it may be zero, or potentially
non-zero) and also that the next 5 bits are also zero.  

Some clarity might be achieved by noting the MT-ID field for OSPF is 0..127.

988	   where the Type and Length fields of the TLV are defined in Table 5.
989	   The OSPF Route Type field values are defined in the OSPF protocol and
990	   can be one of the following:

992	   o  Intra-Area (0x1)
993	   o  Inter-Area (0x2)

995	   o  External 1 (0x3)

997	   o  External 2 (0x4)

999	   o  NSSA 1 (0x5)

1001	   o  NSSA 2 (0x6)

The semantics of these are from OSPF, but what's the matching RFC section
reference for the code points?

1003	4.2.3.2.  IP Reachability Information

1005	   The IP Reachability Information TLV is a mandatory TLV for IPv4 &
1006	   IPv6 Prefix NLRI types.  The TLV contains one IP address prefix (IPv4
1007	   or IPv6) originally advertised in the IGP topology.  Its purpose is
1008	   to glue a particular BGP service NLRI, using its BGP next-hop, to a
1009	   given node in the LSDB.  A router SHOULD advertise an IP Prefix NLRI

"BGP service NLRI" isn't a well-defined thing.  I suggest defining it.

1013	      0                   1                   2                   3
1014	      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
1015	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1016	     |              Type             |             Length            |
1017	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1018	     | Prefix Length | IP Prefix (variable)                         //
1019	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

1021	             Figure 14: IP Reachability Information TLV Format

1023	   The Type and Length fields of the TLV are defined in Table 5.  The
1024	   following two fields determine the reachability information of the
1025	   address family.  The Prefix Length field contains the length of the
1026	   prefix in bits.  The IP Prefix field contains the most significant
1027	   octets of the prefix, i.e., 1 octet for prefix length 1 up to 8, 2
1028	   octets for prefix length 9 to 16, 3 octets for prefix length 17 up to
1029	   24, 4 octets for prefix length 25 up to 32, etc.

We have a possible need for additional pedantry here regarding "trailing bits".
RFC 4271, §4.3 defines the format for IPv4 NLRI.  It says:

 :      b) Prefix:
 :
 :         The Prefix field contains an IP address prefix, followed by
 :         the minimum number of trailing bits needed to make the end
 :         of the field fall on an octet boundary.  Note that the value
 :         of trailing bits is irrelevant.

A common fault in BGP implementations is failing to ignore the trailing bits for
purposes of NLRI keying.  This particular bug tends to be less of a problem for
AFI/SAFIs where multiple NLRI may be packed in the same UPDATE message.
Individual BGP Routes are unpacked as tuples of Path Attributes and their
attached NLRI.

In BGP-LS, the IP Reachability Information TLV is a component that is not
surrounded solely by other prefixes, it's embedded.  The procedures for BGP-LS
push implementors toward simply doing binary comparison of various TLV contents.
Even more so, as discussed under my notes on error handling, the implementor is
pushed to ignore most semantic validation errors.

The implication is that most developers are basically doing memcmp() for their
comparison work.  Since "trailing bits" are irrelevant, but matter for
comparison purposes, I strongly suggest text be added that trailing bits are
zeroed.

1031	4.3.  The BGP-LS Attribute

1033	   The BGP-LS Attribute is an optional, non-transitive BGP attribute
1034	   that is used to carry link, node, and prefix parameters and
1035	   attributes.  It is defined as a set of Type/Length/Value (TLV)

It'd be useful to note that IANA has assigned code point 29 here.  It is noted
in the IANA Considerations.


1044	   The size of the BGP-LS Attribute may potentially grow large depending
1045	   on the amount of link-state information associated with a single
1046	   Link-State NLRI.  The BGP specification [RFC4271] mandates a maximum
1047	   BGP message size of 4096 octets.  It is RECOMMENDED that an
1048	   implementation support [RFC8654] to accommodate a larger size of
1049	   information within the BGP-LS Attribute.  BGP-LS Producers MUST
1050	   ensure that they limit the TLVs included in the BGP-LS Attribute to
1051	   ensure that a BGP update message for a single Link-State NLRI does
1052	   not cross the maximum limit for a BGP message.  The determination of
1053	   the types of TLVs to be included MAY be made by the BGP-LS Producer
1054	   based on the BGP-LS Consumer applications requirement and is outside
1055	   the scope of this document.  When a BGP-LS Propagator finds that it
1056	   is exceeding the maximum BGP message size due to addition or update
1057	   of some other BGP Attribute (e.g.  AS_PATH), it MUST consider the
1058	   BGP-LS Attribute to be malformed and handle the propagation as
1059	   described in Section 7.2.2.

While discussing RFC 8654 is appropriate, a related deployment consideration
should be mentioned:  If the BGP route propagation path is such that a large
message (> 4096 octets) is created, it can only be sent along a contiguous path
of BGP sessions that all support RFC 8654.

Since loss of messages can result in incomplete link state information, this may
result in a broken LSDB.

1174	4.3.1.5.  Opaque Node Attribute TLV

1176	   The Opaque Node Attribute TLV is an envelope that transparently
1177	   carries optional Node Attribute TLVs advertised by a router.  An
1178	   originating router shall use this TLV for encoding information
1179	   specific to the protocol advertised in the NLRI header Protocol-ID
1180	   field or new protocol extensions to the protocol as advertised in the
1181	   NLRI header Protocol-ID field for which there is no protocol-neutral
1182	   representation in the BGP Link-State NLRI.  The primary use of the
1183	   Opaque Node Attribute TLV is to bridge the document lag between,
1184	   e.g., a new IGP link-state attribute being defined and the protocol-
1185	   neutral BGP-LS extensions being published.

This, and the related Opaque sections could use some discussion about what
happens when that bridge has been crossed.

Consider a feature that can move from a new link-state attribute carried in the
Opaque information and has a code point later added to carry in BGP-LS:  There's
possibly an incremental deployment motivation to carry the information in both
spots.  This means keeping the values consistent in each place becomes important.

1273	      0                   1                   2                   3
1274	      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
1275	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1276	     |              Type             |             Length            |
1277	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1278	     |L|R|  Reserved |
1279	     +-+-+-+-+-+-+-+-+

1281	                     Figure 19: MPLS Protocol Mask TLV

Here, and later in the document, "Reserved" likely should be "MUST be set to
zero and SHOULD be ignored on receipt".


1451	      0                   1                   2                   3
1452	      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
1453	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1454	     |              Type             |             Length            |
1455	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1456	     |D|N|L|P|       |
1457	     +-+-+-+-+-+-+-+-+

1459	                      Figure 25: IGP Flag TLV Format

Suggest marking the empty space as Reserved.  See prior comment about Reserved.


2032	6.1.4.  BGP-LS Node Flags Registry

2034	   The "BGP-LS Node Flags" registry is requested to be created for the
2035	   one-octet sized flags field of the Node Flag Bits TLV (1024) and
2036	   populated with the initial values shown below:

2038	              Bit   Description             Reference
2039	              -----------------------------------------------
2040	               0    Overload Bit (O-bit)    [This document]
2041	               1    Attached Bit (A-bit)    [This document]
2042	               2    External Bit (E-bit)    [This document]
2043	               3    ABR Bit (B-bit)         [This document]
2044	               4    Router Bit (R-bit)      [This document]
2045	               5    V6 Bit (V-bit)          [This document]
2046	               6-7  Unassigned

2048	   Allocations within the registry under the "RFC Required" policy (see
2049	   [RFC8126]).

This registry is absent from the IANA Considerations.  Similarly, IANA does not
have a registry in the expected place:
https://www.iana.org/assignments/bgp-ls-parameters/bgp-ls-parameters.xhtml

2051	6.1.5.  BGP-LS MPLS Protocol Mask Registry

2053	   The "BGP-LS MPLS Protocol Mask" registry is requested to be created
2054	   for the one-octet sized flags field of the MPLS Protocol Mask TLV
2055	   (1094) and populated with the initial values shown below:

2057	    Bit   Description                                Reference
2058	    ------------------------------------------------------------------
2059	     0    Label Distribution Protocol (L-bit)        [This document]
2060	     1    Extension to RSVP for LSP Tunnels (R-bit)  [This document]
2061	     2-7  Unassigned

2063	   Allocations within the registry under the "RFC Required" policy (see
2064	   [RFC8126]).

See prior comment about missing IANA Considerations and registry at IANA.

2066	6.1.6.  BGP-LS IGP Prefix Flags Registry

2068	   The "BGP-LS IGP Prefix Flags" registry is requested to be created for
2069	   the 1 octet sized flags field of the IGP Flags TLV (1152) and
2070	   populated with the initial values shown below:

2072	        Bit   Description                        Reference
2073	        ----------------------------------------------------------
2074	         0    IS-IS Up/Down Bit (D-bit)          [This document]
2075	         1    OSPF "no unicast" Bit (N-bit)      [This document]
2076	         2    OSPF "local address" Bit (L-bit)   [This document]
2077	         3    OSPF "propagate NSSA" Bit (P-bit)  [This document]
2078	         4-7  Unassigned

2080	   Allocations within the registry under the "RFC Required" policy (see
2081	   [RFC8126]).

See prior comment about missing IANA Considerations and registry at IANA.