[OPSAWG]Re: AD review of draft-ietf-opsawg-teas-attachment-circuit

[[e-mail]]

Hi Mahesh, 

Thank you for the review.

The changes to take into account your review can be tracked at: https://author-tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/attachment-circuit-model/draft-ietf-opsawg-teas-attachment-circuit.txt&url_2=https://boucadair.github.io/attachment-circuit-model/AD-Review-of-AC-Common/draft-ietf-opsawg-teas-attachment-circuit.txt.

Please see inline for more details.

Cheers,
Med 


Orange Restricted

> -----Message d'origine-----
> De : Mahesh Jethanandani <[e-mail]>
> Envoyé : samedi 26 octobre 2024 00:57
> À : [e-mail]
> Cc : opsawg <[e-mail]>
> Objet : AD review of draft-ietf-opsawg-teas-attachment-circuit
> 
> 
> Hi Authors of draft-ietf-opsawg-teas-attachment-circuit,
> 
> First of all my apologies for taking all this time to review this
> document. This was no small document, specially if you combine
> with the fact that there are 3 other supporting documents.
> Regardless, here are some of my comments. I would like to see
> some discussion around the COMMENTs, and leave you to deal with
> the NITS as you feel.
> 
> -----------------------------------------------------------------
> COMMENT
> -----------------------------------------------------------------
> 
> Overall comments:
> 
> The four documents
> - this draft,
> - draft-ietf-opsawg-ntw-attachment-circuit
> - draft-ietf-opsawg-teas-common-ac
> - draft-ietf-opsawg-ac-lxlm-lxnm-glue
> 
> as I understand should be treated as a cluster. That includes the
> fact that they should be have been shephered by a single person,
> and sent up for AD review as a cluster. As I put the pieces
> together, here are general comments that apply to all the drafts.
> Individual comments on each draft will be provided in their own
> review.
> 
> Also note that this review is for -14 version of the document.
> The present version of the draft would be more like -17.
> 
> First of all, the models are fairly comprehensive in themselves.
> So thank you for putting all the word behind them. I can see it
> working as a template, where different users pick different
> pieces of the model. The authors have worked to add more
> deployment scenarios to explain how the model could be used in
> those scenarios as part of overall discussion on the model.
> 
> The rest of the review will focus on aspects of this draft.
> 
> I found it amusing that the 5 authors and 5 contributors formed
> the major portion of the "broad consensus" the document got from
> the WG. That combined with the fact that the document got few WG
> members supporting it concerns me. I am not sure if it is the
> lack of interest, or the document was difficult to follow.
> 
> "Abstract", paragraph 2
> >    Also, the document specifies a set of reusable groupings.
> Whether
> >    other service models reuse structures defined in the AC
> models or
> >    simply include an AC reference is a design choice of these
> service
> >    models.  Utilizing the AC service model to manage ACs over
> which a
> >    service is delivered has the advantage of decoupling service
> >    management from upgrading AC components to incorporate
> recent AC
> >    technologies or features.
> 
> This paragraph could be moved into the Introduction section, thus
> keeping the Abstract concise.

[Med] ACK. I think we can simply remove it as we do already have text that echoes that main message in the introduction.

> 
> Section 4.1, paragraph 7
> >    *  Customers may request protection schemes in which the ACs
> >       associated with their endpoints are terminated by the
> same PE
> >       (e.g., CE#3), distinct PEs (e.g., CE#34), etc.  The
> network
> >       provider uses this request to decide where to terminate
> the AC in
> >       the provider network (i.e., select which PE(s) to use)
> and also
> >       whether to enable specific capabilities (e.g., Virtual
> Router
> >       Redundancy Protocol (VRRP) [RFC9568]).  Note that
> placement
> >       constraints may also be requested during the
> instantiation of the
> >       underlying bearers (Section 5.1).
> 
> There is no node called CE#34 in the diagram.
> 

[Med] Changed to "CE#4".

> Section 5.1, paragraph 3
> >       +--rw locations
> >       |  +--rw customer-name?   string
> 
> Not clear on what the difference is between this customer-name,
> and the customer name under bearers, and the customer-name under
> bearer. Are these different customers?
> 

[Med] The updated location structure in -17 is as follows:

      +--rw locations
      |  +--rw customer* [name peer-as]
      |     +--rw name        string

This intended use is described in the narrative text:

   In some deployments, a customer may first retrieve a list of
   available presence locations before actually placing an order for a
   bearer creation.

That customer can then place a bearer request. The name of the customer that ordered a bearer is then tracked here:

         +--rw bearer* [name]
            +--rw name                           string
            +--rw description?                   string
            +--rw customer-name?                 string


> Shouldn't bearers be under each location? Isn't the bearer an
> underlay between a particular CE and PE? In other words, do all
> locations have access to all bearers?

[Med] Not sure to get the question. A customer only has access the location of the bearers it requested. A customer may not be eligible to create bearers in all presence locations of a provider. We do separate these views for operational purposes.

> 
> Section 5.1, paragraph 2
> >       |  +--rw local-as?        inet:as-number
> >       |  +--rw peer-as?         inet:as-number
> >       |  +--ro location* [location-name]
> >       |     +--ro location-name    string
> >       |     +--ro address?         string
> >       |     +--ro postal-code?     string
> >       |     +--ro state?           string
> >       |     +--ro city?            string
> >       |     +--ro country-code?    string
> 
> The IVY WG has a inventory model that defines locations in the
> form of a grouping. Can those definitions be used here instead of
> redefining them?

[Med] For the record, the IVY inspired from the structure we are using in this draft, especially this part:

        |  |  +--rw address?        string
        |  |  +--rw postal-code?    string
        |  |  +--rw state?          string
        |  |  +--rw city?           string
        |  |  +--rw country-code?   string

The YANG description  of ivy doc is a identical to what we had acaas.

We do also offer a location-information grouping that IVY doc can reuse :-) 

That's said, I don' think we need to add normative dependency here between these pieces. Please note also that the inventory is more a network model (with granular information to be covered), while we are concerned here with a limited set of information from a service standpoint. 
 
> 
> Section 5.2, paragraph 1
> >    The full tree diagram of the module can be generated using,
> e.g., the
> >    "pyang" tool [PYANG].  That tree is not included here
> because it is
> >    too long (Section 3.4 of [I-D.ietf-netmod-rfc8407bis]).
> Instead,
> >    subtrees are provided for the reader's convenience.  The
> full tree of
> >    the 'ac-svc' is provided in [AC-svc-Tree].
> 
> While an abridged tree diagram is what should be used to describe
> a model, depending on the discussion that is happening on the
> netmod mailing list, I hope the authors will consider adding a
> full tree diagram in the Appendix.

[Med] Idem as for the other AC documents. Will adjust as a function of the netmod outcome.

> 
> Section 5.2.2.1, paragraph 1
> >    The 'specific-provisioning-profiles' container (Figure 6)
> can be used
> >    by a service provider to maintain a set of reusable
> profiles.  The
> >    profiles definitions are similar to those defined in
> [RFC9181],
> >    including: Quality of Service (QoS), BFD, forwarding, and
> routing
> >    profiles.  The exact definition of the profiles is local to
> each
> >    service provider.  The model only includes an identifier for
> these
> >    profiles in order to facilitate identifying and binding
> local
> >    policies when building an AC.
> 
> I was unable to determine whether the 'specific-provisioning-
> profiles' defined in RFC9181 were reused here or redefined in
> this model.
> 

[Med] Section 5.2.2.1 says the following: 

   The 'specific-provisioning-profiles' container (Figure 8) can be used
   by a service provider to maintain a set of reusable profiles.  The
   profiles definitions are similar to those defined in [RFC9181],
   including

Please let is me know if any change is needed.

> Section 5.2.5.5, paragraph 4
> >    The 'security' container specifies the authentication and
> the
> >    encryption to be applied to traffic for a given AC.  Tthe
> model can
> >    be used to directly control the encryption to be applied
> (e.g., Layer
> >    2 or Layer 3 encryption) or invoke a local encryption
> profile.
> 
> Can the authors provide an example of what kind of traffic would
> be encrypted using this encryption profile? As I understand it,
> this is not end-to-end encrypted traffic. It is traffic encrypted
> as it is traversing the AC. Is that right?
> 

[Med] Like any other parts of this model, this covers only the AC portion not the end-to-end traffic. Please note that we do already have an example in Section 5.2.5.5:

   example, a service provider may use IPsec when a customer requests
   Layer 3 encryption for an AC.

> Section 7, paragraph 1
> >    This section uses the template described in Section 3.7 of
> >    [I-D.ietf-netmod-rfc8407bis].
> 
> I hope this section will be updated with the new text that has
> been agreed upon.

[Med] ACK.

> 
> Section 7, paragraph 18
> >    Several data nodes ('bgp', 'ospf', 'isis', and 'rip') rely
> upon
> >    [RFC8177] for authentication purposes.  As such, the AC
> service
> >    module inherits the security considerations discussed in
> Section 5 of
> >    [RFC8177].  Also, these data nodes support supplying
> explicit keys as
> >    strings in ASCII format.  The use of keys in hexadecimal
> string
> >    format would afford greater key entropy with the same number
> of key-
> >    string octets.  However, such a format is not included in
> this
> >    version of the AC service model because it is not supported
> by the
> >    underlying device modules (e.g., [RFC8695]).
> 
> Do the authors want to add a statement that says no rpcs are
> defined, and therefore no security considerations need to be
> documented for them.

[Med] Not sure that is needed. Please note that the template only requests to add RPC cons where these are defined: 

    -- if your YANG module has defined any RPC operations
    -- describe their specific sensitivity or vulnerability.

Do you see a value in saying it explicitly? If so, we need to have this include in the template.

> 
> Section 9.2, paragraph 4
> >    [I-D.ietf-opsawg-ac-lxsm-lxnm-glue]
> >               Boucadair, M., Roberts, R., Barguil, S., and O.
> G. de
> >               Dios, "A YANG Data Model for Augmenting VPN
> Service and
> >               Network Models with Attachment Circuits", Work in
> >               Progress, Internet-Draft, draft-ietf-opsawg-ac-
> lxsm-lxnm-
> >               glue-10, 10 June 2024,
> >
> <https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fdatatracker.ietf.org%2Fdoc%2Fhtml%2Fdraft-ietf-opsawg-
> &data=05%7C02%7Cmohamed.boucadair%40orange.com%7C8784edbff8534f2c
> 606208dcf5485639%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7C638
> 654938262397632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=WC3G
> f%2F9IEbB2Wc7n0Q8B6KupqeSMi6i8pr5ex9qyI6M%3D&reserved=0
> >               ac-lxsm-lxnm-glue-10>.
> >
> >    [I-D.ietf-opsawg-ntw-attachment-circuit]
> >               Boucadair, M., Roberts, R., de Dios, O. G.,
> Barguil, S.,
> >               and B. Wu, "A Network YANG Data Model for
> Attachment
> >               Circuits", Work in Progress, Internet-Draft,
> draft-ietf-
> >               opsawg-ntw-attachment-circuit-11, 15 May 2024,
> >
> <https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fdatatracker.ietf.org%2Fdoc%2Fhtml%2Fdraft-ietf-opsawg-
> &data=05%7C02%7Cmohamed.boucadair%40orange.com%7C8784edbff8534f2c
> 606208dcf5485639%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7C638
> 654938262413953%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=xpNR
> V672CREnOkCrNKhIvjvUo1k2eJwiomMKLEfl3iw%3D&reserved=0
> >               ntw-attachment-circuit-11>.
> 
> Are the YANG modules from these drafts not being imported by the
> YANG modules defined in this draft? If so, these should be moved
> to Normative Section of references.

[Med] No, those are not imported in this module.

> 
> "Appendix A.", paragraph 0
> >    This section includes a non-exhaustive list of examples to
> illustrate
> >    the use of the service models defined in this document.  An
> example
> >    instance data can also be found at [Instance-Data].
> 
> I am hoping that these examples have been validated with tools
> such as yanglint or similar. Since these examples are not marked
> as code, it is difficult to extract them from the draft for me to
> validate them.

[Med] All the examples are validated using yangson.

> 
> Found terminology that should be reviewed for inclusivity; see
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2
> Fwww.rfc-
> editor.org%2Fpart2%2F%23inclusive_language&data=05%7C02%7Cmohamed
> .boucadair%40orange.com%7C8784edbff8534f2c606208dcf5485639%7C90c7
> a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7C638654938262424826%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=qjF%2FaqWPUOujd4ztCHr8UNUZt
> laj8gij6jlLwQK8keE%3D&reserved=0 for background and more
> guidance:
> 
>  * Term "natively"; alternatives might be "built-in",

[Med] ACK.

> "fundamental",
>    "ingrained", "intrinsic", "original"
> 
> -----------------------------------------------------------------
> 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
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2
> Fgithub.com%2Flarseggert%2Fietf-
> reviewtool&data=05%7C02%7Cmohamed.boucadair%40orange.com%7C8784ed
> bff8534f2c606208dcf5485639%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0
> %7C0%7C638654938262435251%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&
> sdata=2gL0RqqjSPSizLjfak4Z6i90sy8ZKTgI1MriF2mzOw8%3D&reserved=0),
> so there will likely be some false positives. There is no need to
> let me know what you did with these suggestions.
> 
> Section 5.2.1, paragraph 9
> >    Features are used to tag conditional protions of the model
> in order
> >    to accomodate various deployments (support of layer 2 ACs,
> Layer 3
> >    ACs, IPv4, IPv6, routing protocols, Bidirectional Forwarding
> >    Detection (BFD), etc.).
> 
> s/protions/portions/

[Med] ACK.

> 
> 
> Document references draft-ietf-opsawg-teas-common-ac-11, but -12
> 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-teas-ietf-network-slice-nbi-yang-
> 13, but -16 is the latest available revision.
> 
> Document references draft-ietf-idr-bgp-model-17, but -18 is the
> latest available revision.
> 
> "OPSAWG", paragraph 0
> > 024 YANG Data Models for Bearers and 'Attachment Circuits'-as-
> a-Service (ACa
> >                                      ^
> Unpaired symbol: "'" seems to be missing.
> 
> Section 1.1, paragraph 3
> > roviding programmatic means to expose 'Attachment Circuits'-as-
> a-Service (ACa
> >                                       ^
> Unpaired symbol: "'" seems to be missing.
> 
> Section 9.2, paragraph 1
> > The attachment circuit in this case use a SAP identifier to
> refer to the phy
> >                                     ^^^
> The verb form "use" does not seem to match the subject "case".
> 

[Med] Fixed this one.

> Mahesh Jethanandani
> [e-mail]
> 
> 
____________________________________________________________________________________________________________
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.