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

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


While we *should not* circumvent conntrack when a stateful ACL of higher
priority is present on the switch, we should do so only when
allow-stateless and allow-stateful directions are the same, otherwise we
should still skip conntrack for allow-stateless ACLs.

Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")

Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>

===

v1: initial commit
v2: fixed ddlog rule matching
---
 northd/lswitch.dl    | 64 +++++++++++++++++++++----------
 northd/ovn-northd.c  | 89 ++++++++++++++++++++++++++++++++++----------
 northd/ovn_northd.dl | 32 ++++++++--------
 tests/ovn-northd.at  | 72 +++++++++++++++++++++++++++++++++++
 4 files changed, 201 insertions(+), 56 deletions(-)

diff --git a/northd/lswitch.dl b/northd/lswitch.dl
index c81b871cf..8fd132647 100644
--- a/northd/lswitch.dl
+++ b/northd/lswitch.dl
@@ -134,16 +134,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
     LogicalSwitchACL(ls, acl),
     &nb::ACL(._uuid = acl, .action = "allow-stateless").
 
+relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
+
+LogicalSwitchStatelessFromACL(ls, acl) :-
+    LogicalSwitchStatelessACL(ls, acl),
+    &nb::ACL(._uuid = acl, .direction = "from-lport").
+
+// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
+// is an output relation:
+output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool)
+
+LogicalSwitchHasStatelessFromACL(ls, true) :-
+    LogicalSwitchStatelessFromACL(ls, _).
+
+LogicalSwitchHasStatelessFromACL(ls, false) :-
+    nb::Logical_Switch(._uuid = ls),
+    not LogicalSwitchStatelessFromACL(ls, _).
+
+relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
+
+LogicalSwitchStatelessToACL(ls, acl) :-
+    LogicalSwitchStatelessACL(ls, acl),
+    &nb::ACL(._uuid = acl, .direction = "to-lport").
+
 // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
 // is an output relation:
-output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool)
+output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool)
 
-LogicalSwitchHasStatelessACL(ls, true) :-
-    LogicalSwitchStatelessACL(ls, _).
+LogicalSwitchHasStatelessToACL(ls, true) :-
+    LogicalSwitchStatelessToACL(ls, _).
 
-LogicalSwitchHasStatelessACL(ls, false) :-
+LogicalSwitchHasStatelessToACL(ls, false) :-
     nb::Logical_Switch(._uuid = ls),
-    not LogicalSwitchStatelessACL(ls, _).
+    not LogicalSwitchStatelessToACL(ls, _).
 
 // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
 // is an output relation:
@@ -230,17 +253,18 @@ typedef Switch = Switch {
     external_ids:      Map<string,string>,
 
     /* Additional computed fields. */
-    has_stateful_acl:  bool,
-    has_stateless_acl: bool,
-    has_acls:          bool,
-    has_lb_vip:        bool,
-    has_dns_records:   bool,
-    has_unknown_ports: bool,
-    localnet_ports:    Vec<(uuid, string)>,  // UUID and name of each localnet port.
-    subnet:            Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
-    ipv6_prefix:       Option<in6_addr>,
-    mcast_cfg:         Intern<McastSwitchCfg>,
-    is_vlan_transparent: bool,
+    has_stateful_acl:       bool,
+    has_stateless_from_acl: bool,
+    has_stateless_to_acl:   bool,
+    has_acls:               bool,
+    has_lb_vip:             bool,
+    has_dns_records:        bool,
+    has_unknown_ports:      bool,
+    localnet_ports:         Vec<(uuid, string)>,  // UUID and name of each localnet port.
+    subnet:                 Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
+    ipv6_prefix:            Option<in6_addr>,
+    mcast_cfg:              Intern<McastSwitchCfg>,
+    is_vlan_transparent:    bool,
 
     /* Does this switch have at least one port with type != "router"? */
     has_non_router_port: bool
@@ -267,8 +291,9 @@ Switch[Switch{
            .other_config      = ls.other_config,
            .external_ids      = ls.external_ids,
 
-           .has_stateful_acl  = has_stateful_acl,
-           .has_stateless_acl = has_stateless_acl,
+           .has_stateful_acl       = has_stateful_acl,
+           .has_stateless_from_acl = has_stateless_from_acl,
+           .has_stateless_to_acl   = has_stateless_to_acl,
            .has_acls          = has_acls,
            .has_lb_vip        = has_lb_vip,
            .has_dns_records   = has_dns_records,
@@ -282,7 +307,8 @@ Switch[Switch{
        }.intern()] :-
     nb::Logical_Switch[ls],
     LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
-    LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl),
+    LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl),
+    LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_acl),
     LogicalSwitchHasACLs(ls._uuid, has_acls),
     LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
     LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 54f75d094..bd344ccf4 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -633,6 +633,8 @@ struct ovn_datapath {
     uint32_t port_key_hint;
 
     bool has_stateful_acl;
+    bool has_stateless_from;
+    bool has_stateless_to;
     bool has_lb_vip;
     bool has_unknown;
     bool has_acls;
@@ -4765,19 +4767,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs)
     hmap_destroy(nb_pgs);
 }
 
+static bool
+ls_get_acl_flags_for_acl(struct ovn_datapath *od,
+                         const struct nbrec_acl *acl)
+{
+    if (!strcmp(acl->action, "allow-related")) {
+        od->has_stateful_acl = true;
+        if (od->has_stateless_to && od->has_stateless_from) {
+            return true;
+        }
+    }
+    if (!strcmp(acl->action, "allow-stateless")) {
+        if (!strcmp(acl->direction, "from-lport")) {
+            od->has_stateless_from = true;
+            if (od->has_stateful_acl && od->has_stateless_to) {
+                return true;
+            }
+        } else {
+            od->has_stateless_to = true;
+            if (od->has_stateful_acl && od->has_stateless_from) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
 static void
 ls_get_acl_flags(struct ovn_datapath *od)
 {
     od->has_acls = false;
     od->has_stateful_acl = false;
+    od->has_stateless_from = false;
+    od->has_stateless_to = false;
 
     if (od->nbs->n_acls) {
         od->has_acls = true;
 
         for (size_t i = 0; i < od->nbs->n_acls; i++) {
             struct nbrec_acl *acl = od->nbs->acls[i];
-            if (!strcmp(acl->action, "allow-related")) {
-                od->has_stateful_acl = true;
+            if (ls_get_acl_flags_for_acl(od, acl)) {
                 return;
             }
         }
@@ -4790,8 +4819,7 @@ ls_get_acl_flags(struct ovn_datapath *od)
 
             for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) {
                 struct nbrec_acl *acl = ls_pg->nb_pg->acls[i];
-                if (!strcmp(acl->action, "allow-related")) {
-                    od->has_stateful_acl = true;
+                if (ls_get_acl_flags_for_acl(od, acl)) {
                     return;
                 }
             }
@@ -4990,17 +5018,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op,
 }
 
 static bool
-apply_to_each_acl_of_action(struct ovn_datapath *od,
-                            const struct hmap *port_groups,
-                            struct hmap *lflows, const char *action,
-                            void (*func)(struct ovn_datapath *,
-                                         const struct nbrec_acl *,
-                                         struct hmap *))
+apply_to_each_acl_of_action_and_direction(
+        struct ovn_datapath *od,
+        const struct hmap *port_groups,
+        struct hmap *lflows,
+        const char *action, const char *direction,
+        void (*func)(struct ovn_datapath *,
+                     const struct nbrec_acl *,
+                     struct hmap *))
 {
     bool applied = false;
     for (size_t i = 0; i < od->nbs->n_acls; i++) {
         const struct nbrec_acl *acl = od->nbs->acls[i];
-        if (!strcmp(acl->action, action)) {
+        if (!strcmp(acl->action, action) &&
+                (!direction || !strcmp(acl->direction, direction))) {
             func(od, acl, lflows);
             applied = true;
         }
@@ -5011,7 +5042,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od,
         if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
             for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
                 const struct nbrec_acl *acl = pg->nb_pg->acls[i];
-                if (!strcmp(acl->action, action)) {
+                if (!strcmp(acl->action, action) &&
+                        (!direction || !strcmp(acl->direction, direction))) {
                     func(od, acl, lflows);
                     applied = true;
                 }
@@ -5046,8 +5078,9 @@ build_stateless_filters(struct ovn_datapath *od,
                         const struct hmap *port_groups,
                         struct hmap *lflows)
 {
-    return apply_to_each_acl_of_action(
-        od, port_groups, lflows, "allow-stateless", build_stateless_filter);
+    return apply_to_each_acl_of_action_and_direction(
+        od, port_groups, lflows, "allow-stateless", NULL,
+        build_stateless_filter);
 }
 
 static void
@@ -5074,17 +5107,33 @@ build_stateful_override_filter(struct ovn_datapath *od,
     ds_destroy(&match);
 }
 
+static void
+build_stateful_override_filters_for_direction(struct ovn_datapath *od,
+                                              const struct hmap *port_groups,
+                                              struct hmap *lflows,
+                                              const char *direction)
+{
+    apply_to_each_acl_of_action_and_direction(
+        od, port_groups, lflows, "allow-related", direction,
+        build_stateful_override_filter);
+    apply_to_each_acl_of_action_and_direction(
+        od, port_groups, lflows, "allow", direction,
+        build_stateful_override_filter);
+}
+
 static void
 build_stateful_override_filters(struct ovn_datapath *od,
                                 const struct hmap *port_groups,
                                 struct hmap *lflows)
 {
-    apply_to_each_acl_of_action(
-        od, port_groups, lflows, "allow-related",
-        build_stateful_override_filter);
-    apply_to_each_acl_of_action(
-        od, port_groups, lflows, "allow",
-        build_stateful_override_filter);
+    if (od->has_stateless_from) {
+        build_stateful_override_filters_for_direction(
+            od, port_groups, lflows, "from-lport");
+    }
+    if (od->has_stateless_to) {
+        build_stateful_override_filters_for_direction(
+            od, port_groups, lflows, "to-lport");
+    }
 }
 
 static void
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3b6396e5e..1e4b3de98 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -1861,23 +1861,21 @@ for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl)) {
             }
         }
     };
-    if (sw.has_stateless_acl) {
-        if ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") {
-            if (acl.direction == "from-lport") {
-                Flow(.logical_datapath = ls_uuid,
-                     .stage            = s_SWITCH_IN_PRE_ACL(),
-                     .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
-                     .__match          = "ip && ${acl.__match}",
-                     .actions          = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;",
-                     .external_ids     = stage_hint(acl._uuid))
-            } else {
-                Flow(.logical_datapath = ls_uuid,
-                     .stage            = s_SWITCH_OUT_PRE_ACL(),
-                     .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
-                     .__match          = "ip && ${acl.__match}",
-                     .actions          = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;",
-                     .external_ids     = stage_hint(acl._uuid))
-            }
+    if ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") {
+        if (sw.has_stateless_from_acl and acl.direction == "from-lport") {
+            Flow(.logical_datapath = ls_uuid,
+                 .stage            = s_SWITCH_IN_PRE_ACL(),
+                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
+                 .__match          = "ip && ${acl.__match}",
+                 .actions          = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;",
+                 .external_ids     = stage_hint(acl._uuid))
+        } else if (sw.has_stateless_to_acl and acl.direction == "to-lport") {
+            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 bda11fe06..e80d600c7 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2674,6 +2674,78 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | sort], [0],
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related of different direction])
+ovn_start
+
+# Create two switches to validate direction. We can't use the same switch for
+# both ports, otherwise both in and out pipelines are triggered.
+ovn-nbctl lr-add r0
+for i in $(seq 1 2); do
+    check ovn-nbctl --wait=sb ls-add sw$i
+
+    check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24
+    check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0
+    check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router
+    check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i
+    check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router
+
+    check ovn-nbctl --wait=sb lsp-add sw$i lsp$i
+    check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24"
+done
+
+ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless
+ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related
+ovn-nbctl --wait=sb sync
+
+flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01'
+flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100'
+flow_tcp='tcp && tcp.dst == 80'
+flow_udp='udp && udp.dst == 80'
+lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
+
+# TCP packets should not go to conntrack because allow-related direction is different.
+flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
+AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl
+# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+ip.ttl--;
+eth.src = f0:00:00:00:00:02;
+eth.dst = fe:00:00:00:00:02;
+output("lsp2");
+])
+
+# Add allow-related with the same direction.
+ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related
+ovn-nbctl --wait=sb sync
+
+# TCP packets should now go to conntrack.
+AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl
+# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+ct_next(ct_state=new|trk) {
+    ip.ttl--;
+    eth.src = f0:00:00:00:00:02;
+    eth.dst = fe:00:00:00:00:02;
+    output("lsp2");
+};
+])
+
+# Reduce priority for allow-related rule of the same direction.
+ovn-nbctl acl-del sw1 from-lport 4 tcp
+ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related
+ovn-nbctl --wait=sb sync
+
+# TCP packets should no longer go to conntrack
+AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl
+# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+ip.ttl--;
+eth.src = f0:00:00:00:00:02;
+eth.dst = fe:00:00:00:00:02;
+output("lsp2");
+])
+
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related])
 ovn_start
-- 
2.31.1



More information about the dev mailing list