Re: [Idr] Genart last call review of draft-ietf-idr-bgp-ls-sbfd-extensions-07

Ketan Talaulikar <ketant.ietf@gmail.com> Sun, 17 April 2022 17:15 UTC

Return-Path: <ketant.ietf@gmail.com>
X-Original-To: idr@ietfa.amsl.com
Delivered-To: idr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 6E1513A12BE; Sun, 17 Apr 2022 10:15:30 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.107
X-Spam-Level:
X-Spam-Status: No, score=-2.107 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id liFayFowb9aQ; Sun, 17 Apr 2022 10:15:27 -0700 (PDT)
Received: from mail-vk1-xa34.google.com (mail-vk1-xa34.google.com [IPv6:2607:f8b0:4864:20::a34]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 009613A12D3; Sun, 17 Apr 2022 10:15:26 -0700 (PDT)
Received: by mail-vk1-xa34.google.com with SMTP id bc42so5339575vkb.12; Sun, 17 Apr 2022 10:15:26 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9fUmv4WIMVXlMvChpKv7Jo8qLi73I2eheQREaWPgCQw=; b=Q+LF1tMwSJDntt+APEp0CitPfveUvbiUUsGoTZ2O+rA8okdhaSS1uIFm/KQlR7uPRX cqVxIug1jsdVZwocAvBHw6omm5TcEGh0FALqsT0r5/5UoctQbCNo9kriZAIjOWL/8JyX FE+0Dtp9Vqu7bY5oyeLoD5EbZwfSzWXYz9VWqCnMSe2G0ou9Iq1C9MBq8w7RMAcGD+8q yW8ShTitdmOVCymlsuCyPYDh3x/x1TzlY6NVOPi3rvEo7ULLWYvpWWYabygdV9f49CbU i9HcoW0FFYieB++x6GhSUC8PcDrcxB4qdbH58waG3+YbeVPrtWQYtlQwba2I/whsGtJ3 8BZQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9fUmv4WIMVXlMvChpKv7Jo8qLi73I2eheQREaWPgCQw=; b=PUJQ0zmL4PXO00JGzGeXeE3PlGRUIm/qkpms7eAIxAcfk8LwdshJdYxbAlVipzLTKQ CYIUJeCNa3lImehqCminro+ctHlwuHjw2WyZtdYEi/y5O+02aoKgP9HDTFf/2wvkyRyK 0JvEG+4rFIILtoS1OAUtrBoAUsA4KXtTzi5cSCRAxfJ9dCNcg/EDHCmAyjc59p/6kgM3 g8YXjFAKMBseG8iabVj+eQNTQrm9Z2P+FJAhnHgQXlTujMVteRodcb/SYO1ieYNUcMqX Z3zB+dLRoGKnXh6Vslk/Zb7zHtvwIHrVKVdKls+MVklmR6vtH404RVu6UTt+Emm+wWie 0GRA==
X-Gm-Message-State: AOAM533rkFS5VRSK9C9x6Dj9u8jnI2NmLGaUnhi/0dyI9jCXiSqSlXjU UDihn1AEyp3ignhtMtQ+OmRS4R0Kg18cjfQoz8hvoSeE
X-Google-Smtp-Source: ABdhPJxTSGHyLtCd0zBpNtC7ksUAzVMxrL0MpK11cRDnNcuLdlNGV+MqtXKatIG8DYWfbUnArm6JyxVlstz0N28WwBA=
X-Received: by 2002:a1f:2d56:0:b0:339:578b:471d with SMTP id t83-20020a1f2d56000000b00339578b471dmr1799773vkt.7.1650215725233; Sun, 17 Apr 2022 10:15:25 -0700 (PDT)
MIME-Version: 1.0
References: <165012491377.2723.10114043848098788296@ietfa.amsl.com>
In-Reply-To: <165012491377.2723.10114043848098788296@ietfa.amsl.com>
From: Ketan Talaulikar <ketant.ietf@gmail.com>
Date: Sun, 17 Apr 2022 22:45:12 +0530
Message-ID: <CAH6gdPwq9X55r3xEnHdga55zi3RtW6O+1gtWpqqOfKAZH+8qPA@mail.gmail.com>
To: Thomas Fossati <thomas.fossati@arm.com>
Cc: gen-art@ietf.org, draft-ietf-idr-bgp-ls-sbfd-extensions.all@ietf.org, idr@ietf.org, last-call@ietf.org
Content-Type: multipart/alternative; boundary="0000000000001afb1105dcdccaa4"
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/t1sv8zLjMO0pGhdwx3jV0y38nnw>
Subject: Re: [Idr] Genart last call review of draft-ietf-idr-bgp-ls-sbfd-extensions-07
X-BeenThere: idr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Inter-Domain Routing <idr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/idr>, <mailto:idr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/idr/>
List-Post: <mailto:idr@ietf.org>
List-Help: <mailto:idr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/idr>, <mailto:idr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 17 Apr 2022 17:15:31 -0000

Hi Thomas,

Thanks for your review and please check inline below for responses. We will
incorporate the changes discussed below in the next update.


On Sat, Apr 16, 2022 at 9:31 PM Thomas Fossati via Datatracker <
noreply@ietf.org> wrote:

> Reviewer: Thomas Fossati
> Review result: Ready
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-idr-bgp-ls-sbfd-extensions-??
> Reviewer: Thomas Fossati
> Review Date: 2022-04-16
> IETF LC End Date: 2022-04-27
> IESG Telechat date: Not scheduled for a telechat
>
> This document is compact and well written.  I think it is ready for
> publication.  Thank you editors, and all the people involved in its
> drafting.  I have left a few small editorial suggestions (caveat: I am
> completely unfamiliar with the subject matter) in case you may find them
> useful.
>
> ## Section 3, first para:
>
> OLD
>    The BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of
>    nodes and their attributes using the BGP-LS Attribute.
>
> NEW
>    BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of
>    nodes and their attributes using the BGP-LS Attribute.


KT> Agree. Will fix.


>
>
> ## Section 3, second para:
>
> I had to jump a couple of times between the textual definition and the
> TLV layout to understand how the two actually match.  Specifically, I
> think it's the "Length: variable." bit that triggered my confusion.
>
> So, I have an editorial suggestion that would have saved me from a
> (very) mild cognitive dissonance:
>
> OLD
>       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
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |              Type             |             Length            |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                         Discriminator 1                       |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                    Discriminator 2 (Optional)                 |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                               ...                             |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                    Discriminator n (Optional)                 |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>                Figure 1: S-BFD Discriminators TLV
>
>      where:
>
>    o  Type: 1032 (early allocation by IANA)
>
>    o  Length: variable.  It MUST be a minimum of 4 octets and increments
>       of 4 octets for each additional discriminator.
>
>    o  Discriminator n: 4 octets each, carrying an S-BFD local
>       discriminator value of the node.  At least one discriminator MUST
>       be included in the TLV.
>
> NEW
>       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
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |              Type             |          Length=4*n           |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                         Discriminator 1                       |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                    Discriminator 2 (Optional)                 |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                               ...                             |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>      |                    Discriminator n (Optional)                 |
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>    where Type is 1032, Length is 4*n, and n is the number of S-BFD local
>    discriminator values of the node carried in the TLV.  n MUST be at least
>    1.
>

KT> I believe the existing text is quite simple and aligned to existing
conventions for BGP-LS TLV description.


>
> ## Section 3, ignorant hat on:
>
> I am not sure what is the typical number of discriminators expected in a
> TLV?  It looks to me that ~65K/4 is a decent magnitude.  What is the
> expectation in case the number of discriminators is higher than the
> maximum allowed by the TLV encoding?  Is this a valid scenario?  If so,
> how is overflow handled?
>

KT> The overflow is not really valid. The info is sourced from IGPs and
there the size for ISIS is much lower while for OSPF is the same as BGP-LS.
Therefore, I don't see the the possibility of overflow.


>
> ## Section 4, Table 1:
>
> The structure doesn't seem to match the layout of the "BGP-LS Node
> Descriptor, etc." IANA registry.  In particular:
> * The length field seems spurious?
> * The IS-IS TLV/Sub-TLV is missing — but I guess this can be skipped, so
>   not a problem;
> * The first column heading should be "TLV Code Point";
> * The Reference column is missing.
>
> OLD
>          +---------------+--------------------------+----------+
>          |  Code Point   | Description              | Length   |
>          +---------------+--------------------------+----------+
>          |     1032      | S-BFD Discriminators TLV | variable |
>          +---------------+--------------------------+----------+
>
> NEW
>          +----------------+--------------------------+-----------+
>          | TLV Code Point | Description              | Reference |
>          +----------------+--------------------------+-----------+
>          |      1032      | S-BFD Discriminators TLV |  RFCthis  |
>          +----------------+--------------------------+-----------+
>

KT> Agree. Will fix.


>
> ## Section 5, first sentence:
>
> OLD
>    The new protocol extensions introduced in this document augment the
>    existing IGP topology information that was distributed via [RFC7752].
>
> NEW
>    The new protocol extensions introduced in this document augment the
>    existing IGP topology information that was distributed via BGP-LS
>    [RFC7752].
>

KT> Agree.


>
> ## Section 6, first para:
>
> OLD
>    The new protocol extensions introduced in this document augment the
>    existing IGP topology information that can be distributed via
>    [RFC7752].
>
> NEW
>    The new protocol extensions introduced in this document augment the
>    existing IGP topology information that can be distributed via BGP-LS
>    [RFC7752].
>

KT> Agree


>
> # Section 5, third sentence:
>
> Typo: "now encompasses" should be "now encompass"
>

KT> Agree

Thanks,
Ketan