> On 18 Mar 2023, at 01:04, Numan Siddique <[email protected]> wrote: > > On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana > <[email protected]> wrote: >> >> This patch adds support to commit connections dropped/rejected by >> ACLs to the connection tracking table. Dropped connections are >> committed to conntrack only if NB_Global options:ct_commit_acl_drop >> is set to true (false by default) and ACL dropping/rejecting the >> connection has label configured. The dropped connections are >> committed in a separate conntrack zone so that they can be managed >> independently and do not interact with the connection tracking state >> of allowed connections. >> >> This provides a new approach to identify connections dropped by ACLs >> besides the existing ACL logging and drop sampling approaches. >> >> Each logical switch is assigned a new conntrack zone for committing >> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >> A new lflow action "ct_commit_drop" is introduced that commits flows >> to connection tracking table in a zone identified by >> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >> and non-empty label translates to include "ct_commit_drop" in its >> actions instead of simply dropping/rejecting the packet. >> >> Signed-off-by: Abhiram Sangana <[email protected]> >
Hi Numan, Thanks for reviewing the patch. > Hi Abhiram, > > Sorry for the delays in the reviews. > > Overall the patch looks good to me. > > I do have a few comments and suggestions. > > Instead of adding a global option, how about an option per ACL (in the > ACL options column) ? > Users can enable or disable conntrack commit per ACL. Sure, will do that. > > Are you going to use this feature all the time in your deployment ? > Or enable it only when debugging an issue ? > If you want to enable it during debugging, is it a bit of a hassle to > enable the ct_commit option for the interested ACL ? Yes, we are planning to use this feature all the time in our deployment. So, it shouldn't be an issue to enable ct_commit option for interested ACLs. > > I'm a little bit concerned with allocating a zone id for each logical > switch even if there are no ACLs configured with this option. > But I think it's not a big deal. Yes, I did try to look into allocating CT zone on an as-needed basis but had some difficulty implementing it. Will try to explore further in a subsequent patch. Thanks, Abhiram > > Thanks > Numan _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
