Hi Mahesh, Thank you for the careful review. Much appreciated.
A diff to track the changes can be found at: https://author-tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/attachment-circuit-model/draft-ietf-opsawg-teas-common-ac.txt&url_2=https://boucadair.github.io/attachment-circuit-model/AD-Review-of-AC-Common/draft-ietf-opsawg-teas-common-ac.txt. Please see inline for more context. Cheers, Med Orange Restricted > -----Message d'origine----- > De : Mahesh Jethanandani <mjethanand...@gmail.com> > Envoyé : samedi 26 octobre 2024 05:42 > À : draft-ietf-opsawg-teas-common...@ietf.org > Cc : opsawg <opsawg@ietf.org> > Objet : AD review of draft-ietf-opsawg-teas-common-ac > > > Hi Authors of draft-ietf-opsawg-teas-common-ac, > > Much like the other document in this cluster, here is my review > of this document. While I hope to have some discussion around the > COMMENTs in this review, the NITs are something you can choose > whether you want to respond to or not. > > ----------------------------------------------------------------- > COMMENT > ----------------------------------------------------------------- > > "Abstract", paragraph 0 > > Abstract > > > The shepherd writeup mentions that one of the authors is also > author of draft-ietf-teas-ietf-network-slice-nbi-yang, which is > great. My question however is, was this document vetted in the > TEAS WG, and if so what was the feedback? [Med] FWIW, the AC documents were presented in teas at least two times and the WGLC was also sent to TEAS. The ACaaS spec in particular is used as a normative reference in draft-ietf-teas-ietf-network-slice-nbi-yang. > > The writeup also mentions that the TEAS document has an > implementation. Can this document mention that in its > Implementation Report? > > Section 4, paragraph 3 > > The full tree diagram of the module can be generated using > the > > "pyang" tool [PYANG] with "-f tree --tree-print-groupings" > command- > > line parameters. That tree is not included here because it > is too > > long (Section 3.3 of [RFC8340]). Instead, subtrees are > provided for > > the reader's convenience. > > > > The full tree of the "ietf-ac-common" module is available > at > > [AC-Common-Tree]. > > This paragraph needs to be updated to reflect the consensus on > "long trees" in the netmod mailing list, and the complete tree > diagram added. [Med] There is actually no statement in the bis to say that long trees MUST be included ;-) Seriously speaking, will update that part once we have a conclusion in NETMOD. > > Section 4.2, paragraph 6 > > 'bgp-capability': Used to indicate a BGP capability > [RFC5492]. > > Examples of BGP capabilities are Multiprotocol extensions > for > > BGP-4 [RFC4760], route refresh [RFC2918], graceful > restart > > [RFC4724], ADD-PATH [RFC7911], or BGP Role [RFC9234]}. > > Please use identities in IANA YANG modules defined by draft-ietf- > idr-bg-model, specifically iana-bgp-types.yang, instead of > redefining them here. > [Med] We considered that but, unlike the device model, we don't need the full support of all capabilities. Only a subset makes sense at the service/network level. Added this NEW: "Only a subset of BGP capabilities might be useful for customization at the service/network AC levels. As such, this document does not use the full list defined in {{Section 7.3 of ?I-D.ietf-idr-bgp-model}}." As a side note, I have also a concern to add a normative dependency to the IDR document which was out there since 2013 and for which we don't have a visibility when it will be published. > Section 4.3, paragraph 11 > > +-- address-allocation-type? identityref > > +-- (allocation-type)? > > +--:(dynamic) > > +-- (provider-dhcp)? > > | +--:(dhcp-service-type) > > | +-- dhcp-service-type? enumeration > > +-- (dhcp-relay)? > > +--:(customer-dhcp-servers) > > +-- customer-dhcp-servers > > +-- server-ip-address* inet:ipv4- > address > > Wonder why you need both the leaf 'address-allocation-type' and > the choice 'allocation-type' where the latter tells you what is > the address allocation type is? [Med] There are cases where allocation-type nodes are not present at all; 'address-allocation-type' is thus useful to indicate the type. Please note that we are using the same structure as RFC 8299 and RFC 9182. > > Section 4.3, paragraph 10 > > +-- address-allocation-type? identityref > > +-- (allocation-type)? > > Same here. [Med] In addition to the same reasoning as IPv4, we do have the slaac case for IPv6 which distinct from dhcp-based schemes. Added NEW text to clarify this. > > Section 4.3, paragraph 17 > > +--:(explicit) > > +-- key-id? uint32 > > +-- key? string > > +-- crypto-algorithm? identityref > > > Doesn't the keychain model define these parameters? Why model > them differently? > > Same for the next 3 sets of authentications. [Med] keychain refs are supported as well. In order to be consistent with, e.g., RFC9129, the groupings allows to cover keychain and explicit parameter configurations to accommodate legacy implem. RFC9129 has the following justification: "Additionally, local specification of OSPF authentication keys and the associated authentication algorithm is supported for legacy implementations that do not support key chains [RFC8177]." Added a new text to echo this. > > Section 5, paragraph 64 > > list qos-profile-identifier { > > key "id"; > > description > > "List of QoS profile identifiers."; > > leaf id { > > type string; > > description > > "Identification of the QoS profile to be used."; > > } > > } > > Shouldn't the id be a leafref instead of a string? [Med] Strings are used here because there is no exposure of the internal structures of these profiles. The description text says the following: "Container for valid provider profile identifiers. The profiles only have significance within the service provider's administrative domain."; This structure is inherited from RFC8299, RFC9182, RFC9291, etc. > > Section 6, paragraph 0 > > This section uses the template described in Section 3.7 of > > [I-D.ietf-netmod-rfc8407bis]. > > Much like the comment in the other draft, hope this template will > be updated with the text that was recently agreed upon. [Med] Done. > > Possible DOWNREF from this Standards Track doc to [ISO10589]. If > so, the IESG needs to approve it. [Med] I'm not sure DOWNREF procedure applies here as this is an ISO standard. Please note that this standards was normatively referenced by several standard track RFCs (e.g., https://datatracker.ietf.org/doc/rfc5308/). > > ----------------------------------------------------------------- > NIT > ----------------------------------------------------------------- > > All comments below are about very minor potential issues that you > may choose to address in some way - or ignore - as you see fit. > Some were flagged by automated tools (via > > > "OPSAWG", paragraph 1 > > Section 1, paragraph 4 > + s/specific to a given service or be shared/specific for a given > + service or are shared/ > [Med] ACK > > Section 1, paragraph 1 > > Connectivity services are provided by networks to customers > via > > dedicated terminating points (e.g., Service Functions (SFs), > Customer > > Premises Equipment (CPEs), Autonomous System Border Routers > (ASBRs), > > data centers gateways, or Internet Exchange Points). A > connectivity > > service is basically about ensuring data transfer received > from (or > > destined to) a given terminating point to (or from) other > terminating > > points that belong to the same customer/service, an > interconnection > > node, or an ancillary node. A set of objectives for the > connectivity > > service may eventually be negotiated and agreed upon between > a > > customer a network provider. For that data transfer to take > place > > within the provider network, it is assumed that adequate > setup is > > provisioned over the links that connect customer terminating > points > > and a provider network (a Provider Edge (PE), typically) so > that data > > can be successfully exchanged over these links. The > required setup > > is referred to in this document as Attachment Circuits > (ACs), while > > the underlying link is referred to as "bearer" > > s/customer a network provider/customer and a network provider/ > [Med] ACK > Section 1, paragraph 3 > > When a customer requests a new value-added service, the > service can > > be bound to existing attachment circuits or trigger the > instantiation > > of new attachment circuits. Whether these attachment > circuits are > > specific to a given service or be shared to deliver a > variety of > > services is deployment-specific. > > s/specific to a given service or be shared/specific for a given > service or are shared/ [Med] ACK. > > Document references draft-ietf-teas-ietf-network-slice-nbi-yang- > 13, but -16 is the latest available revision. > > Document references draft-ietf-netmod-rfc8407bis-14, but -20 is > the latest available revision. > > Document references draft-ietf-opsawg-ntw-attachment-circuit-11, > but -13 is the latest available revision. > > Document references draft-ietf-opsawg-teas-attachment-circuit-13, > but -17 is the latest available revision. > [Med] This will be automatically fixed when releasing the new version. Thanks. > Mahesh Jethanandani > mjethanand...@gmail.com > > ____________________________________________________________________________________________________________ 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 To unsubscribe send an email to opsawg-le...@ietf.org
