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

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 1 Nov 2017 17:13:18 +0630

Kristian,

I think there is confusion in how the ACL model is going to be implemented by vendors and used by customers.

As Eliot alluded to, the model is trying to address the issue of the capabilities of each platform as they exist across the industry but also within each vendor. So the first thing an implementation would do is to pick which feature they do support. A low end platform that supports only layer 2 ACL will pick l2-acl as their feature, while a high end router capable of support l2 and l3, ipv4 and ipv6 ACL will declare l2-l3-ipv4-ipv6 as the feature they support. That pretty much takes out all the other containers in the matches container. The additional features they might want to choose include support for TCP, UDP and ICMP. For a high end router this boils down to having definitions for

- ethernet
- ipv4
- ipv6
- tcp
- udp
- icmp

which is the list you have in mind, but this time making sure that the platform is capable of supporting each one of the definitions. Imagine if the low end platform advertised a model for all the above capabilities only to reject them when you tried to configure a ipv6 address that it already knows it cannot support.

Similarly the condition on tcp/udp/icmp container is to make sure the platform is capable of supporting them. Only if the platform declares tcp-acl, udp-acl, and icmp-acl feature, will those containers be visible from a configuration perspective.

The acl-type is used as a check to make sure the user is aware what kind of ACL the platform supports and the ACL they are trying to configure matches the acl-type. If the platform declared the feature l2-acl and if the user tries to set the ACL type to l2-l3-ipv4-ipv6 then the configuration would be rejected. In the Linux/nftables case, the platform should declare the feature l2-l3-ipv4-ipv6, tcp-acl, udp-acl, and icmp-acl if that is what it wants to support. The interface attachment point has been defined but it is not mandatory that a configuration has to define it. So if in Linux, the ACL list is global, one would not define a attachment point, which implies it is a global list.

I will take a look at the remaining issues/comments you raise, but I wanted to make sure that the overall design of the model was clear, which from this email I can only guess could be made more clear.

Cheers.

> On Oct 31, 2017, at 4:55 PM, Kristian Larsson <[email-masked]> wrote:
> 
> On Fri, Oct 20, 2017 at 09:37:04PM +0000, Kent Watsen wrote:
>> 
>> All,
>> 
>> This starts a two-week working group last call on
>> draft-ietf-netmod-acl-model-14.
>> 
>> The working group last call ends on November 3.
>> Please send your comments to the netmod mailing list.
> 
> 
> I initially read this draft (or an older version) close to a year
> ago and meant to give feedback back then. I first wanted to read
> up on the list archive on any previous discussions (since the
> initial draft is reaaaally old) but ran out of time.
> 
> I have still not read all previous discussions (there are 687
> emails when searching for 'acl' in the archive) and therefore
> I'll apologise beforehand as I'm bound to reiterate questions or
> topics that have been brought up already. I'd be grateful if
> those with the history in memory can help me by providing
> references to the list archive.
> 
> Anyway, to the model. There's this concept of unified ACLs. I
> see two dimensions:
> * mixed layer ACLs, where we match on headers in different
>   layers in the OSI/TCP/IP model, like ethernet and ipv4
> * mixed family ACLs, where we in one ACL match on different
>   protocols in the same OSI model layer, like both IPv4 and
>   IPv6. However, within one ACE we always just match on one
>   protocol.
> 
> What I don't understand is why under the matches container there
> are these other containers, like l2-acl, ipv4-acl, ipv6-acl,
> l2-l3-ipv4-acl etc? Depending on the type I would input the
> ethernet matches in different places. That seems suboptimal.  If
> we wanted an ACL type per AFI then we could have just created a
> top level list per AFI instead of trying to pretend they are
> unified by putting them in one list and then splitting it up
> further down under the matches container. In that case, the
> attachment points would also have been made much simpler with
> just a single reference instead of having two leaves like the
> current model.
> 
> It seems to me that this can be modeled in a more elegant way by
> having the following containers under matches:
> * ethernet
> * ipv4
> * ipv6
> * tcp
> * udp
> * icmp
> 
> The ethernet containers presence is conditioned on the acl-type
> being one of l2-acl, l2-l3-ipv4-acl, l2-l3-ipv6-acl or
> l2-l3-ipv4-ipv6-acl.
> 
> The ipv4 container is conditioned on the acl-type being one of
> ipv4-acl, l2-l3-ipv4-acl, l2-l3-ipv4-ipv6-acl.
> 
> The ipv6 container is conditioned on the acl-type being one of
> ipv6-acl, l2-l3-ipv6-acl, l2-l3-ipv4-ipv6-acl.
> 
> In addition, there is a condition that prevents both the IPv4 and
> IPv6 container being present at the same time, since we can't
> match on both of them with the same ACE. Another ACE in the same
> ACL can however match on something else.
> 
> Similarly, there's a condition on tcp, udp and icmp preventing
> them all from being configured. Perhaps it should just look
> differently, like a choice? Or maybe a match on protocol=tcp/udp
> and then we have a container for tcp-flags etc.  We can delve
> into the details later, I just want to first understand why the
> current model is thought of as a good approach for expressing
> this data? IMHO it's awkward.
> 
> 
> This brings us to the acl-type. It seems to me that this is
> primarily for being able to do YANG validation when a device does
> NOT support a unified model. I.e. if Linux nftables was all we
> wanted to model, then we wouldn't need this and the only
> (implied) acl-type would be l2-l3-ipv4-acl. In reality though, we
> need acl-type because most current network devices out there have
> per-AFI types and we want to be able to say:
> * this interface attachment point can only do ipv4-acl
> And still be able to validate the data based on the YANG model.
> Is this correct? It seems like one hell of a design trade-off to
> be able to achieve that. Wouldn't we be better off with actually
> having different list of ACLs, again vastly simplifying the
> attachment points and making data validation much easier?
> 
> If all we want to do is limit so the source address can't be
> configured to be an IPv4 address when the destination address is
> IPv6 I think it's better to have a "family" leaf per ACE that
> defines ipv4 or ipv6, or just let the ipv4 and ipv6 containers be
> mutually exclusive through other means, as I eluded to
> previously.
> 
> 
> The current attachment points seem to be a list of interfaces
> using the interface-ref type from ietf-interfaces. I guess there
> was a reason we don't augment the ietf-interfaces module. What if
> the device is Linux with nftables? There's no attachment to an
> interface as it's a global rule list. I think this is
> conceptually the same as attaching the same ACL on all interfaces
> but that would be an awkward way of describing a global
> attachment point. Would it not be better to if-feature wrap this
> and allow a global attachment point which has a more direct
> mapping to nftables? The same is of course for any device type
> with a global table, like most firewalls.
> 
> 
> 
> Other issues / questions;
> * in 1. mentions it can be used in routing protocols - is
>   that really intended?
> * in 1. says "In ordet to apply an ACL to any attachment
>   point, vendors would have to augment the ACL YANG model", is
>   this really true? Surely we have standard attachment points.
> * in 1. the examples of use start with policy based
>   routing and then firewalls. ISTM that ACLs are primarily used for
>   "packet filters" so it's weird it's not even included.
>   Firewall often implies statefulness, which is not really what
>   we are dealing with here and PBR is not nearly as use as
>   packet filters. Maybe everyone knows this already, but then
>   why write anything at all?
> * in 1. "in case vendors supports it, metadata matches apply.." why
>   include a condition on if the vendors supports it? this is
>   true for anything, "in case the vendor supports it, the BGP
>   routing protocol works this way...". I think we can require
>   certain metadata matches in the model, or just do if-feature,
>   but constantly prefixing everything with a "in case vendor.."
>   is unnecessary IMHO
> * in 1. ISTM: s/networked devices/networking device/
> * in 3. "each ACE has a group of match criteria and a group of action
>   criteria" - no, it does not, actions are not a criteria!?
> * indent is mix of tabs and spaces
> * the icmp-off action leaf is IMHO weirdly modeled and it's a
>   weird option in itself - can you point to vendors implementing
>   similar options? this seems doable by just having an ACE match
>   on ICMP and action=drop
> * why eth-acl vs l2-acl. this is mixing apples and pears. L2 is
>   a layer in the TCP/IP model whereas ethernet is one
>   implementation of an L2 protocol. Why name the identify
>   eth-acl and the match container l2-acl?
> * why have the "acl-sets" container? why not just have the list
>   directly?
> * the leafrefs in the interface-acl grouping are relative making
>   it impossible to re-use the grouping at a different "depth"
> * letting the matched-packets be EITHER per-interface per-ACE OR
>   per-ACE across all interfaces seems insane. We have to know
>   what we are getting back. Better to have separate counters
>   then and let vendor fill in one or the other? Or declare
>   deviations? Curreny mode is not useful at all.
> 
> Again, apologies for my ignorance.
> 
> Kind regards,
>   Kristian.
> 
> 
> -- 
> Kristian Larsson                                        KLL-RIPE
> +46 704 264511                                [email-masked]
> 
> _______________________________________________
> netmod mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netmod

Mahesh Jethanandani
[email-masked]

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 1 Nov 2017 12:22:49 +0100

Mahesh,

I think the question is why we need to have different match containers
for each possible feature set combination instead of having a single
match container with groups of leafs in it marked as features. This
would seem to cut down the size of the module and the tree diagram
significantly. I think this will also make clients simpler sicne they
do not have to select a certain container based on the feature set
announced. BTW, how do I filter on TCP flags in combination with
a source IP address? There seem to be reasonable combinations of
features not even covered.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: Kristian Larsson <[email-masked]>
Date: Wed, 1 Nov 2017 14:26:31 +0100

Mahesh,

On Wed, Nov 01, 2017 at 05:13:18PM +0630, Mahesh Jethanandani wrote:
> I think there is confusion in how the ACL model is going to be
> implemented by vendors and used by customers.
>
> As Eliot alluded to, the model is trying to address the issue
> of the capabilities of each platform as they exist across the
> industry but also within each vendor. So the first thing an
> implementation would do is to pick which feature they do
> support. A low end platform that supports only layer 2 ACL will
> pick l2-acl as their feature, while a high end router capable
> of support l2 and l3, ipv4 and ipv6 ACL will declare
> l2-l3-ipv4-ipv6 as the feature they support. That pretty much
> takes out all the other containers in the matches container.
> The additional features they might want to choose include
> support for TCP, UDP and ICMP. For a high end router this boils
> down to having definitions for
> 
> - ethernet
> - ipv4
> - ipv6
> - tcp
> - udp
> - icmp
> 
> which is the list you have in mind, but this time making sure
> that the platform is capable of supporting each one of the
> definitions. Imagine if the low end platform advertised a model
> for all the above capabilities only to reject them when you
> tried to configure a ipv6 address that it already knows it
> cannot support.

You imply that the low end platform would advertise features that
it doesn't support. Why would it do that?

I don't suggest removing features - only restructure where the
data is held.  In my example, under the ethernet container I can
do: if-feature "eth-acl or eth-ipv4-acl..."; to check if the
device supports a given feature so why are you saying that we
need different containers for the ethernet matching part of an
ACL of type eth-acl vs say eth-ipv4-acl?

Right now the features themselves affect the structure of the
model and thus the data, which I rather dislike. The config to
match e.g. ethernet headers should always go in the same place.

The model gives the illusion of supporting "unified" ACLs but
actually doesn't at all.

> Similarly the condition on tcp/udp/icmp container is to make
> sure the platform is capable of supporting them. Only if the
> platform declares tcp-acl, udp-acl, and icmp-acl feature, will
> those containers be visible from a configuration perspective.

And if the device supports all of those features I can write an
ACE that matches on TCP flags at and ICMP types.. and it would
validate according to the model but cleary makes no sense.

> The acl-type is used as a check to make sure the user is aware
> what kind of ACL the platform supports and the ACL they are
> trying to configure matches the acl-type. If the platform
> declared the feature l2-acl and if the user tries to set the
> ACL type to l2-l3-ipv4-ipv6 then the configuration would be
> rejected.

Another way of structuring this is to have completely different
lists of ACLs where each type of ACL is its own list, e.g.
 * eth-acl
 * ipv4-acl
 * ipv6-acl
 * eth-ipv4-ipv6-acl

What advantage do you think your currently proposed model holds
over such an approach?

What are your thoughts on the attachment point using two leaves?
Are you satisfied with this?

> In the Linux/nftables case, the platform should
> declare the feature l2-l3-ipv4-ipv6, tcp-acl, udp-acl, and
> icmp-acl if that is what it wants to support. The interface
> attachment point has been defined but it is not mandatory that
> a configuration has to define it. So if in Linux, the ACL list
> is global, one would not define a attachment point, which
> implies it is a global list.

Naturally one has to define an attachment point or the system
wouldn't know which one of potentially multiple ACLs to apply to
nftables. It might however be up to another YANG model to define
that attachment point.

The list of features seem to align poorly with reality. How can
we have a list of attachment points in the model but without
if-feature wrapping it? Surely a Linux machine must be able to
announce that it doesn't support per-interface ACLs? A side
question is why that attachment list exists in the first place -
why isn't it an augmentation of the interfaces module?

   kll

---

From: Kristian Larsson <[email-masked]>
Date: Wed, 1 Nov 2017 16:57:58 +0100

I think there's a problem with the current approach for features
where it conflates the limitations of the device with the
limitations of an attachment point.  Ultimately it is the
limitations of the attachment point that matter. Creating an ACL
in config on a device doesn't actually mean anything until you
attach it somewhere.

It's not hard to imagine a device where different limitations
apply, let's say a fairly simple Ethernet switch built on some
commodity chip. It consists of, say:
 * 24 * 10GE on a commodity chip
 * some control plane (a x86 PC) connected to that commodity chip

The commodity chip supports matching ethernet headers, so ACLs
attached to those ports are "eth-acl". The control plane on the
other hand supports complex matching on anything you can imagine.
What YANG features should such a device advertise?

   kll

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 2 Nov 2017 06:13:04 +0630

Juergen,

> On Nov 1, 2017, at 5:52 PM, Juergen Schoenwaelder <[email-masked]> wrote:
> 
> Mahesh,
> 
> announced. 

The current design of match containers was chosen to allow platforms to select one container that matched what the hardware supported from a l2, l3 and ipv{4,6} perspective. I would argue that even though the overall diagram is bigger with this design, once the platform selects the container of choice, the tree and the configuration itself would be a little simpler/smaller. 

Take the case where the desired selection is l2,-l3, ipv4 and ipv6. The current tree looks like this:

        |        |  +--rw l2-l3-ipv4-ipv6-acl {l2-l3-ipv4-ipv6-acl}?
        |        |  |  +--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 dscp?                           inet:dscp
        |        |  |  +--rw ecn?                            uint8
        |        |  |  +--rw length?                         uint16
        |        |  |  +--rw ttl?                            uint8
        |        |  |  +--rw protocol?                       uint8
        |        |  |  +--rw source-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operation?    operator
        |        |  |  +--rw destination-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operations?   operator
        |        |  |  +--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
        |        |  |  +--rw next-header?                    uint8
        |        |  |  +--rw destination-ipv6-network?       inet:ipv6-prefix
        |        |  |  +--rw source-ipv6-network?            inet:ipv6-prefix
        |        |  |  +--rw flow-label?
        |        |  |          inet:ipv6-flow-label

whereas, if the design went with one match container with each group of leafs in their own container (to support the if-feature statement for that container), the tree would look like this:

        |        |  +--rw l2-acl {l2-acl}?
        |        |  |  +--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 ipv4-acl {ipv4-acl}?
        |        |  |  +--rw dscp?                       inet:dscp
        |        |  |  +--rw ecn?                        uint8
        |        |  |  +--rw length?                     uint16
        |        |  |  +--rw ttl?                        uint8
        |        |  |  +--rw protocol?                   uint8
        |        |  |  +--rw source-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operation?    operator
        |        |  |  +--rw destination-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operations?   operator
        |        |  |  +--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
        |        |  +--rw ipv6-acl {ipv6-acl}?
        |        |  |  +--rw dscp?                       inet:dscp
        |        |  |  +--rw ecn?                        uint8
        |        |  |  +--rw length?                     uint16
        |        |  |  +--rw ttl?                        uint8
        |        |  |  +--rw protocol?                   uint8
        |        |  |  +--rw source-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operation?    operator
        |        |  |  +--rw destination-port-range!
        |        |  |  |  +--rw lower-port    inet:port-number
        |        |  |  |  +--rw upper-port?   inet:port-number
        |        |  |  |  +--rw operations?   operator
        |        |  |  +--rw next-header?                uint8
        |        |  |  +--rw destination-ipv6-network?   inet:ipv6-prefix
        |        |  |  +--rw source-ipv6-network?        inet:ipv6-prefix
        |        |  |  +--rw flow-label?                 inet:ipv6-flow-label

The difference though is small and comes down to a preference. Select one feature statement and get one container with everything in it, or define multiple feature statements and assemble together the pieces to define the ACE entry.

> BTW, how do I filter on TCP flags in combination with

If the platform has declared support for feature that includes ipv4 (and/or ipv6) and tcp-acl, for a given ACE entry, it should be able to define a match filter that includes both the source IP address and the TCP flags. Do you see something that prevents it?

Thanks.

Mahesh Jethanandani
[email-masked]

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 2 Nov 2017 06:35:25 +0630

Kristian,

> On Nov 1, 2017, at 10:27 PM, Kristian Larsson <[email-masked]> wrote:
> 

The feature statement is a “static” statement that a particular implementation defines for what it supports across the entire device. YANG does not allow for a feature to be defined for a particular node in the model. 

For the behavior you desire, one would define a feature statement that covers everything the device supports, and will have to use must statements to control what parts of the model get supported by the ports and what parts of the model are supported in the control plane. That will usually be a implementation level detail that this model cannot cover.

Thanks.

Mahesh Jethanandani
[email-masked]

---

From: Kristian Larsson <[email-masked]>
Date: Thu, 2 Nov 2017 08:43:18 +0100

On Thu, Nov 02, 2017 at 06:13:04AM +0630, Mahesh Jethanandani wrote:
>     On Nov 1, 2017, at 5:52 PM, Juergen Schoenwaelder <
>     [email-masked]> wrote:
> 
>     Mahesh,
> 
> The current design of match containers was chosen to allow platforms to select
> one container that matched what the hardware supported from a l2, l3 and ipv
> {4,6} perspective.

Sure, but you are conflating the structure of the model with the
feature-wraps. Without changing the features of the model, we can
structure it in a different way where there is not a 1:1 mapping
between features and containers under the matches container.

> I would argue that even though the overall diagram is bigger
> with this design, once the platform selects the container of choice, the tree
> and the configuration itself would be a little simpler/smaller. 

I am arguing the opposite. It's really awkward to place the same
type of data, like IPv4 match conditions, under different paths
based on a feature.

If the system model had done the same we would have:
/system
/system-with-ntp
/system-with-ntp-and-radius

How do you augment in new leaves? While you are using groupings
which allows for a reuse of data across all these containers,
someone who is augmenting can't, since you can't augment a
container, only where it is used. Someone wishing to add a leaf
to the model needs to augment in three different locations to
support a new match condition for IPv4 (let's say some meta-data
attribute).

> whereas, if the design went with one match container with each group of leafs
> in their own container (to support the if-feature statement for that
> container), the tree would look like this:
> 
> 
> The difference though is small and comes down to a preference. Select one
> feature statement and get one container with everything in it, or define
> multiple feature statements and assemble together the pieces to define the ACE
> entry.

Again, I think you mix up the features available vs the structure
of the data.

This is the current list of features (which again, I want to
change but that's separate):
 * l2-acl
 * ipv4-acl
 * ipv6-acl
 * mixed-ipv4-acl
 * mixed-ipv6-acl
 * l2-l3-ipv4-ipv6-acl
 * tcp-acl
 * udp-acl
 * icmp-acl
 * any-acl

As per your second example above we have an ipv4-acl container
under matches, i.e. /access-list/acl/aces/ace/matches/ipv4-acl.
It is only possible to define ipv4 matches if one of the
following features are present:
 * ipv4-acl
 * mixed-ipv4-acl
 * l2-l3-ipv4-ipv6-acl

Thus you write an if-feature statement to reflect that, like
this:

 if-feature "ipv4-acl mixed-ipv4-acl l2-l3-ipv4-ipv6-acl";

So you see, the tight coupling you have between the data
structure and the if-features isn't necessary.

I think the structure should first be established without
features and then features can be inserted where they make sense
to make part of the model optional. Starting with the list of
features and then designing the structure around them makes for a
much less natural data structure IMHO.

Kind regards,
   Kristian.

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 2 Nov 2017 09:19:50 +0100

> Take the case where the desired selection is l2,-l3, ipv4 and ipv6. The current tree looks like this:

[...]
 
> whereas, if the design went with one match container with each group of leafs in their own container (to support the if-feature statement for that container), the tree would look like this:

[...]
 
> The difference though is small and comes down to a preference. Select one feature statement and get one container with everything in it, or define multiple feature statements and assemble together…

Well, you leave out all the duplication you have and you leave out
that the client needs to calculate where to configure an acl based on
the feature set of a server. Copying the acl from one system to
another requires to adjust containers for not good reason.

The number of feature combinations will grow soon out of control and
things get worse if vendors follow the direction and define vendor
specific feature combinations and associated containers.
 
> If the platform has declared support for feature that includes ipv4 (and/or ipv6) and tcp-acl, for a given ACE entry, it should be able to define a match filter that includes both the source IP add…

So if tcp-acl can be combined with other acls, why do I need in other
places of the data model combinations like l2-l3-ipv4-ipv6-acl? Why do
I not have tcp-acl, eth-acl, ipv4-acl, and ipv6-acl? (Note that l2-acl
is somewhat a misnomer - hence I used eth-acl instead.)

/js

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 2 Nov 2017 15:20:34 +0630

Kristian,

I hear you. What I am providing is the rational for the current design. 

I would like to hear from others in the WG. We have been reviewing this draft for the last couple of years, and we are now at the tail end of the LC. I would really like to see this draft move forward, particularly since it is not broken.

Thanks.

> On Nov 2, 2017, at 2:13 PM, Kristian Larsson <[email-masked]> wrote:
> 

Mahesh Jethanandani
[email-masked]

---

From: Dean Bogdanovic <[email-masked]>
Date: Thu, 2 Nov 2017 06:36:03 -0400

I agree with points raised by Juergen and Kristian. Because of design changes I have stepped down as co-author of the draft.

> On Nov 2, 2017, at 4:50 AM, Mahesh Jethanandani <[email-masked]> wrote:
> 
> Kristian,
>

---

From: Robert Wilton <[email-masked]>
Date: Thu, 2 Nov 2017 11:24:40 +0000

Hi Mahesh,

I also think that the model would be cleaner if you don't have separate 
containers for each "type of ACL".  In particular, I think that the 
model is easier for clients, and perhaps easier to implement, if a given 
ACE field is always on the same path rather that being on a different 
path based on ACL type.  I've suggested a minor change to the model that 
may retain the same benefits, but keep consistent paths for the ACE fields.

Hence, would the following help improve the model?

1) Move the L2, IPv4, IPv6 match fields under their own containers, as 
Juergen/Kristian have suggested.
2) Under each of those containers remove the "if-feature", and change 
the "must" statement to a "when statement".
3) Add if-feature statements under the acl-type identity definitions, 
allowing a device to control what acl-types mixes they support.

E.g.

   // ACL features defined as below.

   // ACL type identities updated to:
   identity ipv4-acl {
     base acl:acl-base;
     if-feature "ipv4-acl";
   }
   identity ipv6-acl {
     base acl:acl-base;
     if-feature "ipv6-acl";
  }
   identity eth-acl {
     base acl:acl-base;
     if-feature "l2-acl";
  }
   identity mixed-l2-l3-ipv4-acl {
     base "acl:ipv4-acl";
     base "acl:eth-acl";
     if-feature "mixed-ipv4-acl";
  }
   identity mixed-l2-l3-ipv6-acl {
     base "acl:ipv6-acl";
     base "acl:eth-acl";
     if-feature "mixed-ipv6-acl";
  }

...

...
       leaf acl-type {
         type acl-type;
       }
       container aces {
         list ace {
           key "rule-name";
           ordered-by user;
           leaf rule-name ...

           container matches {
             container l2 {
               when "derived-from(../../../../acl-type, 'acl:eth-acl')";
               uses packet-fields:acl-eth-header-fields;
               description
                 "Rule set for L2 ACL.";
             }
             container ipv4 {
               when "derived-from(../../../../acl-type, acl:ipv4-acl')";
               uses packet-fields:acl-ip-header-fields;
                     uses packet-fields:acl-ipv4-header-fields;
               description
                 "Rule set that supports IPv4 headers.";
             }
             container ipv6 {
               ...
             }

Thanks,
Rob

On 02/11/2017 08:50, Mahesh Jethanandani wrote:
> .

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 02 Nov 2017 13:26:34 +0100 (CET)

Robert Wilton <[email-masked]> wrote:
> Hi Mahesh,
> 
> I also think that the model would be cleaner if you don't have
> separate containers for each "type of ACL".  In particular, I think
> that the model is easier for clients, and perhaps easier to implement,
> if a given ACE field is always on the same path rather that being on a
> different path based on ACL type.

+1

> I've suggested a minor change to
> the model that may retain the same benefits, but keep consistent paths
> for the ACE fields.
> 

There seem to be some inconsistencies in these names.  For example,
why is the identity called "eth-acl" but the feature "l2-acl"?  And
when the identity for eth is used with "mixed-" it is called "l2".

Also, why is "mixed-l2-l3-ipv4-acl" not called "mixed-l2-ipv4-acl"?
And why is the corresponding feature just called "mixed-ipv4-acl"?

/martin

> > I would like to hear from others in the WG. We have been reviewing
> > this draft for the last couple of years, and we are now at the tail
> > end of the LC. I would really like to see this draft move forward,
> > particularly since it is not broken.
> >
> > Thanks.
> >
> >>> Take the case where the desired selection is l2,-l3, ipv4 and
> >>> ipv6. The current
> >>> tree looks like this:
> >>>
> >>>
> >>>         |        |  |  +--rw destination-mac-address-mask?
> >>>         |        |  |  yang:mac-address
> >>>         |        |  |  +--rw destination-mac-address-mask?
> >>>         |        |  |  yang:mac-address

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 2 Nov 2017 19:10:30 +0630

Ok. Will update the model to reflect the discussion on this thread.

Mahesh Jethanandani
[email-masked]

> On Nov 2, 2017, at 6:56 PM, Martin Bjorklund <[email-masked]> wrote:
>

---

From: Robert Wilton <[email-masked]>
Date: Thu, 2 Nov 2017 12:53:29 +0000

> And why is the corresponding feature just called "mixed-ipv4-acl"?
I agree that aligning the feature names and identity names would be 
helpful.   In my example of how the model could be tweaked (and below) I 
was just reusing the existing ones that are currently defined in the draft.

One further refinement might also be to make the ACL type features a bit 
more hierarchical as well, but I don't know if that makes it too complex?

For example, the model could define separate features for what type of 
ACE matching is supported by the device, separately from what types of 
ACE combinations are allowed.

E.g.

// New 'match type' features.

feature match-on-l2-eth-hdr {
    // Device can match on L2 Ethernet header fields.
}
feature match-on-ipv4-hdr {
    // Device can match on IPv4 header fields.
}
feature match-on-ipv6-hdr {
    // Device can match on IPv6 header fields.
}

The existing ACL type features could then depend on these:

   feature l2-acl {
     if-feature "match-on-l2-eth-hdr";
     description "Layer 2 ACL supported";
   }

   feature ipv4-acl {
     if-feature "match-on-ipv4-hdr";
     description "Layer 3 IPv4 ACL supported";
   }

   feature ipv6-acl {
    if-feature "match-on-ipv6-hdr";
    description "Layer 3 IPv6 ACL supported";
   }

   feature mixed-ipv4-acl {
     if-feature "match-on-l2-eth-hdr and "match-on-ipv4-hdr";
     description "Layer 2 and Layer 3 IPv4 ACL supported";
   }
   ...

This then means that the ACE match fields can be predicated on a "match type" if-feature statement:

   ...
           container matches {
             container l2 {
*if-feature "**match-on-l2-eth-hdr**";*
               when "derived-from(../../../../acl-type, 'acl:eth-acl')";
               uses packet-fields:acl-eth-header-fields;
               description
                 "Rule set for L2 ACL.";
             }
             container ipv4 {
*if-feature "**match-on-ipv4-hdr**";*
               when "derived-from(../../../../acl-type, acl:ipv4-acl')";
               uses packet-fields:acl-ip-header-fields;
                     uses packet-fields:acl-ipv4-header-fields;
               description
                 "Rule set that supports IPv4 headers.";
             }
             container ipv6 {
*if-feature "**match-on-ipv6-hdr**";*
               ...
             }

This would keep the actual model a bit smaller for devices that don't 
support all match types (regardless of what combinations they support).

Thanks,
Rob

---

From: Kristian Larsson <[email-masked]>
Date: Thu, 2 Nov 2017 17:41:49 +0100

On Thu, Nov 02, 2017 at 12:53:29PM +0000, Robert Wilton wrote:
> One further refinement might also be to make the ACL type features a bit more
> hierarchical as well, but I don't know if that makes it too complex?

I was pondering this for a bit but I'm not sure it actually
helps.

> For example, the model could define separate features for what type of ACE
> matching is supported by the device, separately from what types of ACE
> combinations are allowed.
> 
> E.g.
> 

Features dependent on features... inception. I didn't even know
this was possible with YANG. Learned something today \o/

Anyway, I don't think you can actually deduce that a device
supports an ACL that maches on ethernet and IPv4 based on that it
supports matching on Ethernet headers and IPv4 headers.

For example, on IOS XR there are "ethernet-service access-list"
which can match on Ethernet headers but they are distinct from
ipv4 access-list and they use different attachment points under
an interface. IPv6 is yet another ACL with its own attachment
point... you cannot mix them in the same ACL. I guess they are
logically evaluated in order, so ethernet first, and if it passes
that then the ipvX ACL is evaluated. I believe the situation is
similar on JUNOS.

Which brings us to how to define attachment points. By using YANG
features a device can declare what ACL types it supports but if
the device has different attachment points then there should
probably be some constraint on what ACL type is attached where.

Are we seeking to have a single style of attachment points? I
think that's difficult in reality. Linux has one style, where a
single global "ACL" is defined. Most routers use per interface
ACL and as seen, they split it up on ethernet vs IP (and v4 vs
v6). I doubt one can be said to be better than the other so
trying to argue that everyone should converge on one way is
pointless. Similarly supporting every different style is also
futile as it's completely against the point of standardisation.

The pragmatic compromies is likely to support a few ways and any
vendor that needs something radically different need to build
their own model, do augment, deviate, refine or whatever. Other
thoughts?

An example (from the top of my head so excuse syntax errors):

grouping interface-attach {
  choice attach-style {
    case mixed {
      if-feature mixed-acl;
      leaf-list mixed {
        description "Any type of ACL that can match on ethernet, ipv4, ipv6 or anything else";
        ordered-by user;
        type leafref {
          path "/access-list/acl/acl-name"; // we can apply any acl-type
        }
      }
    }

    case specific-acl {
      if-feature specific-acl;

      leaf-list ethernet {
        description "ACL for Ethernet";
        ordered-by user;
        type leafref {
          path "/access-list/acl/acl-name";
        }
        must 'derived-from(deref(.)/../acl-type, eth-acl)';
      }

      leaf-list ipv4 {
        description "ACL for IPv4";
        ordered-by user;
        type leafref {
          path "/access-list/acl/acl-name";
        }
        must 'derived-from(deref(.)/../acl-type, ipv4-acl)';
      }

      leaf-list ipv6 {
        description "ACL for IPv6";
        ordered-by user;
        type leafref {
          path "/access-list/acl/acl-name";
        }
        must 'derived-from(deref(.)/../acl-type, ipv6-acl)';
      }
    }
  }
}

// ACLs attached under interface, like most big routers do it
augment "/if:interfaces/if:interface" {
  if-feature interface-acl;
  container acl {
    description
      "ACL attachment point";

    container ingress {
      uses interface-attach;
    }
    container egress {
      uses interface-attach;
    }
  }
}

// ACL globally attached, like on a Linux machine
augment /access-list {
  if-feature global-attach;
  leaf system-acl {
    type leafref {
      path "/access-list/acl/acl-name";
    }
  }
}

Kind regards,
   Kristian.

---

From: Robert Wilton <[email-masked]>
Date: Thu, 2 Nov 2017 17:38:02 +0000

> this was possible with YANG. Learned something today \o/
This just means that a device cannot say that it supports a 
"mixed-ipv4-acl" ACL unless it supports classifying traffic on Ethernet 
and classifying traffic on IPv4.

So the 'match type' feature specify what header fields the hardware is 
capable of match on.  E.g. a basic L2 device might say that is only 
matches on the Ethernet header.

The "ACL types" features specify what combination of header fields can 
be combined into a single ACL.

The real benefit for defining "match type" is that the unsupported 
fields in the ACL can then be cleanly left out of the device schema.  
E.g. a hypothetical new breed of IPv6 only routers that only support 
matching on match-on-ipv6-hdr, and don't want to carry v4 baggage.

> supports matching on Ethernet headers and IPv4 headers.
The YANG is stating the reverse:
  - You can enable the feature that matches on mixed ACLs, if and only 
if, you support matching on Ethernet header fields and IPv4 header fields.

> that then the ipvX ACL is evaluated. I believe the situation is
So for XR, you might just list the following features:

feature match-on-l2-eth-hdr;
feature match-on-ipv4-hdr;
feature match-on-ipv6-hdr;
feature l2-acl;
feature ipv4-acl;
feature ipv6-acl;

I.e. it individually supports L2, v4, and v6 ACLs, but a given ACL cannot have mixed entries.

Probably the vendors would need to augment this module with addition configuration restrictions to which ACLs can be applied together on an interface.  E.g. either an L2 or L3 ACL.  If an L3 ACL it can either be a v4 ACL, a v6 ACL, or one of each.

> probably be some constraint on what ACL type is attached where.
I think that this is probably hard to encode in a generic model.  As per 
above, I suspect that such constraints may be more cleanly implemented 
as vendor specific deviations.

> their own model, do augment, deviate, refine or whatever. Other
For interface attachments I think that the approach in the draft looks 
OK, and reasonably generic, but will need vendor deviations. This is 
probably OK.

Personally, I would put the ACL interface attachment points as an 
augmentation of if:interfaces/interface rather than having a separate 
top level list, but perhaps that is just want I am used to ...

Thanks,
Rob

>    if-feature interface-acl;
I think that the existing model is g

---

From: Kristian Larsson <[email-masked]>
Date: Fri, 3 Nov 2017 09:42:31 +0100

On Thu, Nov 02, 2017 at 05:38:02PM +0000, Robert Wilton wrote:
> This just means that a device cannot say that it supports a "mixed-ipv4-acl"
> ACL unless it supports classifying traffic on Ethernet and classifying
> traffic on IPv4.
> 
> capable of match on.  E.g. a basic L2 device might say that is only matches
> on the Ethernet header.
> 
> The "ACL types" features specify what combination of header fields can be
> combined into a single ACL.

Ah, right. Sorry, I was thinking that match-on-l2-eth-hdr and
match-on-ipv4-hdr would imply mixed-ipv4-acl. My bad.

> The real benefit for defining "match type" is that the unsupported fields in
> the ACL can then be cleanly left out of the device schema.  E.g. a
> hypothetical new breed of IPv6 only routers that only support matching on
> match-on-ipv6-hdr, and don't want to carry v4 baggage.

Since I work for a network that aspires of being v6 only I quite
like this ;)

> For interface attachments I think that the approach in the draft looks OK,
> and reasonably generic, but will need vendor deviations. This is probably
> OK.

I don't agree. Given the length we've gone in trying to convey the
match capabilities of the platform I think we can afford to give
implementors some options when it comes to attachment too! :)

Arguing that one way of doing ACL attachment is better than
another is futile and my personal opinion is that there simply is
no one way that's better than all else. A single global
attachment point like what Linux does is not better or worse from
the per interface ACL style that most router vendors employ. It's
just different. I believe the model should allow vendors and
users to express the most common forms we currently have. Not all
ways, but the two or three most common ways, which probably
covers the vast majority of all platforms.

The current model is an ill fit for platforms that attach filters
globally, like most host firewalls (Linux *tables, OpenBSD pf,
FreeBSD ipfw etc) or the vast majority of firewalls (Juniper SRX,
Cisco ASA, Checkpoint etc). There must be different attachment
options for "per interface" vs "global". 

As for the per-interface style options
 * current draft; a list of ACLs of varying "type", evaluated in
   specified order, per-interface and per direction
 * three separate ACLs for ethernet, ipv4 and ipv6 and
   per-interface and per direction

The only vendor I know of that have a single attachment point on
an interface is Huawei. However, it's not just an attachment
point for ACLs but for what Huawei calls a "traffic-policy" which
mixes in ACLs and QoS in the same place. It is so different from
everything else that deviations or augmentations will no doubt be
required to express what they have. The other large router
vendors; Cisco, Juniper and Nokia, all use separate attachment
points for ipv4, ipv6 and ethernet.

Shouldn't we have a model that supports;
 * virtually all host network stacks
 * virtually all firewalls
 * the vast majority of high end routers

Rather than:
 * no currently existing platform (or is there one I don't know of?)

> augmentation of if:interfaces/interface rather than having a separate top
> level list, but perhaps that is just want I am used to ...

+1 on augmentation of interfaces.

   kll

---

From: Kristian Larsson <[email-masked]>
Date: Fri, 3 Nov 2017 09:51:06 +0100

> I hear you. What I am providing is the rational for the current design. 

Ok, thank you! That is valuable to me so please don't stop :)

> I would like to hear from others in the WG. We have been
> reviewing this draft for the last couple of years, and we are
> now at the tail end of the LC.

Believe me, I have no intention of stopping this draft. I just
want to improve it.

I actually wanted to start using it earlier this year but found
the structure so unwieldy to work with that I eventually gave up
and instead decided to try and improve the model. It took a wee
bit longer than I intended but here I am.

For the interested, I wanted to build a YANG translation service
in NCS (now Cisco NSO) that could translate ACLs from one format
into the native format of four different vendors. I currently
keep feature parity across four different platforms and doing
that for something like ACLs is highly error prone.

> I would really like to see this draft move forward,

I want to have a standard ACL model too.

I am not complaining just for the sake of complaining, it is
because I found the structure unnatural or otherwise difficult to
use. I will try my best to not just criticise but instead provide
actual suggestions on how to improve things.

Kind regards,
   Kristian.

---

From: Kristian Larsson <[email-masked]>
Date: Fri, 3 Nov 2017 09:52:45 +0100

> Ok. Will update the model to reflect the discussion on this thread.

Mahesh, would it be helpful if I prepared changes in the form of
pull requests on the github repo?

I can write code, we can discuss it here and merge once agreed?

   kll

---

From: Kristian Larsson <[email-masked]>
Date: Fri, 3 Nov 2017 10:03:39 +0100

Another question somewhat related to attachment point. Why is
acl-type part of the list key?

I think compound keys are really quite clunky and should be
avoided if possible. In this case I really don't see why acl-type
needs to be part of the list key.

For the list of ACLs it means that the acl-name needs to be
unique instead of the combination of the type and name. This
seems rather uncontroversial to me.

Is it because we want to have constraints on the acl-type? ISTM
that we can apply such constraints anyway.

I just don't understand why it's part of the list key. Can it
please be removed?

   kll

---

From: Robert Wilton <[email-masked]>
Date: Fri, 3 Nov 2017 09:30:01 +0000

> implementors some options when it comes to attachment too! :)
I  agree that interface vs global are two different styles, and happy if 
the model supports both.

But for interface attachments, if there can be one generic way that can 
apply to all platforms (perhaps with further constraints) then that will 
be easier for clients to code to.

If different vendors do interface attachment in different ways then that 
ends up being more code on the client side.

> covers the vast majority of all platforms.
OK, would be happy for 2 ways:
  - interfaces
  - global

With each of them generic enough to apply reasonably widely.

> options for "per interface" vs "global".
Sure.

>     per-interface and per direction
I think that the current model accommodates both of these.

For the 3 separate ACLs, the vendor would "deviate" the model with 
additional constraints so that there could only be one ACL of each type 
in the list for an interface.

Given that the model allows mixed ACLs, this approach still seems 
reasonable and quite generic to me.

>   * the vast majority of high end routers
Yes, if we can.

Thanks,
Rob

---

From: Kristian Larsson <[email-masked]>
Date: Fri, 3 Nov 2017 11:12:25 +0100

On Fri, Nov 03, 2017 at 09:30:01AM +0000, Robert Wilton wrote:
> I  agree that interface vs global are two different styles, and happy if the
> model supports both.

Cool.

> additional constraints so that there could only be one ACL of each type in
> the list for an interface.

Fair enough. I actually discussed this solution off-list
yesterday evening but failed to bring it up as a potential
solution now ;)

> Given that the model allows mixed ACLs, this approach still seems reasonable
> and quite generic to me.

Sure. I guess the XPath on IOS XR, which supports one eth acl,
one ipv4 acl and one ipv6 acl per interface becomes a little
tricky. Like we have to do count() based on acl-type I guess to
prevent two ACLs of the same type being set (or say that the
translation mechanism should logically merge multiple ACLs).

JUNOS also has different attachment points that each accept a
list of ACLs so it's just a matter of putting a constraint on
accepted acl-type and the translation mechanism just sorts based
on acl-type.

I'm happy with this :)

   kll

-- 
Kristian Larsson                                        KLL-RIPE
+46 72 5479985                                [email-masked]

---

From: Robert Wilton <[email-masked]>
Date: Fri, 3 Nov 2017 10:40:16 +0000

> translation mechanism should logically merge multiple ACLs).
Yes.  I thought XR either allowed a v4 and  a v6 ACLs, or no IP ACL and 
just an Ethernet ACL.

If so the constraint should be something very roughly like:
- deviate add max-elements 2
- deviate add unique "type"
- deviate add must (count(set-name) != 2, or acl_type != eth).

Or, if one of each type is allowed, then this would just be:
- deviate add max-elements 3
- deviate add unique "type"

Cool.

Rob

---

From: Mahesh Jethanandani <[email-masked]>
Date: Fri, 3 Nov 2017 18:14:11 +0530

Please do, and we can discuss the changes on the mailing list. 

Thanks. 

Mahesh Jethanandani 
[email-masked]

> On Nov 3, 2017, at 2:22 PM, Kristian Larsson <[email-masked]> wrote:
>

---

From: Sonal Agarwal <[email-masked]>
Date: Sat, 4 Nov 2017 10:38:45 -0700

Kristian,

In response to one of your previous comments:

*"I'm really bothered by the compound key consisting of acl-type*

*and the acl-name since attachment points then need to referenceboth.  It's
also weird because I don't think choosing theacl-type is really a choice of
the user but more of a limitationof the platform.One approach would be to
change the key to only be the acl-namebut let the acl-type leaf remain,
perhaps make it mandatory ordefault to some unified acl-type. I think it's
still possible toimplement a constraint on this, right? Like if a platform
onlysupports a specific type at some attachment point it can add
aconstraint on the acl-type by doing deref() on the leafref."*

The key for an ACL needs to remain as the name and type. They both uniquely
define the presence of the ACL in config.

Sonal.

On Fri, Nov 3, 2017 at 5:44 AM, Mahesh Jethanandani <[email-masked]
> wrote:

---

From: Sonal Agarwal <[email-masked]>
Date: Sat, 4 Nov 2017 11:01:12 -0700

On Fri, Nov 3, 2017 at 3:40 AM, Robert Wilton <[email-masked]> wrote:

>>>>> OK,
>>> the

> IOS-XR allows IPV4 and IPV6 ACL's on L3 interfaces. It allows Ethernet
> ACL's on L2 interfaces

   Sonal.

---

From: Kristian Larsson <[email-masked]>
Date: Wed, 8 Nov 2017 08:47:21 +0100

On Sat, Nov 04, 2017 at 10:38:45AM -0700, Sonal Agarwal wrote:
> "I'm really bothered by the compound key consisting of acl-type
> and the acl-name since attachment points then need to reference
> both.  It's also weird because I don't think choosing the
> acl-type is really a choice of the user but more of a limitation
> of the platform.
> 
> One approach would be to change the key to only be the acl-name
> but let the acl-type leaf remain, perhaps make it mandatory or
> default to some unified acl-type. I think it's still possible to
> implement a constraint on this, right? Like if a platform only
> supports a specific type at some attachment point it can add a
> constraint on the acl-type by doing deref() on the leafref."
> 
> The key for an ACL needs to remain as the name and type. They
> both uniquely define the presence of the ACL in config. 

You can't argue for a design choice by saying "this is how it
is". If we change the key to be acl-name only then it is the name
that "uniquely define the presence of the ACL in config".

What do you perceive as the benefit of having acl-type in the
key? Why can't it be an attribute? We can still check, from the
attachment point, what the acl-type is.

   kll