[External Email. Be cautious of content]
Reviewer: Dave Thaler
Review result: Ready with Issues
As assigned INT directorate reviewer for draft-ietf-rift-rift my comments were
written primarily for the benefit of the Internet Area Directors. Document
editors and shepherd(s) should treat these comments just like they would treat
comments from any other IETF contributors and resolve them along with any other
Last Call comments that have been received. For more details on the INT
Directorate, see <https://urldefense.com/v3/__https://datatracker.ietf.org/group/intdir/about/__;!!NEt6yMaO-gk!Ae5ZOtMnMgYH7U2HYlKSsm6QY9R1BN_-FFm5XnMJoZFGDebEwHACl_jmoJJrG0ILMcxo0ehyYwWSSg$ >.
A summary of my main feedback is in the list below, but a bunch of other
editorial nits are called out in comments or redlines at: PDF:
https://urldefense.com/v3/__https://1drv.ms/b/s!Aqj-Bj9PNivcn-5XwvmeGhtn6fM2pQ?e=tsUrGY__;!!NEt6yMaO-gk!Ae5ZOtMnMgYH7U2HYlKSsm6QY9R1BN_-FFm5XnMJoZFGDebEwHACl_jmoJJrG0ILMcxo0egBUrKVLQ$
DOCX:
https://urldefense.com/v3/__https://1drv.ms/w/s!Aqj-Bj9PNivcn-5VtiY6w4aezViPGw?e=q0ebxd__;!!NEt6yMaO-gk!Ae5ZOtMnMgYH7U2HYlKSsm6QY9R1BN_-FFm5XnMJoZFGDebEwHACl_jmoJJrG0ILMcxo0ehuWswosg$
1. Figure 1 does not mention the address family, nor does the text discussing
it. But the text says "A/32 is node A's loopback" which would seem to imply
that this figure is IPv4-only. Clarify.
jhead>> Good point, we'll add some text to indicate that the information is IPv4.
2. There are some gratuitous differences in labels between ASCII diagrams and
SVG diagrams in the HTML/PDF versions. Some differences are understandable but
some are just gratuitous. E.g., Figure 10 "ToF" vs "Top-of-Fabric" and
"(depth)" vs "(in depth)", etc.
jhead>> Also good, we'll go through and ensure the labels are parallel between both variations.
3. The draft says that messages go over link-scoped multicast (or in one
variant, broadcast). But it allows both TTL=1 and TTL=255. I'd have expected
it to say that the mitigation against things counting down to TTL=1 is to drop
packets that were received on a destination address other than the link scoped
multicast/broadcast address (i.e., drop unicast/anycast packets), but the spec
has no such security protection. Since sending to multicast/broadcast is a
MUST, why not say that receivers should check that?
jhead>> Agreed. Great catch, we'll add it.
4. Section 6.3.1 says "TIEs MUST be sent with an IPv4 Time to Live (TTL) or an
IPv6 Hop Limit (HL) of either 1 or 255 and also MUST be ignored if received
with values different than 1 or 255. This prevents RIFT information from
reaching beyond a single L3 next-hop in the topology." No it doesn’t "prevent"
it… Since a sender could send with TTL=255 and it arrives with TTL=1. It’s
the fact that you use link-scoped multicast addresses and only accept packets
that arrive on that address (NOT unicast, for example) that actually “prevents”
it.
jhead>> Yes, agreed. We'll add it.
5. Section 9.2 says "RIFT explicitly requires the use of a TTL/HL value of 1
*or* 255 when sending/receiving LIEs and TIEs so that implementors have a
choice between the two." On receiving, it seems like there is no choice, one
has to support both in order to ensure interoperability with senders that get a
choice. The text implies that receivers can choose to just support one of the
two, which I don’t think is the case."
jhead>> Okay, we'll clarify. Essentially it's a "MUST" for multicast and "MAY" for broadcast.
6. Page 37 says "A simplified version MAY be implemented on platforms with … no
multicast support … by … sending and receiving LIE frames on IPv6 all routers
multicast address". This sentence doesn’t make sense. It says if you have
“no” multicast support then you can send and receive on this multicast address,
which is a contradiction. Perhaps delete “or no” and let “limited” cover the
intended meanings
jhead>> Agreed, good fix. We'll remove "or no" and let "limited" cover it.
7. Are rows in Table 1 symmetric? e.g., there’s a row for Local=IPv4,IPv6 and
Remote=IPv6 but not vice versa. Also, what about a row for Local=IPv4,IPv6 and
Remote=IPv4 (including vice versa)?
jhead>> Gunter caught this one too, so we'll add that it's fully symmetric (i.e., RIFT FSM on the adjacency is the same on both sides).
8. The last row in Table 2 says "If IPv4 and IPv6 LIEs advertise conflicting
_ipv4_forwarding_capable_ flags, the behavior is unspecified." How can that
happen? The top of page 38 says “If IPv4 forwarding is supported on an
interface, _ipv4_forwarding_capable_ MUST be set to true for all LIEs
advertised from that interface”. So when could it be false in this row? Maybe
just delete this sentence due to that MUST.
jhead>> LIEs on both address families send asynchronously, so a quick mismatch can't be avoided. There is also the odd implementation error to be considered as well.
9. Section 6.3.3 says "The TIEHEader can also carry an _origination_time_
schema element (for fabrics that utilize precision timing) which contains the
absolute timestamp". Section 6.8.4 does talk about clock synchronization so I'd
suggest providing a forward reference to that section.
jhead>> We will add it.
10. Section 6.3.3.1.5 says "On a periodic basis all TIEs with lifetime left > 0
MUST be sent out on the adjacency, removed from TIES_TX list and requeued onto
TIES_RTX list. The specific period is out of scope for this document." Are
there any constraints? Is once a century considered compliant? What’s the point
of having a MUST if there’s no way to tell whether an implementation complies?
Similarly section 9.1 says "a node SHOULD implement a strategy of discarding
contents of all TIEs that were not present in the SPF tree over a certain,
configurable period of time". Are there any recommendations on constraints
(whether max or min) on this period?"
jhead>> We can't define that since a routing protocol will only ever be able to provide eventual consistency. Without knowing the scale of the deployment there is no way to ensure that things are processed
at the necessary pace to support that scale.
11. Section 6.3.9 says "nodes at the same level do *not* have to agree on a
specific algorithm to select the FRs, but overall load balancing should be
achieved so that different nodes at the same level should tend to select
different parents as FRs". Perhaps informatively cite RFC 2991 here, which
discusses several potential algorithms?
jhead>> Sure, we'll add the reference.
12. Section 10.1 requests assignments in the "Service Name and Transport
Protocol Port Number Registry" with: > Assignee: Tony Przygienda
(prz@xxxxxxxxxxx) > Contact: Jordan Head (jhead@xxxxxxxxxxx) RFC 6335 states:
“For assignments done through RFCs published via the "IETF Document Stream"
[RFC4844], the Assignee will be the IESG <iesg@xxxxxxxx>.“ and “For assignments
done through RFCs published via the "IETF Document Stream" [RFC4844], the
Contact will be the IETF Chair <chair@xxxxxxxx>.“
jhead>> We'll fix it.
13. Table 9 has AddressFamilyMinValue. What does this mean? It’s not used
anywhere in the main protocol description, so what does it mean to have the min
value not be IPv4?
jhead>> It's helpful for some programming languages, basically it's just implementation help.
14. Many of the tables in the IANA considerations section have blanks in the
description column.
jhead>> Not everything necessarily benefits from a description. That said, we are working with IANA and will confirm that this is acceptable or not.
15. RFC 8126: Guidelines for Writing an IANA Considerations Section in RFCs
(rfc-editor.org) has requirements for defining registries, which this document
does. Among the requirements is "Size, format, and syntax of registry entries"
which in my view this document does not do adequately. For example, is it ok
for Description to be empty? What about "Max Schema Version"? Must a contact
or assignee be provided? Only the fields that are columns in table 35 and no
more? Any constraints on the "name" field in Table 35, e.g., can it contain
spaces, punctuation, or non-ASCII characters?
jhead>> Similar to above. IANA has been consulted on the format that they want and there are still a few comments to work through. We'll incorporate your suggestions as we iron out the details with IANA.
16. Table 51 has "bandwidth", but in what units? Would using different units
require registering another name?
jhead>> It's in megabits, we'll clarify this. As for the registry question, it's a typedef, so it won't show up in the registry (see BandwithInMegaBitsType in the common.thrift schema).
17. A couple rows in table 53 have "optional" in the text of the description
column. So everything that doesn’t have “optional” in the description column
is mandatory? Why not have a separate column for that then?
jhead>> The thrift spec is clear as IDL as far as what's optional or required. Descriptions in the registry section are just comments from the normative schema (the registry is auto-generated from the
schema). Data type names include units and as such the fields don't require additional information.
18. Table 59 "from_link" description has "Link to which the address belongs
to." Besides the obviously poor grammar of redundant "to", it doesn't say how
the link is identified. Should it be "Link ID of the link to which the address
belongs"? Table 71 has "Remaining lifetime" but has no units. Is it "in
seconds"?
jhead>> from_link is indeed Link ID, self-evident when looking at the associated type in the schema.
jhead>> Remaining lifetime is defined by the schema in seconds, similar to bandwidth comment above the type will define the resolution.
Thanks,
Dave
_______________________________________________
RIFT mailing list
RIFT@xxxxxxxx
https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/rift__;!!NEt6yMaO-gk!Ae5ZOtMnMgYH7U2HYlKSsm6QY9R1BN_-FFm5XnMJoZFGDebEwHACl_jmoJJrG0ILMcxo0ejpdYSkEg$
