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

Dumitru Ceara dceara at redhat.com
Thu Oct 15 07:15:57 UTC 2020


On 10/14/20 10:07 PM, Mark Gray wrote:
> On 13/10/2020 19:06, Han Zhou wrote:
>> On Tue, Oct 13, 2020 at 1:52 AM Dumitru Ceara <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 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 ensure that when selecting the active flow
>>> from all desired flows refering the same installed flow we give
>>> precedence to "allow" over "drop" over "conjunction" flows.
>>>
>>> Essentially, less restrictive flows are always preferred over more
>>> restrictive flows whenever a match conflict happens.
>>>
>>> CC: Daniel Alvarez <dalvarez at redhat.com>
>>> Reported-at: https://bugzilla.redhat.com/1871931
>>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>>> ---
>>> v6:
>>> - First 3 patches of the v5 series were already applied.
>>> - Send patch 4 as v6 addressing Han's comments:
>>>   - Instead of maintaining a sorted list, select active flow by walking
>>>     the list of desired flows.
>>>   - Add missing ovn_flow_log() call.
>>> v5:
>>> - Turn the patch into a series.
>>> - Address Han's comments.
>>> - Ensure predictable flow ordering in all cases (during incremental
>>>   procesing
>>>   or full recomputation).
>>> v4:
>>> - Address Han's comments:
>>>   - make sure only flows with action conjunction are combined.
>>> 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 |  96 +++++++++++++++++++++--
>>>  tests/ovn.at        | 214
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 305 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>>> index ba0c61c..0edb2b9 100644
>>> --- a/controller/ofctrl.c
>>> +++ b/controller/ofctrl.c
>>> @@ -229,6 +229,8 @@ static struct installed_flow *installed_flow_lookup(
>>>  static void installed_flow_destroy(struct installed_flow *);
>>>  static struct installed_flow *installed_flow_dup(struct desired_flow *);
>>>  static struct desired_flow *installed_flow_get_active(struct
>> installed_flow *);
>>> +static struct desired_flow *installed_flow_select_active(
>>> +    struct installed_flow *);
>>>
>>>  static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>>>  static char *ovn_flow_to_string(const struct ovn_flow *);
>>> @@ -796,6 +798,12 @@ ofctrl_recv(const struct ofp_header *oh, enum
>> ofptype type)
>>>  }
>>>
>>>  static bool
>>> +flow_action_has_drop(const struct ovn_flow *f)
>>> +{
>>> +    return f->ofpacts_len == 0;
>>> +}
>>> +
>>> +static bool
>>>  flow_action_has_conj(const struct ovn_flow *f)
>>>  {
>>>      const struct ofpact *a = NULL;
>>> @@ -822,7 +830,7 @@ link_installed_to_desired(struct installed_flow *i,
>> struct desired_flow *d)
>>>  {
>>>      d->installed_flow = i;
>>>      ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
>>> -    return installed_flow_get_active(i) == d;
>>> +    return installed_flow_select_active(i) == d;
>>>  }
>>>
>> Hi Dumitru, I am really sorry that I didn't make it clear enough in my last
>> comments. I meant that I prefer maintaining the order in the desired_refs
> 
> Is the ordering of this documented anywhere? At least in struct
> installed_flow in the comment above desired_refs?
> 

Not in this revision because the order isn't maintained in v6.  I did have a
section in v5 (where order was maintained) in the comment describing
installed_flow.  I'll adapt it and add it to a new revision.

>> list in the function link_installed_to_desired() when the item is inserted,
>> which makes the "first item is active" more useful. I didn't want to you
>> drop the ordering (but just wanted to avoid the array). Otherwise I would
>> not merge the patch 2 & 3 but directly use the temporary patch you created.
>> To be more clear, it could be something like below:
>>
>> /* Returns true if flow a is prefered over flow b. */
>> static bool
>> flow_compare(struct ovn_flow *a, struct ovn_flow *b)
>> {
>>     ...
>>     if (b_has_allow) {
>>         return false;
>>     }
>>     if (a_has_allow) {
>>         return true;
>>     }
>>     if (b_has_drop) {
>>         return false;
>>     }
>>     if (a_has_drop) {
>>         return true;
>>     }
>>     /* both has conjunction only, assert failure */
>>     OVS_NOT_REACHED();
>> }
> 
> I think this will make the code more readable as well. Perhaps the name
> flow_compare() is not descriptive enough. Maybe something like
> flow_is_preferred() or flow_preferred().
> 

Ack, flow_is_preferred() sounds good to me.

>>
>> static bool
>> link_installed_to_desired(struct installed_flow *i, struct desired_flow *d)
>> {
>>     d->installed = i;
>>     bool inserted = false;
>>     LIST_FOR_EACH(f, &i->desired_refs) {
>>         if (flow_compare(&d->flow, &f->flow)) {
> 
> How big are these lists likely to get? i.e. will this scale?
> 

In regular deployments lists should be very short (usually a single node).  In
cases when the CMS configures (partially) overlapping ACLs we'll have more than
one desired_flow referring an installed_flow.  But those should be corner cases
as Han pointed out so walking the list is probably good enough.

Thanks for the review.  I'll be sending a new revision soon.

Regards,
Dumitru



More information about the dev mailing list