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 <[email protected]>
===
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
