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

[Mahesh Jethanandani <[e-mail]>]

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

Mahesh Jethanandani
[e-mail]

> On Nov 2, 2017, at 6:56 PM, Martin Bjorklund <[e-mail]> wrote:
> 
> Robert Wilton <[e-mail]> 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.
>> 
>> 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";
>>  }
> 
> 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
> 
> 
> 
> 
> 
>>  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:
>>> 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 <[e-mail]>
>>>> wrote:
>>>> 
>>>>> On Thu, Nov 02, 2017 at 06:13:04AM +0630, Mahesh Jethanandani wrote:
>>>>>    On Nov 1, 2017, at 5:52 PM, Juergen Schoenwaelder <
>>>>>    [e-mail]> wrote:
>>>>> 
>>>>>    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.
>>>>> 
>>>>> 
>>>>> 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).
>>>> 
>>>> 
>>>>> 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.
>>>> 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.
>>> Mahesh Jethanandani
>>> [e-mail]
>>> _______________________________________________
>>> netmod mailing list
>>> [e-mail]
>>> https://www.ietf.org/mailman/listinfo/netmod
>>> .
>>