Re: [OPSAWG] Last Call: <draft-ietf-opsawg-l3sm-l3nm-10.txt> (A Layer 3 VPN Network YANG Model) to Proposed Standard

[[e-mail]]

Hi Tom, 

Thank you for the review. An updated version that takes into account many of your comments can be tracked at: https://tinyurl.com/l3nm-latest (or in the datatracker when the revised version is public) 

You did already raised the comment about ipv4/ipv6/dual-stack identities during the WGLC. We clarified the intended behavior at the time. I don't see any new element in your comment on this particular point, but I understand that you want to increase awareness so that others may look at this. This is appreciated.

As mentioned in the IETF LC of the vpn-common, this identity is used to characterize the connectivity type provided by the VPN service. We are not manipulating AFIs/SAFIs as those will be derived when translating into specific ** device modules ** (which we aren't as the L3NM is a network model (as per RFC8969)) from the VPN service type and the address family. We added a note about this.  

Selecting the routing protocol version or whether two sessions or one single session will be enabled (BGP case, for example) is deployment-specific. For example, when dual-stack is indicated in the L3NM, whether both OSPFv2 and OPSFv3 instances will be activated or only OSPFv3 is used (i.e., also announce IPv4 routes as per RFC5838) is a function of the underlying device capabilities and local guidelines.  

"dual-stack" identity was added to ** rationalize the module and factorize items ** that are applicable for both IPv4 and IPv6. With this value, we can have more compact network service definition. For example, with the current specification we can express something such as (only an excerpt from the full body):

==
              "address-family": [
                {
                  "address-family": "vpn-common:dual-stack",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  }
                }
              ]
==

While the following would be needed to convey the same configuration if dual-stack identity is not supported. 

==
              "address-family": [
                {
                  "address-family": "vpn-common:ipv4",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  }
                },
                {
                  "address-family": "vpn-common:ipv6",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  }
                }
              ]
==

The dual-stack identity is also useful when a policy is to be set for both IPv4 and IPv6 as ** a set **. For example, the current module can control the maximum number of routes that will apply independently of the address family. We don't require a 50%-50% split between the two as soon as the sum does not exceed the indicated value. As such, we can indicate something such as: 

==
              "address-family": [
                {
                  "address-family": "vpn-common:dual-stack",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  },
                  "maximum-routes": [
                    {
                      "protocol": "vpn-common:any",
                      "maximum-routes": 100
                    }
                  ]
                }
              ]
==

Absent that identity, only strict maximums are possible such as in this example: 

==
              "address-family": [
                {
                  "address-family": "vpn-common:ipv4",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  },
                  "maximum-routes": [
                    {
                      "protocol": "vpn-common:any",
                      "maximum-routes": 50
                    }
                  ]
                },
                {
                  "address-family": "vpn-common:ipv6",
                  "vpn-targets": {
                    "vpn-target": [
                      {
                        "id": "1",
                        "route-targets": [
                          "0:65550:1"
                        ],
                        "route-target-type": "both"
                      }
                    ]
                  },
                  "maximum-routes": [
                    {
                      "protocol": "vpn-common:any",
                      "maximum-routes": 50
                    }
                  ]
                }
              ]
==

The same reasoning applies for various data nodes in the module for which address family is indicated.

Otherwise, please see inline. 

Cheers,
Med

-----Message d'origine-----
De : tom petch [mailto:[e-mail]] 
Envoyé : jeudi 29 juillet 2021 13:13
À : [e-mail]
Cc : [e-mail]; [e-mail]; [e-mail]
Objet : Re: Last Call: <draft-ietf-opsawg-l3sm-l3nm-10.txt> (A Layer 3 VPN Network YANG Model) to Proposed Standard

On 16/07/2021 17:05, The IESG wrote:
>
> The IESG has received a request from the Operations and Management 
> Area Working Group WG (opsawg) to consider the following document: - 
> 'A Layer 3 VPN Network YANG Model'
>    <draft-ietf-opsawg-l3sm-l3nm-10.txt> as Proposed Standard
>
> The IESG plans to make a decision in the next few weeks, and solicits 
> final comments on this action. Please send substantive comments to the 
> [e-mail] mailing lists by 2021-08-06. Exceptionally, 
> comments may be sent to [e-mail] instead. In either case, please 
> retain the beginning of the Subject line to allow automated sorting.

This I-D includes a YANG module which provides configuration for BGP, OSPF, IS-IS, PIM, IGMP, VRRP, RIP, BFD, MLD, NTP, DHCP and such like and, as such, overlaps substantially with existing RFC and I-D.  It does not build on that work, rather it takes a different direction, which seems likely to induce misunderstandings.  At WG LC, the shepherd suggested that with so much coverage of VPN, then IETF LC should be flagged to the BESS WG so that they could review it; I see no sign of that having happened.  By the same token, I think that that suggestion could apply to several other WG.  Thus the coverage of BGP here seems at variance with much if not most of bgp-model.

There is often a need in YANG to differentiate between this version, that version or both, which is modelled as a base YANG identity from which more specific ones are derived, using YANG's function 'derived-from-or-self' when a combination is wanted. Not here.  Here there are three identity for e.g. ipv4; one for ipv4 alone, one for ipv6 alone and 'dualstack' for when both versions are present.  Thus 'ipv6' 
has a different meaning to any other YANG module I know, meaning 'ipv6-without-ipv4', and a test for ipv6 becomes
     when "../address-family = 'vpn-common:ipv6' or "
               + "'vpn-common:dual-stack'" { while a test for ipv4 is
     when "../protocol-type = 'host' and "
              + "../address-family = 'vpn-common:ipv4' or "
              + "'vpn-common:dual-stack'";

The concept of dualstack will be familiar to those implementing hosts with IPv6 support but is not something I have ever seen elsewhere and may be unfamiliar to most, at least with this meaning as opposed to support of other related protocols perhaps outside ther remit of the IETF.

Likewise, this use of address family, with different semantics, is inconsistent with its use in such as BGP and RFC8349 (inter alia). 
Unwise IMHO.

I see this use of dualstack and address family as major flaws, show stoppers.

For OSPF, other documents derive ospfv2 and ospfv3 from a base ospf identity and use that to specify ospfv2 or ospfv3, the natural way to model the OSPF protocols; here, OSPF is differentiated by 'address-family' i.e. by ipv4 or ipv6 or dualstack, not something I have ever seen with OSPF. What does it mean to activate IPv4, as specified here?  OSPFv2 or OPSFv3 or both? If OSPFv3 is used to convey a  IPv4 
prefix, is that ospf/ipv4 or ospf/ipv6 or ospf/dualstack?   The security 
options, in 'container keying-material' are at variance with those in draft-ietf-ospf-yang in 'container authentication', similar but different.  Therein, 'case ipsec' is OPSFv3 only but there is no way to specify that here with its concept of an OSPF address family.

The I-D defines a type for 'area-address' but this is not the one used by OSPF protocols.

[[Med]] Sure, the document does not say that. FWIW, here is what we have for OSPF:

        |     +--rw ospf {vpn-common:rtg-ospf}?
        |     |  +--rw address-family?   identityref
        |     |  +--rw area-id           yang:dotted-quad


That type is used for isis: 

        |     +--rw isis {vpn-common:rtg-isis}?
        |     |  +--rw address-family?   identityref
        |     |  +--rw area-address      area-address


RIP, likewise, is diffentiated by address-family rather than by version.
'leaf flush-interval' here defaults to 180, which is invalid as it is not greater than the 'invalid-interval'; it is 240 in RFC8695.

[[Med]] Good catch. Fixed. Thanks. 

With BGP it seems more accurate to say that most objects here differ from their potential equivalents in bgp-yang, either in identifier, in description or in semantics. BGP, RFC4271, defines holdtime and keepalive - not here, where it is hold-time (with different semantics to RFC4271)

[[Med]] This is the same name used in draft-ietf-idr-bgp-model:

         |  |  +--rw timers
         |  |  |  +--rw connect-retry-interval?             uint16
         |  |  |  +--rw hold-time?                          uint16


 and keepalive while 'address-family' has a narrower meaning than would be expected for BGP.  There appears to be no way to configure the BGP identifier (for me, the first step).

[[Med]] We didn't receive such request from the operators as this can be handled by the controller. Of course, it is completely fine to have in the device module. Why would such identifier be required in a network model? Thanks.


  The local address does not allow configuration of the port.

[[Med]] We are assuming that the default port number is used. We didn't receive any request to control that as part of the service/network module. Please note that even the current BGP device module does not allow for such as you can see in this subtree (this is a read-only data node):  

         +--rw neighbors
         |  +--rw neighbor* [remote-address]
         |  |  +--ro local-address?               inet:ip-address
         |  |  +--ro local-port?                  inet:port-number
         |  |  +--rw remote-address               inet:ip-address
         |  |  +--ro remote-port?                 inet:port-number
         |  |  +--ro peer-type?                   bt:peer-type 


 'neighbor' configuration does not allow for identifier or port.

[[Med]] Idem as above. 

  Here 'multihop' is configured as
     leaf multihop {
       type uint8;
          description
            "Describes the number of IP hops allowed
            between a given BGP neighbor and the PE."; where draft-idr-bgp model has
    leaf multihop-ttl {
       type uint8;
          description
               "Time-to-live value to use when packets are sent to the
                 referenced group or neighbors and ebgp-multihop is enabled"; which I find clearer.

[[Med]] We are reasoning in term of IP hops, not decremented "TIME to leave", as indicated in this part from RFC4271

==
      3) When sending a message to an external peer X, and the peer is
         multiple IP hops away from the speaker (aka "multihop EBGP"):       
==

The limit on prefixes here is specified in container bgp-max-prefix as "It allows to control how many prefixes can be received from a neighbor."
while draft-idr-bgp-model has
"Maximum number of prefixes that will be accepted from the neighbour"
which I find more precise.

[[Med]] The description also says "Indicates the maximum number of BGP prefixes allowed in the BGP session." That's said, will see how to tweak the text for better clarity. 

 The point where the limit is applied matters; currently, there is a discussion in the IDR WG about specifying two different limits, one for inbound and one for outbound, a change which seems likely to be needed.

More significantly, here restart-interval is
    leaf restart-interval {
      type uint16;
       units "minutes";
while bgp-model has
          leaf restart-timer {
            type uint32;
            units "seconds";
a difference in scale. Here the action is
     leaf warning-threshold
         type decimal64 {
            fraction-digits 5;
              range "0..100";    }
                           units "percent"; whereas the bgp model has
          leaf shutdown-threshold-pct {
            type rt-types:percentage;
a difference in approach

[[Med]] The requirement we had is to customize the behaviour as agreed/set with the customer. We prefer to keep what we have in the current spec. 


leaf prepend-global-as
controls the prepend of a second AS, the one for the node in addition to the one for VPN network access level.

bgp-model has
       container set-as-path-prepend {
         description "Action to prepend local AS number to the AS-path a
            specified number of times";
which is, well, different.

[[Med]] Sure, but these are different features. For example, the one in the L3NM is what can be tweaked in https://www.juniper.net/documentation/en_US/junos/topics/topic-map/bgp-local-as.html (other vendors can be cited), while the one you quoted from the bgp-model is more https://www.juniper.net/documentation/us/en/software/junos/routing-policy/topics/concept/policy-adding-as-numbers-to-bgp-as-paths.html (other vendors can be cited). 

Here the specification of the holdtime only caters for its value once the session is established; as RFC4271 says, it may be set to a larger value during session establishment

Compared to bgp-model there is an additional security option,
      case explicit {
which I do not understand but would appear to have a key of type string which sounds like an unmentioned security vulnerability.

[[Med]] Hmm. This is covered by this part:

   Several data nodes ('bgp', 'ospf', 'isis', 'rip', and 'bfd') rely
   upon [RFC8177] for authentication purposes.  Therefore, this module
   inherits the security considerations discussed in Section 5 of
   [RFC8177].

And RFC8177 says: 

   When configured, the key strings can be encrypted using the AES Key
   Wrap algorithm [AES-KEY-WRAP].  The AES key-encryption key (KEK) is
   not included in the YANG model and must be set or derived independent
   of key chain configuration.  When AES key encryption is used, the
   hex-key-string feature is also required since the encrypted keys will
   contain characters that are not representable in the YANG string
   built-in type [YANG-1.1].  It is RECOMMENDED that key strings be
   encrypted using AES key encryption to prevent key chains from being
   retrieved and stored with the key strings in cleartext.  This
   recommendation is independent of the access protection that is
   availed from the NETCONF access control model (NACM) [NETCONF-ACM].

Away from BGP, 'router-id' is imported from 'module ietf-routing-types' 
with the appropriate semantics and there is a definition of router-id as a 32 bit number in s.7.5 but then a second router-id leaf is created with the wrong semantics.

[[Med]] This is to cover when/if router-ids are provided as IPv6 addresses. We do have text explaining this in the draft.  

  The need for a second concept, an addressible one (which 'router-id' is not), has arisen before and draft-ietf-ospf-yang defines such an object and gave it a different name, 'te-rid', TE being one, but not the only, use thereof. Different semantics call for a different name; that name seems suitable for all uses if that is what is wanted here. (The I S I S yang module uses te-rid as well as ipv4-router-id,  ipv6-router-id).


When a range is specified, in YANG, which may be a single address, that case is commonly modeled by specifying 'start' = 'end'.  Here it is modelled with only the start-address with the presence of the end-address implying a range but with no YANG constraint on the value of end-address vis-a-vis start address; that seems unnecessarily flexible.

[[Med]] We are inhering that from RFC8299. 


MD5 is part of the security options but is not mentioned in the Security Considerations; for the NTP yang data model, it was flagged as deprecated while the TLS WG is seeking to eliminate its use.

[[Med]] Already added a new sentence to cover. Please refer to the secdir list. 


Other minor comments

   typedef area-address {
...
     description       "This type defines the area address format.";
Well, worth adding this is not for OSPF

Where 'start' and 'end' are specified, it is customary to impose a YANG constraint of 'start>end' or 'start>=end'

[[Med]] Noted. Will check and fix if it makes sense. Thanks. 


         leaf dr-priority {
...
                                      Numerically larger DR priority allows
              a node to be elected as a DR."; Well, any value allows election; perhaps, as draft-ietf-pim-yang, puts it "A larger value has a higher priority over a smaller value.";

[[Med]] That's better, indeed. Fixed. Thank you. 


         leaf update-interval {
...
       "Indicates the RIP update time.
        That is, the amount of time for which routing updates are sent."; Perhaps "Interval at which RIPv2 or RIPng updates are sent."; as per rtgwg-yang-rip

[[Med]] The OLD text is OK, but we made this change s/routing updates/RIP updates


                     leaf signalling-type {
                         enum ldp {
...
                              In this
                              case, an IGP routing protocol must
                              also be activated."; Probably 'MUST' and likewise for 'enum bgp'


                 container service {
                   leaf mtu {
...
                                      If CsC is enabled,
                        the requested MTU will refer
                        to the MPLS MTU and not to the IP MTU."; MPLS does not really do MTU; mpls-base-yang uses 'leaf maximum-labeled-packet' after a discussion about this issue.


[[Med]] Thanks for pointing to this. Adjusted accordingly. 

CsC should appear in terminology.

[[Med]] OK. Fixed. Thanks again for the review. 

_________________________________________________________________________________________________________________________

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.