Re: [netmod] WG Last Call: draft-ietf-netmod-acl-model-14

[Sonal Agarwal <[e-mail]>]

I have reviewed the changes and they look good to me.

Thanks,
Sonal.


On Fri, Jan 12, 2018 at 2:07 PM, Mahesh Jethanandani <
[e-mail]> wrote:

> An updated version of the draft, along with changes to remove icmp-off
> from the model, and updates to examples has been posted in the PR here
> <https://github.com/netmod-wg/acl-model/pull/20>. If there are no
> objections to the changes by Tuesday, I will pull in the PR, and publish
> the draft.
>
> Cheers.
>
> On Jan 12, 2018, at 7:35 AM, Eliot Lear <[e-mail]> wrote:
>
> Ok.  What is left to agree on at this point?
>
> Thanks Mahesh,
>
> Eliot
>
> On 11.01.18 02:21, Mahesh Jethanandani wrote:
>
> Hi Einar,
>
> I can work on updating the draft as soon as we agree on the changes.
> Should take only a couple of days to turn around and publish the draft.
>
> On Jan 9, 2018, at 11:35 PM, Eliot Lear <[e-mail]> wrote:
>
> Hi Mahesh,
>
> Thanks for this work.  I think this is okay.  In the case of MUD we simply
> won't have the other container.  Can I please ask that you get the draft
> out quickly as draft-ietf-opsawg-mud has been waiting quite some time for
> this work to complete.
>
> Eliot
>
> On 10.01.18 04:08, Mahesh Jethanandani wrote:
>
> I have pulled in the changes as they relate to:
>
> - moving “interface-acl” under the container “attachment-points” making it
> local to that container.
> - reverting “acl-type” to “type”
> - removed “interface-all-aggregate” feature
> - simplified source port and destination port definition
>
> The pull request for the changes can be found here.
>
> https://github.com/netmod-wg/acl-model/pull/20
>
> After discussing with some of the original contributors, decided not to
> include the change as it relates to augmenting ietf-interfaces. We did not
> find that the change had a particular advantage over the current
> implementation. Even if we do not completely understand how ACLs might be
> attached “globally” or on something that is not an interface, having the
> flexibility to attach them to other attachment points is important. Keeping
> it as interface-ref gives us that flexibility.
>
> Cheers.
>
> On Dec 18, 2017, at 4:31 AM, Eliot Lear <[e-mail]> wrote:
>
> So long as nobody expects an interface construct in a MUD file, I'm happy.
>
> On 17.12.17 15:34, Einar Nilsen-Nygaard (einarnn) wrote:
>
> Eliot,
>
> Nothing can force an implementation to have to implement either
> the ietf-interfaces model or the augmentation in the
> ietf-access-control-list model. I appreciate your desire for modularity and
> cohesiveness, but I would resist #1, because I feel that the majority of
> users will be targeting interface-based attachment over time. I’ve adde
> back in use of the “interface-attachment” feature (which I took out as part
> of refactoring interface attachment). Part of:
>
> https://github.com/netmod-wg/acl-model/pull/21
>
>
> The augments part of the tree now looks like:
>
>   augment /if:interfaces/if:interface:
>     +--rw acls *{interface-attachment}*?
>        +--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
>
> Cheers,
>
> Einar
>
>
> On 17 Dec 2017, at 11:29, Eliot Lear <[e-mail]> wrote:
>
> 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) <
> [e-mail]> wrote:
>
>
>
> On 14 Dec 2017, at 08:21, Sonal Agarwal <[e-mail]> 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 <
> [e-mail]> 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) <
>> [e-mail]> 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/nam
>> e
>>     |           +--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/nam
>> e
>>                 +--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 <[e-mail]> 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
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/netmod
>>
>>
>>
>> Mahesh Jethanandani
>> [e-mail]
>>
>>
>> _______________________________________________
>> netmod mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/netmod
>>
>>
>
> _______________________________________________
> netmod mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netmod
>
>
>
>
> _______________________________________________
> netmod mailing [e-mail]://www.ietf.org/mailman/listinfo/netmod
>
>
>
>
> _______________________________________________
> netmod mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netmod
>
>
> Mahesh Jethanandani
> [e-mail]
>
>
>
> _______________________________________________
> netmod mailing [e-mail]://www.ietf.org/mailman/listinfo/netmod
>
>
>
> Mahesh Jethanandani
> [e-mail]
>
>
>
> Mahesh Jethanandani
> [e-mail]
>
>
> _______________________________________________
> netmod mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netmod
>
>