Re: [Rift] AD Review of draft-ietf-rift-rift-16 (4.2.3.3.1)

[Antoni Przygienda <[e-mail]>]

Inside as <prz>

From: Alvaro Retana <[e-mail]>
Date: Wednesday, 15 February 2023 at 14:00
To: Jordan Head <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: AD Review of draft-ietf-rift-rift-16 (4.2.3.3.1)
[External Email. Be cautious of content]


Hi!

This message contains §4.2.3.3.1 and a couple of "detours" -- I
pointed those out with separators; for example I dug introduce
you_are_sending_too_quickly and the comments below indicate that this
way:

===== <you_are_sending_too_quickly> =====
...
===== </you_are_sending_too_quickly> =====


Alvaro.



2448    4.2.3.3.1.  Normative Flooding Procedures

2450       On reception of a TIE with an undefined level value in the packet
2451       header the node MAY issue a warning and indiscriminately discard the
2452       packet.  Such packets can be useful however to establish e.g. via
2453       `instance_name`, `name` and `originator` elements in `LIEPacket`
2454       whether the cabling of the node fulfills expectations, even before
2455       ZTP procedures determine levels across the topology.

[] This paragraph seems out of place.


<prz> must have been moved during one of the endless rewrites/moves.
From 2nd sentence on this applies to LIEs and not TIEs and should be moved into LIE processing section.



[major] "MAY issue a warning and indiscriminately discard the packet"

Given that this is an option, what are the considerations to do so?
Is the warning due to the "TIE with an undefined level" or to the fact
that the packer was discarded?  If the former, can the actions be
performed independently of each other; for example, drop the packet
but not log a warning?

<prz> specification is completely permissive here as long as SOME warning is given since such a TIE should never be sent by a correct implementation.



[minor] "indiscriminately discard...packets can be useful"

I'm guessing that "indiscriminately" was used to emphasize the action.
If the packet can se useful, but not flooded (since we're in this
section), maybe the correct action is to ignore (not to discard).  ??

<prz> as you pointed out, from 2nd sentence on it should be inLIE processing section. So such a TIE should never be sent by a correct implementation and hence I think discard & log is the correct action.



[minor] "useful however to establish e.g. via `instance_name`, `name`
and `originator` elements in `LIEPacket` whether the cabling of the
node fulfills expectations"

I'm guessing this is not explained anywhere and that it is probably an
operational issue to be covered in the applicability document, right?

<prz> well, the LIE processing section explains when a LIE is acceptable. A node can send
`instance_name`, `name` and such stuff optionally on LIEs and with that those elements could be compared on
The remote side to an existing expectation to catch sophisticated multi-instance, non-expected node on other
Sie and other miscablings scenarios. This is not strictly protocol spec AFAIS but optional implementation elements. AFAIS
None of that stuff is normative unless you think we should write lots of verbiage with “MAY implement this and then
MAY do this” which I don’t think we’ll make the document any easier to read or contribute to interoperability.


2457       This section specifies the precise, normative flooding mechanism and
2458       can be omitted unless the reader is pursuing an implementation of the
2459       protocol or looks for a deep understanding of underlying information
2460       distribution mechanism.

[] This seems to be a better first-paragraph for the section.



2462       Flooding Procedures are described in terms of a flooding state of an
2463       adjacency and resulting operations on it driven by packet arrivals.
2464       The FSM itself has basically just a single state and is not well
2465       suited to represent the behavior.  An implementation MUST either
2466       implement the given procedures in a verbatim manner or behave on the
2467       wire in the same way as the provided normative procedures of this
2468       paragraph.

[nit] s/a flooding state/the flooding state


[minor] "The FSM itself has basically just a single state and is not
well suited to represent the behavior."

There's no TIE FSM, right?  Then why even mention it?

<prz> yes, no problem revmoing it. It’s basically saying WHY there is no FSM 😉



[major] "MUST either implement the given procedures in a verbatim
manner or behave on the wire in the same way as the provided normative
procedures of this paragraph."

Normative requirements with options are not great.  s/MUST/is expected to

Also, I don't know what the difference is between the options.

<prz> the difference is subtle and I ran at that stuff in interops on other things where people were testing whether I implemented the spec FSMs precisely (which I didn’t) but instead I implemented something that behaved externally exactly like the FSM of the spec. That led to a _loooong_ discussion with customer why they cannot get logs from me indicating the FSM states of the spec though my implementatqion was completely interoperable and acutally worked better than the spec when two of my vendors’ boxes were hooked up together 😉 Philosophically, a spec should be making absolut minimum possible normative and minimum possible is normalizing “external behavior on the wire” and nothing else. Though in practical terms (good) specs give of course FSM/MSC/procedures and so on …

So, if you want we can modify that to

“MUST implement a behavior that is externally indistinguishable from verbatim implementation of the FSMs and normative procedures given here”

I would think this is less clear than what is here already …



2470       RIFT does not specify any kind of flood rate limiting since such
2471       specifications always assume particular points in available
2472       technology speeds and feeds and those points are shifting at faster
2473       and faster rate (speed of light holding for the moment).

[] Judgement about other technology. :-(

Suggestion> RIFT does not specify any kind of flood rate limiting.
Instead, the packets provide hints...

<prz> I guess I’m not woke enough given no other technology is even named but whatever makes is politically more correct is ok with me :-/




2475       To help with adjustement of flooding speeds the encoded packets
2476       provide hints to react accordingly to losses or overruns via
2477       `you_are_sending_too_quickly` in `LIEPacket` and `Packet Number` in
2478       security envelope described in Section 4.4.3.  Flooding of all
2479       corresponding topology exchange elements SHOULD be performed at
2480       highest feasible rate whereas the rate of transmission MUST be
2481       throttled by reacting to packet elements and adequate features of the
2482       system such as e.g. queue lengths or congestion indications in the
2483       protocol packets.

[nit] s/in `LIEPacket`/in the `LIEPacket`

[nit] s/in security envelope/in the security envelope



===== <you_are_sending_too_quickly> =====

[] I went looking for `you_are_sending_too_quickly`:

2952    4.2.3.5.  'Flood Only Node TIEs' Bit

2954       RIFT includes an optional ECN (Explicit Congestion Notification)
2955       mechanism to prevent "flooding inrush" on restart or bring-up with
2956       many southbound neighbors.  A node MAY set on its LIEs the
2957       corresponding `you_are_sending_too_quickly` flag to indicate to the
2958       neighbor that it should temporarily flood node TIEs only to it and
2959       slow down the flooding of any other TIEs.  It SHOULD only set it in
2960       the southbound direction.  The receiving node SHOULD accommodate the
2961       request to lessen the flooding load on the affected node if south of
2962       the sender and SHOULD ignore the indication if northbound.

[nit] s/flood node TIEs only/only flood Node TIEs


[major] "It SHOULD only set it in the southbound direction."

The "SHOULD only" construct is always confusing.

s/
   A node MAY set on its LIEs the corresponding `you_are_sending_too_quickly`
   flag to indicate to the neighbor that it should temporarily flood node TIEs
   only to it and slow down the flooding of any other TIEs.  It SHOULD only
   set it in the southbound direction.
/
   A node MAY set the `you_are_sending_too_quickly` flag in its LIEs to
   indicate to southbound neighbors to temporarily only flood Node TIEs
   to it and slow down the flooding of any other TIEs.


[major] "The receiving node ...SHOULD ignore the indication if northbound."

s/northbound/north of the sender

When is it ok to not ignore the indication if north of the sender?
Why is this action recommended and not required?  The schema says that
it is "Ignored when received from southbound neighbor.", which sounds
to me more like a "MUST".


<prz> so


  1.  Yes, SHOULD only _should_ be rewritten 😉
  2.  Why is it still possibly useful to be send northbound? Well, because some implementatons may use it. Observe that southbound identical copies of things are flooded out all interfaces and there is no flood reduction (since it’s one hop only). An implementation may choose to keep one flooding queue for _all_ interafces. It’s arguably simpler. With that it cannot do anything with one neighbor “choking”. Another implementation may be more sophisticated and keep a queue _per_ southbound interface. With that it could do something with such indication (i.e. reorder TIEs it floods and so on). And that’s why it’s written that way, MAY set it and it SHOULD set it only southbound (implicitly MAY set it northbound) . And a very smart node may ignore the SHOULD ignore on recption when northbound since it may do something with it.

None of that is “just random ruminations” but direct discussons with different implementations and implications of that flag as people use it in different implementations today.




2964       Obviously this mechanism is most useful in the southbound direction.
2965       The distribution of node TIEs guarantees correct behavior of
2966       algorithms like disaggregation or default route origination.
2967       Furthermore though, the use of this bit presents an inherent trade-
2968       off between processing load and convergence speed since suppressing
2969       flooding of northbound prefixes from neighbors permanently will lead
2970       to traffic loss.

[minor] "Obviously this mechanism is most useful in the southbound direction."

It may not be obvious to everyone...

Is there any utility in the other direction?  If
`you_are_sending_too_quickly` is ignored anyway, it seems like this
sentence is not needed.

<prz> I explained above. Putting that into spec is however overspecification and starting to ruminate on implementation details which become obvious to someone who implements/deploys and are basically just fluff obscuring stuff for anybody else. Hence I would NOT include it in the spec.



===== </you_are_sending_too_quickly> =====



===== <Packet Number> =====

[] I also went looking for `Packet Number`.

Where's the Security Envelope in the schema?


<prz> I’m lost what you mean, in 4.4.3 first figure is the envelope with packet number prominent in it



>From §4.4.3:

5275       Packet Number:
5276          16 bits.  An optional, per adjacency, per packet type
5277          monotonically increasing number rolling over using sequence number
5278          arithmetic defined in Appendix A.  A node SHOULD correctly set the
5279          number on subsequent packets or otherwise MUST set the value to
5280          `undefined_packet_number` as provided in the schema.  This number
5281          can be used to detect losses and misordering in flooding for
5282          either operational purposes or in implementation to adjust
5283          flooding behavior to current link or buffer quality.  This number
5284          MUST NOT be used to discard or validate the correctness of
5285          packets.  Packet numbers are incremented on each interface and
5286          within that for each type of packet independently.  This allows to
5287          parallelize packet generation and processing for different types
5288          within an implementation if so desired.

[nit] s/using sequence number arithmetic/using the sequence number arithmetic


[major] "SHOULD correctly set the number on subsequent packets"

This is not a good Normative statement -- among other things, by
recommending it you're opening the door for it to be ok for the number
to not be "correctly set", not to mention the definition of "correct",
etc.

Given the references to Appendix A, it is Normative (right?), so
perhaps the right approach is to simply drop the sentence.

Suggestion>
s/
   An optional, per adjacency, per packet type monotonically increasing
   number rolling over using sequence number arithmetic defined in
   Appendix A.  A node SHOULD correctly set the number on subsequent
   packets or otherwise MUST set the value to `undefined_packet_number`
   as provided in the schema.
/
   An optional, per adjacency, per packet type number set using the
   sequence number arithmetic defined in Appendix A.  If the arithmetic
   in Appendix A is not used the node MUST set the value to
   `undefined_packet_number`.


<prz> Agree. This is better …


===== </Packet Number> =====



===== <Appendix A> =====

[] I also looked at Appendix A.

6968    Appendix A.  Sequence Number Binary Arithmetic

6970       The only reasonably reference to a cleaner than [RFC1982] sequence
6971       number solution is given in [Wikipedia].  It basically converts the
6972       problem into two complement's arithmetic.  Assuming a straight two
6973       complement's subtractions on the bit-width of the sequence number the
6974       corresponding >: and =: relations are defined as:

[] I'm not going to pretend to understand the details, but it seems
that you're saying that RFC1982 is not great and that what is on
Wikipedia is better -- Wikipedia claims the same thing.  Besides not
comparing what RIFT uses with other technology...

The references to Appendix A are Normative (as in, Appendix A is how
RIFT does it), right?  But the reference to RFC1982 is listed as
Normative while the one to Wikipedia is Informative.  Which one should
the implementation follow?  The options I see are: RFC1982, Wikipedia,
or Appendix A (the specification is self-contained there).

I would be very happy with the answer being Appendix A as Wikipedia is
not really a stable reference and you already said that RFC1982 is not
a good solution.  This would mean eliminating the two references and
relying on the text in the Appendix.

<prz> Yes, Appendix A is normative. We can kill all the other refs if you want, I personally am coming originally from Academia where publishing anything without showing previous art is called plagiarism so AFAIS _some_ kind of reference to 1982 as informative may be good.



===== </Appendix A> =====



2485       A node SHOULD NOT send out any topology information elements if the
2486       adjacency is not in a "ThreeWay" state.  No further tightening of
2487       this rule as to e.g. sequence is possible due to possible link
2488       buffering and re-ordering of LIEs and TIEs/TIDEs/TIREs in a real
2489       implementation for e.g.  performance purposes.

[major] "SHOULD NOT send...if the adjacency is not in a "ThreeWay" state"

Is it ok to ever send TIEs if not in ThreeWay?  Why is this behavior
recommended and not required?


<prz> because an implementation may be very asynchronous and the TIE component not be in sync with the flooding component (we see that today even in some ISIS/OSPF things). Plus, add odd buffering issues in a box these days until a packet finally makes it out. So it cannot be precluded that TIEs are still flying out the box after 3-way is down …

So it’s NOT ok but it can happen despite best intentions. In practidcal terms Hence the SHOULD NOT to make the remote aware that it better deals with the situation.


§4.2.2.1 defines ThreeWay as:

   ThreeWay: this state signifies that ThreeWay valid LIEs from a
   neighbor are received. On achieving this state the link can be
   advertised in `neighbors` element in `NodeTIEElement`.


I read that to mean that the TIE describing the local node MUST NOT be
sent unless in ThreeWay.  But that is different from forwarding other
TIEs.

I saw that the next sentence says that "No further tightening of this
rule as to e.g. sequence is possible..." but what you wrote is not
clear (maybe missing a couple of commas) and it doesn't seem to be
related to origination vs flooding.


<prz> well, should we write something like “SHOULD NOT send _any_ TIEs” ? to cover origination/forwarding/whatever? Until 3-way on a link no TIEs receive/send, end story (modulo the subtle implementation issues I mentioned).




2491       A node MUST drop any received TIEs/TIDEs/TIREs unless it is in
2492       ThreeWay state.

[] This sentence sort of answers some of my questions above.

<prz> Ah ok, so it’s already there …



2494       TIDEs and TIREs MUST NOT be re-flooded the way TIEs of other nodes
2495       MUST be always generated by the node itself and cross only to the
2496       neighboring node.

[] Suggestion>
   TIEs generated by other nodes MUST be re-flooded.  TIDEs and TIREs
   MUST NOT be re-flooded.


<prz> yes, it’s bits lot verbiage that’s more confusing than helpful


Juniper Business Use Only