Hi Adrian, Many thanks for the review. A diff to track the changes can be found here: 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/opsdir-review/draft-ietf-opsawg-teas-attachment-circuit.txt. Also fixed some nits here and there. Please see inline. Cheers, Med > -----Message d'origine----- > De : Adrian Farrel via Datatracker <noreply@xxxxxxxx> > Envoyé : vendredi 3 janvier 2025 22:01 > À : ops-dir@xxxxxxxx > Cc : draft-ietf-opsawg-teas-attachment-circuit.all@xxxxxxxx; > last-call@xxxxxxxx; opsawg@xxxxxxxx > Objet : Opsdir telechat review of draft-ietf-opsawg-teas- > attachment-circuit-18 > > > Reviewer: Adrian Farrel > Review result: Has Issues > > draft-ietf-opsawg-teas-attachment-circuit-18 > Reviewer: Adrian Farrel > Review result: Has issues that should be resolved before > publication > > Hi, > > I have reviewed this document as part of the Operational > directorate's ongoing effort to review all IETF documents being > processed by the IESG. > These comments were written with the intent of improving the > operational aspects of the IETF drafts. Comments that are not > addressed in last call may be included in AD reviews during the > IESG review. Document editors and WG chairs should treat these > comments just like any other last call comments. > > = Summary = > > This document focuses on Attachment Circuits (ACs) that are the > connections exposed by a network to its customers. It provides > two YANG models for managing these ACs treated as services, and > for managing the underlying data links that carry these ACs. > > The document contains a fair amount of explanation and > justification, which, while interesting, gets in the way of > reading the models and the description of how they are used. > > I found the document relatively easy to read, but there were a > large number of nits. Since the main purpose of my review was not > nit-catching I have probably missed a good number. The document > would benefit from a further pass by a friendly reader. > > With the inclusion of all of the introductory material and a lot > of examples, this is a very large document. I feel that we do not > do ourselves a service producing so much text for what is, in > effect, a relatively simple piece of work. > > = Management Considerations = > > The document is all about the management of attachment circuits > and the bearer channels that carry them. The management aspects > of this are clear enough, and the examples are plentiful. > > The YANG appears to validate cleanly. > > = Review = > > == Medium == > > I had a lot of trouble matching the tree diagram in Figure 6 to > the YANG in Section 6.1. I got into this when I saw the quartet > of 'requested- start', 'requested-stop', 'actual-start', and > 'actual-stop' appear twice in Figure 6, but only explained once > in the text that follows. That made me try to unpick the YANG to > see whether the Figure is wrong or the text or both. > > The clue, of course, is the "uses ac-common:op-instructions;" [Med] :-) > > You will probably claim the excuse that the description of the > leafs is prefixed by "The descriptions of the bearer data nodes > are as follows:" > meaning that the text is not intended to apply to the general > 'bearers' > grouping. In that case, where you have "A request to create a > bearer may include a set of constraints" you should probably also > note the timings. > And, the devious brain asks what happens when the time requested > for an individual bearer falls outside the window set for the > group of bearers. > Easily explained, but you need to say it in the text and possibly > in the YANG module itself since it is supposed to be extractable > and standalone. [Med] Good point. Updated the narrative text + YANG to clarify which one takes precedence. Made similar changes for the AC part as well. > > I am not a YANG expert, but I wondered whether the tree needs to > show {ac-common:op-instractions} after each of these four > elements. > [Med] We can't use {ac-common:op-instractions} as this is not a feature. We could use '--tree-no-expand-uses' pyang option. The tree would display as follows: module: ietf-bearer-svc ... +--ro bearer-reference? string {ac-common:server-assigned-reference}? +--ro ac-svc-ref* ac-svc:attachment-circuit-reference +---u ac-common:op-instructions +---u ac-common:service-status This is a matter of taste, but I personally prefer the expanded form of the trees. > By the way: I like that you use rw or ro for all of the leafs in > your trees. (Slightly irritating that draft-ietf-opsawg-teas- > common-ac defaults to rw and only indicates the exceptional ro.) > [Med] FWIW, the reasoning for defaults/exception in common-ac is as follows: common-ac defines reuseable types/etc. With that in mind, common-ac was designed with the following guidance from 8407 in mind: * Do not include a "config" substatement on a data node unless the value applies on all possible contexts. > == Minor == > > 1.1 > > It is unfortunate that the best reference for AC that you have is > RFC > 4364 because the text there (that you quote) is a bit garbled. It > looks like an AC is the connection between any pair of routers! > > Given that you have just (concisely) defined an AC, I wonder why > you need this reference and quote. > [Med] That text was useful when starting this work, but you are right that this can be removed. Done. > --- > > 1.1 > > When a customer requests a new value-added service > > Why the qualifier "value-added"? Surely this applies to any > connectivity service that the customer requests? > > (BTW: twice in this paragraph, and then onwards in the document.) > [Med] Fair. Deleted. > --- > > 1.1 > > Since the provisioning of an AC requires a bearer to be in > place, > this document introduces a new module called "ietf-bearer-svc" > that > enables customers to manage their bearer requests (Section > 6.1). > > You have previously introduced the term "bearer" as: > > the underlying link is referred to as "bearer". > > You go on to provide a good definition of "bearer" in section 2. > > Now, what is a "bearer request"? I would translate as "manage > their underlying link requests" and get no clarity of meaning. > (Yes I could go and read 6.1, but I don't want to jump around in > the document!) > [Med] Removed "requests" and simply reason about "managing bearers". > --- > > 1.2 > > The explanations of the relationships to other modules are good. > And it is useful to include brief notes about "why not LxSM". > However, I think this section should mention RFC 9181 since you > do, in fact, use stuff from that module. > [Med] Added a note that ACaaS reuses types/structures from 9181. > --- > > 6.1 > > I was surprised that there are so few identities defined in base > bearer-type. Does this reflect reality or convenience? > [Med] The goal was not to be exhaustive but have the key ones. > --- > > 6.1 > > I'm surprised that the location information is not already > something that exists in another YANG module. I appreciate that > you have made it importable/useable as a reuseable grouping from > this document, but I wonder if it would be better in a separate > module rather than having to import the whole ietf-bearer-svc > module. Please think about this, and then "make the right > decision". [Med] There is the geo location grouping defined in rfc9179, but we don't use that grouping here because we wanted to minimize misalignment with the structure in 8299 that we use as basis for the location info. The bearer-svc is a short module. I don't think there is an issue to import it to reuse that grouping. > > --- > > 6.1 > > leaf sync-phy-type { > when "../sync-phy-enabled='true'"; > type identityref { > base sync-phy-type; > } > description > "Type of the physical layer synchronization."; > } > > Shouldn't this be > when "../sync-phy-capable='true'"; and > config false; > > I say this because 'sync-phy-capable' has [my emphasis] > "Indicates when set to true that *a* mechanism for > physical > layer synchronization is supported for this bearer. > [Med] A client may request the activation of a specific sync type. Such a request cannot include sync-phy-capable as this is ro. The request will be rejected by the provider if sync-phy-capable=false. A typical use is to first perform a GET to retrieve "sync-phy-capable" value. If that is set to true, the client can then request the sync to be enabled with or without a type. Updated the description of sync-phy-type to further insist this is the type that is enabled. > --- > > 7. > > I like that you flag 'customer-name' as a vulnerability. But I > don't understand what solution you offer. I suspect that the > solution lies in authentication being applied to control read > access not only to the whole module, but to specific sub-trees in > the module. This needs a little additional text to make it clear. [Med] This is a covered by this text: "These protocols have to use a secure transport layer (e.g., SSH [RFC4252], TLS [RFC8446], and QUIC [RFC9000]) and have to use mutual authentication." Please note that we are using the security template defined in draft-ietf-netmod-rfc8407bis. > > Why do you only call out 'customer-name' from the "ietf-ac-svc" > module and not from the "ietf-bearer-svc" module? [Med] This should be listed as well. Fixed. Thanks. > > --- > > Surely section 7 should at least include a reference to section > 5.2.5.5. [Med] Added a new sentence with that ref. > > I would expect some guidance about when to use this grouping (and > when it is OK not to), and particular suggestions about what > would happen if an attacker was able to mess with the leafs in > the grouping. > > I also see the 'encryption-choice' grouping. Shouldn't that be > mentioned too? [Med] This is covered by the following: 'ac': An attacker who is able to access this data node can modify the attributes of an AC (e.g., QoS, bandwidth, routing protocols, keying material), leading to malfunctioning of services that will be delivered over that AC and therefore to SLA violations. In addition, an attacker could attempt to add a new AC. We don't repeat those be lower ac layers because when reused this adds an encryption profile or a customer key. The profile part is already covered by the nacm guard: 'specific-provisioning-profiles': This container includes a set of sensitive data that influence how an AC will be delivered. For example, an attacker who has access to these data nodes may be able to manipulate routing policies, QoS policies, or encryption properties. These profiles are defined with "nacm:default-deny-write" tagging [I-D.ietf-opsawg-teas-common-ac]. That’s said, updated the text to also cite 'customer-key-chain': NEW: Several data nodes ('bgp', 'ospf', 'isis', 'rip', and 'customer-key-chain') rely upon [RFC8177] for authentication purposes. As such, the AC service module inherits the security considerations discussed in Section 5 of [RFC8177]. > > --- > > > > == Nits == > > Please consider "Attachment Circuit" or "attachment circuit" and > be consistent throughout. > [Med] Went with "attachment circuit". > --- > > Abstract > > s/service model/TANG service model/ [Med] OK > > You should explain "bearer" because it is a specific term you are > using in this document. > > Probably split the Abstract into two paragraphs to make it > clearer that there are two models in the document. > [Med] With the explanation of bearer/AC, the text is now split into tree parts. > --- > > 1. > > You should not have an empty section. I.e., you should have some > text between the header of 1. and the header of 1.1. [Med] I think this is OK at least rfc7322.html#section-4.8 says: "Sections are allowed to contain nothing but subsections." > > --- > > 1.1 > > "terminating points" > Do you mean "termination points"? > [Med] Yeah. Fixed. > --- > > 1.1 > > A connectivity service is basically about ensuring data > transfer > received from or destined to a given terminating point to or > from > other terminating points within the same customer/service, an > interconnection node, or an ancillary node. > > After reading four or five times, I think I can parse this. > Surely you can find a clearer way to carry the "and / or" > meaning. > [Med] Removed "within the same customer/service, an interconnection node, or an ancillary node.". > --- > > 1.1 > > s/document as Attachment Circuit/document as an Attachment > Circuit/ > [Med] OK to add "an" > --- > > 1.1 > > s/ACs prior or during/ACs prior to or during/ > [Med] OK. > --- > > 1.1 > > > The "ietf-ac-svc" module (Section 6.2) includes a set of > reusable > groupings. Whether a service model reuses structures defined > in the > "ietf-ac-svc" or simply includes an AC reference (that was > communicated during AC service instantiation) is a design > choice of > these service models. > > I think, "Whether a service model that wants to describe the > attachment circuits associated with the service reuses > structures..." > [Med] This is better. Thanks. > --- > > 1.1 > > Relying upon the AC service model to manage > ACs over which services are delivered has the merit of > decorrelating > the management of the (core) service vs. upgrade the AC > components to > reflect recent AC technologies or new features (e.g., new > encryption > scheme, additional routing protocol). > > I see what you are saying, but perhaps it could be expressed > better? > It's not that you don't have to upgrade for new AC technologies, > it is that you only have to do it in one place rather than in > each service model. Perhaps: > > Relying upon the AC service model to manage > ACs over which services are delivered has the merit of > decorrelating > the management of the (core) service from the ACs. This > allows > upgrades (to reflect recent AC technologies or new features > such as > new encryption schemes, or additional routing protocols) to be > done > in just one place rather than in each (core) service model. > [Med] Went with your proposed text. Thank you. > --- > > 1.1 > > Good list of use cases and forward references to the Appendixes. > Any reason why you omit A.9? > [Med] Good catch. Updated the list with A.9 and also A.12 examples. > --- > > 1.3 > > Please apply the following replacements: > > * XXXX --> the assigned RFC number for this I-D > > * 2023-11-13 --> the actual date of the publication of this > document > > I found no instances of "2023-11-13". I did find a number of > "2023-12-12" > > On the other hand, I did find "RFC CCCC" > > Note also that sometimes you use "xxxx" > [Med] Fixed all those. Updated the TLP date to 2025 as well. Thanks > -- > > 2. > > Should also include LxVPN [Med] Done. > > --- > > 2. (parent bearer and parent AC) > s/inherit th/inherit the/ > [Med] ACK > --- > > 2. > > Service provider: A service provider that offers network > services > (e.g., Layer 2 VPN, Layer 3 VPN, or Network Slice > Services). > > This definition is possibly too self-referential. > [Med] Updated the text: s/A service provider/An entity > --- > > 3. > > s/bt/by/ > [Med] ACK > --- > > 3. and Figure 1 > > It would be useful to say what the directional arrow means. > I think it is "depends upon material in" which is why draft-ietf- > opsawg- teas-common-ac is a normative reference but the other > docs are just informative. > [Med] added a legend to say that this is about direction of imports. Fixed also the direction from ac-svc to bearer-svc as referencing in that direction does not require an import. > --- > > Figure 5 shows a marker "NETCONF/CLI" on the lines from Domain > Orchestration to Network. > > Is this supposed to carry across to the line between Network > Controller and Network? If so: fix the figure. [Med] Done. If not: some > commentary might help. > > --- > > 6.1 > > grouping location-information { > leaf state { > > The name is unfortunate because (of course) it is not the state > of the bearer or of the location-information. However, the > context is probably enough. Would 'region' be a better name? [Med] It might be a better name, however I think that I prefer to keep the OLD as this eases the mapping with existing service models such as 8299 (where these nodes were actually grabbed from). > > And should 'city' be positioned before 'postal-code' and 'state'? > [Med] Works for me. Done. > --- > > 6.1 > > leaf sync-phy-capable and leaf sync-phy-enabled > > s/Indicates when set to true that/Indicates, when set to true, > that/ > [Med] ACK > --- > > 6.2 8341 is missing from the list of RFCs from which imports are > made. > [Med] Added a new sentence to indicate that we use the extensions in 8341. > --- > > In your examples, you use line wrapping per RFC 8792, and you say > so clearly. But you don't have a reference to. > > I suggest starting the Appendixes with a statement like: > Some of the examples below use line wrapping per [RFC8792] and > then you can include the reference. > > [Med] Done. ____________________________________________________________________________________________________________ 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. -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
