Re: [Gen-art] Genart last call review of draft-ietf-ospf-ospfv3-segment-routing-extensions-18

Peter Psenak <ppsenak@cisco.com> Fri, 16 November 2018 14:39 UTC

Return-Path: <ppsenak@cisco.com>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 20732130ED1; Fri, 16 Nov 2018 06:39:23 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.97
X-Spam-Level:
X-Spam-Status: No, score=-14.97 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.47, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.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 NW2lLSv1xAtW; Fri, 16 Nov 2018 06:39:20 -0800 (PST)
Received: from aer-iport-4.cisco.com (aer-iport-4.cisco.com [173.38.203.54]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id AC358130ED0; Fri, 16 Nov 2018 06:39:19 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=3447; q=dns/txt; s=iport; t=1542379160; x=1543588760; h=message-id:date:from:mime-version:to:cc:subject: references:in-reply-to:content-transfer-encoding; bh=dkDNuu0dCBZZ3ct4XBTmoqbiQiqyFjr9sV4N+2cXL1g=; b=nAOcSkajwmB+RXL0Eqp0NU34aFVKxKCrFXgmoxaWvY9donhND03nzHyi nSAnUWE9weobyn6Tnix2oaLHH6sdwPVypRAvon5w0cj4QOjStyFkA/gig CqBvbuFGF/Y0UBuDsmsITg+iRXj3osowsyDHdVQACxLtRXBCc2Jpv56nT I=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0AFAABM1e5b/xbLJq1jGQEBAQEBAQE?= =?us-ascii?q?BAQEBAQcBAQEBAQGBUgMBAQEBAQsBgmlwEoQfiHeNKpc2FIFmDSOESQKDdjU?= =?us-ascii?q?IDQEDAQECAQECbRwMhTwBAQEDASMVQAEFCwsUBAICBRYLAgIJAwIBAgFFBgE?= =?us-ascii?q?MAQcBAYMegXkID6gTgSEOhUGEWwWBC4sRgUA/gRGDEoMbAoFJgxyCVwKJRYY?= =?us-ascii?q?MhVmKQAmGeootGIFYhQeCfSaGeI01iluBRwE2gVUzGggbFYMogiMDFxKITIU?= =?us-ascii?q?/PgOOFQEB?=
X-IronPort-AV: E=Sophos;i="5.56,240,1539648000"; d="scan'208";a="8037383"
Received: from aer-iport-nat.cisco.com (HELO aer-core-4.cisco.com) ([173.38.203.22]) by aer-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Nov 2018 14:39:17 +0000
Received: from [10.147.24.43] ([10.147.24.43]) by aer-core-4.cisco.com (8.15.2/8.15.2) with ESMTP id wAGEdG79016727; Fri, 16 Nov 2018 14:39:17 GMT
Message-ID: <5BEED694.9060606@cisco.com>
Date: Fri, 16 Nov 2018 15:39:16 +0100
From: Peter Psenak <ppsenak@cisco.com>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:24.0) Gecko/20100101 Thunderbird/24.4.0
MIME-Version: 1.0
To: Pete Resnick <resnick@episteme.net>, gen-art@ietf.org
CC: lsr@ietf.org, ietf@ietf.org, draft-ietf-ospf-ospfv3-segment-routing-extensions.all@ietf.org
References: <154237593213.617.1926432231677036618@ietfa.amsl.com>
In-Reply-To: <154237593213.617.1926432231677036618@ietfa.amsl.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-Outbound-SMTP-Client: 10.147.24.43, [10.147.24.43]
X-Outbound-Node: aer-core-4.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/gen-art/-mGE8TNvAzp2eEiEtO1ZLvKtWzQ>
Subject: Re: [Gen-art] Genart last call review of draft-ietf-ospf-ospfv3-segment-routing-extensions-18
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 16 Nov 2018 14:39:23 -0000

Hi Pete,

please see inline:

On 16/11/18 14:45 , Pete Resnick wrote:
> Reviewer: Pete Resnick
> Review result: Ready with Issues
>
> 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-ospf-ospfv3-segment-routing-extensions-18
> Reviewer: Pete Resnick
> Review Date: 2018-11-16
> IETF LC End Date: 2018-11-16
> IESG Telechat date: Not scheduled for a telechat
>
> Summary: One serious concern, one minor issue, and some nits.
>
> Major issues:
>
> In 3.1:
>
> I get worried when I see this:
>
>        SID/Label: If length is set to 3, then the 20 rightmost bits
>        represent a label.
>
> So, the Length is not a length, but rather a flag. This seems like it has the
> potential for an interop problem. If a general implementation treats it as a
> length, it's going to get the left-most 24 bits when the value is 3. Even if
> the implementation chooses the right-most 24 bits, it's only supposed to take
> the right-most 20 bits and mask off the extra 4 bits. Or are you going to
> specify that implementations must always set the extra 4 bits to 0?

no. We have other documents that use the same and none of them enforces 
the other bits to 0. When we say we only use specific bits, the rest can 
be set to anything.

>
> Maybe this sort of thing is the way things have always been done for TLVs, and
> if so feel free to ignore me, but from an code implementation perspective, this
> seems like an accident waiting to happen. (Known sometimes as a "foot-gun".)
>
> Minor issues:
>
> In 4.4:
>
>     The SRMS Preference TLV MAY only be advertised once in the OSPFv3
>     Router Information Opaque LSA and has the following format:
>
> You mean MUST, not MAY there.

sure, I will change to MUST.

>
> In 7.1 and 7.2:
>
> If SID/Index/Label is a label, is it using the low 20 bits of the first 3 bytes
> of the field (i.e., bits 4-23)?

right.

> Is there a requirement that the high 4 bits and
> the low 8 bits must be cleared by the implementation?

there is no such requirement really.

>Some clarification would be useful.
>
> In 8.1:
>
> You talk about setting the IA-flag, but this version of the document doesn't
> define the IA-flag anymore. Is it defined elsewhere?

my bad, we have removed IA in last version, but I forgot to remove this 
part. I removed it.

>
> Nits/editorial comments:
>
> In 3.1:
>
> Ignoring the issue stated above, I also found this text a bit confusing:
>
>        Length: Variable, 3 or 4 octets
>
> Obviously you mean that the value of Length is either 3 or 4. At first I read
> it as the value of Length was variable, and that it took up 3 or 4 octets in
> the Sub-TLV. If this is the way you've always written these things, fine, but
> it seems to me it would be clearer to say.
>
>        Length: Either 3 or 4

Done.


>
> In 5:
>
>     s/we need a single advertisement/a single advertisement is needed
>
> Just being pedantic. If you like it, use it. If not, don't.

Done.

Please let me know if you agree or disagree with my responses.

thanks,
Peter

>
>
> .
>