Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

"Jan Lindblad (jlindbla)" <jlindbla@cisco.com> Wed, 13 September 2017 13:31 UTC

Return-Path: <jlindbla@cisco.com>
X-Original-To: l3sm@ietfa.amsl.com
Delivered-To: l3sm@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 7844F133036 for <l3sm@ietfa.amsl.com>; Wed, 13 Sep 2017 06:31:40 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.519
X-Spam-Level:
X-Spam-Status: No, score=-14.519 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, 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 lMP-Cp0uWuOg for <l3sm@ietfa.amsl.com>; Wed, 13 Sep 2017 06:31:38 -0700 (PDT)
Received: from alln-iport-4.cisco.com (alln-iport-4.cisco.com [173.37.142.91]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 16A221321A1 for <l3sm@ietf.org>; Wed, 13 Sep 2017 06:31:38 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=23250; q=dns/txt; s=iport; t=1505309498; x=1506519098; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=YtXuW+K5BBLyzahs4wMvrYR5VEcRvOM7a3botMDBiCQ=; b=RXbilOx9MpcEvREldBS/NhR2ZDNAKA8CAFcWIMgo9x++kdnYvk6TL5Xr jKg7M1lnI/+PNvQT25ymzuobVIWksT68JT5c/WQrRGNgx/xsIGEuakM3O S3d2SOMpcQhRXjddZPSYWUiTDmfbJwF9RUvYvcSc7I1Hhi+FqZO5nBZwO g=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0BxBQA6MrlZ/5hdJa1UCRkBAQEBAQEBA?= =?us-ascii?q?QEBAQcBAQEBAYJva4FSJweDcJpGkluFTYIECoU+AhqEOFcBAgEBAQEBAmsohRk?= =?us-ascii?q?GDhVCEgIQAgEIPwMCAgIwFBECBAENBYlNZKt+gSOCJyeLDwEBAQEBAQEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAR2DK4ICgVCCDguBZYENhDoFDgMBDi+CfDCCMQWgeAKUUJJ0lQI?= =?us-ascii?q?CERkBgTgBV4ENdxVKEgGFBhyBZ3aFeQElgQyBDwEBAQ?=
X-IronPort-AV: E=Sophos;i="5.42,388,1500940800"; d="scan'208,217";a="2896552"
Received: from rcdn-core-1.cisco.com ([173.37.93.152]) by alln-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Sep 2017 13:31:37 +0000
Received: from XCH-ALN-002.cisco.com (xch-aln-002.cisco.com [173.36.7.12]) by rcdn-core-1.cisco.com (8.14.5/8.14.5) with ESMTP id v8DDVb3m026258 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Wed, 13 Sep 2017 13:31:37 GMT
Received: from xch-aln-004.cisco.com (173.36.7.14) by XCH-ALN-002.cisco.com (173.36.7.12) with Microsoft SMTP Server (TLS) id 15.0.1263.5; Wed, 13 Sep 2017 08:31:36 -0500
Received: from xch-aln-004.cisco.com ([173.36.7.14]) by XCH-ALN-004.cisco.com ([173.36.7.14]) with mapi id 15.00.1263.000; Wed, 13 Sep 2017 08:31:36 -0500
From: "Jan Lindblad (jlindbla)" <jlindbla@cisco.com>
To: "Benoit Claise (bclaise)" <bclaise@cisco.com>, Qin Wu <bill.wu@huawei.com>
CC: "David Ball -X (daviball - ENSOFT LIMITED at Cisco)" <daviball@cisco.com>, l3sm <l3sm@ietf.org>, adrian <adrian@olddog.co.uk>
Thread-Topic: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt
Thread-Index: AQHTJvl1LUN8x/LDjUWvHXDkwgSGUKKnpP4ggAfVTYCAAR3wEIAAlZcAgAAMJQCAAF2VAIABl4kA
Date: Wed, 13 Sep 2017 13:31:36 +0000
Message-ID: <A56DC6CF-17E5-4739-9C64-76740299EC0E@cisco.com>
References: <B8F9A780D330094D99AF023C5877DABA9AAFC86C@nkgeml513-mbx.china.huawei.com> <b85886fa-7f8f-3e56-a8cb-7d72c4828fba@cisco.com> <B8F9A780D330094D99AF023C5877DABA9AB0DDDD@nkgeml513-mbx.china.huawei.com> <B5B3032C-A0CB-4BD0-9497-191F2554F723@cisco.com> <a1067e3b-3d3c-e964-70ab-5432663a69f8@cisco.com> <9e9c1527-08a9-bcef-c463-892bd9ed2f87@cisco.com>
In-Reply-To: <9e9c1527-08a9-bcef-c463-892bd9ed2f87@cisco.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.147.40.148]
Content-Type: multipart/alternative; boundary="_000_A56DC6CF17E547399C6476740299EC0Eciscocom_"
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/l3sm/4U7kFOgAbpbKW340cQW9oFkRXS4>
Subject: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt
X-BeenThere: l3sm@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: L3VPN Service YANG Model discussion group <l3sm.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/l3sm>, <mailto:l3sm-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/l3sm/>
List-Post: <mailto:l3sm@ietf.org>
List-Help: <mailto:l3sm-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/l3sm>, <mailto:l3sm-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 13 Sep 2017 13:31:40 -0000

I saw the -04 was released now, but I had a quick review of the YANG model in -03. I think it has improved a great deal from a YANG perspective since my last look. I'd say it's in good shape, but of course I have some nits and comments still.

- The indentation is off, which needs to be fixed before publication.
- There are still many optional elements with no default or obvious behavior specified in the description for when they are absent. It's a lot better now than it has been, but work remains.
- A few literal comparisons with identity values are still there. The move to YANG 1.1 helps a lot, but we could improve the correctness and flexibility of the model by changing them to use appropriate XPATH functions.



#1)
579 choice list-flavor
No default, and not mandatory. What does it mean if the operator does not choose any of permit-any, deny-any-except, permit-any-except?

#2)
1064 leaf l4-src-port vs. l4-src-port-range/lower-port
What would it mean if l4-src-port and l4-src-port-range/lower-port and higher-port are set at the same time? What if only lower-port or only higher-port are set?
1094 Same question again for dst-port.

#3)
1300 leaf rate-limit { type uint8; units percent;
What happens if rate-limit is configured with a value >100%. Is that the same as 100%, or would 200% actually mean double the configured rate? Maybe introduce a range 0..100 ?
1362 Same question again for guaranteed-bw-percent

#4)
1415 encryption/layer mandatory even if encryption/enabled = 'false'
Maybe add a must statement to require a value when enabled is true?

#5)
1734 identityref with string compare
     must "current() != 'slaac' and current() != 'provider-dhcp-slaac'" {
is missing the prefixes, so will always allow slaac. Instead of simply adding the prefix, I suggest the more flexible
     must "derived-from-or-self(current(), 'l3vpn-svc:slaac') or derived-from-or-self(current(), 'l3vpn-svc:provider-dhcp-slaac')" {

#6)
1747 container provider-dhcp: provider-address and mask
Both of these are optional. What does it mean to leave provider-address blank? What if no mask? What if mask but no provider-address?
Maybe add a description to the provider-address and a must statement to the mask (if mask specified, address has to as well)
1810 Same question fot dhcp-relay
1886 Same question for ipv6/provider-dhcp
1949 Same question for ipv6/dhcp-relay

#7)
1763 number-of-dynamic-address
Modeled as an uint8. For ipv4 I understand it's hard to imagine anyone asking for more than 255 addresses. But should the number be limited by the choice of integer size?
1898 Same question for ipv6. Perhaps more relevant there. Is 255 an astronomically large number here? ;-)

#8)
1774 group-id description
The description says "Group-id the list of start-to-end address belongs to." I beg your pardon.

#9)
1842 container addresses: provider-address, customer-address, mask
The provider-address and mask are marked mandatory. customer-address optional. Is the mask for provider-, customer-address or perhaps both?
1981 Same question for ipv6

#10)
1880 identityref with string compare
        when "../address-allocation-type = 'l3vpn-svc:provider-dhcp' "+
        "or ../address-allocation-type "+"= 'l3vpn-svc:provider-dhcp-slaac'" {
has the right prefixes, so this one works. Still, for the same reason as 1734, I suggest the more flexible
        when "derived-from-or-self(../address-allocation-type, 'l3vpn-svc:provider-dhcp') "+
        "or derived-from-or-self(../address-allocation-type, 'l3vpn-svc:provider-dhcp-slaac')" {

#11)
1918 list address-group: start-address and end-address
Both are optional. What if you don't give either one, or if you give start but not end, or vice versa?

#12)
2024 container bfd: fixed-value
Optional and has no default value, but is selected by default, so what is the BFD holdtime if not set?

#13)
2184 address-family and address both optional
What if you set address but not address-family, or vice versa?


I did not check any of the examples this time.

Thanks,
/jan



Dear all,

I was about the click the "IETF LC" call button, but I understand from Qin that a new document version will be produced.
I'll then click this button when the next version is posted.

Regards, B.

Completely agree.  In the case in question (address-allocation-type leaves), no value means "IPv4/IPv6 is not enabled for this site-network-access".


    David

On 12/09/2017 07:54, Jan Lindblad (jlindbla) wrote:
Qin, team,


  *
For the address-allocation-type leaves, I saw you removed the default (as agreed) but also added 'mandatory true' (which was not discussed).  Making these leaves mandatory does not address the problem - if anything, it makes it worse.  (Issue 15 from draft-02)

[Qin]: Fine to me, it looks we need to seek balance between making all parameters mandatory and making all parameters optional. I hope Jan will be happy with these changes.

Sometimes mandatory true is needed to make a sane model, but mandatory elements also tend to make a model clunky, examples large etc. So I generally like optional elements. The problem with optional elements is people tend to forget that it may not be obvious what a system is supposed to do when there is no value specified. Adding a default or text in the description is therefore important. At the end of the day, we're writing a contract. For interoperability to happen, there must be no holes in the contract that are open to (differing) interpretation.

So sure, you can have optional elements (this should even be the normal case), and they don't need to have a default statement. But if so, *describe* what it means; what the system is supposed to do. No value is also a value.

Best,
/jan



--
David Ball
<daviball@cisco.com><mailto:daviball@cisco.com>