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

["Jan Lindblad (jlindbla)" <[e-mail]>]

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