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

[Qin Wu <[e-mail]>]

Hi, David:
Good catch, please see my reply inline.
发件人: L3sm [mailto:[e-mail]] 代表 David Ball
发送时间: 2017年10月9日 21:51
收件人: Qin Wu
抄送: l3sm; Jan Lindblad (jlindbla)
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt


Hi Qin,

A few minor comments on the changes in the latest version:

  1.  In the flow definition grouping, the 'must' statements that have been added for l4-src-port, l4-src-port-range/lower-port and l4-src-port-range/upper-port mean, I think, that if all three of these exist, they must all have the same value (since l4-src-port <= lower-port, lower-port <= upper-port and upper-port <= l4-src-port).  Was that the intent?  Or was the 'must' statement on l4-src-port supposed to have an 'or' instead of an 'and' in it?  Given that (according the the description for l4-src-port-range) when only the lower-port is present, it represents a single port, perhaps the separate l4-src-port leaf is not needed and could be removed?
[Qin]:The intent is to use ‘or’ instead of ‘and’. l4-src-port-range  focuses on continuous range. So we can keep l4-src-port, l4-src-port-range coexist, but we should not allow l4-src-port overlap with l4-src-port-range.
For example, If We set l4-src-port as 20, set l4-src-port-range as (15,30).  This setting is not allowed. So if l4-src-port coexist with l4-src-port-range, the value of l4-src-port should be less than lower-port of l4-src-range(i.e., 15) or Greater than upper-port of l4-src-port-range(i.e.,30), Does this make sense?

  1.  Same applies to dst-port
[Qin]:Fixed.

  1.  The range for guaranteed-bw-percent is given as 0..255 - if this is a percentage, shouldn't the range be 0..100?
             [Qin]:Okay, we can limit the range to 0..100.

  1.  In grouping site-attachment-ip-connection, I think the 'must' statement for leaf ipv4/address-allocation-type has been inverted.  It used to say (in draft-04): "current() != 'slaac' and current() != 'provider-dhcp-slaac'", which was the correct logic since SLAAC doesn't apply to IPv4.  Now (in draft-05) it says: "derived-from-or-self(current(), 'l3vpn-svc:slaac') or derived-from-or-self(current(), 'l3vpn-svc:provider-dhcp-slaac')" - which I think means the value for IPv4 must be slaac or provider-dhcp-slaac.  I don't think that's the intent!
[Qin]: You are right,  my proposed change is:
NEW TEXT:
“
    must "derived-from-or-self(current(), 'l3vpn-svc:slaac')!='true' or "+
        "derived-from-or-self(current(), 'l3vpn-svc:provider-dhcp-slaac')!='true'" {
      error-message "SLAAC is only applicable to IPv6";
     }
”
Thanks,


    David

On 16/09/2017 04:14, Qin Wu wrote:
Update is done based on your comments:
https://www.ietf.org/rfcdiff?url2=draft-wu-l3sm-rfc8049bis-05

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

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:[e-mail]]
发送时间: 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

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

<[e-mail]><mailto:[e-mail]>






_______________________________________________

L3sm mailing list

[e-mail]<mailto:[e-mail]>

https://www.ietf.org/mailman/listinfo/l3sm



--

David Ball

<[e-mail]><mailto:[e-mail]>