[ovs-dev,v6] 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 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@redhat.com>
Reported-at: https://bugzilla.redhat.com/1871931
Signed-off-by: Dumitru Ceara <dceara@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(-)

On Tue, Oct 13, 2020 at 1:52 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 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@redhat.com>
> Reported-at: https://bugzilla.redhat.com/1871931
> Signed-off-by: Dumitru Ceara <dceara@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
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();
}

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)) {
            /* Insert d before f */
            ovs_list_insert(&f->installed_ref_list_node,
&d->installed_ref_list_node);
            inserted = true;
            break;
        }
    }
    if (!inserted) {
        ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
    }
}

>  /* Replaces 'old_desired' with 'new_desired' in the list of desired flows
> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow *i,
struct desired_flow *d)
>
>      ovs_assert(d && d->installed_flow == i);
>      ovs_list_remove(&d->installed_ref_list_node);
> +    installed_flow_select_active(i);

This is not needed if the order is always maintained in
link_installed_to_desired like above example.

>      d->installed_flow = NULL;
>      return old_active == d;
>  }
> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow *f)
>      return NULL;
>  }
>
> +/* Walks the list of desired flows that refer 'f' and selects the least
> + * restrictive one to be used as active flow, moving it to the front of
> + * the list.
> + *
> + * Returns the new active flow.
> + */
> +static struct desired_flow *
> +installed_flow_select_active(struct installed_flow *f)
> +{
> +    struct desired_flow *old_active = installed_flow_get_active(f);
> +    struct desired_flow *active = NULL;
> +
> +    /* If there is at most one desired_flow, no selection needed, return
> +     * early.
> +     */
> +    if (!old_active || ovs_list_is_short(&f->desired_refs)) {
> +        return old_active;
> +    }
> +
> +    struct desired_flow *d_allow = NULL;
> +    struct desired_flow *d_drop = NULL;
> +    struct desired_flow *d_conj = NULL;
> +    struct desired_flow *d;
> +
> +    LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) {
> +        if (flow_action_has_drop(&d->flow)) {
> +            if (!d_drop) {
> +                d_drop = d;
> +            }
> +        } else if (flow_action_has_conj(&d->flow)) {
> +            /* Conjunction flows are always merged together into a single
> +             * flow.  There should never be more than one referring the
same
> +             * installed flow.
> +             */
> +            ovs_assert(!d_conj);
> +            d_conj = d;
> +        } else {
> +            if (!d_allow) {
> +                d_allow = d;
> +            }
> +        }
> +    }
> +
> +    /* Give precedence to "allow" over "drop" over "conjunction" action.
*/
> +    if (d_allow) {
> +        active = d_allow;
> +    } else if (d_drop) {
> +        active = d_drop;
> +    } else {
> +        active = d_conj;
> +    }

If there are multiple flows in the same category (e.g. allow) in the list,
this function will select a different flow everytime, and move it to the
front, resulting in an unnecessary flow-mod. (But never mind, because this
function is not needed with the above suggested change)

> +
> +    /* There must be an active flow otherwise we should have returned
early. */
> +    ovs_assert(active);
> +
> +    /* Move the newly selected active flow to the front of the list. */
> +    if (active != old_active) {
> +        ovs_list_remove(&active->installed_ref_list_node);
> +        ovs_list_push_front(&f->desired_refs,
> +                            &active->installed_ref_list_node);
> +    }
> +    return active;
> +}
> +
>  static struct desired_flow *
>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
>                        const struct ovn_flow *target,
> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct
ovn_desired_flow_table *flow_table,
>              link_installed_to_desired(i, d);
>          } else if (!d->installed_flow) {
>              /* This is a desired_flow that conflicts with one installed
> -             * previously but not linked yet. */
> -            link_installed_to_desired(i, d);
> +             * previously but not linked yet.  However, if this flow
becomes
> +             * active, e.g., it is less restrictive than the previous
active
> +             * flow then modify the installed flow.
> +             */
> +            if (link_installed_to_desired(i, d)) {
> +                installed_flow_mod(&i->flow, &d->flow, msgs);
> +                ovn_flow_log(&i->flow, "updating installed (conflict)");
> +            }
>          }
>      }
>  }
> @@ -1919,8 +1998,15 @@ 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. */
> -                link_installed_to_desired(i, f);
> +                 * flow, so add it to the link.  If this flow becomes
active,
> +                 * e.g., it is less restrictive than the previous active
flow
> +                 * then modify the installed flow.
> +                 */
> +                if (link_installed_to_desired(i, f)) {
> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
> +                    ovn_flow_log(&i->flow,
> +                                 "updating installed (tracked
conflict)");
> +                }
>              }
>              /* The track_list_node emptyness is used to check if the
node is
>               * already added to track list, so initialize it again here.
*/
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 488fd11..9420b1e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13727,6 +13727,220 @@ 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 acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 ||
ip4.src==10.0.0.42) && (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 two less restrictive allow ACLs for src IP 10.0.0.1.
> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 ||
ip4.src==10.0.0.1' allow
> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the less restrictive flows should have been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# 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
> +
> +#sleep infinity
> +
> +# Remove the first less restrictive allow ACL.
> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the second less restrictive allow ACL should have
been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# 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
> +
> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
actions=conjunction(2,2/2),conjunction(3,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# 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])
> +
> +# Re-add the 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
> +
> +# Check OVS flows, the less restrictive flows should have been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> +   grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +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 10/13/20 8:06 PM, Han Zhou wrote:
> 
> 
> On Tue, Oct 13, 2020 at 1:52 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 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@redhat.com <mailto:dalvarez@redhat.com>>
>> Reported-at: https://bugzilla.redhat.com/1871931
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@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 <http://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 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:
> 

Hi Han,

I guess I misunderstood your comments indeed. I thought you didn't want to keep
the list partially sorted.

However, I'm now curious what your concerns were regarding my original
suggestion, apart from the missing ovn_flow_log():

http://patchwork.ozlabs.org/project/ovn/patch/20201011120607.3374.49659.stgit@dceara.remote.csb/

> /* 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();
> }
> 
> 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)) {
>             /* Insert d before f */
>             ovs_list_insert(&f->installed_ref_list_node,
> &d->installed_ref_list_node);
>             inserted = true;
>             break;
>         }
>     }
>     if (!inserted) {
>         ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
>     }
> }
> 
>>  /* Replaces 'old_desired' with 'new_desired' in the list of desired flows
>> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow *i,
> struct desired_flow *d)
>>
>>      ovs_assert(d && d->installed_flow == i);
>>      ovs_list_remove(&d->installed_ref_list_node);
>> +    installed_flow_select_active(i);
> 
> This is not needed if the order is always maintained in
> link_installed_to_desired like above example.
> 
>>      d->installed_flow = NULL;
>>      return old_active == d;
>>  }
>> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow *f)
>>      return NULL;
>>  }
>>
>> +/* Walks the list of desired flows that refer 'f' and selects the least
>> + * restrictive one to be used as active flow, moving it to the front of
>> + * the list.
>> + *
>> + * Returns the new active flow.
>> + */
>> +static struct desired_flow *
>> +installed_flow_select_active(struct installed_flow *f)
>> +{
>> +    struct desired_flow *old_active = installed_flow_get_active(f);
>> +    struct desired_flow *active = NULL;
>> +
>> +    /* If there is at most one desired_flow, no selection needed, return
>> +     * early.
>> +     */
>> +    if (!old_active || ovs_list_is_short(&f->desired_refs)) {
>> +        return old_active;
>> +    }
>> +
>> +    struct desired_flow *d_allow = NULL;
>> +    struct desired_flow *d_drop = NULL;
>> +    struct desired_flow *d_conj = NULL;
>> +    struct desired_flow *d;
>> +
>> +    LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) {
>> +        if (flow_action_has_drop(&d->flow)) {
>> +            if (!d_drop) {
>> +                d_drop = d;
>> +            }
>> +        } else if (flow_action_has_conj(&d->flow)) {
>> +            /* Conjunction flows are always merged together into a single
>> +             * flow.  There should never be more than one referring the same
>> +             * installed flow.
>> +             */
>> +            ovs_assert(!d_conj);
>> +            d_conj = d;
>> +        } else {
>> +            if (!d_allow) {
>> +                d_allow = d;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Give precedence to "allow" over "drop" over "conjunction" action. */
>> +    if (d_allow) {
>> +        active = d_allow;
>> +    } else if (d_drop) {
>> +        active = d_drop;
>> +    } else {
>> +        active = d_conj;
>> +    }
> 
> If there are multiple flows in the same category (e.g. allow) in the list, this
> function will select a different flow everytime, and move it to the front,

No. The flows are always pushed to the back of the list. So if there already
was a flow in the same category this function will still find it as "active"
and won't replace it.

> resulting in an unnecessary flow-mod. (But never mind, because this function is
> not needed with the above suggested change)
> 

Right, this is not needed if we maintain a sorted list.

Thanks,
Dumitru

>> +
>> +    /* There must be an active flow otherwise we should have returned early. */
>> +    ovs_assert(active);
>> +
>> +    /* Move the newly selected active flow to the front of the list. */
>> +    if (active != old_active) {
>> +        ovs_list_remove(&active->installed_ref_list_node);
>> +        ovs_list_push_front(&f->desired_refs,
>> +                            &active->installed_ref_list_node);
>> +    }
>> +    return active;
>> +}
>> +
>>  static struct desired_flow *
>>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
>>                        const struct ovn_flow *target,
>> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct
> ovn_desired_flow_table *flow_table,
>>              link_installed_to_desired(i, d);
>>          } else if (!d->installed_flow) {
>>              /* This is a desired_flow that conflicts with one installed
>> -             * previously but not linked yet. */
>> -            link_installed_to_desired(i, d);
>> +             * previously but not linked yet.  However, if this flow becomes
>> +             * active, e.g., it is less restrictive than the previous active
>> +             * flow then modify the installed flow.
>> +             */
>> +            if (link_installed_to_desired(i, d)) {
>> +                installed_flow_mod(&i->flow, &d->flow, msgs);
>> +                ovn_flow_log(&i->flow, "updating installed (conflict)");
>> +            }
>>          }
>>      }
>>  }
>> @@ -1919,8 +1998,15 @@ 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. */
>> -                link_installed_to_desired(i, f);
>> +                 * flow, so add it to the link.  If this flow becomes active,
>> +                 * e.g., it is less restrictive than the previous active flow
>> +                 * then modify the installed flow.
>> +                 */
>> +                if (link_installed_to_desired(i, f)) {
>> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>> +                    ovn_flow_log(&i->flow,
>> +                                 "updating installed (tracked conflict)");
>> +                }
>>              }
>>              /* The track_list_node emptyness is used to check if the node is
>>               * already added to track list, so initialize it again here. */
>> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
>> index 488fd11..9420b1e 100644
>> --- a/tests/ovn.at <http://ovn.at>
>> +++ b/tests/ovn.at <http://ovn.at>
>> @@ -13727,6 +13727,220 @@ 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 acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.42)
> && (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 <http://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 two less restrictive allow ACLs for src IP 10.0.0.1.
>> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' allow
>> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
>> +ovn-nbctl --wait=hv sync
>> +
>> +# Check OVS flows, the less restrictive flows should have been installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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 <http://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
>> +
>> +#sleep infinity
>> +
>> +# Remove the first less restrictive allow ACL.
>> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
>> +ovn-nbctl --wait=hv sync
>> +
>> +# Check OVS flows, the second less restrictive allow ACL should have been
> installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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
>> +
>> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
> actions=conjunction(2,2/2),conjunction(3,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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 <http://ovs-pcap.in>"
> hv1/vif2-tx.pcap > 2.packets
>> +AT_CHECK([cat 2.packets], [0], [expout])
>> +
>> +# Re-add the 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
>> +
>> +# Check OVS flows, the less restrictive flows should have been installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +   grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +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 Wed, Oct 14, 2020 at 12:21 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 10/13/20 8:06 PM, Han Zhou wrote:
> >
> >
> > On Tue, Oct 13, 2020 at 1:52 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 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@redhat.com <mailto:dalvarez@redhat.com>>
> >> Reported-at: https://bugzilla.redhat.com/1871931
> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com <mailto:
dceara@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 <http://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 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:
> >
>
> Hi Han,
>
> I guess I misunderstood your comments indeed. I thought you didn't want
to keep
> the list partially sorted.
>
> However, I'm now curious what your concerns were regarding my original
> suggestion, apart from the missing ovn_flow_log():
>
>
http://patchwork.ozlabs.org/project/ovn/patch/20201011120607.3374.49659.stgit@dceara.remote.csb/
>

My last comment only suggested avoiding the array of lists for patch 4. It
is a little overkill to store such data that in most cases only 1 list item
exists in only one of the array items.

> > /* 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();
> > }
> >
> > 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)) {
> >             /* Insert d before f */
> >             ovs_list_insert(&f->installed_ref_list_node,
> > &d->installed_ref_list_node);
> >             inserted = true;
> >             break;
> >         }
> >     }
> >     if (!inserted) {
> >         ovs_list_push_back(&i->desired_refs,
&d->installed_ref_list_node);
> >     }
> > }
> >
> >>  /* Replaces 'old_desired' with 'new_desired' in the list of desired
flows
> >> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow
*i,
> > struct desired_flow *d)
> >>
> >>      ovs_assert(d && d->installed_flow == i);
> >>      ovs_list_remove(&d->installed_ref_list_node);
> >> +    installed_flow_select_active(i);
> >
> > This is not needed if the order is always maintained in
> > link_installed_to_desired like above example.
> >
> >>      d->installed_flow = NULL;
> >>      return old_active == d;
> >>  }
> >> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow
*f)
> >>      return NULL;
> >>  }
> >>
> >> +/* Walks the list of desired flows that refer 'f' and selects the
least
> >> + * restrictive one to be used as active flow, moving it to the front
of
> >> + * the list.
> >> + *
> >> + * Returns the new active flow.
> >> + */
> >> +static struct desired_flow *
> >> +installed_flow_select_active(struct installed_flow *f)
> >> +{
> >> +    struct desired_flow *old_active = installed_flow_get_active(f);
> >> +    struct desired_flow *active = NULL;
> >> +
> >> +    /* If there is at most one desired_flow, no selection needed,
return
> >> +     * early.
> >> +     */
> >> +    if (!old_active || ovs_list_is_short(&f->desired_refs)) {
> >> +        return old_active;
> >> +    }
> >> +
> >> +    struct desired_flow *d_allow = NULL;
> >> +    struct desired_flow *d_drop = NULL;
> >> +    struct desired_flow *d_conj = NULL;
> >> +    struct desired_flow *d;
> >> +
> >> +    LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) {
> >> +        if (flow_action_has_drop(&d->flow)) {
> >> +            if (!d_drop) {
> >> +                d_drop = d;
> >> +            }
> >> +        } else if (flow_action_has_conj(&d->flow)) {
> >> +            /* Conjunction flows are always merged together into a
single
> >> +             * flow.  There should never be more than one referring
the same
> >> +             * installed flow.
> >> +             */
> >> +            ovs_assert(!d_conj);
> >> +            d_conj = d;
> >> +        } else {
> >> +            if (!d_allow) {
> >> +                d_allow = d;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    /* Give precedence to "allow" over "drop" over "conjunction"
action. */
> >> +    if (d_allow) {
> >> +        active = d_allow;
> >> +    } else if (d_drop) {
> >> +        active = d_drop;
> >> +    } else {
> >> +        active = d_conj;
> >> +    }
> >
> > If there are multiple flows in the same category (e.g. allow) in the
list, this
> > function will select a different flow everytime, and move it to the
front,
>
> No. The flows are always pushed to the back of the list. So if there
already
> was a flow in the same category this function will still find it as
"active"
> and won't replace it.

Sorry, I misread it.
>
> > resulting in an unnecessary flow-mod. (But never mind, because this
function is
> > not needed with the above suggested change)
> >
>
> Right, this is not needed if we maintain a sorted list.
>
> Thanks,
> Dumitru
>
> >> +
> >> +    /* There must be an active flow otherwise we should have returned
early. */
> >> +    ovs_assert(active);
> >> +
> >> +    /* Move the newly selected active flow to the front of the list.
*/
> >> +    if (active != old_active) {
> >> +        ovs_list_remove(&active->installed_ref_list_node);
> >> +        ovs_list_push_front(&f->desired_refs,
> >> +                            &active->installed_ref_list_node);
> >> +    }
> >> +    return active;
> >> +}
> >> +
> >>  static struct desired_flow *
> >>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
> >>                        const struct ovn_flow *target,
> >> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct
> > ovn_desired_flow_table *flow_table,
> >>              link_installed_to_desired(i, d);
> >>          } else if (!d->installed_flow) {
> >>              /* This is a desired_flow that conflicts with one
installed
> >> -             * previously but not linked yet. */
> >> -            link_installed_to_desired(i, d);
> >> +             * previously but not linked yet.  However, if this flow
becomes
> >> +             * active, e.g., it is less restrictive than the previous
active
> >> +             * flow then modify the installed flow.
> >> +             */
> >> +            if (link_installed_to_desired(i, d)) {
> >> +                installed_flow_mod(&i->flow, &d->flow, msgs);
> >> +                ovn_flow_log(&i->flow, "updating installed
(conflict)");
> >> +            }
> >>          }
> >>      }
> >>  }
> >> @@ -1919,8 +1998,15 @@ 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. */
> >> -                link_installed_to_desired(i, f);
> >> +                 * flow, so add it to the link.  If this flow becomes
active,
> >> +                 * e.g., it is less restrictive than the previous
active flow
> >> +                 * then modify the installed flow.
> >> +                 */
> >> +                if (link_installed_to_desired(i, f)) {
> >> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
> >> +                    ovn_flow_log(&i->flow,
> >> +                                 "updating installed (tracked
conflict)");
> >> +                }
> >>              }
> >>              /* The track_list_node emptyness is used to check if the
node is
> >>               * already added to track list, so initialize it again
here. */
> >> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at
>
> >> index 488fd11..9420b1e 100644
> >> --- a/tests/ovn.at <http://ovn.at>
> >> +++ b/tests/ovn.at <http://ovn.at>
> >> @@ -13727,6 +13727,220 @@ 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 acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 ||
ip4.src==10.0.0.42)
> > && (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 <http://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 two less restrictive allow ACLs for src IP 10.0.0.1.
> >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 ||
ip4.src==10.0.0.1' allow
> >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
> >> +ovn-nbctl --wait=hv sync
> >> +
> >> +# Check OVS flows, the less restrictive flows should have been
installed.
> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(2,2/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
actions=conjunction(3,2/2)
> >> +])
> >> +
> >> +# 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 <http://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
> >> +
> >> +#sleep infinity
> >> +
> >> +# Remove the first less restrictive allow ACL.
> >> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 ||
ip4.src==10.0.0.1'
> >> +ovn-nbctl --wait=hv sync
> >> +
> >> +# Check OVS flows, the second less restrictive allow ACL should have
been
> > installed.
> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(2,2/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
actions=conjunction(3,2/2)
> >> +])
> >> +
> >> +# 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
> >> +
> >> +# Check OVS flows, the 10.0.0.1 conjunction should have been
reinstalled.
> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
> > actions=conjunction(2,2/2),conjunction(3,2/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(2,2/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
actions=conjunction(3,2/2)
> >> +])
> >> +
> >> +# 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 <http://ovs-pcap.in>"
> > hv1/vif2-tx.pcap > 2.packets
> >> +AT_CHECK([cat 2.packets], [0], [expout])
> >> +
> >> +# Re-add the 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
> >> +
> >> +# Check OVS flows, the less restrictive flows should have been
installed.
> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> >> +   grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> > actions=conjunction(2,1/2),conjunction(3,1/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(2,2/2)
> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
actions=conjunction(3,2/2)
> >> +])
> >> +
> >> +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 13/10/2020 19:06, Han Zhou wrote:
> On Tue, Oct 13, 2020 at 1:52 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 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@redhat.com>
>> Reported-at: https://bugzilla.redhat.com/1871931
>> Signed-off-by: Dumitru Ceara <dceara@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?

> 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().

> 
> 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?

>             /* Insert d before f */
>             ovs_list_insert(&f->installed_ref_list_node,
> &d->installed_ref_list_node);
>             inserted = true;
>             break;
>         }
>     }
>     if (!inserted) {
>         ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
>     }
> }
> 
>>  /* Replaces 'old_desired' with 'new_desired' in the list of desired flows
>> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow *i,
> struct desired_flow *d)
>>
>>      ovs_assert(d && d->installed_flow == i);
>>      ovs_list_remove(&d->installed_ref_list_node);
>> +    installed_flow_select_active(i);
> 
> This is not needed if the order is always maintained in
> link_installed_to_desired like above example.
> 
>>      d->installed_flow = NULL;
>>      return old_active == d;
>>  }
>> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow *f)
>>      return NULL;
>>  }
>>
>> +/* Walks the list of desired flows that refer 'f' and selects the least
>> + * restrictive one to be used as active flow, moving it to the front of
>> + * the list.
>> + *
>> + * Returns the new active flow.
>> + */
>> +static struct desired_flow *
>> +installed_flow_select_active(struct installed_flow *f)
>> +{
>> +    struct desired_flow *old_active = installed_flow_get_active(f);
>> +    struct desired_flow *active = NULL;
>> +
>> +    /* If there is at most one desired_flow, no selection needed, return
>> +     * early.
>> +     */
>> +    if (!old_active || ovs_list_is_short(&f->desired_refs)) {
>> +        return old_active;
>> +    }
>> +
>> +    struct desired_flow *d_allow = NULL;
>> +    struct desired_flow *d_drop = NULL;
>> +    struct desired_flow *d_conj = NULL;
>> +    struct desired_flow *d;
>> +
>> +    LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) {
>> +        if (flow_action_has_drop(&d->flow)) {
>> +            if (!d_drop) {
>> +                d_drop = d;
>> +            }
>> +        } else if (flow_action_has_conj(&d->flow)) {
>> +            /* Conjunction flows are always merged together into a single
>> +             * flow.  There should never be more than one referring the
> same
>> +             * installed flow.
>> +             */
>> +            ovs_assert(!d_conj);
>> +            d_conj = d;
>> +        } else {
>> +            if (!d_allow) {
>> +                d_allow = d;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Give precedence to "allow" over "drop" over "conjunction" action.
> */
>> +    if (d_allow) {
>> +        active = d_allow;
>> +    } else if (d_drop) {
>> +        active = d_drop;
>> +    } else {
>> +        active = d_conj;
>> +    }
> 
> If there are multiple flows in the same category (e.g. allow) in the list,
> this function will select a different flow everytime, and move it to the
> front, resulting in an unnecessary flow-mod. (But never mind, because this
> function is not needed with the above suggested change)
> 
>> +
>> +    /* There must be an active flow otherwise we should have returned
> early. */
>> +    ovs_assert(active);
>> +
>> +    /* Move the newly selected active flow to the front of the list. */
>> +    if (active != old_active) {
>> +        ovs_list_remove(&active->installed_ref_list_node);
>> +        ovs_list_push_front(&f->desired_refs,
>> +                            &active->installed_ref_list_node);
>> +    }
>> +    return active;
>> +}
>> +
>>  static struct desired_flow *
>>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
>>                        const struct ovn_flow *target,
>> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct
> ovn_desired_flow_table *flow_table,
>>              link_installed_to_desired(i, d);
>>          } else if (!d->installed_flow) {
>>              /* This is a desired_flow that conflicts with one installed
>> -             * previously but not linked yet. */
>> -            link_installed_to_desired(i, d);
>> +             * previously but not linked yet.  However, if this flow
> becomes
>> +             * active, e.g., it is less restrictive than the previous
> active
>> +             * flow then modify the installed flow.
>> +             */
>> +            if (link_installed_to_desired(i, d)) {
>> +                installed_flow_mod(&i->flow, &d->flow, msgs);
>> +                ovn_flow_log(&i->flow, "updating installed (conflict)");
>> +            }
>>          }
>>      }
>>  }
>> @@ -1919,8 +1998,15 @@ 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. */
>> -                link_installed_to_desired(i, f);
>> +                 * flow, so add it to the link.  If this flow becomes
> active,
>> +                 * e.g., it is less restrictive than the previous active
> flow
>> +                 * then modify the installed flow.
>> +                 */
>> +                if (link_installed_to_desired(i, f)) {
>> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>> +                    ovn_flow_log(&i->flow,
>> +                                 "updating installed (tracked
> conflict)");
>> +                }
>>              }
>>              /* The track_list_node emptyness is used to check if the
> node is
>>               * already added to track list, so initialize it again here.
> */
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 488fd11..9420b1e 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -13727,6 +13727,220 @@ 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 acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 ||
> ip4.src==10.0.0.42) && (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 two less restrictive allow ACLs for src IP 10.0.0.1.
>> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 ||
> ip4.src==10.0.0.1' allow
>> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
>> +ovn-nbctl --wait=hv sync
>> +
>> +# Check OVS flows, the less restrictive flows should have been installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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
>> +
>> +#sleep infinity
>> +
>> +# Remove the first less restrictive allow ACL.
>> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
>> +ovn-nbctl --wait=hv sync
>> +
>> +# Check OVS flows, the second less restrictive allow ACL should have
> been installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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
>> +
>> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
> actions=conjunction(2,2/2),conjunction(3,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +# 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])
>> +
>> +# Re-add the 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
>> +
>> +# Check OVS flows, the less restrictive flows should have been installed.
>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> +   grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> +])
>> +
>> +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
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 10/14/20 6:11 PM, Han Zhou wrote:
> 
> 
> On Wed, Oct 14, 2020 at 12:21 AM Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>> wrote:
>>
>> On 10/13/20 8:06 PM, Han Zhou wrote:
>> >
>> >
>> > On Tue, Oct 13, 2020 at 1:52 AM Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>
>> > <mailto: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 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@redhat.com <mailto:dalvarez@redhat.com>
> <mailto:dalvarez@redhat.com <mailto:dalvarez@redhat.com>>>
>> >> Reported-at: https://bugzilla.redhat.com/1871931
>> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>
> <mailto:dceara@redhat.com <mailto:dceara@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 <http://ovn.at> <http://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 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:
>> >
>>
>> Hi Han,
>>
>> I guess I misunderstood your comments indeed. I thought you didn't want to keep
>> the list partially sorted.
>>
>> However, I'm now curious what your concerns were regarding my original
>> suggestion, apart from the missing ovn_flow_log():
>>
>>
> http://patchwork.ozlabs.org/project/ovn/patch/20201011120607.3374.49659.stgit@dceara.remote.csb/
>>
> 
> My last comment only suggested avoiding the array of lists for patch 4. It is a
> little overkill to store such data that in most cases only 1 list item exists
> in only one of the array items.
> 

Ok, thanks for the explanation.

I'll send a new revision soon.

Regards,
Dumitru

>> > /* 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();
>> > }
>> >
>> > 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)) {
>> >             /* Insert d before f */
>> >             ovs_list_insert(&f->installed_ref_list_node,
>> > &d->installed_ref_list_node);
>> >             inserted = true;
>> >             break;
>> >         }
>> >     }
>> >     if (!inserted) {
>> >         ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
>> >     }
>> > }
>> >
>> >>  /* Replaces 'old_desired' with 'new_desired' in the list of desired flows
>> >> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow *i,
>> > struct desired_flow *d)
>> >>
>> >>      ovs_assert(d && d->installed_flow == i);
>> >>      ovs_list_remove(&d->installed_ref_list_node);
>> >> +    installed_flow_select_active(i);
>> >
>> > This is not needed if the order is always maintained in
>> > link_installed_to_desired like above example.
>> >
>> >>      d->installed_flow = NULL;
>> >>      return old_active == d;
>> >>  }
>> >> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow *f)
>> >>      return NULL;
>> >>  }
>> >>
>> >> +/* Walks the list of desired flows that refer 'f' and selects the least
>> >> + * restrictive one to be used as active flow, moving it to the front of
>> >> + * the list.
>> >> + *
>> >> + * Returns the new active flow.
>> >> + */
>> >> +static struct desired_flow *
>> >> +installed_flow_select_active(struct installed_flow *f)
>> >> +{
>> >> +    struct desired_flow *old_active = installed_flow_get_active(f);
>> >> +    struct desired_flow *active = NULL;
>> >> +
>> >> +    /* If there is at most one desired_flow, no selection needed, return
>> >> +     * early.
>> >> +     */
>> >> +    if (!old_active || ovs_list_is_short(&f->desired_refs)) {
>> >> +        return old_active;
>> >> +    }
>> >> +
>> >> +    struct desired_flow *d_allow = NULL;
>> >> +    struct desired_flow *d_drop = NULL;
>> >> +    struct desired_flow *d_conj = NULL;
>> >> +    struct desired_flow *d;
>> >> +
>> >> +    LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) {
>> >> +        if (flow_action_has_drop(&d->flow)) {
>> >> +            if (!d_drop) {
>> >> +                d_drop = d;
>> >> +            }
>> >> +        } else if (flow_action_has_conj(&d->flow)) {
>> >> +            /* Conjunction flows are always merged together into a single
>> >> +             * flow.  There should never be more than one referring the same
>> >> +             * installed flow.
>> >> +             */
>> >> +            ovs_assert(!d_conj);
>> >> +            d_conj = d;
>> >> +        } else {
>> >> +            if (!d_allow) {
>> >> +                d_allow = d;
>> >> +            }
>> >> +        }
>> >> +    }
>> >> +
>> >> +    /* Give precedence to "allow" over "drop" over "conjunction" action. */
>> >> +    if (d_allow) {
>> >> +        active = d_allow;
>> >> +    } else if (d_drop) {
>> >> +        active = d_drop;
>> >> +    } else {
>> >> +        active = d_conj;
>> >> +    }
>> >
>> > If there are multiple flows in the same category (e.g. allow) in the list, this
>> > function will select a different flow everytime, and move it to the front,
>>
>> No. The flows are always pushed to the back of the list. So if there already
>> was a flow in the same category this function will still find it as "active"
>> and won't replace it.
> 
> Sorry, I misread it.
>>
>> > resulting in an unnecessary flow-mod. (But never mind, because this function is
>> > not needed with the above suggested change)
>> >
>>
>> Right, this is not needed if we maintain a sorted list.
>>
>> Thanks,
>> Dumitru
>>
>> >> +
>> >> +    /* There must be an active flow otherwise we should have returned
> early. */
>> >> +    ovs_assert(active);
>> >> +
>> >> +    /* Move the newly selected active flow to the front of the list. */
>> >> +    if (active != old_active) {
>> >> +        ovs_list_remove(&active->installed_ref_list_node);
>> >> +        ovs_list_push_front(&f->desired_refs,
>> >> +                            &active->installed_ref_list_node);
>> >> +    }
>> >> +    return active;
>> >> +}
>> >> +
>> >>  static struct desired_flow *
>> >>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
>> >>                        const struct ovn_flow *target,
>> >> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct
>> > ovn_desired_flow_table *flow_table,
>> >>              link_installed_to_desired(i, d);
>> >>          } else if (!d->installed_flow) {
>> >>              /* This is a desired_flow that conflicts with one installed
>> >> -             * previously but not linked yet. */
>> >> -            link_installed_to_desired(i, d);
>> >> +             * previously but not linked yet.  However, if this flow becomes
>> >> +             * active, e.g., it is less restrictive than the previous active
>> >> +             * flow then modify the installed flow.
>> >> +             */
>> >> +            if (link_installed_to_desired(i, d)) {
>> >> +                installed_flow_mod(&i->flow, &d->flow, msgs);
>> >> +                ovn_flow_log(&i->flow, "updating installed (conflict)");
>> >> +            }
>> >>          }
>> >>      }
>> >>  }
>> >> @@ -1919,8 +1998,15 @@ 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. */
>> >> -                link_installed_to_desired(i, f);
>> >> +                 * flow, so add it to the link.  If this flow becomes active,
>> >> +                 * e.g., it is less restrictive than the previous active flow
>> >> +                 * then modify the installed flow.
>> >> +                 */
>> >> +                if (link_installed_to_desired(i, f)) {
>> >> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>> >> +                    ovn_flow_log(&i->flow,
>> >> +                                 "updating installed (tracked conflict)");
>> >> +                }
>> >>              }
>> >>              /* The track_list_node emptyness is used to check if the node is
>> >>               * already added to track list, so initialize it again here. */
>> >> diff --git a/tests/ovn.at <http://ovn.at> <http://ovn.at> b/tests/ovn.at
> <http://ovn.at> <http://ovn.at>
>> >> index 488fd11..9420b1e 100644
>> >> --- a/tests/ovn.at <http://ovn.at> <http://ovn.at>
>> >> +++ b/tests/ovn.at <http://ovn.at> <http://ovn.at>
>> >> @@ -13727,6 +13727,220 @@ 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 acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.42)
>> > && (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 <http://ovs-pcap.in>
> <http://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 two less restrictive allow ACLs for src IP 10.0.0.1.
>> >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
> allow
>> >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
>> >> +ovn-nbctl --wait=hv sync
>> >> +
>> >> +# Check OVS flows, the less restrictive flows should have been installed.
>> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> >> +])
>> >> +
>> >> +# 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 <http://ovs-pcap.in>
> <http://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
>> >> +
>> >> +#sleep infinity
>> >> +
>> >> +# Remove the first less restrictive allow ACL.
>> >> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
>> >> +ovn-nbctl --wait=hv sync
>> >> +
>> >> +# Check OVS flows, the second less restrictive allow ACL should have been
>> > installed.
>> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> >> +])
>> >> +
>> >> +# 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
>> >> +
>> >> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled.
>> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> >> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
>> > actions=conjunction(2,2/2),conjunction(3,2/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> >> +])
>> >> +
>> >> +# 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 <http://ovs-pcap.in>
> <http://ovs-pcap.in>"
>> > hv1/vif2-tx.pcap > 2.packets
>> >> +AT_CHECK([cat 2.packets], [0], [expout])
>> >> +
>> >> +# Re-add the 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
>> >> +
>> >> +# Check OVS flows, the less restrictive flows should have been installed.
>> >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
>> >> +   grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>> > actions=conjunction(2,1/2),conjunction(3,1/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
>> >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
>> >> +])
>> >> +
>> >> +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 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@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@redhat.com>
>>> Reported-at: https://bugzilla.redhat.com/1871931
>>> Signed-off-by: Dumitru Ceara <dceara@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