[ovs-dev] [PATCH ovn v2] ovn-northd: Support mixing stateless/stateful ACLs with Stateless_Filter.

Dumitru Ceara dceara at redhat.com
Wed Sep 2 15:17:44 UTC 2020


On 8/27/20 9:04 PM, Han Zhou wrote:
> 
> 
> On Thu, Aug 27, 2020 at 12:24 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>>
>> On 8/27/20 1:39 AM, Han Zhou wrote:
>> > Hi Dumitru,
>> >
>> > Please see my comments below.
>> >
>> > Thanks,
>> > Han
>>
>> Hi Han,
>>
>> Thanks for your review.
>>
>> >
>> > On Thu, Aug 20, 2020 at 4:19 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>
>> > <mailto:dceara at redhat.com <mailto:dceara at redhat.com>>> wrote:
>> >>
>> >> A new table is added to OVN_Northbound: Stateless_Filter. Users can
>> >> populate this table with records consisting of <priority, match>. These
>> >> records generate logical flows in the PRE_ACL stages of the logical
>> >> switch pipeline.
>> >>
>> >> Packets matching these flows will completely bypass connection tracking
>> >> for ACL purposes. In specific scenarios CMSs can predetermine which
>> >> traffic must be firewalled statefully or not, e.g., UDP vs TCP.
> However,
>> >> until now, if at least one stateful ACL (allow-related) is configured
>> >> on the switch, all traffic gets sent to connection tracking.
>> >> This induces a hit in performance when forwarding packets that don't
>> >> need stateful processing.
>> >>
>> >> New command line arguments are added to ovn-nbctl (stateless-filter-*)
>> >> to allow the users to interact with the Stateless_Filter table.
>> >>
>> >> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>
>> > <mailto:dceara at redhat.com <mailto:dceara at redhat.com>>>
>> >> ---
>> >> V2:
>> >> - address Numan's comments:
>> >>   - fix spacing in the logical flow match.
>> >>   - add a new table to the NB DB instead of using a config option
> on the
>> >>     logical switch.
>> >> - add ovn-nbctl CLI commands for the new table and also unit tests for
>> >>   them.
>> >> - reword the commit message.
>> >> NOTE: checkpatch.py will complain about lines lacking whitespacec
> around
>> >> operators in the ovn-nbctl help string but this is a false positive and
>> >> should be ignored.
>> >> ---
>> >>  NEWS                          |   3 +
>> >>  northd/ovn-northd.8.xml       |  20 ++++
>> >>  northd/ovn-northd.c           | 146 ++++++++++++++++++-----
>> >>  ovn-nb.ovsschema              |  26 ++++-
>> >>  ovn-nb.xml                    |  57 ++++++++-
>> >>  tests/ovn-nbctl.at <http://ovn-nbctl.at> <http://ovn-nbctl.at>    
>        |  53 +++++++++
>> >>  tests/ovn-northd.at <http://ovn-northd.at> <http://ovn-northd.at>
>           | 263
>> > ++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/system-common-macros.at <http://system-common-macros.at>
> <http://system-common-macros.at> |   8 ++
>> >>  tests/system-ovn.at <http://system-ovn.at> <http://system-ovn.at>
>           | 113
>> > ++++++++++++++++++
>> >>  utilities/ovn-detrace.in <http://ovn-detrace.in>
> <http://ovn-detrace.in>      |  12 ++
>> >>  utilities/ovn-nbctl.c         | 213 ++++++++++++++++++++++++++++++++--
>> >>  11 files changed, 871 insertions(+), 43 deletions(-)
>> >>
>> >> diff --git a/NEWS b/NEWS
>> >> index a1ce4e8..eedd091 100644
>> >> --- a/NEWS
>> >> +++ b/NEWS
>> >> @@ -11,6 +11,9 @@ Post-v20.06.0
>> >>       called Chassis_Private now contains the nb_cfg column which is
>> > updated
>> >>       by incrementing the value in the NB_Global table, CMSes
> relying on
>> >>       this mechanism should update their code to use this new table.
>> >> +   - Added support for bypassing connection tracking for ACL
>> > processing for
>> >> +     specific types of traffic through the user supplied
> Stateless_Filter
>> >> +     configuration.
>> >>
>> >>  OVN v20.06.0
>> >>  --------------------------
>> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>> >> index 989e364..1f89942 100644
>> >> --- a/northd/ovn-northd.8.xml
>> >> +++ b/northd/ovn-northd.8.xml
>> >> @@ -322,6 +322,16 @@
>> >>      </p>
>> >>
>> >>      <p>
>> >> +      For each record in table <code>Stateful_Filter</code> in the
>> >> +      <code>OVN_Northbound</code> database, a flow with
>> >> +      <code>priority + 1000</code> is added and sets <code>reg0[7] =
>> > 1</code>
>> >> +      for traffic that matches the condition in the <code>match</code>
>> >> +      column and advances to next table.  <code>reg0[7]</code> acts
>> > as a hint
>> >> +      for tables <code>Pre-Stateful</code> and <code>ACL</code> to
> avoid
>> >> +      sending this traffic to the connection tracker.
>> >> +    </p>
>> >> +
>> >
>> > It seems documentation is missing for the flows that uses reg0[7] in
>> > Pre-Stateful and ACL stages.
>> >
>>
>> Ack, I'll add the documentation for ACL stage but in Pre-Stateful I
>> didn't use reg0[7] (REGBIT_SKIP_ACL_CT).
>>
>> >> +    <p>
>> >>        This table also has a priority-110 flow with the match
>> >>        <code>eth.dst == <var>E</var></code> for all logical switch
>> >>        datapaths to move traffic to the next table. Where <var>E</var>
>> >> @@ -1383,6 +1393,16 @@ output;
>> >>      </p>
>> >>
>> >>      <p>
>> >> +      For each record in table <code>Stateful_Filter</code> in the
>> >> +      <code>OVN_Northbound</code> database, a flow with
>> >> +      <code>priority + 1000</code> is added and sets <code>reg0[7] =
>> > 1</code>
>> >> +      for traffic that matches the condition in the <code>match</code>
>> >> +      column and advances to next table.  <code>reg0[7]</code> acts
>> > as a hint
>> >> +      for tables <code>Pre-Stateful</code> and <code>ACL</code> to
> avoid
>> >> +      sending this traffic to the connection tracker.
>> >> +    </p>
>> >> +
>> >> +    <p>
>> >>        This table also has a priority-110 flow with the match
>> >>        <code>eth.src == <var>E</var></code> for all logical switch
>> >>        datapaths to move traffic to the next table. Where <var>E</var>
>> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> >> index 212de2f..b8f457b 100644
>> >> --- a/northd/ovn-northd.c
>> >> +++ b/northd/ovn-northd.c
>> >> @@ -211,6 +211,7 @@ enum ovn_stage {
>> >>  #define REGBIT_DNS_LOOKUP_RESULT "reg0[4]"
>> >>  #define REGBIT_ND_RA_OPTS_RESULT "reg0[5]"
>> >>  #define REGBIT_HAIRPIN           "reg0[6]"
>> >> +#define REGBIT_SKIP_ACL_CT       "reg0[7]"
>> >>
>> >>  /* Register definitions for switches and routers. */
>> >>
>> >> @@ -245,11 +246,11 @@ enum ovn_stage {
>> >>   * OVS register usage:
>> >>   *
>> >>   * Logical Switch pipeline:
>> >> - * +---------+-------------------------------------+
>> >> - * | R0      | REGBIT_{CONNTRACK/DHCP/DNS/HAIRPIN} |
>> >> - * +---------+-------------------------------------+
>> >> - * | R1 - R9 |              UNUSED                 |
>> >> - * +---------+-------------------------------------+
>> >> + * +---------+-------------------------------------------------+
>> >> + * | R0      | REGBIT_{CONNTRACK/DHCP/DNS/HAIRPIN/SKIP_ACL_CT} |
>> >> + * +---------+-------------------------------------------------+
>> >> + * | R1 - R9 |              UNUSED                             |
>> >> + * +---------+-------------------------------------------------+
>> >>   *
>> >>   * Logical Router pipeline:
>> >>   *
>> >
> +-----+--------------------------+---+-----------------+---+---------------+
>> >> @@ -4713,6 +4714,12 @@ has_stateful_acl(struct ovn_datapath *od)
>> >>      return false;
>> >>  }
>> >>
>> >> +static bool
>> >> +has_stateful_acl_bypass(struct ovn_datapath *od)
>> >> +{
>> >> +    return od->nbs->n_stateless_filters > 0;
>> >> +}
>> >> +
>> >>  static void
>> >>  build_lswitch_input_port_sec(struct hmap *ports, struct hmap
> *datapaths,
>> >>                               struct hmap *lflows)
>> >> @@ -4881,7 +4888,47 @@ skip_port_from_conntrack(struct ovn_datapath
>> > *od, struct ovn_port *op,
>> >>  }
>> >>
>> >>  static void
>> >> -build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>> >> +build_stateless_filter(struct ovn_datapath *od,
>> >> +                       const struct nbrec_stateless_filter *filter,
>> >> +                       struct hmap *lflows)
>> >> +{
>> >> +    /* Stateless filters must be applied in both directions so
> that reply
>> >> +     * traffic bypasses conntrack too.
>> >> +     */
>> >> +    ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL,
>> >> +                            filter->priority + OVN_ACL_PRI_OFFSET,
>> >> +                            filter->match,
>> >> +                            REGBIT_SKIP_ACL_CT" = 1; next;",
>> >> +                            &filter->header_);
>> >> +    ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL,
>> >> +                            filter->priority + OVN_ACL_PRI_OFFSET,
>> >> +                            filter->match,
>> >> +                            REGBIT_SKIP_ACL_CT" = 1; next;",
>> >> +                            &filter->header_);
>> >> +}
>> >> +
>> >> +static void
>> >> +build_stateless_filters(struct ovn_datapath *od, struct hmap
>> > *port_groups,
>> >> +                        struct hmap *lflows)
>> >> +{
>> >> +    for (size_t i = 0; i < od->nbs->n_stateless_filters; i++) {
>> >> +        build_stateless_filter(od, od->nbs->stateless_filters[i],
>> > lflows);
>> >> +    }
>> >> +
>> >> +    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_stateless_filters;
> i++) {
>> >> +                build_stateless_filter(od,
>> > pg->nb_pg->stateless_filters[i],
>> >> +                                       lflows);
>> >> +            }
>> >> +        }
>> >> +    }
>> >> +}
>> >> +
>> >> +static void
>> >> +build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups,
>> >> +               struct hmap *lflows)
>> >>  {
>> >>      bool has_stateful = has_stateful_acl(od);
>> >>
>> >> @@ -4926,6 +4973,13 @@ build_pre_acls(struct ovn_datapath *od, struct
>> > hmap *lflows)
>> >>                        "nd || nd_rs || nd_ra || "
>> >>                        "(udp && udp.src == 546 && udp.dst == 547)",
>> > "next;");
>> >>
>> >> +        /* Ingress and Egress Pre-ACL Table (Stateless_Filter).
>> >> +         *
>> >> +         * If the logical switch is configured to bypass conntrack for
>> >> +         * specific types of traffic, skip conntrack for that traffic.
>> >> +         */
>> >> +        build_stateless_filters(od, port_groups, lflows);
>> >> +
>> >>          /* Ingress and Egress Pre-ACL Table (Priority 100).
>> >>           *
>> >>           * Regardless of whether the ACL is "from-lport" or
> "to-lport",
>> >> @@ -5260,7 +5314,8 @@ build_reject_acl_rules(struct ovn_datapath *od,
>> > struct hmap *lflows,
>> >>
>> >>  static void
>> >>  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>> >> -             struct nbrec_acl *acl, bool has_stateful)
>> >> +             struct nbrec_acl *acl, bool has_stateful,
>> >> +             bool has_stateful_bypass)
>> >>  {
>> >>      bool ingress = !strcmp(acl->direction, "from-lport") ? true
> :false;
>> >>      enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL :
> S_SWITCH_OUT_ACL;
>> >> @@ -5285,7 +5340,19 @@ consider_acl(struct hmap *lflows, struct
>> > ovn_datapath *od,
>> >>              struct ds match = DS_EMPTY_INITIALIZER;
>> >>              struct ds actions = DS_EMPTY_INITIALIZER;
>> >>
>> >> -            /* Commit the connection tracking entry if it's a new
>> >> +            /* If traffic matched the acl-stateful-bypass rule, we
> don't
>> >> +             * need to commit the connection tracking entry.
>> >> +             */
>> >> +            if (has_stateful_bypass) {
>> >> +                ds_put_format(&match, "(" REGBIT_SKIP_ACL_CT "== 1 &&
>> > (%s)",
>> >> +                              acl->match);
>> >> +                build_acl_log(&actions, acl);
>> >> +                ds_put_format(&actions, "next;");
>> >> +                ds_clear(&match);
>> >> +                ds_clear(&actions);
>> >
>> > It seems this whole "if" block is useless. The match and actions are set
>> > but then cleared without being used.
>> >
>>
>> Ooops, thanks for pointing it out. I'll remove it.
>>
>> >> +            }
>> >> +
>> >> +            /* Otherwise commit the connection tracking entry if it's
>> > a new
>> >>               * connection that matches this ACL.  After this commit,
>> >>               * the reply traffic is allowed by a flow we create at
>> >>               * priority 65535, defined earlier.
>> >> @@ -5297,10 +5364,11 @@ consider_acl(struct hmap *lflows, struct
>> > ovn_datapath *od,
>> >>               * by ct_commit in the "stateful" stage) to indicate
> that the
>> >>               * connection should be allowed to resume.
>> >>               */
>> >> -            ds_put_format(&match, "((ct.new && !ct.est)"
>> >> -                                  " || (!ct.new && ct.est && !ct.rpl "
>> >> -                                       "&& ct_label.blocked == 1)) "
>> >> -                                  "&& (%s)", acl->match);
>> >> +            ds_put_format(&match, REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                          "&& ((ct.new && !ct.est)"
>> >> +                          " || (!ct.new && ct.est && !ct.rpl "
>> >> +                               "&& ct_label.blocked == 1)) "
>> >> +                          "&& (%s)", acl->match);
>> >>              ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
>> >>              build_acl_log(&actions, acl);
>> >>              ds_put_cstr(&actions, "next;");
>> >> @@ -5315,11 +5383,16 @@ consider_acl(struct hmap *lflows, struct
>> > ovn_datapath *od,
>> >>               * deletion.  There is no need to commit here, so we
> can just
>> >>               * proceed to the next table. We use this to ensure
> that this
>> >>               * connection is still allowed by the currently defined
>> >> -             * policy. Match untracked packets too. */
>> >> +             * policy. Match untracked packets too.
>> >> +             *
>> >> +             * This flow also allows traffic that matches the
>> >> +             * acl-stateful-bypass rule.
>> >> +             */
>> >>              ds_clear(&match);
>> >>              ds_clear(&actions);
>> >>              ds_put_format(&match,
>> >> -                          "(!ct.trk || (!ct.new && ct.est && !ct.rpl"
>> >> +                          "(" REGBIT_SKIP_ACL_CT " == 1 || !ct.trk
> || "
>> >> +                          "(!ct.new && ct.est && !ct.rpl"
>> >>                            " && ct_label.blocked == 0)) && (%s)",
>> >>                            acl->match);
>> >
>> > Because of this lflow, each ACL is translated to 3 extra OVS flows (2
>> > before this patch). If large address set/port groups used in the ACL the
>> > cost can be huge. One way to optimize it could be introducing a new
>> > stage with a single logical flow to match (" REGBIT_SKIP_ACL_CT " == 1
>> > || !ct.trk ||  (!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0),
>> > and set a new flag "NO_TRACK", and then in the current ACL table it only
>> > needs a single (extra) flow for each ACL: NO_TRACK == 1 && <acl match>.
>> >
>> > Something similar can be done for "reject/drop" rules handling for the
>> > lflows with several (x '||' y) operators plus a "&&" with the real ACL
>> > match.
>> >
>> > I am not 100% sure if a new stage worth it, but I think at least it is
>> > something to be considered.
>> >
>>
>> Sounds good to me, it does reduce the number of OVS flows. I'll change
>> it as you suggested.
>>
>> >>
>> >> @@ -5346,7 +5419,7 @@ consider_acl(struct hmap *lflows, struct
>> > ovn_datapath *od,
>> >>              /* If the packet is not tracked or not part of an
> established
>> >>               * connection, then we can simply reject/drop it. */
>> >>              ds_put_cstr(&match,
>> >> -                        "(!ct.trk || !ct.est"
>> >> +                        "(" REGBIT_SKIP_ACL_CT " == 1 || !ct.trk ||
>> > !ct.est"
>> >>                          " || (ct.est && ct_label.blocked == 1))");
>> >>              if (!strcmp(acl->action, "reject")) {
>> >>                  build_reject_acl_rules(od, lflows, stage, acl, &match,
>> >> @@ -5373,7 +5446,8 @@ consider_acl(struct hmap *lflows, struct
>> > ovn_datapath *od,
>> >>               */
>> >>              ds_clear(&match);
>> >>              ds_clear(&actions);
>> >> -            ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
>> >> +            ds_put_cstr(&match, REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                        "&& ct.est && ct_label.blocked == 0");
>> >>              ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1;
>> > }; ");
>> >>              if (!strcmp(acl->action, "reject")) {
>> >>                  build_reject_acl_rules(od, lflows, stage, acl, &match,
>> >> @@ -5478,6 +5552,7 @@ build_acls(struct ovn_datapath *od, struct hmap
>> > *lflows,
>> >>             struct hmap *port_groups)
>> >>  {
>> >>      bool has_stateful = has_stateful_acl(od);
>> >> +    bool has_stateful_bypass = has_stateful_acl_bypass(od);
>> >>
>> >>      /* Ingress and Egress ACL Table (Priority 0): Packets are
> allowed by
>> >>       * default.  A related rule at priority 1 is added below if there
>> >> @@ -5508,11 +5583,15 @@ build_acls(struct ovn_datapath *od, struct
>> > hmap *lflows,
>> >>           * Subsequent packets will hit the flow at priority 0 that
> just
>> >>           * uses "next;". */
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1,
>> >> -                      "ip && (!ct.est || (ct.est && ct_label.blocked
>> > == 1))",
>> >> -                       REGBIT_CONNTRACK_COMMIT" = 1; next;");
>> >> +                      REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                      "&& ip "
>> >> +                      "&& (!ct.est || (ct.est && ct_label.blocked ==
>> > 1))",
>> >> +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1,
>> >> -                      "ip && (!ct.est || (ct.est && ct_label.blocked
>> > == 1))",
>> >> -                       REGBIT_CONNTRACK_COMMIT" = 1; next;");
>> >> +                      REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                      "&& ip "
>> >> +                      "&& (!ct.est || (ct.est && ct_label.blocked ==
>> > 1))",
>> >> +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
>> >>
>> >>          /* Ingress and Egress ACL Table (Priority 65535).
>> >>           *
>> >> @@ -5522,10 +5601,14 @@ build_acls(struct ovn_datapath *od, struct
>> > hmap *lflows,
>> >>           *
>> >>           * This is enforced at a higher priority than ACLs can be
>> > defined. */
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
>> >> -                      "ct.inv || (ct.est && ct.rpl &&
>> > ct_label.blocked == 1)",
>> >> +                      REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                      "&& (ct.inv "
>> >> +                           "|| (ct.est && ct.rpl && ct_label.blocked
>> > == 1))",
>> >>                        "drop;");
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
>> >> -                      "ct.inv || (ct.est && ct.rpl &&
>> > ct_label.blocked == 1)",
>> >> +                      REGBIT_SKIP_ACL_CT " == 0 "
>> >> +                      "&& (ct.inv "
>> >> +                           "|| (ct.est && ct.rpl && ct_label.blocked
>> > == 1))",
>> >>                        "drop;");
>> >>
>> >>          /* Ingress and Egress ACL Table (Priority 65535).
>> >> @@ -5538,11 +5621,13 @@ build_acls(struct ovn_datapath *od, struct
>> > hmap *lflows,
>> >>           *
>> >>           * This is enforced at a higher priority than ACLs can be
>> > defined. */
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
>> >> -                      "ct.est && !ct.rel && !ct.new && !ct.inv "
>> >> +                      REGBIT_SKIP_ACL_CT "== 0 "
>> >> +                      "&& ct.est && !ct.rel && !ct.new && !ct.inv "
>> >>                        "&& ct.rpl && ct_label.blocked == 0",
>> >>                        "next;");
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
>> >> -                      "ct.est && !ct.rel && !ct.new && !ct.inv "
>> >> +                      REGBIT_SKIP_ACL_CT "== 0 "
>> >> +                      "&& ct.est && !ct.rel && !ct.new && !ct.inv "
>> >>                        "&& ct.rpl && ct_label.blocked == 0",
>> >>                        "next;");
>> >>
>> >> @@ -5558,11 +5643,13 @@ build_acls(struct ovn_datapath *od, struct
>> > hmap *lflows,
>> >>           * related traffic such as an ICMP Port Unreachable through
>> >>           * that's generated from a non-listening UDP port.  */
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
>> >> -                      "!ct.est && ct.rel && !ct.new && !ct.inv "
>> >> +                      REGBIT_SKIP_ACL_CT "== 0 "
>> >> +                      "&& !ct.est && ct.rel && !ct.new && !ct.inv "
>> >>                        "&& ct_label.blocked == 0",
>> >>                        "next;");
>> >>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
>> >> -                      "!ct.est && ct.rel && !ct.new && !ct.inv "
>> >> +                      REGBIT_SKIP_ACL_CT "== 0 "
>> >> +                      "&& !ct.est && ct.rel && !ct.new && !ct.inv "
>> >>                        "&& ct_label.blocked == 0",
>> >>                        "next;");
>> >>
>> >> @@ -5578,13 +5665,14 @@ build_acls(struct ovn_datapath *od, struct
>> > hmap *lflows,
>> >>      /* Ingress or Egress ACL Table (Various priorities). */
>> >>      for (size_t i = 0; i < od->nbs->n_acls; i++) {
>> >>          struct nbrec_acl *acl = od->nbs->acls[i];
>> >> -        consider_acl(lflows, od, acl, has_stateful);
>> >> +        consider_acl(lflows, od, acl, has_stateful,
> has_stateful_bypass);
>> >>      }
>> >>      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++) {
>> >> -                consider_acl(lflows, od, pg->nb_pg->acls[i],
>> > has_stateful);
>> >> +                consider_acl(lflows, od, pg->nb_pg->acls[i],
>> > has_stateful,
>> >> +                             has_stateful_bypass);
>> >>              }
>> >>          }
>> >>      }
>> >> @@ -6617,7 +6705,7 @@ build_lswitch_flows(struct hmap *datapaths,
>> > struct hmap *ports,
>> >>              continue;
>> >>          }
>> >>
>> >> -        build_pre_acls(od, lflows);
>> >> +        build_pre_acls(od, port_groups, lflows);
>> >>          build_pre_lb(od, lflows, meter_groups, lbs);
>> >>          build_pre_stateful(od, lflows);
>> >>          build_acls(od, lflows, port_groups);
>> >> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
>> >> index 0c939b7..ef0121d 100644
>> >> --- a/ovn-nb.ovsschema
>> >> +++ b/ovn-nb.ovsschema
>> >> @@ -1,7 +1,7 @@
>> >>  {
>> >>      "name": "OVN_Northbound",
>> >> -    "version": "5.25.0",
>> >> -    "cksum": "1354137211 26116",
>> >> +    "version": "5.26.0",
>> >> +    "cksum": "1450952466 27225",
>> >>      "tables": {
>> >>          "NB_Global": {
>> >>              "columns": {
>> >> @@ -35,6 +35,12 @@
>> >>                                             "refType": "strong"},
>> >>                                     "min": 0,
>> >>                                     "max": "unlimited"}},
>> >> +                "stateless_filters": {
>> >> +                    "type": {"key": {"type": "uuid",
>> >> +                                     "refTable": "Stateless_Filter",
>> >> +                                     "refType": "strong"},
>> >> +                             "min": 0,
>> >> +                             "max": "unlimited"}},
>> >>                  "acls": {"type": {"key": {"type": "uuid",
>> >>                                            "refTable": "ACL",
>> >>                                            "refType": "strong"},
>> >> @@ -150,6 +156,12 @@
>> >>                                             "refType": "weak"},
>> >>                                     "min": 0,
>> >>                                     "max": "unlimited"}},
>> >> +                "stateless_filters": {
>> >> +                    "type": {"key": {"type": "uuid",
>> >> +                                     "refTable": "Stateless_Filter",
>> >> +                                     "refType": "strong"},
>> >> +                             "min": 0,
>> >> +                             "max": "unlimited"}},
>> >>                  "acls": {"type": {"key": {"type": "uuid",
>> >>                                            "refTable": "ACL",
>> >>                                            "refType": "strong"},
>> >> @@ -201,6 +213,16 @@
>> >>                      "type": {"key": "string", "value": "string",
>> >>                               "min": 0, "max": "unlimited"}}},
>> >>              "isRoot": false},
>> >> +        "Stateless_Filter": {
>> >> +            "columns": {
>> >> +                "priority": {"type": {"key": {"type": "integer",
>> >> +                                              "minInteger": 0,
>> >> +                                              "maxInteger": 32767}}},
>> >> +                "match": {"type": "string"},
>> >> +                "external_ids": {
>> >> +                    "type": {"key": "string", "value": "string",
>> >> +                             "min": 0, "max": "unlimited"}}},
>> >> +            "isRoot": false},
>> >
>> > Is there any specific consideration that "direction" is not needed?
>> >
>>
>> Initially I had added direction too but found it a bit confusing to use.
>> My idea was that if traffic hits a stateless filter in one direction
>> then there's no point for replies to that type of traffic to be sent to
>> conntrack.
>>
>> Not having a "direction" field does move a bit the responsibility on the
>> CMS to make sure that the "match" is true both for request and for reply
>> traffic.
>>
>> Alternatively, we could add a "direction" field but then the CMS will
>> (probably always) have to define the stateless_filters for both
> directions.
>>
>> I don't expect a lot of stateless filters to be configured and the ones
>> that will be configured should probably be quite generic. The only use
>> case I know of today is for ovn-k8s where all TCP traffic could be
>> firewalled in a stateless way while UDP would still need to go to
> conntrack.
>>
>> I don't have a strong preference though so I can add a "direction" field
>> if you think that may become useful in the future.
>>
> I don't have a preference either. Just wanted to understand the
> considerations behind. Thanks for explaining.
> 

Hi Han,

I sent a v3, turning this into a series out of which the first patch can
be applied earlier if accepted as it should reduce the number of OVS
flows generated for stateful ACLs:

http://patchwork.ozlabs.org/project/ovn/list/?series=199101

Thanks,
Dumitru



More information about the dev mailing list