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

[Tony Przygienda <[e-mail]>]

missed all reply

catching up on reviews today ...

On Mon, Feb 13, 2023 at 2:00 PM Alvaro Retana <[e-mail]>
wrote:

> 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.
>

? RIFT works on p2p links which are always an L3 next hop. Hence
the subnet is 2 routers only (be it v4, v6 or v6 LLC)


>
>
>
> 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.
>

ok, I like your last suggestion


>
>
> [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.
>

answer is No. Each node is completely free to choose how it spreads the
info across TIEs of same type.


>
> 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.
>

RIFT being very symmetrical this applies to _all_ tie types. we can mention
that, giving more examples
I think is counter productive.


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

ack


>
>
>
> ...
> 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".
>

Yes, south ties hold less information. The text is correct. North TIEs are
really a full link state database of the node (since we have north flooding)
while south is a "compressed" view with mostly just default prefix


>
>
>
> 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:
>

ok, let's remove the table and refer to schema only


>
> /** 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.
>

yes, that's one way to see it, a TIE has a type and the type forces the
according
schema element


>
>
>
> 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.
>

ok, yes, let's align the terminology to the schema


>
>
> 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.
>

as I said, let's remove it and refer to schema only to prevent copies

>
>
>
> 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.
>

ok


>
> [major] s/SouthPrefixesElement/PrefixTIEElement/g
>
>
>
> [major] For the prefixes, "cost" is used.  However, the schema uses
> "metric" in the PrefixAttributes structure:
>

ok, yeah, can do.


>
> /** 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.
>

cost = sum of metrics. We should include that in the glossary. Having said
that

cost to neighbor should be metric really and adjusted (since it's not sum).


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

yeah, I don't want to go full tilt math definitions where topology defines
those terms strictly but
we should unify the terms here

metric = how far is something in one hop
cost = sum of metrics = distance


> 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.
>

oversight or optimizing space. Jordan, pls check


>
>
> 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
>

yeah, stuff changed over time


>
> 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...
>

same. simplification.


>
>
> ...
> 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.
>

ack


>
> [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?
>

tons of them that are touching node TIEs, reachability, checks on all kind
of algorithms. Explained in the document throughtout

Join has to be done for all. Join is bits tricky, we should probably be
normative as you say

run over all TIEs sorted on IDs and join all found copies together


>
>
> 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`.
>

well, we can say, sort based on TIE ID and use last found.


>
>
> [major] "a warning SHOULD be generated after a period of time"
>
> How long is that period?  Is there a timer somewhere?
>

impossible to specify, maybe we should say "generated after a configurable
period of time" ?

>
>
>
> 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?
>

Miscabling is purely operational help, that's also why it's "SHOULD"

An implementation may NOT do anything. And miscabling is impossible to
define, different people have different opinions what is miscabled and an
implemention is free to choose what it considers "miscabled". Classical
would
be subnet mismatch (the simplest case) but e.g. all ISIS implementations
include a knob to override it.

So, nothing to specify on the wire except "SHOULD include miscabled" AFAIS.


>
>
>
> 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.
>

ack. yes, agreed.


>
>
> [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?
>

yeah, it should be normative


>
> [nit] "...can be used further to form input to complex multi-plane
> elections."
>
> It would be nice to reference where that is specified.
>

this is in auto-evpn and auto-fabric drafts. I don't think we should
reference those.

Maybe we can just kick the whole sentence here.


>
>
> 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".
>

ok


>
>
> [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.
>

yes, it does. When it's set on the TIEID the type determines what is
carried in the `TIEElement`


>
>
> [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.
>
>
ok, you're splitting hairs AFAIS but fine. As I said the TIEType on the
TIEID
indicates WHICH of those opetional elements MUST be set and there is text
saying what to do on mismatches. AFAIS spec is sufficient but we can
do more "precise" verbiage.


>
> [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).
>

what is "unknown". The treatement of not understood TIETypes is
described AFAIR (flood on),

>
>
> [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.
>

they are "MUST" when the TIE Type is set accordingly as
explained in previous paragraph. I think writing
 a whole novel repeating all the causality/normative chain that is
logically compiled during
the text of the spec on every condition will not be particualrly
productive.


>
> 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.
>

that breaks the spec. It's _not_ a may, in case positive disaggreagtion
is needed, the stuff MUST be advertised southbound.


>
> [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 PrefixTIEElemen
>
> - 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.
>

there is a whole section on negative disaggregation in the intro and
previous
comment applies as well, i.e. it is a MUST _if_ negative disaggregation
happens.
If we missed that negative MUST be computed & propagated then we need to
fix that in
context.
Again, we cannot write every time the whole
causality chain repeating the whole context as in (well, if _this_ applies
and _this_ is set and as said before, this is true, then you MUST do that).
Either the reader keeps the correct context (i.e. we have negative
disaggreagation scenario _and_ the tie ID type is set correctly _and_
direction is this, THEN) or otherwise every sentence will become 1/2 spec
repeated.


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

again, AFAIS, that breaks the spec since you consider only the context of
packet format and not the operational condition.