On Fri, May 7, 2021 at 10:35 AM Numan Siddique <[email protected]> wrote: > > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <[email protected]> wrote: > > > > 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. > > Thanks for the review. Sure I'll rebase and submit v2.
v2 submitted - https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Request to take a look. Thanks Numan > > > > > > 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. > > > > Ack. Sounds good. > > > > + > > > /* 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;"); > > } > > Not really. > > If a logical switch has no ACLs or no LB VIPs we want to add > 65535-prio flow to advance the > packet to the next stage. But if a logical switch has any ACL (be it > allow, allow-related or drop) > we want to add a normal 0-priority flow to advance the packet to the next > stage. > > Thanks > Numan > > > > > Regards, > > Dumitru > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
