On Tue, Sep 22, 2020 at 5:40 AM Numan Siddique <num...@ovn.org> wrote:
> > > 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. > > I will take another look at this tomorrow. Thanks, Han > 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
