[ovs-dev] [PATCH ovn v2] northd: Support flow offloading for logical switches with no ACLs.

numans at ovn.org numans at ovn.org
Fri May 7 14:42:58 UTC 2021


From: Numan Siddique <numans at ovn.org>

Some smart NICs can't offload datapath flows matching on conntrack
fields.  If a deployment desires to make use of such smart NICs
then it cannot configure ACLs on the logical switches.  If suppose
a logical switch S1 has no ACLs configured and a logical switch S2
has ACLs configured, then the CMS would expect the datapath flows
belonging to S1 logical ports are offloaded since it has no ACLs.
But this is not working as expected (even if S1 and S2 are
not connected via a logical router).

ovn-northd generates the below logical flows in ls_in_acl_hint
and ls_in_acl stages for S1

table=8 (ls_in_acl_hint     ), priority=0    , match=(1), action=(next;)
table=9 (ls_in_acl          ), priority=0    , match=(1), action=(next;)

And the below for S2

table=8 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[7] = 1; reg0[9] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[7] = 1; reg0[9] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[8] = 1; reg0[9] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[8] = 1; reg0[10] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[9] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[9] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[10] = 1; next;)
table=8 (ls_in_acl_hint     ), priority=0    , match=(1), action=(next;)
table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
table=9 (ls_in_acl          ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;)
table=9 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[1] = 1; next;)
table=9 (ls_in_acl          ), priority=0    , match=(1), action=(next;)

Because there are higher priority flows in 'ls_in_acl_hint' and
'ls_in_acl' with the match on conntrack fields,
ovs-vswitchd will generate a datapath flow with the match on ct_state fields as -
'ct_state(-new-est-rel-rpl-inv-trk)' for the packet from S1, even though
the S1 pipeline doesn't have logical flows which match on conntrack
fields.  [1] has more information.

Modifying the below flows if a logical switch has no ACLs solves this
problem.

table=8 (ls_in_acl_hint     ), priority=65535    , match=(1), action=(next;)
table=9 (ls_in_acl          ), priority=65535    , match=(1), action=(next;)

With the above flows with higher priority, ovs-vswitchd will not
consider other flows in the same table during translation.

This patch addresses this issue by using higher prioriy flows (for both
ls_in_acl* and ls_out_acl* stages).

[1] - https://bugzilla.redhat.com/show_bug.cgi?id=1955191#c8

Signed-off-by: Numan Siddique <numans at ovn.org>
---
v1 -> v2
----
 * Rebased to resolve conflicts.
 * Addressed review comment from Dumitru. Combined ls_has_stateful_acl()
   and ls_has_acl() into one single function - od_ls_update_acls_flags().

 northd/lswitch.dl       |  12 ++++
 northd/ovn-northd.8.xml |  39 ++++++++----
 northd/ovn-northd.c     | 103 +++++++++++++++++++------------
 northd/ovn_northd.dl    |  91 +++++++++++++++-------------
 tests/ovn-northd.at     |  49 +++++++++++----
 tests/system-ovn.at     | 130 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 321 insertions(+), 103 deletions(-)

diff --git a/northd/lswitch.dl b/northd/lswitch.dl
index 27ac3cadc5..8b1f35ac43 100644
--- a/northd/lswitch.dl
+++ b/northd/lswitch.dl
@@ -125,6 +125,15 @@ LogicalSwitchHasStatefulACL(ls, false) :-
     nb::Logical_Switch(._uuid = ls),
     not LogicalSwitchStatefulACL(ls, _).
 
+relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool)
+
+LogicalSwitchHasACLs(ls, true) :-
+    LogicalSwitchACL(ls, _).
+
+LogicalSwitchHasACLs(ls, false) :-
+    nb::Logical_Switch(._uuid = ls),
+    not LogicalSwitchACL(ls, _).
+
 /*
  * LogicalSwitchLocalnetPorts maps from each logical switch UUID
  * to the logical switch's set of localnet ports.  Each localnet
@@ -189,6 +198,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
 relation &Switch(
     ls:                nb::Logical_Switch,
     has_stateful_acl:  bool,
+    has_acls:          bool,
     has_lb_vip:        bool,
     has_dns_records:   bool,
     has_unknown_ports: bool,
@@ -215,6 +225,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
 
 &Switch(.ls                = ls,
         .has_stateful_acl  = has_stateful_acl,
+        .has_acls          = has_acls,
         .has_lb_vip        = has_lb_vip,
         .has_dns_records   = has_dns_records,
         .has_unknown_ports = has_unknown_ports,
@@ -226,6 +237,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
         .is_vlan_transparent = is_vlan_transparent) :-
     nb::Logical_Switch[ls],
     LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
+    LogicalSwitchHasACLs(ls._uuid, has_acls),
     LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
     LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
     LogicalSwitchHasUnknownPorts(ls._uuid, has_unknown_ports),
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 1aef75b273..7720d45487 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -573,6 +573,14 @@
     <p>
       The table contains the following flows:
     </p>
+    <ul>
+      <li>
+        A priority-65535 flow to advance to the next table if the logical
+        switch has <code>no</code> ACLs configured, otherwise a
+        priority-0 flow to advance to the next table.
+      </li>
+    </ul>
+
     <ul>
       <li>
         A priority-7 flow that matches on packets that initiate a new session.
@@ -613,9 +621,6 @@
         This flow sets <code>reg0[10]</code> and then advances to the next
         table.
       </li>
-      <li>
-        A priority-0 flow to advance to the next table.
-      </li>
     </ul>
 
     <h3>Ingress table 9: <code>from-lport</code> ACLs</h3>
@@ -661,9 +666,14 @@
     </ul>
 
     <p>
-      This table also contains a priority 0 flow with action
-      <code>next;</code>, so that ACLs allow packets by default.  If the
-      logical datapath has a stateful ACL or a load balancer with VIP
+      This table contains a priority-65535 flow to advance to the next table
+      if the logical switch has <code>no</code> ACLs configured, otherwise a
+        priority-0 flow to advance to the next table so that ACLs allow
+        packets by default.
+    </p>
+
+    <p>
+      If the logical datapath has a stateful ACL or a load balancer with VIP
       configured, the following flows will also be added:
     </p>
 
@@ -677,7 +687,7 @@
       </li>
 
       <li>
-        A priority-65535 flow that allows any traffic in the reply
+        A priority-65532 flow that allows any traffic in the reply
         direction for a connection that has been committed to the
         connection tracker (i.e., established flows), as long as
         the committed flow does not have <code>ct_label.blocked</code> set.
@@ -690,19 +700,19 @@
       </li>
 
       <li>
-        A priority-65535 flow that allows any traffic that is considered
+        A priority-65532 flow that allows any traffic that is considered
         related to a committed flow in the connection tracker (e.g., an
         ICMP Port Unreachable from a non-listening UDP port), as long
         as the committed flow does not have <code>ct_label.blocked</code> set.
       </li>
 
       <li>
-        A priority-65535 flow that drops all traffic marked by the
+        A priority-65532 flow that drops all traffic marked by the
         connection tracker as invalid.
       </li>
 
       <li>
-        A priority-65535 flow that drops all traffic in the reply direction
+        A priority-65532 flow that drops all traffic in the reply direction
         with <code>ct_label.blocked</code> set meaning that the connection
         should no longer be allowed due to a policy change.  Packets
         in the request direction are skipped here to let a newly created
@@ -710,11 +720,18 @@
       </li>
 
       <li>
-        A priority-65535 flow that allows IPv6 Neighbor solicitation,
+        A priority-65532 flow that allows IPv6 Neighbor solicitation,
         Neighbor discover, Router solicitation, Router advertisement and MLD
         packets.
       </li>
+    </ul>
 
+    <p>
+      If the logical datapath has any ACL or a load balancer with VIP
+      configured, the following flow will also be added:
+    </p>
+
+    <ul>
       <li>
         A priority 34000 logical flow is added for each logical switch datapath
         with the match <code>eth.dst = <var>E</var></code> to allow the service
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 1d4c5b67b3..b137cba6b6 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -629,6 +629,7 @@ struct ovn_datapath {
     bool has_stateful_acl;
     bool has_lb_vip;
     bool has_unknown;
+    bool has_acls;
 
     /* IPAM data. */
     struct ipam_info ipam_info;
@@ -667,9 +668,6 @@ struct ovn_datapath {
     struct hmap nb_pgs;
 };
 
-static bool ls_has_stateful_acl(struct ovn_datapath *od);
-static bool ls_has_lb_vip(struct ovn_datapath *od);
-
 /* Contains a NAT entry with the external addresses pre-parsed. */
 struct ovn_nat {
     const struct nbrec_nat *nb;
@@ -4761,27 +4759,38 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs)
     hmap_destroy(nb_pgs);
 }
 
-static bool
-ls_has_stateful_acl(struct ovn_datapath *od)
+static void
+od_ls_update_acls_flags(struct ovn_datapath *od)
 {
-    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")) {
-            return true;
+    od->has_acls = false;
+    od->has_stateful_acl = 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;
+                return;
+            }
         }
     }
 
     struct ovn_ls_port_group *ls_pg;
     HMAP_FOR_EACH (ls_pg, key_node, &od->nb_pgs) {
-        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")) {
-                return true;
+        if (ls_pg->nb_pg->n_acls) {
+            od->has_acls = true;
+
+            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;
+                    return;
+                }
             }
         }
     }
-
-    return false;
 }
 
 /* Logical switch ingress table 0: Ingress port security - L2
@@ -5274,7 +5283,11 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
         enum ovn_stage stage = stages[i];
 
         /* In any case, advance to the next stage. */
-        ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
+        if (!od->has_acls && !od->has_lb_vip) {
+            ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;");
+        } else {
+            ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
+        }
 
         if (!od->has_stateful_acl && !od->has_lb_vip) {
             continue;
@@ -5674,10 +5687,19 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
     bool has_stateful = od->has_stateful_acl || od->has_lb_vip;
 
     /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by
-     * default.  A related rule at priority 1 is added below if there
+     * default.  If the logical switch has no ACLs or no load balancers,
+     * then add 65535-priority flow to advance the packet to next
+     * stage.
+     *
+     * A related rule at priority 1 is added below if there
      * are any stateful ACLs in this datapath. */
-    ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1", "next;");
-    ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1", "next;");
+    if (!od->has_acls && !od->has_lb_vip) {
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "1", "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "1", "next;");
+    } else {
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1", "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1", "next;");
+    }
 
     if (has_stateful) {
         /* Ingress and Egress ACL Table (Priority 1).
@@ -5708,7 +5730,7 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
                       "ip && (!ct.est || (ct.est && ct_label.blocked == 1))",
                        REGBIT_CONNTRACK_COMMIT" = 1; next;");
 
-        /* Ingress and Egress ACL Table (Priority 65535).
+        /* Ingress and Egress ACL Table (Priority 65532).
          *
          * Always drop traffic that's in an invalid state.  Also drop
          * reply direction packets for connections that have been marked
@@ -5718,13 +5740,13 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
         char *match =
             xasprintf("%s(ct.est && ct.rpl && ct_label.blocked == 1)",
                       use_ct_inv_match ? "ct.inv || " : "");
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
                       match, "drop;");
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
                       match, "drop;");
         free(match);
 
-        /* Ingress and Egress ACL Table (Priority 65535).
+        /* Ingress and Egress ACL Table (Priority 65535 - 3).
          *
          * Allow reply traffic that is part of an established
          * conntrack entry that has not been marked for deletion
@@ -5737,9 +5759,9 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
                           "ct.rpl && ct_label.blocked == 0",
                           use_ct_inv_match ? " && !ct.inv" : "");
 
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
                       match, "next;");
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
                       match, "next;");
         free(match);
 
@@ -5757,18 +5779,18 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
         match = xasprintf("!ct.est && ct.rel && !ct.new%s && "
                           "ct_label.blocked == 0",
                           use_ct_inv_match ? " && !ct.inv" : "");
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
                       match, "next;");
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
                       match, "next;");
         free(match);
 
-        /* Ingress and Egress ACL Table (Priority 65535).
+        /* Ingress and Egress ACL Table (Priority 65532).
          *
          * Not to do conntrack on ND packets. */
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
                       "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;");
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
                       "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;");
     }
 
@@ -5855,15 +5877,18 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
             actions);
     }
 
-    /* Add a 34000 priority flow to advance the service monitor reply
-     * packets to skip applying ingress ACLs. */
-    ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 34000,
-                  "eth.dst == $svc_monitor_mac", "next;");
 
-    /* Add a 34000 priority flow to advance the service monitor packets
-     * generated by ovn-controller to skip applying egress ACLs. */
-    ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 34000,
-                  "eth.src == $svc_monitor_mac", "next;");
+    if (od->has_acls || od->has_lb_vip) {
+        /* Add a 34000 priority flow to advance the service monitor reply
+        * packets to skip applying ingress ACLs. */
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 34000,
+                    "eth.dst == $svc_monitor_mac", "next;");
+
+        /* Add a 34000 priority flow to advance the service monitor packets
+        * generated by ovn-controller to skip applying egress ACLs. */
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 34000,
+                    "eth.src == $svc_monitor_mac", "next;");
+    }
 }
 
 static void
@@ -6792,8 +6817,8 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od,
                                      struct hmap *lbs)
 {
     if (od->nbs) {
-        od->has_stateful_acl = ls_has_stateful_acl(od);
         od->has_lb_vip = ls_has_lb_vip(od);
+        od_ls_update_acls_flags(od);
 
         build_pre_acls(od, lflows);
         build_pre_lb(od, lflows, meter_groups, lbs);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index ffd09c35f5..4ccd32acad 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -2290,20 +2290,28 @@ for (UseCtInvMatch[use_ct_inv_match]) {
     var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
     {
         /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by
-         * default.  A related rule at priority 1 is added below if there
+         * default.  If the logical switch has no ACLs or no load balancers,
+         * then add 65535-priority flow to advance the packet to next
+         * stage.
+         *
+         * A related rule at priority 1 is added below if there
          * are any stateful ACLs in this datapath. */
-        Flow(.logical_datapath = ls._uuid,
-             .stage            = s_SWITCH_IN_ACL(),
-             .priority         = 0,
-             .__match          = "1",
-             .actions          = "next;",
-             .external_ids     = map_empty());
-        Flow(.logical_datapath = ls._uuid,
-             .stage            = s_SWITCH_OUT_ACL(),
-             .priority         = 0,
-             .__match          = "1",
-             .actions          = "next;",
-             .external_ids     = map_empty());
+        var priority = if (not sw.has_acls and not sw.has_lb_vip) { 65535 } else { 0 }
+        in
+        {
+            Flow(.logical_datapath = ls._uuid,
+                .stage            = s_SWITCH_IN_ACL(),
+                .priority         = priority,
+                .__match          = "1",
+                .actions          = "next;",
+                .external_ids     = map_empty());
+            Flow(.logical_datapath = ls._uuid,
+                .stage            = s_SWITCH_OUT_ACL(),
+                .priority         = priority,
+                .__match          = "1",
+                .actions          = "next;",
+                .external_ids     = map_empty())
+        };
 
         if (has_stateful) {
             /* Ingress and Egress ACL Table (Priority 1).
@@ -2340,7 +2348,7 @@ for (UseCtInvMatch[use_ct_inv_match]) {
                  .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; next;",
                  .external_ids     = map_empty());
 
-            /* Ingress and Egress ACL Table (Priority 65535).
+            /* Ingress and Egress ACL Table (Priority 65532).
              *
              * Always drop traffic that's in an invalid state.  Also drop
              * reply direction packets for connections that have been marked
@@ -2349,18 +2357,18 @@ for (UseCtInvMatch[use_ct_inv_match]) {
              * This is enforced at a higher priority than ACLs can be defined. */
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_IN_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = ct_inv_or ++ "(ct.est && ct.rpl && ct_label.blocked == 1)",
                  .actions          = "drop;",
                  .external_ids     = map_empty());
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_OUT_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = ct_inv_or ++ "(ct.est && ct.rpl && ct_label.blocked == 1)",
                  .actions          = "drop;",
                  .external_ids     = map_empty());
 
-            /* Ingress and Egress ACL Table (Priority 65535).
+            /* Ingress and Egress ACL Table (Priority 65532).
              *
              * Allow reply traffic that is part of an established
              * conntrack entry that has not been marked for deletion
@@ -2371,20 +2379,20 @@ for (UseCtInvMatch[use_ct_inv_match]) {
              * This is enforced at a higher priority than ACLs can be defined. */
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_IN_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "ct.est && !ct.rel && !ct.new " ++ and_not_ct_inv ++
                                      "&& ct.rpl && ct_label.blocked == 0",
                  .actions          = "next;",
                  .external_ids     = map_empty());
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_OUT_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "ct.est && !ct.rel && !ct.new " ++ and_not_ct_inv ++
                                      "&& ct.rpl && ct_label.blocked == 0",
                  .actions          = "next;",
                  .external_ids     = map_empty());
 
-            /* Ingress and Egress ACL Table (Priority 65535).
+            /* Ingress and Egress ACL Table (Priority 65532).
              *
              * Allow traffic that is related to an existing conntrack entry that
              * has not been marked for deletion (bit 0 of ct_label).
@@ -2397,31 +2405,31 @@ for (UseCtInvMatch[use_ct_inv_match]) {
              * that's generated from a non-listening UDP port.  */
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_IN_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "!ct.est && ct.rel && !ct.new " ++ and_not_ct_inv ++
                                      "&& ct_label.blocked == 0",
                  .actions          = "next;",
                  .external_ids     = map_empty());
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_OUT_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "!ct.est && ct.rel && !ct.new " ++ and_not_ct_inv ++
                                      "&& ct_label.blocked == 0",
                  .actions          = "next;",
                  .external_ids     = map_empty());
 
-            /* Ingress and Egress ACL Table (Priority 65535).
+            /* Ingress and Egress ACL Table (Priority 65532).
              *
              * Not to do conntrack on ND packets. */
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_IN_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "nd || nd_ra || nd_rs || mldv1 || mldv2",
                  .actions          = "next;",
                  .external_ids     = map_empty());
             Flow(.logical_datapath = ls._uuid,
                  .stage            = s_SWITCH_OUT_ACL(),
-                 .priority         = 65535,
+                 .priority         = 65532,
                  .__match          = "nd || nd_ra || nd_rs || mldv1 || mldv2",
                  .actions          = "next;",
                  .external_ids     = map_empty())
@@ -2439,20 +2447,22 @@ for (UseCtInvMatch[use_ct_inv_match]) {
                  .external_ids     = map_empty())
         };
 
-        /* Add a 34000 priority flow to advance the service monitor reply
-         * packets to skip applying ingress ACLs. */
-        Flow(.logical_datapath = ls._uuid,
-             .stage            = s_SWITCH_IN_ACL(),
-             .priority         = 34000,
-             .__match          = "eth.dst == $svc_monitor_mac",
-             .actions          = "next;",
-             .external_ids     = map_empty());
-        Flow(.logical_datapath = ls._uuid,
-             .stage            = s_SWITCH_OUT_ACL(),
-             .priority         = 34000,
-             .__match          = "eth.src == $svc_monitor_mac",
-             .actions          = "next;",
-             .external_ids     = map_empty())
+        if (sw.has_acls or sw.has_lb_vip) {
+            /* Add a 34000 priority flow to advance the service monitor reply
+            * packets to skip applying ingress ACLs. */
+            Flow(.logical_datapath = ls._uuid,
+                .stage            = s_SWITCH_IN_ACL(),
+                .priority         = 34000,
+                .__match          = "eth.dst == $svc_monitor_mac",
+                .actions          = "next;",
+                .external_ids     = map_empty());
+            Flow(.logical_datapath = ls._uuid,
+                .stage            = s_SWITCH_OUT_ACL(),
+                .priority         = 34000,
+                .__match          = "eth.src == $svc_monitor_mac",
+                .actions          = "next;",
+                .external_ids     = map_empty())
+        }
     }
 }
 
@@ -2474,7 +2484,8 @@ AclHintStages[s_SWITCH_OUT_ACL_HINT()].
 for (sw in &Switch(.ls = ls)) {
     for (AclHintStages[stage]) {
         /* In any case, advance to the next stage. */
-        Flow(ls._uuid, stage, 0, "1", "next;", map_empty())
+        var priority = if (not sw.has_acls and not sw.has_lb_vip) { 65535 } else { 0 } in
+        Flow(ls._uuid, stage, priority, "1", "next;", map_empty())
     };
 
     for (AclHintStages[stage])
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 10b2046247..bedd556d53 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2101,9 +2101,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e
   table=3 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=3 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=4 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
   table=8 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
@@ -2112,9 +2112,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e
   table=8 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=9 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
 ])
 
 AS_BOX([Check match ct_state with load balancer])
@@ -2124,7 +2124,8 @@ check ovn-nbctl --wait=sb \
     -- lb-add lb "10.0.0.1" "10.0.0.2" \
     -- ls-lb-add ls lb
 
-AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl
+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | sort], [0], [dnl
+  table=3 (ls_out_acl_hint    ), priority=0    , match=(1), action=(next;)
   table=3 (ls_out_acl_hint    ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
   table=3 (ls_out_acl_hint    ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
   table=3 (ls_out_acl_hint    ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
@@ -2132,10 +2133,16 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e
   table=3 (ls_out_acl_hint    ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
   table=3 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=3 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl         ), priority=0    , match=(1), action=(next;)
   table=4 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
-  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;)
+  table=4 (ls_out_acl         ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;)
+  table=4 (ls_out_acl         ), priority=34000, match=(eth.src == $svc_monitor_mac), action=(next;)
+  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=4 (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=4 (ls_out_acl         ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
+  table=8 (ls_in_acl_hint     ), priority=0    , match=(1), action=(next;)
   table=8 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
@@ -2143,12 +2150,28 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e
   table=8 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
   table=8 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=9 (ls_in_acl          ), priority=0    , match=(1), action=(next;)
   table=9 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
-  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;)
+  table=9 (ls_in_acl          ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;)
+  table=9 (ls_in_acl          ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;)
+  table=9 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=9 (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=9 (ls_in_acl          ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
 ])
 
+ovn-nbctl --wait=sb clear logical_switch ls acls
+ovn-nbctl --wait=sb clear logical_switch ls load_balancer
+
+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | sort], [0], [dnl
+  table=3 (ls_out_acl_hint    ), priority=65535, match=(1), action=(next;)
+  table=4 (ls_out_acl         ), priority=65535, match=(1), action=(next;)
+  table=8 (ls_in_acl_hint     ), priority=65535, match=(1), action=(next;)
+  table=9 (ls_in_acl          ), priority=65535, match=(1), action=(next;)
+])
+
+
 AT_CLEANUP
 ])
 
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index f89a74cff9..adc087b8d4 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -5890,3 +5890,133 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
 /.*terminating with signal 15.*/d"])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- No ct_state matches in dp flows when no ACLs in an LS])
+AT_KEYWORDS([no ct_state match])
+ovn_start
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+check ovn-nbctl ls-add sw0
+
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03"
+
+check ovn-nbctl lsp-add sw0 sw0-p2
+check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
+check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
+
+
+# Create the second logical switch with one port and configure some ACLs.
+check ovn-nbctl ls-add sw1
+check ovn-nbctl lsp-add sw1 sw1-p1
+
+# Create port group and ACLs for sw1 ports.
+check ovn-nbctl pg-add pg1 sw1-p1
+check ovn-nbctl acl-add pg1 from-lport 1002 "ip" allow-related
+check ovn-nbctl acl-add pg1 to-lport 1002 "ip" allow-related
+
+
+OVN_POPULATE_ARP
+ovn-nbctl --wait=hv sync
+
+ADD_NAMESPACES(sw0-p1)
+ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \
+         "10.0.0.1")
+
+
+ADD_NAMESPACES(sw0-p2)
+ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \
+         "10.0.0.1")
+
+ADD_NAMESPACES(sw1-p1)
+ADD_VETH(sw1-p1, sw1-p1, br-int, "20.0.0.4/24", "30:54:00:00:00:04", \
+         "20.0.0.1")
+
+wait_for_ports_up
+
+NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+ovs-appctl dpctl/dump-flows
+
+# sw1-p1 may send IPv6 traffic.  So filter this out.  Since sw1-p1 has
+# ACLs configured, the datapath flows for the packets from sw1-p1 will have
+# matches on ct_state and ct_label fields.
+# Since sw0 doesn't have any ACLs, there should be no match on ct fields.
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [1], [dnl
+0
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [1], [dnl
+0
+])
+
+# Add an ACL to sw0.
+check ovn-nbctl --wait=hv acl-add sw0 to-lport 1002 ip allow-related
+
+NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+ovs-appctl dpctl/dump-flows
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [0], [ignore])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [0], [ignore])
+
+# Clear ACL for sw0
+check ovn-nbctl --wait=hv clear logical_switch sw0 acls
+
+check ovs-appctl dpctl/del-flows
+
+check ovn-nbctl --wait=hv sync
+
+NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+ovs-appctl dpctl/dump-flows
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [1], [dnl
+0
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [1], [dnl
+0
+])
+
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+])
-- 
2.30.2




More information about the dev mailing list