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 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 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." --