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:
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 protected]> wrote:

On Thu, Nov 02, 2017 at 06:13:04AM +0630, Mahesh Jethanandani wrote:
    On Nov 1, 2017, at 5:52 PM, Juergen Schoenwaelder <
    [email protected]> 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
[email protected]
_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod
.


_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod

Reply via email to