[Idr] Mohamed Boucadair's Yes on draft-ietf-idr-nhc-05: (with COMMENT)

[Mohamed Boucadair via Datatracker <[e-mail]>]

Mohamed Boucadair has entered the following ballot position for
draft-ietf-idr-nhc-05: Yes

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ 
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-idr-nhc/



----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

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