| Message ID | 1599743157-13626-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofctrl: Add a predictable resolution for conflicting flow actions. | expand |
On Thu, Sep 10, 2020 at 6:06 AM Dumitru Ceara <dceara@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 perform such configuration. 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 follows 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 > desired flow doesn't then we prefer the already installed flow. > Here is a typo. I think the last "desired" should be "installed". The idea looks good to me. So it attempts to ensure the behavior is consistent and predictable, although it can't tell what's the real intention of the user. This makes sense. However, the implementation seems to have a problem. Please see my comment below. > CC: Daniel Alvarez <dalvarez@redhat.com> > CC: Han Zhou <hzhou@ovn.org> > CC: Mark Michelson <mmichels@redhat.com> > CC: Numan Siddique <numans@ovn.org> > Reported-at: https://bugzilla.redhat.com/1871931 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ofctrl.c | 120 +++++++++++++++++++++++++++++++++++++++-- > tests/ovn.at | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 265 insertions(+), 5 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 81a00c8..8b13b23 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,42 @@ 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. > */ > + if (!existing_conj && new_conj) { > + desired_flow_destroy(f); > + return; > + } > + I think we should keep both desired flows when there is a conflict, and add the new one in the same way as ofctrl_add_flow(). Otherwise, incremental processing would have problem because we would lose track of the mapping between logical flows and desired flows. I.e. when lflow-A maps to desired-flow-A, but lflow-B maps to nothing. Then in next iteration if lflow-A is deleted incrementally, the I-P engine would only delete desired-flow-A, not knowing that it should add desired-flow for lflow-B. I think the decision should be made during flow installation for which desired flow should get installed (this information is maintained in the installed-flow structure, for both the desired-refs and the one that is installed). This way, we don't lose any information and I-P will be able to handle properly. > 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 +1766,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 +1911,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..eb3bfbf 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -13709,6 +13709,156 @@ 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 > + > +# 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 > + > +# 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 > + > +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 >
On 9/10/20 7:49 PM, Han Zhou wrote: > > > > On Thu, Sep 10, 2020 at 6:06 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@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 perform such configuration. 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 follows 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 >> desired flow doesn't then we prefer the already installed flow. >> > Here is a typo. I think the last "desired" should be "installed". > > The idea looks good to me. So it attempts to ensure the behavior is > consistent and predictable, although it can't tell what's the real > intention of the user. This makes sense. > However, the implementation seems to have a problem. Please see my > comment below. > >> CC: Daniel Alvarez <dalvarez@redhat.com <mailto:dalvarez@redhat.com>> >> CC: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> >> CC: Mark Michelson <mmichels@redhat.com <mailto:mmichels@redhat.com>> >> CC: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >> Reported-at: https://bugzilla.redhat.com/1871931 >> Signed-off-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> >> --- >> controller/ofctrl.c | 120 +++++++++++++++++++++++++++++++++++++++-- >> tests/ovn.at <http://ovn.at> | 150 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 265 insertions(+), 5 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 81a00c8..8b13b23 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,42 @@ 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. >> */ >> + if (!existing_conj && new_conj) { >> + desired_flow_destroy(f); >> + return; >> + } >> + > > I think we should keep both desired flows when there is a conflict, and > add the new one in the same way as ofctrl_add_flow(). Otherwise, > incremental processing would have problem because we would lose track of > the mapping between logical flows and desired flows. I.e. when lflow-A > maps to desired-flow-A, but lflow-B maps to nothing. Then in next > iteration if lflow-A is deleted incrementally, the I-P engine would only > delete desired-flow-A, not knowing that it should add desired-flow for > lflow-B. I think the decision should be made during flow installation > for which desired flow should get installed (this information is > maintained in the installed-flow structure, for both the desired-refs > and the one that is installed). This way, we don't lose any information > and I-P will be able to handle properly. > Thanks for the review Han! You're right, I had missed this case. I updated it now and also updated the tests to check for the flows with the less restrictive ACL installed/removed. v2 available at: http://patchwork.ozlabs.org/project/ovn/patch/1599764632-23170-1-git-send-email-dceara@redhat.com/ Thanks, Dumitru
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 81a00c8..8b13b23 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,42 @@ 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. */ + if (!existing_conj && new_conj) { + desired_flow_destroy(f); + 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); + } ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); free(existing->flow.ofpacts); @@ -1687,6 +1766,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 +1911,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..eb3bfbf 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13709,6 +13709,156 @@ 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 + +# 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 + +# 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 + +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
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 perform such configuration. 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 follows 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 desired flow doesn't then we prefer the already installed flow. CC: Daniel Alvarez <dalvarez@redhat.com> CC: Han Zhou <hzhou@ovn.org> CC: Mark Michelson <mmichels@redhat.com> CC: Numan Siddique <numans@ovn.org> Reported-at: https://bugzilla.redhat.com/1871931 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ofctrl.c | 120 +++++++++++++++++++++++++++++++++++++++-- tests/ovn.at | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 5 deletions(-)