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

Ihar Hrachyshka ihrachys at redhat.com
Tue Jun 1 21:38:22 UTC 2021


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>

===

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



More information about the dev mailing list