[OPSAWG] Benjamin Kaduk's Discuss on draft-ietf-opsawg-l3sm-l3nm-11: (with DISCUSS and COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-opsawg-l3sm-l3nm-11: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/blog/handling-iesg-ballot-positions/
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-opsawg-l3sm-l3nm/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

(1) I think there may be some ambiguity we need to resolve, relating to
per-AF router IDs and other per-AF lists:

                   list router-id {
                     key "address-family";
                     description
                       "Router-id per address family.";
                     leaf address-family {
                       type identityref {
                         base vpn-common:address-family;
                       }
                       description
                         "Indicates the address family for which the
                          Router-ID applies.";

What actually gets used as the router-id for a given address family if
both "dual-stack" and that address family are present in this list?

There's some similar potential for amiguity in the
"redistribute-connected" list for BGP routing, that is also keyed on an
address-family identityref.

(2) In a similar vein as Roman's Discuss (and perhaps obviated by it?), if
we're going to allow raw keys to be specified, as a string type, we
should be very clear about whether the string is hex-encoded,
base64-encoded, etc., in light of deployed experience with devices that
take the string and use it as the raw key (thereby eliminating a good
chunk of the key space from potential use).

(2.5) For raw keys, should we be using nacm:default-deny-all?

(3) the ipsec authentication option for the various routing protocols
uses a string to identify an (IKE, unspecified version thereof) SA.  RFC
7296 doesn't have the concept of a name for an IKE SA itself, so I think
we need to provide more details on what is being named and what the
naming authority is.  IKE does have identities for the peers, if the
goal is to refer to the peer's identity for the SA.

(4) I'd also like to have a discussion about the NTP configuration
options; in particular, we currently have an enumeration to select
between broadcast client and broadcast server, with no option apparent
for symmetric or other NTP modes.  Given the rigidity of YANG
enumerations, I'd like to confirm that no other NTP modes could be
appropriate on the network access before we lock in to this model.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I expect there's a large degree of personal preference involved, but I
find it easier to read the document when the tree (fragment) precedes
the per-field descriptions; this document has many instances of the
other order (as well as some instances of my preferred order).

Section 1

   The L3NM focuses on BGP Provider Edge (PE) based Layer 3 VPNs as
   described in [RFC4026][RFC4110][RFC4364] and Multicast VPNs as
   described in [RFC6037][RFC6513].

RFC 6037 is Historic; is it nonetheless still a worthwhile reference?

Section 7.6.1

   tunnel selection.  The container can also identify the pseudowire
   (Section 5.2 of [RFC4447]).

RFC 4447 is showing up as obsolete, obsoleted by RFC RFC 8077.

Section 7.6.2

Is the indentation of the tree diagram correct between
"(allocation-type)?" and ":(dhcp-relay)"?  I think that dhcp-relay
should be at the same level as provider-dhcp -- maybe the implicit
'case' around "container provider-dhcp" is confusing the tooling?

Section 7.6.3

        When the IPv4 or dual-stack address-family is requested, it is
        up to the implementation to decide whether OSPFv2 [RFC4577] or

Which implementation?  The service orchestrator's?  (The phrase "up to
the implementation" appears at least one other place, as well.)

     'authentication':  Controls the authentication schemes to be
        enabled for the OSPF instance.  The following options are
        supported: IPsec for OSPFv3 authentication [RFC4552],
        authentication trailer for OSPFv2 [RFC5709] [RFC7474] and
        OSPFv3 [RFC7166].
     [...]
        |     |  +--rw authentication
        |     |  |  +--rw enable?            boolean
        |     |  |  +--rw keying-material
        |     |  |     +--rw (option)?
        |     |  |        +--:(md5)
        |     |  |        |  +--rw md5-keychain?
        |     |  |        |          kc:key-chain-ref
        |     |  |        +--:(ipsec)
        |     |  |           +--rw sa?  string

I don't think "md5" is the right identifier for the node holding the OSPF
authentication-trailer keying material.

Section 7.6.6

Interjecting some (sub-?)sub-subtrees and their fields before completing the
list of nodes in the "toplevel" subtree makes the narrative somewhat
hard to follow.

Section 7.7

   The model supports a single type of tree: Any-Source Multicast (ASM),
   Source-Specific Multicast (SSM), or bidirectional.

It's surprising (to me) to see that the YANG is not a choice, then,
which would be an intuitive fit for "choose exactly one type".
(nit) maybe say single type of tree per VPN instance?

   When a particular VPN using ASM requires a more optimal traffic
   delivery, 'optimal-traffic-delivery' can be set.  When set to 'true',

"optimal traffic delivery" sounds like something that every customer is
going to want ... if that's what it does, why is it even an option? ;)

Section 8

Comments at the end of blocks for what container/etc. is being closed
might help readability.

     typedef area-address {
       type string {
         pattern '[0-9A-Fa-f]{2}(\.[0-9A-Fa-f]{4}){0,6}';

I guess this is related to Alvaro's Abstain, but it is very surprising
that there is not some preexisting definition of IS-IS area-address that
we can import and use.

               leaf ne-id {
                 type string;
                 description
                   "Unique identifier of the network element where the VPN
                    node is deployed.";

When I first saw this I wondered if a union with leafref could be
useful, for cases where there were nodes already identified in a
different data model.  Since this service model operates at a layer
that's rather removed from specific devices, though, it's not really
clear how often such a case would come up in practice.

                   leaf interface-id {
                     type string;
                     description
                       "Identifier for the physical or logical
                        interface.
                        The identification of the sub-interface
                        is provided at the connection and/or IP
                        connection levels.";

(similarly here)

                       container dot1q {
                         when "derived-from-or-self(../type, "
                            + "'vpn-common:dot1q')" {
                           description
                             "Only applies when the type of the
                              tagged interface is 'dot1q'.";
                         }
                         if-feature "vpn-common:dot1q";

The identity itself is conditional on the vpn-common:dot1q feature, so
it's not entirely clear to me if there's value in repeating the
if-feature stanza here.
Likewise for qinq.
(A similar scenario arises for the specific routing-protocols, I think.)

                         leaf type {
                           type identityref {
                             base l2-tunnel-type;
                           }
                           description
                             "Selects the tunnel termiantion option for
                              each vpn-network-access.";
                         }
                         container pseudowire {
                           description
                             "Includes pseudowire termination parameters.";

Why no "when" stanzas for the pseudowire/vpls/vxlan containers, so they
only appear for the relevant tunnel type?

                           choice service-type {
                             description
                               "Choice based on the DHCPv6 service type.";
                             case provider-dhcp-servers {
                               leaf-list server-ip-address {
                                 type inet:ipv6-address;
                                 description
                                   "IPv6 addresses of the provider's
                                    DHCPv6 server.";
                               }
                             }
                             case server {

I don't think I understand what the distinction is supposed to be
between "provider-dhcp-servers" and just "server".  It seems like the
"server" case is still describing a scenario with provider-run DHCP
servers.  There doesn't seem to be much discussion in §7.6.2 around
Figure 12 to help, either.

                         container bgp-timers {
                           description
                             "Includes two BGP timers that can be
                              customized when building a VPN service
                              with BGP used as CE-PE routing
                              protocol.";
                           leaf keepalive {
                             type uint16 {
                               range "0..21845";

Why is the maximum of 0x5555 hardcoded?  RFC 4271 says that "a
reasonable maximum time between KEEPALIVE messages would be one third of
the Hold Time interval", but that seems like general guidance and not a
protocol invariant.

                         leaf ping-reply {
                           type boolean;
                           description
                             "Controls whether the VRRP speaker should
                              answer to ping requests.";

What's the default behavior?

                     container ntp {

It would be great if we could work in an NTS reference somewhere (RFC
8915).

                       leaf remote-source {
                         type boolean;
                         default "false";
                         description
                           "When true, there is no PIM adjacency on
                            the interface.";
                       }

I think some more description would be very helpful here.  E.g., RFC
7761 does not mention "remote" or "adjacency", so it's hard to figure
out where to start reading to understand this configuration.

Section 9

Thank you for calling out the privacy considerations relating to
customer names and IP addresses!

I am happy to see that you already answered Roman's question about the
sensitivity to write operations of vpn-profiles; thanks for that as
well.

Thanks as well for adding the note about MD5 security in response to the
last-call feedback.

I expect that there are some ways in which knowing the internal
configuration of the VPN service would make attacking it easier (knowing
what DHCP addresses to squat on, internal addresses to target with DoS
attack, etc.), but don't see any fundamental or critical aspects that
need to be called out specifically.

Should we mention that the keychain functionality uses NACM to deny
access to the keys, or is that considered "well known" at this point?

Section 11+

Regrettably, I ran out of time when reviewing this document.  So I
didn't really look at the examples or the classification of the
references.

NITS

Section 2

   VPN node:  An abstraction that represents a set of policies applied
      on a PE and that belong to a single VPN service.  A VPN service
      involves one or more VPN nodes.  As it is an abstraction, the
      network controller will take on how to implement a VPN node.  For
      example, typically, in a BGP-based VPN, a VPN node could be mapped
      into a Virtual Routing and Forwarding (VRF).

I don't recall seeing "VRF" used in this manner before; I feel like I
ususally see "instance" or some other word after it.

Section 4

   L3VPN service.  For example, the customer may supply an IP
   Connectivity Provisioning Profile (CPP) [RFC7297], an enhanced VPN
   (VPN+) service [I-D.ietf-teas-enhanced-vpn], or an IETF network slice
   service [I-D.ietf-teas-ietf-network-slices].

I'm having a hard time finding a parallel structure in "customer may
supply [a profile], [a service], or [a service]".  Are the latter two
items intending to refer to a description or instance of the service in
question?

   Note also that both the L3SM and the L3NM may be used in the context
   of the Abstraction and Control of TE Networks (ACTN) [RFC8453].

I would expect to see the word "Framework" after ACTN.

Section 7.2

   each VPN service provider.  The model only includes an identifier to
   these profiles in order to ease identifying and binding local

The RFC Editor will no doubt have suggestions here as well, but I think
"ease identification and binding of" or "facilitate identifying and
binding" would flow better here.

Section 7.6

   'vpn-instance-profile':  Provides a pointer to an active VPN instance
      profile at the VPN node level.  Referencing an active VPN instance
      profile implies that all associated data nodes will be inherited
      by the VPN network access.  However, some inherited data nodes
      (e.g., multicast) can be refined at the VPN network access level.
      In such case, refined values take precedence over inherited ones.

IIUC this is not the YANG "refine" directive, so maybe a different word
like "overridden" at the VPN network access level, and "local values
take precedence over inherited ones", is better?

Section 7.6.3

     'max-prefix':  Indicates the maximum number of BGP prefixes
        allowed in the BGP session.  If the limit is reached, the
        action indicated in 'action-violate' will be followed.

s/action-violate/violate-action/ to match the actual model

Section 8

     identity discard {
       base local-defined-next-hop;
       description
         "Indicates an action to discard traffic for the
          corresponding destination.
          For example, this can be used to blackhole traffic.";

I expect that the RFC Editor is going to flag "blackhole" and ask if you
really want to use it, on the inclusive-terminology front.

     grouping vpn-instance-profile {
       description
         "Grouping for data nodes that may be factorized
          among many levels of the model. The grouping can
          be used to define generic profiles at the VPN service
          level and then called at the VPN node and VPN network
          access levels.";

I'm not sure whether "called" is a conventional word for this behavior;
would "referenced" be accurate?

                       leaf type {
                         type identityref {
                           base vpn-common:encapsulation-type;
                         }
                         default "vpn-common:priority-tagged";
                         description
                           "Tagged interface type. By default, the type of
                            the tagged interface is 'priority-tagged'.";

Given that there's a "vpn-common:untagged-int" identity that is usable
here, is "Tagged interface type" really 100% accurate?

                           "Defines a layer 2 tunnel termination.
                            It is only applicable when a tunnel is
                            required. The supported values are:
                            pseudowire, VPLS and, VXLAN. Other
                            values may defined, if needed.";

"may be defined"

                         leaf prepend-global-as {
                           type boolean;
                           default "false";
                           description
                             "In some situations, the ASN that is
                              provided at the VPN node level may be
                              distinct from the one configured at the
                              VPN network access level. When such
                              ASNs are provided, they are both
                              prepended to the BGP route updates
                              for this access. To disable that
                              behavior, the prepend-global-as
                              must be set to 'false'.  [...]

It's surprising to see "when such ASNs are provided, they are both
prepended [...] to disable that behavior" when the default is "false".

                             enum active {
                               description
                                 "Interface sends or receives IS-IS
                                  protocol control packets.";

Maybe s/or/and/?

                     }
                     container encryption-profile {
                       when "../encryption/enabled = 'true'" {
                         description
                           "Indicates the layer on which encryption
                            is enabled.";

This description stanza is for the "when" directive, and doesn't seem
appropriate for that action.

                         }
                         leaf max-entries {
                           type uint32;
                           description
                             "Indicates the maximum MLD entries.";

"maximum number of"