Hi Med, Thanks for the quick reply.
A few comments inline ... > -----Original Message----- > From: mohamed.boucad...@orange.com > <mohamed.boucad...@orange.com> > Sent: 12 July 2021 18:57 > To: Rob Wilton (rwilton) <rwil...@cisco.com>; draft-ietf-opsawg-l3sm- > l3nm....@ietf.org > Cc: opsawg@ietf.org > Subject: RE: AD review of draft-ietf-opsawg-l3sm-l3nm-09 > > 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:rwil...@cisco.com] > > Envoyé : lundi 12 juillet 2021 14:13 > > À : draft-ietf-opsawg-l3sm-l3nm....@ietf.org > > Cc : opsawg@ietf.org > > 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:). > > > > > > 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, > > [Med] This is supposed to be covered by "network model" in the figure. > Updated the figure with an explicit mention. Ok. > > > 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)? > > [Med] Agree. Made this change: s/meaningful locally (e.g., within the > network controller) Ok. > > > > > > > 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? > > [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. > > > > > > > 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 > > [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. > > > 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). Okay, if this always identifies a physical port, then the description should presumably describe it in terms of port rather than interface. > > > > > > > '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? > > [Med] Updated to: "Represents a multipoint connection between the > customer site and the PEs". Okay. > > > > > > > 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? > > [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? > > > > > > > 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"? > > [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 { > > 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? > > [Med] Actually, we aligned with the device module > draft-ietf-isis-yang-isis-cfg > (currently in the RFC Editor Queue). Ack. Makes sense. > > > > > > > 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? > > [Med] It is. This is used in some examples: Ok. > > == > "address-family": [ > { > "address-family": "vpn-common:dual-stack", > == > > Updated to "(IPv4 and/or IPv6)". > > > > > > > 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). > > [Med] Yes. Updated to align with the candidate common module. Okay. > > > > > 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? > > [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. > > > > > > > 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. > > > > [Med] Agree. We prefer to use this one for consistency with other checks in > the module. Okay. > > > > > 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? > > [Med] We used a choice as it more directive about which data nodes to > include as a function of the type. But the when statement already achieves that without also needing the choice and case statements. > > > > > > > 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. > > [Med] Agree. Fixed. Okay. > > > > > > > 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. > > [Med] "id" is used to ease referring to an address in other places when > appropriate (e.g.., acls). Okay. > > > > > > > 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? > > [Med] Good catch. Fixed. Okay. > > > > > > > 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. > > [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? 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" > > > > > > > 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? > > [Med] Updated to: "The BGP session restarts after a time interval." Okay. > > > > > > > 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? > > [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? > > > > > > container carrierscarrier { > > > > 20. Is carrierscarrier as one word normal, would carriers-carrier, > > be better as a leaf name? > > [Med] Agree. Fixed. Okay. > > > > > > > 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). > > > > [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. Okay. > > > > > 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. Thanks. > > > > > (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. Thanks. Rob > > > > 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 > > > ________________________________________________________________ > _________________________________________________________ > > 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. _______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
