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

[Qin Wu <[e-mail]>]

发件人: Jan Lindblad (jlindbla) [mailto:[e-mail]]
发送时间: 2017年9月15日 18:35
收件人: Qin Wu <[e-mail]>
抄送: Benoit Claise (bclaise) <[e-mail]>; Stephane Litkowski <[e-mail]>; Kenichi Ogaki <[e-mail]>; Luis Tomotaki <[e-mail]>; [e-mail]
主题: Re: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

Qin,

Hi, Jan:
Would you like to help me to take a look at my proposed change to address #6) and #13)?

Sure, happy to.



e.g.,
 “
     leaf address-family {
      type address-family;
      description
       "Address family used for management.";
     }
     leaf address {
      type inet:ip-address;
       must "(../address-family)" {
          error-message
            "If address-family is specified, then address should
                        also be specified.If address-family is not specified,
                        then address should also not be specified.";
          description
            " If address-family is specified, then address should
                        also be specified.If address-family is not specified,
                        then address should also not be specified.";
        }
      description
       "Management address.";
     }

”
I am not sure must statement is sufficient in the proposed change to #13,

This must statement ensures that if address is specified, address-family must also be specified. It does not enforce address to be specified if address-family is. For that, you could add another must statement.

[Qin]: Thanks, I think adding must statement to address-family is not needed, since I believe if address-family is specified,  address could be specified or unspecified by customer.
leaf address-family {
  must "../address" { ... }
  ...
}

Another option could be to introduce an optional choice here with one alternative, with both elements mandatory.

choice automatic-or-specific-address {
  case specific-address {
    leaf address-family {
      mandatory true;
      ...
    }
    leaf address {
      mandatory true;
      ...
    }
  }
  description "By default address allocation happens automatically. Customer may override using these settings.";
}

Either way, they would both be optional, but come and go as one unit.

Secondly, for proposed change to #6)
“
     leaf provider-address {
      type inet:ipv4-address;
      description
      "Address of provider side. If provider-address is not specified,
          then mask should not be specified as well,it also implies
          provider-dhcp allocation is not enabled. If provider address
          is specified, then mask may or may not be specified. ";
     }
     leaf mask {
      type uint8 {
      range "0..31";
      }
      description
       "Subnet mask expressed in bits. The value zero
        means unspecified (by the customer)";
     }

”
I am not sure if we add must  statement,e.g., must "(../provider-address)", does this must statement mean mask MUST have a value when provider-address is specified,

By adding the proposed must statement in mask, you will enforce that if a mask is given, the provider address must also be specified. It does not mean that mask must be specified if provider address is. This is probably what you want: they can be both absent, both present or only provider-address present. I would just like the description to be a little more precise, perhaps like this:
description "Subnet mask expressed in bits. If not specified, or specified as zero, this means the customer leaves the actual mask value to the provider.";

[Qin]: Great, make sense to me and will add it.

/jan




发件人: Qin Wu
发送时间: 2017年9月14日 20:18
收件人: 'Jan Lindblad (jlindbla)'
抄送: Benoit Claise (bclaise); Stephane Litkowski; Kenichi Ogaki; Luis Tomotaki; [e-mail]<mailto:[e-mail]>
主题: RE: [L3sm] New Version Notification for draft-wu-l3sm-rfc8049bis-03.txt

Hi, Jan:
Here is the proposed changes to address your comments.
See attached for diff, please also my comments inline below.

-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.
[Qin]: Fixed.
- 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?
[Qin]: By default, all sites in the IP VPN MUST be authorized to access the cloud, I have added a statement in description for choice list-flavor.
#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?
[Qin]:I think l4-src-port and l4-src-port-range/lower-port and higher-port can be set at the same time, if in that case, l4-src-port should not part of l4-src-port-range. When only lower-port is set, it represents a single port, when only upper-port is set, I think we should not support, it is illegal.See my proposed changes in attached diff.
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 ?
[Qin]: Fixed as you suggested.
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?
[Qin]:Fixed as you suggested.
#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')" {
[Qin]: Fixed, thanks.
#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)
[Qin]:I think if provider-address is not specified, the mask should not be specified as well, it also implies
           provider-dhcp allocation is not enabled. If we specify provider-address, mask may or may not be specified.
         See my proposed change, I didn’t add must statement to the mask.
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?
[Qin]: right, how about change uint8 into uint16?
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?
[Qin]: The mask is applied to both provider-address and customer-address. I have added text to clarify this.
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')" {
[Qin]:Fixed.

#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?

[Qin]: If only start address or end-address is specified, it represents a single address. When both start-address and end-address are specified, it implies a range inclusive of both addresses. If no address is specified, it implies customer addresses group is not supported.
#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?
[Qin]: If the provider doesn't allow the customer use this function, the fixed-value will not be set
#13)
2184 address-family and address both optional
What if you set address but not address-family, or vice versa?
[Qin]:I think if address-family is not specified, address should not be specified as well.
If address-family is specified, the address may or may not be specified. See my proposed change in the attached diff.

I did not check any of the examples this time.
[Qin]: Have checking examples. It looks fine.
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]>