On Fri, Sep 11, 2020 at 3:07 AM Dumitru Ceara <dce...@redhat.com> wrote:
> Until now, in case the ACL configuration generates openflows that have > the same match but different actions, ovn-controller was using the > following approach: > 1. If the flow being added contains conjunctive actions, merge its > actions with the already existing flow. > 2. Otherwise, if the flow is being added incrementally > (update_installed_flows_by_track), don't install the new flow but > instead keep the old one. > 3. Otherwise, (update_installed_flows_by_compare), don't install the > new flow but instead keep the old one. > > Even though one can argue that having an ACL with a match that includes > the match of another ACL is a misconfiguration, it can happen that the > users provision OVN like this. Depending on the order of reading and > installing the logical flows, the above operations can yield > unpredictable results, e.g., allow specific traffic but then after > ovn-controller is restarted (or a recompute happens) that specific > traffic starts being dropped. > > A simple example of ACL configuration is: > ovn-nbctl acl-add ls to-lport 3 '(ip4.src==10.0.0.1 || > ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow > ovn-nbctl acl-add ls to-lport 3 'ip4.src==10.0.0.1' allow > ovn-nbctl acl-add ls to-lport 2 'arp' allow > ovn-nbctl acl-add ls to-lport 1 'ip4' drop > > This is a pattern used by most CMSs: > - define a default deny policy. > - punch holes in the default deny policy based on user specific > configurations. > > Without this commit the behavior for traffic from 10.0.0.1 to 10.0.0.5 > is unpredictable. Depending on the order of operations traffic might be > dropped or allowed. > > It's also quite hard to force the CMS to ensure that such match overlaps > never occur. > > To address this issue we now resolve conflicts between flows with the > same match and different actions by giving precedence to less > restrictive flows. This means that if the installed flow has action > "conjunction" and the desired flow doesn't then we prefer the desired > flow. Similarly, if the desired flow has action "conjunction" and the > installed flow doesn't then we prefer the already installed flow. > > CC: Daniel Alvarez <dalva...@redhat.com> > CC: Han Zhou <hz...@ovn.org> > CC: Mark Michelson <mmich...@redhat.com> > CC: Numan Siddique <num...@ovn.org> > Reported-at: https://bugzilla.redhat.com/1871931 > Acked-by: Mark Michelson <mmich...@redhat.com> > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > --- > Thanks Dumitru for the patch. The added test case fails because of recent changes to the ACL table number. With the below changes, the test case passes. *********************** diff --git a/tests/ovn.at b/tests/ovn.at index 9d8ce3bb2e..11e7dfd160 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13808,12 +13808,12 @@ ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow ovn-nbctl --wait=hv sync # Check OVS flows, the less restrictive flows should have been installed. -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=44 | \ +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl -priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45) +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2) priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2) -priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45) +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) ]) @@ -13849,9 +13849,9 @@ ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1' ovn-nbctl --wait=hv sync # Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled. -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=44 | \ +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl -priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45) +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2) priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2) priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2) ********************** The patch LGTM with one minor comment. Acked-by: Numan Siddique <num...@ovn.org> I will wait for Han if he has any comments before applying this patch. Thanks Numan > v3: > - Add Mark's ack. > - Add last missing pcap check in the test. > v2: > - Address Han's comments: > - Do not delete desired flow that cannot be merged, it might be > installed later. > - Fix typos in the commit log. > - Update the test to check the OVS flows. > --- > controller/ofctrl.c | 124 +++++++++++++++++++++++++++++++++++-- > tests/ovn.at | 174 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 293 insertions(+), 5 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 81a00c8..819e8cf 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table > *flow_table, > ovs_list_insert(&stf->flows, &sfr->flow_list); > } > > +static bool > +flow_action_has_conj(const struct ovn_flow *f) > +{ > + const struct ofpact *a = NULL; > + > + OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) { > + if (a->type == OFPACT_CONJUNCTION) { > + return true; > + } > + } > + return false; > +} > + > +/* Returns true if the actions of 'f1' cannot be merged with the actions > of > + * 'f2'. This is the case when one of the flow contains action > "conjunction" > + * and the other doesn't. The function always populates 'f1_has_conj' and > + * 'f2_has_conj'. > + */ > +static bool > +flow_actions_conflict(const struct ovn_flow *f1, const struct ovn_flow > *f2, > + bool *f1_has_conj, bool *f2_has_conj) > +{ > + *f1_has_conj = flow_action_has_conj(f1); > + *f2_has_conj = flow_action_has_conj(f2); > + > + if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) { > + return true; > + } > + > + if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) { > + return true; > + } > + > + return false; > +} > + > +static void > +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1, > + const struct ovn_flow *f2) > +{ > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + > + char *f1_s = ovn_flow_to_string(f1); > + char *f2_s = ovn_flow_to_string(f2); > + > + VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s, flow-2: %s", > + msg, f1_s, f2_s); > + free(f1_s); > + free(f2_s); > +} > + > /* Flow table interfaces to the rest of ovn-controller. */ > > /* Adds a flow to 'desired_flows' with the specified 'match' and > 'actions' to > @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct > ovn_desired_flow_table *desired_flows, > struct desired_flow *existing; > existing = desired_flow_lookup(desired_flows, &f->flow, NULL); > if (existing) { > - /* There's already a flow with this particular match. Append the > - * action to that flow rather than adding a new flow > + /* There's already a flow with this particular match. Try to > append > + * the action to that flow rather than adding a new flow. > + * > + * If exactly one of the existing or new flow includes a > conjunction > + * action then we can't merge the actions. In that case keep the > flow > + * without conjunction action as this is the least restrictive > one. > + */ > + bool existing_conj = false; > + bool new_conj = false; > + > + if (flow_actions_conflict(&existing->flow, &f->flow, > &existing_conj, > + &new_conj)) { > + flow_log_actions_conflict("Cannot merge conj with non-conj > action", > + &existing->flow, &f->flow); > + } > + > + /* If the existing flow is less restrictive (i.e., no conjunction > + * action) then keep it installed. Store however the flow with > + * conjunction action too as it will be installed when the current > + * one is removed. > */ > + if (!existing_conj && new_conj) { > + hmap_insert(&desired_flows->match_flow_table, > &f->match_hmap_node, > + f->flow.hash); > + link_flow_to_sb(desired_flows, f, sb_uuid); > + return; > + } > + > uint64_t compound_stub[64 / 8]; > struct ofpbuf compound; > ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub)); > - ofpbuf_put(&compound, existing->flow.ofpacts, > - existing->flow.ofpacts_len); > + > + /* Only merge actions if both flows either have action conjunction > + * or non-conjunction. Otherwise use the actions in the new flow, > + * the least restrictive. > + */ > + if (existing_conj == new_conj) { > + ofpbuf_put(&compound, existing->flow.ofpacts, > + existing->flow.ofpacts_len); > + } > In the case where 'existing_conj' is true and 'new_conj' is false, we can actually avoid the compound_stub I'd do something like below <*************************************> static void ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b) { struct ofpact *tmp = a->ofpacts; size_t tmp_len = a->ofpacts_len; a->ofpacts = b->ofpacts; a->ofpacts_len = b->ofpacts_len; b->ofpacts = tmp; b->ofpacts_len = tmp_len; } void ofctrl_add_or_append_flow(.....) { ... ... .. if (!existing_conj && new_conj) { ... ... return; } if (existing_conj && !new_conj) { ofctrl_swap_flow_actions(f, existing); } else { uint64_t compound_stub[64 / 8]; struct ofpbuf compound; ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub)); ofpbuf_put(&compound, existing->flow.ofpacts, existing->flow.ofpacts_len); ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); free(existing->flow.ofpacts); existing->flow.ofpacts = xmemdup(compound.data, compound.size); existing->flow.ofpacts_len = compound.size; ofpbuf_uninit(&compound); } desired_flow_destroy(f); f = existing; ... <**********************************************> But I'm ok with the existing code too. Thanks Numan > ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); > > free(existing->flow.ofpacts); > @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct > ovn_desired_flow_table *flow_table, > /* Copy 'd' from 'flow_table' to installed_flows. */ > i = installed_flow_dup(d); > hmap_insert(&installed_flows, &i->match_hmap_node, > i->flow.hash); > + } else if (i->desired_flow != d) { > + /* If a matching installed flow was found but its actions are > + * conflicting with the desired flow actions, we should chose > + * to install the least restrictive one (i.e., no conjunctive > + * action). > + */ > + bool i_conj = false; > + bool d_conj = false; > + > + if (flow_actions_conflict(&i->flow, &d->flow, &i_conj, > &d_conj) && > + i_conj && !d_conj) { > + flow_log_actions_conflict("Use new flow", &i->flow, > &d->flow); > + installed_flow_mod(&i->flow, &d->flow, msgs); > + ovn_flow_log(&i->flow, "updating installed"); > + } > } > link_installed_to_desired(i, d); > } > @@ -1817,7 +1915,23 @@ update_installed_flows_by_track(struct > ovn_desired_flow_table *flow_table, > installed_flow_mod(&i->flow, &f->flow, msgs); > } else { > /* Adding a new flow that conflicts with an existing > installed > - * flow, so just add it to the link. */ > + * flow, so just add it to the link. > + * > + * However, if the existing installed flow is less > restrictive > + * (i.e., no conjunctive action), we should chose it over > the > + * existing installed flow. > + */ > + bool i_conj = false; > + bool f_conj = false; > + > + if (flow_actions_conflict(&i->flow, &f->flow, &i_conj, > + &f_conj) && > + i_conj && !f_conj) { > + flow_log_actions_conflict("Use new flow", > + &i->flow, &f->flow); > + ovn_flow_log(&i->flow, "updating installed > (tracked)"); > + installed_flow_mod(&i->flow, &f->flow, msgs); > + } > link_installed_to_desired(i, f); > } > /* The track_list_node emptyness is used to check if the node > is > diff --git a/tests/ovn.at b/tests/ovn.at > index 4e58722..118ed48 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -13709,6 +13709,180 @@ grep conjunction.*conjunction.*conjunction | wc > -l`]) > OVN_CLEANUP([hv1]) > AT_CLEANUP > > +AT_SETUP([ovn -- Superseeding ACLs with conjunction]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > + > +ovn-nbctl lsp-add ls1 ls1-lp1 \ > +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" > + > +ovn-nbctl lsp-add ls1 ls1-lp2 \ > +-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02" > + > +net_add n1 > +sim_add hv1 > + > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > + set interface hv1-vif2 external-ids:iface-id=ls1-lp2 \ > + options:tx_pcap=hv1/vif2-tx.pcap \ > + options:rxq_pcap=hv1/vif2-rx.pcap \ > + ofport-request=2 > + > +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... > +# > +# This shell function causes an ip packet to be received on INPORT. > +# The packet's content has Ethernet destination DST and source SRC > +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits). > +# The OUTPORTs (zero or more) list the VIFs on which the packet should > +# be received. INPORT and the OUTPORTs are specified as logical switch > +# port numbers, e.g. 11 for vif11. > +test_ip() { > + # This packet has bad checksums but logical L3 routing doesn't check. > + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 > + local > packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\ > +${dst_ip}0035111100080000 > + shift; shift; shift; shift; shift > + as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > + for outport; do > + echo $packet >> $outport.expected > + done > +} > + > +ip_to_hex() { > + printf "%02x%02x%02x%02x" "$@" > +} > + > +reset_pcap_file() { > + local iface=$1 > + local pcap_file=$2 > + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \ > +options:rxq_pcap=dummy-rx.pcap > + rm -f ${pcap_file}*.pcap > + ovs-vsctl -- set Interface $iface > options:tx_pcap=${pcap_file}-tx.pcap \ > +options:rxq_pcap=${pcap_file}-rx.pcap > +} > + > +# Add a default deny ACL and an allow ACL for specific IP traffic. > +ovn-nbctl acl-add ls1 to-lport 2 'arp' allow > +ovn-nbctl acl-add ls1 to-lport 1 'ip4' drop > +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || > ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow > +ovn-nbctl --wait=hv sync > + > +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. > +for src in `seq 1 2`; do > + for dst in `seq 3 4`; do > + sip=`ip_to_hex 10 0 0 $src` > + dip=`ip_to_hex 10 0 0 $dst` > + > + test_ip 1 f00000000001 f00000000002 $sip $dip 2 > + done > +done > + > +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. > +dip=`ip_to_hex 10 0 0 5` > +for src in `seq 1 2`; do > + sip=`ip_to_hex 10 0 0 $src` > + > + test_ip 1 f00000000001 f00000000002 $sip $dip > +done > + > +cat 2.expected > expout > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets > +AT_CHECK([cat 2.packets], [0], [expout]) > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 2.packets > +> 2.expected > + > +# Add a less restrictive allow ACL for src IP 10.0.0.1 > +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow > +ovn-nbctl --wait=hv sync > + > +# Check OVS flows, the less restrictive flows should have been installed. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=44 | \ > + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl > +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45) > +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2) > +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2) > +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45) > +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) > +]) > + > +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. > +for src in `seq 1 2`; do > + for dst in `seq 3 4`; do > + sip=`ip_to_hex 10 0 0 $src` > + dip=`ip_to_hex 10 0 0 $dst` > + > + test_ip 1 f00000000001 f00000000002 $sip $dip 2 > + done > +done > + > +# Traffic 10.0.0.2 -> 10.0.0.5 should be dropped. > +sip=`ip_to_hex 10 0 0 2` > +dip=`ip_to_hex 10 0 0 5` > +test_ip 1 f00000000001 f00000000002 $sip $dip > + > +# Traffic 10.0.0.1 -> 10.0.0.5 should be allowed. > +sip=`ip_to_hex 10 0 0 1` > +dip=`ip_to_hex 10 0 0 5` > +test_ip 1 f00000000001 f00000000002 $sip $dip 2 > + > +cat 2.expected > expout > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets > +AT_CHECK([cat 2.packets], [0], [expout]) > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 2.packets > +> 2.expected > + > +# Remove the less restrictive allow ACL. > +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1' > +ovn-nbctl --wait=hv sync > + > +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=44 | \ > + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl > +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45) > +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2) > +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2) > +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2) > +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) > +]) > + > +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. > +for src in `seq 1 2`; do > + for dst in `seq 3 4`; do > + sip=`ip_to_hex 10 0 0 $src` > + dip=`ip_to_hex 10 0 0 $dst` > + > + test_ip 1 f00000000001 f00000000002 $sip $dip 2 > + done > +done > + > +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. > +dip=`ip_to_hex 10 0 0 5` > +for src in `seq 1 2`; do > + sip=`ip_to_hex 10 0 0 $src` > + > + test_ip 1 f00000000001 f00000000002 $sip $dip > +done > + > +cat 2.expected > expout > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets > +AT_CHECK([cat 2.packets], [0], [expout]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > + > # 3 hypervisors, one logical switch, 3 logical ports per hypervisor > AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL]) > ovn_start > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev