Re: [bess] AD Review of draft-ietf-bess-mvpn-extranet-03

[Eric C Rosen <[e-mail]>]

On 11/12/2015 2:50 PM, Alvaro Retana (aretana) wrote:
> Hi!
>
> I just finished reading this document.

[Eric]  Alvaro, thanks for your review.  I just posted revision -04, 
with a number of edits made in response to your comments.

> As mentioned by the WG chairs at the meeting in Yokohama, the technology
> in bess can be complex and hard to penetrate for the average (or novice)
> reader.  While the target of documents like this one is not always the
> average (or novice) reader, it is important that those readers
> (including the IESG and other review directorates) are able to at least
> understand what's going on.

[Eric] Often, reviewers from various directorates are assigned documents
that they are not qualified to review.  This is certainly a problem, but 
not a problem with the documents.  There is also a problem with certain 
ADs (NOT routing area ADs of course) who don't know nearly as much as 
they think they do.  Well, luckily for them, bidirectional multicast is 
not discussed in this document, as it really seems to freak them out!

> Because of the length of the document, it would be nice to include a
> "road map" to guide the reader (in general: "Section x talks about X,
> Section y covers Y, etc.").  It might be easier to include references to
> the specific sections in 1.4 (Overview).

[Eric]  I think the table of contents is as good a road map as we are 
going to get.  Adding additional pointers to the overview doesn't 
provide additional value, and most likely would just introduce errors.
>
> This is a long standards track document, so it is bound to have a lot of
> normative language.  On one hand I don't want to go overboard with
> explicitly mandating every little step..but at the same time we want to
> be clear as to what is "actually required for interoperation or to limit
> behavior which has potential for causing harm" [RFC2119].  Due to the
> potential of bad configurations resulting in incomplete/illdefined
> extranets, I can see why the behavior around RTs would require normative
> language.  However, there are some places where the use of rfc2119
> keywords may be lacking, not needed or inconsistent.  An example of
> inconsistency is this paragraph from Section 7.2.3.1. (When Inter-Site
> Shared Trees Are Used):
>
>     If VRF-S exports a Source Active A-D route that contains C-S in the
>     Multicast Source field of its NLRI, and if that VRF also exports a
>     UMH-eligible route matching C-S, the Source Active A-D route MUST
>     carry at least one RT in common with the UMH-eligible route.  The RT
>     must be chosen such that the following condition holds: if VRF-R
>     contains an extranet C-receiver allowed by policy to receive extranet
>     traffic from C-S, then VRF-R imports both the UMH-eligible route and
>     the Source Active A-D route.
>
> .. If the "route MUST carry at least one RT", why isn't the condition to
> be met also normative?  I don't want to belabor on this specific case,
> it is just an example.

[Eric] I think you are right about this case; I've fixed the text.

> However, I would appreciate it if the authors
> would scan the document for required or unneeded normative language.  I
> pointed at some cases in my comments below.

[Eric] I looked at each of the 200 or so occurrences of "must", 
"should", etc., and made some changes.  I think it's more consistent 
now, but the criteria for when to say "MUST" and when to say "must" have 
never been very clear.

> I do have a couple of items that I think are Major (see below) that I
> would like to see addressed before starting the IETF Last Call.

> Major:
>
>  1. Section 1.3. (Clarification on Use of Route Distinguishers) uses the
>     word "REQUIREs" in a couple of places.  In a strict manner, the
>     rfc2119 key words is "REQUIRED".  While I think that the meaning of
>     using "REQUIREs" should be obvious, please rephrase the text to
>     strictly use the rfc2119 language.

[Eric]  I changed "this document REQUIREs" to "it is REQUIRED by this
document".  Hopefully, the RFC editor won't ding me for unnecessary use
of the passive voice ;-)

>  2. Section 10 (Security Considerations)
>       * What is a "VPN security violation"?  It is mentioned in several
>         places, but it is not explicitly defined.  Please either add a
>         reference or be clear in what it is.

[Eric] A "VPN security violation" is just any situation in which a
packet gets delivered to a VPN where it isn't supposed to go.  I added a
definition in the terminology section, and also in the Security
Considerations section.

>       * Misconfiguration is a significant risk, for example assigning
>         the wrong RTs to the wrong routes.  I think that risk should be
>         mentioned.

[Eric] This is generally true of any technology based upon RFC4364.  I
added a sentence about this to the Security Considerations.

> Minor:
>
>  1. Section 1.1. (Terminology): "We will sometimes say that a route
>     "matches" a particular host if the route matches an IP address of
>     the host."  Given the previous definition (in the same paragraph) of
>     "match" ("the address prefix of the given route is the longest match
>     in that VRF for the given IP address") and the use of match there
>     makes it unclear whether you're talking about a host route or just
>     the longest match.

[Eric] I don't see the ambiguity here.  A given IP address matches a 
given route in the context of a given VRF if the route's address prefix 
is the longest match in that VRF for that address.  A host matches a 
route if the host's IP address matches the route.

>  2. Section 1.3. (Clarification on Use of Route Distinguishers)
>       * This section explains the "unique VRF per RD" condition a couple
>         of times — even though the explanation seems to be the same, it
>         would be nice if it was explained only once.

[Eric]  I think it is explained only once.  There is also an explanation
of the "unique RD per VRF" condition, which is different than the 
"unique VRF per RD" condition.

>       * ""default RD" (discussed above)"  Where this text appears is in
>         fact the first time that a "default RD" is mentioned.  I'm
>         guessing that this refers to the RD in the "unique VRF per RD"
>         condition, but please don't make the reader guess.

[Eric] You're right, the term "default RD" should not have been
introduced without explanation.  When a VRF is configured with only one
RD, we call that the "default RD" for that VRF.  When a VRF is
configured with two RDs, one is configured to be the "default RD", and
one is configured to be the "extranet RD".  I've tried to clear this up
in the text.

>  3. Section 4.1. (UMH-Eligible Routes)  The "MAY" in the second
>     paragraph appears to be part of the example.  Suggested new text:
>       * The UMH-eligible extranet C-source routes do not necessarily
>         have to be unicast routes, they MAY be SAFI-129 routes (see
>         Section 5.1.1 of   [RFC6513]).  If one wants, e.g., a VPN-R
>         C-receiver to be able to receive extranet C-flows from
>         C-sources in VPN-S, but one does not want any VPN-R system to
>         be able to send unicast traffic to those C-sources, then the
>         UMH-eligible routes exported from VPN-S and imported by VPN-R
>         may be SAFI-129 routes.  The SAFI-129 routes are used only
>         for UMH determination, but not for unicast routing.

[Eric] I've modified the text in Section 4.1 to fix this problem.  (The
new text is based on your suggestion, but is slightly different.)

>  4. Section 4.2. (Distribution of Unicast Routes Matching C-RPs and
>     DRs)  "It follows that if a VRF contains C-S, but does not contain a
>     C-RP for C-G, then the VRF must import a unicast route matching a
>     C-RP for C-G." and "Similarly, if a VRF contains a C-RP for C-G, but
>     does not contain C-S, the VRF must import a unicast route matching
>     the DR for C-S."  Should those "must" be a "MUST"?

[Eric] Yes, fixed.

>  5. Section 4.4.1. (The Extranet Source Extended Community)  "…only
>     useful when all the extranet routes from a given VRF are exported
>     with exactly the same set of RTs.  (Cf.  Section 4.3.1 of [RFC4364],
>     which does provide a mechanism…"   Is there any inference that
>     should be made from this text?  Should the Extranet Source Extended
>     Community not be used when the mechanism in rfc4364 is used?  Any
>     downsides if both are used?

[Eric] The CE puts the Extranet Source EC on a given IP route in order
to tell the PE that when the route is propagated as a VPN-IP route, it
should carry the set of RTs that have been provisioned for extranet
sources.  If the CE is using a different mechanism (such as described in
section 4.3.1 of RFC4364) to tell the PE exactly which RTs the route
should carry, there is no point in using the Extranet Source EC.  So it
doesn't make sense to use both mechanisms.  I've put in some explanatory
text saying characterizing the circumstances under which the CE SHOULD
NOT put on the EC, and saying that the PE SHOULD ignore the EC in those
circumstances.

>  6. Section 5.2. (Considerations for Particular Inclusive Tunnel Types)
>       * Please include a reference (or definition) for "inclusive
>         tunnel" (maybe in the Terminology).  I couldn't find one in
>         rfc6513, but did in rfc7582.

[Eric] I put in some text defining "Inclusive Tunnel", and making it 
clear that it is the same thing that is called "Inclusive Tree" in 
RFC6513. (Unfortunately, terminology is not entirely consistent across 
the entire set of MVPN documents.  RFC6625, e.g., uses "Inclusive 
P-tunnel".)

>       * The third paragraph ("In order for VRF-S to set up the P2MP…")
>         seems to explain what is needed for the setup, which is
>         something that I assume is standardized in a different
>         document.  If so, then the "MUST" shouldn't be normative in this
>         document.  s/MUST/must

[Eric] Okay, fixed.

>  7. Section 7.2.1. (Intra-AS I-PMSI A-D Routes)  "As specified in
>     [RFC6514]…the RTs carried by that route MUST be chosen…"  That
>     "MUST" is really in reference to rfc6514, so it shouldn't be
>     normative in this document.  If you want to stress the "MUST", then
>     one solution could be to quote the relevant text; otherwise s/MUST/must

[Eric] Okay, fixed.

>  8. It would be nice if section names were used in addition to section
>     numbers when referring to them in the text — that would make it
>     easier to recall (or look forward to) the text w/out having to go
>     look at the TOC over and over.

[Eric] The section numbers are inserted by means of the <xref
target="whatever"/> mechanism.  I experimented with adding the section
names by adding &quot;<xref target="whatever" format="title">&quot;
However, I found that xml2rfc often did not render the section names
properly, so I had to back this out.  I don't think it's a good practice 
to put the section names in by hand.

> [This point may be able to be addressed by the RFC Editor.]  It looks like you started doing this
> in some places in Section 7, but it is not consistent.

[Eric] Just an accident.

>  9. In several places the text reads: "This document provides
>     procedures…"  Please include a reference to the section where those
>     procedures are.

[Eric] I'd like to be able to say "this document covers X but does not 
cover Y" without having to stop and  give a complete set of references 
to every place in the document where something relevant to X is 
mentioned.  Leaving out a reference would just introduce an error, and 
in any event the proper sections can be found in the ToC.

> 10. Please expand on first use:
>       * s/S-PMSI/Selective Provider Multicast Service Interface (S-PMSI)
>       *   s/SSM/source-specific multicast (SSM)
>       *   s/I-PMSI/Inclusive-PMSI (I-PMSI)
>       *   s/MI-PMSI/Multidirectional I-PMSI (MI-PMSI)

[Eric] Done.

> 11. References: Update the reference to rfc4601 to
>     draft-ietf-pim-rfc4601bis.

[Eric] The last time I included a normative reference to an 
internet-draft, my document got stuck on the RFC Editor's queue for 
about 5 years!  If 4601bis becomes an RFC before the extranet draft does 
(which I admit does seem likely), this change can be made during the 
AUTH48 period (probably the RFC Editor will do it automatically).

> Nits:
>
>  1. Some replacements:
>       * s/misdelivery of data in those scenarios are outlined in those
>         Section 2.3/misdelivery of data in those scenarios are outlined
>         in Section 2.3
>       * (6.3.1) s/and Each such/and each such
>       * (7.1) s/"corresponds to" to/"corresponds" to

[Eric] Fixed.