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

From: Qin Wu <[email-masked]>
Date: Wed, 6 Sep 2017 10:25:05 +0000

We have incorporated additional comments from David in v-(03).
https://www.ietf.org/rfcdiff?url2=draft-wu-l3sm-rfc8049bis-03
Thank David, thanks for L3SM design team help complete this.

-Qin
-----邮件原件-----
发件人: [email-masked] [mailto:[email-masked]] 
发送时间: 2017年9月6日 18:18
收件人: Qin Wu; Luis Tomotaki; Kenichi Ogaki; Stephane Litkowski
主题: New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

A new version of I-D, draft-wu-l3sm-rfc8049bis-03.txt has been successfully submitted by Qin Wu and posted to the IETF repository.

Name:		draft-wu-l3sm-rfc8049bis
Revision:	03
Title:		YANG Data Model for L3VPN Service Delivery
Document date:	2017-09-06
Group:		Individual Submission
Pages:		181
URL:            https://www.ietf.org/internet-drafts/draft-wu-l3sm-rfc8049bis-03.txt
Status:         https://datatracker.ietf.org/doc/draft-wu-l3sm-rfc8049bis/
Htmlized:       https://tools.ietf.org/html/draft-wu-l3sm-rfc8049bis-03
Htmlized:       https://datatracker.ietf.org/doc/html/draft-wu-l3sm-rfc8049bis-03
Diff:           https://www.ietf.org/rfcdiff?url2=draft-wu-l3sm-rfc8049bis-03

Abstract:
   This document defines a YANG data model that can be used for
   communication between customers and network operators and to deliver
   a Layer 3 provider-provisioned VPN service.  This document is limited
   to BGP PE-based VPNs as described in RFCs 4026, 4110, and 4364.  This
   model is intended to be instantiated at the management system to
   deliver the overall service.  It is not a configuration model to be
   used directly on network elements.  This model provides an abstracted
   view of the Layer 3 IP VPN service configuration components.  It will
   be up to the management system to take this model as input and use
   specific configuration models to configure the different network
   elements to deliver the service.  How the configuration of network
   elements is done is out of scope for this document.

   If approved, this document obsoletes RFC 8049.  The changes are a
   series of small fixes to the YANG module, and some clarifications to
   the text.

Please note that it may take a couple of minutes from the time of submission until the htmlized version and diff are available at tools.ietf.org.

The IETF Secretariat

---

From: David Ball <[email-masked]>
Date: Mon, 11 Sep 2017 18:55:39 +0100

Thanks Qin.

I checked and I think a few things that we agree previously haven't yet 
been incorporated:

  * In Sn 6.12.3.2, regarding the confusion between the second paragraph
    (begining "In the case of a provider-managed or co-managed
    connection, ...") and the bullet about direction - I think we agreed
    to delete the paragraph (at least, you asked on the list if anyone
    objected and I didn't see any replies).  This was issue 9 in my
    comments on draft-02.
  * The provider address and mask are now optional in the case of DHCP,
    as agreed; however, in the case of static, they are still marked as
    mandatory.  I think the following leaves need to have mandatory
    removed as well (issue 11 from draft-02)
      o l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv4/addresses/provider-address
      o l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv4/addresses/mask
      o l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv6/addresses/provider-address
      o l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv6/addresses/mask
  * In the filters container/choice, we agreed to add 'when' statements
    for each case based on the type, to prevent invalid combination of
    the type and filter lists as in the example I posted.  (Issue 13
    from draft-02).
  * 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)

Two new comments:

 1. Under the cloud-access list, the "authorized-sites" and
    "denied-sites" have been added back in, in draft-03.  These were
    previously removed because they duplicate the functionality in the
    'list-flavour' choice.
 2. There are a number of leaves that are marked as mandatory or have
    defaults, which I think should not be - sorry for not raising this
    earlier, actually it was only just pointed out to me by someone else:
     1. l3vpn-service/sites/site/security/encryption/layer - this should
        either be optional, or have a 'when' statement that means it
        only exists when encryption is enabled.  If encryption is
        disabled, there is no need to specify the layer.
     2. l3vpn-service/sites/site/security/encryption/encryption-profile/profile
        - same
     3. l3vpn-service/sites/site/service/qos/qos-profile/qos-profile
        (and same under site-network-accesses) - I'm not sure why this
        choice is mandatory, that means one of the options has to be
        specified for both the site and the site-network-access.  I
        think the intent is that it is optional in both places, so it
        can be specified in one or the other.
     4. l3vpn-service/sites/site/site-network-accesses/site-network-access/service/multicast/multicast-address-family/ipv[46]
        - IPv4 has default true but IPv6 has default false; I think they
        should both be default false.

Thanks,

     David

-- 
David Ball
<[email-masked]>

---

From: Qin Wu <[email-masked]>
Date: Tue, 12 Sep 2017 03:11:02 +0000

发件人: L3sm [mailto:[email-masked]] 代表 David Ball
发送时间: 2017年9月12日 1:56
收件人: Qin Wu; l3sm
抄送: Benoit Claise (bclaise); adrian
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

Thanks Qin.

I checked and I think a few things that we agree previously haven't yet been incorporated:

  *   In Sn 6.12.3.2, regarding the confusion between the second paragraph (begining "In the case of a provider-managed or co-managed connection, ...") and the bullet about direction - I think we agreed to delete the paragraph (at least, you asked on the list if anyone objected and I didn't see any replies).  This was issue 9 in my comments on draft-02.
            [Qin]: See discussion we had earlier: https://www.ietf.org/mail-archive/web/l3sm/current/msg00746.html
            I think there is a typo in my response to you. That is to say I think the 2nd paragraph in section 6.12.3.2 is fine, but if we really enhance the 2nd paragraph in section 6.12.3.2 to make it consistent with introduced new   parameter “direction”, I need to get confirmation from L3SM design team, unfortunately I didn’t get feedback on this, that is why I leave as it does. I will ping them again, to see how to enhance the current text.

  *   The provider address and mask are now optional in the case of DHCP, as agreed; however, in the case of static, they are still marked as mandatory.  I think the following leaves need to have mandatory removed as well (issue 11 from draft-02)
     *   l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv4/addresses/provider-address
     *   l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv4/addresses/mask
     *   l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv6/addresses/provider-address
     *   l3vpn-service/sites/site/site-network-accesses/site-network-access/ip-connection/ipv6/addresses/mask
       [Qin]: Your proposed change is fine to me.

  *   In the filters container/choice, we agreed to add 'when' statements for each case based on the type, to prevent invalid combination of the type and filter lists as in the example I posted.  (Issue 13 from draft-02).
[Qin]: Missed this, will add.

  *   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.

Two new comments:

  1.  Under the cloud-access list, the "authorized-sites" and "denied-sites" have been added back in, in draft-03.  These were previously removed because they duplicate the functionality in the 'list-flavour' choice.
   [Qin]: Removed.

  1.  There are a number of leaves that are marked as mandatory or have defaults, which I think should not be - sorry for not raising this earlier, actually it was only just pointed out to me by someone else:
     *   l3vpn-service/sites/site/security/encryption/layer - this should either be optional, or have a 'when' statement that means it only exists when encryption is enabled.  If encryption is disabled, there is no need to specify the layer.
                    [Qin]:I prefer to make it optional.

     *   l3vpn-service/sites/site/security/encryption/encryption-profile/profile – same [Qin]: Fixed.
     *   l3vpn-service/sites/site/service/qos/qos-profile/qos-profile (and same under site-network-accesses) - I'm not sure why this choice is mandatory, that means one of the options has to be specified for both the site and the site-network-access.  I think the intent is that it is optional in both places, so it can be specified in one or the other.
               [Qin]: Fine with me.

     *   l3vpn-service/sites/site/site-network-accesses/site-network-access/service/multicast/multicast-address-family/ipv[46] - IPv4 has default  true but IPv6 has default false; I think they should both be default false.
             [Qin]: Okay.

Thanks,

    David

-Qin

-----邮件原件-----

发件人: [email-masked]<mailto:[email-masked]> [mailto:[email-masked]]

发送时间: 2017年9月6日 18:18

Revision: 03

Pages:           181

Abstract:

   the text.

The IETF Secretariat

--

David Ball

<[email-masked]><mailto:[email-masked]>

---

From: "Jan Lindblad (jlindbla)" <[email-masked]>
Date: Tue, 12 Sep 2017 06:54:33 +0000

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)

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

---

From: David Ball <[email-masked]>
Date: Tue, 12 Sep 2017 08:37:56 +0100

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
<[email-masked]>

---

From: Benoit Claise <[email-masked]>
Date: Tue, 12 Sep 2017 15:12:52 +0200

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.
>> 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.
>>

---

From: Qin Wu <[email-masked]>
Date: Wed, 13 Sep 2017 10:10:52 +0000

Thank Jan and David for discussing this, I will go with this proposal in the update.

-Qin
发件人: David Ball [mailto:[email-masked]]
发送时间: 2017年9月12日 15:38
收件人: Jan Lindblad (jlindbla); Qin Wu
抄送: l3sm; Benoit Claise (bclaise); adrian
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

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

  *

Best,
/jan

--

David Ball

---

From: Qin Wu <[email-masked]>
Date: Wed, 13 Sep 2017 10:18:58 +0000

Update done now.
https://www.ietf.org/rfcdiff?url1=draft-wu-l3sm-rfc8049bis-03&url2=draft-wu-l3sm-rfc8049bis-04

-Qin
发件人: Benoit Claise [mailto:[email-masked]]
发送时间: 2017年9月12日 21:13
收件人: David Ball; Jan Lindblad (jlindbla); Qin Wu
抄送: l3sm; adrian
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

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.

    David

  *

Best,
/jan

--

David Ball

---

From: Benoit Claise <[email-masked]>
Date: Wed, 13 Sep 2017 15:11:30 +0200

And the IETF LC has been requested.

Regards, Benoit.
> *发件人:*Benoit Claise [mailto:[email-masked]]
> *发送时间:*2017年9月12日21:13
> *收件人:*David Ball; Jan Lindblad (jlindbla); Qin Wu
> *抄送:*l3sm; adrian
> *主题:*Re: [L3sm] New Version Notification for 
> draft-wu-l3sm-rfc8049bis-03.txt
>
> Dear all,
>
>     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
>
>                 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.
>
>         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
>
>     <[email-masked]> <mailto:[email-masked]>
>

---

From: "Jan Lindblad (jlindbla)" <[email-masked]>
Date: Wed, 13 Sep 2017 13:31:36 +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,

Regards, B.

    David

  *

Best,
/jan

---

From: Qin Wu <[email-masked]>
Date: Thu, 14 Sep 2017 01:11:53 +0000

Thank Jan for all YANG related comments, since we are back and forth on choosing between mandatory and optional, definitely some piece in the model need to be cleaned up.
I will address your comments as part of IETF LC review. Thanks again.

-Qin
发件人: Jan Lindblad (jlindbla) [mailto:[email-masked]]
发送时间: 2017年9月13日 21:32
收件人: Benoit Claise (bclaise); Qin Wu
抄送: David Ball -X (daviball - ENSOFT LIMITED at Cisco); l3sm; adrian
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

#1)
579 choice list-flavor

#2)

#3)

#4)

#5)

#6)

#7)

#8)

#9)

#10)

#11)

#12)

#13)

Thanks,
/jan

Dear all,

Regards, B.

    David

  *

Best,
/jan

--

David Ball

---

From: David Ball <[email-masked]>
Date: Thu, 14 Sep 2017 10:09:39 +0100

Looks good, thanks Qin.

     David

-- 
David Ball
<[email-masked]>