On 9/10/20 7:49 PM, Han Zhou wrote: > > > > On Thu, Sep 10, 2020 at 6:06 AM 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 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 <[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 >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> >> --- >> 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/[email protected]/ Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
