On 5/6/21 3:52 PM, [email protected] wrote:
> From: Numan Siddique <[email protected]>
>
> 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 <[email protected]>
> ---
Hi Numan,
This needs a rebase after the recent commits to master.
> northd/lswitch.dl | 12 ++++
> northd/ovn-northd.8.xml | 32 ++++++----
> northd/ovn-northd.c | 62 ++++++++++++++-----
> northd/ovn_northd.dl | 62 +++++++++++--------
> tests/ovn-northd.at | 51 ++++++++++++----
> tests/system-ovn.at | 130 ++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 284 insertions(+), 65 deletions(-)
>
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index 47c497e0cf..9bcfe9c321 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 54e88d3fac..90a1f7d0b3 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -545,6 +545,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.
> @@ -585,9 +593,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>
> @@ -633,9 +638,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>
>
> @@ -649,7 +659,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.
> @@ -662,19 +672,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
> @@ -682,7 +692,7 @@
> </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>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 94fae5648a..dfe4225bb3 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -627,6 +627,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;
> @@ -4783,6 +4784,23 @@ ls_has_stateful_acl(struct ovn_datapath *od)
> return false;
> }
>
> +static bool
> +ls_has_acls(struct ovn_datapath *od)
> +{
> + if (od->nbs->n_acls) {
> + return true;
> + }
> +
> + struct ovn_ls_port_group *ls_pg;
> + HMAP_FOR_EACH (ls_pg, key_node, &od->nb_pgs) {
> + if (ls_pg->nb_pg->n_acls) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
nit: ls_has_stateful_acl() and ls_has_acl() both walk the port groups
associated to a logical switch. I wonder if it makes sense to combine
the functions in one that sets both "has_acls" and "has_stateful_acl" in
one go.
> +
> /* Logical switch ingress table 0: Ingress port security - L2
> * (priority 50).
> * Ingress table 1: Ingress port security - IP (priority 90 and 80)
> @@ -5237,7 +5255,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;
I didn't test this it out thoroughly but isn't it enough to change this
whole block to:
if (!od->has_stateful_acl && !od->has_lb_vip) {
ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;");
continue;
} else {
ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
}
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
