Skip to main content

YANG Data Model for Network Access Control Lists (ACLs)
RFC 8519

Yes

(Ignas Bagdonas)

No Objection

Alvaro Retana
(Deborah Brungard)
(Terry Manderson)

Note: This ballot was opened for revision 19 and is now closed.

Alvaro Retana No Objection

Warren Kumari (was Discuss) No Objection

Comment (2018-09-26 for -19)
--- 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.

(Ignas Bagdonas; former steering group member) Yes

Yes (for -19)

                            

(Adam Roach; former steering group member) No Objection

No Objection (2018-09-26 for -19)
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.

(Alexey Melnikov; former steering group member) No Objection

No Objection (2018-09-27 for -19)
I would have been "Yes" if I read the document more attentively.

Agreeing with Mirja's and Benjamin's DISCUSS points.

(Alissa Cooper; former steering group member) (was Discuss) No Objection

No Objection (2018-09-28 for -19)
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.

(Ben Campbell; former steering group member) No Objection

No Objection (2018-09-26 for -19)
I support Benjamin’s and Mirja’s DISCUSS points.

(Benjamin Kaduk; former steering group member) (was Discuss) No Objection

No Objection (2018-09-28 for -19)
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.

(Deborah Brungard; former steering group member) No Objection

No Objection (for -19)

                            

(Mirja Kühlewind; former steering group member) (was Discuss) No Objection

No Objection (2018-11-06)
Thanks for addressing my discuss!

(Suresh Krishnan; former steering group member) (was Discuss) No Objection

No Objection (2018-10-01 for -20)
Thanks for addressing my DISCUSS point.

(Terry Manderson; former steering group member) No Objection

No Objection (for -19)