[ovs-dev] [PATCH ovn] ofctrl: Add a predictable resolution for conflicting flow actions.

Dumitru Ceara dceara at redhat.com
Thu Sep 10 19:06:48 UTC 2020


On 9/10/20 7:49 PM, Han Zhou wrote:
> 
> 
> 
> On Thu, Sep 10, 2020 at 6:06 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at 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 at redhat.com <mailto:dalvarez at redhat.com>>
>> CC: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
>> CC: Mark Michelson <mmichels at redhat.com <mailto:mmichels at redhat.com>>
>> CC: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
>> Reported-at: https://bugzilla.redhat.com/1871931
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at 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



More information about the dev mailing list