Hi,
This is my AD review of draft-ietf-opsawg-vpn-common-08.
Thank you for this document. Again, just minor comments/suggestions.
1.
In section 3. Description of the VPN Common YANG Module
"Encapsulation features such as" -> "Encapsulation features. Such as"
"Routing features such as" -> "Routing features. Such as"
2.
As a very minor comment. Where you have lists (i.e., encapsulation features,
routing features, service type) and particularly because they have references,
it may be slightly easier to read if the list was indented. Also does it make
sense for this list to just be examples, or are they actually normative lists
of service types that are supported?
E.g.,
'service-type': Used to identify the VPN service type.
Examples of supported service types are:
o L3VPN,
o Virtual Private LAN Service (VPLS) using BGP [RFC4761],
o VPLS using Label Distribution Protocol (LDP) [RFC4762],
o Virtual Private Wire Service (VPWS) [RFC8214],
o BGP MPLS-Based Ethernet VPN [RFC7432],
o Ethernet VPN (EVPN) [RFC8365], and
o Provider Backbone Bridging Combined with Ethernet
VPN (PBB-EVPN) [RFC7623].
In the Yang Module:
3. OPSA => OPSAWG
Please can you check if this needs to be fixed for the L3NM YANG model as well.
4.
feature qinany {
description
"Indicates the support of the QinAny encapsulation.";
}
Is there a reference, or perhaps a more detailed description that you can use
here?
5.
Very minor:
feature ipv4, feature ipv6, is it worth adding references to the RFCs? I
appreciate that they are obvious, but since you have references for everything
else, it seems like it might be worth using adding them?
6.
feature rtg-ospf-sham-link {
description
"Indicates support of OSPF sham links.";
Does this mean that feature rtg-ospf excldues this support? This feature seems
very specific relative to the other features in this YANG module.
7.
As mentioned in the other document, would "feature carrierscarrier" be better
as "feature carriers-carrier"?
8.
"Indicates the support of" => "Indicates support for"
"Indicates support of" -> "Indicates support for"
9.
This model defines a lot of features, and I wasn't sure how helpful that will
really be in practice. Is the intention here for an SP to use features to
customize the model to their needs? I wonder if the heavy use of features
won't work so well if both L2VPN and L3VPN's are being modelled and support
different protocols/etc. Will having the common features act as a limitation?
E.g., an alternative might be to express the features in the L2NM and L3NM
models directly allowing them to enabled/disable different features.
10. Status leaves:
Would UP, DOWN, UNKNWON be better as Up, Down, Unknown?
Would "admin-enabled" be better than "admin-up" and "admin-disabled" be better
than "admin-down".
For all the non-base identities, I would suggest removing the "Identity for"
prefix in the descriptions. It is self-evident that the descriptions are for
identities, and the extra words probably would not help a GUI rendering of the
description strings.
11. For all the "service type" identities, I would suggest ending all the
descriptions with "service".
E.g.,
"L3VPN service.";
"Provider Backbone Bridging (PBB) EVPNs service.";
"Virtual Private LAN Service (VPLS).";
"Point-to-point Virtual Private Wire Service (VPWS).";
"Provider Backbone Bridging (PBB) EVPN service.";
"MPLS based EVPN service.";
"VXLAN based EVPN service.";
12.
For the signalling identity descriptions:
"Layer 2 VPNs using BGP signalling";
"Targeted LDP signalling.";
"L2TP signalling.";
13.
For the bgp-signalling identity, does it make sense for the description to be
specific to L2VPNs? Couldn't bgp-signalling also be used for L3VPNs?
14.
For the routing-protocol-type generic identities, would it make sense to add a
"-routing" suffic to them, e.g., "direct-routing", "any-routing"? Otherwise
some of the identity names look fairly generic, but perhaps that is okay.
15.
vpn-topology:
Is No P2P topology identity required?
16.
For identity qos-profile-direction:
Rather than "site-to-wan" and "wan-to-site" identity names, would it be
better to use "vpn" or "service" instead of wan. E.g., "site-to-vpn", or
"site-to-service"?
17.
identity enhanced-vpn
identity ietf-network-slice
Would it be helpful to add RFC or draft references for these identities? E.g.,
[I-D.ietf-teas-enhanced-vpn], or an IETF network slice service
[I-D.ietf-teas-ietf-network-slices]
18.
identity protocol-type {
description
"Base identity for Protocol Type.";
}
identity unknown {
base protocol-type;
description
"Not known protocol type.";
}
Is this identity required/useful? Wouldn't it be better to just leave the leaf
not populated, but in the general case, shouldn't this always be specified?
19.
identity encapsulation-type {
vs identity tag-type
- These seem to both effectively convey similar information.
- c-s-vlan, should probably be s-c-vlan, since it is normal to specify
encapsulation from outermost inner.
20.
identity tf-type
There is no type for unicast. Is it not required?
21.
grouping vpn-description {
Which, if any, of these fields are expected to uniquely identify the VPN?
E.g., what, if any, uniqueness requirements are there for vpn-name?
leaf vpn-id {
type vpn-id;
description
"VPN identifier.
This identifier has a local meaning.";
}
What is meant by "local meaning", could this be clarified?
22. The vpn-id type seems to be used very generically for quite a few different
things, and I was wondering whether having more specific subtypes of vpn-id
might be helpful?
23.
What's the difference between service-timestamp and service-status?
Both include timestamps and status. Perhaps slightly different names for these
groupings might make them more consistent (e.g., oper-service-status,
admin-oper-service-status).
24.
last-updated is okay, but note that ietf-interfaces.yang uses last-changed
instead. Given the description mentions change, last-change might be better?
25.
I'm not sure that the tree diagram examples in Appendix A is actually that
useful, given that they do not represent what the model is now, just how it
used to be. I would suggest keeping the text that justifies the approach taken
but remove the trees.
I also annotated part of the YANG model (just the grouping descriptions) with
comments inline. Please see suggestions on (#RW:) inline in the attached file.
It is up to you whether and how you want to incorporate these and I don't need
to see your response.
Grammar Warnings (by automated tool):
Section: 3, draft text:
For example, diversity or redundancy constraints can be applied on a per group
basis.
Warning: In this context, per-group forms an adjective and is spelled with a
hyphen.
Suggested change: "per-group"
Regards,
Rob
grouping vpn-profile-cfg {
description
"Grouping for VPN Profile configuration.";
container valid-provider-identifiers {
description
"Container for valid provider profile identifiers.";
list external-connectivity-identifier {
Barguil, et al. Expires November 20, 2021 [Page 45]
Internet-Draft VPN Common YANG Model May 2021
if-feature "external-connectivity";
key "id";
description
"List for profile identifiers that uniquely identify profiles
governing how external connectivity is provided to a VPN.
A profile indicates the type of external connectivity
(Internet, cloud, etc.), the sites/nodes that are associated
with a connectivity profile, etc. A profile can also indicate
filtering rules and/or address translation rules. Such
features may involve PE, P, or dedicated nodes as a function
of the deployment.";
leaf id {
type string;
description
"Identification of an external connectivity profile. It has
a local administration meaning.";
#RW:
I think that it would be helpful to clarify what "local administration meaning"
actually means. I.e., does it mean within service provider provisioning the
service, or something more specific?
If the former, then I suggest:
"The profile only has significance within the service providers adminisitrative
domain"
The same comment applies below, where this phrase is used.
}
}
list encryption-profile-identifier {
key "id";
description
"List for encryption profile identifiers.";
leaf id {
type string;
description
"Identification of the encryption profile to be used. It
has a local administration meaning.";
}
}
list qos-profile-identifier {
key "id";
description
"List for QoS Profile Identifiers.";
leaf id {
type string;
description
"Identification of the QoS profile to be used. It has
a local administration meaning.";
}
}
list bfd-profile-identifier {
key "id";
description
"List for BFD profile identifiers.";
leaf id {
type string;
description
"Identification of the BFD profile to be used.
Barguil, et al. Expires November 20, 2021 [Page 46]
Internet-Draft VPN Common YANG Model May 2021
This identifier has a local administration meaning.";
}
}
list forwarding-profile-identifier {
key "id";
description
"List for forwarding profile identifiers.";
leaf id {
type string;
description
"Identification of the Forwrding Profile Filter to be used.
Local administration meaning.";
}
}
list routing-profile-identifier {
key "id";
description
"List for Routing Profile Identifiers.";
leaf id {
type string;
description
"Identification of the routing profile to be used by the
routing protocols within sites, vpn-network-accesses, or
vpn-nodes for refering VRF's import/export policies.
This identifier has a local meaning.";
#RW:
A similar comment as below. Could the description be more precise as to what
is meant by 'local meaning'?
}
}
nacm:default-deny-write;
#RW:
I'm wondering why this specific part of the model has
"nacm:default-deny-write"? Is it more security sensitive that the rest of the
model? Or is the concern that changing the profiles could have a much more
significant impact to services?
}
}
grouping status-timestamp {
description
"This grouping defines some operational parameters for the
service.";
leaf status {
type identityref {
base operational-status;
}
config false;
description
"Operations status.";
}
leaf last-updated {
type yang:date-and-time;
config false;
description
Barguil, et al. Expires November 20, 2021 [Page 47]
Internet-Draft VPN Common YANG Model May 2021
"Indicates the actual date and time of the service status
change.";
}
}
grouping service-status {
description
"Service status grouping.";
container status {
description
"Service status.";
container admin-status {
description
"Administrative service status.";
leaf status {
type identityref {
base administrative-status;
}
description
"Administrative service status.";
}
leaf last-updated {
type yang:date-and-time;
description
"Indicates the actual date and time of the service status
change.";
}
}
container oper-status {
description
"Operational service status.";
uses status-timestamp;
}
}
}
grouping underlay-transport {
description
"This grouping defines the type of underlay transport for the
VPN service. It can include an identifier to an abstract
transport instance to which the VPN is grafted or indicate a
technical implementation that is expressed as an ordered list
of protocols.";
choice type {
description
"A choice based on the type of underlay transport
constraints.";
case abstract {
Barguil, et al. Expires November 20, 2021 [Page 48]
Internet-Draft VPN Common YANG Model May 2021
description
"Indicates that the transport constraint is an abstract
concept.";
leaf transport-instance-id {
type string;
description
"Includes an identifier of an abstract transport instance.";
#RW:
Perhaps:
"An optional identifier of the abstract transport instance.";
}
leaf instance-type {
type identityref {
base transport-instance-type;
}
description
"Indicates a transport instance type. For example, it can
be a VPN+, an IETF network slice, a virtual network, etc.";
}
}
case protocol {
description
"Indicates a list of protocols.";
leaf-list protocol {
type identityref {
base protocol-type;
}
ordered-by user;
description
"Indicates an ordered-by user list of transport protocols.";
#RW:
Perhaps "A client ordered list of transport protocols.";
}
}
}
}
grouping vpn-route-targets {
description
"A grouping that specifies Route Target (RT) import-export rules
used in a BGP-enabled VPN.";
reference
"RFC 4364: BGP/MPLS IP Virtual Private Networks (VPNs)
RFC 4664: Framework for Layer 2 Virtual Private Networks
(L2VPNs)";
list vpn-target {
key "id";
description
"Route targets. AND/OR operations are available
based on the RTs assigment.";
leaf id {
type int8;
description
Barguil, et al. Expires November 20, 2021 [Page 49]
Internet-Draft VPN Common YANG Model May 2021
"Identifies each VPN Target.";
}
list route-targets {
key "route-target";
description
"List of RTs.";
leaf route-target {
type rt-types:route-target;
description
"Conveys an RT value.";
}
}
leaf route-target-type {
type rt-types:route-target-type;
mandatory true;
description
"Import/export type of the RT.";
}
}
container vpn-policies {
description
"VPN service policies. It contains references to the
import and export policies to be associated with the
VPN service.";
leaf import-policy {
type string;
description
"Defines the 'import' policy.";
}
leaf export-policy {
type string;
description
"Defines the 'export' policy.";
#RW:
As mentioned in the L3VPN NM model review, I wasn't sure why these policies are
just strings.
}
}
}
grouping route-distinguisher {
description
"Grouping for route distinguisher (RD).";
choice rd-choice {
description
"Route distinguisher choice between several options
on providing the route distinguisher value.";
case directly-assigned {
description
"Explicitly assign an RD value.";
leaf rd {
Barguil, et al. Expires November 20, 2021 [Page 50]
Internet-Draft VPN Common YANG Model May 2021
type rt-types:route-distinguisher;
description
"Indicates an RD value that is explicitly
assigned.";
}
}
case directly-assigned-suffix {
description
"Explicitly the value of the Assigned Number subfield
of the RD. The Administrator subfield of the RD will
be based on other configuration information such as
router-id or ASN.";
#RW:
I suggest just "The value of just the Assigned ..."
leaf rd-suffix {
type uint16;
description
"Indicates the value of the Assigned Number
subfield that is explicitly assigned.";
}
}
case auto-assigned {
description
"The RD is auto-assigned.";
container rd-auto {
description
"The RD is auto-assigned.";
choice auto-mode {
description
"Indicates the auto-assignment mode. RD can be
automatically assigned either with or without
indicating a pool from which the RD should be
taken.
#RW:
Suggest:
"assinged with or without ...
For both cases, the server will auto-assign an RD
value 'auto-assigned-rd' and use that value
operationally.";
case from-pool {
leaf rd-pool-name {
type string;
description
"The auto-assignment will be made from the pool
identified by the rd-pool-name.";
}
}
case full-auto {
leaf auto {
type empty;
description
"Indicates an RD is fully auto-assigned.";
Barguil, et al. Expires November 20, 2021 [Page 51]
Internet-Draft VPN Common YANG Model May 2021
}
}
}
leaf auto-assigned-rd {
type rt-types:route-distinguisher;
config false;
description
"The value of the auto-assigned RD.";
}
}
}
case auto-assigned-suffix {
description
"The value of the Assigned Number subfield will
be auto-assigned. The Administrator subfield
will be based on other configuration information such as
router-id or ASN.";
container rd-auto-suffix {
description
"The Assigned Number subfield is auto-assigned.";
choice auto-mode {
description
"Indicates the auto-assignment mode of the Assigned Number
subfield. This number can be automatically assigned
either with or without indicating a pool from which
the value should be taken.
#RW:
assigned either with or without => assigned with or without
For both cases, the server will auto-assign
'auto-assigned-rd-suffix' and use that value to build
the RD that will be used operationally.";
case from-pool {
leaf rd-pool-name {
type string;
description
"The assignment will be made from the pool identified
by the rd-pool-name.";
}
}
case full-auto {
leaf auto {
type empty;
description
"Indicates that the Assigned Number is fully auto
assigned.";
}
}
}
leaf auto-assigned-rd-suffix {
Barguil, et al. Expires November 20, 2021 [Page 52]
Internet-Draft VPN Common YANG Model May 2021
type uint16;
config false;
description
"Includes the value of the Assigned Number subfield that
is auto-assigned .";
}
}
}
case no-rd {
description
"Use the empty type to indicate RD has no value and is not to
be auto-assigned.";
leaf no-rd {
type empty;
description
"No RD is assigned.";
}
}
}
}
grouping vpn-components-group {
description
"Grouping definition to assign group-ids to associate VPN nodes,
sites, or network accesses.";
container groups {
description
"Lists the groups to which a VPN node,a site, or a network
access belongs to.";
list group {
key "group-id";
description
"List of group-ids.";
leaf group-id {
type string;
description
"Is the group-id to which a VPN node, a site, or a network
access belongs to.";
}
}
}
}
grouping placement-constraints {
description
"Constraints for placing a network access.";
list constraint {
key "constraint-type";
Barguil, et al. Expires November 20, 2021 [Page 53]
Internet-Draft VPN Common YANG Model May 2021
description
"List of constraints.";
leaf constraint-type {
type identityref {
base placement-diversity;
}
description
"Diversity constraint type.";
}
container target {
description
"The constraint will apply against this list of groups.";
choice target-flavor {
description
"Choice for the group definition.";
case id {
list group {
key "group-id";
description
"List of groups.";
leaf group-id {
type string;
description
"The constraint will apply against this particular
group-id.";
}
}
}
case all-accesses {
leaf all-other-accesses {
type empty;
description
"The constraint will apply against all other network
accesses of a site.";
}
}
case all-groups {
leaf all-other-groups {
type empty;
description
"The constraint will apply against all other groups that
the customer is managing.";
}
}
}
}
}
}
Barguil, et al. Expires November 20, 2021 [Page 54]
Internet-Draft VPN Common YANG Model May 2021
grouping ports {
description
"Choice of specifying a source or destination port numbers.";
choice source-port {
description
"Choice of specifying the source port or referring to a group
of source port numbers.";
container source-port-range-or-operator {
description
"Source port definition.";
uses packet-fields:port-range-or-operator;
}
}
choice destination-port {
description
"Choice of specifying a destination port or referring to a group
of destination port numbers.";
container destination-port-range-or-operator {
description
"Destination port definition.";
uses packet-fields:port-range-or-operator;
}
}
}
grouping qos-classification-policy {
description
"Configuration of the traffic classification policy.";
list rule {
key "id";
ordered-by user;
description
"List of marking rules.";
leaf id {
type string;
description
"An identifier of the QoS classification policy rule.";
}
choice match-type {
default "match-flow";
description
"Choice for classification.";
case match-flow {
choice l3 {
description
"Either IPv4 or IPv6.";
container ipv4 {
description
Barguil, et al. Expires November 20, 2021 [Page 55]
Internet-Draft VPN Common YANG Model May 2021
"Rule set that matches IPv4 header.";
uses packet-fields:acl-ip-header-fields;
uses packet-fields:acl-ipv4-header-fields;
}
container ipv6 {
description
"Rule set that matches IPv6 header.";
uses packet-fields:acl-ip-header-fields;
uses packet-fields:acl-ipv6-header-fields;
}
}
choice l4 {
description
"Includes Layer 4 specific information.
This version focuses on TCP and UDP.";
container tcp {
description
"Rule set that matches TCP header.";
uses packet-fields:acl-tcp-header-fields;
uses ports;
}
container udp {
description
"Rule set that matches UDP header.";
uses packet-fields:acl-udp-header-fields;
uses ports;
}
}
}
case match-application {
leaf match-application {
type identityref {
base customer-application;
}
description
"Defines the application to match.";
}
}
}
leaf target-class-id {
if-feature "qos";
type string;
description
"Identification of the class of service. This identifier is
internal to the administration.";
}
}
}
Barguil, et al. Expires November 20, 2021 [Page 56]
Internet-Draft VPN Common YANG Model May 2021
}
<CODE ENDS>_______________________________________________
OPSAWG mailing list
OPSAWG@ietf.org
https://www.ietf.org/mailman/listinfo/opsawg
