On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrac...@redhat.com> wrote:
>
> 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")
Is this patch a "fix" or an "optimization"?
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> ---
> northd/lswitch.dl | 88 ++++++++++++++++++++++++++++---------------
> northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++----------
> northd/ovn_northd.dl | 32 ++++++++--------
> tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++
> 4 files changed, 213 insertions(+), 68 deletions(-)
>
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index b73cfd047..8a4c9154c 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -135,16 +135,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:
> @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> /* Switch relation collects all attributes of a logical switch */
>
> relation &Switch(
> - ls: nb::Logical_Switch,
> - 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: Ref<McastSwitchCfg>,
> - is_vlan_transparent: bool,
> + ls: nb::Logical_Switch,
> + 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: Ref<McastSwitchCfg>,
> + is_vlan_transparent: bool,
>
> /* Does this switch have at least one port with type != "router"? */
> has_non_router_port: bool
> @@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string):
Option<in6_addr> {
> }
> }
>
> -&Switch(.ls = ls,
> - .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,
> - .has_unknown_ports = has_unknown_ports,
> - .localnet_ports = localnet_ports,
> - .subnet = subnet,
> - .ipv6_prefix = ipv6_prefix,
> - .mcast_cfg = mcast_cfg,
> - .has_non_router_port = has_non_router_port,
> - .is_vlan_transparent = is_vlan_transparent) :-
> +&Switch(.ls = ls,
> + .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,
> + .has_unknown_ports = has_unknown_ports,
> + .localnet_ports = localnet_ports,
> + .subnet = subnet,
> + .ipv6_prefix = ipv6_prefix,
> + .mcast_cfg = mcast_cfg,
> + .has_non_router_port = has_non_router_port,
> + .is_vlan_transparent = is_vlan_transparent) :-
> 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 a410be1de..1e027eab2 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -627,6 +627,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;
> @@ -4759,19 +4761,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;
> }
> }
> @@ -4784,8 +4813,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;
> }
> }
> @@ -4984,17 +5012,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;
> }
> @@ -5005,7 +5036,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;
> }
> @@ -5040,8 +5072,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
> @@ -5068,17 +5101,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 fc703f62e..8cbc959f0 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1845,23 +1845,21 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl
= &acl, .has_fair_meter = _)) {
> }
>
> for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter
= _)) {
> - 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) {
Should this be: 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 6c38e1a97..1c55310b2 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2664,6 +2664,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.
This test case makes me wonder if the idea of this patch (as an
optimization) is not correct. What if the packet is a return packet of a
request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp
allow-related"? In that case the packet does need to go to CT, otherwise
the CT state will never be "established". I also had the impression that
the priorities for different directions are independent, but it seems we
will have to consider them together for the stateless + stateful scenario.
Any thoughts on this?
Thanks,
Han
> +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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
