Re: [Lsr] AD Review for draft-ietf-ospf-yang-21

Alvaro Retana <aretana.ietf@gmail.com> Mon, 24 June 2019 20:52 UTC

Return-Path: <aretana.ietf@gmail.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 8E0B21200C5; Mon, 24 Jun 2019 13:52:10 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.811
X-Spam-Level:
X-Spam-Status: No, score=0.811 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RAZOR2_CF_RANGE_51_100=1.886, RAZOR2_CHECK=0.922, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id R3e3pI14C01m; Mon, 24 Jun 2019 13:52:06 -0700 (PDT)
Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 18B1012010F; Mon, 24 Jun 2019 13:52:05 -0700 (PDT)
Received: by mail-ed1-x535.google.com with SMTP id d4so23602906edr.13; Mon, 24 Jun 2019 13:52:05 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:in-reply-to:references:mime-version:date:message-id:subject:to :cc; bh=q/uu55mgUeLdY/aABnnHFWaKcXesNJc1gqRZgx055Xo=; b=Oxuee7caq7ox90dbZUunx2QgrR6OELa+675MIzYr1DFyz/FNLVP5Tt0sk5E6k/r2lg 2FmGv7aoc2EIc4rkqO02YJ64X4PzElYcxBMx2jozkfsaUyW/itvA8sW5kSuSdxAt3BOa FeAzY4d9v76D+2EbcP32xcqi0s5kpltNjJ0vQrgThSJMt30v+ZZr7aNjU/ygJAAG6zxY CJOVedbgJn59mkurssSxu7gN73lefX4N2CDVMRxFGJtQcg6vpVklvkTKxddLbZ+FH7Ke Nbcf8vgix4Z4ldXnmA9txSpN9HaKJSqD0SA97nRQnVU3oguzCzDFebGTe8DgcBPoOqGw ezSw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:in-reply-to:references:mime-version:date :message-id:subject:to:cc; bh=q/uu55mgUeLdY/aABnnHFWaKcXesNJc1gqRZgx055Xo=; b=uXxFdgYu6EJcOza7J5MuxM5LuUsNn4B/ay5kjD9wGa6/3czbh6OCj80RMDgoAE1hfW IzoatvnG3ldqEImglGKblFo53d1anhitGqbRgJ3H7oYBbT7UkLmbSxwIjUPVeyIuq3F8 dxylqJmzaAabWehT0GVIcHXwdiPZkJxJhU2W8FzObQSE4g87iOZPj0jaU/kynWR2YtPG tECR9hzYcny0LbbcoSn5hdc2G/Q5C/Rnwy6BXTBc4lKvTr8faVgzgjIPU5bvzoXl78Ok nP04MANdUPUNDw9opOnb5uQuIubbSQrN2s/4Qa4Hjod8K7Yl3F+fAmoN/lNXQSIhVTAU Iryw==
X-Gm-Message-State: APjAAAWoKv45hbi584BQRkyQCUvoRcOWeV3quDx34KluEw/uwaaxKRtf wcaQ/EybD3XpIVmF6dw/6CvN+oBkZg9PKrdGKkQ=
X-Google-Smtp-Source: APXvYqzg/hDimbNBBdPwVU6tzfPPGhExrKKdg98oOFEC9Mz39fw2TcrqmcYaEcX7fcxFKiMca/HS5PekHroUwSkbQ3M=
X-Received: by 2002:a17:906:401a:: with SMTP id v26mr115658426ejj.62.1561409524217; Mon, 24 Jun 2019 13:52:04 -0700 (PDT)
Received: from 1058052472880 named unknown by gmailapi.google.com with HTTPREST; Mon, 24 Jun 2019 16:52:03 -0400
From: Alvaro Retana <aretana.ietf@gmail.com>
In-Reply-To: <77F1A67E-2EB8-453E-8E89-70C55A820E03@cisco.com>
References: <CAMMESsztO1a4fnT2Gx2GDKcYVLtWS52WZ=HmPdQ9VFqSEtvG7Q@mail.gmail.com> <77F1A67E-2EB8-453E-8E89-70C55A820E03@cisco.com>
MIME-Version: 1.0
Date: Mon, 24 Jun 2019 16:52:03 -0400
Message-ID: <CAMMESsxq4dAvGn0n30NnpbygLf13j5uWK6=6feqNJsMDuzuUrQ@mail.gmail.com>
To: "Acee Lindem (acee)" <acee@cisco.com>, "draft-ietf-ospf-yang@ietf.org" <draft-ietf-ospf-yang@ietf.org>
Cc: "lsr@ietf.org" <lsr@ietf.org>, "stephane.litkowski@orange.com" <stephane.litkowski@orange.com>, "lsr-chairs@ietf.org" <lsr-chairs@ietf.org>
Content-Type: multipart/alternative; boundary="0000000000000a3d44058c17fc56"
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/Z2TDbqYCgtKClQRTSDPUawcVrfs>
Subject: Re: [Lsr] AD Review for draft-ietf-ospf-yang-21
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 24 Jun 2019 20:52:11 -0000

On June 22, 2019 at 7:03:58 PM, Acee Lindem (acee) (acee@cisco.com) wrote:

Acee:

Hi!

Thanks for the review - we will address your comments in the -22 version.
See inline.

I have a couple of replies.  Please take a look at the bottom because it
looks like you didn’t get (or missed) part of the review.

Thanks!

Alvaro.


...

237 9. nsr: Support OSPF Non-Stop Routing (NSR).

[minor] Reference?

<acee> No IETF reference for OSPF NSR. However, I have a few patents in
this area - should the model reference these? __

If they are applicable to this document, then yes, please disclose them.
But NSR is not described here (so I wouldn’t think the patents apply),
which is why I was asking for a reference.   Right now the model says NSR
can be enabled/disabled, but it doesn’t explain what is being
enabled/disabled.  Given that, it is not clear what vendors can claim to
support, leading to potential incompatibilities and/or unintended behavior.

I would like to either see a description (but this document is probably not
the right place for that), a pointer to a description (which doesn’t have
to be an IETF document), or to have the support taken out.  If the WG
agreed to what NSR means, then I would be fine with transcribing some of
that here (from an existing mail thread, for example).


...
242 11. admin-control: Support Administrative control of the protocol
243 state.

[minor] This seems like a "basic" feature: enabling/disabling the
protocol. Why was it decided to make it optional? Are there
implementations that don't support enabling/disabling?

<acee> I'm happy to remove it.

I was not asking you to remove it…I was simply wondering why it is optional.

You seem to have removed it completely, but it is still in §2.5.


...

...
464 2.6. OSPF Area Configuration/Operational State
...
570 | | +--rw hello-interval? uint16
571 | | +--rw dead-interval? uint32
572 | | +--rw retransmit-interval? uint16

[minor] Why aren't rt-types:timer-value-* used for the *-timer/interval
definitions? I would have thought this was exactly the reason for the
definitions in rfc8294.

<acee> I'd like to use these types universally. However, YANG seems to have
a restriction that I can't further restrict the range. This really sucks.
Will do what I can and make the best trade-off.

I see you changed all, except retransmit-interval and restart-interval….
Just pointing it out in case you meant to.


...

...
1685 typedef if-state-type {
1686 type enumeration {
...
1712 enum backup {
1713 value "6";
1714 description
1715 "Interface Backup Designated Router (BDR) state.";
1716 }

[nit] s/backup/bdr To match the other instances where a BDR is referenced

<acee> Good catch. Fixed.

Thanks,
Acee

What about the rest of the comments??

https://mailarchive.ietf.org/arch/msg/lsr/LKvR2rfkPHJui_GMRerIdDNHkug


I don’t know if it’s me, but several people seem to not receive the full
length of my comments…but they are complete in the archive…so I’m not sure
that the problem is.  Please take a look at the archive — I have pasted
comments after line 1716 below.  I am changing the state to Revised ID
Needed.

Thanks!

Alvaro.



...

2729       container network {

2730         when "derived-from-or-self(../../header/type, "

2731            + "'ospfv3-network-lsa')" {

2732           description

2733             "Only applies to Network LSAs.";

2734         }

2735         description "Network LSA.";


[nit] Perhaps network-lsa could be a better name.



...

3053     grouping lsa-common {

3054       description

3055           "Common fields for OSPF LSA representation.";

3056       leaf decoded-completed {

3057         type boolean;

3058         description

3059           "The OSPF LSA body is fully decoded.";

3060       }


[minor] What is decoded-completed indicating?  Does it indicate that no
errors where found, IOW, "successful decoding"?  Does it mean that the LSA
has been decoded (but not an indication of success, or not)?  Is it a
time-based indication; IOW, is there a chance that an LSA has not been
processed when retrieving the information...would that result in false??



...

3271     grouping instance-fast-reroute-state {

3272       description "IPFRR state data grouping";


[nit] s/IPFRR/IP-FRR   That is how the term is used everywhere else in this
document.



...

3541       leaf hello-interval {

3542         type uint16 {

3543           range "1..65535";

3544         }

3545         description

3546           "Interval between hello packets (seconds). It must

3547            be the same for all routers on the same network.

3548            Different networks, implementations, and deployments

3549            will use different hello-intervals. A sample value

3550            for a LAN network would be 10 seconds.";

3551       }


[minor] Some implementations support having a sub-second
hello-interval...but with a timer in seconds that can't be done.



...

3664                 leaf ospfv2-key {

3665                   type string;

3666                   description

3667                     "OSPFv2 authentication key. The

3668                      length of the key may be dependent on the

3669                      cryptographic algorithm. In cases where it is

3670                      not, a key length of at least 32 octets should

3671                      be supported to allow for interoperability

3672                      with strong keys.";


[major] Is there a reference for the part about the "key length of at least
32 octets should be supported to allow for interoperability with strong
keys"?



...

3719                 leaf ospfv3-key {

3720                   type string;

3721                   description

3722                     "OSPFv2 authentication key. The

3723                      length of the key may be dependent on the

3724                      cryptographic algorithm. In cases where it is

3725                      not, a key length of at least 32 octets should

3726                      be supported to allow for interoperability

3727                      with strong keys.";


[major] The same comment from ospfv2-key applies here.


[nit] s/OSPFv2 authentication key/OSPFv3 authentication key



...

3856           leaf priority {

3857             type uint8 {

3858               range "1..255";

3859             }

3860             description "Neighbor priority for DR election.";

3861           }


[minor] A range is included in this definition, but not in other priority
statements before...should a range be included in those other definitions?


[minor] Why is 0 not applicable here?



...

3936       leaf dead-timer {

3937         type uint32;

3938         units "seconds";

3939         config false;

3940         description "This timer tracks the remaining time before

3941                      the neighbor is declared dead.";

3942       }


[major] For *-timer: Is tracking the remaining time in seconds enough?  I
would assume that ms would be the right unit.  Why seconds?



...

4390       container preference {

4391         description "Route preference config state.";


[minor] Maybe it's just me, but I don't understand what this piece of the
model does.  It seems like it can be used to configure which type of route
is preferred: external over intra-area, for example...but that doens't make
too much sense to me.  Is that the intended use?  Can you please give me an
example of when this functionality could be used?


4392         choice scope {

4393           description

4394             "Options for expressing preference

4395              as single or multiple values.";

4396           case single-value {

4397             leaf all {

4398               type uint8;

4399               description

4400                 "Preference for intra-area, inter-area, and

4401                  external routes.";

4402             }

4403           }

4404           case multi-values {

4405             choice granularity {

4406               description

4407                 "Options for expressing preference

4408                  for intra-area and inter-area routes.";

4409               case detail {

4410                 leaf intra-area {

4411                   type uint8;

4412                   description

4413                     "Preference for intra-area routes.";

4414                 }

4415                 leaf inter-area {

4416                   type uint8;

4417                   description

4418                     "Preference for inter-area routes.";

4419                 }

4420               }

4421               case coarse {

4422                 leaf internal {

4423                   type uint8;

4424                   description

4425                     "Preference for both intra-area and

4426                      inter-area routes.";

4427                 }

4428               }

4429             }

4430             leaf external {

4431               type uint8;

4432               description

4433                 "Preference for AS external routes.";

4434             }

4435           }


4437         }

4438       }



...

4551       container stub-router {

4552         if-feature stub-router;

4553         description "Set maximum metric configuration";


4555         choice trigger {

4556           description

4557             "Specific triggers which will enable stub

4558              router state.";

4559           container always {

4560             presence

4561               "Enables unconditional stub router support";

4562             description

4563               "Unconditional stub router state (advertise

4564                transit links with max metric";


[minor] s//MaxLinkMetric [rfc6897]



...

5537 4.  Security Considerations

...

5552   There are a number of data nodes defined in ietf-ospf.yang module

5553   that are writable/creatable/deletable (i.e., config true, which is

5554   the default).  These data nodes may be considered sensitive or

5555   vulnerable in some network environments.  Write operations (e.g.,

5556   edit-config) to these data nodes without proper protection can have a

5557   negative effect on network operations.  For OSPF, the ability to

5558   modify OSPF configuration will allow the entire OSPF domain to be

5559   compromised including peering with unauthorized routers to misroute

5560   traffic or mount a massive Denial-of-Service (DoS) attack.  The

5561   security considerations of OSPFv2 [RFC2328] and [RFC5340] apply to

5562   the ietf-ospf.yang module as well.


[minor] s/OSPFv2 [RFC2328] and [RFC5340]/OSPFv2 [RFC2328] and OSPFv3
[RFC5340]


[major] How do the "security considerations of OSPFv2 [RFC2328] and
[RFC5340] apply to the ietf-ospf.yang module"?  rfc2328/rfc5340 only talk
about authentication of the protocol exchange...



...

5626 7.1.  Normative References

...

5679   [RFC4750]  Joyal, D., Ed., Galecki, P., Ed., Giacalone, S., Ed.,

5680              Coltun, R., and F. Baker, "OSPF Version 2 Management

5681              Information Base", RFC 4750, DOI 10.17487/RFC4750,

5682              December 2006, <https://www.rfc-editor.org/info/rfc4750>;.

...

5731   [RFC5643]  Joyal, D., Ed. and V. Manral, Ed., "Management Information

5732              Base for OSPFv3", RFC 5643, DOI 10.17487/RFC5643, August

5733              2009, <https://www.rfc-editor.org/info/rfc5643>;.

...

5745   [RFC5880]  Katz, D. and D. Ward, "Bidirectional Forwarding Detection

5746              (BFD)", RFC 5880, DOI 10.17487/RFC5880, June 2010,

5747              <https://www.rfc-editor.org/info/rfc5880>;.


5749   [RFC5881]  Katz, D. and D. Ward, "Bidirectional Forwarding Detection

5750              (BFD) for IPv4 and IPv6 (Single Hop)", RFC 5881,

5751              DOI 10.17487/RFC5881, June 2010, <https://www.rfc-

5752              editor.org/info/rfc5881>;.


[minor] I think these references can be Informative.



...

5880 7.2.  Informative References


[major] All the references in this section (except maybe rfc905) point at
functionality in the model...as the references above, these should be
Normative as well.


These documents are already in the Downref registry: rfc5309, rfc5443,
rfc5714, rfc6987.


The others are not, but I don't expect any issues in the IETF Last Call.