Last Call Review of draft-ietf-bess-nsh-bgp-control-plane-12
review-ietf-bess-nsh-bgp-control-plane-12-genart-lc-carpenter-2019-12-09-00

Request Review of draft-ietf-bess-nsh-bgp-control-plane
Requested rev. no specific revision (document currently at 18)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2019-12-13
Requested 2019-11-29
Authors Adrian Farrel, John Drake, Eric Rosen, Jim Uttaro, Luay Jalil
Draft last updated 2019-12-09
Completed reviews Rtgdir Last Call review of -13 by Ravi Singh (diff)
Tsvart Last Call review of -13 by Olivier Bonaventure (diff)
Opsdir Last Call review of -13 by Sheng Jiang (diff)
Secdir Last Call review of -13 by Scott Kelly (diff)
Genart Last Call review of -12 by Brian Carpenter (diff)
Assignment Reviewer Brian Carpenter 
State Completed
Review review-ietf-bess-nsh-bgp-control-plane-12-genart-lc-carpenter-2019-12-09
Posted at https://mailarchive.ietf.org/arch/msg/gen-art/x56AXHv1koysQ-AiicDeTGdeMXY
Reviewed rev. 12 (document currently at 18)
Review result Ready with Issues
Review completed: 2019-12-09

Review
review-ietf-bess-nsh-bgp-control-plane-12-genart-lc-carpenter-2019-12-09

Gen-ART Last Call review of draft-ietf-bess-nsh-bgp-control-plane-12

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
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-bess-nsh-bgp-control-plane-12.txt
Reviewer: Brian Carpenter
Review Date: 2019-12-10
IETF LC End Date: 2019-12-13
IESG Telechat date:  

Summary: Ready with questions
--------

Comments:
---------

I am not a BGP expert and did not check the BGP details. This
is a pretty complex mechanism so I would have liked to hear of
at least a lab-scale implementation. I wouldn't be shocked if
this was diverted to Experimental.

Minor issues:
-------------
Actually these are mainly questions:

There are numerous references, starting in the Abstract, to the
"Controller" but it isn't defined or described in any one place.
I expected to find it in RFC8300, but no. So what is the Controller?

RFC8300 requires NSH+original_packet to be encapsulated in a Transport
Encapsulation. In section 2.1 we find:

>  Note that the presence of the NSH can make it difficult for nodes in
>  the underlay network to locate the fields in the original packet that
>  would normally be used to constrain equal cost multipath (ECMP)
>  forwarding.  Therefore, it is recommended that the node prepending
>  the NSH also provide some form of entropy indicator that can be used
>  in the underlay network.  How this indicator is generated and
>  supplied, and how an SFF generates a new entropy indicator when it
>  forwards a packet to the next SFF are out of scope of this document.

I would have expected that text to state that the entropy indicator is
a property of the Transport Encapsulation required by RFC8300. (Isn't
the Service Function Overlay Network in fact the embodiment of the
Transport Encapsulation?) 

In section 2.2 we find:

>  When choosing the next SFI in a path, the SFF uses the SPI and SI as
>  well as the SFT to choose among the SFIs, applying, for example, a
>  load balancing algorithm or direct knowledge of the underlay network
>  topology as described in Section 4.

I'm probably missing something, but doesn't that risk a conflict with
the statement above about the entropy indicator? How would this choice
of path be guaranteed congruent with the choice of path by the underlay
network? Or doesn't that matter?

> 4.4.  Classifier Operation
>
>  As shown in Figure 1, the Classifier is a component that is used to
>  assign packets to an SFP.
>
>  The Classifier is responsible for determining to which packet flow a
>  packet belongs (usually by inspecting the packet header),...

Would it be better to state explicitly that the method of classification
is out of scope for this document? There is a whole world of complexity
in that "(usually...)".

> 4.5.  Service Function Forwarder Operation

This section left me a bit puzzled. We've got the original packet,
the classifier puts an NSH in front, we've got forwarding state,
but we don't seem to have an IP header in front of the NSH to hand to
the fowarding engine. Where's the Transport Encapsulation?

Nits:
-----
"such errors should be logged" ... "should log the event"
"should either withdraw the SFPR or re-advertise it"
Intentional lower case "should"?

IDnits said:
  -- The document has examples using IPv4 documentation addresses according
     to RFC6890, but does not use any IPv6 documentation addresses.  Maybe
     there should be IPv6 examples, too?