On 9/24/20 4:29 AM, Han Zhou wrote: > > > On Thu, Sep 10, 2020 at 2:37 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >> CC: Han Zhou <[email protected] <mailto:[email protected]>> >> CC: Mark Michelson <[email protected] <mailto:[email protected]>> >> CC: Numan Siddique <[email protected] <mailto:[email protected]>> >> Reported-at: https://bugzilla.redhat.com/1871931 >> Acked-by: Mark Michelson <[email protected] > <mailto:[email protected]>> >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> >> --- >> 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 <http://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; >> +} > > I think we should be more strict here. The only situation that is not > considered as conflict, i.e. flows can be combined, is when both flows > have only conjunction actions. If any of the flows has any actions other > than conjunction, we should consider them conflicting. > >> + >> +/* 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); > > Here I thought about another problem. Consider the following scenario: > 1. flow1 with match m1 and action "drop" added > 2. flow2 with match m1 and action "conjunction(...)" added because it is > considered as conflict with flow1 > 3. Now a flow3 with match m1 and action "conjunction(...)" comes, the > above lookup may find flow1 or flow2, but not both. If it finds flow1, > it will add flow3 as a separate flow. If it finds flow2, it will append > and combine with flow2, which is a random behavior. > > The desired behavior in this case would be to combine flow3 with flow2, > because when flow1 is removed by the user, we expect the two conjunction > flows work as expected. So, in this case we cannot rely on > desired_flow_lookup(), but instead we should do a loop using > HMAP_FOR_EACH_WITH_HASH to find all desired flows with the match, and > there should at most one flow that is not conflict with the new flow, > and in that case we should combine with that flow.
Hi Han, You're right, this is the correct way to fix the issue. I sent a v4 implementing this: http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Most of the changes in ofctrl_add_or_append_flow() are not needed anymore because we now just make sure that we append to conjunctive flows. Thanks, Dumitru > >> 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. > > This comment needs to be updated, because we are keeping both flows. > >> + */ >> + 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; >> + } >> + > The logic doesn't look right here. If there is a conflict, we should add > the flow to match_flow_table regardless of which one is more > restrictive, because we want one of them to get installed when the other > one is removed. > >> 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); >> + } >> 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 <http://ovn.at> b/tests/ovn.at <http://ovn.at> >> index 4e58722..118ed48 100644 >> --- a/tests/ovn.at <http://ovn.at> >> +++ b/tests/ovn.at <http://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 <http://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 <http://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 <http://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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
