Subject: [OPSAWG] AD review of draft-ietf-opsawg-l3sm-l3nm-09

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Mon, 12 Jul 2021 12:13:09 +0000

Hi,

Sorry for the delay in the review (it is a long draft).  The review of the common l2vpn module will follow later today.

I believe that this work will be really helpful for SPs modelling their networks and hence I would like to thank the authors, shepherd, and WG for the effort that they have put into this document.

Most of my comments on this draft are either minor, or editorial in nature.  I've listed the minor questions first, for which a response would be useful, and then the editorial/grammar suggestions are at the end, and in the attached copy of the draft (labelled with #RW:).

	4.  L3NM Reference Architecture

	   The terminology from [RFC8309] is introduced to show the distinction
	   between the customer service model, the service delivery model, the
	   network configuration model, and the device configuration model.  In
	   that context, the "Domain Orchestration" and "Config Manager" roles
	   may be performed by "Controllers".
   
1. Service delivery model doesn't seem to be included in figure 1, and doesn't appear to be referenced further.  Does it need to be mentioned at all?

	7.3.  VPN Services

	   The 'vpn-service' is the data structure that abstracts a VPN service
	   in the service provider network.  Each 'vpn-service' is uniquely
	   identified by an identifier: 'vpn-id'.  Such 'vpn-id' is only
	   meaningful locally within the network controller.  The subtree of the
   
2. Why limit the vpn-id to the network controller?  Presumably, an implementation could allow these identifiers to be unique within the SP's management network (e.g., perhaps by using a network controller specific prefix)?

	7.4.  VPN Instance Profiles

                |     |  |  +--rw vpn-policies
                |     |  |     +--rw import-policy?   string
                |     |  |     +--rw export-policy?   string

3. Is it right that import-policy and export-policy are plain strings?

	7.6.  VPN Network Access

				+--rw vpn-network-access* [id]
				   +--rw id                         vpn-common:vpn-id
				   +--rw port-id?                   vpn-common:vpn-id

4. I'm surprised that port-id is of type vpn-common:vpn:id
Also, is the name port-id better than interface-id?  E.g., if the connectivity was not via a physical port?  I think that the description also defines this as an interface rather than a port.

      'multipoint':  Represents a broadcast connection between the
         endpoints.  The controller must keep the association between a
         logical or physical interface on the device with the 'id' of
         the 'vpn-network-access'.

5. Is broadcast the right description here?

YANG Model:

  identity port-id {
    base bearer-inf-type;
    description
      "Identity for the priority-tagged interface.";
  }
  
6. Is this description right?  Is port-id automatically priority-tagged?

  identity lag-id {
    base bearer-inf-type;
    description
      "Identity for the lag-tagged interface.";
  }
  
7. Is "lag-tagged" right, should this be the just "Identity for LAG interface"?

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

8. This looks like hex can be entered in case insensitive way.
Is this value ever compared?  Perhaps define a canonical form?

  grouping vpn-instance-profile {
   ...
      leaf address-family {
        type identityref {
          base vpn-common:address-family;
        }
        description
          "Indicates the address family (IPv4 or IPv6).";

9. Should this be (IPv4 and/or IPv6)? I.e., is 'dual-stack' an allowed identity here?

                      leaf tag-type {
                        type identityref {
                          base vpn-common:tag-type;
                        }
                        default "vpn-common:c-s-vlan";
                        description
                          "Tag type. By default, the tag type is
                           'c-s-vlan'.";

10. What is meant by c-s-vlan?
Does this mean an outer S-VLAN with an inner C-VLAN?  I would normally expect this to be described as s-c-vlan (i.e., starting with the outermost tag first).

                      container pseudowire {
                        description
                          "Includes pseudowire termination parameters.";
                        leaf vcid {
                          type uint32;
                          description
                            "Indicates a PW or VC identifier.";
                        }
                        leaf far-end {
                          type union {
                            type uint32;
                            type inet:ip-address;
                          }
                          description
                            "Neighbor reference.";
							
11. What does it mean when a uint32 is used for the far end?  Should the description cover this?

                    leaf address-allocation-type {
                      type identityref {
                        base address-allocation-type;
                      }
                      must "not(derived-from-or-self(current(), "
                         + "'slaac') or derived-from-or-self(current(),"
                         + " 'provider-dhcp-slaac'))" {
                        error-message
                          "SLAAC is only applicable to IPv6.";
                      }
                      description
                        "Defines how addresses are allocated to the
                         peer site.

                         If there is no value for the address
                         allocation type, then IPv4 addressing is not
                         enabled.";
                    }

12. An alternative, possible cleaner approach, could have been to make both the ipv4 and ipv6 containers to have presence, and then make the address-allocation-type leaf to be mandatory.

                          description
                            "Choice based on the DHCP service type.";
                          case relay {
                            when "./dhcp-service-type = 'relay'";

13. It is slightly strange to have a case statement with a when statement, normally I would expect to see a model use either of these approaches rather than both.  Perhaps the choice and case statements are not required?

                                  list address-pool {
                                    key "pool-id";
                                    description
                                      "Describes IP addresses to be
                                       allocated by DHCP.

                                       When only start-address or only
                                       end-address is present, it
                                       represents a single address.

                                       When both start-address and
                                       end-address are specified, it
                                       implies a range inclusive of both
                                       addresses.";

14. Why not make start-address mandatory, and require that is always specified, e.g., for a single address.

                        list address {
                          key "address-id";
                          description
                            "Lists the IPv4 addresses that are used.";
                          leaf address-id {
                            type string;
                            description
                              "An identifier of the static IPv4
                               address.";
                          }
                          leaf customer-address {
                            type inet:ipv4-address;
                            description
                              "IPv4 address at the customer side.";
                          }
						  
15. I was slightly surprised that the customer-address isn't the list key, that would allow the address-id to be optional extra information rather than a list key.

                          leaf next-hop {
                            type union {
                              type inet:ip-address;
                              type predefined-next-hop;
                            }
                            description
                              "The next-hop that is to be used for the
                               static route. This may be specified as
                               an IP address, an interface, or a
                               pre-defined next-hop type (e.g.,
                               discard or local-link).";
                          }
						  
16. The description states that an interface can be specified as the next hop.  Is this correct?

                      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 set to
                           'true', this parameter prevents that
                           the ASN provided at the VPN node
                           level is also prepended to the BGP
                           route updates for this access.";
                      }
17. Is this description correct?  I.e., I don't understand the "prevent that ..." part.

                        leaf violate-action {
                          type enumeration {
                            enum warning {
                              description
                                "Only a warning message is sent to
                                 the peer when the limit is
                                 exceeded.";
                            }
                            enum discard-extra-paths {
                              description
                                "Discards extra paths when the
                                 limit is exceeded.";
                            }
                            enum restart {
                              description
                                "Restarts after a time interval.";
								
18. What is meant by restarts?  Should this have a more detailed description?

                        leaf inbound-rate-limit {
                          type decimal64 {
                            fraction-digits 5;
                            range "0..100";
                          }
                          units "percent";
                          description
                            "Specifies whether/how to rate-limit the
                             inbound traffic matching this QoS policy.
                             It is expressed as a percent of the value
                             that is indicated in 'input-bandwidth'.";

19. I note that inbound and outbound are used here, but input/output bandwidth were used previously.  Is input and inbound applying to traffic flowing in the same direction?

                  container carrierscarrier {

20. Is carrierscarrier as one word normal, would carriers-carrier, be better as a leaf name?

21. In the examples
             "address-allocation-type": "static-address
             "address-allocation-type": "ietf-l3vpn-ntw:static-address
Does static address need a namespace qualifier?  Both are valid, but I would suggest using the simple form (and being consistent in the examples).

22. I note that you have some references in the YANG model, but potentially, it might be worth looking to see if there are any extra references that could be added, although I will leave this to your discretion.

Finally, I have a few forms of mostly editorial suggestions.  I don't need to know whether you choose to incorporate these:

(i) I have attached an annotated a version of the draft with some suggested editorial rewording of some descriptions in the YANG model (the questions above are also included and can be ignored.  You can see these in the attached file with "#RW:".

(ii) I have also run a spelling/grammar tool over the draft (which doesn't currently check the YANG module).  The warnings that it flagged up are below (they may not all be correct).

Spelling typos:,
crietria,
ilustrate,
rouing,

Grammar Warnings:
Section: 1, draft text:
Some of the information captured in the L3SM can be passed by the orchestrator in the L3NM (e.g., customer) or be used to feed some of the L3NM attributes (e.g., actual forwarding policies). 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "Some"

Section: 1, draft text:
Some of the information captured in the L3SM can be passed by the orchestrator in the L3NM (e.g., customer) or be used to feed some of the L3NM attributes (e.g., actual forwarding policies). 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "some"

Section: 1, draft text:
Some of the information captured in L3SM may be maintained locally within the orchestrator; which is in charge of maintaining the correspondence between a customer view and its network instantiation. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "Some"

Section: 1, draft text:
Likewise, some of the information captured and exposed using the L3NM can feed the service layer (e.g., capabilities) to drive VPN service order handling, and thus the L3SM.
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "some"

Section: 2, draft text:
The service orchestrator is responsible of the Customer Edge (CE) - Provider Edge (PE) attachment circuits, the PE selection, and requesting the VPN service to the network controller. 
Warning:  The usual collocations for "responsible" is "responsible for". Did you mean responsible for or in charge of?
Suggested change:  "responsible for"

Section: 7.2, draft text:
An external connectivity may be an access to the Internet or a restricted connectivity such as access to a public/private cloud. 
Warning:  Uncountable nouns are usually not used with an indefinite article. Use simply access.
Suggested change:  "access"

Section: 7.5, draft text:
However, the model also allows to point to an abstract node. 
Warning:  Did you mean pointing? Or maybe you should add a pronoun? In active voice, 'allow' + 'to' takes an object, usually a pronoun.
Suggested change:  "pointing"

Section: 7.5, draft text:
The structure of 'active-vpn-instance-profiles' is the same as the one discussed in [ie_profiles] with the exception of 'router-id'. 
Warning:  Consider using except or except for
Suggested change:  "except"

Section: 7.6, draft text:
However, some of the inherited data nodes (e.g., multicast) can be refined at the VPN network access level. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "some"

Section: 7.6.2, draft text:
To identify which of the addresses is the primary address of a connection ,the 'primary-address' reference MUST be set with the corresponding 'address-id'.
Warning:  Put a space after the comma, but not before the comma.
Suggested change:  ","

Section: 7.6.3, draft text:
The type of a routing instance is indicated in 'type'. 
Warning:  If 'type' is a classification term, 'a' is not necessary. Use type of. (The phrases 'kind of' and 'sort of' are informal if they mean 'to some extent'.)
Suggested change:  "type of"

Section: 7.6.3, draft text:
The values of this attributes are those defined in [I-D.ietf-opsawg-vpn-common] ('routing-protocol-type' identity).
Warning:  Did you mean these?
Suggested change:  "these"

Section: 7.6.3, draft text:
Local policies of a service provider (e.g., filtering) will be implemented as part of the device configuration; these are not captured in the L3NM, but the model allows to associate local profiles with routing instances ('routing-profiles').
Warning:  Did you mean associating? Or maybe you should add a pronoun? In active voice, 'allow' + 'to' takes an object, usually a pronoun.
Suggested change:  "associating"

Section: 7.6.3, draft text:
- The module adheres to the recommendations in Section 13.2 of [RFC4364] as it allows to enable TCP-AO [RFC5925] and accommodates the installed base that makes use of MD5. 
Warning:  Did you mean enabling? Or maybe you should add a pronoun? In active voice, 'allow' + 'to' takes an object, usually a pronoun.
Suggested change:  "enabling"

Section: 7.6.3, draft text:
- The model ([vrrp]) allows to enable VRRP on the 'vpn-network-access' interface. 
Warning:  Did you mean enabling? Or maybe you should add a pronoun? In active voice, 'allow' + 'to' takes an object, usually a pronoun.
Suggested change:  "enabling"

Section: 9, draft text:
The YANG module specified in this document defines schema for data that is designed to be accessed via network management protocols such as NETCONF [RFC6241] or RESTCONF [RFC8040] . 
Warning:  Don't put a space before the full stop.
Suggested change:  "."

Section: 9, draft text:
Some of the readable data nodes in this YANG module may be considered sensitive or vulnerable in some network environments. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "Some"

Thanks,
Rob

---

From: <[email-masked]>
Date: Mon, 12 Jul 2021 17:56:53 +0000

Re-,

Many thanks for the review. A candidate version can be tracked at: https://tinyurl.com/l3nm-latest.

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Rob Wilton (rwilton) [mailto:[email-masked]]
> Envoyé : lundi 12 juillet 2021 14:13
> À : [email-masked]
> Cc : [email-masked]
> Objet : AD review of draft-ietf-opsawg-l3sm-l3nm-09
> 
> Hi,
> 
> Sorry for the delay in the review (it is a long draft).  The review
> of the common l2vpn module will follow later today.
> 
> I believe that this work will be really helpful for SPs modelling
> their networks and hence I would like to thank the authors,
> shepherd, and WG for the effort that they have put into this
> document.
> 
> Most of my comments on this draft are either minor, or editorial in
> nature.  I've listed the minor questions first, for which a response
> would be useful, and then the editorial/grammar suggestions are at
> the end, and in the attached copy of the draft (labelled with #RW:).
> 
> 
> 1. Service delivery model doesn't seem to be included in figure 1,

[Med] This is supposed to be covered by "network model" in the figure. Updated the figure with an explicit mention.  

> and doesn't appear to be referenced further.  Does it need to be
> mentioned at all?
> 
> 
> 	7.3.  VPN Services
> 
> 2. Why limit the vpn-id to the network controller?  Presumably, an
> implementation could allow these identifiers to be unique within the
> SP's management network (e.g., perhaps by using a network controller
> specific prefix)?

[Med] Agree. Made this change: s/meaningful locally (e.g., within the network controller)

> 3. Is it right that import-policy and export-policy are plain

[Med] That usage is correct as we define vpn-id in the common draft as a generic identifier. That is why we mention "service identifier" as an example. We can make that more clearer in the common I-D.

> 4. I'm surprised that port-id is of type vpn-common:vpn:id

[Med]  We don't require any structure for the identification of the port; hence the use of the string (vpn-common:vpn-id) type.

 Also, is
> the name port-id better than interface-id?  E.g., if the
> connectivity was not via a physical port?  I think that the
> description also defines this as an interface rather than a port.

[Med] The details about logical interfaces is covered in the connection and IP connection (e.g., lx-termination-point).  

> 5. Is broadcast the right description here?

[Med] Updated to: "Represents a multipoint connection between the customer site and the PEs".

> 6. Is this description right?  Is port-id automatically priority-

[Med] What is meant here is defining an identity for priority-tagged interface. Please note that we removed the bearer-inf-type as it is not used in the module.  

> 7. Is "lag-tagged" right, should this be the just "Identity for LAG

[Med] Yes, that's better. Please note that we removed the bearer-inf-type as it is not used in the module.

> Is this value ever compared?  Perhaps define a canonical form?

[Med] Actually, we aligned with the device module draft-ietf-isis-yang-isis-cfg (currently in the RFC Editor Queue). 

> 9. Should this be (IPv4 and/or IPv6)? I.e., is 'dual-stack' an

[Med] It is. This is used in some examples: 

==
                     "address-family": [
                       {
                         "address-family": "vpn-common:dual-stack",
==

Updated to "(IPv4 and/or IPv6)". 

> Does this mean an outer S-VLAN with an inner C-VLAN?  I would
> normally expect this to be described as s-c-vlan (i.e., starting
> with the outermost tag first).

[Med] Yes. Updated to align with the candidate common module. 

> the description cover this?

[Med] That covers the case in https://datatracker.ietf.org/doc/html/rfc4447#section-5.2 

> 12. An alternative, possible cleaner approach, could have been to
> make both the ipv4 and ipv6 containers to have presence, and then
> make the address-allocation-type leaf to be mandatory.
> 

[Med] Agree. We prefer to use this one for consistency with other checks in the module.  

> 13. It is slightly strange to have a case statement with a when
> statement, normally I would expect to see a model use either of
> these approaches rather than both.  Perhaps the choice and case
> statements are not required?

[Med] We used a choice as it more directive about which data nodes to include as a function of the type. 

> specified, e.g., for a single address.

[Med] Agree. Fixed.

> 15. I was slightly surprised that the customer-address isn't the
> list key, that would allow the address-id to be optional extra
> information rather than a list key.

[Med] "id" is used to ease referring to an address in other places when appropriate (e.g.., acls). 

> next hop.  Is this correct?

[Med] Good catch. Fixed. 

> 17. Is this description correct?  I.e., I don't understand the

[Med] Changed to: 

                          "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 'true'. In such a case,
                           the ASN that is provided at 
                           the VPN node level is not prepended to 
                           the BGP route updates for this access.";

Better?

> 18. What is meant by restarts?  Should this have a more detailed

[Med] Updated to: "The BGP session restarts after a time interval."

> 19. I note that inbound and outbound are used here, but input/output
> bandwidth were used previously.  Is input and inbound applying to
> traffic flowing in the same direction?

[Med] Yeah. Joe raised a similar comment, but we maintain "input" to ease the mapping with the L3NM. This would ease building L3SM-bis to make use of the common module if such bis is to be worked out in the future. 
> be better as a leaf name?

[Med] Agree. Fixed. 

>              "address-allocation-type": "ietf-l3vpn-ntw:static-
> address Does static address need a namespace qualifier?  Both are
> valid, but I would suggest using the simple form (and being
> consistent in the examples).
> 

[Med] Both are allowed as per RFC7951:

"Otherwise, both the simple and namespace-
   qualified forms are permitted."

But I agree it is better to be consistent. 

> 
> 22. I note that you have some references in the YANG model, but
> potentially, it might be worth looking to see if there are any extra
> references that could be added, although I will leave this to your
> discretion.
> 
> 
> Finally, I have a few forms of mostly editorial suggestions.  I
> don't need to know whether you choose to incorporate these:
> 
> (i) I have attached an annotated a version of the draft with some
> suggested editorial rewording of some descriptions in the YANG model
> (the questions above are also included and can be ignored.  You can
> see these in the attached file with "#RW:".

[Med] Considered almost all your embedded comments. 

> 
> (ii) I have also run a spelling/grammar tool over the draft (which
> doesn't currently check the YANG module).  The warnings that it
> flagged up are below (they may not all be correct).
>

[Med] Thanks. Addressed almost all of them. 

> Spelling typos:,
> crietria,
> ilustrate,
> rouing,
> 
> 
> Grammar Warnings:
> Section: 1, draft text:
> Some of the information captured in the L3SM can be passed by the
> orchestrator in the L3NM (e.g., customer) or be used to feed some of
> the L3NM attributes (e.g., actual forwarding policies).
> Some of the information captured in the L3SM can be passed by the
> orchestrator in the L3NM (e.g., customer) or be used to feed some of
> the L3NM attributes (e.g., actual forwarding policies).
> Some of the information captured in L3SM may be maintained locally
> within the orchestrator; which is in charge of maintaining the
> correspondence between a customer view and its network
> instantiation.
> Likewise, some of the information captured and exposed using the
> L3NM can feed the service layer (e.g., capabilities) to drive VPN
> service order handling, and thus the L3SM.
> The service orchestrator is responsible of the Customer Edge (CE) -
> Provider Edge (PE) attachment circuits, the PE selection, and
> requesting the VPN service to the network controller.
> Warning:  The usual collocations for "responsible" is "responsible
> for". Did you mean responsible for or in charge of?
> An external connectivity may be an access to the Internet or a
> restricted connectivity such as access to a public/private cloud.
> Warning:  Did you mean pointing? Or maybe you should add a pronoun?
> In active voice, 'allow' + 'to' takes an object, usually a pronoun.
> The structure of 'active-vpn-instance-profiles' is the same as the
> one discussed in [ie_profiles] with the exception of 'router-id'.
> Warning:  Consider using except or except for Suggested change:
> "except"
> 
> To identify which of the addresses is the primary address of a
> connection ,the 'primary-address' reference MUST be set with the
> corresponding 'address-id'.
> Warning:  If 'type' is a classification term, 'a' is not necessary.
> Use type of. (The phrases 'kind of' and 'sort of' are informal if
> they mean 'to some extent'.) Suggested change:  "type of"
> 
> The values of this attributes are those defined in [I-D.ietf-opsawg-
> vpn-common] ('routing-protocol-type' identity).
> Local policies of a service provider (e.g., filtering) will be
> implemented as part of the device configuration; these are not
> captured in the L3NM, but the model allows to associate local
> profiles with routing instances ('routing-profiles').
> Warning:  Did you mean associating? Or maybe you should add a
> pronoun? In active voice, 'allow' + 'to' takes an object, usually a
> pronoun.
> - The module adheres to the recommendations in Section 13.2 of
> [RFC4364] as it allows to enable TCP-AO [RFC5925] and accommodates
> the installed base that makes use of MD5.
> Warning:  Did you mean enabling? Or maybe you should add a pronoun?
> In active voice, 'allow' + 'to' takes an object, usually a pronoun.
> Warning:  Did you mean enabling? Or maybe you should add a pronoun?
> In active voice, 'allow' + 'to' takes an object, usually a pronoun.
> The YANG module specified in this document defines schema for data
> that is designed to be accessed via network management protocols
> such as NETCONF [RFC6241] or RESTCONF [RFC8040] .
> Some of the readable data nodes in this YANG module may be
> considered sensitive or vulnerable in some network environments.

_________________________________________________________________________________________________________________________

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.

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 13 Jul 2021 12:51:31 +0000

Hi Med,

Thanks for the quick reply.

A few comments inline ...

> -----Original Message-----
> From: [email-masked]
> <[email-masked]>
> Sent: 12 July 2021 18:57
> To: Rob Wilton (rwilton) <[email-masked]>; draft-ietf-opsawg-l3sm-
> [email-masked]
> Cc: [email-masked]
> Subject: RE: AD review of draft-ietf-opsawg-l3sm-l3nm-09
> 
> Re-,
> 
> [Med] This is supposed to be covered by "network model" in the figure.
> Updated the figure with an explicit mention.

Ok.

> [Med] Agree. Made this change: s/meaningful locally (e.g., within the

Ok.

> [Med] That usage is correct as we define vpn-id in the common draft as a
> generic identifier. That is why we mention "service identifier" as an example.
> We can make that more clearer in the common I-D.

Yes, making it clearer in the common I-D, will be helpful.

> [Med]  We don't require any structure for the identification of the port;
> hence the use of the string (vpn-common:vpn-id) type.

Okay, my point is that the model isn't just saying that it is a string (since it is using the type vpn-id) but is using a type with more meaning assigned to it.  I.e., I would expect that a vpn-id should be the identifier for a VPN.  If isn't one of these, then I don't think that it should be a vpn-id.

> connection (e.g., lx-termination-point).

Okay, if this always identifies a physical port, then the description should presumably describe it in terms of port rather than interface.

> customer site and the PEs".

Okay.

> [Med] What is meant here is defining an identity for priority-tagged interface.
> Please note that we removed the bearer-inf-type as it is not used in the
> module.

Does this mean that this port-id identity has also been removed, or just the base identity?

> (currently in the RFC Editor Queue).

Ack.  Makes sense.

> [Med] It is. This is used in some examples:

Ok.

> [Med] Yes. Updated to align with the candidate common module.

Okay.

> [Med] That covers the case in
> https://datatracker.ietf.org/doc/html/rfc4447#section-5.2

Okay.  Please can you update the description to make this clear, and possibly add a reference statement.

> [Med] Agree. We prefer to use this one for consistency with other checks in

Okay.

> include as a function of the type.

But the when statement already achieves that without also needing the choice and case statements.

> > specified, e.g., for a single address.

Okay.

> appropriate (e.g.., acls).

Okay.

> > next hop.  Is this correct?

Okay.

>                            the BGP route updates for this access.";

I'm still confused by the fact that the leaf is called prepend-global-as, and if you set it to true then "VPN node level is not prepended"

> [Med] Updated to: "The BGP session restarts after a time interval."

Okay.

> [Med] Yeah. Joe raised a similar comment, but we maintain "input" to ease
> the mapping with the L3NM. This would ease building L3SM-bis to make use
> of the common module if such bis is to be worked out in the future.

I would argue that it would be better for this model to be internally consistent.

Could this inbound-bandwidth and outbound-bandwidth, and in the comment make it clear that they have the reverse sense to the L3SM model?  E.g., perhaps in a future version of the L3SM model the existing names could be deprecated and use new ones with reverse sense?

> > be better as a leaf name?

Okay.

> But I agree it is better to be consistent.

Okay.

> [Med] Considered almost all your embedded comments.

Thanks.

> [Med] Thanks. Addressed almost all of them.

Thanks.

Rob

> ________________________________________________________________
> _________________________________________________________
> 
> Ce message et ses pieces jointes peuvent contenir des informations
> confidentielles ou privilegiees et ne doivent donc
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages
> electroniques etant susceptibles d'alteration,
> This message and its attachments may contain confidential or privileged
> information that may be protected by law;

---

From: <[email-masked]>
Date: Tue, 13 Jul 2021 14:42:09 +0000

Ho Rob,

I updated the files to take into account your feedback:

* Fix input/output in the common I-D (diff): https://github.com/IETF-OPSAWG-WG/lxnm/commit/ff456448a9398311457eb51c1d78adc8210fd8f8 
* L3NM (diff): https://github.com/IETF-OPSAWG-WG/lxnm/commit/ed48b938990e6e7e4be7e329d986592ebcdc0c37 
* I-D (diff): https://tinyurl.com/l3nm-latest 

Please see inline for the context. 

Cheers,
Med

> Envoyé : mardi 13 juillet 2021 14:52
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>;
> [email-masked]
> Cc : [email-masked]
> Objet : RE: AD review of draft-ietf-opsawg-l3sm-l3nm-09
> 
> Hi Med,
> 
> > > De : Rob Wilton (rwilton) [mailto:[email-masked]] Envoyé :
> lundi
> > > 12 juillet 2021 14:13 À : draft-ietf-opsawg-l3sm-
> [email-masked]
> > > Cc : [email-masked]
> > > 	   between the customer service model, the service delivery
> model,
> > > the
> > > 	   network configuration model, and the device configuration
> model.
> > > In
> > > 	   that context, the "Domain Orchestration" and "Config
> Manager"
> > > roles
> > > 	   The 'vpn-service' is the data structure that abstracts a
> VPN
> > > service
> > > 	   meaningful locally within the network controller.  The
> subtree
> > > of the
> > >
> > [Med] Agree. Made this change: s/meaningful locally (e.g., within
> the
> > network controller)
> 
> Ok.
> 
> >
> > >
> > >
> > [Med] That usage is correct as we define vpn-id in the common
> draft as
> > a generic identifier. That is why we mention "service identifier"
> as an example.
> > [Med]  We don't require any structure for the identification of
> the
> > port; hence the use of the string (vpn-common:vpn-id) type.
> 
> Okay, my point is that the model isn't just saying that it is a
> string (since it is using the type vpn-id) but is using a type with
> more meaning assigned to it.  I.e., I would expect that a vpn-id
> should be the identifier for a VPN.  If isn't one of these, then I
> don't think that it should be a vpn-id.
> 

[Med] Went with your proposal. Only maintained in few places where I think it makes sense. 

> > [Med] The details about logical interfaces is covered in the
> > connection and IP connection (e.g., lx-termination-point).
> 
> Okay, if this always identifies a physical port, then the
> description should presumably describe it in terms of port rather
> than interface.
> 

[Med] After thinking about this, I changed it to "interface-id' as we use it also to manage loopbacks. I updated the description to mention this is a physical or logical interface. I also checked the text we have about sub-interfaces. 

> > >          logical or physical interface on the device with the
> 'id'
> > > of
> > > 6. Is this description right?  Is port-id automatically
> priority-
> > > tagged?
> >
> > Please note that we removed the bearer-inf-type as it is not used
> in
> > the module.
> 

[Med] Yes, port-id was removed. 

> > > 7. Is "lag-tagged" right, should this be the just "Identity for
> LAG
> > > interface"?
> >
> > [Med] Yes, that's better. Please note that we removed the
> > bearer-inf-type as it is not used in the module.
> >
> > >
> > >
> > >   typedef area-address {
> > >     type string {
> > [Med] Actually, we aligned with the device module
> > draft-ietf-isis-yang-isis-cfg (currently in the RFC Editor Queue).
> 
> Ack.  Makes sense.
> 
> 
> >
> > >
> > >
> >                          "address-family": "vpn-common:dual-
> stack", ==
> >
> > > 11. What does it mean when a uint32 is used for the far end?
> Should
> > > the description cover this?
> >

[Med] Done. Thanks.

> > >                          allocation type, then IPv4 addressing
> is
> > > not
> > >                          enabled.";
> > >                     }
> > >
> > [Med] We used a choice as it more directive about which data nodes
> to
> > include as a function of the type.
> 

[Med] Fair. Fixed this one (and similar ones for consistency). 

> > >                                        When only start-address
> or
> > > only
> > >                                        implies a range inclusive
> of
> > > both
> > >                                        addresses.";
> > >
> > > 14. Why not make start-address mandatory, and require that is
> always
> > > specified, e.g., for a single address.
> >
> > [Med] Agree. Fixed.
> 
> Okay.
> 
> 
> >
> > >
> > >
> > >                         list address {
> > >                           key "address-id";
> > >                           description
> > >                               "The next-hop that is to be used
> for
> > > the
> > >                                static route. This may be
> specified
> > > as
> > > 16. The description states that an interface can be specified as
> the
> > > next hop.  Is this correct?
> >
> > [Med] Good catch. Fixed.
> 
> Okay.
> 
> 
> >
> > >
> > >
> > >                       leaf prepend-global-as {
> > >                         type boolean;
> > >                         default "false";
> > >                         description
> I'm still confused by the fact that the leaf is called prepend-
> global-as, and if you set it to true then "VPN node level is not
> prepended"
> 

[Med] Argh.. s/true/false. Always good to have fresh eyes. Thanks.

> > [Med] Yeah. Joe raised a similar comment, but we maintain "input"
> to
> > ease the mapping with the L3NM. This would ease building L3SM-bis
> to
> > make use of the common module if such bis is to be worked out in
> the future.
> 
> Could this inbound-bandwidth and outbound-bandwidth, and in the
> comment make it clear that they have the reverse sense to the L3SM
> model?  E.g., perhaps in a future version of the L3SM model the
> existing names could be deprecated and use new ones with reverse
> sense?
> 

[Med] Fair. Fixed this in the L3NM and the common I-D.

> > > 20. Is carrierscarrier as one word normal, would carriers-
> carrier,
> > > be better as a leaf name?
> >
> > [Med] Agree. Fixed.
> 
> Okay.
> 
> >
> > >
> > >
> > > 21. In the examples

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 13 Jul 2021 15:54:38 +0000

Hi Med,

Looking at the diff, I think that you have a typo for "oubound".

But just to check, the "inbound-rate-limit" and "inbound-bandwidth" both act in the same direction, right?  That was the consistency that I was striving for.  Normally, when I think of a QoS policy as acting on say a PE interface, I think that inbound/outbound would have the reverse sense to have the input-bandwidth/outbound-bandwidth is described.   I think that it would be good for these directions to be consistency if possible, but at a minimum the description needs to be very clear and using input vs inbound perhaps makes more sense if they are acting in different directions.

Thanks,
Rob

> Sent: 13 July 2021 15:42
> * Fix input/output in the common I-D (diff): https://github.com/IETF-
> OPSAWG-WG/lxnm/commit/ff456448a9398311457eb51c1d78adc8210fd8f8
> * L3NM (diff): https://github.com/IETF-OPSAWG-
> WG/lxnm/commit/ed48b938990e6e7e4be7e329d986592ebcdc0c37
> [Med] After thinking about this, I changed it to "interface-id' as we use it also
> to manage loopbacks. I updated the description to mention this is a physical
> or logical interface. I also checked the text we have about sub-interfaces.
> 
> >
> >
> > >
> > > >
> > > >

---

From: <[email-masked]>
Date: Tue, 13 Jul 2021 16:04:25 +0000

Re-,

Please see inline. 

Cheeers,
Med

> Envoyé : mardi 13 juillet 2021 17:55

[Med] Will be fixed. 

> 
> But just to check, the "inbound-rate-limit" and "inbound-bandwidth"
> both act in the same direction, right?

[Med] Yes. The text says inbound-rate-limit is a % of inbound-bandwidth.

The text also indicates that the direction is from the perspective of the customer site. That definition is also inherited from the common module. We don't revert the directions to ease the mapping between L3SM and L3NM. 

  That was the consistency
> that I was striving for.  Normally, when I think of a QoS policy as
> acting on say a PE interface, I think that inbound/outbound would
> have the reverse sense to have the input-bandwidth/outbound-
> bandwidth is described.   I think that it would be good for these
> directions to be consistency if possible, but at a minimum the
> description needs to be very clear and using input vs inbound
> perhaps makes more sense if they are acting in different directions.
> 
> Thanks,
> Rob
>

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 13 Jul 2021 16:35:17 +0000

Hi Med,

I think that you are good to go.

Thanks for accommodating my comments.

Regards,
Rob

> Sent: 13 July 2021 17:04
> The text also indicates that the direction is from the perspective of the
> customer site. That definition is also inherited from the common module. We
> don't revert the directions to ease the mapping between L3SM and L3NM.
> 
> 
>   That was the consistency