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

[[e-mail]]

Hi Ben, 

Focusing on the COMMENT part. All good points. Many thanks for the time and the effort put into looking this.  

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk via Datatracker [mailto:[e-mail]]
> Envoyé : jeudi 23 septembre 2021 10:45
> À : The IESG <[e-mail]>
> Cc : [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Objet : Benjamin Kaduk's Discuss on draft-ietf-opsawg-l3sm-l3nm-11:
> (with DISCUSS and COMMENT)
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-opsawg-l3sm-l3nm-11: Discuss
> 
> ----------------------------------------------------------------------
> 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).

[[Med]] Rearranged the figures for consistency. Went with the one where the trees precede the description. Hope that will be better. 

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

[[Med]] Yeah, please refer to the discussion in Section 6.3.

> 
> 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.

[[Med]] Agree. It was already fixed when addressing Lars review.  

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

[[Med]] This is a bug in Figure 12 when stripping manually the tree to fit the allowed max line length. Fixed to be similar to what we have in Figure 11. Thank you for catching this. 
	
> 
> 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.)

[[Med]] It is the network orchestrator. Updated the text to say: " It is up to each implementation (typically, the
   network orchestrator shown in Figure 1)".

Also updated other places to provide "network orchestrator as an example". 

> 
>      '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.

[[Med]] You are completely right. This is a bug when inserting the subtree. Updated the tree to reflect the yang module. 

> 
> 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.

[[Med]] Fair. Added some sub-sections to better structure the text and (hopefully) ease the reading. 

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

[[Med]] I forgot the context why we have a leaf-list here rather than a list, but in any case we don't need to have a choice. Will think more about why we have a leaf-list and adjust accordingly. It is really appreciated to have fresh eyes to look into documents. So, thanks.  

> 
>    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? ;)

[[Med]] ... because there are features to be enabled and may be paid :-)

These techniques are hinted in 

   When a particular VPN using ASM requires a more optimal traffic
   delivery, 'optimal-traffic-delivery' can be set.  When set to 'true',
   the implementation must use any mechanism to provide a more optimal
   traffic delivery for the customer.  For example, anycast is one of
   the mechanisms to enhance RPs redundancy, resilience against
   failures, and to recover from failures quickly.

Appropriate data nodes are supported in the module (anycast, for example). 

Please note that we are echoing what is the service request (L3SM, excerpt from RFC8299, page 13):

       |     |     |     +--rw provider-managed
       |     |     |     |  +--rw enabled?                    boolean
       |     |     |     |  +--rw rp-redundancy?              boolean
       |     |     |     |  +--rw optimal-traffic-delivery?   boolean

> 
> 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.

[[Med]] Actually, we are using exactly the same typedef as in draft-ietf-isis-yang-isis-cfg:

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

Importing the full device module just to use this typedef is suboptimal.


> 
>                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.

[[Med]] We spent many cycles on a similar concern but with vpn-node-id. We used to have an union in previous version. For example, draft-ietf-opsawg-l3sm-l3nm-05:

            leaf vpn-node-id {
              type union {
                type vpn-common:vpn-id;
                type uint32;
              }
              description
                "Type STRING or NUMBER Service-Id.";
            }


But we decided to remove the union thing after checking with other implementers involved in the work. The reasoning is that we don't need this complexity to accommodate one single implementation limitation. Having an id structured as string is sufficient. 


> 
>                    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)

[[Med]] Idem as above. 

> 
>                        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.)

[[Med]] This is to avoid misuses when the dot1q is inserted while the type is not set to dot1q (even if the feature is supported). 

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

[[Med]] Because we missed it. Fixed now in my local copy. Thanks.

> 
>                            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.

[[Med]] This was a copy/paste issue. As in the IPv4 case, the service type can be relay or server. Changed "provider-dhcp-servers" to "relay"/

> 
>                          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.

[[Med]] Agree but we do also receive many comment to align as much as possible with is supported by device module. For this particular case we are trying to align as much as we can with draft-ietf-idr-bgp-model, which includes the following:

       leaf keepalive {
         type uint16 {
           range "0..21845";
         }
         units "seconds";

> 
>                          leaf ping-reply {
>                            type boolean;
>                            description
>                              "Controls whether the VRRP speaker should
>                               answer to ping requests.";
> 
> What's the default behavior?

[[Med]] Added a default set to "flase" to align with the accept-mode in RFC5798:

       The default is False.
       Deployments that rely on, for example,
       pinging the address owner's IPvX address
       may wish to configure Accept_Mode to
       True.

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

[[Med]] Sure. Added this note to also cite RFC8633:

NEW:
   [RFC8633] describes best current practices to be considered in VPNs
   making use of NTP.  Moreover, a mechanism to provide cryptographic
   security for NTP is specified in [RFC8915].


> 
>                        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.

[[Med]] Ack. Will udpate this.

> 
> 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.

[[Med]] Indeed. 

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

[[Med]] I'm afraid this is redundant with with the NACM text.  

> 
> 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.

[[Med]] Both are used. For example, you can read the following in RFC8299: 

   The route distinguisher (RD) is a critical parameter of PE-based
   L3VPNs as described in [RFC4364] that provides the ability to
   distinguish common addressing plans in different VPNs.  As for route
   targets (RTs), a management system is expected to allocate a VRF on
   the target PE and an RD for this VRF.                      


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

[[Med]] Actually, all of these variants describe the requirements of a requested connectivity service. All of them can be assimilated to service requests.    

> 
>    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.

[[Med]] OK.

> 
> 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.

[[Med]] Fixed. Thanks. 

> 
> 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,

[[Med]] Fixed

 and "local values
> take precedence over inherited ones", is better?

[[Med]] went with "adjusted ..." instead of "local". Local may be confusing. 

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

[[Med]] Fixed. 

> 
> 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.

[[Med]] Seems this one wasn't catched by Lars checker :-) 

I don't have a better term right now. Will leave this for the RFC Editor. 

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

[[Med]] Went with "referenced".

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

[[Med]] Agree. Changed to "Encapsulation type".

> 
>                            "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"

[[Med]] Already fixed as part of Lars's review. Thanks. 

> 
>                          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".

[[Med]] Setting this to false will have an effect to avoid that both are prepended. That’s the expected behavior. 

> 
>                              enum active {
>                                description
>                                  "Interface sends or receives IS-IS
>                                   protocol control packets.";
> 
> Maybe s/or/and/?

[[Med]] I prefer the OLD. 

> 
>                      }
>                      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.

[[Med]] Indeed. Fixed. 

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

[[Med]] Fixed. Thanks. 

FWIW, the changes to address your review can be seen at: https://github.com/IETF-OPSAWG-WG/lxnm/tree/master/I-D-L3NM 


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.