Early Review of draft-ietf-bess-bgp-multicast-controller-11
review-ietf-bess-bgp-multicast-controller-11-rtgdir-early-dukes-2023-11-06-00

I'm completing this early review on behalf of the routing area directorate as
requested by the BESS chairs.

Summary
==============
I found the documents terminology a bit difficult to follow initially, it
heavily relies on draft-ietf-bess-bgp-multicast without fully stating what and
where terminology was borrowed, a terminology section that fully defines
terminology used in the draft (with specific references) should help resolve
this for other readers.

I've not followed this draft's progress but when looking for discussion on-list
for this draft I didn't notice much direct discussion BESS. As an implementor,
this makes me wonder if there are implementations of the draft and where that
discussion has taken place.

Check nits and resolve them please.

Otherwise I found the draft to be complete in its description of how to signal
multicast state from a controller, but with issues that should be resolved
before progressing.

MAJOR
==============
Section 1 and section 4 significantly overlap - which is normative? Combining
these could be helpful.

Security Considerations: This is essentially empty. Is there really no
consideration for a single centralized controller using BGP to set up multicast
distribution trees?  At a minimum, referencing the security considerations in
other RFCs for AAA, confidentiality, etc that make this assertion true seems
useful. Are there new considerations when a single controller is a single point
of attack to inject forwarding replication in a network? Is there no potential
for new DOS attacks? Are there any additional considerations when multicast
controllers must work together across domains as in section 1.6?

MODERATE
==============
Section 4.3.1 ...
 - Does a receiving node NACK if a tunnel other than those specified is
 received? Is there text that describes the scenario? - The draft states "If a
 tunnel is of type MPLS, MPLS in GRE or MPLS in UDP, it is similar to the
 Any-Encapsulation case."  Must it also have a tunnel egress endpoint sub-tlv
 if its similar to Any-Encapsulation? Are all the differences listed?

Section 4.3.3 "It originates a route with similar NLRI" ? "similar" is not
descriptive, please be explicit, describing exactly what is in the
acknowledgement.

IANA Considerations: Please read BCP 26 section 2.2 for an example of
expectations for IANA Considerations.  Use consistent identifiers in the IANA
Considerations and in the body of the draft so IANA can easily replace the text
as values are assigned.

MINOR
==============

The minor issues are editorial and grammar nits with some minor questions (see
***) that should be addressed.

--OLD
"How the calculation is done are outside the scope of this document."

--NEW
"How the calculations are done is outside the scope of this document."

--

This was hard to parse, is the suggested text still correct?
--OLD
"It is a local matter when for the downstream to switch to the new path - it
could be data driven (e.g., after traffic arrives on the new path) or timer
driven."

--NEW
"When to switch the downstream to the new path is a local matter - it could be
data-driven (e.g., after traffic arrives on the new path) or timer-driven."

--

--OLD
"In an SR network, assignment from the common SRGB if it's required to use a
single common label per unidirectional tree, or otherwise assignment from SRLB
is a good choice because it does not require support for context label spaces."

--NEW
"In an SR network, the assignment from the common SRGB is used if it's required
to use a single common label per unidirectional tree; otherwise, the assignment
from SRLB is a good choice because it does not require support for context
label spaces."

--

--OLD
"Because this is no longer triggered hop-by-hop from downstream to upstream, it
is possible that the upstream change happens before the downstream, causing
traffic loop."

--NEW
"Because this is no longer triggered hop-by-hop from downstream to upstream, it
is possible that the upstream change happens before the downstream, causing a
traffic loop."

--

--OLD
"In this case in the multicast packet's label stack the tree label and upstream
neighbor label (if used in case of single common-label per tree) are preceded
by a downstream-assigned "context label"."

--NEW
"In this case, in the multicast packet's label stack, the tree label and
upstream neighbor label (if used in case of a single common-label per tree) are
preceded by a downstream-assigned "context label"."

--

--OLD
"The Replication State route may also have a PMSI Tunnel Attribute (PTA)
attached to specify the provider tunnel while the TEA specifies the local PE-CE
interfaces where traffic need to be sent out."

--NEW
"The Replication State route may also include a PMSI Tunnel Attribute (PTA) to
specify the provider tunnel, while the TEA specifies the local PE-CE interfaces
where the traffic needs to be directed."

--

This one was hard to parse, suggest starting with the need.
--OLD
"If dynamic switching between inclusive and selective tunnels based on data
rate is needed, the ingress PE can advertise/withdraw S-PMSI routes targeted
only at the controllers, without PMSI Tunnel Attribute attached."

--NEW
"If there's a need for dynamic switching between inclusive and selective
tunnels based on data rate, the ingress PE can advertise or withdraw S-PMSI
routes targeted only at the controllers, without attaching a PMSI Tunnel
Attribute."

--

NRLI typo
--OLD
"3.1. Enhancements to TEA
A TEA may encode a list of tunnels. A TEA attached to an MCAST-TREE NLRI
encodes replication information for a <tree, node > that is identified by the
NRLI."

--NEW
"3.1. Enhancements to TEA
A TEA can encode a list of tunnels. A TEA attached to an MCAST-TREE NLRI
encodes replication information for a <tree, node> that is identified by the
NLRI."

--

--OLD
"*An interface's local address - when a packet needs to sent out of the
corresponding interface natively. On a LAN multicast MAC address MUST be used."

--NEW
"*An interface's local address - when a packet needs to be sent out of the
corresponding interface natively. On a LAN multicast MAC address MUST be used."

--

--OLD
"Other than type difference, the two are the encoded the same way."

--NEW
"Other than type difference, the two are encoded the same way."

--

--OLD
"The value field contains a label stack with the same encoding as value part of
the MPLS Label Stack sub-TLV, but with a different type." --NEW "The value
field contains a label stack with the same encoding as the value part of the
MPLS Label Stack sub-TLV, but with a different type."

--

--OLD
"In case of MPLS, the tunnel contains an Receiving MPLS Label Stack sub-TLV for
downstream traffic from the upstream node, and in case of MP2MP it also
contains a regular MPLS Label Stack sub-TLV for upstream traffic to the
upstream node."

--NEW
"In the case of MPLS, the tunnel contains a Receiving MPLS Label Stack sub-TLV
for downstream traffic from the upstream node, and in the case of MP2MP, it
also includes a regular MPLS Label Stack sub-TLV for upstream traffic to the
upstream node."

--

--OLD
"The backup tunnels can be going to the same or different nodes reached by the
original tunnel."

--NEW
"The backup tunnels can lead to the same or different nodes reached by the
original tunnel."

--

--OLD
"As described in Section 1.3, tree label instead of tree identification could
be encoded in the NLRI to identify the tree in the control plane as well as in
the forwarding plane."

--NEW
"As described in Section 1.3, the tree label, instead of tree identification,
could be encoded in the NLRI to identify the tree in the control plane as well
as in the forwarding plane."

--

--OLD
"The MPLS Label Stack sub-TLV can be used to specify the complete label stack
used to send traffic, with the stack including both a transport label (stack)
and label(s) that identify the (tree, neighbor) to the downstream node."

--NEW
"The MPLS Label Stack sub-TLV can be utilized to specify the complete label
stack used to transmit traffic, with the stack including both a transport label
(stack) and label(s) that identify the (tree, neighbor) to the downstream node."

--

--OLD
"The length is two-octet. The value part encodes a one-octet flags field and a
variable length Tunnel Encapsulation Attribute."

--NEW
"The length is two-octet. The value segment encodes a one-octet flags field and
a variable-length Tunnel Encapsulation Attribute."

--

The double if was confusing..
--OLD
"If the tunnel carries a RPF sub-TLV and a Backup Tunnel sub-TLV, then both
traffic arriving on the original tunnel and on the tunnels encoded in the
Backup Tunnel sub-TLV's TEA can be accepted, if the Parallel (P-)bit in the
flags field is set."

--NEW
"If the tunnel carries an RPF sub-TLV and a Backup Tunnel sub-TLV, then both
traffic arriving on the original tunnel and on the tunnels encoded in the
Backup Tunnel sub-TLV's TEA can be accepted, provided the Parallel (P-)bit in
the flags field is set."

--

--OLD
"For option 2 and 3, how the controller learns the common SRGB or each node's
SRLB is outside the scope of this document."

--NEW
"For options 2 and 3, the process through which the controller learns the
common SRGB or each node's SRLB is beyond the scope of this document."

--

Make the text in this bullet identical to the preceding ones to call out the
"NOT" - it took me a few readings to get the difference and not pause at the
first comma. --OLD "*If the sub-TLV includes two labels, the first label is not
allocated for a label forwarding table, then it is assumed to be for a
particular neighbor."

--NEW
"*If the sub-TLV includes two labels and the first label is not allocated for a
label forwarding table, then it is assumed to be for a particular neighbor."

--

--OLD
"To generalize, a label stack of one label (for option 3), two labels (for
option 2 and 1 if neighbor-identifying label is not needed), or three labels
(for option 2 and 1 if neighbor-identifying label is needed). In the rest of
the document, tree-identifying label(-stack) term is used generically."

--NEW
"To generalize, a label stack can contain one label (for option 3), two labels
(for options 2 and 1 if a neighbor-identifying label is not needed), or three
labels (for options 2 and 1 if a neighbor-identifying label is needed). In the
rest of the document, the term 'tree-identifying label(-stack)' is used
generically."

--

--OLD
"After the controller calculates a tree, it constructs one or more Replication
State Route for each tree node as following:"

--NEW
"After the controller calculates a tree, it constructs one or more Replication
State Routes for each tree node as follows:"

--

--OLD
"An IP Address Specific Route Target is attached, with the Global
Administration Field set to the same as the Tree Node's IP Address in the NLRI,
and the Local Admin Field set to 0."

--NEW
"An IP Address Specific Route Target is attached, with the Global
Administration Field matching the Tree Node's IP Address in the NLRI, and the
Local Admin Field set to 0."

--

--OLD
"A Tunnel Encapsulation Attribute (TEA) is attached to encode replication
information, as detailed below."

--NEW
"A Tunnel Encapsulation Attribute (TEA) is attached to encode the replication
information, as detailed below."

--

***The loopback addr usage sounded like a MUST, is it?
--OLD
"The TEA includes one or more tunnels. If the route is for the root node, one
and only one tunnel of type Any-Encapsulation MAY be included with a RPF
sub-TLV and a Receiving MPLS Label Stack sub-TLV to encode a binding SID if it
is assigned for the tree. If the route is for a leaf or bud node (which is both
a leaf node and transit node at the same time), one and only one tunnel of type
Any-Encapsulation MUST be included with a Tunnel Egress Endpoint sub-TLV whose
address is set to a loopback address on the node."

--NEW
"The TEA encompasses one or more tunnels. If the route is for the root node, a
single tunnel of type Any-Encapsulation MAY be included with an RPF sub-TLV and
a Receiving MPLS Label Stack sub-TLV to encode a binding SID, if it is assigned
for the tree. If the route is for a leaf or bud node (which serves both as a
leaf node and transit node simultaneously), a single tunnel of type
Any-Encapsulation MUST be included with a Tunnel Egress Endpoint sub-TLV. The
address of this sub-TLV MUST be set to a loopback address on the node."

--

***Are these field values MUSTs as proposed in NEW?
--OLD
"Each potential tree node MUST be (auto-)configured with an IP Address Specific
Route Target to import Replication State Routes targeted at itself. The Route
Target's Global Administration Field is set to a local address known to be used
by the controller to identify the node, and the Local Administration Field is
set to 0."

--NEW
"Each potential tree node MUST be (auto-)configured with an IP Address Specific
Route Target to import Replication State Routes targeted at itself. The Global
Administration Field of the Route Target MUST be set to a local address, which
is known to be used by the controller to identify the node, and the Local
Administration Field MUST be 0."

--

--OLD
"When a BGP speaker receives Replication State Route and the attached Route
Target matches its (auto-)configured Route Target to import the route, it MUST
stops re-advertising the route further. Otherwise, normal BGP route propagation
rules apply."

--NEW
"When a BGP speaker receives a Replication State Route and the attached Route
Target matches its (auto-)configured Route Target to import the route, it MUST
stop re-advertising the route further. Otherwise, normal BGP route propagation
rules apply."

--

--OLD
"If an imported Replication State Route carries an Extented Community derived
from a Route Target for a local VRF, the route is imported into that VRF
otherwise it is imported into the default routing instance."

--NEW
"If an imported Replication State Route carries an Extended Community derived
from a Route Target for a local VRF, the route is imported into that VRF.
Otherwise, it is imported into the default routing instance."

--

--OLD
"If the tree is a bidirectional (*,g) IP multicast, a (*,g) route is installed,
pointing to the nexthop built as above,"

--NEW
"If the tree is a bidirectional (*,g) IP multicast, a (*,g) route is installed,
which points to the previously constructed nexthop,"

--

--OLD
"If the tree is unidirectional, only one of the tunnels MAY have a Receiving
MPLS Label Stack sub-TLV. If it is bidirectional, multiple tunnels MAY have the
Receiving MPLS Label Stack sub-TLV. For each tunnel with the Receiving MPLS
Label Stack sub-TLV:"

--NEW
"If the tree is unidirectional, only one of the tunnels MAY contain a Receiving
MPLS Label Stack sub-TLV. If it is bidirectional, multiple tunnels MAY contain
the Receiving MPLS Label Stack sub-TLV. For each tunnel with the Receiving MPLS
Label Stack sub-TLV:"

--

--OLD
"After processing a received Replication State Route, the node MUST send an
acknowledgement back to the controller. It originates a route with similar NLRI
except that the Originating Router's IP Address is set to the same Tree Node's
IP Address. It attaches a IP Address Specific Route Target with the Global
Administration Field set to the same as the Originating Router's IP Address in
the receive route and the Local Administration Field to 0."

--NEW
"After processing a received Replication State Route, the node MUST send an
acknowledgement back to the controller. It originates a route with a similar
NLRI, except that the Originating Router's IP Address is set to match the Tree
Node's IP Address. It attaches an IP Address Specific Route Target, with the
Global Administration Field set to match the Originating Router's IP Address in
the received route, and sets the Local Administration Field to 0."

--