Subject: Re: [Rift] AD Review of draft-ietf-rift-rift-12 (Part 2a)

From: Alvaro Retana <[email-masked]>
Date: Thu, 1 Apr 2021 13:08:12 -0700

[+ authors, WG, Shepherd, Chairs]

On March 7, 2021 at 3:56:09 PM, Tony Przygienda wrote:

Tony:

Hi!

I have some replies to your answers (inline).

Some of your answers point me to the schema -- which is ok, we had
agreed on it being Normative.  But it makes me wonder, what are we
standardizing?  I know we had talked about having the schema in the
document -- but I don't think we're on the same page as to what that
means.  I'm starting a separate thread with the WG (not everyone plays
attention to the review threads), so let's please talk about it there.

Thanks!

Alvaro.

...
> > 1341 4.2. Specification
...
> > 1358 Any attempt to transition from a state towards another on reception
> > 1359 of an event where no action is specified MUST be considered an
> > 1360 unrecoverable error.
...
> > [major] "MUST be considered an unrecoverable error" What is the result of
> > that? Should the adjacencies be reset, the rift process restarted, the
> > interfaces shut down, etc.?? There's no other place in the document that
> > mentions "unrecoverable".
>
> implementation dependent. It's pretty much the same as the protocol throwing
> a "core" or memory corruption. From that point behavior is unspecified but we
> want it to be considered something that should not attempt a recovery. In
> most stark terms don't even start RIFT again but we can't mandate any such
> thing. It's implementation guidelines.

Why can't you mandate that?  This is *the* spec! :-)

If you need this behavior for interoperability (by using MUST), then
you need to specify what the behavior is.  Yes, there are probably
internals that you can't specify, but the external behavior is fair
game: "all adjacencies MUST be reset and not be restarted" (or
something like that).

...
> > 1371 4.2.1. Transport
> >
> > 1373 All packet formats are defined in Thrift [thrift] models in
> > 1374 Appendix B.
> >
> > [] Can we get an index/TOC or some type of guide in the appendix to make
> > it easier to find specific parts? For example, finding where a LIE is
> > defined is not straight forward -- I happened to stumble on it in B.2.
> > Another option would be to point in §4.2.2 to look for LIEPacket in B.2.
>
> yes, as we said there will be a more extensive intro and a reader's guide.
> And I'll refer to model & according structure.

An idea just occurred to me -- not sure if it would work: would a
summary similar to the overview of a YANG module work?  It might not
end up looking like a tree, and it would include packets/fields (not
ro/rw information)...but it might show a summary of the schema without
showing the full schema upfront.

Just thinking out loud.

...
> > 1376 The serialized model is carried in an envelope within a UDP frame
> > 1377 that provides security and allows validation/modification of several
> > 1378 important fields without de-serialization for performance and
> > 1379 security reasons.
> >
> > [minor] This is a very short section -- please put references to other
> > places where these topics are explained more, if any.
>
> aha, ok. I probably expound the section a bit as in "this MD5 protects this
> and then this MD5 on top protects this" so e'thing is completely convered.

MD5 won't pass the Security scrutiny.   I just did a quick scan and
found the Security Envelope, but no details on what is MTI in terms of
algorithms, etc.

...
> > 1381 4.2.2. Link (Neighbor) Discovery (LIE Exchange)
...
> > [major] "An implementation MAY listen and send LIEs on IPv4 and/or IPv6
> > multicast addresses. A node MUST NOT originate LIEs on an address family
> > if it does not process received LIEs on that family."
...
> So, generally I prefer this flexiblity the spec has now since practically,
> all kind of combinations arise (add v4ov6 to the whole thing) & that's also
> why we have AFs carried on the link information as well to be able to figure
> out during SPF/from topology what AFs are on and detect discontinuity if so
> desired or even allow to fwd. v4 normal then jump a v4ov6 and get there.
...

To be clear, I like the flexibility.  The comment is about the
specification: as is, it is easy to deadlock.  You can either try to
"fix" the specification (make it so that somehow there's no deadlock),
or you can add some "disclaimers" about the need for the operator to
configure things right -- and declare that outside the scope of this
document.

[I have a suggestion later on.]

...
> > 1418 A three-way adjacency over any address family implies support for
> > 1419 IPv4 forwarding if the `v4_forwarding_capable` flag is set to true
> > 1420 and a node can use [RFC5549] type of forwarding in such a situation.
> > 1421 It is expected that the whole fabric supports the same type of
> > 1422 forwarding of address families on all the links. Operation of a
> > 1423 fabric where only some of the links are supporting forwarding on an
> > 1424 address family and others do not is outside the scope of this
> > 1425 specification.
...
> > [major] "`v4_forwarding_capable` flag" (This comment is not specific to
> > this flag.)
> >
> > The specific LIE packet format has not been presented, introduced or even
> > referenced up to now. Pointing to a flag, which happens to be optional in
> > an optional part of the LIEPacket, comes out of the blue and feels
> > completely out of place without prior explanation.
>
> I can try to describe all the different scenarios of v4/v6/v4ov6/mix but fact
> is, it will read more and more confused so I try to keep it fairly dry and
> short. It's obvious when one deploys/implements.

I'm sure it's obvious to you! ;-)

> ```
> A three-way adjacency over any address family implies support for IPv4
> forwarding if the `v4_forwarding_capable` flag is set to true and a node can
> use type of forwarding in such a situation. It is expected that the whole
> fabric supports the same type of forwarding of address families on all the
> links. Operation of a fabric where only some of the links are supporting
> forwarding on an address family and others do not is outside the scope of
> this specification.
> ```
>
> reads completely clear to me with ref to 5549 so what am I supposed to add
> that will clarify it further without muddying things.

You are not the audience.

...
> > [major] "a node can use [RFC5549] type of forwarding"
> >
> > First of all, rfc5549 is a BGP-specific RFC. How do the BGP mechanisms
> > defined there (and in rfc8950) map to RIFT? IOW, please specify how RIFT
> > works.
> >
> > Because you listed rfc5549 as a Normative reference, I assume you want
> > this behavior to be normative. s/can use/MUST/SHOULD/MAY ??
>
> Sigh, Alvaro, is it really the job of RIFT to start describe forwarding path
> all major chip vendors implemented? I do NOT think it's a wise idea. It
> refers to RFC5549 because the forwarding behavior is ultimately precisely
> the same but I don't want to write models of forwarding paths, ARP tables
> etc.
>
> The protocol spec describes all the control plane necessary to interop
> properly, it does not explain LPM or v4ov6 forwarding or any such thing.
>
> I would expect some more guidance here but again, this is not an
> implementation primer for a DC switch including fwd path, it's a control
> protocol spec.

Some comments:

- rfc2328/rfc4271 don't include references to other protocols when
describing how the protocol works -- I expect the same from RIFT.
This is exactly the same point as referencing OSPF/ISIS...

- No, you don't need to describe chip implementations -- if you do,
then the document is focusing on the wrong parts.  The specification
should (as you mention above) focus on describing the control plane.

- If the forwarding plane is out of scope, then say that.  If
possible, recommend (don't require) behavior.  It is better to leave
details (that can change based on internal implementation details) out
of scope than trying to address every corner case.

Let's go back to the paragraph above:

1418	   A three-way adjacency over any address family implies support for
1419	   IPv4 forwarding if the `v4_forwarding_capable` flag is set to true
1420	   and a node can use [RFC5549] type of forwarding in such a situation.
1421	   It is expected that the whole fabric supports the same type of
1422	   forwarding of address families on all the links.  Operation of a
1423	   fabric where only some of the links are supporting forwarding on an
1424	   address family and others do not is outside the scope of this
1425	   specification.

rfc5549/rfc8959 don't explicitly talk about forwarding -- they talk
about the extensions to BGP to carry IPv6 NEXT_HOPs when advertising
an IPv4 route.  The use of that information is not specified; instead,
pointers to where it could be applied (for example, softwire
applications) is all that is provided.

Given that, what does it mean to to support "[RFC5549] type of forwarding"?

In this document, you haven't yet talked about how the routes are
calculated.  But I'm assuming that when that happens you will clearly
specify where the next hop comes from --- and the fact that if the
adjacency is over an IPv6 link and v4_forwarding_capable is set, then
the IPv6 next hop can be used to forward IPv4 traffic.  To explain
that you don't need to go into details of how it could be done, just
that it can.

BTW, if the adjacency is over IPv4 then the "[RFC5549] type of
forwarding" is not needed.  Or is it?  Below you mention support for
"v4 over v6 only", but not IPv6 over IPv4 only -- was that just an
omission, or is that not supported?  I'm assuming it is.  The point
here is that the text talks about support for IPv4, but it says
nothing about IPv6 -- that is the reason for the rest of the questions
below.

> > [major] "It is expected that the whole fabric supports the same type of
> > forwarding of address families on all the links. Operation of a fabric
> > where only some of the links are supporting forwarding on an address
> > family and others do not is outside the scope of this specification."
> >
> > Please remind me, is IPv4 optional or required? I'm assuming that IPv6 is
> > required and IPv4 optional, but I can't find anything definitive in the
> > document.
>
> neither is required. RIFT will work with any combination (v4 only, v6 only,
> v4 and v6, v4 over v6 only). If neither is implemented then there is no
> forwarding obviously. LIEs basically exchange enough info over both AF and
> the 'v4 capable' flag to negotiate _any_ viable combination. So what do you
> want me to say? People deploy all of the above variants & are religious about
> it. Why would I say "v6 is required" if protocol works just fine with v4
> only. And why have "v4 required" if it works fine with v6 only as well.

Let's take a step back.  At this point (§4.2.2) we're talking about
LIE Exchange.

There's no explicit signaling for IPv6 support (ala
v4_forwarding_capable).  So the only way to know if a neighbor
supports IPv6 forwarding is to assume that it does because the LIEs
were sent form an IPv6 address.  OR  Once I get to the section about
TIEs it'll probably explain that both IPv4/IPv6 routes can be
advertised regardless of which AF the adjacency was formed on.   These
are just guesses (because the text so far doesn't say neither).  We
may be getting ahead of ourselves with all this talk about forwarding.

...
> You cannot figure that out BTW, a fabric may partition along AFs
> supported/capabilities and unless you run reachabiliuty per AF the fabric
> will blackhole likely. Running computona per AF is a very bad idea,
> partitioned (partially connected) networks are operationally a nightmare. I
> learned that the hard way in MT years ago.
>
> Applicability draft may want to loose a word or two about it.

I'm asking here because this document sets an expectation.  If the
protocol expects something, then it would be nice to know (at least)
what happens if the expectation is not met -- or, even better, if the
protocol has a mechanism to get out of trouble.

I am ok with some of these considerations (I mention operational
considerations in other places too) to be in the applicability draft.
This document should then reference the applicability one.  Also, I
will progress them together.

One more thing; I will probably continue to make comments about
operational considerations with the objective to not forget to later
check the applicability draft for guidance.

...
> > 1436 Unless ZTP as described in Section 4.2.7 is used, each node is
> > 1437 provisioned with the level at which it is operating. It MAY be also
> > 1438 provisioned with its PoD. If any of those values is undefined, then
> > 1439 accordingly a default level and/or an "undefined" PoD are assumed.
> > 1440 This means that leaves do not need to be configured at all if initial
> > 1441 configuration values are all left at "undefined" value. Nodes above
> > 1442 ToP MUST remain at "any" PoD value which has the same value as
> > 1443 "undefined" PoD. This information is propagated in the LIEs
> > 1444 exchanged.
...
> > [minor] "It MAY be also provisioned with its PoD." What does this mean?
> > Does it mean that all the nodes in the PoD have the same level? ...
>
> I don't understand how PoD and level can be confused. The glossary is super
> explicit about those things. There is never anywhere anything that somehow
> implies PoD numbering has anything to do with level.

That's one of the reasons I don't understand what this sentence means.
The full text is:

   Unless ZTP as described in Section 4.2.7 is used, each node is
   provisioned with the level at which it is operating.  It MAY be also
   provisioned with its PoD.  If any of those values is undefined, then
   accordingly a default level and/or an "undefined" PoD are assumed.

It looks like the "It" refers to "level".  Does it?  Or are you
referring to the node (maybe that's it)?

If the latter, then s/MAY/may because you seem to be stating a fact.

...
> > [major] BTW, this reminds me, what are the manageability considerations
> > related to RIFT? Among other things, it would be nice to have a summary of
> > parameters that are expected to be configured and their defaults. Please
> > take a look at rfc5706.
>
> Alvaro, since when are manageability considerations part of the base protocol
> spec? we have yang model spec, we have applicability spec, we can have a
> manageablity spec, fine with me. Base spec is here to say enough to build an
> interoperable implementation. That has been proven by a clean room open
> source that inter'oped day one.

See rfc5706.

...
> > 1449 A node tries to form a three-way adjacency if and only if
...
> > [major] "if and only if" Can this be translated into a Normative statement?
> > Are all these required (MUST) before the formation of a 3-way adjacency
> > started?
>
> I do NOT want to touch the LIE rules, those have been ironed over 2
> implementations super carefully and held. mathematicians use IIF religiously,
> good enough for me. Beside me @ least 2 or 3 people implemented those rules
> correctly (based on interop) already so the description clearly works.

I'm not asking you to change the rules.  I'm asking you to express the
rules in the "IETF's language" (rfc2119).

The fact that some people understand the meaning -- and that these are
probably the same people who have participated in writing the text
doesn't make it clear to others.  The specification is not for your
consumption.

...
> > 1454 2. the neighboring node is running the same MAJOR schema version AND
...
> > [minor] Are there any requirements related to the minor version?
>
> the schema versioning does describe that in excruciating details @ beginning
> of appendix B. I'll fwd reference

Sorry, I meant: for this step, does the minor version matter?  Or is
it ok if just the major version is the same?

> > 1456 3. the neighbor is not member of some PoD while the node has a
> > 1457 northbound adjacency already joining another PoD AND
...
> > [major] "not [a] member of [the] some PoD" #1 says that the "node is in
> > the same PoD" -- it looks like there's some context missing -- I guess
> > about the ability to join other PoDs. Is this only a northbound
> > characteristic?
>
> no, there is no such thing. You can basically configure PoD ooptionally and
> that's it if you choose. That gives protection against cross-PoD miswiring,
> there is nothing more here.

Maybe I didn't explain myself correctly, but I'm still confused:

1449	   A node tries to form a three-way adjacency if and only if

1451	   1.  the node is in the same PoD or either the node or the neighbor
1452	       advertises "undefined/any" PoD membership (PoD# = 0) AND
...
1456	   3.  the neighbor is not member of some PoD while the node has a
1457	       northbound adjacency already joining another PoD AND

#1 says "the node is in the same PoD", while #3 says that "the
neighbor is not member of some PoD", and these are joined by an "AND".
I don't understand how the neighbor can be in the same PoD, *and* not
be there at the same time.

If the case here is specifically related to the second part of #1
("or...advertises "undefined/any" PoD membership"), then that
condition should be clarified in #3.

...
> > 1464 6. the advertised MTUs match on both sides AND
...
> > [major]
> > struct LIEPacket {
> > ...
> > /** Layer 3 MTU, used to discover to mismatch. */
> > 4: optional common.MTUSizeType link_mtu_size =
> > common.default_mtu_size;
> >
> > If MTUSizeType is not included, where is it specified that the
> > default_mtu_size must be considered? Without that specification it is not
> > possible to satisfy this condition.
>
> all in schemas
> /** @note MUST be interpreted in implementation as unsigned */
> typedef i32 MTUSizeType
> /** default MTU link size to use */
> const MTUSizeType default_mtu_size = 1400
> encoding model includes common model

Well, that just tells me what the default is, not that if my neighbor
doesn't advertise anything I should consider the default.   The
language used in #6 refers to the "advertised MTU"...

> > 1466 7. both nodes advertise defined level values AND
> >
> > [major] The advertisement of LevelType in PacketHeader is optional. That
> > means that even if there is a default value a node may not "*advertise*
> > defined level values". Also, there are only 2 level values defined:
> >
> > const LevelType top_of_fabric_level = 24
> > const LevelType leaf_level = 0
> >
> > What am I missing?
>
> its own section explains ZTP algorithm in detail, advertisement etc
>
> It's impossible to describe in LIE processing section ZTP already, it's all a
> big muddle then. Level is optional because a ZTP node coming up has no level
> (except ToF) so it cannot show anything. Then ZTP starts to propagate, people
> get level offers, tie-break them, start to pick up levels & see whether that
> converges and so on.

Part of the problem that I'm having in this section is that this list
of conditions is preceded by: "A node tries to form a three-way
adjacency if and only if".  This sounds as if the conditions have to
be true even before the node tries to do anything.

Also, §4.2.7 says that ZTP is optional...and that "Configured nodes
and nodes operating in ZTP can be mixed".  Your answer implies that
ZTP takes care of figuring out what the level (in this case) is, and
that is great...if ZTP is used.  But if ZTP is not used, then what?
It seems to me that indicating the requirement that a configured node
always advertises its (configured) level would address the problem
with optional fields and defaults.

...
> > 1478 iii) both nodes are at level 0 AND both indicate support for
> > 1479 Section 4.3.8 OR
> >
> > [minor] "support for Section 4.3.8" Is there a name for that? How is it
> > indicated? §4.3.8 says that a leaf can "advertise the LEAF_2_LEAF flag in
> > its node capabilities"; there is nothing called "LEAF_2_LEAF" in the
> > schema, but "leaf_only_and_leaf_2_leaf_procedures" does show up. Please be
> > consistent in the terminology. [Also, there are places where "leaf-2-leaf"
> > is used.]
>
> yes, leaf-2-leaf procedures but it's a mouthful and it's not precise either
> since it's also leaf-only
>
> leaf-2-leaf is basically a common term for two flags in `
> HierarchyIndications
> `
> called
>
> leaf_only_and_leaf_2_leaf_procedures
>
> leaf_only
>
> whereas first includes second (it's a complex relationship we modelled like
> this)
>
> hence section 4.3.8 is bascially " anything around leaf only or leaf-2-leaf or
> both"
>
> So, I think refering to the section is still least confusing thing.

How can it be if the relationship above is not explained?  §4.3.8
talks about the non-existent LEAF_2_LEAF flag, and the relationship
with the two real flags is not explained anywhere.

> Scheam uses leaf_2_leaf because of thrift id rules but people prefer
> leaf-2-leaf in the document so far.

Please be consistent!

> > 1486 The rules checking PoD numbering MAY be optionally disregarded by a
> > 1487 node if PoD detection is undesirable or has to be ignored. This will
> > 1488 not affect the correctness of the protocol except preventing
> > 1489 detection of certain miscabling cases.
...
> > [major] "MAY be optionally disregarded by a node if PoD detection is
> > undesirable or has to be ignored."
> >
> > This is the only place where "PoD detection" is used. What is it? Where is
> > it specified? Why would be it undesirable? When would it have to be
> > ignored? Are there operational considerations for making that decision?
>
> in the LIE processing. you check whethe the other side matches your PoD or
> either one is ANY_POD=0 and that's it. No further magic.

Going back to the list, which starts with "if and only if" (which I
interpret as a requirement) followed by a bunch of ANDs.  Why would it
be undesirable to look at the PoD number?  When would it have to be
ignored?

Besides wanting to know the answers, which may be in the applicability
document, the fact that a piece of a required set of conditions is
optional is a contradiction.  Maybe the list is not required, just
recommended...??

> > [major] "...except preventing detection of certain miscabling cases."
> > Where is miscabling detection described? Which cases may not be covered?
> > §4.2.7.6 says that "internal ruleset flags a possible miscabling", but
> > that is the closest that I came to finding a description (and rulesets are
> > not mentioned anywhere else).
>
> miscabling computation is not part of the protocol spec. We allow to indicate
> "miscabling" but what is declared miscabling is impossible to specify.
> Peoiple do ALL kind of things in fabric, e.g. when your link starts to loose
> 5% some folks declare link "faulty" or "miscabled". There are no rules. An
> implementation can declare miscabling and do whatever it wants with the
> adjacency (probably hold down) and then it can put it into
>
> /** If any local links are miscabled, the indication is flooded. */
> 10: optional set miscabled_links;
>
> so you can look @ the ToF LSDB and see your whole fabric with links with
> problems. It's pretty well liked.

...but undefined...

The reason I asked about "miscabling" as a "major" comment is that it
is associated with Normative language (MAY), and the fact that by
disregarding that rule some functionality seems to be lost.  If
miscabling is not part of the spec, then it should not be part of the
spec.

There is no guidance anywhere about when the miscabled_links
indication can/should be set.  While the functionality may be
useful/liked, it could indicate anything -- in general, "there's an
issue".  I can wait for the applicability document to tell us about
any operational considerations, but it would be nice for the spec to
be clear about what the indication means.

Also, note that the first sentence of §4.2.2 says that "LIE
exchange...discovers miscablings", but in reality it doesn't.

...
> > 1507 4.2.2.1. LIE FSM
...
> > 1515 Event `MultipleNeighbors` occurs normally when more than two nodes
> > 1516 see each other on the same link or a remote node is quickly
> > 1517 reconfigured or rebooted without regressing to `OneWay` first. Each
> > 1518 occurrence of the event SHOULD generate a clear, according
> > 1519 notification to help operational deployments.
...
> > [minor] "occurs normally when..." Are there other conditions (which are
> > not considered "normal")?
>
> yes, weird ones like e.g. a node reboots & changes system ID and all of a
> sudden it looks like 3 nodes on a link (which is hinted). This is all too
> far for a protocol spec, those are day one/applicability things if one wants
> to go into detailed details of all the sceanrios.

I don't think we need to go into details -- the wording caught my attention.

...
> > [major] "SHOULD generate a clear, according notification to help
> > operational deployments." Normatively, what is "clear"? Is there
> > information that you have in mind that should be included to make it
> > "clear"? I assume that a notification is a message logged on the node, is
> > that true?
> >
> > Suggestion>
> > ...SHOULD log a message including the System IDs of each node.
> >
> > (Anything else?)
>
> logging is the old days. today people want telemetry, yang, open config,
> splunk channel, dancing bears, hence the "notification"

Sure -- the main point is really that the characteristics of a "clear,
according notification" are not, well, clear.  What should be included
in the notification?

...
> > 2069 4.2.3.3. Flooding
> > ...
> > 2082 As described before, TIEs themselves are transported over UDP with
> > 2083 the ports indicated in the LIE exchanges and using the destination
> > 2084 address on which the LIE adjacency has been formed. For unnumbered
> > 2085 IPv4 interfaces same considerations apply as in equivalent OSPF case.
> >
> > [major] (Related to a comment in 4.2.2.) "using the destination address on
> > which the LIE adjacency has been formed" If multiple addresses are used
> > (from different AFs), which one is selected? Is there a "primary" address
> > that is considered when the adjacency is formed?
>
> very good catch.
>
> No, the spec does not give the rules. An implementation will pick what it
> likes and the other side has to follow because it sees it as LIE source
> address.
>
> Trying to start to build "prefrences" is a futile exercise. everybody has a
> different opinion what is "best" address on an interface and then the
> "unnumbered" wars start ;-)

Ok.

Having not fully read this section yet...  Can there be "race
conditions" where one side picks the IPv6 address and the other picks
IPv4 (at the same time) -- they both send, the other side receives --
is the operation like that ok?   I ask because you mention that "the
other side has to follow".

...
> > 6362 B.1. common.thrift
> > ...
> > 6730 /** Link capabilities. */
> > 6731 struct LinkCapabilities {
> > 6732 /** Indicates that the link is supporting BFD. */
> > 6733 1: optional bool bfd =
> > 6734 common.bfd_default;
> > 6735 /** Indicates whether the interface will support v4 forwarding.
> >
> > 6737 @note: This MUST be set to true when LIEs from a v4 address are
> > 6738 sent and MAY be set to true in LIEs on v6 address. If v4
> > 6739 and v6 LIEs indicate contradicting information the
> > 6740 behavior is unspecified. */
> >
> > 6742 2: optional bool v4_forwarding_capable =
> > 6743 true;
> > 6744 }
> >
> > [major] Not knowing thrift, and assuming that this structure is some sort
> > of enum/list, what should a receiver do if the undefined/unknown value 3
> > is received?
>
> the packet won't even be decoded. you'll get an encoder/decoder error, it's
> all handled internally by thrift.

I guess that in general this means that the packet is ignored.

> That's the beauty of modelling. 80%+ of problems/worries we bean count
> manually in protocols just vanish magically.

Yes, but...what are we standardizing?  This is one of the reasons for
my note at the top.  Is the objective for the model to be the standard
(similar to what we do with YANG models), or to specify something that
allows other languages/models to be used?  I think it's the latter.

> Remember, we had _no_ bugs in first interop of LIEs with a blind 2nd
> implementation.

If I remember correctly, was that in Python?

No bugs doesn't necessarily mean that the edge cases were tested.

> > [major] "contradicting information the behavior is unspecified." How can
> > the behavior be unspecified if this *is* the specification? More to the
> > point: if the indication is required in IPv4 LIEs, but only optional in
> > IPv6 LIEs, why wouldn't the information from the IPv4 LIEs be considered
> > as the truth?
>
> because _sooo_ many things can go wrong. LIEs on different AFs can be
> buffered, arrive out of sequence and do a myriad crazy things. We were
> ruminating for a long time whether we should have one LIE on a single AF but
> then like I told you there is no "required AF" in RIFT based on customer
> input, e'one runs what they want on their fabric. And customers prefer AF
> fate that is not shared. So de facto v4 and v6 run as ships in the night
> (except the v4 capable v4ov6) and we end up advertising the bfd/v4 capable
> twice (because again, we don't know wehther we'll have either AF or both on
> an adjacency). Works fine in practice, an implementation seeing persistent
> mismatch should probably just declare link mismatch and shut down adjacency
> but that's really implementation specific.

As specified ("MUST be set to true when LIEs from a v4 address are
sent and MAY be set to true in LIEs on v6 address"), there is already
significant room for issues because, regardless of the actual ability
to support IPv4, the setting on an IPv6 LIE is optional.  IOW, the
v4_forwarding_capable flag may never be set on an IPv6 LIE because it
is optional -- the the implementation would comply with this
specification: it is not explicitly mentioned that it is required for
the flag to be set, and it is assumed (correctly, I think) that an
IPv4 LIE means that IPv4 forwarding is supported.

I really don't like the last part about letting the implementation
decide what to do: some may decide to shut down the adjacency while
others may keep trying -- neither will result in an established
adjacency (not sure if that's the correct name), but the external
behavior is inconsistent.  I would prefer it if something was
recommended.

Suggestion:

OLD>
6735	     /** Indicates whether the interface will support v4 forwarding.

6737	         @note: This MUST be set to true when LIEs from a v4 address are
6738	                sent and MAY be set to true in LIEs on v6 address. If v4
6739	                and v6 LIEs indicate contradicting information the
6740	                behavior is unspecified. */

NEW>

   /** Indicates whether the interface supports IPv4 forwarding.

      @note: v4_forwarding_capable MUST be set to TRUE if IPv4 forwarding is
      supported on the interface, regardless of the source IP address of the
      LIEs.  If the IPv4 and IPv6 LIEs indicate contradicting information then
      the adjacency SHOULD be shut down.

[End]

---

From: Antoni Przygienda <[email-masked]>
Date: Thu, 1 Apr 2021 20:26:23 +0000

Ok, on 2nd thought I concur

-- tony

On 01/04/2021, 22:08, "Alvaro Retana" <[email-masked]> wrote:

    [External Email. Be cautious of content]

    Tony:

    Hi!

    Thanks!

    Alvaro.

    ...
    ...
    ...

    ...
    > yes, as we said there will be a more extensive intro and a reader's guide

Juniper Business Use Only

---

From: Tony Przygienda <[email-masked]>
Date: Fri, 2 Apr 2021 14:28:43 +0200

Alvaro, thanks for detailed input. it will be a good topic for today's
weekly rift review hour. inline.

> game: "all adjacencies MUST be reset and not be restarted" (or

as I said, you're right, we MUST it roughly along the lines you say as
to the protocol basically having to initialize from start on.

> showing the full schema upfront.

Alvaro, it would be a good idea in early rift work if IETF had anything
along
"yang for packets" working, alas, it didn't and it does not.

If a "yang for packet" gets developed we surely can start new version of
rift exchanging all information via such yang schemas. Idea best kept
for WG chairs meet and I'm surely interested in talking about any such
future effort.

However, rift currently is modeled using thrift which has been around for a
very long time and e.g. runs
lots of critical hyperscaler infrastructure and tons of apache related
stuff.
the format is well understood by very large open source community.

So, coming up with some made-up pseudo-notation for some kind of "summary"
will just  lead to manual repetition of things and bloat the spec.

Your comment about referencing forward to the schema structs/fields stands
of
course and needs be added in the spec where the mechanisms are explained.
That may be the kind of "summary for the reader" you look for.

> found the Security Envelope, but no details on what is MTI in terms of

OK, I will cut out any reference to any specific algorithm. I just mention
that
the key id is basically also indicating the authentication schema used.

Observe that the envelope is agnostic, it just provides offset and anything
can go inside. RIFT avoids the really pesky problem of protecting lifetimes
BTW so in a sense (modulo the newest ISIS rfc we put out which is hacky
but pretty good hacky) better than anything we have today already.

> configure things right -- and declare that outside the scope of this

further down the line

> I'm sure it's obvious to you! ;-)

further down the line

> You are not the audience.

further down the line

> This is exactly the same point as referencing OSPF/ISIS...

point taken. I thought that RFC5549 reference is very helpful since
peple fight the same kind of problems but sure, no prob, it will go out.

> of scope than trying to address every corner case.

okey, agreed. I try to describe what happens on LIEs when you have v4/v6
mixes and stop there.

v4 on both sides:
  LIEs exchanged over v4
v6 on both sides:
  LIEs exchanged over v6
v4 and v6 on both sides.
  LIEs exchanged over both
v6 only on both sides but you want v4 forwarded if possible
  LIEs exchanged over both, the capability field is set.

> Given that, what does it mean to to support "[RFC5549] type of forwarding"?

okey, as I said, we cut out 5549 completely

> that you don't need to go into details of how it could be done, just

This is forwarding path, it is not up to RIFT to describe details
which may (in fact are) very ASIC specific often. We can of course say

"MAY be used to forward v4 using v6 nexthops"

but anything beyond that would be overspecification.

> nothing about IPv6 -- that is the reason for the rest of the questions

there is no such thing as v6 over v4 next hops as far as I know practically
speaking.
Unless you mean some kind of tunneling of which RIFT does not talk @ all so
this is
really outside the scope.

I will add that v6 over v4 is outside the scope of this spec.

> may be getting ahead of ourselves with all this talk about forwarding.

yes, WHY are we talking about forwarding?

if something exchanges over v6 it probably forwards v6, otherwise we
are breaking somewhat silly ground, don't we?  Same for v4, right? Or do you
think THIS also need explanation in an IGP spec?

if something sends v6 it MAY forward over v4 and that needs signalling
because it's undetectable and that's the field and as I wrote above, we
will add the text what LIEs are exchanged on presense of which AF (but
we can't mandate it, we cannot mandate a RIFT implementaiton MUST
send v6 LIEs if it has a v6 interface [whatever THAT means])

so we add the LIE scenario text and reference forward to model and abstain
from standardizing fast path behavior in this spec.

> will progress them together.

Is there some kind of precedence case where a base protocol spec
has been held by its applicability draft? Sounds extremely new to me.
Even more so given there is an open source implementation to refer
to, a day-one
book explaining in minute details deployment and tested behavior of two
implementations.

> referring to the node (maybe that's it)?

I suggest to authors to  completely remove PoD considerations from the spec
except carrying a PoD number in the packets for information purposes since
you find it so confusing.

> > source that inter'oped day one.

that's an informational RFC (valuable) that does neither have the power or
set expecationas to mandate a base protocol spec to start write operational
or manageability  guidelines, considerations or tutorials to the protocol.

I look @ things like RFC5036 and I see no manageability considerations
anywhere in the spec. Security sections are clearly mandated but I wonder
where all your requirements to operational sections, manageability sections
are being pulled from.

Yes, we have an applicability draft and that should run its own pace and so
we should have a draft on manageability which is in fact probably already
the
yang draft and more manageability considerations should go in there.
You intend to gate the twice implemented (and more in flight),
deployed & interop'ed protocol gated on RFC'ing those?

> doesn't make it clear to others.  The specification is not for your

I will carefully try to reword some of that stuff into MUST as far I see it
not losing clarity.

> it ok if just the major version is the same?

I quote the spec:

"

2.  the neighboring node is running the same MAJOR schema version AND

"

The spec does explicitly NOT say anything about minor and this is intended
exactly like this.
Omitting stuff that does not need be written is better than starting to add
lots of language stating what must not be done.

the rules on increasing minor and major are very specific on the schema

"

   This section introduces the schema for information elements.

   On schema changes that

   1.  change field numbers or

   2.  add new required fields or

   3.  remove fields or

   4.  change lists into sets, unions into structures or

   5.  change multiplicity of fields or

   6.  changes name of any field

   7.  change datatypes of any field or

   8.  adds or removes a default value of any field or

   9.  changes default value of any field

   major version of the schema MUST increase.  All other changes MUST
   increase minor version within the same major.

"
so minor is used in the precise sense of https://semver.org/ which is a
widely established standard (whatever IETF may think of its status).
And no, reference to it AFAIS does not need to be added, the rules above
govern precisely this behavior and it would be a tautology.

> condition should be clarified in #3.

all those clauses are very explicitly joined with a capital AND while
the last is OR'ed (8) and als explicitly joined with an AND. That's about
as close a language to first order logic in math as I can get written but
I will explain it @ the beginning of the paragraph.

As I said, I will add some kind of MUST language to describe the IIF which
is used
in math, hopefully not to the detriment of clarity of the spec.

nit "I don't see the AND on 5)", need be added

As I said, I suggest to cut out PoD completely since it seems contentious.

> language used in #6 refers to the "advertised MTU"...

thrift spec is very clear that if the field is omitted the default is to be
assumed

"

"

> be true even before the node tries to do anything.

nowhere in the ZTP section is it specified that a node must have a 3-way
adjacency formed.
Maybe if you explain how you ended up with that assumption I can add some
clarification.

> with optional fields and defaults.

the spec says specifically

"

Configured nodes and nodes operating in ZTP can be
   mixed and will form a valid topology if achievable.

"

We can add to the spec

"
And node with a configured level MUST always use that value to advertise
its level.
"

No other reader before seemed to ask for that.

thrift mandates the behavior of optional fields precisely if they have
default and we don't need to say anything more.

> with the two real flags is not explained anywhere.

I will forward reference the schema.

yes, we will rename LEAF_2_LEAF to LEAF_2_LEAF_PROCEDURES (in thrift for
consistency purposes e'thing is kept small, we can make it small here as
well)

_and_  seems to indicate pretty clearly that both flags are present.

LEAF_2_LEAF_PROCEDURES cannot be present without LEAF_ONLY and that's why
it's modelled that way.

> > leaf-2-leaf in the document so far.

tell that to the plethora of reviewers, each with his little pony ;-)

seriously, e'thing will be renamed leaf_2_leaf_procedures and leaf_only

> optional is a contradiction.  Maybe the list is not required, just

PoD will be removed is my suggestion

> miscabling is not part of the spec, then it should not be part of the

okey, we make the MAY small and with that it's not normative. we just leave
element in the schema and say, it's for informational purposes.

> exchange...discovers miscablings", but in reality it doesn't.

I will remove any suggestion as to computaton/use and basically say

"an implementation may advertise problems with interfaces such as
miscabling in the schema field X"

> I don't think we need to go into details -- the wording caught my

aha, ok.

> according notification" are not, well, clear.  What should be included

I remove the notification requirement.

> other side has to follow".

no, there is no race condition. And there is no need to add things.
To support an AF an implementation will be forced to send a packet with a
source address on the AF. And since it advertises port it will listen for
TIEs
on this address on this port. I think spec says as much but nothing else
makes any sense frankly.

TIEs may be coming on either or both and they are all considered the same.
Think aobut BGP with 2 sessions to same neighbor, one over v6, one over v4.

v4ov6 is a special case but it really has little to do with LIEs except
seeing
the source address.

> I guess that in general this means that the packet is ignored.

this would be mandating implementation. If an implementation chooses to
refuse to talk to peer there is nothing we can say. If it chooses to fix
the 3 to
default there is nothing we can do. This is akin to receiving
corrupt IGP packets everywhere else. What does one do?

Question is also largely academic since no'one write ser/deser by hand for
thrift, it's all compiled. Well, if that code is buggy most likely nothng
works anyway but let's say there is a corner case of ser/deser library
doing something like you suggest. Deser will fail and it's like a CRC
failure, there is really nothing smart one can do except dropping the
packet.

As you know the ambitious

https://datatracker.ietf.org/doc/draft-ietf-idr-error-handling/19/

took 19 revisions and 6 years to publish an RFC where lots of people still
vehemently agree
deteriorated the protocol behavior.

> allows other languages/models to be used?  I think it's the latter.

we standardize the model, I think you heard from Bruno 100% same
experience/opinion.

_IF_ we ever get to yang-encoding-packets (and not only some paper tiger
but real useful compilers) we should have a RIFT-YANG which I assume will
largely reference current rift RFC and possibly consists of only a small
snippet of python to do model-2-model translation of the thrift model (I do
daily oodles of xml-2-thrift and vice versa based on automatically
translated models, a fairly trivial exercise).

> If I remember correctly, was that in Python?

open source is python, JNPR is something else, I had some private runs
against a Go implementation (which seems dormant since then)

> No bugs doesn't necessarily mean that the edge cases were tested.

it never does. Please refer to extensive automated test suite in open
source python that Bruno built and ask to add more testcases you can think
of. I will be interested in that since I have extensive testing suite
myself for all kind of weirdo stuff.

_Before_ the interop Bruno however found a nice problem (multiple neighbors
causing amplification) based on graphs he was generating and that led to
extension of the spec. So there are problems but not in the thrift
ser/deser which is based on libraries tested over very large deployment
bases for a very long time.

Can those libraries break in the future? The ":technology", especially
software we build and deliver those days everywhere is akin to
leaking/non-leaking roofs, it's either broken or not broken yet.

> behavior is inconsistent.  I would prefer it if something was

again, thrift clearly mandates behavior and semantics of an optional field
with a default, Alvaro.
So if the flag is NOT present it's false.

I think your gripe is partially real (the flag really means "v4 forwarding
over v6 will work if you don't see v4" so showing it false on a v4 packet
is really a shutdown reason) and I'm ok with the change.

>       the adjacency SHOULD be shut down.

uff ...

-- tony

---

From: Alvaro Retana <[email-masked]>
Date: Fri, 2 Apr 2021 07:08:48 -0700

On April 2, 2021 at 8:29:21 AM, Tony Przygienda wrote:

Tony:

> Alvaro, thanks for detailed input. it will be a good topic for today's weekly
> rift review hour. inline.

Please keep in mind that most of my comments are made thinking about
option (b) [*] -- but, so far, yours and Bruno's expectations are
different.  IOW, if I need to get in an (a) mindset then the comments
may be different.  I would really like to hear from others in the WG.

See a couple of things inline.

Thanks!

Alvaro.

[*] From the "What is being standardized in the base spec?" thread.

...
> that the key id is basically also indicating the authentication schema used.

I actually meant it the other way around: there should be
mandatory-to-implement security.  The "limited domain" arguments don't
always work.  We can of course try...

...
> has been held by its applicability draft? Sounds extremely new to me.

All kinds!  If it is not clear what the tradeoffs of the options are
-- that is the reason I keep pointing at rfc5706.

Again, let's settle the "What is being standardized in the base spec?"
discussion first.  I want to make sure that we're all on the same page
and that we agree on exactly what is needed.   This includes making
sure the rest of the IESG are in the loop and don't ask for things
later.

As I said at the top, my comments are made with one set of
expectations -- you're answering with a different set.  I know we had
talked (including the Chairs): we all heard the same words, but we
interpreted them differently.

...
> > > the schema versioning does describe that in excruciating details @
> > > beginning of appendix B. I'll fwd reference
> >

Great!  I wouldn't be doing my job if I didn't ask. ;-)

---

From: Tony Przygienda <[email-masked]>
Date: Fri, 2 Apr 2021 16:19:02 +0200

Alvaro, inline

> always work.  We can of course try...

well, two observations why I think this is futile

a) RIFT's big appeal is ZTP. ZTP (full, true ZTP) does not work with
security (unless you pre-configure a key e'where or something but that's
already not ZTP). We have a _very_ extensive section explaining the
security models and implications and trade-offs.

b) whatever we standardize as "must have" algorithm will be laughably weak
in 20 years.

No limited domain argument here, argument is rather "By default, it's ZTP
and that means no security, the more security you want the more you
configure, RIFT supports e'thing up to port security model and the key ID
will give you newest flavor of algorithm"

otherwise pls suggest the "specific" algorithm we have to refer to and
standardize you think makes sense.

> sure the rest of the IESG are in the loop and don't ask for things

ack, agreed.

> interpreted them differently.

okey, point taken.

I am all for operational & management considerations/documewntaitons of a
protocol but for me, that has nothing to do with base spec.

And as I said, full spec open source implementatyion is there, interop
done, day one book, yang model in flight, applicability in flight. That
seems to me a pretty good plate for IETF standards as to protocol work.
Each of those things have to progress but why any of them should gate the
other (well, man and app is probably waiting for rift rfc to quote) is not
easily comprehensible to me (unless I misunderstood again).

>
>
>
> ...
> > > > > 1454 2. the neighboring node is running the same MAJOR schema
> version
> > > > > AND
> > > ...

> Great!  I wouldn't be doing my job if I didn't ask. ;-)

understood. yes, the Art of Carving Away. I know that sometimes it leaves
the reader taking assumptions they should not and leaves questions open. I
still prefer it to overspecify or repeat excessively. Mostly because
repetitions are hard to maintain and start to contradict each other
quickly.

-- tony

---

From: "Pascal Thubert (pthubert)" <[email-masked]>
Date: Fri, 2 Apr 2021 16:52:10 +0000

I have a problem, maybe outlook. But the mail with the part 1 review is interrupted abriupty here

1014    allow all the ToF nodes to see each other via south reflection

Does anyone have the rest of Alvaro’s review?

Pascal

From: RIFT <[email-masked]> On Behalf Of Alvaro Retana
Sent: vendredi 5 mars 2021 16:36
To: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: [Rift] AD Review of draft-ietf-rift-rift-12 (Part 2a)

Dear authors:

This is Part 2a of my review of this document.  I wanted this second installment to be longer, but given the IETF meeting next week, I am sending this fragment out now.

Part 2a starts with §4.2 (Specification) and goes just through §4.2.2 -- I just started reading the LIE FSM/§4.2.2.1.  I included a couple of comments on the schema itself.

While reading this part of the document I've been learning thrift and applying my programming and modeling experience (*).  In general, the model seems straight forward (so far).  It is important to highlight somewhere at the start of §4.2 that Appendix B is Normative.  The existing references don't explicitly say so.

In this first part of the specification there are a number of assumptions made that should be explained explicitly and not by reference.  For example, it seems to be assumed that the reader knows what a three-way handshake is -- while most readers of this specification will be familiar with other routing protocols, this document is *the* RIFT spec and should not rely on other protocol specifications, much less when the specification is not (can't!) be the same -- this is what §4.2.2 says:

   It uses a three-way handshake mechanism which is a cleaned up version of
   [RFC5303].  Observe that for easier comprehension the terminology of
   one/two and three-way states does NOT align with OSPF or ISIS FSMs albeit
   they use roughly same mechanisms.

IOW, "similar to IS-IS, but not the same."   There are more specific comments inline.

"Traditional" specifications usually present packet formats and describe the fields before explaining their use.  §4.2.2 jumped right into talking about a specific flag (v4_forwarding_capable) without introducing it or the place where it is carried (LIEPacket).  It is important to describe the different structures, and provide a way to make them simpler to find in Appendix B.

In my comments I'm asking for a lot of pointers to things that have not been discussed.  I don't know (at this point) whether the overall organization of the document makes sense as is or if it might be worth rethinking (i.e. shuffle things around).  Food for thought.

These comments/questions are for the Chairs/Shepherd:

- It looks like an early TSV review was requested, but I didn't see the review come in.  Did I miss it?

- §8.1 includes a set of suggested UDP ports, but they are not reflected in the registry.  Early allocation has not been requested -- you should consider doing so.  See rfc7120.

Thanks!

Alvaro.

(*) I don't have any. ;-)

[Line numbers from idnits.]

...
1341   4.2.  Specification
...
1350     "On Entry" actions on FSM state are performed every time and right
1351     before the according state is entered, i.e. after any transitions
1352     from previous state.

[nit] Suggestion>
   "On Entry" actions at an FSM state are performed right before the
   corresponding state is entered, i.e. after any transitions from a
   previous state.

1354     "On Exit" actions are performed every time and immediately when a
1355     state is exited, i.e. before any transitions towards target state are
1356     performed.

[nit] Suggestion>
   "On Exit" actions are performed immediately when a state is exited,
   i.e. before a transition towards a target state is performed.

[major] Any type of action, or are you referring to an "on exit" one?

[major] "MUST be considered an unrecoverable error"  What is the result of that?  Should the adjacencies be reset, the rift process restarted, the interfaces shut down, etc.??   There's no other place in the document that mentions "unrecoverable".

1362     The FSMs and procedures are normative in the sense that an
1363     implementation MUST implement them either literally or an
1364     implementation MUST exhibit externally observable behavior that is
1365     identical to the execution of the specified FSMs.

[major] How can you tell the difference?  rfc2119 keywords "MUST only be used where it is actually required for interoperation".  In this case there are options, and there is no way to verify one or the other as long as the implementations "exhibit externally observable behavior that is identical".  IOW, I am not comfortable with using MUST if it is not required.  Also, I believe the statement to generally be true for any specification and not needed.

Suggestion (borrowing from rfc4271):
   The data structures and FSMs described in this document are conceptual
   and do not have to be implemented precisely as described here, as long
   as the implementations support the described functionality and exhibit
   the same externally visible behavior.

1367     Where a FSM representation is inconvenient, i.e. the amount of
1368     procedures and kept state exceeds the amount of transitions, we defer
1369     to a more procedural description on data structures.

[] This paragraph can also be eliminated.

1371   4.2.1.  Transport

[] Can we get an index/TOC or some type of guide in the appendix to make it easier to find specific parts?  For example, finding where a LIE is defined is not straight forward -- I happened to stumble on it in B.2.  Another option would be to point in §4.2.2 to look for LIEPacket in B.2.

[major] It must be stated somewhere that the contents of Appendix B are Normative.

[minor] This is a very short section -- please put references to other places where these topics are explained more, if any.

1383     RIFT LIE exchange auto-discovers neighbors, negotiates ZTP parameters
1384     and discovers miscablings.  It uses a three-way handshake mechanism
1385     which is a cleaned up version of [RFC5303].  Observe that for easier
1386     comprehension the terminology of one/two and three-way states does
1387     NOT align with OSPF or ISIS FSMs albeit they use roughly same
1388     mechanisms.  The formation progresses under normal conditions from
1389     one-way to two-way and then three-way state at which point it is
1390     ready to exchange TIEs per Section 4.2.3.

[minor] "...which is a cleaned up version of [RFC5303]."  There's no need to hint at the fact that other implementations/protocols may not be as good/clean as rift.  Please take this part of the sentence out.

[minor] "Observe that for easier comprehension the terminology of one/two and three-way states does NOT align with OSPF or ISIS FSMs albeit they use roughly same mechanisms."   I don't know how changing everything I know makes comprehension easier. ;-)   Seriously: we don't need to tell the reader what is not used.

1392     LIE exchange happens over well-known administratively locally scoped
1393     and configured or otherwise well-known IPv4 multicast address
1394     [RFC2365] and/or link-local multicast scope [RFC4291] for IPv6
1395     [RFC8200] using a configured or otherwise a well-known destination
1396     UDP port defined in Appendix C.1.  LIEs SHOULD be sent with an IPv4
1397     Time to Live (TTL) / IPv6 Hop Limit (HL) of 1 to prevent RIFT
1398     information reaching beyond a single L3 next-hop in the topology.
1399     LIEs SHOULD be sent with network control precedence.

[] The first sentence is long and convoluted.  I think I understood that the assigned destination IP address and UDP port are used, but that they also can be configured.  Is that right?

Suggestion>

   LIEs are exchanged over the well-known multicast addresses and UDP
   port, as summarized in Appendix C.1.  An implementation MAY allow
   for the local configuration of these parameters.

[] If configured, do both sides need to be configured beforehand or is there a discovery mechanism?

[major] "LIEs SHOULD be sent with..."  When is it ok to not use these values?  IOW, why is this action recommended and not required?  (The next comment is related.)

[minor] "IPv4 Time to Live (TTL) / IPv6 Hop Limit (HL) of 1"  Was GTSM (rfc5082) considered?  Several documents (for example, rfc8085 and draft-ietf-opsec-v6) suggest its use.  You may be asked about it later.  It would be good to preempt those questions by adding some text in the Security Considerations about any risks...or using it.

[major] "LIEs SHOULD be sent with network control precedence."  When is it ok to not do so?  IOW, when is this behavior recommended and not required.  BTW, please add a reference.

1401     Originating port of the LIE has no further significance other than
1402     identifying the origination point.  LIEs are exchanged over all links
1403     running RIFT.

[nit] s/Originating port/The originating port

1405     An implementation MAY listen and send LIEs on IPv4 and/or IPv6
1406     multicast addresses.  A node MUST NOT originate LIEs on an address
1407     family if it does not process received LIEs on that family.  LIEs on
1408     same link are considered part of the same negotiation independent of
1409     the address family they arrive on.  Observe further that the LIE
1410     source address may not identify the peer uniquely in unnumbered or
1411     link-local address cases so the response transmission MUST occur over
1412     the same interface the LIEs have been received on.  A node MAY use
1413     any of the adjacency's source addresses it saw in LIEs on the
1414     specific interface during adjacency formation to send TIEs.  That
1415     implies that an implementation MUST be ready to accept TIEs on all
1416     addresses it used as source of LIE frames.

[major] "An implementation MAY listen and send LIEs on IPv4 and/or IPv6 multicast addresses.  A node MUST NOT originate LIEs on an address family if it does not process received LIEs on that family."

The first sentence says that it is optional to listen and send LIEs over either IPv4/IPv6 (IOW, a router doesn't have to do either).  The second one gives the impression that originating a LIE on a specific AF means that the router is willing to only receive/process LIEs on it (e.g. if an IPv6 LIE is not generated then there's no positive indication that the router can receive/process IPv6 LIEs).  The combination results in a potential lock at startup:

For example, if a router chooses to only listen on IPv4 (MAY = optional) and it's neighbor on IPv6, then they will be sending LIEs and never talk to each other.

I'm assuming that the intent is to allow for maximum flexibility: allow to send/receive on either (or both).  But, what if only one AF is supported?  Should an implementation be capable to control which AF is used?  If I have an IPv6-only deployment, should the router be expected to receive LIEs over IPv4?

[nit] s/LIEs on an address family/LIEs using an address family

[nit] s/LIEs on same link/All LIEs on a link

[minor] "part of the same negotiation"  Negotiation of what?  The text at the start of this section mentions that LIEs negotiate ZTP parameters -- is that it?  Other actions don't seem to be negotiations: discovery, for example.   I'm not sure if something like "session" would capture the intended meaning.

[major] "MAY use any of the adjacency's source addresses it saw in LIEs...to send TIEs."  This use of "MAY" makes using the source address from a LIE optional.  §4.2.3.3 says that "TIEs...using the destination address on which the LIE adjacency has been formed", which doesn't make the use optional.

OLD>
   A node MAY use any of the adjacency's source addresses it saw in
   LIEs on the specific interface during adjacency formation to send
   TIEs.

NEW>
   A node may use any of the adjacency's source addresses from the
   LIEs received on the specific interface during adjacency formation
   to send TIEs (Section 4.2.3.3).

[See related question in 4.2.3.3.]

[minor] "three-way adjacency"   I know there's a definition in the terminology section, but it is not a specification of what a three-way adjacency is.  Please add a forward reference to where it is specified.

[major] "`v4_forwarding_capable` flag"   (This comment is not specific to this flag.)

The specific LIE packet format has not been presented, introduced or even referenced up to now.  Pointing to a flag, which happens to be optional in an optional part of the LIEPacket, comes out of the blue and feels completely out of place without prior explanation.

I can see that some of the fields are described in Appendix B, but there should at least be a pointer to that.

I also expect a description somewhere of expectations and error conditions. For example, see my comments in the appendix related to LinkCapabilities.

First of all, rfc5549 is a BGP-specific RFC.  How do the BGP mechanisms defined there (and in rfc8950) map to RIFT?  IOW, please specify how RIFT works.

Because you listed rfc5549 as a Normative reference, I assume you want this behavior to be normative.  s/can use/MUST/SHOULD/MAY ??

[major] "It is expected that the whole fabric supports the same type of forwarding of address families on all the links.  Operation of a fabric where only some of the links are supporting forwarding on an address family and others do not is outside the scope of this specification."

Please remind me, is IPv4 optional or required?  I'm assuming that IPv6 is required and IPv4 optional, but I can't find anything definitive in the document.

Even if IPv4 is optional, I'm assuming that most deployments will support both.  But as time goes on that may (slowly) change to IPv6-only.  Is that a fair assumption?

Because the ability to support IPv4 is signaled per link (v4_forwarding_capable), then the expectation that "the whole fabric supports the same type of forwarding of address families" may be "easily" broken if one link doesn't support IPv4 (including the case where a rogue node may not advertise v4_forwarding_capable to mess things up).  What should a node that detects an inconsistency (e.g. some links use dual-stack, but others only one AF) do?  I'm assuming that it SHOULD/MUST (?) go as far as disabling the adjacencies as described below, right?   Please be explicit and specific on the actions to be taken.

1427     The protocol does NOT support selective disabling of address
1428     families, disabling v4 forwarding capability or any local address
1429     changes in three-way state, i.e. if a link has entered three-way IPv4
1430     and/or IPv6 with a neighbor on an adjacency and it wants to stop
1431     supporting one of the families or change any of its local addresses
1432     or stop v4 forwarding, it has to tear down and rebuild the adjacency.
1433     It also has to remove any information it stored about the adjacency
1434     such as LIE source addresses seen.

[major] There are references made in this paragraph to things that are defined elsewhere (""any" PoD"..."propagated in the LIEs").  Please put pointers to where these things are defined.  As mentioned above, the LIE format hasn't been presented yet.

The text above gives the impression that the type of the PoD is somehow related to the level, so I went in search of that and found this in the LIEPacket:

   struct LIEPacket {
  ...
       /** Node's PoD. */
     7: optional common.PodType            pod =
             common.default_pod;

...but then I can't find anything that corresponds to PodType matching the values above: undefined, any...

...but I did find this:

   /** Common RIFT packet header. */
   struct PacketHeader {
...
     /** Level of the node sending the packet, required on everything
         except LIEs. Lack of presence on LIEs indicates UNDEFINED_LEVEL
         and is used in ZTP procedures.
      */
     4: optional common.LevelType            level;
   }

...and a couple of constants:

  const LevelType   top_of_fabric_level         = 24
  const LevelType   leaf_level                  = 0
  const LevelType   default_level               = leaf_level

If ZTP is not used, are there operational/deployment considerations for an operator to provision the levels?  Should they simply start counting from 0, is incrementing by one good enough or should it be by 2?  Or maybe there are considerations about the use of ZTP?  Please take a look at rfc5706.

[minor] "It MAY be also provisioned with its PoD."  What does this mean?  Does it mean that all the nodes in the PoD have the same level?  ...

[major] BTW, this reminds me, what are the manageability considerations related to RIFT?  Among other things, it would be nice to have a summary of parameters that are expected to be configured and their defaults.  Please take a look at rfc5706.

1446     Further definitions of leaf flags are found in Section 4.2.7 given
1447     they have implications in terms of level and adjacency forming here.

[] "leaf flags"   What are those?  I think this is the first mention.

[] Suggestion>
   Further definitions of leaf flags are found in Section 4.2.7.

[nit] s/tries to form/starts to form

[major] "if and only if"   Can this be translated into a Normative statement?  Are all these required (MUST) before the formation of a 3-way adjacency started?

[minor] "the node is in the same PoD"  As what?   I guess you mean in the same PoD as what a neighbor indicated in its LIE, right?

[] The formats have not been introduced yet.

I peeked into Appendix B and found the PacketHeader.  As with #1, you're assuming a LIE packet has been received from the neighbor, but that is not mentioned.

[] The versioning system has not been introduced.  Please put a reference to it.

[nit] This is the only place where "MAJOR" (all caps) is used.  Please be consistent.

[nit] s/not member of some PoD/not a member of the same PoD

[major] "not [a] member of [the] some PoD"  #1 says that the "node is in the same PoD" -- it looks like there's some context missing -- I guess about the ability to join other PoDs.  Is this only a northbound characteristic?

1459     4.  the neighboring node uses a valid System ID AND

[minor] "System ID" and "SystemID" are both used.  Please pick one.

[major] What is a "valid System ID"?  The only clue that I found is in §4.2.7.2, which makes we wonder whether it is specific to ZTP or not (??):

   RIFT nodes require a 64 bit SystemID which SHOULD be derived as
   EUI-64 MA-L derive according to [EUI64].  The organizationally
   governed portion of this ID (24 bits) can be used to generate
   multiple IDs if required to indicate more than one RIFT instance."

If this is a recommendation (SHOULD), and assuming that it applies beyond ZTP, what is a "valid System ID"?

I also only found one place that talks about an IllegalSystemID, but that is probably not the same as an invalid one:

   /** 0 is illegal for SystemID */
   const SystemIDType IllegalSystemID        = 0

1461     5.  the neighboring node uses a different System ID than the node
1462         itself

[minor] Is there a way to resolve/address duplicate System IDs?  It seems to me that a rogue node may reflect the sender's System ID and prevent an adjacency from forming (along with a number of other values).

[minor] Also, I hope that the selection/assignment of the System ID is discussed elsewhere.  A pointer might be nice.

[] Ahhh...this in in the LIEPacket too.

[major]
   struct LIEPacket {
...

If MTUSizeType is not included, where is it specified that the default_mtu_size must be considered?  Without that specification it is not possible to satisfy this condition.

[major] The advertisement of LevelType in PacketHeader is optional.  That means that even if there is a default value a node may not "*advertise* defined level values".  Also, there are only 2 level values defined:

What am I missing?

1468     8.  [

1470            i) the node is at level 0 and has no three way adjacencies
1471            already to nodes at Highest Adjacency Three-Way level (HAT as
1472            defined later in Section 4.2.7.1) with level different than
1473            the adjacent node OR

[nit] s/three way/three-way/g

...

[minor] "support for Section 4.3.8"  Is there a name for that?  How is it indicated?  §4.3.8 says that a leaf can "advertise the LEAF_2_LEAF flag in its node capabilities"; there is nothing called "LEAF_2_LEAF" in the schema, but "leaf_only_and_leaf_2_leaf_procedures" does show up.  Please be consistent in the terminology.  [Also, there are places where "leaf-2-leaf" is used.]

...

[nit] s/rules checking/rules for checking

[nit] s/MAY be optionally/MAY be    (redundant)

[minor] What are that the "rules [for] checking PoD numbering"?  Where are they specified?

[major] "MAY be optionally disregarded by a node if PoD detection is undesirable or has to be ignored."

This is the only place where "PoD detection" is used.  What is it?  Where is it specified?  Why would be it undesirable?  When would it have to be ignored?  Are there operational considerations for making that decision?

[major] "...except preventing detection of certain miscabling cases."  Where is miscabling detection described?  Which cases may not be covered?  §4.2.7.6 says that "internal ruleset flags a possible miscabling", but that is the closest that I came to finding a description (and rulesets are not mentioned anywhere else).

1491     A node configured with "undefined" PoD membership MUST, after
1492     building first northbound three way adjacencies to a node being in a
1493     defined PoD, advertise that PoD as part of its LIEs.  In case that
1494     adjacency is lost, from all available northbound three way
1495     adjacencies the node with the highest System ID and defined PoD is
1496     chosen.  That way the northmost defined PoD value (normally the ToP
1497     nodes) can diffuse southbound towards the leaves "forcing" the PoD
1498     value on any node with "undefined" PoD.

[nit] s/building first...adjacencies/building its first...adjacency

[nit] s/to a node being in a defined PoD/to a node in a defined PoD

[minor] "...defined PoD is chosen."  ...chosen as the PoD value to advertise.  (or somehting along those lines)

[nit] s/northmost/northernmost

[] I'm missing how using the System ID to select will always result in the north direction...

[minor] "...diffuse southbound towards the leaves "forcing" the PoD value on any node with "undefined" PoD."  An example of this would be nice.

1500     LIEs arriving with IPv4 Time to Live (TTL) / IPv6 Hop Limit (HL)
1501     larger than 1 MUST be ignored.

[major] "MUST be ignored"  The specification of the sender (§4.2.2) says that "LIEs SHOULD be sent with an IPv4 Time to Live (TTL) / IPv6 Hop Limit (HL) of 1".  This is a Normative conflict because it is only recommended to the sender, but required by the receiver.

1503     A node SHOULD NOT send out LIEs without defined level in the header
1504     but in certain scenarios it may be beneficial for trouble-shooting
1505     purposes.

[] See the related comments in #7 above.

[nit] s/trouble-shooting/troubleshooting

[major] #7 above makes advertising defined values a requirement to forming a three-way adjacency, but it is only recommended here.  Unless there are specific troubleshooting cases described, we should retain the requirement.  I believe that a packet can be crafted in any way for troubleshooting/testing, but that doesn't have to be indicated in a specification which should deal with the correct operation of the protocol.  IOW, I think we can do without this paragraph.

1507   4.2.2.1.  LIE FSM

1509     This section specifies the precise, normative LIE FSM and can be
1510     omitted unless the reader is pursuing an implementation of the
1511     protocol.

[nit] s/and can be omitted..../and can be omitted.

[] It would be very nice if there was an upfront description (at least a list) of the different elements: states, events, etc.. to make reading the diagrams easier.  Also, the terminology (PUSH, etc.) should also be upfront.

1513     Initial state is `OneWay`.

[] This event is the only one described.  Is there anything special about it?  Is this the only one for which generating a notification is recommended?  Is that why it is described here?

[minor] "occurs normally when..."   Are there other conditions (which are not considered "normal")?

[major] "more than two nodes see each other on the same link"  The description below is different (and I think more accurate): "more than one neighbor seen on interface".  Also, it doesn't include the second part about a "reconfigured or rebooted" node.  What is the characteristic of that behavior?

[major] "SHOULD generate a clear, according notification to help operational deployments."   Normatively, what is "clear"?  Is there information that you have in mind that should be included to make it "clear"?  I assume that a notification is a message logged on the node, is that true?

Suggestion>

   (Anything else?)

[] This seems to be the only occurrence of a log resulting from a message or other action.  I think I'm missing the significance of this event.

[Ok.  This is where I am in my reading.  There are more comments below.]

...
2069   4.2.3.3.  Flooding
...

[major] (Related to a comment in 4.2.2.) "using the destination address on which the LIE adjacency has been formed"  If multiple addresses are used (from different AFs), which one is selected?  Is there a "primary" address that is considered when the adjacency is formed?

...
6165   10.2.  Informative References
...
6175     [DOT]      Ellson, J. and L. Koutsofios, "Graphviz: open source graph
6176                drawing tools", Springer-Verlag , 2001.

[major] Is there an URI that can be used as a stable pointer?

...
6362   B.1.  common.thrift
...

[major] Not knowing thrift, and assuming that this structure is some sort of enum/list, what should a receiver do if the undefined/unknown value 3 is received?

[major] "contradicting information the behavior is unspecified."  How can the behavior be unspecified if this *is* the specification?   More to the point: if the indication is required in IPv4 LIEs, but only optional in IPv6 LIEs, why wouldn't the information from the IPv4 LIEs be considered as the truth?

[nit] s/interface will support v4 forwarding/interface supports IPv4 forwarding/g

[nit] s/v4/IPv4/g  s/v6/IPv6/g

[major] "Indicates that the link is supporting BFD."  I was going to ask if setting bfd to TRUE means that the link can support BFD (is capable) or if it BFS is actively enabled (supporting), but I found this somewhere else:

   /** default link being BFD capable */
   const bool            bfd_default                = true

...so I'm assuming that it just means that the link is capable, which makes sense given that is it in LinkCapablities.

s/Indicates that the link is supporting BFD./Indicates that the link is BFD-capable./g

Also, note that §4.3.5 says that "RIFT MAY incorporate BFD", which is not completely aligned with the default indication of the link being BFD-capable.  Using BFD is different than being BFD-capable, so it is ok for its use to be optional (§4.3.5).  However, there is no stated requirement for a link to be BFD-capable, as indicated in the default setting.

[End of Review 2a]

---

From: "Pascal Thubert (pthubert)" <[email-masked]>
Date: Wed, 7 Apr 2021 13:20:51 +0000

Dear all;

Continuing changes starting at line 1014 of review part 1. I attached the result for Alvaro’s convenience.

The main commits are https://github.com/przygienda/rift-draft/commit/8d1d382c9ae2321fa7d488a4ba1881361c87e542
and https://github.com/przygienda/rift-draft/commit/237655f8d04cd33f1e5b151e3c5f9ae190b34594

please see below:

1013      with this, positive disaggregation can heal all failures and still
1014      allow all the ToF nodes to see each other via south reflection.
1015      Disaggregation will be explained in further detail in Section 4.2.5.

[nit] s/deployment is it introduces/deployment is, it introduces

Done

1017      In order to scale beyond the "single plane limit", the Top-of-Fabric
1018      can be partitioned by a N number of identically wired planes where N
1019      is an integer divider of K_LEAF.  The 1:1 ratio and the desired
1020      symmetry are still served, this time with (K_TOP * N) ToF nodes, each
1021      of (P * K_LEAF / N) ports.  N=1 represents a non-partitioned Spine
1022      and N=K_LEAF is a maximally partitioned Spine.  Further, if R is any
1023      integer divisor of K_LEAF, then N=K_LEAF/R is a feasible number of
1024      planes and R a redundancy factor.  If proves convenient for
1025      deployments to use a radix for the leaf nodes that is a power of 2 so
1026      they can pick a number of planes that is a lower power of 2.  The
1027      example in Figure 11 splits the Spine in 2 planes with a redundancy
1028      factor R=3, meaning that there are 3 non-intersecting paths between
1029      any leaf node and any ToF node.  A ToF node must have, in this case,
1030      at least 3*P ports, and be directly connected to 3 of the 6 PoD-ToP
1031      nodes (spines) in each PoD.

[nit] s/by a N number/by an N number

Done

[minor] "(K_TOP * N) ToF nodes, each of (P * K_LEAF / N) ports"
Again, the use of the terminology without a reference assumes a
specific interpretation by the reader.

Yes the definition was screwed somehow. The best I can do is probably to revise/fix the definition in the terminology
“

   K: Denotes half of the radix of a symmetrical switch, meaning that

      the switch has K ports pointing north and K ports pointing south.

      K_LEAF (K of a leaf) thus represents both the number of access

      ports in a leaf Node and the maximum number of planes in the

      fabric, whereas K_TOP (K of a ToP) represents the number of leaves

      in the PoD and the number of ports pointing north in a ToP Node

      towards a higher spine level, thus the number of ToF nodes in a

      plane.  To simplify the visual aids, notations and further

      considerations, we assume that the switches are symmetrical, so K

      is set to Radix/2.
“

[minor] "if R is any integer divisor of K_LEAF, then N=K_LEAF/R is a
feasible number of planes and R a redundancy factor."  Please expand
on the meaning of the redundancy factor.

Added:
“
                                       R a redundancy factor that denotes the
    number of independent paths between 2 leaves within a plane.
“

[minor] "6 PoD-ToP nodes"  I count 8.

The leaves are vertical, half of their port in each plane. The have 6 ports north so there are 6 ToP nodes.
The ToP nodes are horizontal. 6 of them. What you count to 8 is K-TOP, the number of ports in a ToP Node.
My biggest regret is that the actual drawing is not built into a real switch. I patented the general structure of that 3-D box: https://pdfpiw.uspto.gov/.piw?docid=10973148&SectionNum=1&IDKey=CAD3F825B319&HomeUrl=http://patft.uspto.gov/netacgi/nph-Parser?Sect1=PTO2%2526Sect2=HITOFF%2526p=1%2526u=%25252Fnetahtml%25252FPTO%25252Fsearch-bool.html%2526r=1%2526f=G%2526l=50%2526co1=AND%2526d=PTXT%2526s1=%252522thubert%252Bpascal%252522.INNM.%2526OS=IN/%252522thubert%252Bpascal%252522%2526RS=IN/%252522thubert%252Bpascal%252522

Back to the text, I added:
“
    The ToP nodes are represented horizontally with K_TOP=8 ports
    northwards each.
“

1033           +---+  +---+  +---+  +---+  +---+  +---+  +---+  +---+
1034         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1035         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1036         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1037         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1038         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1039         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1040         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1041         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1042         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1043           +---+  +---+  +---+  +---+  +---+  +---+  +---+  +---+

1045         Plane 1
1046        ----------- . ------------ . ------------ . ------------ . --------
1047         Plane 2

1049           +---+  +---+  +---+  +---+  +---+  +---+  +---+  +---+
1050         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1051         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1052         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1053         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1054         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1055         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1056         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1057         | | o |  | o |  | o |  | o |  | o |  | o |  | o |  | o | |
1058         +-|   |--|   |--|   |--|   |--|   |--|   |--|   |--|   |-+
1059           +---+  +---+  +---+  +---+  +---+  +---+  +---+  +---+
1060                    ^
1061                    |
1062                    |      ----------------
1063                    +----- Top-of-Fabric node
1064                          "across" depth
1065                           ----------------

1067       Figure 11: Northern View of a Multi-Plane ToF Level, K_LEAF=6, N=2

1069      At the extreme end of the spectrum it is even possible to fully
1070      partition the spine with N = K_LEAF and R=1, while maintaining
1071      connectivity between each leaf node and each Top-of-Fabric node.  In
1072      that case the ToF node connects to a single Port per PoD, so it
1073      appears as a single port in the projected view represented in
1074      Figure 12.  The number of ports required on the Spine Node is more or
1075      equal to P, the number of PoDs.

[minor] "more or equal to P"  ??

Changed to:
“
more than or equal to P
“

...
1121    4.1.3.  Fallen Leaf Problem
...
1140      In a maximally partitioned fabric, the redundancy factor is R= 1, so
1141      any breakage in the fabric may cause one or more fallen leaves.
1142      However, not all cases require disaggregation.  The following cases
1143      do not require particular action in such scenario:

[major] A quick look at §4.2.5.1 doesn't explicitly mention how a node
considers the redundancy factor...but that may be included in the "DAG
computation" mentioned in the first step.  I'm putting this comment
here so I don't forget later...

There is no operation in RIFT that is based on the value of R.
It’s just that after R breakages in a plane, it becomes possible that a leaf falls in that plane. If R=1, it’s guaranteed that a breakage will cause fallen leaves. R=2 guarantees that a single breakage will not cause fallen leaves.

Changed to:
“

   In a maximally partitioned fabric, the redundancy factor is R= 1, so
   any breakage in the fabric will cause one or more fallen leaves in
   the affected plane.  R=2 guarantees that a single breakage will not
   cause a fallen leaf.
“

1145         If a southern link on a node goes down, then connectivity through
1146         that node is lost for all nodes south of it.  There is no need to
1147         disaggregate since the connectivity to this node is lost for all
1148         spine nodes in a same fashion.

1150         If a ToF Node goes down, then northern traffic towards it is
1151         routed via alternate ToF nodes in the same plane and there is no
1152         need to disaggregate routes.
...
1159         If the breakage is the last northern link from a ToP node to a ToF
1160         node going down, then the fallen leaf problem affects only The ToF
1161         node, and the connectivity to all the nodes in the PoD is lost
1162         from that ToF node.  This can be observed by other ToF nodes
1163         within the plane where the ToP node is located and positively
1164         disaggregated within that plane.

[nit] s/only The ToF/only the ToF

done

1166      On the other hand, there is a need to disaggregate the routes to
1167      Fallen Leaves in a transitive fashion, all the way to the other
1168      leaves in the following cases:

[] Without having seen the specific mechanism, this overview is hard to digest.

Yes, I’d expect the reader to come back here later; this is for advanced rifters ; ) changed to
“
    On the other hand, there is a need to disaggregate the routes to
    Fallen Leaves within the plane in a transitive fashion, that is,
    all the way to the other leaves, in the following cases
“

1170      o  If the breakage is the last northern link from a leaf node within
1171         a plane (there is only one such link in a maximally partitioned
1172         fabric) that goes down, then connectivity to all unicast prefixes
1173         attached to the leaf node is lost within the plane where the link
1174         is located.  Southern Reflection by a leaf node, e.g., between ToP
1175         nodes, if the PoD has only 2 levels, happens in between planes,
1176         allowing the ToP nodes to detect the problem within the PoD where
1177         it occurs and positively disaggregate.  The breakage can be
1178         observed by the ToF nodes in the same plane through the North
1179         flooding of TIEs from the ToP nodes.  The ToF nodes however need
1180         to be aware of all the affected prefixes for the negative,
1181         possibly transitive disaggregation to be fully effective (i.e.  a
1182         node advertising in control plane that it cannot reach a certain
1183         more specific prefix than default whereas such disaggregation must
1184         in extreme condition propagate further down southbound).  The
1185         problem can also be observed by the ToF nodes in the other planes
1186         through the flooding of North TIEs from the affected leaf nodes,
1187         together with non-node North TIEs which indicate the affected
1188         prefixes.  To be effective in that case, the positive
1189         disaggregation must reach down to the nodes that make the plane
1190         selection, which are typically the ingress leaf nodes.  The
1191         information is not useful for routing in the intermediate levels.

[nit] s/in control plane/in the control plane
done

[nit] s/in extreme condition/in the extreme condition
done

1193      o  If the breakage is a ToP node in a maximally partitioned fabric -
1194         in which case it is the only ToP node serving the plane in that
1195         PoD - goes down, then the connectivity to all the nodes in the PoD
1196         is lost within the plane where the ToP node is located.
1197         Consequently, all leaves of the PoD fall in this plane.  Since the
1198         Southern Reflection between the ToF nodes happens only within a
1199         plane, ToF nodes in other planes cannot discover fallen leaves in
1200         a different plane.  They also cannot determine beyond their local
1201         plane whether a leaf node that was initially reachable has become
1202         unreachable.  As the breakage can be observed by the ToF nodes in
1203         the plane where the breakage happened, the ToF nodes in the plane
1204         need to be aware of all the affected prefixes for the negative
1205         disaggregation to be fully effective.  The problem can also be
1206         observed by the ToF nodes in the other planes through the flooding
1207         of North TIEs from the affected leaf nodes, if there are only 3
1208         levels and the ToP nodes are directly connected to the leaf nodes,
1209         and then again it can only be effective it is propagated
1210         transitively to the leaf, and useless above that level.

[nit] s/fabric -...- goes down,/fabric -...-,

Actually I meant
“

               If the breakage is a ToP node in a maximally
               partitioned fabric - in which case it is the only ToP
               node serving the plane in that PoD that goes down -
“

1212      For the sake of easy comprehension let us roll the abstractions back
1213      into a simple example and observe that in Figure 3 the loss of link
1214      Spine 122 to Leaf 122 will make Leaf 122 a fallen leaf for Top-of-
1215      Fabric plane B.  Worse, if the cabling was never present in first
1216      place, plane B will not even be able to know that such a fallen leaf
1217      exists.  Hence partitioning without further treatment results in two
1218      grave problems:

[] "For the sake of easy comprehension...Figure 3..."  Finally!
Hmmm...sorry...I mean, it is a little ironic that after all the new
terminology, detailed descriptions and figures, the clearer
explanation uses the simplest drawing.

I leave that to Tony

[nit] s/in first place/in the first place
done

1220      o  Leaf 111 trying to route to Leaf 122 MUST choose Spine 111 in
1221         plane A as its next hop since plane B will inevitably blackhole
1222         the packet when forwarding using default routes or do excessive
1223         bow tying.  This information must be in its routing table.

[major] s/MUST/must   This is not a Normative statement, just a
statement of fact (inside an example).
done

1225      o  Any kind of "flooding" or distance vector trying to deal with the
1226         problem by distributing host routes will be able to converge only
1227         using paths through leaves.  The flooding of information on Leaf
1228         122 would have to go up to Top-of-Fabric A and then "loopback"
1229         over other leaves to ToF B leading in extreme cases to traffic for
1230         Leaf 122 when presented to plane B taking an "inverted fabric"
1231         path where leaves start to serve as TOFs, at least for the
1232         duration of a protocol's convergence.

[] "Any kind of "flooding" or distance vector..."  I can guess the
meaning, but it would be better that I don't have to.   Maybe
something like: "Any advertisement..."

Changed to:
“
  o  A path computation trying to deal with the problem by distributing
      host routes may only form paths through leaves.

“

[minor] "information on Leaf 122"  s/on/ about (?), or maybe from. ??

Used “about”

1234    4.1.4.  Discovering Fallen Leaves

1236      As illustrated later, and without further proof, the way to deal with
1237      fallen leaves in multi-plane designs, when aggregation is used, is
1238      that RIFT requires all the ToF nodes to share the same north topology
1239      database.  This happens naturally in single plane design by the means
1240      of northbound flooding and south reflection but needs additional
1241      considerations in multi-plane fabrics.  To satisfy this RIFT, in
1242      multi-plane designs, relies at the ToF level on ring interconnection
1243      of switches in multiple planes.  Other solutions are possible but
1244      they either need more cabling or end up having much longer flooding
1245      paths and/or single points of failure.

[minor] "As illustrated later..."  Where?

See next

[] "and without further proof"  I hope this is at least specified at
that later point.

No need to prove anything. We made a design decision. We’re not routing though leaves -which are the only place where the planes meet in classical designs- so we need to create a wormhole between planes. The natural place for that iss the superspine. See next.

[nit] s/To satisfy this RIFT, in multi-plane designs, relies/To
satisfy this need in multi-plane designs, RIFT relies

I changed the text a little bit more:
“
   When aggregation is used, RIFT deals with fallen leaves by ensuring
   that all the ToF nodes share the same north topology database.  This
   happens naturally in single plane design by the means of northbound
   flooding and south reflection but needs additional considerations in
   multi-plane fabrics.  To enable routing to fallen leaves in multi-
   plane designs, RIFT requires additional interconnection across planes
   between the ToF nodes, e.g., using rings as illustrated in Figure 13.
   Other solutions are possible but they either need more cabling or end
   up having much longer flooding paths and/or single points of failure.

“

1247      In detail, by reserving two ports on each Top-of-Fabric node it is
1248      possible to connect them together by interplane bi-directional rings
1249      as illustrated in Figure 13.  The rings will be used to exchange full
1250      north topology information between planes.  All ToFs having same
1251      north topology allows by the means of transitive, negative
1252      disaggregation described in Section 4.2.5.2 to efficiently fix any
1253      possible fallen leaf scenario.  Somewhat as a side-effect, the
1254      exchange of information fulfills the ask to present full view of the
1255      fabric topology at the Top-of-Fabric level, without the need to
1256      collate it from multiple points by additional complexity of
1257      technologies like [RFC7752].

[nit] s/fulfills the ask to present full view/fulfills the requirement
to have a full view
done

[] "..., without the need to collate it from multiple points by
additional complexity of technologies like [RFC7752]."  This last
phrase is unnecessary: because carrying RIFT information in BGP-LS is
not defined, and more importantly, there's no need to criticize other
technology to make RIFT look better.
done

1259              +---+  +---+  +---+  +---+  +---+  +---+  +--------+
1260           |   |  |   |  |   |  |   |  |   |  |   |  |        |
1261           |      |      |      |      |      |      |        |
1262            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1263          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1264          | | o |  | o |  | o |  | o |  | o |  | o |  | o | |    | Plane A
1265          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1266            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1267          |      |      |      |      |      |      |         |
1268            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1269          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1270          | | o |  | o |  | o |  | o |  | o |  | o |  | o | |    | Plane B
1271          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1272            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1273           |      |      |      |      |      |      |        |
1274                               ...                            |
1275           |      |      |      |      |      |      |        |
1276            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1277          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1278          | | o |  | o |  | o |  | o |  | o |  | o |  | o | |    | Plane X
1279          +-|   |--|   |--|   |--|   |--|   |--|   |--|   |-+    |
1280            +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+  +-o-+      |
1281           |      |      |      |      |      |      |        |
1282           |   |  |   |  |   |  |   |  |   |  |   |  |        |
1283              +---+  +---+  +---+  +---+  +---+  +---+  +--------+
1284    Rings    1      2      3      4      5      6      7

1286        Figure 13: Connecting Top-of-Fabric Nodes Across Planes by Rings

[minor] Is that one ring per plane, multiple rings per plane or a big
ring for all the planes?  The drawing is not clear to me. :-(

Maybe you missed that the Rings are numbered at the bottom?
I tweaked the image as follows, I hope this helps:
“

Ring   1       2       3       4       5       6        7
      /   \   /   \   /   \   /   \   /   \   /   \   /     \
      |   .   |   .   |   .   |   .   |   .   |   .   |     .
      |   .   |   .   |   .   |   .   |   .   |   .   |     .
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
  | | O | . | O | . | O | . | O | . | O | . | O | . | O | | .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
      |   .   |   .   |   .   |   .   |   .   |   .   |     .   Plane A
   ---|---.---|---.---|---.---|---.---|---.---|---.---|-----.-----------
      |   .   |   .   |   .   |   .   |   .   |   .   |     .   Plane B
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
  | | O | . | O | . | O | . | O | . | O | . | O | . | O | | .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
      |   .   |   .   |   .   |   .   |   .   |   .   |     .   Plane B
   ---|---.---|---.---|---.---|---.---|---.---|---.---|-----.-----------
      |   .   |   .   |   .   |   .   |   .   |   .   |     .

                               ...

      |   .   |   .   |   .   |   .   |   .   |   .   |     .
   ---|---.---|---.---|---.---|---.---|---.---|---.---|-----.-----------
      |   .   |   .   |   .   |   .   |   .   |   .   |     .   Plane X
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
  | | O | . | O | . | O | . | O | . | O | . | O | . | O | | .
  +-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-.-|   |-+ .
    +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+ . +-o-+   .
      |   .   |   .   |   .   |   .   |   .   |   .   |     .
      |   .   |   .   |   .   |   .   |   .   |   .   |     .
      \   /   \   /   \   /   \   /   \   /   \   /   \     /
Ring   1       2       3       4       5       6        7

“

1288    4.1.5.  Addressing the Fallen Leaves Problem

1290      One consequence of the "Fallen Leaf" problem is that some prefixes
1291      attached to the fallen leaf become unreachable from some of the ToF
1292      nodes.  RIFT proposes two methods to address this issue, the positive
1293      and the negative disaggregation.  Both methods flood South TIEs to
1294      advertise the impacted prefix(es).

[nit] s/RIFT proposes two methods/RIFT defines two methods
Done

Many tanks again for the careful reading. Sorry that the 3-D representation takes time and effort to get used to.

Please let me know how you fell about the changes above.

Keep safe

Pascal

[End of Review - Part 1]

Dear authors:

Thanks!

Alvaro.

...
1341   4.2.  Specification
...

[nit] Suggestion>

[nit] Suggestion>

1371   4.2.1.  Transport

Suggestion>

OLD>

NEW>

   struct LIEPacket {
  ...
       /** Node's PoD. */

...but I did find this:

[] Suggestion>

[major]
   struct LIEPacket {
...

What am I missing?

1468     8.  [

...

...

1507   4.2.2.1.  LIE FSM

Suggestion>

   (Anything else?)

...
2069   4.2.3.3.  Flooding
...

...

...
6362   B.1.  common.thrift
...

[End of Review 2a]