[ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless

Han Zhou hzhou at ovn.org
Wed Jun 2 04:21:44 UTC 2021


On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka <ihrachys at redhat.com> wrote:
>
> On Thu, May 20, 2021 at 9:55 PM Han Zhou <hzhou at ovn.org> wrote:
> >
> >
> >
> > On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou at ovn.org> wrote:
> > >
> > >
> > >
> > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys at redhat.com>
wrote:
> > > >
> > > > While we *should not* circumvent conntrack when a stateful ACL of
higher
> > > > priority is present on the switch, we should do so only when
> > > > allow-stateless and allow-stateful directions are the same,
otherwise we
> > > > should still skip conntrack for allow-stateless ACLs.
> > > >
> > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL
verb")
> > >
> > > Is this patch a "fix" or an "optimization"?
> > >
> > > >
> > > > Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> > > > ---
> > > >  northd/lswitch.dl    | 88
++++++++++++++++++++++++++++---------------
> > > >  northd/ovn-northd.c  | 89
++++++++++++++++++++++++++++++++++----------
> > > >  northd/ovn_northd.dl | 32 ++++++++--------
> > > >  tests/ovn-northd.at  | 72 +++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 213 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > > > index b73cfd047..8a4c9154c 100644
> > > > --- a/northd/lswitch.dl
> > > > +++ b/northd/lswitch.dl
> > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > > >      LogicalSwitchACL(ls, acl),
> > > >      nb::ACL(._uuid = acl, .action = "allow-stateless").
> > > >
> > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > > > +
> > > > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > > > +    LogicalSwitchStatelessACL(ls, acl),
> > > > +    nb::ACL(._uuid = acl, .direction = "from-lport").
> > > > +
> > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > > +// is an output relation:
> > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
has_stateless_from_acl: bool)
> > > > +
> > > > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > > > +    LogicalSwitchStatelessFromACL(ls, _).
> > > > +
> > > > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > > > +    nb::Logical_Switch(._uuid = ls),
> > > > +    not LogicalSwitchStatelessFromACL(ls, _).
> > > > +
> > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > > > +
> > > > +LogicalSwitchStatelessToACL(ls, acl) :-
> > > > +    LogicalSwitchStatelessACL(ls, acl),
> > > > +    nb::ACL(._uuid = acl, .direction = "to-lport").
> > > > +
> > > >  // "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > >  // is an output relation:
> > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
has_stateless_to_acl: bool)
> > > >
> > > > -LogicalSwitchHasStatelessACL(ls, true) :-
> > > > -    LogicalSwitchStatelessACL(ls, _).
> > > > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > > > +    LogicalSwitchStatelessToACL(ls, _).
> > > >
> > > > -LogicalSwitchHasStatelessACL(ls, false) :-
> > > > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > > >      nb::Logical_Switch(._uuid = ls),
> > > > -    not LogicalSwitchStatelessACL(ls, _).
> > > > +    not LogicalSwitchStatelessToACL(ls, _).
> > > >
> > > >  // "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > >  // is an output relation:
> > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > > >  /* Switch relation collects all attributes of a logical switch */
> > > >
> > > >  relation &Switch(
> > > > -    ls:                nb::Logical_Switch,
> > > > -    has_stateful_acl:  bool,
> > > > -    has_stateless_acl: bool,
> > > > -    has_acls:          bool,
> > > > -    has_lb_vip:        bool,
> > > > -    has_dns_records:   bool,
> > > > -    has_unknown_ports: bool,
> > > > -    localnet_ports:    Vec<(uuid, string)>,  // UUID and name of
each localnet port.
> > > > -    subnet:            Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > > -    ipv6_prefix:       Option<in6_addr>,
> > > > -    mcast_cfg:         Ref<McastSwitchCfg>,
> > > > -    is_vlan_transparent: bool,
> > > > +    ls:                     nb::Logical_Switch,
> > > > +    has_stateful_acl:       bool,
> > > > +    has_stateless_from_acl: bool,
> > > > +    has_stateless_to_acl:   bool,
> > > > +    has_acls:               bool,
> > > > +    has_lb_vip:             bool,
> > > > +    has_dns_records:        bool,
> > > > +    has_unknown_ports:      bool,
> > > > +    localnet_ports:         Vec<(uuid, string)>,  // UUID and name
of each localnet port.
> > > > +    subnet:                 Option<(in_addr/*subnet*/,
in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > > +    ipv6_prefix:            Option<in6_addr>,
> > > > +    mcast_cfg:              Ref<McastSwitchCfg>,
> > > > +    is_vlan_transparent:    bool,
> > > >
> > > >      /* Does this switch have at least one port with type !=
"router"? */
> > > >      has_non_router_port: bool
> > > > @@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string):
Option<in6_addr> {
> > > >      }
> > > >  }
> > > >
> > > > -&Switch(.ls                = ls,
> > > > -        .has_stateful_acl  = has_stateful_acl,
> > > > -        .has_stateless_acl = has_stateless_acl,
> > > > -        .has_acls          = has_acls,
> > > > -        .has_lb_vip        = has_lb_vip,
> > > > -        .has_dns_records   = has_dns_records,
> > > > -        .has_unknown_ports = has_unknown_ports,
> > > > -        .localnet_ports    = localnet_ports,
> > > > -        .subnet            = subnet,
> > > > -        .ipv6_prefix       = ipv6_prefix,
> > > > -        .mcast_cfg         = mcast_cfg,
> > > > -        .has_non_router_port = has_non_router_port,
> > > > -        .is_vlan_transparent = is_vlan_transparent) :-
> > > > +&Switch(.ls                     = ls,
> > > > +        .has_stateful_acl       = has_stateful_acl,
> > > > +        .has_stateless_from_acl = has_stateless_from_acl,
> > > > +        .has_stateless_to_acl   = has_stateless_to_acl,
> > > > +        .has_acls               = has_acls,
> > > > +        .has_lb_vip             = has_lb_vip,
> > > > +        .has_dns_records        = has_dns_records,
> > > > +        .has_unknown_ports      = has_unknown_ports,
> > > > +        .localnet_ports         = localnet_ports,
> > > > +        .subnet                 = subnet,
> > > > +        .ipv6_prefix            = ipv6_prefix,
> > > > +        .mcast_cfg              = mcast_cfg,
> > > > +        .has_non_router_port    = has_non_router_port,
> > > > +        .is_vlan_transparent    = is_vlan_transparent) :-
> > > >      nb::Logical_Switch[ls],
> > > >      LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
> > > > -    LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl),
> > > > +    LogicalSwitchHasStatelessFromACL(ls._uuid,
has_stateless_from_acl),
> > > > +    LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_acl),
> > > >      LogicalSwitchHasACLs(ls._uuid, has_acls),
> > > >      LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
> > > >      LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index a410be1de..1e027eab2 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -627,6 +627,8 @@ struct ovn_datapath {
> > > >      uint32_t port_key_hint;
> > > >
> > > >      bool has_stateful_acl;
> > > > +    bool has_stateless_from;
> > > > +    bool has_stateless_to;
> > > >      bool has_lb_vip;
> > > >      bool has_unknown;
> > > >      bool has_acls;
> > > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap
*nb_pgs)
> > > >      hmap_destroy(nb_pgs);
> > > >  }
> > > >
> > > > +static bool
> > > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od,
> > > > +                         const struct nbrec_acl *acl)
> > > > +{
> > > > +    if (!strcmp(acl->action, "allow-related")) {
> > > > +        od->has_stateful_acl = true;
> > > > +        if (od->has_stateless_to && od->has_stateless_from) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +    if (!strcmp(acl->action, "allow-stateless")) {
> > > > +        if (!strcmp(acl->direction, "from-lport")) {
> > > > +            od->has_stateless_from = true;
> > > > +            if (od->has_stateful_acl && od->has_stateless_to) {
> > > > +                return true;
> > > > +            }
> > > > +        } else {
> > > > +            od->has_stateless_to = true;
> > > > +            if (od->has_stateful_acl && od->has_stateless_from) {
> > > > +                return true;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +    return false;
> > > > +}
> > > > +
> > > >  static void
> > > >  ls_get_acl_flags(struct ovn_datapath *od)
> > > >  {
> > > >      od->has_acls = false;
> > > >      od->has_stateful_acl = false;
> > > > +    od->has_stateless_from = false;
> > > > +    od->has_stateless_to = false;
> > > >
> > > >      if (od->nbs->n_acls) {
> > > >          od->has_acls = true;
> > > >
> > > >          for (size_t i = 0; i < od->nbs->n_acls; i++) {
> > > >              struct nbrec_acl *acl = od->nbs->acls[i];
> > > > -            if (!strcmp(acl->action, "allow-related")) {
> > > > -                od->has_stateful_acl = true;
> > > > +            if (ls_get_acl_flags_for_acl(od, acl)) {
> > > >                  return;
> > > >              }
> > > >          }
> > > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od)
> > > >
> > > >              for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) {
> > > >                  struct nbrec_acl *acl = ls_pg->nb_pg->acls[i];
> > > > -                if (!strcmp(acl->action, "allow-related")) {
> > > > -                    od->has_stateful_acl = true;
> > > > +                if (ls_get_acl_flags_for_acl(od, acl)) {
> > > >                      return;
> > > >                  }
> > > >              }
> > > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct
ovn_datapath *od, struct ovn_port *op,
> > > >  }
> > > >
> > > >  static bool
> > > > -apply_to_each_acl_of_action(struct ovn_datapath *od,
> > > > -                            const struct hmap *port_groups,
> > > > -                            struct hmap *lflows, const char
*action,
> > > > -                            void (*func)(struct ovn_datapath *,
> > > > -                                         const struct nbrec_acl *,
> > > > -                                         struct hmap *))
> > > > +apply_to_each_acl_of_action_and_direction(
> > > > +        struct ovn_datapath *od,
> > > > +        const struct hmap *port_groups,
> > > > +        struct hmap *lflows,
> > > > +        const char *action, const char *direction,
> > > > +        void (*func)(struct ovn_datapath *,
> > > > +                     const struct nbrec_acl *,
> > > > +                     struct hmap *))
> > > >  {
> > > >      bool applied = false;
> > > >      for (size_t i = 0; i < od->nbs->n_acls; i++) {
> > > >          const struct nbrec_acl *acl = od->nbs->acls[i];
> > > > -        if (!strcmp(acl->action, action)) {
> > > > +        if (!strcmp(acl->action, action) &&
> > > > +                (!direction || !strcmp(acl->direction,
direction))) {
> > > >              func(od, acl, lflows);
> > > >              applied = true;
> > > >          }
> > > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct
ovn_datapath *od,
> > > >          if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
> > > >              for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
> > > >                  const struct nbrec_acl *acl = pg->nb_pg->acls[i];
> > > > -                if (!strcmp(acl->action, action)) {
> > > > +                if (!strcmp(acl->action, action) &&
> > > > +                        (!direction || !strcmp(acl->direction,
direction))) {
> > > >                      func(od, acl, lflows);
> > > >                      applied = true;
> > > >                  }
> > > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath
*od,
> > > >                          const struct hmap *port_groups,
> > > >                          struct hmap *lflows)
> > > >  {
> > > > -    return apply_to_each_acl_of_action(
> > > > -        od, port_groups, lflows, "allow-stateless",
build_stateless_filter);
> > > > +    return apply_to_each_acl_of_action_and_direction(
> > > > +        od, port_groups, lflows, "allow-stateless", NULL,
> > > > +        build_stateless_filter);
> > > >  }
> > > >
> > > >  static void
> > > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct
ovn_datapath *od,
> > > >      ds_destroy(&match);
> > > >  }
> > > >
> > > > +static void
> > > > +build_stateful_override_filters_for_direction(struct ovn_datapath
*od,
> > > > +                                              const struct hmap
*port_groups,
> > > > +                                              struct hmap *lflows,
> > > > +                                              const char
*direction)
> > > > +{
> > > > +    apply_to_each_acl_of_action_and_direction(
> > > > +        od, port_groups, lflows, "allow-related", direction,
> > > > +        build_stateful_override_filter);
> > > > +    apply_to_each_acl_of_action_and_direction(
> > > > +        od, port_groups, lflows, "allow", direction,
> > > > +        build_stateful_override_filter);
> > > > +}
> > > > +
> > > >  static void
> > > >  build_stateful_override_filters(struct ovn_datapath *od,
> > > >                                  const struct hmap *port_groups,
> > > >                                  struct hmap *lflows)
> > > >  {
> > > > -    apply_to_each_acl_of_action(
> > > > -        od, port_groups, lflows, "allow-related",
> > > > -        build_stateful_override_filter);
> > > > -    apply_to_each_acl_of_action(
> > > > -        od, port_groups, lflows, "allow",
> > > > -        build_stateful_override_filter);
> > > > +    if (od->has_stateless_from) {
> > > > +        build_stateful_override_filters_for_direction(
> > > > +            od, port_groups, lflows, "from-lport");
> > > > +    }
> > > > +    if (od->has_stateless_to) {
> > > > +        build_stateful_override_filters_for_direction(
> > > > +            od, port_groups, lflows, "to-lport");
> > > > +    }
> > > >  }
> > > >
> > > >  static void
> > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > > > index fc703f62e..8cbc959f0 100644
> > > > --- a/northd/ovn_northd.dl
> > > > +++ b/northd/ovn_northd.dl
> > > > @@ -1845,23 +1845,21 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls},
.acl = &acl, .has_fair_meter = _)) {
> > > >  }
> > > >
> > > >  for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl,
.has_fair_meter = _)) {
> > > > -    if (sw.has_stateless_acl) {
> > > > -        if ((sw.has_stateful_acl and acl.action == "allow") or
acl.action == "allow-related") {
> > > > -            if (acl.direction == "from-lport") {
> > > > -                Flow(.logical_datapath = ls._uuid,
> > > > -                     .stage            = s_SWITCH_IN_PRE_ACL(),
> > > > -                     .priority         = acl.priority +
oVN_ACL_PRI_OFFSET(),
> > > > -                     .__match          = "ip && ${acl.__match}",
> > > > -                     .actions          =
"${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;",
> > > > -                     .external_ids     = stage_hint(acl._uuid))
> > > > -            } else {
> > > > -                Flow(.logical_datapath = ls._uuid,
> > > > -                     .stage            = s_SWITCH_OUT_PRE_ACL(),
> > > > -                     .priority         = acl.priority +
oVN_ACL_PRI_OFFSET(),
> > > > -                     .__match          = "ip && ${acl.__match}",
> > > > -                     .actions          =
"${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;",
> > > > -                     .external_ids     = stage_hint(acl._uuid))
> > > > -            }
> > > > +    if ((sw.has_stateful_acl and acl.action == "allow") or
acl.action == "allow-related") {
> > > > +        if (sw.has_stateless_from_acl and acl.direction ==
"from-lport") {
> > > > +            Flow(.logical_datapath = ls._uuid,
> > > > +                 .stage            = s_SWITCH_IN_PRE_ACL(),
> > > > +                 .priority         = acl.priority +
oVN_ACL_PRI_OFFSET(),
> > > > +                 .__match          = "ip && ${acl.__match}",
> > > > +                 .actions          = "${rEGBIT_CONNTRACK_DEFRAG()}
= 1; next;",
> > > > +                 .external_ids     = stage_hint(acl._uuid))
> > > > +        } else if (sw.has_stateless_to_acl) {
> > >
> > > Should this be: else if (sw.has_stateless_to_acl and acl.direction ==
"to-lport") ?
> > >
> > > > +            Flow(.logical_datapath = ls._uuid,
> > > > +                 .stage            = s_SWITCH_OUT_PRE_ACL(),
> > > > +                 .priority         = acl.priority +
oVN_ACL_PRI_OFFSET(),
> > > > +                 .__match          = "ip && ${acl.__match}",
> > > > +                 .actions          = "${rEGBIT_CONNTRACK_DEFRAG()}
= 1; next;",
> > > > +                 .external_ids     = stage_hint(acl._uuid))
> > > >          }
> > > >      }
> > > >  }
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index 6c38e1a97..1c55310b2 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -2664,6 +2664,78 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | sort], [0],
> > > >  AT_CLEANUP
> > > >  ])
> > > >
> > > > +OVN_FOR_EACH_NORTHD([
> > > > +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related of
different direction])
> > > > +ovn_start
> > > > +
> > > > +# Create two switches to validate direction. We can't use the same
switch for
> > > > +# both ports, otherwise both in and out pipelines are triggered.
> > > > +ovn-nbctl lr-add r0
> > > > +for i in $(seq 1 2); do
> > > > +    check ovn-nbctl --wait=sb ls-add sw$i
> > > > +
> > > > +    check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i
f0:00:00:00:00:0$i 192.168.$i.1/24
> > > > +    check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0
> > > > +    check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router
> > > > +    check ovn-nbctl --wait=sb lsp-set-options sw$i-r0
router-port=r0-sw$i
> > > > +    check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router
> > > > +
> > > > +    check ovn-nbctl --wait=sb lsp-add sw$i lsp$i
> > > > +    check ovn-nbctl --wait=sb lsp-set-addresses lsp$i
"fe:00:00:00:00:0$i 192.168.$i.100/24"
> > > > +done
> > > > +
> > > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless
> > > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related
> > > > +ovn-nbctl --wait=sb sync
> > > > +
> > > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst ==
f0:00:00:00:00:01'
> > > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst ==
192.168.2.100'
> > > > +flow_tcp='tcp && tcp.dst == 80'
> > > > +flow_udp='udp && udp.dst == 80'
> > > > +lsp1_inport=$(fetch_column Port_Binding tunnel_key
logical_port=lsp1)
> > > > +
> > > > +# TCP packets should not go to conntrack because allow-related
direction is different.
> > >
> > > This test case makes me wonder if the idea of this patch (as an
optimization) is not correct. What if the packet is a return packet of a
request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp
allow-related"? In that case the packet does need to go to CT, otherwise
the CT state will never be "established". I also had the impression that
the priorities for different directions are independent, but it seems we
will have to consider them together for the stateless + stateful scenario.
Any thoughts on this?
> > >
> > I thought more about it, and it seems the return traffic problem is
there for the mixed stateful and stateless scenarios regardless of this
patch. For the return traffic belonging to a flow allowed by a
"allow_related" ACL to work, we will have to send all packets that are
possibly related to a stateful ACL to conntrack, regardless of its priority
(because priority should be examined for the initiation direction only).
However, it seems really complex to define logical flows to identify if a
packet is "related" to a stateful ACL (if possible at all). I'd suggest
considering this as a limitation of stateless ACLs and document it clearly
so that when a user uses "allow-stateless" they understand the
consequences: if there is an allow-stateless rule defined on direction A,
any stateful rule of direction B that could have any return packets
matching this allow-stateless rule may not work. Probably the ideal
situation when "allow-stateless" is particularly useful is when the
stateful and stateless rules don't have any overlapping, e.g. one for TCP
and the other for UDP. Think about this further, if we make it clear that
allow-stateless should be used when there is no overlapping with stateful
rules, the original patch (that has been merged) works perfectly because
priority doesn't matter if there is no overlapping, and the code complexity
and number of flows are both reduced. This way, the limitation is also
easier to be documented and understood (comparing with clarifying
accurately the limitation of priority + direction + "possible return
traffic"). What do you think?
>
> I am for documenting the mechanics, but I think we should implement
> the API as defined in schema (incl. priority and direction). There is
> already some text in documentation that explains that allow-stateless
> requires more legwork and attention to define proper rules for
> returning traffic, we can expand that more to also explain that
> overlapping allow-stateless/allow-related rules may also interact.
>

Hi Ihar,

It would be great if we can define the behavior clearly with the proper
implementation. However, at least it is still not clear to me what these 3
patches could achieve on top of the original patch for real world use
cases. With your original patch for "allow-stateless", if there are
overlapping stateful and stateless rules, the stateless rules will override
regardless of priority. Now with these 3 patches, it is expected to honor
priority. But consider a typical example:

All traffic from A to B (egress) is stateless:
ACL1: from-lport 100 'inport == "A" && ip.dst == B' allow-stateless

So does the return traffic:
ACL2: to-lport 100 'outport == "A" && ip.src == B' allow-stateless

Now we need the traffic initiated from A to B's TCP port 80 to be stateful:
ACL3: from-lport 200 'inport == "X" && ip.dst == B && tcp.dst == 80'
allow-related

This seems to be what we want to achieve, right? With the priority 200 we
hope that TCP traffic between A <-> B:80 is handled as stateful and allows
the return traffic (i.e. B:80 -> A) automatically, and also probably
prevent invalid packets. However, even with these 3 patches, it doesn't
work, because packets of the direction B:80 -> A will match the stateless
ACL2 and bypass conntrack, so the CT entry committed by the A->B:80 SYN
packets is never going to become "established" state because the CT table
only sees one direction of the traffic.

I hope this example is general enough for the discussion. So we need to
answer these questions:
1. How do we define the meaning of "priority" in above scenario: what is
expected and what extra configuration needs to be done to achieve the
desired behavior.
2. If the above example is not what we want to support, then what use cases
are actually supported by adding these 3 patches (but not by the original
patch).
3. If 2) has an answer, does it worth the complexity and the extra flows
added?

Could you clarify more on this?

Thanks,
Han

> Ihar
>
> >
> > > Thanks,
> > > Han
> > >
> > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} &&
${flow_tcp}"
> > > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl
> > > > +#
tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > > > +ip.ttl--;
> > > > +eth.src = f0:00:00:00:00:02;
> > > > +eth.dst = fe:00:00:00:00:02;
> > > > +output("lsp2");
> > > > +])
> > > > +
> > > > +# Add allow-related with the same direction.
> > > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related
> > > > +ovn-nbctl --wait=sb sync
> > > > +
> > > > +# TCP packets should now go to conntrack.
> > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1
"${flow}"], [0], [dnl
> > > > +#
tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > > > +ct_next(ct_state=new|trk) {
> > > > +    ip.ttl--;
> > > > +    eth.src = f0:00:00:00:00:02;
> > > > +    eth.dst = fe:00:00:00:00:02;
> > > > +    output("lsp2");
> > > > +};
> > > > +])
> > > > +
> > > > +# Reduce priority for allow-related rule of the same direction.
> > > > +ovn-nbctl acl-del sw1 from-lport 4 tcp
> > > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related
> > > > +ovn-nbctl --wait=sb sync
> > > > +
> > > > +# TCP packets should no longer go to conntrack
> > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1
"${flow}"], [0], [dnl
> > > > +#
tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > > > +ip.ttl--;
> > > > +eth.src = f0:00:00:00:00:02;
> > > > +eth.dst = fe:00:00:00:00:02;
> > > > +output("lsp2");
> > > > +])
> > > > +
> > > > +AT_CLEANUP
> > > > +])
> > > > +
> > > >  OVN_FOR_EACH_NORTHD([
> > > >  AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related])
> > > >  ovn_start
> > > > --
> > > > 2.31.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list