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

[David Ball <[e-mail]>]

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?
 2. Same applies to dst-port
 3. The range for guaranteed-bw-percent is given as 0..255 - if this is
    a percentage, shouldn't the range be 0..100?
 4. 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!


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]>; Benoit Claise 
> (bclaise) <[e-mail]>
> *抄送:*l3sm <[e-mail]>; adrian <[e-mail]>; David Ball -X 
> (daviball - ENSOFT LIMITED at Cisco) <[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]
> https://www.ietf.org/mailman/listinfo/l3sm

-- 
David Ball
<[e-mail]>