Einar, I think this change is fine, with one exception. I would rather the augment to the interface not be required for implementations that don't actually have interfaces. I understand that there may be two ways to go about this:
1. Separate out the augment into a separate module (same doc is fine); or 2. Somehow "feature-ize" the augment. I don't know how to do (2) but if you do, that's okay by me. Eliot On 16.12.17 14:19, Einar Nilsen-Nygaard (einarnn) wrote: > All, > > After a series of discussions on- and off-list, I have a candidate PR > that includes the changes in the PR Mahesh sent out plus some more > edits. Please see consolidated PR here: > > https://github.com/netmod-wg/acl-model/pull/21 > > > Main changes in addition to Mahesh’s PR are: > > * Moved interface attachment to be via an interface augmentation. > * Restructured port matches slightly under both IPv4 and IPv6 > containers. > * Removed unnecessary identity 'interface-acl-aggregate’. > * Removed action ‘icmp-off’, can be augmented later. > > > For reference, here is the current YANG tree plus “--ietf” logs: > > 13:12 $ pyang --ietf --lint -f tree ietf-access-control-list.yang > ietf-access-control-list.yang:51: error: bad value "YYYY-MM-DD" > (should be date) > module: ietf-access-control-list > +--rw access-lists > +--rw acl* [name] > +--rw name string > +--rw type? acl-type > +--rw aces > +--rw ace* [name] > +--rw name string > +--rw matches > | +--rw (l2)? > | | +--:(eth) > | | +--rw eth {match-on-eth}? > | | +--rw destination-mac-address? > yang:mac-address > | | +--rw destination-mac-address-mask? > yang:mac-address > | | +--rw source-mac-address? > yang:mac-address > | | +--rw source-mac-address-mask? > yang:mac-address > | | +--rw ethertype? > eth:ethertype > | +--rw (l3)? > | | +--:(ipv4) > | | | +--rw ipv4 {match-on-ipv4}? > | | | +--rw dscp? inet:dscp > | | | +--rw ecn? uint8 > | | | +--rw length? uint16 > | | | +--rw ttl? uint8 > | | | +--rw protocol? uint8 > | | | +--rw (source-port-range-or-operator)? > | | | | +--:(range) > | | | | | +--rw source-port-lower > inet:port-number > | | | | | +--rw source-port-upper > inet:port-number > | | | | +--:(operator) > | | | | +--rw source-operator > operator > | | | | +--rw source-port > inet:port-number > | | | +--rw (destination-port-range-or-operator)? > | | | | +--:(range) > | | | | | +--rw destination-port-lower > inet:port-number > | | | | | +--rw destination-port-upper > inet:port-number > | | | | +--:(operator) > | | | | +--rw destination-operator > operator > | | | | +--rw destination-port > inet:port-number > | | | +--rw ihl? uint8 > | | | +--rw flags? bits > | | | +--rw offset? uint16 > | | | +--rw identification? uint16 > | | | +--rw destination-ipv4-network? > inet:ipv4-prefix > | | | +--rw source-ipv4-network? > inet:ipv4-prefix > | | +--:(ipv6) > | | +--rw ipv6 {match-on-ipv6}? > | | +--rw dscp? inet:dscp > | | +--rw ecn? uint8 > | | +--rw length? uint16 > | | +--rw ttl? uint8 > | | +--rw protocol? uint8 > | | +--rw (source-port-range-or-operator)? > | | | +--:(range) > | | | | +--rw source-port-lower > inet:port-number > | | | | +--rw source-port-upper > inet:port-number > | | | +--:(operator) > | | | +--rw source-operator > operator > | | | +--rw source-port > inet:port-number > | | +--rw (destination-port-range-or-operator)? > | | | +--:(range) > | | | | +--rw destination-port-lower > inet:port-number > | | | | +--rw destination-port-upper > inet:port-number > | | | +--:(operator) > | | | +--rw destination-operator > operator > | | | +--rw destination-port > inet:port-number > | | +--rw destination-ipv6-network? > inet:ipv6-prefix > | | +--rw source-ipv6-network? > inet:ipv6-prefix > | | +--rw flow-label? > inet:ipv6-flow-label > | +--rw (l4)? > | | +--:(tcp) > | | | +--rw tcp {match-on-tcp}? > | | | +--rw sequence-number? uint32 > | | | +--rw acknowledgement-number? uint32 > | | | +--rw data-offset? uint8 > | | | +--rw reserved? uint8 > | | | +--rw flags? bits > | | | +--rw window-size? uint16 > | | | +--rw urgent-pointer? uint16 > | | | +--rw options? uint32 > | | +--:(udp) > | | | +--rw udp {match-on-udp}? > | | | +--rw length? uint16 > | | +--:(icmp) > | | +--rw icmp {match-on-icmp}? > | | +--rw type? uint8 > | | +--rw code? uint8 > | | +--rw rest-of-header? uint32 > | +--rw egress-interface? if:interface-ref > | +--rw ingress-interface? if:interface-ref > +--rw actions > | +--rw forwarding identityref > | +--rw logging? identityref > +--ro statistics {acl-aggregate-stats}? > +--ro matched-packets? yang:counter64 > +--ro matched-octets? yang:counter64 > > augment /if:interfaces/if:interface: > +--rw acls > +--rw ingress > | +--rw acl-sets > | +--rw acl-set* [name] > | +--rw name -> /access-lists/acl/name > | +--rw type? -> /access-lists/acl/type > | +--ro ace-statistics* [name] {interface-stats}? > | +--ro name -> > /access-lists/acl/aces/ace/name > | +--ro matched-packets? yang:counter64 > | +--ro matched-octets? yang:counter64 > +--rw egress > +--rw acl-sets > +--rw acl-set* [name] > +--rw name -> /access-lists/acl/name > +--rw type? -> /access-lists/acl/type > +--ro ace-statistics* [name] {interface-stats}? > +--ro name -> > /access-lists/acl/aces/ace/name > +--ro matched-packets? yang:counter64 > +--ro matched-octets? yang:counter64 > > Comments welcome! > > Cheers, > > Einar > > > >> On 14 Dec 2017, at 18:50, Einar Nilsen-Nygaard (einarnn) >> <[email protected] <mailto:[email protected]>> wrote: >> >> >> >>> On 14 Dec 2017, at 08:21, Sonal Agarwal <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Hi Einar, >>> >>> You had 3 questions for me on all the several e-mail threads. >>> 1. Global attachment point >>> 2. icmp-off >>> 3. acl-aggregate-interface stats. >>> >>> For (1), my first preference is to have the model define attachment >>> point for interfaces only. >> >> einarnn> I have some diffs, layered on top of Mahesh’s PR to >> netmod-wg/acl-model that do this. Nearly like the augmentation I have >> below. Feel free to take a look at: >> >> https://github.com/mjethanandani/acl-model/pull/3 >> >> >>> However, Kristian wants the global attachment point as well so that >>> he can add the ACL to the linux tables. >> >> einarnn> I think Kristian doesn’t feel a global attachment point >> needs to be in this first revision. But he can confirm. >> >>> If an ACL is attached globally, does this mean it is per direction >>> or does it mean it is across directions? >> >> einarnn> I don’t know right now :-) >> >>> This global ACL may not be applicable to any of Cisco's service >>> provider routers as I don't see any platform actually replicating >>> the ACL to all line cards and attaching it in ingress and egress >>> directions across all interfaces. >> >> einarnn> Per other emails, I don’t think we understand this enough >> yet to specify it, so I suggest we just leave it out for now. Nothing >> in the model prevents a “global attachment point” being added later >> once we understand what it really means. >> >>> For (2), I am ok with removing icmp-off. >> >> einarnn> Done in my PR above. >> >>> For (3), this would have to be a combination of ACL stats across all >>> interfaces for all ACL's. Something like this is possible on an XR >>> box where ACES have counter names associated with it. Let's chat >>> about this offline tomorrow. >> >> einarnn> I’ll ping you to clarify, and we can bring any conclusion >> back to the list. >> >> Cheers, >> >> Einar >> >> >> >>> Sonal. >>> >>> >>> On Wed, Dec 13, 2017 at 12:10 PM, Mahesh Jethanandani >>> <[email protected] <mailto:[email protected]>> wrote: >>> >>> We want to support “global” attachment point down the line, and >>> that “global” attachment point will be one of the choices (the >>> other being the interface), what would this augment look like. >>> Note, as far as I know, you cannot augment inside a choice node. >>> >>>> On Dec 13, 2017, at 6:57 AM, Einar Nilsen-Nygaard (einarnn) >>>> <[email protected] <mailto:[email protected]>> wrote: >>>> >>>> Perhaps like this, as an augmentation to the interface: >>>> >>>> augment /if:interfaces/if:interface: >>>> +--rw ingress-acls >>>> | +--rw acl-sets >>>> | +--rw acl-set* [name] >>>> | +--rw name -> /access-lists/acl/name >>>> | +--rw type? -> /access-lists/acl/type >>>> | +--ro ace-statistics* [name] {interface-stats}? >>>> | +--ro name -> >>>> /access-lists/acl/aces/ace/name >>>> | +--ro matched-packets? yang:counter64 >>>> | +--ro matched-octets? yang:counter64 >>>> +--rw egress-acls >>>> +--rw acl-sets >>>> +--rw acl-set* [name] >>>> +--rw name -> /access-lists/acl/name >>>> +--rw type? -> /access-lists/acl/type >>>> +--ro ace-statistics* [name] {interface-stats}? >>>> +--ro name -> >>>> /access-lists/acl/aces/ace/name >>>> +--ro matched-packets? yang:counter64 >>>> +--ro matched-octets? yang:counter64 >>>> >>>> >>>> Could also put an “aces” container above both these & rename >>>> “ingress-acls" to “ingress”, etc. to give a single root for the >>>> augmentation if preferred. >>>> >>>> Cheers, >>>> >>>> Einar >>>> >>>> >>>>> On 6 Dec 2017, at 19:43, Eliot Lear <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> >>>>> >>>>> On 12/6/17 7:23 PM, Mahesh Jethanandani wrote: >>>>>> How does one move the interface attachment point, currently an >>>>>> 'interface-ref', to an augmentation of the >>>>>> if:interfaces/interface, >>>>>> inside of the ‘acl’ container? Down the line we might need >>>>>> to have an >>>>>> container for "attachment points" to accommodate the >>>>>> possibility of >>>>>> attaching an ACL either to an interface or “globally”. >>>>>> >>>>> >>>>> Keeping in mind that one use is that an ACL doesn't attach to an >>>>> interface at all. >>>>> >>>>> _______________________________________________ >>>>> netmod mailing list >>>>> [email protected] <mailto:[email protected]> >>>>> https://www.ietf.org/mailman/listinfo/netmod >>>>> <https://www.ietf.org/mailman/listinfo/netmod> >>>> >>> >>> Mahesh Jethanandani >>> [email protected] <mailto:[email protected]> >>> >>> >>> _______________________________________________ >>> netmod mailing list >>> [email protected] <mailto:[email protected]> >>> https://www.ietf.org/mailman/listinfo/netmod >>> <https://www.ietf.org/mailman/listinfo/netmod> >>> >>> >> >> _______________________________________________ >> netmod mailing list >> [email protected] <mailto:[email protected]> >> https://www.ietf.org/mailman/listinfo/netmod > > > > _______________________________________________ > netmod mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/netmod
signature.asc
Description: OpenPGP digital signature
_______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
