[ovs-dev] [PATCH ovn v2 1/4] Honor allow-related priority when stateless present

Han Zhou hzhou at ovn.org
Wed Jun 2 06:29:12 UTC 2021


On Tue, Jun 1, 2021 at 2:38 PM Ihar Hrachyshka <ihrachys at redhat.com> wrote:
>
> For each allow-stateless ACL, a rule was added earlier in the pipeline
> that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of
> whether other, e.g. allow-related ACLs with higher priority were
> present.
>
> Now, when allow-stateless ACLs are present on the switch, for each
> allow-related, insert an early pipeline rule that would set DEFRAG bit
> for the corresponding match and priority.
>
> Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
>
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
>

Thanks Ihar for the update. Each individual patch in this series looks
good, but I have some concerns not sorted out overall. Please see my reply
here (I replied before I see this new version):
https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383513.html

Han

> ===
>
> v1: initial commit
> v2: document northd flow change
> v2: combine two similar ddlog statements
> v2: don't map has_fair_meter in ddlog statement.
> ---
>  northd/lswitch.dl       |  20 ++++++++
>  northd/ovn-northd.8.xml |   4 +-
>  northd/ovn-northd.c     |  91 ++++++++++++++++++++++++++++--------
>  northd/ovn_northd.dl    |  21 ++++++++-
>  tests/ovn-northd.at     | 100 ++++++++++++++++++++++++++++++++++++++--
>  5 files changed, 210 insertions(+), 26 deletions(-)
>
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index 402df48ef..c81b871cf 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -128,6 +128,23 @@ LogicalSwitchHasStatefulACL(ls, false) :-
>      nb::Logical_Switch(._uuid = ls),
>      not LogicalSwitchStatefulACL(ls, _).
>
> +relation LogicalSwitchStatelessACL(ls: uuid, acl: uuid)
> +
> +LogicalSwitchStatelessACL(ls, acl) :-
> +    LogicalSwitchACL(ls, acl),
> +    &nb::ACL(._uuid = acl, .action = "allow-stateless").
> +
> +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> +// is an output relation:
> +output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> +
> +LogicalSwitchHasStatelessACL(ls, true) :-
> +    LogicalSwitchStatelessACL(ls, _).
> +
> +LogicalSwitchHasStatelessACL(ls, false) :-
> +    nb::Logical_Switch(._uuid = ls),
> +    not LogicalSwitchStatelessACL(ls, _).
> +
>  // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
>  // is an output relation:
>  output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool)
> @@ -214,6 +231,7 @@ typedef Switch = Switch {
>
>      /* Additional computed fields. */
>      has_stateful_acl:  bool,
> +    has_stateless_acl: bool,
>      has_acls:          bool,
>      has_lb_vip:        bool,
>      has_dns_records:   bool,
> @@ -250,6 +268,7 @@ Switch[Switch{
>             .external_ids      = ls.external_ids,
>
>             .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,
> @@ -263,6 +282,7 @@ Switch[Switch{
>         }.intern()] :-
>      nb::Logical_Switch[ls],
>      LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
> +    LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl),
>      LogicalSwitchHasACLs(ls._uuid, has_acls),
>      LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
>      LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 407464602..656534491 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -450,7 +450,9 @@
>        priority-110 flow is added to skip over stateful ACLs. IPv6
Neighbor
>        Discovery and MLD traffic also skips stateful ACLs. For
"allow-stateless"
>        ACLs, a flow is added to bypass setting the hint for connection
tracker
> -      processing.
> +      processing. If both "allow-stateless" and "allow-related" rules are
> +      present, rules with appropriate priority are added to set
> +      <code>reg0[0]</code> for stateful traffic.
>      </p>
>
>      <p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 9652ce252..fd57e62e8 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4989,6 +4989,38 @@ skip_port_from_conntrack(struct ovn_datapath *od,
struct ovn_port *op,
>      ds_destroy(&match_out);
>  }
>
> +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 *))
> +{
> +    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)) {
> +            func(od, acl, lflows);
> +            applied = true;
> +        }
> +    }
> +
> +    struct ovn_port_group *pg;
> +    HMAP_FOR_EACH (pg, key_node, port_groups) {
> +        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)) {
> +                    func(od, acl, lflows);
> +                    applied = true;
> +                }
> +            }
> +        }
> +    }
> +    return applied;
> +}
> +
>  static void
>  build_stateless_filter(struct ovn_datapath *od,
>                         const struct nbrec_acl *acl,
> @@ -5009,28 +5041,47 @@ build_stateless_filter(struct ovn_datapath *od,
>      }
>  }
>
> -static void
> -build_stateless_filters(struct ovn_datapath *od, struct hmap
*port_groups,
> +static bool
> +build_stateless_filters(struct ovn_datapath *od,
> +                        const struct hmap *port_groups,
>                          struct hmap *lflows)
>  {
> -    for (size_t i = 0; i < od->nbs->n_acls; i++) {
> -        const struct nbrec_acl *acl = od->nbs->acls[i];
> -        if (!strcmp(acl->action, "allow-stateless")) {
> -            build_stateless_filter(od, acl, lflows);
> -        }
> -    }
> +    return apply_to_each_acl_of_action(
> +        od, port_groups, lflows, "allow-stateless",
build_stateless_filter);
> +}
>
> -    struct ovn_port_group *pg;
> -    HMAP_FOR_EACH (pg, key_node, port_groups) {
> -        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, "allow-stateless")) {
> -                    build_stateless_filter(od, acl, lflows);
> -                }
> -            }
> -        }
> +static void
> +build_stateful_override_filter(struct ovn_datapath *od,
> +                               const struct nbrec_acl *acl,
> +                               struct hmap *lflows)
> +{
> +    struct ds match = DS_EMPTY_INITIALIZER;
> +    ds_put_format(&match, "ip && %s", acl->match);
> +
> +    if (!strcmp(acl->direction, "from-lport")) {
> +        ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL,
> +                                acl->priority + OVN_ACL_PRI_OFFSET,
> +                                ds_cstr(&match),
> +                                REGBIT_CONNTRACK_DEFRAG" = 1; next;",
> +                                &acl->header_);
> +    } else {
> +        ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL,
> +                                acl->priority + OVN_ACL_PRI_OFFSET,
> +                                ds_cstr(&match),
> +                                REGBIT_CONNTRACK_DEFRAG" = 1; next;",
> +                                &acl->header_);
>      }
> +    ds_destroy(&match);
> +}
> +
> +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);
>  }
>
>  static void
> @@ -5063,7 +5114,9 @@ build_pre_acls(struct ovn_datapath *od, struct hmap
*port_groups,
>                                       110, lflows);
>          }
>
> -        build_stateless_filters(od, port_groups, lflows);
> +        if (build_stateless_filters(od, port_groups, lflows)) {
> +            build_stateful_override_filters(od, port_groups, lflows);
> +        }
>
>          /* Ingress and Egress Pre-ACL Table (Priority 110).
>           *
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index cb8418540..a29c9fba8 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1841,7 +1841,7 @@ for (&Switch(._uuid =ls_uuid)) {
>           .external_ids     = map_empty())
>  }
>
> -for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl,
.has_fair_meter = fair_meter)) {
> +for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl)) {
>      if (sw.has_stateful_acl) {
>          if (acl.action == "allow-stateless") {
>              if (acl.direction == "from-lport") {
> @@ -1860,6 +1860,25 @@ for (&SwitchACL(.sw = sw@&Switch{._uuid =
ls_uuid}, .acl = &acl, .has_fair_meter
>                       .external_ids     = stage_hint(acl._uuid))
>              }
>          }
> +    };
> +    if (sw.has_stateless_acl) {
> +        if (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))
> +            }
> +        }
>      }
>  }
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 8bd6e48ec..0ff367af6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2674,6 +2674,97 @@ 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])
> +ovn_start
> +
> +ovn-nbctl ls-add ls
> +ovn-nbctl lsp-add ls lsp1
> +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01
> +ovn-nbctl lsp-add ls lsp2
> +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02
> +
> +for direction in from to; do
> +    ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related
> +done
> +ovn-nbctl --wait=sb sync
> +
> +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02'
> +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66'
> +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 go to conntrack.
> +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +ct_next(ct_state=new|trk) {
> +    ct_next(ct_state=new|trk) {
> +        output("lsp2");
> +    };
> +};
> +])
> +
> +# Add allow-stateless with lower priority.
> +for direction in from to; do
> +    ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should still go to conntrack.
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +ct_next(ct_state=new|trk) {
> +    ct_next(ct_state=new|trk) {
> +        output("lsp2");
> +    };
> +};
> +])
> +
> +# Add another allow-stateless with higher priority.
> +for direction in from to; do
> +    ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should no longer go to conntrack
> +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +output("lsp2");
> +])
> +
> +# Remove the higher priority allow-stateless.
> +for direction in from to; do
> +    ovn-nbctl acl-del ls ${direction}-lport 5 tcp
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should again go to conntrack.
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +ct_next(ct_state=new|trk) {
> +    ct_next(ct_state=new|trk) {
> +        output("lsp2");
> +    };
> +};
> +])
> +
> +# Remove the allow-related ACL.
> +for direction in from to; do
> +    ovn-nbctl acl-del ls ${direction}-lport 3 tcp
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should no longer go to conntrack
> +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +output("lsp2");
> +])
> +
> +AT_CLEANUP
> +])
> +
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([ovn -- ACL allow-stateless omit conntrack - Logical_Switch])
>  ovn_start
> @@ -2722,7 +2813,7 @@ ct_next(ct_state=new|trk) {
>
>  # Allow stateless for TCP.
>  for direction in from to; do
> -    ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> +    ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
>  done
>  ovn-nbctl --wait=sb sync
>
> @@ -2778,7 +2869,7 @@ ct_lb {
>
>  # Allow stateless for TCP.
>  for direction in from to; do
> -    ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> +    ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
>  done
>  ovn-nbctl --wait=sb sync
>
> @@ -2827,7 +2918,6 @@ done
>  ovn-nbctl --wait=sb sync
>
>  lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
> -echo $lsp1_inport
>
>  flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02'
>  flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66'
> @@ -2858,7 +2948,7 @@ ct_next(ct_state=new|trk) {
>
>  # Allow stateless for TCP.
>  for direction in from to; do
> -    ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless
> +    ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless
>  done
>  ovn-nbctl --wait=sb sync
>
> @@ -2914,7 +3004,7 @@ ct_lb {
>
>  # Allow stateless for TCP.
>  for direction in from to; do
> -    ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless
> +    ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless
>  done
>  ovn-nbctl --wait=sb sync
>
> --
> 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