A Common YANG Data Model for Layer 2 and Layer 3 VPNs
draft-ietf-opsawg-vpn-common-12

[S3, question]

* Should "ipv6/ttl" actually be "ipv6/hoplimit"?

  I'm not sure what might have been defined in other YANG documents
  elsewhere, but "ttl" is not the term normally associated with IPv6
  (8200s3).

* Should "ipv6/protocol" actually be "ipv6/nextheader"?

  The field in the header is actually "Next Header" (8200s3), but
  is the intention here to identify the next "logical higher-layer
  protocol", i.e. skipping over other headers that might be in
  between the IPv6 header and, say, a TCP header?

* How does "private or public cloud" count as "external connectivity"?

  Seems like the referenced 4364s11 is primarily concerned with
  Internet access.  I guess it seems odd to me to consider a VPN
  endpoint in a cloud instance especially different from any other
  (e.g., physical) endpoint...

** The following YANG items would benefit from references: 

-- Section 6.  feature qinany.
-- Section 6.  feature bearer-reference.  Perhaps RFC8049?
-- Section 6.  feature fast-reroute.  Perhaps RFC6714?
-- Section 6.  identity control-mode. 

** Section 6.  identity customer-application.  It is unclear what taxonomy is guiding this list or how this will be used.  A few common user applications that didn’t seem to fit into the existing categories included: printing, version control,  proxies, name/directory/auth services (e.g., DNS, LDAP, Kerberos, AD; is that network management?),

** Section 6.  identity embb, urllc and mmtc.  These appear to be the 5G key words.  If that’s the intent, cite it as such.  The current text of “… demands network performance with a  wide variety of characteristics, such as data rate, latency,  loss rate, reliability, and many other parameters” doesn’t explain anything.

** Section 6.  feature rtg-isis.  Typo. s/routeing/routing/

Thank you for this document.

Also thanks to Tim for the OpsDir review.

The version -11 of this document addresses, my discuss points. Thanks for addressing the issue. 


Thanks to the authors for working on this specifications and addressing TSVART review comments. Thanks Wesley Eddy for your TSVART reviews.

Comments -

* In this specification, UDP match criteria is described and claimed that the model can be augmented to include more L4 transport protocols. QUIC (RFC9000) is a new L4 transport protocol and uses UDP as substrate. For such L4 transport protocols, it might be ambiguous to apply qos classification policy based on what is defined here. In case of QUIC, it needs to identify from other UDP traffic that is traversing the network. Read more on QUIC traffic identification here ( https://quicwg.org/ops-drafts/draft-ietf-quic-manageability.html#name-identifying-quic-traffic).

I think this specification should consider such potential substrate usage of L4 protocols (specially UDP) and hint on the potential augmentations (there might be several ways to do that) or scope it down to not support such cases.

* May be the commented text in the section 4 for protocol identifiers should be updated to reflect what is describes in the section 3 for "underlay-transport". Section 3 talks about underlay transports and how they are set.

As I am abroad on vacations, I had not time to review in depth this document, hence I am trusting the Internet directorate Last-Call review by Suresh:
https://datatracker.ietf.org/doc/review-ietf-opsawg-vpn-common-09-intdir-lc-krishnan-2021-08-30/

I was about to post similar comments as Erik Kline's ones about the lack of IPv6 terminology in the classification but Med's reply is OK. May I suggest though to clearly reference RFC 8519 (which should be updated) where this topic is discussed ? I also appreciate the reuse of the (incomplete) ACL YANG modules.

Regards

-éric

I have a bold proposal for consideration: since we are defining a new
common set of groupings for VPN and, in some cases, proposing to rename
existing terminology already, can we consider moving away from the term
"VPN" itself?  The word "private" is ambiguous as to what level of
privacy is being provided (e.g., network routing vs cryptographic) and
who the conveyed content is to be private from.  A term like "virtual
network" removes that ambiguity, and lets us use the explicit attributes
to convey whether encryption is in use when appropriate.

There is no particular triggering point (at least, not yet) at which it
becomes clearly correct to make a breaking change like this, so we may
end up just having to pick a time somewhat arbitrarily, if we are to
make such a change at all.  Perhaps now is that time; perhaps not.

Section 3

          grouping vpn-profile-cfg
            +-- valid-provider-identifiers
               +-- external-connectivity-identifier* [id]
               |       {external-connectivity}?
               |  +-- id?   string

Presumably I am just misreading RFC 8340, but I thought that the list
key was implicitly mandatory, so it would appear as just "id" (as
opposed to "id?").  (Many instances.)

   'vpn-route-targets':
      *  A YANG grouping that defines Route Target (RT) import/export
         rules used in a BGP-enabled VPN (e.g., [RFC4364][RFC4664]).

On very quick skim, it's not clear what motivates the RFC 4664
reference -- "route target" does not appear, for example, nor does
"import" or "export".

Section 4

     identity pim-proto {
       if-feature "pim";
       base routing-protocol-type;

This is in the section with the group-management-protocols; is
"routing-protocol-type" really the intended base?

     identity embb {
     [...]
     identity urllc {
     [...]
     identity mmtc {

Similar to Roman's comment, a reference seems useful for these.
If we intend to implicitly rely on RFCs 8299 and/or 8466 for
terminology, we should say so in the terminology section.

       leaf vpn-id {
         type vpn-common:vpn-id;
         description
           "A VPN identifier that uniquely identifies a VPN.
            This identifier has a local meaning, e.g., within
            a service provider network.";

Thank you for indicating the scope of validity of the identifier!
(Elsewhere as well.)

     grouping service-status {
       [...]
           leaf last-change {
             type yang:date-and-time;
             description
               "Indicates the actual date and time of the service status
                change.";

What's the motiviation behind leaving this as "config true"?  When would
it be useful to set it manually?

       list vpn-target {
         [...]
         leaf id {
           type int8;
           description
             "Identifies each VPN Target.";

I wasn't able to find the motivation for limiting to int8 in RFC 4364,
though I mostly assume I'm just looking in the wrong place.
(But why *signed* int8?)

Section 5

While there may be no direct security considerations from what is
effectively just defining some new compound types for reuse, I think we
may want to consider some privacy considerations.  For example, we have
the "customer-name" field in the vpn-description, and it is sometimes
the case that customers want to remain confidential.  Thus, any
instantiations of the grouping would incur a potential need for
confidentiality and minimizing the scope of distribution.

NITS

Section 4

     feature bearer-reference {
       description
         "Indicates support for the bearer reference access constraint.
          That is, the reuse of a network connection that was already
          ordered to the service provider apart from the IP VPN site.";

I echo Roman's comment that this feature would benefit from a reference
or further discussion; the current description leaves me unclear on what
is meant and with no recourse for learning more.  (RFCs 4026 and 4176
are presented as generic references for VPN-related terms, but do not
cover the concept of "bearer reference".)

       reference
         "I-D.ietf-teas-ietf-network-slice-framework:
            Framework for IETF Network Slices";

draft-ietf-teas-ietf-network-slice-framework is replaced by
draft-ietf-teas-ietf-network-slices.

     identity rsvp-te {
       base protocol-type;
       description
         "Transport is based on RSVP-TE.";
       reference
         "RFC 3209: RSVP-TE: Extensions to RSVP for LSP Tunnels";

Is the transport itself RSVP-TE, or is it that RSVP-TE is used to
provision the tunnels used as transport?  (Similar question for bgp-lu?)

     identity dot1q {
       if-feature "dot1q";
       base encapsulation-type;
       description
         "Dot1q encapsulation.";

I suppose the feature declaration does so, but maybe some "802" is in
order here as well?

     identity ethernet-type {
       base encapsulation-type;
       description
         "Ethernet encapsulation type.";
     }

     identity vlan-type {
       base encapsulation-type;
       description
         "VLAN encapsulation.";
     }

     identity untagged-int {
       base encapsulation-type;
       description
         "Untagged interface type.";
     [...]

Should we be more consistent about whether the description ends in
"type"?

     identity lag-int {
       if-feature "lag-interface";
       base encapsulation-type;
       description
         "LAG interface type.";
       reference
         "IEEE Std. 802.1AX: Link Aggregation";

lag-int is the only encapsulation-type identify element that has a
reference stanza.  We did mention LAG in the context of 802.1AX earlier,
so maybe it's not needed?

     identity web {
       base customer-application;
       description
         "Web applications (e.g., HTTP, HTTPS).";
     [...]
     identity file-transfer {
       base customer-application;
       description
         "File transfer application (e.g., FTP, SFTP).";

Maybe we could just list HTTPS and SFTP as the (respective) examples, in 2021?

       leaf vpn-name {
         type string;
         description
           "A name used to refer to the VPN.";

Should we say a little more about how the name differs from the id,
given that both are "type string"?

         leaf import-policy {
           type string;
           description
             "Defines the 'import' policy.";
         }
         leaf export-policy {
           type string;
           description
             "Defines the 'export' policy.";

Does it "define" or merely "identify" the policy?  Naively, a
"definition" would be a rather long string...

     grouping vpn-components-group {
       [...]
       container groups {
         description
           "Lists the groups to which a VPN node,a site, or a network

space after comma.

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term "traditional"; alternatives might be "classic", "classical",
   "common", "conventional", "customary", "fixed", "habitual", "historic",
   "long-established", "popular", "prescribed", "regular", "rooted",
   "time-honored", "universal", "widely used", "widespread".

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 3. , paragraph 62, nit:
> or the QinAny encapsulation. The outer outer VLAN tag is set to a specific va
>                                  ^^^^^^^^^^^
Possible typo: you repeated a word.

Document references draft-ietf-opsawg-l3sm-l3nm-10, but -11 is the latest
available revision.

Document references draft-ietf-opsawg-l2nm-04, but -06 is the latest available
revision.

Document references draft-ietf-teas-ietf-network-slice-framework-00, but -04 is
the latest available revision.