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;"
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.
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.
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.)
== 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.
---
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.)
---
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!)
---
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.
---
6.1
I was surprised that there are so few identities defined in base
bearer-type. Does this reflect reality or convenience?
---
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".
---
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.
---
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.
Why do you only call out 'customer-name' from the "ietf-ac-svc" module
and not from the "ietf-bearer-svc" module?
---
Surely section 7 should at least include a reference to section 5.2.5.5.
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?
---
== Nits ==
Please consider "Attachment Circuit" or "attachment circuit" and be
consistent throughout.
---
Abstract
s/service model/TANG service model/
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.
---
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.
---
1.1
"terminating points"
Do you mean "termination points"?
---
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.
---
1.1
s/document as Attachment Circuit/document as an Attachment Circuit/
---
1.1
s/ACs prior or during/ACs prior to or during/
---
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..."
---
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.
---
1.1
Good list of use cases and forward references to the Appendixes. Any
reason why you omit A.9?
---
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"
--
2.
Should also include LxVPN
---
2. (parent bearer and parent AC)
s/inherit th/inherit the/
---
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.
---
3.
s/bt/by/
---
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.
---
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. 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?
And should 'city' be positioned before 'postal-code' and 'state'?
---
6.1
leaf sync-phy-capable and leaf sync-phy-enabled
s/Indicates when set to true that/Indicates, when set to true, that/
---
6.2 8341 is missing from the list of RFCs from which imports are made.
---
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.
--
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx
