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

[David Ball <[e-mail]>]

On 10/10/2017 11:00, Qin Wu wrote:
>
> 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?
>

[DB]
Yes, that makes sense.  Should they also be strict inequalities, or is 
it ok if l4-src-port is equal to lower-port?

>  2. Same applies to dst-port
>
> [Qin]:Fixed.
>
>  3. 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.
>
>  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!
>
> [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'" {
>

[DB]
I think you mean "and" here not "or". :)  I'm not an XPath expert but I 
expect you can write:

     must "!derived-from-or-self(current(), 'l3vpn-svc:slaac') and 
!derived-from-or-self(current(), 'l3vpn-svc:provider-dhcp-slaac')" {


     David


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

-- 
David Ball
<[e-mail]>