Re: AD-review comments on draft-ietf-idr-bgp4-20

[Yakov Rekhter <[e-mail]>]

Alex,

> Folks,
> 
>  Please find below my AD-review comments. Hopefully they will help
>  improve the document. I tried to consult Andrew's list as much as
>  possible, but do feel free to point out if something has already been
>  discussed and agreed upon.
>  
>  Thanks go to Yakov for kicking me often enough ;)
> --
> Alex Zinin
> 
> Some nits:
> - run it by a spelling checker, please
> - disable hyphenation if possible
> - include boilerplates for IPR notice, Copyright notice

Sure.

> 
> General comment:
> 
>   in some places I highlighted the fact that required behavior is not
>   described using the 2119 language, so it is not clear if a MUST or
>   SHOULD or MAY is applicable. I am sure I've missed some more places
>   like this. I'd like to ask the editors to go through the doc and
>   check this.

Sure.

> > Status of this Memo
> > 
> > 
> ...
> >    The list of Internet-Draft Shadow Directories can be accessed at
> >    http://www.ietf.org/shadow.html.
> >
> > Specification of Requirements
> 
> Nit: move Abstract here. Move requirements after the Acks.

Ok.

> > Abstract
> 
> Should the Abstract say that this spec covers IPv4 only?

Sure.

> > 3. Summary of Operation
> ...
> >    This document uses the term `Autonomous System' (AS) throughout.  The
> >    classic definition of an Autonomous System is a set of routers under
> >    a single technical administration, using an interior gateway protocol
> >    (IGP) and common metrics to determine how to route packets within the
> >    AS, and using an inter-AS routing protocol to determine how to route
> >    packets to other ASs. Since this classic definition was developed, it
> >    has become common for a single AS to use several IGPs and sometimes
> >    several sets of metrics within an AS. The use of the term Autonomous
> >    System here stresses the fact that, even when multiple IGPs and met-
> >    rics are used, the administration of an AS appears to other ASs to
> >    have a single coherent interior routing plan and presents a consis-
> >    tent picture of what destinations are reachable through it.
> 
> Ed: Since 'AS' has been defined before, do we need to repeat the
> definition here?

The definition section before presents a *summary* of the definitions
used in the document. I think that the text reads fine as is, so
I would prefer not to change it.

> ...
> >    peer in the same AS is referred to as an internal peer. Internal BGP
> >    and external BGP are commonly abbreviated IBGP and EBGP.
> 
> Ed: These two have been defined before too

See my previous comment.

> ...
> > Care must be taken to
> >    ensure that the interior routers have all been updated with transit
> >    information before the BGP speakers announce to other ASs that tran-
> >    sit service is being provided.
> 
> What does the last sentence really mean from the implementation
> perspective? It used to mean the BGP/IGP synchronization check. Now
> that iBGP everywhere is assumed, how do we check this condition?

In the absence of any objections by June 10 I suggest to take this 
sentence out.

> >    This document specifies the base behavior of the BGP protocol. This
> >    behavior can and is modified by extention specifications.  When the
> Ed: "extension"

Sure.

> >    protocol is extended the new behavior is fully documented in the
> >    extention specifications.
> Ed: "extension"

Sure.

> 
> > 3.1 Routes: Advertisement and Storage
> > 
> >    For the purpose of this protocol, a route is defined as a unit of
> >    information that pairs a set of destinations with the attributes of a
> >    path to those destinations. The set of destinations are systems whose
> >    IP addresses are contained in one IP address prefix carried in the
> >    Network Layer Reachability Information (NLRI) field of an UPDATE mes-
> >    sage, and the path is the information reported in the path attributes
> >    field of the same UPDATE message.
> Ed: Repeated definition again

See above.

> ...
> >    If a BGP speaker chooses to advertise the route, it MAY add to or
> >    modify the path attributes of the route before advertising it to a
> >    peer.
> 
> The intent here is to say that it's ok to modify the attribute set of
> a previously received route when it's announced further. The way it
> reads though is that self-originated routes are also within the
> context and MAY sounds like you don't have to add attributes when
> announcing those.

I will replace "If a BGP speaker chooses to advertise the route" with
"If a BGP speaker chooses to advertise a previously received route".

> 
> ...
> 
> >    Changing attribute of a route is accomplished by advertising a
> >    replacement route. The replacement route carries new (changed)
> >    attributes and has the same NLRI as the original route.
> 
> "same NLRI" implies the same prefix, but not the NLRI field, which can
> be different (containing other routes), should the use of this term be
> normalized throughout the document?

I will replace "the same NLRI" with "the same address prefix".

> 
> > 4.2 OPEN Message Format
> > 
> >    After a TCP is established, the first message sent by each side is an
> 
> "TCP connection"

ok.

> > 5. Path Attributes
> ...
> >    If a path with recognized transitive optional attribute is accepted
> >    and passed along to other BGP peers and the Partial bit in the
> >    Attribute Flags octet is set to 1 by some previous AS, it is not 
> 
> 'MUST NOT' here?

Sure.

> > set
> >    back to 0 by the current AS. Unrecognized non-transitive optional
> >    attributes MUST be quietly ignored and not passed along to other BGP
> >    peers.
> ...
> >    The same attribute (attribute with the same type) can not appear more
> >    than once within the Path Attributes field of a particular UPDATE
> >    message.
> 
> What should an implementation do if this happens?

See section 6.3:

  If any attribute appears more than once in the UPDATE message, then
  the Error Subcode is set to Malformed Attribute List.

  
> >    The mandatory category refers to an attribute which MUST be present
> >    in both IBGP and EBGP exchanges if NLRI are contained in the UPDATE
> 
> Ed: "if the NLRI field is contained" instead?

No, as the NLRI field is always present in the UPDATE message
(although if NLRI is not present, then the NLRI field is empty).

> 
> > 5.1.2 AS_PATH
> ...
> >       b) When a given BGP speaker advertises the route to an external
> >       peer, then the advertising speaker updates the AS_PATH attribute
> >       as follows:
> > 
> >          1) if the first path segment of the AS_PATH is of type
> >          AS_SEQUENCE, the local system prepends its own AS number as the
> >          last element of the sequence (put it in the leftmost position).
> 
> 'Leftmost position'... isn't this still open for interpretation? How
> about wording this relative to the position of the octets in the
> protocol message?

I'll replace "the leftmost position" with "the leftmost position with
respect to the position of octets in the protocol message".

> >          If the act of prepending will cause an overflow in the AS_PATH
> >          segment, i.e. more than 255 ASs, it is legal to prepend a new
> >          segment of type AS_SEQUENCE and prepend its own AS number to
> >          this new segment.
> 
> What's the recommended behavior here?

"it is legal to prepend" really means "it SHOULD prepend". 
In the absence of any objections by June 10 I'll update the text.

> 
> 
> > 5.1.4 MULTI_EXIT_DISC
> > 
> > 
> >    The MULTI_EXIT_DISC is an optional non-transitive attribute which is
> >    intended to be used on external (inter-AS) links to discriminate
> >    among multiple exit or entry points to the same neighboring AS.  The
> >    value of the MULTI_EXIT_DISC attribute is a four octet unsigned num-
> >    ber which is called a metric. All other factors being equal, the exit
> >    point with lower metric SHOULD be preferred. If received over EBGP,
> >    the MULTI_EXIT_DISC attribute MAY be propagated over IBGP to other
> >    BGP speakers within the same AS. The MULTI_EXIT_DISC attribute
> 
> seems that a reference to 9.1.2.2 is due here, as using MED in local
> route calculation and not propagating it further is dangerous

Sure.

> >    received from a neighboring AS MUST NOT be propagated to other neigh-
> >    boring ASs.
> > 
> >    A BGP speaker MUST IMPLEMENT a mechanism based on local configuration
>                         ^^^^^^^^^lower-case

Sure.

> >    which allows the MULTI_EXIT_DISC attribute to be removed from a
> >    route. This MAY be done prior to determining the degree of preference
> 
> what's the recommended behavior here?

What the text is saying is that a BGP speaker optionally (MAY)
remove MED from a route. If the speaker does this, then this *has
to* happen prior to determining the degree of preference for the
route. So, what "This MAY" refers to is the fact that removing MED
is optional. To clarify I would replace "This MAY be done" with
"Removal of the MULTI_EXIT_DISC attribute MAY be done".

> >    of the route and performing route selection (decision process phases
> >    1 and 2).
> > 
> >    An implementation MAY also (based on local configuration) alter the
> >    value of the MULTI_EXIT_DISC attribute received over EBGP.  This MAY
> >    be done prior to determining the degree of preference of the route
> 
> what's the recommended behavior here?

The same as the previous comment.

> > 5.1.5 LOCAL_PREF
> ...
> > A BGP speaker SHALL calculate the degree of preference for
> >    each external route based on the locally configured policy, and
> 
> Should we be more honest here and say that the implementation must
> allow the admin to SET the degree of preference through the local
> policy to influence the best-path selection process, i.e., I don't
> think any implementation really *calculates* it.

Please see my answer to you comment on 9.1.1

> > 5.1.6 ATOMIC_AGGREGATE
> ...
> >    A BGP speaker that receives a route with the ATOMIC_AGGREGATE
> >    attribute MUST NOT make any NLRI of that route more specific (as
> >    defined in 9.1.4) when advertising this route to other BGP speakers.
> 
> Since deaggregation is not described in this document, do we need this
> para?

I would prefer to keep the current text, as to make sure that an
implementation wouldn't do deaggregation.

> >   A BGP speaker that receives a route with the ATOMIC_AGGREGATE
> >    attribute needs to be cognizant of the fact that the actual path to
> >    destinations, as specified in the NLRI of the route, while having the
> >    loop-free property, may not be the path specified in the AS_PATH
> >    attribute of the route.
> 
> What does this really mean from the implementation perspective?

This is mostly FYI. It has to do with the user of BGP...

> > 5.1.7 AGGREGATOR
> > 
> > 
> >    AGGREGATOR is an optional transitive attribute which MAY be included
> >    in updates which are formed by aggregation (see Section 9.2.2.2). A
> >    BGP speaker which performs route aggregation MAY add the AGGREGATOR
> 
> What's the recommended behavior here? Include or not, and under what
> circumstances?

The spec doesn't provide any recommendation on this, as it is optional.

> > 6. BGP Error Handling.
> ...
> >    The phrase "the BGP connection is closed" means that the TCP connec-
> >    tion has been closed, the associated Adj-RIB-In has been cleared, and
> >    that all resources for that BGP connection have been deallocated.
> >    Entries in the Loc-RIB associated with the remote peer are marked as
> >    invalid. The fact that the routes have become invalid is passed to
> >    other BGP peers before the routes are deleted from the system.
> 
> What does "the fact is passed" mean? Should we instead say that local
> route recalculation happens and peers are sent either updated best
> routes or withdrawals?

How about the following replacement for the last sentence:

   The local system recalculates its best routes for the destinations
   of the routes marked as invalid, and advertises to its peers either
   withdraws for the routes marked as invalid, or the new best routes
   before the invalid routes are deleted from the system.

> > 6.2 OPEN message error handling.
> ...
> >    If the Autonomous System field of the OPEN message is unacceptable,
> >    then the Error Subcode is set to Bad Peer AS. The determination of
> >    acceptable Autonomous System numbers is outside the scope of this
> >    protocol.
> 
> Shouldn't we say that configuration based detection should be
> supported, i.e., when remote-as is configured for the peer?

No.

> ...
> >   If the BGP Identifier field of the OPEN message is syntactically
> >    incorrect, then the Error Subcode is set to Bad BGP Identifier.  Syn-
> >    tactic correctness means that the BGP Identifier field represents a
> >    valid IP host address.
> 
> Is "valid IP host address" defined somewhere, btw?

Certainly not in this document. Perhaps for clarity I'll add
"unicast" in front of "IP host address".

> > 6.3 UPDATE message error handling.
> > 
> > 
> >    All errors detected while processing the UPDATE message are indicated
> >    by sending the NOTIFICATION message with Error Code UPDATE Message
> >    Error. The error subcode elaborates on the specific nature of the
> >    error.
> 
> "are indicated..." is this a MUST, SHOULD, or MAY?

MUST.

> ...
> >    If the ORIGIN attribute has an undefined value, then the Error Sub-
> >    code is set to Invalid Origin Attribute. The Data field contains the
> >    unrecognized attribute (type, length and value).
> 
> Curious: do we really have to drop a session on this condition? Given
> that the attribute was syntactically correct and the TLV was not
> broken, so the stream is still in sync and we can move on? Of course,
> if this is what current implementations do, we have no other choice.

In the current spec all the errors are fatal. Including errors
in the ORIGIN attribute.
  
> ...
> >    If the UPDATE message is received from an external peer, the local
> >    system MAY check whether the leftmost AS in the AS_PATH attribute is
> 
> Same comment about 'leftmost'... Maybe we should define this somewhere
> in the beginning of the spec?

I will replace "the leftmost AS" with "the leftmost AS with
respect to the position of octets in the protocol message".
  
> ...
> >    The NLRI field in the UPDATE message is checked for syntactic valid-
> >    ity. If the field is syntactically incorrect, then the Error Subcode
> >    is set to Invalid Network Field.
> 
> Should we give more data on what syntactic validity means in this case
> so people behave consistently?

As Curtis suggested a while ago:

     If the document is unclear to the well qualified reader (one
     possessing a thorough understanding of foundations of this work,
     including IP routing, TCP, TCP programming, and the referenced
     documents) then the document may need to be changed to improve
     clarity.

The case you mentioned above suggests that the reader is not
well qualified.

> > 6.7 Cease.
> ...
> > If the BGP speaker decides to terminate its BGP
> >    connection with a neighbor because the number of address prefixes
> >    received from the neighbor exceeds the locally configured upper
> >    bound, then the speaker MUST send to the neighbor a NOTIFICATION mes-
> >    sage with the Error Code Cease.
> 
> Should we also say that when the peer decides to discard incoming
> prefixes, this event should be logged locally?

In the absence of any objections by June 10 I'll add the following to 
the text:

    The speaker MAY also log this locally.

> > 9. UPDATE Message Handling
> > 
> > 
> >    An UPDATE message may be received only in the Established state.
>
> What if it is received in another state?

It is an error. To make this clear I'll add the following to the
text:

   Receiving an UPDATE message in any other state is an error.

> ...
> > 9.1 Decision Process
> > 
> > 
> >    The Decision Process selects routes for subsequent advertisement by
> >    applying the policies in the local Policy Information Base (PIB) to
> >    the routes stored in its Adj-RIBs-In. The output of the Decision Pro-
> >    cess is the set of routes that will be advertised to peers; the
> >    selected routes will be stored in the local speaker's Adj-RIB-Out
> RIB-Out or RIBs-out (plural)?

Plural.

> >    according to policy.
> > 
> >    The selection process is formalized by defining a function that takes
> >    the attribute of a given route as an argument and returns either (a)
> >    a non-negative integer denoting the degree of preference for the
> >    route, or (b) a value denoting that this route is ineligible to be
> >    installed in LocRib and will be excluded from the next phase of route
> 
> Loc-RIB

Ok.

> >    selection.
> ...
> >    The Decision Process operates on routes contained in the Adj-RIB-In,
> Adj-RIBs-In (plural) ?

Plural.

> >    and is responsible for:
> 
> > 9.1.1 Phase 1: Calculation of Degree of Preference
> ...
> >       If the route is learned from an external peer, then the local BGP
> >       speaker computes the degree of preference based on preconfigured
> >       policy information. If the return value indicates that the route
> >       is ineligible, the route MAY NOT serve as an input to the next
> >       phase of route selection; otherwise the return value is used as
> >       the LOCAL_PREF value in any IBGP readvertisement.
> 
> So, AFAIK, the major implementations do not follow this step
> (calculating the degree of preference, and then announcing). Instead,
> implementations allow setting the LOCAL_PREF value locally, which is
> taken into consideration during the best path selection, and is also
> reannounced further.

It is important to keep in mind that the whole section on the BGP
decision process does *not* mean that an implementation must implement
it precisely as it is described in the spec, as long as the implementation 
support the described functionality and its externally visible behavior 
is the same. With this in mind how about if I'll add the following:

   The BGP Decision Process in this document is conceptual and do
   not have to be implemented precisely as described here, as long
   as the implementations support the described functionality and
   their externally visible behavior is the same.

> Also "is used" is not specific enough. Is it SHOULD or MUST?

MUST.

> > 9.1.2 Phase 2: Route Selection
> ...
> >    If the AS_PATH attribute of a BGP route contains an AS loop, the BGP
> >    route should be excluded from the Phase 2 decision function.  AS loop
> >    detection is done by scanning the full AS path (as specified in the
> >    AS_PATH attribute), and checking that the autonomous system number of
> >    the local system does not appear in the AS path.  Operations of a BGP
> >    speaker that is configured to accept routes with its own autonomous
> >    system number in the AS path are outside the scope of this document.
> 
> If we're checking for an AS loop here (in Phase 2) as opposed to
> during the UPDATE message sanity checking, the route is already
> received and accepted in the peer's Adj-RIB-In. Those implementations
> I know don't even install such routes in the RIB...

This is the text that the WG agreed on (see e-mail from John Scudder on
Mon, 02 Dec 2002 10:54:45 EST). Also, see my response to your previous
comment.

> > 9.1.2.2 Breaking Ties (Phase 2)
> ...
> >       Similarly, neighborAS(n) is a function which returns the neighbor
> >       AS from which the route was received.  If the route is learned via
> >       IBGP, and the other IBGP speaker didn't originate the route, it is
> >       the neighbor AS from which the other IBGP speaker learned the
> >       route. If the route is learned via IBGP, and the other IBGP
> >       speaker originated the route, it is the local AS.
> 
> What if the route is locally originated?

Breaking ties has to do with the routes received from other BGP speakers,
not with the routes locally originated.

> > 9.1.4 Overlapping Routes
> ...
> >    When overlapping routes are present in the same Adj-RIB-In, the more
> >    specific route takes precedence, in order from more specific to least
> >    specific.
> > 
> Doesn't this happen at the packet forwarding stage?

Yes, it does. But only if both routes are present in the FIB.
I also think that this sentence isn't needed, so in the absence
of any objections by June 10 I propose to remove it.

> >    The set of destinations described by the overlap represents a portion
> >    of the less specific route that is feasible, but is not currently in
> >    use.  If a more specific route is later withdrawn, the set of desti-
> >    nations described by the overlap will still be reachable using the
> >    less specific route.
> > 
> >    If a BGP speaker receives overlapping routes, the Decision Process
> >    MUST consider both routes based on the configured acceptance policy.
> >    If both a less and a more specific route are accepted, then the Deci-
> >    sion Process MUST either install both the less and the more specific
>   
> Install where?

In Loc-RIB. I'll insert "in Loc-RIB" to make this clear.

> >    routes or it MUST aggregate the two routes and install the aggregated
> >    route, provided that both routes have the same value of the NEXT_HOP
> >    attribute.
> 
> anyone really does the latter?

Will find this from the implemenation report.

> >    If a BGP speaker chooses to aggregate, then it SHOULD either include
> >    all AS used to form the aggreagate in an AS_SET or add the
> >    ATOMIC_AGGREGATE attribute to the route.  This attribute is now pri-
> >    marily informational.  With the elimination of IP routing protocols
> >    that do not support classless routing and the elimination of router
> >    and host implementations that do not support classless routing, there
> >    is no longer a need to deaggregate.  Routes SHOULD NOT be de-aggre-
> >    gated.  A route that carries ATOMIC_AGGREGATE attribute in particular
> >    MUST NOT be de-aggregated. That is, the NLRI of this route can not be
> >    made more specific. Forwarding along such a route does not guarantee
> >    that IP packets will actually traverse only ASs listed in the AS_PATH
> >    attribute of the route.
> 
> Since we don't do deaggregation any more, should we remove the
> discussion about it completely and indicate in the "changes" section
> that deaggregation has been deprecated?

As I said before, I would prefer to keep the text on de-aggregation in.

> > 9.2 Update-Send Process
> ...
> >    When a BGP speaker receives an UPDATE message from an internal peer,
> >    the receiving BGP speaker SHALL NOT re-distribute the routing infor-
> >    mation contained in that UPDATE message to other internal peers,
> >    unless the speaker acts as a BGP Route Reflector [RFC2796].
> 
> Suggest to put "unless..." in brackets () to make it more apparent
> that this is not a normative ref.

Ok.

> > 9.2.1.1 Frequency of Route Advertisement
> >    Since fast convergence is needed within an autonomous system, either
> >    (a) the MinRouteAdvertisementInterval used for internal peers SHOULD
> >    be shorter than the MinRouteAdvertisementInterval used for external
> >    peers, or (b) the procedure describe in this section SHOULD NOT apply
> >    for routes sent to internal peers.
> 
> It sounded like MinRouteAdvertisementInterval was an architectural
> constant, but now it sounds like either this is a timer that can be
> assigned different settings or there are two constants:
> MinRouteAdvIntIBGP and MinRouteAdvIntEBGP.

There is a timer (MinRouteAdvertisementInterval) that can be assigned 
different settings.
  
> > 9.2.2.2 Aggregating Routing Information
> > 
> 
> Hmmm... I expected to see in this section some text talking about when
> and how an aggregate would be announced, i.e., when an aggregate
> prefix is configured, and more specific routes are present, the
> aggregate is announced, when no specifics are left--withdraw the
> aggregate. I haven't found anything on this topic...

That is outside the scope of the *protocol* spec. See rfc1519 for
more on this.

> > 9.3 Route Selection Criteria
> >
> >    Generally speaking, additional rules for comparing routes among sev-
> >    eral alternatives are outside the scope of this document. There are
> >    two exceptions:
> > 
> >       - If the local AS appears in the AS path of the new route being
> >       considered, then that new route can not be viewed as better than
> >       any other route (provided that the speaker is configured to accept
> >       such routes). If such a route were ever used, a routing loop could
> >       result.
> > 
> >       - In order to achieve successful distributed operation, only
> >       routes with a likelihood of stability can be chosen. Thus, an AS
> >       SHOULD avoid using unstable routes, and it SHOULD NOT make rapid
> >       spontaneous changes to its choice of route. Quantifying the terms
> >       "unstable" and "rapid" in the previous sentence will require expe-
> >       rience, but the principle is clear.
> 
> Where does this (the second one) fit within and how does this affect
> the route selection criteria?

Routes that flap often can be "penalize" (e.g., route dampening).
I'll add a pointer to the route dampening spec here.
  
> >    Care must be taken to ensure that BGP speakers in the same AS do not
> >    make inconsistent decisions.
> 
> How? 

By means outside of the protocol. How about if I'll just remove this
sentence ?

> What does this mean for the implementor?
>
> > 9.4 Originating BGP routes
> > 
> >    A BGP speaker may originate BGP routes by injecting routing informa-
> >    tion acquired by some other means (e.g. via an IGP) into BGP. A BGP
> >    speaker that originates BGP routes assigns the degree of preference
> > 
> 
> "assigns the degree of preference"... how do the implementations
> really do that?

E.g., via CLI. I'll add "(e.g., via CLI") after "assigns the degree
of preference".
  
> > 10 BGP Timers
> ...
> >    The suggested default value for the MinRouteAdvertisementInterval is
> >    30 seconds.
> 
> This was described as a parameter, not a timer. Further, it was
> earlier suggested that it should be shorter for iBGP than it is for
> eBGP. I'd expect the document to specify the recommended value for
> both.

This is for eBGP. For iBGP the suggested value is 5 secs (I'll add this
to the draft).

> > IANA Considerations
> ...
> >    All extensions to this protocol, including new message types and Path
> >    Attributes MUST only be made using the Standards Action process
> >    defined in [RFC2434].
> 
> This section should include the description of each registry that
> needs to be created (if needed) and maintained by IANA, as well as the
> allocation policy that is in the text already.

Sure.

Yakov.