Skip to main content

Last Call Review of draft-ietf-netmod-acl-extensions-12
review-ietf-netmod-acl-extensions-12-yangdoctors-lc-andersson-2024-12-18-00

Request Review of draft-ietf-netmod-acl-extensions-11
Requested revision 11 (document currently at 14)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2024-11-15
Requested 2024-10-21
Requested by Mahesh Jethanandani
Authors Oscar Gonzalez de Dios , Samier Barguil , Mohamed Boucadair , Qin Wu
I-D last updated 2024-12-18
Completed reviews Yangdoctors Early review of -04 by Mahesh Jethanandani (diff)
Tsvart Last Call review of -11 by David L. Black (diff)
Intdir Last Call review of -11 by Tim Wicinski (diff)
Yangdoctors Last Call review of -12 by Per Andersson (diff)
Genart Last Call review of -13 by Russ Housley (diff)
Secdir Last Call review of -14 by Linda Dunbar
Comments
The draft is adding support for IPv6 Extension Headers, TCP Flags, fragment handling and payload filtering. I am hoping that the reviews can zoom in one some of these areas to make sure the document is not missing anything.
Assignment Reviewer Per Andersson
State Completed
Request Last Call review on draft-ietf-netmod-acl-extensions by YANG Doctors Assigned
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/dyc6b9M6fOiVvWw8CrSHcDp4GOU
Reviewed revision 12 (document currently at 14)
Result Ready w/issues
Completed 2024-12-18
review-ietf-netmod-acl-extensions-12-yangdoctors-lc-andersson-2024-12-18-00
Dear WG,

This is my Last Call review of draft-ietf-netmod-acl-extensions-12. My
conclusion is that the four modules are Ready with issues.

In general the modules are in good shape. The following issue from the
early yangdoctor review is still not addressed and should be fixed.

* RFCs cited in the YANG modules should be listed in the introductory
  section as references. Section 4 needs to add references to RFC 9293,
  4443, 3032, 792, and IEEE 802.1Q, IEEE 802.1ah

A general comment when using XSLT to create the initial IANA modules.
The supplied XSLT stylesheets use
<https://github.com/llhotka/iana-yang>.
I suggest this is mentioned because I did not know about this project
and had search for it myself by the import of iana-yinx.xsl.

Regarding match-on-payload, it is not understandable from Section 3.6
alone how the mechanism works. From my reading of the rest of the
document it works by matching the data after a given offset, with a
specific length, and computing the result of the operator applied to the
extracted payload and the binary pattern. I find the naming of
"offset-end" and "prefix" confusing, since "offset-end" is really a
length and not a position; suggest renaming it to e.g. "payload-length"
or "length". Regarding "prefix" I suggest renaming it to "pattern" or
"bitmask", since it might not be a prefix but an arbitrary part of a
payload and it is used for the matching.

ietf-acl-enh@2024-05-16.yang:

* pyang OK
* yanglint complains on imported module
  iana-icmp4-types@2020-09-25.yang..

Some identityrefs use prefix, some don't; suggest to be consistent and
add prefix to all. Especially since they are members of reusable
groupings.

L393: base tcp-flag;
L450: base label-positions;
L497: base offset-type;

iana-icmpv4-types@2020-09-25.yang:

* pyang fails with

    iana-icmpv4-types@2020-09-25.yang:280: error: unterminated statement
    definition for keyword "enum", looking at I

* yanglint validation fails:

    libyang err : Invalid character sequence
    "ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby",
    expected a keyword. (Line number 280.) libyang err : Parsing module
    "iana-icmpv4-types" failed. libyang err : Loading "iana-icmpv4-types"
    module failed. libyang err : Parsing module "ietf-acl-enh" failed.

Missing reference to RFC 2780 for Type 0, 3, 8, 9, 10, 11, 12, 13, 14,
and 40. I notice that there is a discrepancy in the IANA ICMP Parameters
registry listing of ICMP Type Numbers section and the Code Fields
section. Suggest to notify IANA and rebuild the YANG module.

The enum
ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby is
problematic because it is so long (when YANG modules should fit in 69
characters line length). This is why the tools can't validate the
module. Perhaps truncate it to
ICMPmessagesutilizedbyexperimentalmobilityprotocols?

Reading RFC 4065 it seems that ICMPExperimentalMobilityType is a fitting
name, since it is designed not only for Seamoby but also for other
experimental mobility protocols..

The following XSLT patch would remove "suchasSeamoby" from the enum
name.

--- iana-icmpv4-types.xsl.orig  2024-12-19 02:16:49.552187474 +0100
+++ iana-icmpv4-types.xsl       2024-12-19 02:16:01.684572966 +0100
@@ -63,8 +63,8 @@
                  '(Deprecated)')),' ','')"/>
          </when>
          <otherwise>
-           <value-of select="translate(normalize-space
-                   (iana:description),' ','')"/>
+           <value-of select="substring-before(translate(normalize-space
+                   (iana:description),' ',''),'suchasSeamoby')"/>
          </otherwise>
        </choose>
      </with-param>

iana-icmpv6-types@2023-04-28.yang:
* pyang OK
* yanglint OK

The following change is done when running

    $ pyang -f yang --yang-line-length=69 iana-icmpv6-types@2023-04-28.yang \
    > iana-icmpv6-types@2023-04-28.post.yang

    $ diff -u iana-icmpv6-types@2023-04-28.yang \
    > iana-icmpv6-types@2023-04-28.post.yang

-      enum ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby {
+      enum
+        ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby {

iana-ipv6-ext-types@2023-09-29.yang:
* pyang OK
* yanglint OK

Nits:

Quoting is wrong in Section 5

s/"iana-ipv6-ext-types defines"/"iana-ipv6-ext-types" defines/

Section 6.3.2 references [IANA-ICMPv4] when it should reference
[IANA-ICMPv6].

Section 6.3.3 references [IANA-ICMPv6] when it should reference
[IANA-IPv6].

--
Per