| Message ID | 20210506135229.3284757-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] northd: Support flow offloading for logical switches with no ACLs. | expand |
On 5/6/21 3:52 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > 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 <numans@ovn.org> > --- 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
On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/6/21 3:52 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > 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 <numans@ovn.org> > > --- > > Hi Numan, > > This needs a rebase after the recent commits to master. Thanks for the review. Sure I'll rebase and submit v2. > > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, May 7, 2021 at 10:35 AM Numan Siddique <numans@ovn.org> wrote: > > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > On 5/6/21 3:52 PM, numans@ovn.org wrote: > > > From: Numan Siddique <numans@ovn.org> > > > > > > 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 <numans@ovn.org> > > > --- > > > > 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/20210507144258.510878-1-numans@ovn.org/ 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 5/7/21 4:35 PM, Numan Siddique wrote: > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 5/6/21 3:52 PM, numans@ovn.org wrote: >>> From: Numan Siddique <numans@ovn.org> >>> >>> 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 <numans@ovn.org> >>> --- >> >> Hi Numan, >> >> This needs a rebase after the recent commits to master. > > Thanks for the review. Sure I'll rebase and submit v2. > >> >>> 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. > But if the switch has no stateful ACL or load balancer we might as well skip adding the flows that set CT hints, because they will not be used, I think. Regards, Dumitru
On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/7/21 4:35 PM, Numan Siddique wrote: > > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 5/6/21 3:52 PM, numans@ovn.org wrote: > >>> From: Numan Siddique <numans@ovn.org> > >>> > >>> 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 <numans@ovn.org> > >>> --- > >> > >> Hi Numan, > >> > >> This needs a rebase after the recent commits to master. > > > > Thanks for the review. Sure I'll rebase and submit v2. > > > >> > >>> 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. > > > > But if the switch has no stateful ACL or load balancer we might as well > skip adding the flows that set CT hints, because they will not be used, > I think. Yes. I agree. We do the same with the current master and this patch too doesn't change that behavior. This patch adds a 65535 flow to advance the pkt to the next stage in ls_in_acl_hint if there are no ACLs at all. If there is at least one ACL (be it allow, drop or allow-related) associated with the logical switch, priority-0 flow will be added to advance the pkt to the next stage. So you want a 65535 flow to advance the pkt to next stage if there are no stateful ACLs ? Please note that in the stage - "ls_in_acl" we cannot add 65535-priority flow to advance the pkt to next stage if there are stateless ACLs associated with logical switch (be it allow or drop). So I thought to be consistent for both the stages - ls_in_acl_hint and ls_in_acl. i.e If there are NO acls associated with a logical switch, then the priority of the flow to advance the pkt to next stage will be 65535, else it will be 0. Please let me know if I've missed anything or if I misunderstood you ? Thanks Numan > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 5/10/21 11:58 AM, Numan Siddique wrote: > On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 5/7/21 4:35 PM, Numan Siddique wrote: >>> On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> On 5/6/21 3:52 PM, numans@ovn.org wrote: >>>>> From: Numan Siddique <numans@ovn.org> >>>>> >>>>> 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 <numans@ovn.org> >>>>> --- >>>> >>>> Hi Numan, >>>> >>>> This needs a rebase after the recent commits to master. >>> >>> Thanks for the review. Sure I'll rebase and submit v2. >>> >>>> >>>>> 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. >>> >> >> But if the switch has no stateful ACL or load balancer we might as well >> skip adding the flows that set CT hints, because they will not be used, >> I think. > > Yes. I agree. We do the same with the current master and this patch > too doesn't change > that behavior. > > This patch adds a 65535 flow to advance the pkt to the next stage in > ls_in_acl_hint if there are > no ACLs at all. If there is at least one ACL (be it allow, drop or > allow-related) associated with the logical switch, > priority-0 flow will be added to advance the pkt to the next stage. > > So you want a 65535 flow to advance the pkt to next stage if there are > no stateful ACLs ? Yes, that was what I was thinking. > > Please note that in the stage - "ls_in_acl" we cannot add > 65535-priority flow to advance the pkt to next stage > if there are stateless ACLs associated with logical switch (be it > allow or drop). I see now. > > So I thought to be consistent for both the stages - ls_in_acl_hint and > ls_in_acl. > > i.e If there are NO acls associated with a logical switch, then the > priority of the flow to advance the pkt to next stage > will be 65535, else it will be 0. > > Please let me know if I've missed anything or if I misunderstood you ? I'm worried of the case when: LS1-ACLs: - <match1>, prio X, then "allow-related" LS2-ACLs: - <match2>, prio Y, then "allow" Even though the two logical switches are independent and the ACLs defined on LS2 are "stateless" we'll still not be able to offload the flows, right? What if reduce the maximum number of allowed ACL priorities? It's currently (UINT16_MAX - 1000). Would it make sense to split it in half and when a logical switch has only "allow" ACLs use the top half, otherwise use the bottom half? That would ensure that flows for ACLs defined on LS2 always have higher priority than flows defined for ACLs on LS1 (which match on ct_state). > > Thanks > Numan > Thanks, Dumitru
On Mon, May 10, 2021 at 9:29 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/10/21 11:58 AM, Numan Siddique wrote: > > On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 5/7/21 4:35 PM, Numan Siddique wrote: > >>> On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: > >>>> > >>>> On 5/6/21 3:52 PM, numans@ovn.org wrote: > >>>>> From: Numan Siddique <numans@ovn.org> > >>>>> > >>>>> 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 <numans@ovn.org> > >>>>> --- > >>>> > >>>> Hi Numan, > >>>> > >>>> This needs a rebase after the recent commits to master. > >>> > >>> Thanks for the review. Sure I'll rebase and submit v2. > >>> > >>>> > >>>>> 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. > >>> > >> > >> But if the switch has no stateful ACL or load balancer we might as well > >> skip adding the flows that set CT hints, because they will not be used, > >> I think. > > > > Yes. I agree. We do the same with the current master and this patch > > too doesn't change > > that behavior. > > > > This patch adds a 65535 flow to advance the pkt to the next stage in > > ls_in_acl_hint if there are > > no ACLs at all. If there is at least one ACL (be it allow, drop or > > allow-related) associated with the logical switch, > > priority-0 flow will be added to advance the pkt to the next stage. > > > > So you want a 65535 flow to advance the pkt to next stage if there are > > no stateful ACLs ? > > Yes, that was what I was thinking. > > > > > Please note that in the stage - "ls_in_acl" we cannot add > > 65535-priority flow to advance the pkt to next stage > > if there are stateless ACLs associated with logical switch (be it > > allow or drop). > > I see now. > > > > > So I thought to be consistent for both the stages - ls_in_acl_hint and > > ls_in_acl. > > > > i.e If there are NO acls associated with a logical switch, then the > > priority of the flow to advance the pkt to next stage > > will be 65535, else it will be 0. > > > > Please let me know if I've missed anything or if I misunderstood you ? > > I'm worried of the case when: > > LS1-ACLs: > - <match1>, prio X, then "allow-related" > > LS2-ACLs: > - <match2>, prio Y, then "allow" > > Even though the two logical switches are independent and the ACLs > defined on LS2 are "stateless" we'll still not be able to offload the > flows, right? > > What if reduce the maximum number of allowed ACL priorities? It's > currently (UINT16_MAX - 1000). Would it make sense to split it in half > and when a logical switch has only "allow" ACLs use the top half, > otherwise use the bottom half? > > That would ensure that flows for ACLs defined on LS2 always have higher > priority than flows defined for ACLs on LS1 (which match on ct_state). > This is an interesting idea. Thanks for the suggestion. Until now all the flows would match on ct_state even if one binding in the chassis belongs to a logical switch configured with ACLs. I think we can probably divide this in 2 stages. 1. What this patch does. 2. What you suggested. If that sounds good, I will work on your suggestion as a follow up patch ? I think having (1) would still help. I don't see too much of work for (2), but I might take some time to address it. Let me know if it works for you ? Thanks Numan > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 5/10/21 4:44 PM, Numan Siddique wrote: > On Mon, May 10, 2021 at 9:29 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 5/10/21 11:58 AM, Numan Siddique wrote: >>> On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> On 5/7/21 4:35 PM, Numan Siddique wrote: >>>>> On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>>>> >>>>>> On 5/6/21 3:52 PM, numans@ovn.org wrote: >>>>>>> From: Numan Siddique <numans@ovn.org> >>>>>>> >>>>>>> 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 <numans@ovn.org> >>>>>>> --- >>>>>> >>>>>> Hi Numan, >>>>>> >>>>>> This needs a rebase after the recent commits to master. >>>>> >>>>> Thanks for the review. Sure I'll rebase and submit v2. >>>>> >>>>>> >>>>>>> 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. >>>>> >>>> >>>> But if the switch has no stateful ACL or load balancer we might as well >>>> skip adding the flows that set CT hints, because they will not be used, >>>> I think. >>> >>> Yes. I agree. We do the same with the current master and this patch >>> too doesn't change >>> that behavior. >>> >>> This patch adds a 65535 flow to advance the pkt to the next stage in >>> ls_in_acl_hint if there are >>> no ACLs at all. If there is at least one ACL (be it allow, drop or >>> allow-related) associated with the logical switch, >>> priority-0 flow will be added to advance the pkt to the next stage. >>> >>> So you want a 65535 flow to advance the pkt to next stage if there are >>> no stateful ACLs ? >> >> Yes, that was what I was thinking. >> >>> >>> Please note that in the stage - "ls_in_acl" we cannot add >>> 65535-priority flow to advance the pkt to next stage >>> if there are stateless ACLs associated with logical switch (be it >>> allow or drop). >> >> I see now. >> >>> >>> So I thought to be consistent for both the stages - ls_in_acl_hint and >>> ls_in_acl. >>> >>> i.e If there are NO acls associated with a logical switch, then the >>> priority of the flow to advance the pkt to next stage >>> will be 65535, else it will be 0. >>> >>> Please let me know if I've missed anything or if I misunderstood you ? >> >> I'm worried of the case when: >> >> LS1-ACLs: >> - <match1>, prio X, then "allow-related" >> >> LS2-ACLs: >> - <match2>, prio Y, then "allow" >> >> Even though the two logical switches are independent and the ACLs >> defined on LS2 are "stateless" we'll still not be able to offload the >> flows, right? >> >> What if reduce the maximum number of allowed ACL priorities? It's >> currently (UINT16_MAX - 1000). Would it make sense to split it in half >> and when a logical switch has only "allow" ACLs use the top half, >> otherwise use the bottom half? >> >> That would ensure that flows for ACLs defined on LS2 always have higher >> priority than flows defined for ACLs on LS1 (which match on ct_state). >> > > This is an interesting idea. Thanks for the suggestion. Until now all > the flows would match on ct_state even if one binding in the chassis > belongs to a logical switch configured with ACLs. > > I think we can probably divide this in 2 stages. > 1. What this patch does. > 2. What you suggested. > > If that sounds good, I will work on your suggestion as a follow up patch ? > > I think having (1) would still help. I don't see too much of work for > (2), but I might > take some time to address it. > > Let me know if it works for you ? Sure, sounds good to me. I'll have a look at the v2 of this patch soon. Regards, Dumitru
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; +} + /* 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; @@ -5637,10 +5659,19 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, bool has_stateful = od->has_stateful_acl || od->has_lb_vip; /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by - * default. A related rule at priority 1 is added below if there + * default. If the logical switch has no ACLs or no load balancers, + * then add 65535-priority flow to advance the packet to next + * stage. + * + * A related rule at priority 1 is added below if there * are any stateful ACLs in this datapath. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1", "next;"); + if (!od->has_acls && !od->has_lb_vip) { + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "1", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "1", "next;"); + } else { + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1", "next;"); + } if (has_stateful) { /* Ingress and Egress ACL Table (Priority 1). @@ -5671,21 +5702,21 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, "ip && (!ct.est || (ct.est && ct_label.blocked == 1))", REGBIT_CONNTRACK_COMMIT" = 1; next;"); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Always drop traffic that's in an invalid state. Also drop * reply direction packets for connections that have been marked * for deletion (bit 0 of ct_label is set). * * This is enforced at a higher priority than ACLs can be defined. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", "drop;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", "drop;"); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65535 - 3). * * Allow reply traffic that is part of an established * conntrack entry that has not been marked for deletion @@ -5694,11 +5725,11 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, * direction to hit the currently defined policy from ACLs. * * This is enforced at a higher priority than ACLs can be defined. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, "ct.est && !ct.rel && !ct.new && !ct.inv " "&& ct.rpl && ct_label.blocked == 0", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, "ct.est && !ct.rel && !ct.new && !ct.inv " "&& ct.rpl && ct_label.blocked == 0", "next;"); @@ -5714,21 +5745,21 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, * a dynamically negotiated FTP data channel), but will allow * related traffic such as an ICMP Port Unreachable through * that's generated from a non-listening UDP port. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, "!ct.est && ct.rel && !ct.new && !ct.inv " "&& ct_label.blocked == 0", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, "!ct.est && ct.rel && !ct.new && !ct.inv " "&& ct_label.blocked == 0", "next;"); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Not to do conntrack on ND packets. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); } @@ -6827,6 +6858,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, if (od->nbs) { od->has_stateful_acl = ls_has_stateful_acl(od); od->has_lb_vip = ls_has_lb_vip(od); + od->has_acls = ls_has_acls(od); build_pre_acls(od, lflows); build_pre_lb(od, lflows, meter_groups, lbs); diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 7953325aa3..6cc38d1bc3 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -2242,20 +2242,29 @@ for (sw in &Switch(.ls = ls)) var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in { /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by - * default. A related rule at priority 1 is added below if there + * default. If the logical switch has no ACLs or no load balancers, + * then add 65535-priority flow to advance the packet to next + * stage. + * + * A related rule at priority 1 is added below if there * are any stateful ACLs in this datapath. */ - Flow(.logical_datapath = ls._uuid, - .stage = s_SWITCH_IN_ACL(), - .priority = 0, - .__match = "1", - .actions = "next;", - .external_ids = map_empty()); - Flow(.logical_datapath = ls._uuid, - .stage = s_SWITCH_OUT_ACL(), - .priority = 0, - .__match = "1", - .actions = "next;", - .external_ids = map_empty()); + var priority = if (not sw.has_acls and not sw.has_lb_vip) { 65535 } else { 0 } + in + { + Flow(.logical_datapath = ls._uuid, + .stage = s_SWITCH_IN_ACL(), + .priority = priority, + .__match = "1", + .actions = "next;", + .external_ids = map_empty()); + Flow(.logical_datapath = ls._uuid, + .stage = s_SWITCH_OUT_ACL(), + .priority = priority, + .__match = "1", + .actions = "next;", + .external_ids = map_empty()) + }; + if (has_stateful) { /* Ingress and Egress ACL Table (Priority 1). @@ -2292,7 +2301,7 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; next;", .external_ids = map_empty()); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Always drop traffic that's in an invalid state. Also drop * reply direction packets for connections that have been marked @@ -2301,18 +2310,18 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in * This is enforced at a higher priority than ACLs can be defined. */ Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_IN_ACL(), - .priority = 65535, + .priority = 65532, .__match = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", .actions = "drop;", .external_ids = map_empty()); Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_OUT_ACL(), - .priority = 65535, + .priority = 65532, .__match = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", .actions = "drop;", .external_ids = map_empty()); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Allow reply traffic that is part of an established * conntrack entry that has not been marked for deletion @@ -2323,20 +2332,20 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in * This is enforced at a higher priority than ACLs can be defined. */ Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_IN_ACL(), - .priority = 65535, + .priority = 65532, .__match = "ct.est && !ct.rel && !ct.new && !ct.inv " "&& ct.rpl && ct_label.blocked == 0", .actions = "next;", .external_ids = map_empty()); Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_OUT_ACL(), - .priority = 65535, + .priority = 65532, .__match = "ct.est && !ct.rel && !ct.new && !ct.inv " "&& ct.rpl && ct_label.blocked == 0", .actions = "next;", .external_ids = map_empty()); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Allow traffic that is related to an existing conntrack entry that * has not been marked for deletion (bit 0 of ct_label). @@ -2349,31 +2358,31 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in * that's generated from a non-listening UDP port. */ Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_IN_ACL(), - .priority = 65535, + .priority = 65532, .__match = "!ct.est && ct.rel && !ct.new && !ct.inv " "&& ct_label.blocked == 0", .actions = "next;", .external_ids = map_empty()); Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_OUT_ACL(), - .priority = 65535, + .priority = 65532, .__match = "!ct.est && ct.rel && !ct.new && !ct.inv " "&& ct_label.blocked == 0", .actions = "next;", .external_ids = map_empty()); - /* Ingress and Egress ACL Table (Priority 65535). + /* Ingress and Egress ACL Table (Priority 65532). * * Not to do conntrack on ND packets. */ Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_IN_ACL(), - .priority = 65535, + .priority = 65532, .__match = "nd || nd_ra || nd_rs || mldv1 || mldv2", .actions = "next;", .external_ids = map_empty()); Flow(.logical_datapath = ls._uuid, .stage = s_SWITCH_OUT_ACL(), - .priority = 65535, + .priority = 65532, .__match = "nd || nd_ra || nd_rs || mldv1 || mldv2", .actions = "next;", .external_ids = map_empty()) @@ -2425,7 +2434,8 @@ AclHintStages[s_SWITCH_OUT_ACL_HINT()]. for (sw in &Switch(.ls = ls)) { for (AclHintStages[stage]) { /* In any case, advance to the next stage. */ - Flow(ls._uuid, stage, 0, "1", "next;", map_empty()) + var priority = if (not sw.has_acls and not sw.has_lb_vip) { 65535 } else { 0 } in + Flow(ls._uuid, stage, priority, "1", "next;", map_empty()) }; for (AclHintStages[stage]) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 32afb4fa89..098d4ee7ec 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2101,9 +2101,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e table=4 (ls_out_acl_hint ), priority=6 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) table=4 (ls_out_acl_hint ), priority=7 , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) table=5 (ls_out_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) - table=5 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=5 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=5 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=5 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=5 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=5 (ls_out_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) 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=2 , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), action=(reg0[[9]] = 1; next;) @@ -2112,9 +2112,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e 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=7 , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; 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=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=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) ]) AS_BOX([Check match ct_state with load balancer]) @@ -2124,7 +2124,8 @@ check ovn-nbctl --wait=sb \ -- lb-add lb "10.0.0.1" "10.0.0.2" \ -- ls-lb-add ls lb -AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl +AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | sort], [0], [dnl + table=4 (ls_out_acl_hint ), priority=0 , match=(1), action=(next;) table=4 (ls_out_acl_hint ), priority=1 , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;) table=4 (ls_out_acl_hint ), priority=2 , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) table=4 (ls_out_acl_hint ), priority=3 , match=(!ct.est), action=(reg0[[9]] = 1; next;) @@ -2132,10 +2133,16 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e table=4 (ls_out_acl_hint ), priority=5 , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;) table=4 (ls_out_acl_hint ), priority=6 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) table=4 (ls_out_acl_hint ), priority=7 , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) + table=5 (ls_out_acl ), priority=0 , match=(1), action=(next;) table=5 (ls_out_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) - table=5 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=5 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=5 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=5 (ls_out_acl ), priority=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;) + table=5 (ls_out_acl ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;) + table=5 (ls_out_acl ), priority=34000, match=(eth.src == $svc_monitor_mac), action=(next;) + table=5 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=5 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=5 (ls_out_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=5 (ls_out_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) + table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(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=2 , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), action=(reg0[[9]] = 1; next;) @@ -2143,12 +2150,30 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e 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=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=7 , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) + table=9 (ls_in_acl ), priority=0 , match=(1), 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=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=1001 , match=(reg0[[7]] == 1 && (ip)), action=(reg0[[1]] = 1; next;) + table=9 (ls_in_acl ), priority=1001 , match=(reg0[[8]] == 1 && (ip)), action=(next;) + table=9 (ls_in_acl ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) +ovn-nbctl --wait=sb clear logical_switch ls acls +ovn-nbctl --wait=sb clear logical_switch ls load_balancer + +AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | sort], [0], [dnl + table=4 (ls_out_acl_hint ), priority=65535, match=(1), action=(next;) + table=5 (ls_out_acl ), priority=34000, match=(eth.src == $svc_monitor_mac), action=(next;) + table=5 (ls_out_acl ), priority=65535, match=(1), action=(next;) + table=8 (ls_in_acl_hint ), priority=65535, match=(1), action=(next;) + table=9 (ls_in_acl ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;) + table=9 (ls_in_acl ), priority=65535, match=(1), action=(next;) +]) + + AT_CLEANUP ]) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index b6c6799079..13b6322e7c 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -5890,3 +5890,133 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /.*terminating with signal 15.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- No ct_state matches in dp flows when no ACLs in an LS]) +AT_KEYWORDS([no ct_state match]) +ovn_start + +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +# Start ovn-controller +start_daemon ovn-controller + +check ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03" + +check ovn-nbctl lsp-add sw0 sw0-p2 +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" + + +# Create the second logical switch with one port and configure some ACLs. +check ovn-nbctl ls-add sw1 +check ovn-nbctl lsp-add sw1 sw1-p1 + +# Create port group and ACLs for sw1 ports. +check ovn-nbctl pg-add pg1 sw1-p1 +check ovn-nbctl acl-add pg1 from-lport 1002 "ip" allow-related +check ovn-nbctl acl-add pg1 to-lport 1002 "ip" allow-related + + +OVN_POPULATE_ARP +ovn-nbctl --wait=hv sync + +ADD_NAMESPACES(sw0-p1) +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ + "10.0.0.1") + + +ADD_NAMESPACES(sw0-p2) +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \ + "10.0.0.1") + +ADD_NAMESPACES(sw1-p1) +ADD_VETH(sw1-p1, sw1-p1, br-int, "20.0.0.4/24", "30:54:00:00:00:04", \ + "20.0.0.1") + +wait_for_ports_up + +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +ovs-appctl dpctl/dump-flows + +# sw1-p1 may send IPv6 traffic. So filter this out. Since sw1-p1 has +# ACLs configured, the datapath flows for the packets from sw1-p1 will have +# matches on ct_state and ct_label fields. +# Since sw0 doesn't have any ACLs, there should be no match on ct fields. +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [1], [dnl +0 +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [1], [dnl +0 +]) + +# Add an ACL to sw0. +check ovn-nbctl --wait=hv acl-add sw0 to-lport 1002 ip allow-related + +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +ovs-appctl dpctl/dump-flows + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [0], [ignore]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [0], [ignore]) + +# Clear ACL for sw0 +check ovn-nbctl --wait=hv clear logical_switch sw0 acls + +check ovs-appctl dpctl/del-flows + +check ovn-nbctl --wait=hv sync + +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +ovs-appctl dpctl/dump-flows + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_state | grep -v ipv6 -c], [1], [dnl +0 +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep ct_label | grep -v ipv6 -c], [1], [dnl +0 +]) + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) +AT_CLEANUP +])