Ballot for draft-ietf-netmod-acl-model
Yes
No Objection
Note: This ballot was opened for revision 19 and is now closed.
--- original DISCUSS for archives ---- Be ye not afraid -- this DISCUSS is easily cleared, but sufficiently important that I thought it worth making, and making sure it didn't slip through the cracks. The description for match-on-ipv4 says: "The device can support matching on IPv4 headers.", but the description for 'match-on-tcp', 'match-on-udp', 'match-on-icmp' say: "The device can support <protocol> headers." I really think that these need to be "The device can support matching on <protocol> headers." ------ Section 1: "In case a vendor supports it, metadata matches apply to fields associated with the packet but not in the packet header such as input interface or overall packet length". I don't have a suggested replacement, but seeing as this is introductory text, I figured it was aimed at people not familiar with how forwarding / filtering works. I'm slightly concerned that some people will get confused, because almost all protocols include a "packet length" in the header. Perhaps just dropping the "or overall packet length"? (Yes, we could get into a long thing on protocol packet length, and overall length, etc, but that's likely to not be helpful in the document). Section 2: Nit: "It is very important that model can be used easily by applications/attachments." models. Section 3: "Packet header matching applies to fields visible in the packet such as address or CoS or port numbers." CoS isn't expanded, and isn't in the well known acronyms list. RFC2474 perhaps? Section 3: "These include features such as "Device can support ethernet headers" or "Device can support of IPv4 headers". "can support of" makes no sense. Also, I *think* Ethernet is uppercase. This is a nit.
Thanks to everyone who contributed their time and knowledge to this document. I have two minor comments. Throughout the data module, the terms "ace" and "ACE" are used interchangeably. It would probably be good to rationalize these (I would suggest "ACE"). --------------------------------------------------------------------------- §4.3 and 4.4: These examples use IPv4 addresses exclusively. Please update to use IPv6 or a mix of IPv4 and IPv6. See https://www.iab.org/2016/11/07/iab-statement-on-ipv6/ for additional information.
I would have been "Yes" if I read the document more attentively. Agreeing with Mirja's and Benjamin's DISCUSS points.
Thanks all for getting my question about the IEEE answered. Original COMMENT: Sec 1: s/Policy Based Routing, Firewalls etc./policy-based routing, firewalls, etc./ "The matching of filters and actions in an ACE/ACL are triggered only after application/attachment of the ACL to an interface, VRF, vty/tty session, QoS policy, routing protocols amongst various other config attachment points." This is a sentence fragment. s/in the ACE's/in the ACEs/ Sec 3.1: "There are two YANG modules in the model." Is this technically correct, given that ietf-ethertypes is also defined here? Also, I don't think the definition of ietf-ethertypes belongs in an appendix under the heading "Extending ACL model examples." I can imagine that other modules will want to import this module and that seems like a strange place to put it. Sec 4.1: For avoidance of confusion, I would suggest replacing "l2," "l3," and "l4" with "layer2," "layer3," and "layer4," respectively. s/Definitions of action for this ace entry/Definitions of action for this ACE entry/ s/Specifies the forwarding action per ace entry/Specifies the forwarding action per ACE entry/ Sec 4.2: "This module imports definitions from Common YANG Data Types [RFC6991] and references IP [RFC0791], ICMP [RFC0792], Definition of the Differentiated Services Field in the IPv4 and IPv6 Headers [RFC2474], The Addition of Explicit Congestion Notification (ECN) to IP [RFC3168], , IPv6 Scoped Address Architecture [RFC4007], IPv6 Addressing Architecture [RFC4291], A Recommendation for IPv6 Address Text Representation [RFC5952], IPv6 [RFC8200]." It looks like something is missing from this list, possibly RFC 793. Sec 5: In this section or elsewhere it would be nice to see a sentence noting that this YANG model allows the configuration of packet logging, which if used would additionally warrant protections against unauthorized log access and a logs retention policy.
I support Benjamin’s and Mirja’s DISCUSS points.
Thank you for quickly (and, partially, preemptively!) addressing my Discuss points. Original comments preserved below. I tried to call out the editorial nits as such; there are a couple non-editorial comments embedded within. Section 1 The match criteria allows for definition of packet headers and metadata, all of which must be true for the match to occur. nit: Is this missing a word like "contents"? The matching of filters and actions in an ACE/ACL are triggered only after application/attachment of the ACL to an interface, VRF, vty/tty session, QoS policy, routing protocols amongst various other config attachment points. nit: I think the end of this list needs some clarification/termination, like "and routing protocols, amongst" Section 3 The match criteria allows for definition of packet headers or metadata, if supported by the vendor. [...] (same nit as above re "contents") Metadata matching applies to fields associated with the packet, but not in the packet header such as input interface, packet length, or source or destination prefix length. The actions can be any sort of nit: comma after "not in the packet header" Section 4.1 nit: The feature match-on-udp and -icmp descriptions should probably use the plural "headers" to match the other features' descriptions. The mixed-<blah> features seem to implicitly assume that if features X and Y are individually supported, then the combination is also supported. I could imagine that there might exist hardware for which that assumption is not true, but don't know if there actually is any such hardware or it's common enough to be worth caring about here. grouping acl-counters { leaf matched-packets { [...] An implementation should provide this counter on a per-interface per-ACL-entry if possible. nit: missing "basis"? (Also in subsequent instances.) Section A.1 It's unclear that using abc@newco.com (in particular, the @newco.com part) in an example is reasonable; @newco.example would be better.
Thanks for addressing my discuss!
Thanks for addressing my DISCUSS point.