Re: [Teas] Benjamin Kaduk's Discuss on draft-ietf-teas-yang-te-types-12: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Wed, Nov 13, 2019 at 05:18:16AM +0000, Tarek Saad wrote:
> Hi Benjamin,
> 
> Thanks again. We are OK to make the suggested change to remove ambiguity as below. We are OK either uploading rev -13 (once tool reopens) or address it as
> an RFC-Editor note..

Either works for me as well; let's wait to see what Deborah wants to do.

Thanks,

Ben

> OLD:
>       typedef admin-group {
>         type yang:hex-string {
>           /* 01:02:03:04 */
>           length "1..11";
>         }
>         description
>           "Administrative group/Resource class/Color representation in
>            hex-string type.
>            The Most Significant Byte (MSB) is the farthest to the
>            left in the byte sequence.";
> 
> NEW:
>       typedef admin-group {
>         type yang:hex-string {
>           /* 01:02:03:04 */
>           length "1..11";
>         }
>         description
>           "Administrative group/Resource class/Color representation in
>            hex-string type.
>            The Most Significant Byte (MSB) is the farthest to the
>            left in the byte sequence. Leading zero bytes in the configured
>            value may be omitted for brevity. ";
> 
> Regards,
> Tarek (for authors)
> 
> On 11/12/19, 7:22 PM, "Teas on behalf of Benjamin Kaduk via Datatracker" <[e-mail] on behalf of [e-mail]> wrote:
> 
>     Benjamin Kaduk has entered the following ballot position for
>     draft-ietf-teas-yang-te-types-12: Discuss
>     
>     When responding, please keep the subject line intact and reply to all
>     email addresses included in the To and CC lines. (Feel free to cut this
>     introductory paragraph, however.)
>     
>     
>     Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
>     for more information about IESG DISCUSS and COMMENT positions.
>     
>     
>     The document, along with other ballot positions, can be found here:
>     https://datatracker.ietf.org/doc/draft-ietf-teas-yang-te-types/
>     
>     
>     
>     ----------------------------------------------------------------------
>     DISCUSS:
>     ----------------------------------------------------------------------
>     
>     Thank you for the updates in the -12; they're some good improvements!
>     I think I am still looking for a little more clarity on one point, though it
>     can probably be addressed with an RFC-Editor note:
>     
>     (1) the semantics of admin-group hex strings of length smaller than 11
>     
>     In the -12 we now have:
>     
>       typedef admin-group {
>         type yang:hex-string {
>           /* 01:02:03:04 */
>           length "1..11";
>         }
>         description
>           "Administrative group/Resource class/Color representation in
>            hex-string type.
>            The Most Significant Byte (MSB) is the farthest to the
>            left in the byte sequence.";
>     
>         reference "RFC3630 and RFC5305";
>       }
>     
>     This description still feels potentially ambiguous as to how to interpret
>     a byte sequence with fewer than four bytes -- does "MSB" mean "MSB of the
>     input string" or "MSB of the full-width value"?
>     
>     I suggest adding  "Leading zero bytes in the configured value may be omitted for brevity."
>     (but anything with similar clarifying value would be fine).
>     
>     
>     ----------------------------------------------------------------------
>     COMMENT:
>     ----------------------------------------------------------------------
>     
>     [comment section from -10 preserved unchanged, for convenience]
>     
>     In general it's hard to examine many of these types and see whether they
>     are fit for purpose without some sense of how they would be used, but of
>     course following a chain of references for all of the types in question
>     wouldu be tedious to the point of exhaustion.  I'm reluctant to start
>     getting into examples (since such a list would necessarily be
>     incomplete), but if you want one, the 'suppression-interval' leaf has a
>     very terse description, that will presumably be supplemented by some
>     text in some other document about what exactly is getting suppressed and
>     when, but if I were to try to implement from just this document I'd be
>     doing a lot of guessing.
>     
>     Section 3
>     
>        types for TE generic types and ietf-te-packet-types for packet
>        specific types.  Other technology specific TE types are outside the
>     
>     nits: "packet-specific" and "technology-specific" to hyphenate the
>     compound adjectives.
>     
>     Section 3.1
>     
>        te-node-id:
>     
>           A type representing the identifier for a node in a TE topology.
>           The identifier is represented as 32-bit unsigned integer in the
>           dotted-quad notation.  This attribute MAY be mapped to the Router
>     
>     Why is te-node-id IPv4-centric?  E.g., RFC 6119 describes an "IPv6 TE
>     Router ID" TLV as well as the "TE Router ID" TLV that we reference here
>     for the node ID.
>     Furthermore, either it's a 32-bit unsigned integer or it's represented
>     in the dotted-quad notation, but the dotted-quad notation doesn't really
>     count as a 32-bit unsigned integer.
>     
>        te-metric:
>     
>           A type representing the TE link metric as defined in [RFC3785].
>     
>     (editorial) the phrase "link metric" does not appear in RFC 3785.
>     
>     Section 3.2
>     
>        The ietf-te-packet-types module covers the common types and groupings
>        specific packet technology.
>     
>     nit: I think there's a word missing here.
>     
>        performance-metrics-attributes-packet:
>     
>           A YANG grouping for the augmentation of packet specific metrics to
>           the generic performance metrics grouping parameters.
>     
>     nit(?) I can't tell which way the augmentation is supposed to go, just
>     from this text.
>     
>     Section 4
>     
>       typedef admin-group {
>         type yang:hex-string {
>           /* 01:02:03:04 */
>           length "1..11";
>         }
>         description
>           "Administrative group/Resource class/Color representation in
>            hex-string type.";
>         reference "RFC3630 and RFC5305";
>     
>     RFC 5305 says to treat this as essentially a bitmask.  What are the
>     semantics when I only supply a string of (say) length 1 or 2?
>     
>       typedef performance-metrics-normality {
>            [...]
>         reference
>           "RFC7471: OSPF Traffic Engineering (TE) Metric Extensions.
>            RFC8570: IS-IS Traffic Engineering (TE) Metric Extensions.
>            RFC7823: Performance-Based Path Selection for Explicitly
>            Routed Label Switched Paths (LSPs) Using TE Metric
>            Extensions";
>     
>     None of these three references describes "normal" or "abnormal".
>     
>       typedef te-bandwidth {
>         type string {
>           pattern
>             '0[xX](0((\.0?)?[pP](\+)?0?|(\.0?))|'
>           + '1(\.([\da-fA-F]{0,5}[02468aAcCeE]?)?)?[pP](\+)?(12[0-7]|'
>           + '1[01]\d|0?\d?\d)?)|0[xX][\da-fA-F]{1,8}|\d+'
>           + '(,(0[xX](0((\.0?)?[pP](\+)?0?|(\.0?))|'
>           + '1(\.([\da-fA-F]{0,5}[02468aAcCeE]?)?)?[pP](\+)?(12[0-7]|'
>           + '1[01]\d|0?\d?\d)?)|0[xX][\da-fA-F]{1,8}|\d+))*';
>         }
>     
>     Can the document shepherd please report what validation has been done
>     for this pattern?
>     
>            For packet switching type, a float number is used, such as
>            0x1p10.
>     
>     What kind of units are implied for this (and the other types)?
>     
>       typedef te-metric {
>         type uint32;
>         description "TE link metric";
>         reference "RFC3785";
>     
>     [same comment about "link metric" and 3785]
>     
>       typedef te-node-id {
>         type yang:dotted-quad;
>         description
>           "A type representing the identifier for a node in a TE
>            topology.
>            The identifier is represented as 32-bit unsigned integer in
>            the dotted-quad notation.
>     
>     [same comment about uint32 vs. dotted-quad]
>     
>       typedef te-oper-status {
>         [...]
>     
>     Is there some way (a grouping?) to deduplicate the admin and operational
>     status enumerations?  They seem to be nearly identical in terms of the
>     range of possible values.
>     
>       identity se-style-desired {
>         [...]
>     
>     (et seq) The rtgdir's comments about whether 'base' should be present
>     seems relevant for these as well.
>     
>       identity oob-mapping-flag {
>     [....]
>       identity entropy-label-capability {
>     
>     Are all of the flags based on lsp-attributes-flags supposed to be
>     "desired" in the description (these aren't)?
>     
>       identity oam-mep-entity-desired {
>         base lsp-attributes-flags;
>         description "OAM MEP entities desired";
>     
>     We probably should expand Maintenance Entity Group End Point (and MIP,
>     below).
>     
>       identity loopback-desired {
>         base lsp-attributes-flags;
>         description
>           "This flag indicates a particular node on the LSP is
>            required to enter loopback mode.  This can also be
>     
>     Is there a mismatch between "desired" and "required"?
>     
>       identity rtm-set-desired {
>         base lsp-attributes-flags;
>         description
>           "Residence Time Measurement (RTM) attribute flag";
>     
>     Is there a mismatch regarding "desired" in the identity name vs.
>     description?
>     
>       identity link-protection-type {
>         description "Base identity for link protection type.";
>       }
>       identity link-protection-unprotected {
>         base link-protection-type;
>     
>     [I assume the WG had some good discussion about identities vs.
>     enumeration and don't want to second-guess that.]
>     
>       identity association-type-recovery {
>         base association-type;
>         description
>           "Association Type Recovery used to association LSPs of
>            same tunnel for recovery";
>     
>     nit: the grammar seems weird, here; maybe a missing word?
>     
>       identity path-locally-computed {
>         base path-computation-method;
>         description
>           "indicates a constrained-path LSP in which the
>           path is computed by the local LER";
>         reference "RFC3209";
>     
>     In line with the gen-art reviewer's comments, a section ref might be
>     useful here.
>     
>       identity path-externally-queried {
>         base path-computation-method;
>         description
>           [...]
>           to the external source to aid computation); and the path that is
>           returned by the external source is not required to provide a
>           wholly resolved path back to the originating system - that is to
>           say, some local computation may also be required";
>     
>     nit: "provide a wholly resolved path back to the originating system"
>     could potentially be read ambiguously about whether "back to the
>     originating system refers to the path or where the (partial) result is
>     returned to.
>     
>       identity te-tunnel-type {
>         [...]
>     
>     There is no need for a mp2p or mp2mp tunnel type?
>     
>       identity tunnel-state-type {
>         description
>           "Base identity for TE tunnel states";
>     
>     This is just "tunnel state" not "operational state"?
>     
>       identity lsp-state-type {
>         description
>           "Base identity for TE LSP states";
>     
>     Is there a state machine for which transitions between states based on
>     this identity are allowed, or is it basically just a full mesh?
>     
>       identity path-invalidation-action-drop-tear {
>         base path-invalidation-action-type;
>         description
>           "TE path invalidation action tear";
>     
>     Is there a reference for this (or the other
>     path-invalidation-action-type)?  My naive reading wants to complete it
>     to "teardown" but I have low confidence that is correct.
>     
>       identity path-metric-optimize-includes {
>         [...]
>       identity path-metric-optimize-excludes {
>         base path-metric-type;
>         description
>           "A metric that optimizes the number of excluded resources
>            specified in a set";
>     
>     optimizes to minimize or maximize it?
>     
>       grouping performance-metrics-one-way-bandwidth {
>         [....]
>     
>     What are the units on the bandwidth values herein?  bits per second?
>     
>           container threshold-accelerated-advertisement {
>             description
>               "When the difference between the last advertised value and
>                current measured value exceed this threshold, anomalous
>                announcement will be triggered.";
>             uses performance-metrics-thresholds;
>     
>     Are we reusing the performance-metrics-threshold container, which up
>     until now has been used for absolute measurements, to hold the
>     corresponding relative values (differences)?
>     
>       grouping optimization-metric-entry {
>         [...]
>         leaf weight {
>     
>     Where is the sense of the 'weight' leaf defined?  That is, to larger
>     weight values get more traffic or less traffic?
>     
>         container path-srlgs-names {
>             [...]
>             leaf-list names {
>               type string;
>               description "List named SRLGs";
>     
>     nit: missing "of"?
>     
>       grouping generic-path-disjointness {
>         description "Path disjointness grouping";
>         leaf disjointness {
>           type te-path-disjointness;
>           description
>             "The type of resource disjointness.
>              Under primary path, disjointness level applies to
>              all secondary LSPs. Under secondary, disjointness
>              level overrides the one under primary";
>     
>     What does "under" mean?  It seems like it has something to do with the
>     position in the hierarchy where this grouping gets instantiated, which
>     is potentially subtle.
>     
>         container path-properties {
>           config false;
>           [...]
>           container path-route-objects {
>             description
>               "Container for the list of route objects either returned by
>                the computation engine or actually used by an LSP";
>             list path-route-object {
>               key index;
>               ordered-by user;
>     
>     (How do 'config false' and 'ordered-by user' interact?)
>     
>     Section 5
>     
>        grouping performance-metrics-attributes-packet {
>          [...]
>          uses te-types:performance-metrics-attributes {
>            augment performance-metrics-one-way {
>              leaf one-way-min-delay {
>              [...]
>              leaf one-way-max-delay {
>     
>     What kind of sanity or error checking is there for the nodes in this
>     model, e.g., if max delay is configured to be smaller than min delay?
>     
>              leaf one-way-delay-variation {
>                type uint32 {
>                  range 0..16777215;
>                }
>                description "One-way delay variation in micro seconds.";
>     
>     What does "variation" mean?  Deviation from mean?  Variance?  Mean
>     absolute error?
>     
>              leaf one-way-packet-loss {
>                type decimal64 {
>                  fraction-digits 6;
>                  range "0 .. 50.331642";
>     
>     Is there a reason for the oddly specific upper limit?
>     I also can't quite convince myself from the RFC 7950 grammar whether the
>     quotes on the range are valid and/or needed.
>     
>     I also am unsure as to how useful the "normality" is for some of these,
>     like the packet loss and delay-variation values.
>     
>     [same comments about two-way metrics as one-way metrics, I think.
>     Also the -packet variants.]
>     
>     Section 7
>     
>     We probably could say more here if we want, though I'm as-yet decided on
>     whether we should.
>     That is, things like editing  the config could cause traffic to be
>     disallowed, routed on suboptimal paths, routed through locations more
>     easily targeted for other purposes, etc., and reading config/status
>     could reveal information about network topology, which sites might be
>     most sensitive as targets for subsequent attacks, etc..  It's less clear
>     if there's much that would allow for identification of specific
>     users/customers or their traffic, though.  Maybe some of the DSCP stuff,
>     in some cases.
>     
>     Section 10.2
>     
>     A question for the responsible AD: do references that are used in
>     "reference" statements need to be normative references?  (E.g., RFC
>     3471.)
>     
>     
>     _______________________________________________
>     Teas mailing list
>     [e-mail]
>     https://www.ietf.org/mailman/listinfo/teas
>