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

Ihar Hrachyshka ihrachys at redhat.com
Tue Jun 1 19:28:10 UTC 2021


On Thu, May 20, 2021 at 6: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"?
>

It can be argued both ways. On one hand, indeed, it reduces the number
of flows in some scenarios. On the other hand, sending traffic to
conntrack when it, considering direction, should not be sent there,
may be considered a bug. I tried to split the series into parts but
here there are intersecting in such a way that the first patch in the
series introduces incorrect behavior in a corner case that is then
adjusted/fixed by this patch under review.

> >
> > 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") ?
>

Ouch. You are right.


> > +            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?
>
> 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