[Idr] AD Evaluation of draft-ietf-idr-vpn-prefix-orf-23

[Ketan Talaulikar <[e-mail]>]

Hello Authors/WG,

I've done the review as part of AD evaluation
of draft-ietf-idr-vpn-prefix-orf-23 and would like to share the same with
you.

Summary: The document needs some more work - editorial as well technical -
before it can be progressed further.

technical tldr:
1) Mischaracterization of existing solutions in this space
2) Normative procedures with BCP14 keywords provided as examples instead of
standalone normative text
3) Allocating one of 5 reserved bits in ORF common header for this new type
specific purpose (and not doing that via IANA registry)
4) Mix of protocol procedures, operational and deployment considerations
that results in lack of clarity and important details not getting called
out prominently
5) Procedures specified as pseudocode but missing several conditions and
gaps in logic
6) Aspects related to manageability considerations for quotas (which might
require provisioning support on routers?) and the requirements on operators
for their management seem under specified

Please find below my comments in the idnits output of v23 of this document.
The end of the review is marked by the tag <EoRv23>

Thanks,
Ketan

2 IDR Working Group                                                W. Wang
3 Internet-Draft                                                   A. Wang
4 Intended status: Experimental                              China Telecom

<major> The document is missing the rationale for why it is to be
published
on the experimental track as opposed to the proposed standard track.  There
is Appendix A that is describing the experimental topology. I assume that
this
draft is describing an experiment that is to be carried out. Please correct
me
if I am wrong. If it is describing an experiment, then I suggest that
Appendix A
be more generalized to describing the experiment, its goals/motivation, how
it
is desired to be conducted, success criteria, etc. Then, there can be a
sentence
at the end of the introduction section which points to this appendix.

16 Abstract

18   This draft defines a new type of Outbound Route Filter (ORF), known

<major> s/a new type/an experimental type ... similar in introduction

19   as the Virtual Private Network (VPN) Prefix ORF.  The VPN Prefix ORF
20   mechanism is applicable when VPN routes from different Virtual
21   Routing and Forwardings (VRFs) are exchanged through a single shared

<minor> s/Forwardings/Forwarding instances ?

22   Border Gateway Protocol (BGP) session.

<major> The abstract should also mention the purpose of this experimental
ORF
type.

94   which consequently affects the route processing performance of other
95   normal VRFs (such as route dropping, processing delays, and abnormal

<minor> what is "normal" here? Suggest omitting that word.

96   customer services).  That is to say, the excessive VPN routes
97   advertisement SHOULD be controlled individually for each VRF in such
98   shared BGP session.

<major> This looks like an incorrect usage of a BCP14 keyword. How about:

Therefore, it is desirable that the excessive VPN routes advertisement be
controlled individually for each VRF in such a shared BGP session.

114   However, there are limitations to existing solutions:

<major-editorial> Suggest to introduce a new top-level section called
"Existing
Solutions" and in there create sub-sections where each of the solution is
identified (the bullet list above) along with its limitation (the text under
each numbered item below). This section can be just before the new
mechanism is
described (i.e., current section 4). I believe this would improve the
readability
of this document.

116   1) Route Target Constraint

118   RTC can only filter the VPN routes from any uninterested VRFs, if the
119   "offending routes (prefixes)" come from an interested VRF, the RTC
120   mechanism can't filter them.

<major> Suggest to rephrase to avoid use of "offending" throughout this
document.
How about "route overload" or something like that which describes the
nature of
these routes in a more technical manner? "Offending" is not a technical
term.

122   2) Address Prefix ORF

124   Using Address Prefix ORF to filter VPN routes requires a pre-
125   configuration, but it is impossible to know in advance which prefix
126   MAY exceed the predefined threshold.

<major> Incorrect use of BCP14 keyword. s/MAY/may

135   CP-ORF is applicable in Virtual Hub-and-Spoke[RFC7024] VPN and also
136   BGP/MPLS Ethernet VPN (EVPN)[RFC7432] networks, but its primary
137   function is to retrieve interested VPN prefixes and it cannot be used
138   to filter overwhelmed VPN prefixes dynamically.

<minor> s/overwhelmed/overload of ?

140   4) PE-CE edge peer Maximum Prefix
141   The BGP Maximum-Prefix feature is used to control how many prefixes
142   can be received from a neighbor.  By default, this feature allows a
143   router to bring down a peer when the number of received prefixes from

<major> This is a mischaracterization. Peer down is not the only solution.
You
can check various implementations from major vendors. You can also refer to
https://www.rfc-editor.org/rfc/rfc4271.html#section-6.7

   When the upper bound is reached, the
   speaker, under control of local configuration, either (a) discards
   new address prefixes from the neighbor (while maintaining the BGP
   connection with the neighbor), or (b) terminates the BGP connection
   with the neighbor.


147   from different VRFs will share the common fate.  If the number of VPN
148   routes of a certain VPN exceeds the configured Maximum-Prefix limit,
149   the BGP session will be shut down, which will affect the operation of
150   other VPN routes transmitted via this BGP session.

<major> There also seems to be a mischaracterization here. I had the
impression
that this option was about putting the limit on the PE towards the CE (which
is an eBGP IPv4/IPv6 unicast afi/safi session) and is for a specific
VRF/VPN.

152   5) Configuring the Maximum Prefix for each VRF on edge nodes

154   When a VRF overflows, it stops the import of routes.  Any additional
155   VPN routes are held into its Routing Information Base (RIB).

<major> This is implementation specific and may not always be the case.
Perhaps
fix this text to say "... some implementations may ...".

156   However, PEs still need to parse the incoming BGP messages.  This
157   will cost CPU cycles and further burden the overflowed PE.

<major-editorial> In continuation of a previous comment, the rest of this
section
below belongs in the introduction. You could also put a forward reference to
the new section where analysis of existing solutions is provided.

169   The purpose of this mechanism is to control the outage within the

<minor> perhaps s/outage/overload ?

176 2.  Conventions used in this document

178   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
179   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
180   document are to be interpreted as described in [RFC2119] .

<major> Please update to the latest BCP14 template and consider introducing
as a
section 1.1 "Requirements Language" or something like that.

190   *  BGP: Border Gateway Protocol, defined in [RFC4760]

<major> It should be RFC4271

215 4.  The general procedures of VPN Prefix ORF mechanism

<major-editorial> I see an issue with the organization of sections 4, 7, 8,
and
10. First, there are the protocol procedures - Tx side how ORF entries are
triggered, encoded and sent, Rx side how ORF entries are handled and
corresponding actions taken for the route-refresh processing and subsequent
propagation of the VPN routes. Second, there are deployment considerations,
same/unique RD, RT usage, intra-AS, inter-AS, etc. - those can be captured
in its own section. Finally, there are the operational considerations which
is what the operator needs to bear in mind on how to provision (e.g.,
quotas),
design, manage (manual clearing of ORF entries), and monitor (looking at
alerts).

289   For intra-AS VPN deployment, there are three scenarios:

291   *  RD is allocated per VPN per PE, each VRF only import one RT (see
292      Section 4.1.1).

294   *  RD is allocated per VPN per PE.  Multiple RTs are associated with
295      such VPN routes, and are imported into different VRFs in other
296      devices(see Section 4.1.2).

298   *  RD is allocated per VPN, each VRF imports one/multiple RTs (see
299      Section 4.1.3).

<minor> Can this be simplified to say that there are 2 main RD allocation
schemes - unique RD (per VPN, per PE) and the same RD (per VPN, same on all
PEs)?

304 4.1.1.  Scenario-1 and Solution (Unique RD, One RT)

<major> Sections 4.1.1, 4.1.2 and 4.1.3 are all examples but contain
normative
text with BCP14 languages. This is not appropriate. Normative text on
procedures
need to stand on their own. The examples can be described in an informal
language
and moved into the appendix. When working on the normative text for all 3
of these
sections, I am hoping that you would see the commonalities and that there
can be
a single normative procedure that is independent of the RD and RT design.
After
all, in the BGP code, I am assuming there won't be any checks on whether
the
deployment design is using same or unique RD and how RTs are used?

339   If quota value is not set on PE1, and each VRF has a prefix limit on

<major> What is this quota value? It is used in several places in this
document
but not formally defined (closest match is in the operational
considerations).
Perhaps this can be added in the Terminology section? Then, this quota
mechanism
needs to be specified in this document along with its
provisioning/manageability
requirements - perhaps as its own separate section and before this quota
starts
to get used in the procedures section.

607 5.  Source PE Extended Community

609   We usually use next hop to identify the source, but it MAY NOT be
610   useful in the following scenarios:

<major> MAY NOT is not a valid BCP14 keyword. Perhaps s/it MAY NOT be/it is
not
Also, that claim/statement is incorrect - perhaps say ... Next hop does not
always identify the source ...?

627   The AS number of source PE can be conveyed by Source AS Extended
628   Community, as defined in [RFC6514]

<major> Is the Source AS EC always required to be included along with the
Source PE EC? Or is it only required in multi-AS deployments where BGP RIDs
are
not unique?

660   The SPE EC SHOULD be attached by source PE, or else the RR SHOULD
661   attach it, with the value set as the router-id of source PE.  When
662   none of them attach the SPE EC, the ASBR SHOULD attach it when the
663   packet leaves the source AS, with the value set as the ORIGINATOR_ID.

<minor> The above paragraph is duplicating the bullet list above it. Please
consider consolidating both?

665   This section updates route reflection procedures, which means
666   [RFC4456] needs to be updated.

<major> I don't think the above statement is correct. If you agree, please
remove
it. Please let me know if I am missing something and if you really want
this
document to update that standards track RFC.

668 6.  VPN Prefix ORF Encoding

<major-editorial> I would recommend that this section (as well section 5)
before
section 4 so the reader has seen and understood the ORF type before they
start
reading the procedures. Also, this will avoid repetition of the same
information
(e.g., take the default ORF entry) that is repeated in both the sections.

670   In this section, we defined a new ORF type called VPN Prefix Outbound
671   Route Filter (VPN Prefix ORF).  The ORF entries are carried in the
672   BGP ROUTE-REFRESH message as defined in [RFC5291].  A BGP ROUTE-
673   REFRESH message can carry one or more ORF entries.  The ROUTE-REFRESH
674   message which carries ORF entries contains the following fields:

<minor> Can you please rephrase the above to indicate that none of this is
new
and the description below is how the VPN Prefix ORF is encoded in the
refresh
message format of RFC5291.

676   *  AFI (2 octets)

678   *  SAFI (1 octet)

<minor> Some of the settings of these fields are repeated further below in
this
section itself. Please update that text here so that everything is in one
place.


686   A VPN Prefix ORF entry contains a common part and type-specific part.
687   The common part is encoded as follows:

689   *  Action (2 bits): the value is ADD, REMOVE or REMOVE-ALL

691   *  Match (1 bit): the value is PERMIT or DENY

693   *  Offending VPN routes process method (1 bit): if the value is set
694      to 0, it means all offending VPN routes on the sender of VPN
695      Prefix ORF message SHOULD be withdrawn; if the value is set to 1,
696      it means the sender of VPN Prefix ORF message refuse to receive
697      new offending VPN routes.  The default value is 0.

<major> Looks like this document is allocating one of the reserved bits from
the common part of the ORF container that would be applicable not just for
this
new ORF type but all ORF types. Now RFC5291 did not create an IANA registry
for these reserved bits, but perhaps there is now a need to create an IANA
registry for this 8-bit field to perform this allocation. This is going to
take
some work and might be challenging given the experimental status of this
document. Another option is to do this via signaling in the type-specific
portion below since I don't see the technical merit of burning a common
bit for something type-specific. The authors and WG will need to decide
this.


699   *  Reserved (4 bits)

<minor> It would be very helpful if there was a figure showing all of the
above
fields just like the Figure 5 below.


762   *  If the AFI is set to L2VPN, the SAFI MUST be set to BGP EVPN.

<major> The document does not specify which EVPN Route Types this ORF type
is
applicable for.


782   Source PE TLV is defined to identify the source of the VPN routes.
783   For the sender of VPN Prefix ORF, it will check the existence of SPE
784   EC.  If it exists, the sender will put it into Source PE TLV.

<minor> Perhaps ... check the existence of SPE EC on the VPN route being
matched.


788   The Source PE TLV SHOULD only appear once within an individual ORF
789   entry.  If one ORF entry contains multiple Source PE TLVs, it SHOULD
790   be ignored.

<major> all should be ignored? first considered and rest ignored?

792   The source PE TLV contains the following types:

<major> What is meant by "contains" here? Are these sub-TLVs? I assume
there are
3 types of TLVs used for identifying the "source". If so, please clarify
this
entire section starting with the title.

794   *  IPv4 Source PE TLV: Type = 1 (suggested), Length = 4 octets, value
795      = next hop address in IPv4 format.

<minor> Since this is a new TLV space, there is no need to say "suggested"
next
to each entry. This applies to all the TLVs and also in the IANA
considerations.

797   *  IPv6 Source PE TLV: Type = 2 (suggested), Length = 16 octets,
798      value = next hop address in IPv6 format.

<major> Only global IPv6 addresses allowed?

800   *  Source PE identifier TLV: Type = 3 (suggested), Length = 4 octets,
801      value = the value of ORIGINATOR_ID in Source PE Extended
802      Community.

<question> At this point, the document is experimental. I don't see any
implementation reporting support for any of the Source PE TLVs. And I
already
find 3 types of TLVs! Seems complicated to me. Why not just have the
Source PE identifier TLV alone in this experiment? If/when this becomes
standard
track, you may add the other types if they are really determined to be
necessary?

818 6.3.  Route Target TLV

820   Route Target TLV is defined to identify the RT of the offending VPN
821   routes.  RT and RD can be used together to filter VPN routes when the
822   source VRF contains multiple RTs, and the VPN routes with different
823   RTs MAY be assigned to different VRFs on the receiver.  The Route
824   Target TLV contains the following types:

826      Type = 5 (suggested), Length = 8*n (n is the number of RTs that
827      the offending VPN routes attached) octets, value = the RT of the
828      offending VPN routes.  If multiple RTs are included, there MUST be
829      an exact match.

<major> And if only one RT is present in this TLV and there are multiple
RTs on
the VPN route? Please clarify.

831 7.  Operation process of VPN Prefix ORF mechanism on receiver

<major> This isn't an operational process but more like a protocol procedure
on the router receiving these ORF entries. The title is misleading. More
importantly, what are the similar procedures on the sending side?

833   The VPN Prefix ORF is used mainly to block the unwanted BGP updates.
834   When the receiver receives VPN Prefix ORF entry, it SHOULD check
835   first whether the "Match" bit is "DENY" or not.

837   If the "Match" bit is "PERMIT", and is the "default" entry (the
838   offending VPN routes process method equal to 0, sequence equal to
839   0xFFFFFFFF, length is equal to 8, and Route Distinguisher is equal to
840   0), the entry SHOULD be installed.  Otherwise, if the "Match" bit is

<major> Why SHOULD and not MUST? Isn't this essential?

841   "PERMIT", the entry SHOULD be discarded and a warning SHOULD be sent
842   to the operator.

<major> Why SHOULD and not MUST?

844   The following procedures will only be evaluated when the "Match" bit
845   is "DENY".

847   The receiver of VPN Prefix ORF entries, which MAY be a RR, ASBR or

<major> s/MAY/may

848   PE, when receives VPN Prefix ORF entry from its BGP peer, it does the
849   following:

851   S01. The receiver checks the combination of <AFI/SAFI, ORF-Type,
852        Sequence, Route Distinguisher> of the received VPN Prefix
853        ORF entry.
854   S02. If (the combination does not already exist in the ORF-Policy
855        table) {
856   S03.     The receiver adds the VPN Prefix ORF entry to the
857            ORF-Policy table.

<major> This seems odd - what if the action was REMOVE?

858   S04. } else {
859   S05.     If (Action is ADD) {
860   S06.         Overwrite the old VPN Prefix ORF entry with the new
861                one.
862   S07.     else {
863                   Remove the corresponding VPN Prefix ORF entry.

<major> What about REMOVE-ALL handling?

866   The filtering conditions for the stored VPN Prefix ORF entries
867   contain the RD and RT of the source PE.

<major> You mean the contents of the TLVs are stored? If so, please state it
that way.

869   If the SPE EC is not attached to the BGP Update message of the VPN
870   prefixes, the receiver SHOULD use NEXT_HOP or ORIGINATOR_ID as the

<major> Why not MUST?

873   After installing the filter entries for the outbound VPN prefixes,
874   the RR or ASBR does the following before sending VPN routes:

<major> It is not clear if the steps below are related to the route refresh
processing after getting the ORF update, or the usual VPN route propagation,
or both. The processing is different in those cases and so please clarify.

896 8.  Withdraw of VPN Prefix ORF entries

<major> Is "withdraw" the same as REMOVE or REMOVE-ALL? I am very confused.
Seems like this entire section needs to go into the operational
considerations
section. This has nothing to do with protocol procedures.

931 9.  Applicability

<major> This is not applicability. Feels more like examples to me. However,
there
is some normative text in this section which is confusing. For all
normative
aspects, please move the text into the sections where either the encoding
or
procedures are specified. If these examples need to be retained, please
move them
into an informative appendix section.

1004 10.  Operational Considerations

1006 10.1.  Quota value calculation

1008   Quota is a threshold to limit the number of VPN routes under specific
1009   granularities (such as <PE>, <RD, Source AS>).

<major> It is not very clear if the operator specifying these quotas via
their
NMS is a prerequisite for this feature to work. There is a need for an
applicability section in this document (just before the encodings and
procedures)
that describes where this feature can be deployed, for which types of VPNs,
what are the things that operators need to do (e.g., this quota
provisioning), and
any other requirements and limitations.

1029 11.  Implementation Consideration

<major-editorial> This section does not contain any implementation
considerations.
Please remove it. The implementation status should be captured in the report
on the IDR wiki as is the WG norm. The Experimental topology can be moved
into
the Appendix section describing the experiment.

1068 13.  IANA Considerations

<minor> There are 3 actions below. Suggest to create 3 sub-sections and in
each
list precisely the IANA actions requested or done.

1073   We would want to refer to the text from [RFC5291]: This new ORF is
1074   exchanged using outbound route filtering capability defined in
1075   [RFC5291] (for the sake of completeness).

1077   under "BGP Outbound Route Filtering (ORF) Types"
1078   Registry: "VPN Prefix Outbound Route Filter (VPN Prefix ORF)"
1079   Registration Procedure(s): First Come, First Served
1080   Value: 66

<major> I am not able to follow the above two paragraphs. The allocation
for value
66 for the new type has already been done by IANA. Simply state that. Next,
it seems like a request for creation of a new IANA registry - please say
that clearly.


1095    +=====================+=============+===========================+
1096    | Registry            |     Type    |       Meaning             |
1097    +=====================+=============+===========================+
1098    |Reserved             | 0(suggested)|Reserved                   |
1099    +---------------------+-------------+---------------------------+
1100    |IPv4 Source PE TLV   | 1(suggested)|IPv4 address for source PE.|
1101    +---------------------+-------------+---------------------------+
1102    |IPv6 Source PE TLV   | 2(suggested)|IPv6 address for source PE.|
1103    +---------------------+-------------+---------------------------+
1104    |Source PE Identifier | 3(suggested)|ORIGINATOR_ID in Source PE |
1105    |TLV                  |             |Extended Community for     |
1106    |                     |             |source PE                  |
1107    +---------------------+-------------+---------------------------+
1108    |Source AS TLV        | 4(suggested)|Source AS for source PE    |
1109    +---------------------+-------------+---------------------------+
1110    |Route Target TLV     | 5(suggested)|Route Target of the        |
1111    |                     |             |offending VPN routes       |
1112    +---------------------+-------------+---------------------------+

<major> The above table has initial allocations which can be done straight
away.
So the "suggested" is not required. I don't think the "meaning" column is
necessary. What is required is the reference column that points to this
document.

1114   This document also requests a new Transitive Extended Community Type.
1115   The new Transitive Extended Community Type name SHALL be "Source PE
1116   Extended Community".

1118           Under "BGP Transitive Extended Community Types:"
1119           Registry: "Source PE Extended Community" type
1120            0x0d(suggested)         Source PE Extended Community

<major> The above is not clear to me - exactly which registry the
allocation is to
be made under and under which registry-group. You cannot make suggestions
here.
If required, allocations can be done with IANA under FCFS - what is being
done
here is problematic (squatting on code points?).

<EoRv23>