Re: [Lsr] John Scudder's No Objection on draft-ietf-lsr-isis-srv6-extensions-14: (with COMMENT)

Peter Psenak <ppsenak@cisco.com> Mon, 17 May 2021 13:00 UTC

Return-Path: <ppsenak@cisco.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C98453A3706; Mon, 17 May 2021 06:00:30 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -9.599
X-Spam-Level:
X-Spam-Status: No, score=-9.599 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_MED=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_NONE=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 U3HNfB_9vaBZ; Mon, 17 May 2021 06:00:25 -0700 (PDT)
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 9004D3A3705; Mon, 17 May 2021 06:00:24 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=8729; q=dns/txt; s=iport; t=1621256425; x=1622466025; h=subject:to:cc:references:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=4MThlVmm+ysA237opzfD8Ixd3afGPt9Hmx7zNP0bwaw=; b=K9b9JZk7QAuZFu5pvWtzVxBk+NGFMECooYTO8RXi5ZWprAlgH+qEU7By plHWVghzOesjEAg/p9Svd+tyFd02YhlGg8WD65he4lRbb6iaPnER6Vwif qOB+vqd5zLGgOD4JxwgEs0VKwsCOtnWDRe7G3oxKKLZDNRJRD2S6jrlgj A=;
X-IPAS-Result: =?us-ascii?q?A0AuAAC6Z6JglxbLJq1aGQEBAQEBAQEBAQEBAQEBAQEBA?= =?us-ascii?q?RIBAQEBAQEBAQEBAQFAgVeBfIEmVgE5MYQJPokEiEUtA49Bi2iBaAsBAQEPN?= =?us-ascii?q?QwEAQGETwKBdSY4EwIEAQEBAQMCAwEBAQEFAQEFAQEBAgEGBBQBAQEBAQEBA?= =?us-ascii?q?WiFUA2GRAEBAQMBDBcECwEFQQULCQIOBAYCAiYCAkkOBgEMCAEBgm0BgmYhD?= =?us-ascii?q?406mw96fzOBAYNeQUSDLYE+BoEQKgGJa4N3Q4FJRIEVJwyCbz6CYQIBAoEXE?= =?us-ascii?q?QELBwEHgzGCYwSCQAYBYwEDCBAKGRYCLyAILj0UCB4BBBEqO5BygnemNYEVg?= =?us-ascii?q?yCDS4Y3kzUFCQUjg1qLE4V1kFuVNoIWiXCTEAsOhHyBayFrWBEHMxoIGxU7g?= =?us-ascii?q?mlQGQ6OKw0JFW0BAoddhUs/Ay8CNgIGAQkBAQMJikmBaV4BAQ?=
IronPort-HdrOrdr: A9a23:TYW796CuVo4fg4XlHemd55DYdb4zR+YMi2TDGXocdfUnSL3+qy nIpoV86faUskd0ZJhOo7C90cW7L080sKQFhLX5Xo3SPjUO2lHIEGgK1+KLqAEIWReQygc378 1dmsZFZeEYQWIK7voTJGKDYq4dKB7tytHQudvj
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-AV: E=Sophos;i="5.82,307,1613433600"; d="scan'208";a="36015777"
Received: from aer-iport-nat.cisco.com (HELO aer-core-3.cisco.com) ([173.38.203.22]) by aer-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-SEED-SHA; 17 May 2021 13:00:18 +0000
Received: from [10.60.140.52] (ams-ppsenak-nitro3.cisco.com [10.60.140.52]) by aer-core-3.cisco.com (8.15.2/8.15.2) with ESMTP id 14HD0IR6025890; Mon, 17 May 2021 13:00:18 GMT
To: John Scudder <jgs@juniper.net>, The IESG <iesg@ietf.org>
Cc: draft-ietf-lsr-isis-srv6-extensions@ietf.org, lsr-chairs@ietf.org, lsr@ietf.org, Christian Hopps <chopps@chopps.org>, aretana.ietf@gmail.com
References: <162092059520.16031.2606295470570253120@ietfa.amsl.com>
From: Peter Psenak <ppsenak@cisco.com>
Message-ID: <99e86e7b-ea98-0df5-4472-7339996822fa@cisco.com>
Date: Mon, 17 May 2021 15:00:18 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <162092059520.16031.2606295470570253120@ietfa.amsl.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-Outbound-SMTP-Client: 10.60.140.52, ams-ppsenak-nitro3.cisco.com
X-Outbound-Node: aer-core-3.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/XygjMrINMbHHRswj60y1kg5vZkc>
Subject: Re: [Lsr] John Scudder's No Objection on draft-ietf-lsr-isis-srv6-extensions-14: (with COMMENT)
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 17 May 2021 13:00:31 -0000

Hi John,

thanks for your comments, please see inline (##PP):

On 13/05/2021 17:43, John Scudder via Datatracker wrote:
> John Scudder has entered the following ballot position for
> draft-ietf-lsr-isis-srv6-extensions-14: No Objection
> 
> 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/iesg/statement/discuss-criteria.html
> for more information about DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-lsr-isis-srv6-extensions/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Below are some questions and comments. I'd appreciate a reply, thanks.
> 
> 1. Abstract
> 
>     The Segment Routing (SR) allows for a flexible definition of end-to-
> 
> You either have too many, or not enough words here. Either “segment routing“
> (remove “the“), or something like “the segment routing architecture“.
> 
>     called "segments".  Segment routing architecture can be implemented
> 
> And here you do need “the“ in front of “segment”.
> 
>     over an MPLS data plane as well as an IPv6 data plane.  This document
> 
> “An” -> “the” (two places)
> 
>     over an IPv6 data plane.
> 
> “An” -> “the”
> 
>     This documents updates RFC 7370 by modifying an existing registry.
> 
> “Documents” -> “document”

##PP
I reworded the abstract. This text was written prior to me taking over 
the document...


> 
> Also, this doesn’t seem to me like an update to RFC 7370. It’s normal for an
> RFC to update an IANA registry, without saying it updates a previous RFC that
> established that registry. I think the “updates” just confuses matters and
> clutters things up, and should be removed.

##PP
looks like we converged towards keeping it.

> 
> 2. Section 1
> 
>     This documents updates [RFC7370] by modifying an existing registry
>     (Section 11.1.2).
> 
> “Documents” -> “document”

##PP
fixed


> 
> See also comment #1 regarding update.
> 
> 3. Section 4.3
> 
>     A non-zero SRH Max H.encaps MSD indicates that the headend can insert
>     an SRH up to the advertised value.
> 
> “Up to the advertised value” doesn’t make sense. I guess you mean “can insert
> an SRH with up to the advertised number of SIDs”?

##PP
fixed

> 
> 4. Section 4.4
> 
>     The Maximum End D MSD Type specifies the maximum number of SIDs
>     present in an SRH when performing decapsulation.
> 
> That makes sense.
> 
>                These includes, but
>     not limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with
>     USD as defined in [RFC8986].
> 
> That doesn’t. How can a number include all those specific things? A number’s
> just an integer, right? Maybe you are providing some helpful context about the
> types of SIDs that are permitted to be present? 

##PP
yes, it was specifically requested by Alvaro during his AD review.

> If so, I expect this is
> specified elsewhere already, and this sentence is not helping; I would suggest
> removing it. If it does need to stay, it needs a rewrite for grammar and
> clarity. (Maybe you want something like “As specified in [RFC 8986] the
> permitted SID types include, but are not limited to, <list>.”)

##PP
I changed to:

"As specified in [RFC 8986] the permitted SID types include, but are not 
limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with USD."


> 
>        If the advertised value is zero or no value is advertised
>        then the router cannot apply any behavior that results in
>        decapsulation and forwarding of the inner packet if the
>        other IPv6 header contains an SRH.
> 
> What “other” IP header? Do you mean the outer header? The inner header?
> Something else?

##PP
should be "outer" instead of "other", it was a typo, I fixed it.

> 
> 5. Section 5
> 
>     In cases where a locator advertisement is received in both a Prefix
>     Reachability TLV and an SRv6 Locator TLV - (e.g. prefix, prefix-
>     length, MTID all being equal and Algorithm being 0 in Locator TLV),
> 
> Since “e.g.” means “for example” you’re saying the thing in the parentheses is
> only one example of a locator advertisement received in both? What’s another
> example? (Maybe the algo being 1 in both cases?)
> 
> But in any case the text above appears redundant with the text that immediately
> follows. Can’t the text above be deleted?

##PP
yeah, that has been pointed out by other reviewers, I have left the old 
text there by mistake. I have removed the above.

> 
>     In case where the same prefix, with the same prefix-length, MTID, and
>     algorithm is received in both a Prefix Reachability TLV and an SRv6
>     Locator TLV, the Prefix Reachability advertisement MUST be preferred
>     when installing entries in the forwarding plane.  This is to prevent
>     inconsistent forwarding entries between SRv6 capable and SRv6
>     incapable routers.
> 
> “In case” -> “in the case” or “in cases”

##PP
fixed.

> 
> 6. Section 7.2
> 
>     Supported behavior values together with parent TLVs in which they
>     area advertised are specified in Section 10 of this document.  Please
> 
> “Area” -> “are”

##PP
fixed.

> 
>        Length: variable.
> 
>        Flags: 1 octet.  No flags are currently defined.
> 
> When you write “length: variable“, I think you mean “length: a 1 octet field,
> whose value is variable“, don’t you? Just like you write “flags: 1 octet“? I
> get that the value placed into the length field is variable, but the way you’ve
> written it, it says that the length of the length field is itself variable,
> which would make no sense.

##PP
It refers to the value of the Length field. In the case it is static, we 
specify exact value, if it is variable we say "variable". We use this in 
many ISIS RFCs (e.g. RFC8667, section 2.2.2.)


> 
> This is not the only place in the document you specify a length field this way,
> but I guess you should fix all of them. I’m only noting it here.
> 
>     The SRv6 End SID MUST be a subnet of the associated Locator.  SRv6
>     End SIDs which are not a subnet of the associated locator MUST be
>     ignored.
> 
> Is a host route (which is what a SID is) technically a “subnet”? Can something
> which is not a net, be a subnet? It reads funny that way. If you change it,
> possibly “MUST be covered by the associated Locator“? (Although then for
> clarity you might need to define “covered“, which might not be any better.)
> (Same comment applies to Section 8 which also mentions “subnet”.)

##PP
what about:

"The SRv6 End SID MUST be allocated from its associated locator's 
prefix. SRv6 End SIDs that are not allocated from the associated 
locator's prefix MUST be ignored."


> 
> 7. Section 8.1
> 
>           B-Flag: Backup flag.  If set, the SID is eligible for
>           protection (e.g., using IPFRR) as described in [RFC8355].
> 
> Please expand IPFRR on first use. And since you say “e.g.” meaning “for
> example”, do you contemplate some other kind of protection which is not IPFRR?
> If not, I think you could delete the parenthesis without loss of clarity.

##PP
done

> 
> 8. Section 9
> 
>     SRv6 SID Structure Sub-Sub-TLV is used to advertise the as defined in
>     [RFC8986].  It has the following format:

##PP
fixed.

> 
> You’re missing something between “advertise the” and “as defined”.
> 
>        Length: 4 octets.
> 
> Similar to my earlier comment, I think what you mean is something like “Length:
> a 1 octet field, whose value is 4”.
> 
>     associated with it.  It's usage is outside of the scope of this
>     document.
> 
> “It’s” -> “its”

##PP
fixed
	
> 
> 9. Section 11
> 
>     and sub-sub-TLVs as well updating the ISIS TLV registry and defining
> 
> “As well as”

##PP
fixed

> 
	>     a new registry.
> 
> Doesn’t it define several new registries?


##PP
indeed, after we added registries for flag fields

> 
> 10. Section 11.2
> 
> Shouldn’t there be a new subsection for the registry creation?

##PP
added

thanks,
Peter

> 
> 
> 
> 
>