Re: [Bier] Genart last call review of draft-ietf-bier-te-arch-10

[Toerless Eckert <[e-mail]>]

Thanks, Robert

Detailed replies for your comments below inline.
It should resolve all your concerns.
These have been integrated into draft rev -12
which the authors feel is ready for RFC editor.

Full diff from -11 to -12:

http://tools.ietf.org//rfcdiff?url1=https://www.ietf.org/archive/id/draft-ietf-bier-te-arch-11.txt&url2=https://www.ietf.org/archive/id/draft-ietf-bier-te-arch-12.txt

Diff with just the deltas for comments by Robert Sparks, Yingzhen Qu and Martin Duke.

http://tools.ietf.org//rfcdiff?url1=https://raw.githubusercontent.com/toerless/bier-te-arch/32d75563d07b8f7559bc2a88e80f113e228ad21a/draft-ietf-bier-te-arch.txt&url2=https://raw.githubusercontent.com/toerless/bier-te-arch/fbb26ac4d2cc5dd841e36a00801f58ad5c7eae11/draft-ietf-bier-te-arch.txt

Thanks again for your review.

Toerless

On Sat, Aug 07, 2021 at 03:42:02PM -0700, Robert Sparks via Datatracker wrote:
> Reviewer: Robert Sparks
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-bier-te-arch-10
> Reviewer: Robert Sparks
> Review Date: 2021-08-07
> IETF LC End Date: 2021-08-11
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary: Essentially ready for publication as a Proposed Standard RFC, but with
> nits, and possibly a minor issue, to address before publication.
> 
> Possible minor issue:
> 
> The Security Considerations section of this document could discuss an
> exponential amplification attack by way of misconfiguration using the DNC bit.
> If you have the following configuration (best read in a fixed-width font):
> 
> BFRA: p1 -> forward_connected(DNC) BFRC
>       p2 -> forward_connected(DNC) BFRD
>       p3 -> local_decap
> 
> BFRB: p1 -> forward_connected(DNC) BFRC
>       p2 -> forward_connected(DNC) BFRD
>       p3 -> local_decap
> 
> BFRC: p1 -> forward_connected(DNC) BFRA
>       p2 -> forward_connected(DNC) BFRB
>       p3 -> local_decap
> 
> BFRD: p1 -> forward_connected(DNC) BFRA
>       p2 -> forward_connected(DNC) BFRB
>       p3 -> local_decap
> 
> A single packet with bits (p1,p2,p3) introduced to any of the 4 BFR will
> produce an exponentially increasing amount of internal traffic, and traffic
> sent to whatever the local_decaped packets is configured for.
> 
>          p3              p3
>         BFRA p1 ---- p1 BFRC
>               \     /
>                \   /
>                  X
>                /   \
>               /     \
>         BFRB p2 ---- p2 BFRD
>          p3              p3
> 
> This may be easy to achieve accidentally if following the suggestion for
> creating a bidirectional ring described in section 5.1.6.

This point was cleared up in email thread before, and we decided to not
add more text about this (with Alvara, Robert).

> Larger Nits:
> 
> The first example uses terms defined later in the document (like local_decap).
> It would help the reader to say that right before the example.

Done.

> Section 2.3 list 2 item 2. What is a "reference option"?

The one option for a component that you always use when explaining/defining things
or raisinig requirements.

> This first sentence of
> this item should be simplified, possibly by breaking it into two or more.

Split into two sentences.

> The first sentence of section 2.4 does not parse. Perhaps "BIER-TE forwarding
> is designed to make it easy to build or program common forwarding hardware."
> but I'm lost at "with BIER". Please clarify.

rewrote first sentence as follows:

BIER-TE forwarding rules, especially the Bitstring parsing are designed to be as close
as possible to those of BIER in the expectation that this eases the programming of BIER-TE forwarding
code and/or BIER-TE forwarding hardware on platforms supporting BIER.

> In section 3.2, there is a nested list that is introduced as "functionality"
> but later referred to as "steps". Please provide a clearer description, and
> make it obvious that these are the steps that the subsections (such as 3.2.1)
> refer to.Is the outer list the steps 3.2.1 refers to?

Yes. I did change the text in section 3.2 to introduce two terms for those two
main parts of functionality, BIER-TE topology control and BIER-TE tree control.
I then put xref into the three places in the following sections and refer to those
two new terms as well. Alas, i gave up again after one hour trying to make XMLv2 (which
this was written in) to give me useful references, but it can only render useless "()"
(i already know this problem). So i also added note to RFC-editor and will work with
them to get this fixed after v3 conversion (i won't do that now).

<t>[RFC-Editor: the following text has three references to anchors topology-control, topology-control-1 and tree-control. Unfortunately, XMLv2 does not offer any tagging that reasonable references are generated (i had this problem already in RFCs last year. Please make sure there are useful-to-read cross-references in the RFC in these three places after you convert to XMLv3.]</t>

Worse yet: I don't intend to ever write an XML v3 draft directly but wanted to move directly
from v2 to Carsten's markup tool, but i wouldn't know how to put labelled
tags into list elements and then refer to them. Hopefully there will be a way
to do this for next work...

> Section 3.2 list item 2, sublist item 3. "Install state on the BFIR to
> imposition the desired BIER packet header(s) for packets of the overlay flow"
> does not parse. Please clarify.

Already changed for other reviewer:

Install state on the BFIR to impose the desired BIER packet header(s) for packets of the overlay flow.

(imposition/impose is term also used in rfc8279, hence i wanted to stick to it).

> Please clarify the last sentence (starting "See also") of 3.2.1.2. I don't know
> what you are really trying to say but "solution describes interaction" doesn't
> make sense.

Already changed for other reviewer to:

See also [I-D.ietf-bier-multicast-http-response] for an application leveraging BIER-TE engineered trees.

> At 3.2.1.4, what does "out-of-scope FRR procedures" mean? I suggest just
> dropping "out-of-scope". If that's not right, more clarification is needed.

Changed sentence already for another reviewer to:

When link or nodes fail or recover in the topology, BIER-TE could
quickly respond with FRR procedures such as [I-D.eckert-bier-te-frr],
the details of which are out of scope for this document.

Putting possible FRR procedures outside this doc was agreed upon by WG a
long time ago, and like rfc8279 also mentions several important adjacencies
as being out-of-scope i think it is important to be explicit about this
instead of not mentioning it and letting readers guess.

> Section 3.4 second paragraph last sentence: Did you mean "BIER specific
> routing" rather than "BFER specific routing"?

Yes. Great catch.

> Section 4.1, Check that you really meant BFR-id = SI * 2^BSL + BP in paragraph
> three.

Thanks.  There actually is an offset bug because BP starts to count from 1 according to rfc8279,
so gixed up text:

indices into the BIFT are SI:BitString and
BFR-id. BitString is indicating a BP, and therefore: SI:BP = BFR-id = SI * 2^BSL + (BP - 1).

> Section 5.1, second paragraph: The list of sections '(4.1, ... 4.8)' didn't
> track a document change. I think you mean to point to '(5.1.1, ...'. But the
> list is unnecessary - I suggest deleting it.

Thanks. Gone. Seems this list was inserted a long time ago between co-author discuss, so
no ask from IETF/IESG review.

> In section 5.1.6, I think you are using L3 to mean different things in the
> first paragraph and in the example.

Hah. good catch for overload of TLA. Replaced L3 in first paragraph with "layer 3",
as the rest of the document only uses Li to refer to some Link<i>.

> Section 5.1.9 after Figure 17, you point to figure 20, but I think you really
> meant figure 17.

Reference already fixed through another reviewer.

> (It's interesting that figure 17 and 20 are essentially the
> same, perhaps the document could be simplified).

I actually had one picture first but Alvaro strongly suggested to duplicate
the picture locally to 5.3.6.1, and that allowed me also to somewhat modify the
picture so that it would better fit the explanation needed in that second place.
So i think its actually better with two local, slightly different pictures.

> Section 5.3.5: Where did this 20%..80% range come from? The last sentence is
> unclear.

The range came from an (un?)educated guessing by yours truly. I still think its a good guestimate,
but i already removed the range for another reviewer. 

> Section 5.3.6.2: paragraph 3. The sentence starting "This is much worse" is
> complex. Please break it into several.

rewritten to:

In the worst case, one assigns random subsets of BFERs 
to different SIs. This will result in an outcome much worse than in (non-TE) BIER: It 
maximizes the amount of unnecessary topology overlap across SI and therefore 
reduces the number of BFER that can be reached across each individual SI.

> Section 5.3.7 First paragraph. This long sentence fails to parse. Please
> simplify it.

rewrote paragraph as follows:

<t>BIER-TE can, like BIER, support multiple SIs within a sub-domain. This allows
to apply the mapping BFR-id = SI:BP = SI*2^BSL + (BP - 1). This allows to re-use
the BIER architecture concept of BFR-id and therefore minimize BIER-TE specific functions in possible
BIER layer control plane mechanisms with BIER-TE, including flow overlay methods and BIER header fields.</t>

> Section 7 paragraph 6 (beginning "For additional,"): This sentence is very hard
> to parse. Please simplify it.

Changed to:

<t>When any of these security mechanisms/protocols are used for communications
between a BIER-TE controller and BFRs, their security considerations apply to BIER-TE.

> Smaller Nits:
> 
> Overview: Expand BIFT on first use.

Fixed through other review already.

> The Overview claims that the reader is expected to be familiar with both
> RFC8279 and RFC 8296, but only lists the first as a normative reference.
> Consider making RFC 8296 normative, or adjusting the introduction text.

rfc8296 already made normative through another review.

> First bullet in overview: "reference forwarding examples" -> "forwarding
> examples"

Fixed.

> Last bullet in overview: "sections of document" -> "sections of the document"

Fixed.

> In 2.1, "To send in addition to BFR6 via BFR4 also a copy to BFR3" is hard to
> parse. Perhaps: "To send a copy to BFR3 in addition to BFR6 via BFR4".

Changed to: To send a copy to BFR6 via BFR4 and also a copy to BFR3, ...

> Section 3.2.1: Why is "Notwithstanding other option," necessary. I suggest
> deleting it.

Ok. Deleted. But then added more clearly in following paragraph:

The single, centralized BIER-TE controller is used in this document as reference option for the BIER-TE control plane but other options are equally feasible.

> Section 3.2.1: "without any active dynamic components" is odd. Perhaps replace
> the sentence with "Automated configuration of the BIER control plane is not
> required. An operator can manually configure the BIER control plane via CLI on
> the BFRs."

(this is following directly the above sentence i inserted).

Rewritten to: The BIER-TE control plane could equally be implemented without automated configuration/protocols, by an operator via CLI on the BFRs. 

> Section 3.3: "constitutes of the following components" is obtuse. Perhaps you
> could replace it with "consists of"?

Ben brought this up as well. I tried to google what is best, and if the word
is uncommon, i would rather pick "is composed from", but for now i am pushing
to RFC editor:

<t>[RFC-editor Q: "constitutes of" / "consists of" / "composed from..." ???]</t>

> Section 3:3: I suggest replacing "This is done to inhibit that packets can
> loop" with "This inhibits loop detection."

Thast replacement would mean the opposite of whats intended (inhibit loops).

I did try to rewrite and split the whole sentence to make it better:

Clearing these bits inhibits
packets from looping when the BitString erroneously includes a forwarding loop.
When a forward_connected() adjacency has the "DoNotClear" (DNC) flag
set, then this BP is re-set for the packet copied to that adjacency. 
See <xref target="forward-connected"/>).</t>


> Section 4.6: I suggest replacing "support to configure" with "support
> configuring" and "support to have" with "support having".

Already replaced by another hopefully better wording from another review:

BFR that support BIER-TE and BIER MUST support configuration that
enables BIER-TE...

> Section 4.6 (and other places) Saying "clear DNC", that is clear Do-Not-Clear
> is dissonant. It might help avoid confusion to say "unset DNC", or replace
> "with clear DNC flag" with "with the DNC flag net set".

Fixed. But couldn't find another place with this issue.

> Section 5.1.7: I suggest replacing "allows to use" with "allows using"

Lars raised the same point, and when i tried to dig down into
rules i ended up with a pointer that had me punt the point to RFC-Editor:

<t>[RFC-Editor: A reviewer (Lars Eggert) noted that the infinite "to use" in the following sentence is not correct. The same was also noted for several other similar instances. The following URL seems to indicate though that this is a per-case decision, which seems undefined: https://writingcenter.gmu.edu/guides/choosing-between-infinitive-and-gerund-to-do-or-doing. What exactly should be done about this ?].</t>

> Section 5.3.4: Why is complex quoted in 'how "complex" the Tree Engineering is'?

The text in the following paragraphs introduces two different methods,
"independent branches" and "interdepenend branches". I was calling this
different "complexities", aka, trying to define "complex" as a classifier.

But the sentence becomes hopefully easier by replacing "complex" with type:

... based on what type of Tree Engineering
the BIER-TE controller needs to performs for a particular application.

> Section 5.3.4: "Communications between BFIR flow" -> "Communications between
> the BFIR flow"

Fixed.

> Section 5.3.6.1, paragraph 3 first sentence: You say "over time" three times in
> this sentence. Please simply it.

Fixed to:

<t>Given how networks can grow over time, replication efficiency in an area
will then also go down over time when BFR-ids are only allocated sequentially, network wide.

> Section 5.3.6.1, paragraph 3, last sentence: I suggest changing "consider to
> use" to "consider using"

See above RFC-Editor note.