# Gunter Van de Velde, RTG AD, comments for draft-ietf-idr-nhc-06

# The line numbers used are rendered from IETF idnits tool: https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-idr-nhc-06.txt

# Many thanks Shepherd writeup from Sue Hares and the RTGDIR reviews from Mach Chen and Andrew Alston

# The document is well written, many thanks for your efforts on making this such a pleasure to read.

# COMMENTS
# ========

309	   Due to the nature of BGP optional transitive path attributes, any BGP
310	   speaker that does not implement this specification will propagate the
311	   NHC, the requirements of this section notwithstanding.  Such a
312	   speaker will not update the NHC, however.

GV> If NHC is implemented, does this imply that the functionality is automatically enabled by default?
Operators often prefer to explicitly enable new control-plane processing behavior rather than have it activated automatically. It may therefore be useful to clarify that an NHC speaker that does not support NHC, or has NHC support administratively disabled, will still propagate the NHC unchanged.

397	   When a BGP speaker receives a BGP route that includes the NHC, it
398	   MUST compare the address given in the header portion of the NHC and
399	   illustrated in Figure 1 to the next hop of the BGP route.  If the two

GV> In section 2.2.1 is mentioned that in some cases the NH is not globally unique. In such situation the BGPID TLV should be used i suspect? Should this type of processing added? and what if there is a non-matching NH but a matching BGPID TLV? (similar line of thought as with my next review comment)

492	3.  BGP Identifier Characteristic

GV> The text seems to keep the door slightly open to attach a BGPID characteristic even with a globally unique NH. It does not forbid doing this. However doing so, the NHC has two potential datapoints to verify if NHC is valid or not for the route entry. In such situation, must the receiving router only look at the matching NH, or only at matching BGPID, or both? or is that considered as incorrect NHC?

Many thanks for this document,

Kind Regards,
Gunter Van de Velde
Routing AD

Hi Bruno, Kireeti, Serge, Satya, John, Kevin, and Bin, 

Thank you for the effort put in this specification. It was an interesting read to go through all the one decade I-Ds branches that lead to this effort.

The document is well-written and well-articulated, although there is a mix of protocol specifications and operational considerations (incremental deployment, configuration knobs, when special care is needed from those who deploy, log abnormal behavior, etc.) but that’s fine as far as these are there.

Please find some comments, fwiw:

# Initial values

I suggest we clarify the following: 

## Add a mention that entries can be modified (deleted or content altered). This is to have a provision for some of these I-Ds to change the description if they want so or update the reference if any of these I-Ds make it to an RFC.

## The table does not track the document/event that led to a registration. For example, there is no visible information in the registry that indicates 1-2-4-5 are registered by this doc. Maybe add a note column to disclose that?

### Do we envisage in future having attributes that can be deprecated? If so, should that indication be supported by the registry (that is, add a status column)?  

# Mismatch

CURRENT:
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |   Address Family Identifier   |     SAFI      | Next Hop Len  |
      +-------------------------------+---------------+---------------+
      |                                                               |
      ~             Network Address of Next Hop (variable)            ~
      |                                                               |
      +---------------------------------------------------------------+
      |                                                               |
      ~                 Characteristic TLVs (variable)                ~
      |                                                               |
      +---------------------------------------------------------------+

                            Figure 1: NHC Format

   The meanings of the header fields (Address Family Identifier, SAFI or
   Subsequent Address Family Identifier, Length of Next Hop, and Network
   Address of Next Hop) are as given in Section 3 of [RFC4760].

## Mismatch between the figure and description for “Next Hop Len”.

## Please note that RFC4760 uses “Length of Next Hop Network Address”, not “Length of Next Hop” or “Next Hop Len”.

# Applicable only for NHC-aware routers

CURRENT:
   When a BGP speaker receives a BGP route that includes the NHC, it
   MUST compare the address given in the header portion of the NHC and
   illustrated in Figure 1 to the next hop of the BGP route.  

This seems to be assumed, but it is better to make it explicit this (and all Section 2.3) is discussing the behavior of NHC-aware routers.

# NHC Information 

CURRENT:
   Since the NHC is intended chiefly for conveying information about
   forwarding plane features, it needs to be regenerated whenever the
   BGP route's next hop is changed.  

## There is nothing in the document that actually constrains the nature of information that can be conveyed in an NHC. The allocation FCFS policy won’t help rationalize that space. 

## Having a more constrained range would be my favorite here (minimum Expert Review range). 


# “potentially useful”

CURRENT:
   The NHC signals
   potentially useful information related to the forwarding plane
   features, so it is desirable to make it transitive to ensure

## I’m sure there is a reasoning behind that subtle mention, but this raises the questions as why one would announce something that isn’t useful?

## Shouldn’t we simply say:

NEW:
   The NHC signals
   information related to the forwarding plane
   features, so it is desirable to make it transitive to ensure

# Characteristic Code

CURRENT:
   Characteristic Code: a two-octet unsigned integer that indicates the
   type of characteristic advertised and unambiguously identifies an
   individual characteristic.

## Shouldn’t the description include a mention about “forwarding” as that is the intended use?

## I suggest to add a pointer to the registry

NEW:
  Values are taken from the registry (Section 4).

## As I’m there, the document is silent about sending reserved values

Consider adding text such as the following here or in Section 2.2:

NEW:
  By default, characteristics with Code set to a reserved value (Section 4)
  MUST NOT be used when sending an NHC.

“by default” is on purpose as this allows to relax the rule for experimentation or testing.

Alternatively, we may only cover 0 and 65535 cases:

NEW:
  Characteristics with Code set to 0 and 65535 MUST NOT be used when sending an NHC.

# Characteristic Value

CURRENT:
   Characteristic Length: a two-octet unsigned integer that indicates
   the length, in octets, of the Characteristic Value field.  A length
   of 0 indicates that the Characteristic Value field is zero-length,
   i.e., it has a null value.

   Characteristic Value: a variable-length field.  It is interpreted
   according to the value of the Characteristic Code.

Should Characteristic Value description say that it includes a non-null value to avoid having two ways to encode null values?

# Redundant behavior

2.2.1

   To mitigate this problem, if a BGP speaker constructs a route whose
   next hop has no global part, it MUST include a BGPID TLV (Section 3).

Vs.

3.2
   when a route
   includes only a link-local address and no global address, the BGPID
   MUST be included.   

I would keep the normative language in one single place.

# Picky

## Peer vs Peers

A BGP speaker may have several peers.

OLD:
   RFC 5492 allows a BGP speaker to advertise its capabilities to its
   peer.  When a route is propagated beyond the immediate peer, it is

NEW:
   RFC 5492 allows a BGP speaker to advertise its capabilities to its
   peers.  When a route is propagated beyond an immediate peer, it is

or

NEW2:
   RFC 5492 allows a BGP speaker to advertise its capabilities to a
   peer. When a route is propagated beyond an immediate peer, it is

A similar construct is also present in the introduction. 

## Multiple ASNs

A speaker may have multiple ASNs.

OLD:
  AS Number: The Autonomous System Number [RFC6793]

NEW:
  AS Number: An Autonomous System Number [RFC6793]

## It is the ASN that is carried in the attribute

OLD: Autonomous System carried in the BGPID.

NEW: Autonomous System Number carried in the BGPID.

Cheers,
Med