Early Review of draft-ietf-netmod-acl-extensions-04
review-ietf-netmod-acl-extensions-04-yangdoctors-early-jethanandani-2024-01-02-00

I had started providing my review comments before I was assigned to (formally)
review the document. I will therefore (re)use that thread here. And in order to
understand the level of quotation, I have added either >>, a >, or nothing to
indicate the level.

================================================

>> I do support this work, as it is much needed, and would like to see it
progress. However, I do believe that the document needs to undergo a revision
to qualify for LC. Some of the comments are editorial or minor, and can be
addressed easily, but others are not. They should all be addressed for the WG
to call the document ready.

>> - The Security Considerations section has both the read/write nodes and the
read-only nodes as empty (or marked as TBC, which I imagine stands for To Be
Completed). This needs to be filled out, or if no nodes are worth any security
considerations, it should be stated so, and why.

> [Med] ACK. We don’t repeat what is already in 8519 but focus on key additions
in the spec: https://github.com/boucadair/enhanced-acl-netmod/pull/65/files

[mj] Thanks for updating the section.

s/setf/set/
s/Simialr/Similar/

and in other place
s/modelled/modeled/

>> - Isn’t the YANG model normative portion of the document? Isn’t what this
document all about? Why is it then in the Appendix?

> [Med] We are using a script to generate the IANA modules + we are actually
following this part from the 8407bis:

>   It is RECOMMENDED to include the URL from where to retrieve the
   recent version of the module.  When a script is used, the Internet-
   Draft that defines an IANA-maintained module SHOULD include an
   appendix with the initial full version of the module.  Including such
   an appendix in pre-RFC versions is meant to assess the correctness of
   the outcome of the supplied script.  The authors MUST include a note
   to the RFC Editor requesting that the appendix be removed before
   publication as RFC and that RFC IIII is replaced with the RFC number
   that is assigned to the document.  Initial versions of IANA-
   maintained modules that are published in RFCs may be misused despite
   the appropriate language to refer to the IANA registry to retrieve
   the up-to-date module.

[mj] I am not clear on what happens to the IANA module once the draft is
published as an RFC based on what you cite from 8407bis. The document states
that the reference to “RFC IIII” is replaced with the actual RFC number, but 
it also says that the Appendix be removed. What happens to the initial version
of the module itself? Is it removed if the Appendix is removed? Or does it
remain in the Appendix as an initial version, with language that indicates that
the IANA registry should be used to retrieve the most up-to-date model? The
language in Section 1.1 item (2) does not help.

The above text from 8407bis needs to be explicit on what happens to the initial
version of the module as part of the RFC publication.

>> - Why is the Section titled "Initial Version of the The ICMPv4 Types
IANA-Maintained Module”, when the model in question is
"iana-icmpv6-types@2020-09-25.yang”? > [Med] This was a typo. Fixed.

[mj] You fixed it another location. However, I still see the following in the
-04 version of the document.

B.2. Initial Version of the The ICMPv4 Types IANA-Maintained Module

<CODE BEGINS> file "iana-icmpv6-types@2020-09-25.yang"

module iana-icmpv6-types {

>> - ‘defined-sets’ and ‘aliases’ have been defined in a the separate model
‘ietf-acl-enh’. Are these sets and aliases defined to be used outside of ACL?
If that is the case then having them outside the ‘ietf-access-control-list’
model makes sense. Otherwise, almost everything in the ‘ietf-acl-enh’ is an
augmentation of the model defined in RFC 8519, as stated in the Introduction of
the document

> [Med] These are defined to be consumed for ACL policies.

>> "The YANG module in this document is solely based on augmentations to the
ACL YANG module defined in [RFC8519].”

> [Med] The intent was to highlight that we are not using a bis approach.
Tweaked the paragraph that includes that text for better clarity.

[mj] I think it already clear that this model an augmentation and not a bis. A
bis is when you take the original document and edit it for updates, and this is
clearly not that.

I actually agree with your above statement in the Introduction that you had,
about the module being solely an enhancement of the ACL YANG model, and was
surprised to see it taken out. The point I was making was that just like what
you have done with augmenting "/acl:acls/acl:acl/acl:aces/acl:ace/acl:matches”
to add ‘choice payload’, ‘choice alias’ etc, you could have augmented
“/acl:acls” to add ‘defined-sets’ and ‘aliases’. Right now, as is, the
ietf-acl-enh module sits on the root of the config tree, with no relation to
the ACL model, other than references to it from within the ACL model. If the
definitions in ietf-acl-enh are to be consumed by the ACL model only, why not
augment the ACL model (as shown below) to add them in the ACL tree?

>> If that is the case I see no reason why those containers should not be
augmentations into the same model, as in

>> augment “/acl:acls” {
  container defined-sets {
  ….
  }

  container aliases {
     …
  }
}

>> - I just pulled down the latest version (-03) of the draft, and ran into
this error.

>> $ pyang ietf-acl-enh@2022-10-24.yang
iana-icmpv6-types@2020-09-25.yang:1: error: unexpected latest revision
"2023-04-28" in iana-icmpv6-types@2020-09-25.yang, should be "2020-09-25”.

> [Med] Fixed. Thanks.

>> - Section 3.4. TCP Flags Handling. The document states that.

>> "Clients that support both 'flags-bitmask' and 'flags' matching fields MUST
NOT set these fields in the same request.”.

>> Can the model have a must statement to prevent this from being configured
inadvertently?

> [Med] We don’t see how to do that with a must statement, hence the normative
language in the narrative text.

[mj] How about something like

must
“not(/acl:acls/acl:acl/acl:aces/acl:ace/acl:matches/acl:l4/acl:tcp/acl:flags)” 
{
  error-message
    “Either flags or flags-bitmask should be configured, but not both.”;
}

under ‘flags-bitmask’?

If you are feeling adventurous, you could add a deviation add statement to add
a similar must statement under tcp/flags also :-).

>> Same for Section 3.5 Fragments Handling
> [Med] Same answer :-)

>> - There should be clear direction to the RFC Editor on what should be done
with revision dates. The same is true for other placeholder text. For example,
what is the RFC Editor to do with text "RFC XXXX"? > [Med] Done:
https://github.com/boucadair/enhanced-acl-netmod/pull/59/files

[mj] Thanks.

>> - References in the YANG model should be expanded to include the title of
the RFC.

> [Med] We are echoing references as listed in an IANA registry, so we do not
have control over that reference.

>> - Examples are always good. Not only can they be used to validate the model,
but users get to understand how it can be used. See other models such as BGP,
TCP, BFD on how an example can be added.

> [Med] We do already have many in the core document. Will consider adding more
if needed.

[mj] I am referring to the example as stated in Section 3.12 of RFC 8407. If by
core you are referring to RFC 8519, then unfortunately, we the authors missed
it too -:( But here is a module usage example from another draft.

https://datatracker.ietf.org/doc/html/draft-ietf-idr-bgp-model-17#name-creating-bgp-instance

>> - How is this a reference?
        reference
          "- Bill Simpson <mailto:Bill.Simpson&um.cc.umich.edu>

> [Med] We are echoing a reference as cited in an IANA registry, so we do not
have control over that reference.

[mj] Regardless, and I am repeating the question, how is this a reference? I
think having RFC 6918 as a reference is good enough.

And that brings up another point. The sections that contain the YANG models
need to list out all the references cited in the model at the beginning of the
section. For example, Section 4 needs to list RFC 9293, 3032, 792, 4443 etc. at
the beginning of the section, such that they are included in the Normative list
of references. See Section 3.9 of RFC 8407.