[RTG-DIR] Rtgdir telechat review of draft-ietf-babel-rfc6126bis-11

[Yingzhen Qu via Datatracker <[e-mail]>]

Reviewer: Yingzhen Qu
Review result: Has Issues

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft.

Document: draft-ietf-babel-rfc6126bis
Reviewer: Yingzhen Qu
Review Date: 1 August 2019
Intended Status: Standards Track

Summary:
This document describes the Babel routing protocol, and obsoletes RFC 6126 and
7557. Note: both RFC 6126 and 7557 are experimental, the current bis version is
standards track. I don’t know the history behind, but just want to point it out.

Overall comments:
The document is mostly well written.
I’m not a native speaker, so I’d leave all language nits to RFC editors.

Issues: (the line number is used is from idnits)

There are security concerns raised by Secdir review:
https://datatracker.ietf.org/doc/review-ietf-babel-rfc6126bis-10-secdir-lc-kaufman-2019-06-28/

The description about packet trailer is in RFC 7557 section 2.5, but not
included in the bis version. Considering this document will obsolete RFC 7557,
I suppose it should be self-complete, so readers don’t need to go back to RFC
7557 for more information.

There are multiple places in the document about TLV/Sub-TLV being
“self-terminating”, but I didn’t find what it means. Maybe I’m missing
something here?

Section 1.1
“unmanaged and wireless environment”, what does “unmanaged” mean here?

Section 3.2.3
The interface Hello seqno is changed to outing multicast hello seqno, however
there is no description about unicast hello at all. While I was reading it, I
kept thinking what if it’s unicast hello? Is there a unicast hello timer
needed? From later sections, I realized there are unicast hellos. So I’d
suggest add some descriptions to avoid the confusion.

Section 3.2.4
There is “the expected incoming Unicast Hello sequence number” and “the
outgoing Unicast Hello sequence number”, but it was not mentioned in section
3.2.3 (related with previous comment).

Section 3.8.1.1
After a node receives a route request, if the given prefix doesn’t exist in its
route table, it MUST send a retraction for that prefix. So my question is
whether a node is allowed to send multiple explicit requests for a given
prefix? If so, what happens if both retractions and updates are received?

Section 4.4
1616    4.4.  Sub-TLV Format

1618       Every TLV carries an explicit length in its header; however, most
1619       TLVs are self-terminating, in the sense that it is possible to
1620       determine the length of the body without reference to the explicit
1621       Length field.  If a TLV has a self-terminating format, then it MAY
1622       allow a sequence of sub-TLVs to follow the body.
This section is about Sub-TLV, however the description language in this
paragraph is mainly about TLV.

Section 4.5
it says that “an implementation may choose to use a dedicated stateless parser
to parse the packet trailer”. Will the packet trailer be able to use the state
parser if there are state there useful or just for implementation purpose?
Although there is no packet trailer defined at this moment.

Section 4.5
1674       parsing a TLV MUST update the parser state even if the TLV is
1675       otherwise ignored due to an unknown mandatory sub-TLV.

Section 4.6.5
1805                 send a new scheduled Hello TLV with the same setting of the
1806                 Unicast flag.  If this is 0, then this Hello represents an
I’d suggest changing the text to “the same setting of flags” instead of “the
same setting of the Unicast flag”, considering the flags will be extended later.

Nits:

1049       metric) from a neighbour neigh with a link cost value equal to cost,
1050       it checks whether it already has a route table entry indexed by
[neighbour neigh] => [neighbour]

Thanks,
Yingzhen