[ovs-git] [ovn-org/ovn] 986b3d: ofctrl.c: Add a predictable resolution for conflic...

[Dumitru Ceara]

  Branch: refs/heads/master
  Home:   https://github.com/ovn-org/ovn
  Commit: 986b3d5e4ad6f05245d021ba699c957246294a22
      https://github.com/ovn-org/ovn/commit/986b3d5e4ad6f05245d021ba699c957246294a22
  Author: Dumitru Ceara <dceara at redhat.com>
  Date:   2020-10-15 (Thu, 15 Oct 2020)

  Changed paths:
    M controller/ofctrl.c
    M tests/ovn.at

  Log Message:
  -----------
  ofctrl.c: Add a predictable resolution for conflicting flow actions.

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 all desired flows refering the
same installed flow are partially sorted in the following way:
- first all flows with action "allow".
- then all flows with action "drop".
- then a single flow with action "conjunction" (resulting from merging
  all flows with the same match and action conjunction).

This ensures that "allow" flows have precedence over "drop" flows which
in turn have precedence 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>
Acked-by: Mark Gray <mark.d.gray at redhat.com>
Signed-off-by: Han Zhou <hzhou at ovn.org>