[ovs-dev] [PATCH v4 0/3] Associate identifier with OVN ACL connection tracking entry

[Numan Siddique]

On Sat, Nov 9, 2019 at 8:20 AM Ankur Sharma <ankur.sharma at nutanix.com> wrote:
>
> I submitted this patch long time back and somehow lost track it.
> Resubmitting the series, calling it as V4, as it addresses the
> review comments given till v3.
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-April/358280.html
>
> What:
> ====
> a. Goal is to be able to associate some identifier with a connection tracking
> entry.
>
> b. This identifier can be used to map OVN ACL which added this entry or
> higher level constructs like openstack security group etc.
>
> c. There are 2 connection tracking fields which can be used for it.
> ct.mark (32 bits) and ct.label (128 bits).
>
> d. Patch intends to use ct.label, as this is a longer field and
> hence would be put to a better use, if it stores the identifier.
>
> Why:
> ====
> a. Adding an identifier would help in debugging.
> b. Now, we can map a connection tracking entry to corresponding
>    acl, security group etc.
> c. One of the use cases for this mapping would be to identify
>    ACLs which added corresponding connection tracker entry, which
>    is causing unexpected drops/leaks.
>
> How:
> ====
> Following is the sequence of changes:
>
> Patch 1:
>    i.  Current implementation uses a bit ct.label to handle policy update cases,
>        where we use a bit in ct.label to indicate that reply traffic should
>        be dropped now.
>   ii.  Swap the usage of ct.label in current implementation with ct.mark.
>
> Patch 2:
>    i. Add support in parser to allow ct.label and mark to be set from registers
>       as well (as of now only integer/masked integer is allowed).
>
> Patch 3:
>    i. Add a new column (named 'label') to Table ACL in northbound schema.
>   ii. ovn-northd changes to enhance logical flows to set ct.label to acl->label.
>       For example:
>       table=4 (ls_out_acl         ), .... action=(reg0[1] = 1; reg0[3] = 1; xxreg1 = 0x1234; next;)
>       .
>       .
>       .
>       table=7 (ls_out_stateful    ), ... match=(reg0[1] == 1 && reg0[3] == 1),
>
>

Hi Ankur,

Overall the series looks good to me. Can you please rebase and submit v5.

In patch 2,  using register offsets is not supported. Something like below
"ct_commit(ct_label=xreg1[10..30]);"

I would suggest to support that case as well.

Thanks
Numan

> Ankur Sharma (3):
>   OVN ACL: Replace the usage of ct_label with ct_mark
>   OVN ACL: Allow ct_mark and ct_label values to be set from register as
>     well
>   OVN ACL: Allow a user to input ct.label value for an acl
>
>  Documentation/tutorials/ovn-openstack.rst | 14 ++---
>  include/ovn/actions.h                     |  3 +
>  lib/actions.c                             | 72 ++++++++++++++++++----
>  lib/logical-fields.c                      |  3 +
>  northd/ovn-northd.8.xml                   | 14 ++---
>  northd/ovn-northd.c                       | 87 +++++++++++++++++----------
>  ovn-nb.ovsschema                          |  5 +-
>  ovn-nb.xml                                | 12 ++++
>  ovn-sb.xml                                | 41 +++++++++----
>  tests/ovn-nbctl.at                        | 12 +++-
>  tests/ovn.at                              | 99 +++++++++++++++++++++++++++++--
>  utilities/ovn-nbctl.c                     | 24 +++++++-
>  12 files changed, 310 insertions(+), 76 deletions(-)
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev