[Rift] AD Review draft-ietf-rift-rift-16 (§4.2.3 thru §4.2.3.2)

[Alvaro Retana <[e-mail]>]

Hi!

This message covers §4.2.3 to the end of §4.2.3.2.

Alvaro.



2207	4.2.3.  Topology Exchange (TIE Exchange)

2209	4.2.3.1.  Topology Information Elements
...
2214	   The TIE exchange mechanism uses the port indicated by each node in
2215	   the LIE exchange as `flood_port` in `LIEPacket` and the interface on
2216	   which the adjacency has been formed as destination.  TIEs MUST be
2217	   sent with an IPv4 Time to Live (TTL) or an IPv6 Hop Limit (HL) of
2218	   either 1 or 255 and also MUST be ignored if received with values
2219	   different than 1 or 255.  This prevents RIFT information from
2220	   reaching beyond a single L3 next-hop in the topology.  TIEs SHOULD be
2221	   sent with network control precedence unless an implementation is
2222	   prevented from doing so [RFC2474].

[major] "This prevents RIFT information from reaching beyond a single
L3 next-hop in the topology."

This is not completely true for TTL = 1.  It is true that the TIE
cannot be forwarded beyond the local subnet.  But it is not true that
a TIE cannot be forwarded *into* the local subnet.  This is one of the
attack vectors that are left open -- these are the same concerns as
with the LIEs.



2224	   TIEs contain sequence numbers, lifetimes and a type.  Each type has
2225	   ample identifying number space and information is spread across
2226	   possibly many TIEs of a certain type by the means of a hash function
2227	   that an implementation can individually determine.  One extreme
2228	   design choice is a prefix per TIE which leads to more BGP-like
2229	   behavior where small increments are only advertised on route changes
2230	   vs. deploying with dense prefix packing into few TIEs leading to more
2231	   traditional IGP trade-off with fewer TIEs.  An implementation may
2232	   even rehash prefix to TIE mapping at any time at the cost of
2233	   significant amount of re-advertisements of TIEs.

[major] "TIEs contain sequence numbers, lifetimes and a type."

There is one TIE, but multiple types of TIEElements, right?  And each
TIE carries only one TIEElement at a time, right?

If so, then "information is spread across possibly many TIEs of a
certain type" should say "across multiple TIEElements", or maybe
"across multiple TIEs with the same TIEElement type", or something
like that.


[major] "...information is spread across possibly many TIEs of a
certain type by the means of a hash function that an implementation
can individually determine."

Does the receiver need to know the hash function?  I'm hoping the answer is No.

The rest of this paragraph only talks about prefixes.  Even if
possibly less common, I assume that the ability to spread information
applies to all TIEElement Types.  Is that true?  If so, please be
explicit -- and maybe keep the text about prefixes as an example.


[major] Please remove comparisons or references to other protocols:
"BGP-like behavior...traditional IGP".



...
2238	4.2.3.2.  Southbound and Northbound TIE Representation
...
2248	   The North TIEs hold all of the node's adjacencies and local prefixes
2249	   while the South TIEs hold only all of the node's adjacencies, the
2250	   default prefix with necessary disaggregated prefixes and local
2251	   prefixes.  Section 4.2.5 explains further details.

[minor] I know I should go look at §4.2.5, but the description of
doesn't seem correct:

"North TIEs hold all of the node's adjacencies and local prefixes"

"South TIEs hold only" -- "only" seems to indicate less information --
"all of the node's adjacencies, the default prefix with necessary
disaggregated prefixes and local prefixes".   So the South TIEs have
more information.

Maybe it's a matter of eliminating "only".



2253	   The TIE types are mostly symmetric in both directions and Table 3
2254	   provides a quick reference to main TIE types including direction and
2255	   their function.  The direction itself is carried in `direction` of
2256	   `TIEID` schema element.

[major] The TIE types (the table says "TIE-Types", please be
consistent) in the table are not the same as the types defined in the
schema:

/** Type of TIE.
*/
enum TIETypeType {
    Illegal                                     = 0,
    TIETypeMinValue                             = 1,
    /** first legal value */
    NodeTIEType                                 = 2,
    PrefixTIEType                               = 3,
    PositiveDisaggregationPrefixTIEType         = 4,
    NegativeDisaggregationPrefixTIEType         = 5,
    PGPrefixTIEType                             = 6,
    KeyValueTIEType                             = 7,
    ExternalPrefixTIEType                       = 8,
    PositiveExternalDisaggregationPrefixTIEType = 9,
    TIETypeMaxValue                             = 10,
}

See my comment above about differentiating between the types of TIEs
(shown in the table) and the types of TIEElements (defined in the
schema).  As is, the terminology is confusing because TIE types seem
to have two different meanings. (The last sentence in this section
also uses the same confusing terminology.)

A simplistic interpretation indicates that a TIE type is the same as
the corresponding TIEElement type, but with a direction.



2258	       +=========================+=================================+
2259	       | TIE-Type                | Content                         |
2260	       +=========================+=================================+
2261	       | Node North TIE          | node properties and adjacencies |
2262	       +-------------------------+---------------------------------+
2263	       | Node South TIE          | same content as node North TIE  |
2264	       +-------------------------+---------------------------------+
2265	       | Prefix North TIE        | contains nodes' directly        |
2266	       |                         | reachable prefixes              |
2267	       +-------------------------+---------------------------------+

[nit] s/nodes'/node's


2268	       | Prefix South TIE        | contains originated defaults    |
2269	       |                         | and directly reachable prefixes |
2270	       +-------------------------+---------------------------------+

[minor] "directly reachable prefixes"

The text above, and a definition in the schema, use "local prefixes"
instead.  Please be consistent.


2271	       | Positive Disaggregation | contains disaggregated prefixes |
2272	       | South TIE               |                                 |
2273	       +-------------------------+---------------------------------+
2274	       | Negative Disaggregation | contains special, negatively    |
2275	       | South TIE               | disaggregated prefixes to       |
2276	       |                         | support multi-plane designs     |
2277	       +-------------------------+---------------------------------+
2278	       | External Prefix North   | contains external prefixes      |
2279	       | TIE                     |                                 |
2280	       +-------------------------+---------------------------------+
2281	       | Key-Value North TIE     | contains nodes northbound KVs   |
2282	       +-------------------------+---------------------------------+
2283	       | Key-Value South TIE     | contains nodes southbound KVs   |
2284	       +-------------------------+---------------------------------+

2286	                             Table 3: TIE Types

[major] The table doesn't include all the TIEElement types + direction.



2288	   As an example illustrating a databases holding both representations,
2289	   the topology in Figure 2 with the optional link between spine 111 and
2290	   spine 112 (so that the flooding on an East-West link can be shown) is
2291	   considered.  Unnumbered interfaces are implicitly assumed and for
2292	   simplicity, the key value elements which may be included in their
2293	   South TIEs or North TIEs are not shown.  First, in Figure 15 are the
2294	   TIEs generated by some nodes.

2296	          ToF 21 South TIEs:
2297	          Node South TIE:
2298	            NodeElement(level=2, neighbors((Spine 111, level 1, cost 1),
2299	            (Spine 112, level 1, cost 1), (Spine 121, level 1, cost 1),
2300	            (Spine 122, level 1, cost 1)))
2301	          Prefix South TIE:
2302	            SouthPrefixesElement(prefixes(0/0, cost 1), (::/0, cost 1))

[major] s/NodeElement/NodeTIEElement/g
In order to match the schema.


[major] s/SouthPrefixesElement/PrefixTIEElement/g



[major] For the prefixes, "cost" is used.  However, the schema uses
"metric" in the PrefixAttributes structure:

/** Distance of the prefix. */
2: required common.MetricType            metric
        = common.default_distance;

Note that NodeNeighborsTIEElement uses different language to refer to
the same thing:

/**  Cost to neighbor. Ignore anything larger than `infinite_distance`
and `invalid_distance` */
3: optional common.MetricType               cost
            = common.default_distance;


The terminology section defines cost, but not metric.  Please be consistent.

The definitions are:

   Cost:  The term signifies the weighted distance between two neighbors.

   Distance:  Sum of costs (bound by infinite distance) between two nodes.


There is clearly some type of circular definition between the cost
("weighted distance") and the distance ("sum of costs").  ??

Also, note that "weighted" is only used in this definition -- except
when referring to weighted ECMP or UCMP.



2304	          Spine 111 South TIEs:
2305	          Node South TIE:

2307	            NodeElement(level=1, neighbors((ToF 21, level 2, cost 1,
2308	                        links(...)),
2309	            (ToF 22, level 2, cost 1, links(...)),
2310	            (Spine 112, level 1, cost 1, links(...)),
2311	            (Leaf111, level 0, cost 1, links(...)),
2312	            (Leaf112, level 0, cost 1, links(...))))

[minor] Why aren't the links included in the ToF 21 TIEs?  I know that
they are optional -- it just looks inconsistent.


2313	          Prefix South TIE:
2314	            SouthPrefixesElement(prefixes(0/0, cost 1), (::/0, cost 1))

2316	          Spine 111 North TIEs:
2317	          Node North TIE:
2318	            NodeElement(level=1,
2319	            neighbors((ToF 21, level 2, cost 1, links(...)),
2320	            (ToF 22, level 2, cost 1, links(...)),
2321	            (Spine 112, level 1, cost 1, links(...)),
2322	            (Leaf111, level 0, cost 1, links(...)),
2323	            (Leaf112, level 0, cost 1, links(...))))
2324	          Prefix North TIE:
2325	            NorthPrefixesElement(prefixes(Spine 111.loopback)

[major] s/NorthPrefixesElement/PrefixTIEElement/g

2327	          Spine 121 South TIEs:
2328	          Node South TIE:
2329	            NodeElement(level=1, neighbors((ToF 21,level 2,cost 1),
2330	            (ToF 22, level 2, cost 1), (Leaf121, level 0, cost 1),
2331	            (Leaf122, level 0, cost 1)))

[minor] The links are not shown here...



...
2362	   For node TIEs to carry more adjacencies than fit into an MTU, the
2363	   element `neighbors` may contain different set of neighbors in each
2364	   TIE.  Those disjoint sets of neighbors MUST be joined during
2365	   corresponding computation.  Nevertheless, in case across multiple
2366	   node TIEs

[nit] "node TIEs"

Some places capitalize "Node TIEs" -- please be consistent.


[nit] s/fit into an MTU/fit into an MTU-sized packet


[nit] s/contain different set/contain a different set


[major] "MUST be joined during corresponding computation"

How?  Given that this is a Normative requirement, you should indicate
how.  I get that this is a simple operation -- nonetheless, it should
be specified.  See more below.

Which "corresponding computations"?  I'm assuming that joining
(considering multiple sets of neighbors as one) should be done for
all.  IOW, are there specific cases where it is (not) needed?


2368	   1.  `capabilities` do not match *or*

2370	   2.  `flags` values do not match *or*

2372	   3.  same neighbor repeats in multiple TIEs with different values

2374	   the behavior is undefined and a warning SHOULD be generated after a
2375	   period of time.

[major] "behavior is undefined and a warning SHOULD be generated"

So, when joining (required above), what should a node do if a specific
"neighbor repeats in multiple TIEs with different values".  I know
you're saying that the behavior is undefined, but what happens to that
neighbor?  Is it still considered "during corresponding computation"?
Which version?

It seems to me that generating a warning implies that the node keeps
working -- I just don't understand which state it uses going forward.

The same concerns apply to `capabilities` and `flags`.


[major] "a warning SHOULD be generated after a period of time"

How long is that period?  Is there a timer somewhere?



2377	   The element `miscabled_links` SHOULD be repeated in every node TIE,
2378	   otherwise the behavior is undefined.

[major] "SHOULD be repeated"

When is it ok to not repeat it?  It seems like the answer is "never",
otherwise the behavior is undefined.  Why is this behavior recommended
and not required?

I see, from the schema, that `miscabled_links` is optional, which
means that it would be used only if any links are miscabled.  I can't
find a place that talks about what is considered a miscabled link (I
seem to remember having this discussion before) -- the only other
mention of "miscabled" is in §4.2.7.3, so (even if there's no explicit
mention) it looks like it (or perhaps the whole ZTP section) might be
a good enough reference.

Suggestion>
   If the fabric is miscabled (Secion xxx), the element `miscabled_links`
   MUST be repeated in every Node TIE.  If it isn't, the behavior is
   undefined.

BTW, which behavior is undefined?  I can't find a place that talks
about the effect of miscabling.

Also, no warning is needed in this case?



2380	   A top of fabric node MUST include in the node TIEs in
2381	   `same_plane_tofs` element all the other ToFs it is aware of through
2382	   reflection.  To prevent MTU overrun problems, multiple node TIEs can
2383	   carry disjoint sets of ToFs which can be joined to form a single set.
2384	   This element allows nodes in other planes that are on the multi-plane
2385	   ring with this node to have information describing the complete plane
2386	   and with that all ToFs in a multi-plane fabric are aware of all other
2387	   ToFs which can be used further to form input to complex multi-plane
2388	   elections.

[major] "A top of fabric node MUST include...in `same_plane_tofs`
element all the other ToFs..."

The requirement seems to indicate that "all the other ToFs" have to be
included in the element, but that is not strictly true because the set
can be divided into subsets...

Suggestion>
   A ToF none MUST include all the other ToFs it is aware of
   through reflection.  The `same_plane_tofs` element is used
   to carry this information.


[major] "can be joined to form a single set"

This text is not a requirement (as was the text above about joining
neighbor sets).  Should it be?  Why would it be different?


[nit] "...can be used further to form input to complex multi-plane elections."

It would be nice to reference where that is specified.



2390	   Different TIE types are carried in `TIEElement`.  Schema enum
2391	   `common.TIETypeType` in `TIEID` indicates which elements MUST be
2392	   present in the `TIEElement`. In case of mismatch the unexpected
2393	   elements MUST be ignored.  In case of lack of expected element in the
2394	   TIE an error MUST be reported and the TIE MUST be ignored.  The
2395	   element `positive_disaggregation_prefixes` and
2396	   `positive_external_disaggregation_prefixes` MUST be advertised
2397	   southbound only and ignored in North TIEs.  The element
2398	   `negative_disaggregation_prefixes` MUST be aggregated and propagated
2399	   according to Section 4.2.5.2 southwards towards lower levels to heal
2400	   pathological upper level partitioning, otherwise traffic loss may
2401	   occur in multiplane fabrics.  It MUST NOT be advertised within a
2402	   North TIE and ignored otherwise.

[minor] "Different TIE types are carried in `TIEElement`."

See my comment earlier about the types of TIEs and how the terminology
is confusing.  The last paragraph in the next section also mentions
"TIE types".


[major] "Schema enum `common.TIETypeType` in `TIEID` indicates which
elements MUST be present in the `TIEElement`."

No -- TIETypeType is just an enum, it doesn't indicate any required elements.


[major] "In case of mismatch...In case of lack of expected element..."

The schema says that all the parts of TIEElement are optional:

/** Single element in a TIE. */
union TIEElement {
    /** Used in case of enum common.TIETypeType.NodeTIEType. */
    1: optional NodeTIEElement     node;
    /** Used in case of enum common.TIETypeType.PrefixTIEType. */
    2: optional PrefixTIEElement          prefixes;
    /** Positive prefixes (always southbound). */
    3: optional PrefixTIEElement   positive_disaggregation_prefixes;
    /** Transitive, negative prefixes (always southbound) */
    5: optional PrefixTIEElement   negative_disaggregation_prefixes;
    /** Externally reimported prefixes. */
    6: optional PrefixTIEElement          external_prefixes;
    /** Positive external disaggregated prefixes (always southbound). */
    7: optional PrefixTIEElement
            positive_external_disaggregation_prefixes;
    /** Key-Value store elements. */
    9: optional KeyValueTIEElement keyvalues;
} (python.immutable = "")


I see that each of the parts *TIEElement has required elements in it.
The text above talks about required elements in TIEElement which is
not correct.  They are required elements in the optional components
(*TIEElement) of TIEElement.  Please be precise.


[minor] "unexpected elements MUST be ignored"

Is it just unexpected or also unknown?  I guess unknown are
unexpected, but this sentence talks about a mismatch, indicating that
there are specific yes/no elements (not unknown).


[major] "The element `positive_disaggregation_prefixes` and
`positive_external_disaggregation_prefixes` MUST be advertised
southbound only and ignored in North TIEs."

These elements are optional, so the "MUST" doesn't work.

Suggestion>
   The `positive_disaggregation_prefixes` and
   `positive_external_disaggregation_prefixes` elements MAY be
   advertised in South TIEs and MUST be ignored if advertised
   in North TIEs.


[major] "The element `negative_disaggregation_prefixes` MUST be
aggregated and propagated according to Section 4.2.5.2 southwards
towards lower levels to heal pathological upper level partitioning,
otherwise traffic loss may occur in multiplane fabrics. It MUST NOT be
advertised within a North TIE and ignored otherwise."

- §4.2.5.2 doesn't mention this element (or PrefixTIEElement)
explicitly.  The specification seems to be in §4.2.5.2.2.

- s/aggregated/disaggregated

- "MUST be aggregated and propagated according to Section 4.2.5.2"

§4.2.5.2.2 doesn't require the behavior (MUST), it recommends it
(SHOULD).  Instead of making both sentences use the same word, please
specify the behavior only once.  §4.2.5.2.2 seems like the right
place.

- "...to heal pathological upper level partitioning, otherwise traffic
loss may occur in multiplane fabrics."

If the specification is somewhere else, then this explanation should
be there too.


Suggestion>
   The `negative_disaggregation_prefixes` elements MAY be
   advertised in South TIEs and MUST be ignored if advertised
   in North TIEs.

[EoR - Feb/13]