Early Review of draft-ietf-l3sm-l3vpn-service-model-06

Request Review of draft-ietf-l3sm-l3vpn-service-model-16
Requested rev. 16 (document currently at 19)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2017-02-09
Requested 2017-02-09
Requested by Mehmet Ersue
Authors Stephane Litkowski, Luis Tomotaki, Kenichi Ogaki
Draft last updated 2017-03-06
Completed reviews Genart Last Call review of -16 by Brian Carpenter (diff)
Genart Telechat review of -17 by Brian Carpenter (diff)
Secdir Last Call review of -16 by Hilarie Orman (diff)
Opsdir Last Call review of -16 by Nevil Brownlee (diff)
Rtgdir Early review of -16 by Les Ginsberg (diff)
Yangdoctors Early review of -06 by Giles Heron (diff)
Assignment Reviewer Giles Heron 
State Completed
Review review-ietf-l3sm-l3vpn-service-model-06-yangdoctors-early-heron-2017-03-06
Reviewed rev. 06 (document currently at 19)
Review result Ready
Review completed: 2017-03-06


Some comments from a YANG-doctor perspective

a couple of meta-issues first - these are perhaps things the YANG Doctors need to discuss amongst themselves.

1) I tend to the view that service models should be augments of the I2RS network and topology model rather than being standalone models. 

2) I wonder if the IETF should really be focussing on the interface between the “service component” and the “config component” in your draft.  at that layer we can look at e.g. route targets and route distinguishers so we can focus on IETF technologies, but still take a network and service-centric view rather than a device-centric one.    The true technology-neutral “service” layer is being worked on in e.g. MEF - certainly for L2 and I think for L3 also.   Sure, in the L3 case we could probably argue it’s more IETF’s expertise than MEF’s, but I think there’s still value in having both layers, and I think we do need to make sure IETF and MEF don’t tread on each others’ toes too much.

In terms of more specific comments on the model:

3) the address-families could possibly use an enum rather than identities (as hopefully v4 and v6 is all we’ll ever have!)

4) again if you think the site-vpn-flavors, transport constraints, management types, address allocation types, vpn-topologies, multicast tree types, multicast rp discovery types etc. are definitive lists you could use enums for them.

5) re the site-roles we only really need hub and spoke if we have any-to-any as a default.

6) for the cloud-access perhaps the if-feature should be at the cloud-accesses container level.  given the indentation I suspect you added the container later?

7) for the authorized/denied sites there might be a better way to do that (using a choice - so you have either one list or the other).

8) nat-enabled could maybe be a presence container with customer-nat-address as an optional leaf inside it (since the customer nat address only applies if nat is enabled)

9) again the multicast container inside the vpn-service-multicast could be a presence container indicating that multicast is enabled and then have other various containers as sub-containers within that.

10) tree-flavor could be a leaf-list (it only has one leaf in it).

11) again provider-managed could be a presence container instead of having an enabled leaf.  that also avoids the when statements for the other leaves.

12) bsr-candidate could be a leaf-list (only one leaf in the list).

13) you could use an empty presence container rather than a leaf for carrrierscarrier.

14) the customer location info feels like it may be at too high a level for a service model.

15) do you need two levels of container in the site-diversity grouping?

16) the groups container is used in both site-diversity and access-diversity.  So you could use a grouping for it.   In fact you could probably just have the group list and constraint list - there’s less need for an enclosing container for a list when the list is already embedded in a container.

17) it might be possible to pull in the flow matching definitions from another model - they seem fairly generic.

18) the fast-reroute stuff could again use a presence container.

19) site-security-authentication looks kind of empty.

20) site-security-encryption could again use a presence container instead of an enabled leaf.

21) I’m guessing the layer should be a mandatory leaf for encryption and have a default?

22) I’m guessing the mask for the static address case ought to be in the range 0..31 for IPv4 and 0..127 for IPv6 (as you can’t have a /32 with 2 addresses in it).

23) again for BFD you could use a presence container

24) the container "vpn-policy-list" should perhaps be called “vpn-policies”.

25) the list “entries” should probably be called “entry”.

26) ipv4-lan-prefixes and ipv6-lan-prefixes should probably be leaf-lists and should probably have singular names.

27) the site-role for a VPN site could be made optional if the default was any-to-any (so you’d only need a role for hubs and spokes).

28) shouldn’t the multicast traffic constraints have a leaf-list for the dst-site rather than just a leaf? (you’d most likely want the same constraint for multiple destinations).

29) vpn-svc should probably be called vpn-service for consistency with the enclosing container.