[ovs-discuss] [ACL 2/3] vswitchd: Implement local ACL functionality.

[Jesse Gross]

I'll respond to the code-specific things later but for now some comments 
on the high level issues:
>         * ACLs are specific to a bridge, but they are tracked in
>           a global data structure.  I think that this code is
>           correct, but I was expecting to see something like a
>           "struct acl_engine *" member in either struct bridge or
>           struct ofproto, where the acl_engine would encapsulate
>           everything related to the ACL for that bridge and look
>           at configuration for ACLs for that bridge only.
>   

ACL's aren't specific to a bridge.  The actual ACL's (before they are 
applied to specific ports) are specified globally in the config file and 
are stored globally.  I don't think there is any particular reason for 
ACL definitions to be specific to a particular bridge and there could be 
common ACL's that someone wants to share.
>         * I expected to see a function that composes the entire
>           flow table for a bridge, but instead there is a
>           function that scrubs and replaces the flow table for a
>           single port.  Again, I don't see correctness problems,
>           but I think that the code would be simpler and more
>           obviously correct if the entire classifier were to be
>           cleared and restocked in one go.
>
>           (If you did it this way then you wouldn't need the
>           ofproto_delete_flows_wildcarded() function.)
>   

I'm not sure that this is really any clearer.  Fundamentally, composing 
the flow table involves iterating over the ports.  Maybe it's a little 
nicer to clear the flow table once
beforehand rather than for each port but that's about the only benefit 
that I can see.  Since the ACL code has no knowledge of the bridge 
ports, it can't compose the whole thing in one go.  Sticking it into a 
single function would involve more mixing of bridge and ACL code.
>         * There is one "struct classifier" per ACL port, whereas
>           I was expecting to see no additional classifiers at
>           all, following the model used by NOX, which uses
>           Nicira's OpenFlow extension action NXAST_RESUBMIT to
>           implement both ingress and egress actions within the
>           single OpenFlow classifier table.
>   

Using a single table doesn't work if you want to filter on switch 
ports.  The reason why the existing resubmit action works is NOX filters 
using MAC addresses, not ports.  For out rules, it creates flows that 
are matched based on destination MAC address.  Since there is no 
destination port to match, there would be overlap in rules for different 
ports.  Having different classifier tables essentially adds destination 
port as a classifier field.
>         * It looks like ACLs are actually over bridge interfaces,
>           not bridge ports.  An ordinary port has exactly one
>           interface, of the same name, so there is no difference
>           in that case, but bonded ports have multiple
>           interfaces.  It doesn't (as far as I can tell) make
>           sense to apply an ACL to only one interface of a bonded
>           port, so I would expect that ACLs should specify port
>           names and that an ACL over a bonded port would apply to
>           all of its interfaces in the same way.
>
>   

ACL's are specified in the config file on ports because, as you 
mentioned, it doesn't make sense to filter only part of a bonded port.  
The implementation is that filtering happens at the interface level.  
OpenFlow rules, statistics, and other traffic management happens on 
interfaces, not ports, and as a result that is where ACL's need to live 
as well.