[ovs-dev,v3] ofctrl: 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 resolve conflicts between flows with the
same match and different actions by giving precedence to less
restrictive flows. This means that if the installed flow has action
"conjunction" and the desired flow doesn't then we prefer the desired
flow. Similarly, if the desired flow has action "conjunction" and the
installed flow doesn't then we prefer the already installed flow.

CC: Daniel Alvarez <dalvarez@redhat.com>
CC: Han Zhou <hzhou@ovn.org>
CC: Mark Michelson <mmichels@redhat.com>
CC: Numan Siddique <numans@ovn.org>
Reported-at: https://bugzilla.redhat.com/1871931
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
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 | 124 +++++++++++++++++++++++++++++++++++--
 tests/ovn.at        | 174 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 293 insertions(+), 5 deletions(-)

On Fri, Sep 11, 2020 at 3:07 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 resolve conflicts between flows with the
> same match and different actions by giving precedence to less
> restrictive flows. This means that if the installed flow has action
> "conjunction" and the desired flow doesn't then we prefer the desired
> flow. Similarly, if the desired flow has action "conjunction" and the
> installed flow doesn't then we prefer the already installed flow.
>
> CC: Daniel Alvarez <dalvarez@redhat.com>
> CC: Han Zhou <hzhou@ovn.org>
> CC: Mark Michelson <mmichels@redhat.com>
> CC: Numan Siddique <numans@ovn.org>
> Reported-at: https://bugzilla.redhat.com/1871931
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>



Thanks Dumitru for the patch.

The added test case fails  because of recent changes to the ACL table
number. With the below changes, the test case passes.

***********************
diff --git a/tests/ovn.at b/tests/ovn.at
index 9d8ce3bb2e..11e7dfd160 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13808,12 +13808,12 @@ 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=44 | \
+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(,45)
+priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
 priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
 priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
-priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
+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)
 ])

@@ -13849,9 +13849,9 @@ 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=44 | \
+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(,45)
+priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
 priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
 priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
 priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
**********************

The patch LGTM with one minor comment.

Acked-by: Numan Siddique <numans@ovn.org>

I will wait for Han if he has any comments before applying this patch.

Thanks
Numan


> 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 | 124 +++++++++++++++++++++++++++++++++++--
>  tests/ovn.at        | 174
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 293 insertions(+), 5 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 81a00c8..819e8cf 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table
> *flow_table,
>      ovs_list_insert(&stf->flows, &sfr->flow_list);
>  }
>
> +static bool
> +flow_action_has_conj(const struct ovn_flow *f)
> +{
> +    const struct ofpact *a = NULL;
> +
> +    OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) {
> +        if (a->type == OFPACT_CONJUNCTION) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +/* Returns true if the actions of 'f1' cannot be merged with the actions
> of
> + * 'f2'. This is the case when one of the flow contains action
> "conjunction"
> + * and the other doesn't. The function always populates 'f1_has_conj' and
> + * 'f2_has_conj'.
> + */
> +static bool
> +flow_actions_conflict(const struct ovn_flow *f1, const struct ovn_flow
> *f2,
> +                      bool *f1_has_conj, bool *f2_has_conj)
> +{
> +    *f1_has_conj = flow_action_has_conj(f1);
> +    *f2_has_conj = flow_action_has_conj(f2);
> +
> +    if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) {
> +        return true;
> +    }
> +
> +    if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void
> +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1,
> +                          const struct ovn_flow *f2)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +
> +    char *f1_s = ovn_flow_to_string(f1);
> +    char *f2_s = ovn_flow_to_string(f2);
> +
> +    VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s, flow-2: %s",
> +                 msg, f1_s, f2_s);
> +    free(f1_s);
> +    free(f2_s);
> +}
> +
>  /* Flow table interfaces to the rest of ovn-controller. */
>
>  /* Adds a flow to 'desired_flows' with the specified 'match' and
> 'actions' to
> @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct
> ovn_desired_flow_table *desired_flows,
>      struct desired_flow *existing;
>      existing = desired_flow_lookup(desired_flows, &f->flow, NULL);
>      if (existing) {
> -        /* There's already a flow with this particular match. Append the
> -         * action to that flow rather than adding a new flow
> +        /* There's already a flow with this particular match. Try to
> append
> +         * the action to that flow rather than adding a new flow.
> +         *
> +         * If exactly one of the existing or new flow includes a
> conjunction
> +         * action then we can't merge the actions. In that case keep the
> flow
> +         * without conjunction action as this is the least restrictive
> one.
> +         */
> +        bool existing_conj = false;
> +        bool new_conj = false;
> +
> +        if (flow_actions_conflict(&existing->flow, &f->flow,
> &existing_conj,
> +                                  &new_conj)) {
> +            flow_log_actions_conflict("Cannot merge conj with non-conj
> action",
> +                                      &existing->flow, &f->flow);
> +        }
> +
> +        /* If the existing flow is less restrictive (i.e., no conjunction
> +         * action) then keep it installed. Store however the flow with
> +         * conjunction action too as it will be installed when the current
> +         * one is removed.
>           */
> +        if (!existing_conj && new_conj) {
> +            hmap_insert(&desired_flows->match_flow_table,
> &f->match_hmap_node,
> +                        f->flow.hash);
> +            link_flow_to_sb(desired_flows, f, sb_uuid);
> +            return;
> +        }
> +
>          uint64_t compound_stub[64 / 8];
>          struct ofpbuf compound;
>          ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
> -        ofpbuf_put(&compound, existing->flow.ofpacts,
> -                   existing->flow.ofpacts_len);
> +
> +        /* Only merge actions if both flows either have action conjunction
> +         * or non-conjunction. Otherwise use the actions in the new flow,
> +         * the least restrictive.
> +         */
> +        if (existing_conj == new_conj) {
> +            ofpbuf_put(&compound, existing->flow.ofpacts,
> +                       existing->flow.ofpacts_len);
> +        }
>

In the case where 'existing_conj'  is true and 'new_conj' is false, we can
actually avoid the compound_stub
I'd do something like below

<*************************************>

static void
ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b)
{
    struct ofpact *tmp = a->ofpacts;
    size_t tmp_len = a->ofpacts_len;

    a->ofpacts = b->ofpacts;
    a->ofpacts_len = b->ofpacts_len;

    b->ofpacts = tmp;
    b->ofpacts_len = tmp_len;
}

void
ofctrl_add_or_append_flow(.....)
{
...
...
..
if (!existing_conj && new_conj) {
   ...
   ...
   return;
}

if (existing_conj && !new_conj) {
    ofctrl_swap_flow_actions(f, existing);
} else {
    uint64_t compound_stub[64 / 8];
    struct ofpbuf compound;
    ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));

    ofpbuf_put(&compound, existing->flow.ofpacts,
existing->flow.ofpacts_len);
    ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);

    free(existing->flow.ofpacts);
    existing->flow.ofpacts = xmemdup(compound.data, compound.size);
    existing->flow.ofpacts_len = compound.size;

    ofpbuf_uninit(&compound);
}
desired_flow_destroy(f);
f = existing;
...
<**********************************************>

But I'm ok with the existing code too.

Thanks
Numan




>          ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
>
>          free(existing->flow.ofpacts);
> @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct
> ovn_desired_flow_table *flow_table,
>              /* Copy 'd' from 'flow_table' to installed_flows. */
>              i = installed_flow_dup(d);
>              hmap_insert(&installed_flows, &i->match_hmap_node,
> i->flow.hash);
> +        } else if (i->desired_flow != d) {
> +            /* If a matching installed flow was found but its actions are
> +             * conflicting with the desired flow actions, we should chose
> +             * to install the least restrictive one (i.e., no conjunctive
> +             * action).
> +             */
> +            bool i_conj = false;
> +            bool d_conj = false;
> +
> +            if (flow_actions_conflict(&i->flow, &d->flow, &i_conj,
> &d_conj) &&
> +                    i_conj && !d_conj) {
> +                flow_log_actions_conflict("Use new flow", &i->flow,
> &d->flow);
> +                installed_flow_mod(&i->flow, &d->flow, msgs);
> +                ovn_flow_log(&i->flow, "updating installed");
> +            }
>          }
>          link_installed_to_desired(i, d);
>      }
> @@ -1817,7 +1915,23 @@ 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. */
> +                 * flow, so just add it to the link.
> +                 *
> +                 * However, if the existing installed flow is less
> restrictive
> +                 * (i.e., no conjunctive action), we should chose it over
> the
> +                 * existing installed flow.
> +                 */
> +                bool i_conj = false;
> +                bool f_conj = false;
> +
> +                if (flow_actions_conflict(&i->flow, &f->flow, &i_conj,
> +                                          &f_conj) &&
> +                        i_conj && !f_conj) {
> +                    flow_log_actions_conflict("Use new flow",
> +                                              &i->flow, &f->flow);
> +                    ovn_flow_log(&i->flow, "updating installed
> (tracked)");
> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
> +                }
>                  link_installed_to_desired(i, f);
>              }
>              /* The track_list_node emptyness is used to check if the node
> is
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4e58722..118ed48 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13709,6 +13709,180 @@ 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 --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 a 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=44 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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
> +
> +# 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=44 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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])
> +
> +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 Tue, Sep 22, 2020 at 5:40 AM Numan Siddique <numans@ovn.org> wrote:

>
>
> On Fri, Sep 11, 2020 at 3:07 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 resolve conflicts between flows with the
>> same match and different actions by giving precedence to less
>> restrictive flows. This means that if the installed flow has action
>> "conjunction" and the desired flow doesn't then we prefer the desired
>> flow. Similarly, if the desired flow has action "conjunction" and the
>> installed flow doesn't then we prefer the already installed flow.
>>
>> CC: Daniel Alvarez <dalvarez@redhat.com>
>> CC: Han Zhou <hzhou@ovn.org>
>> CC: Mark Michelson <mmichels@redhat.com>
>> CC: Numan Siddique <numans@ovn.org>
>> Reported-at: https://bugzilla.redhat.com/1871931
>> Acked-by: Mark Michelson <mmichels@redhat.com>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> ---
>>
>
>
>
> Thanks Dumitru for the patch.
>
> The added test case fails  because of recent changes to the ACL table
> number. With the below changes, the test case passes.
>
> ***********************
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9d8ce3bb2e..11e7dfd160 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13808,12 +13808,12 @@ 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=44 | \
> +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(,45)
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> -priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
> +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)
>  ])
>
> @@ -13849,9 +13849,9 @@ 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=44 | \
> +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(,45)
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> **********************
>
> The patch LGTM with one minor comment.
>
> Acked-by: Numan Siddique <numans@ovn.org>
>
> I will wait for Han if he has any comments before applying this patch.
>
>
I will take another look at this tomorrow.

Thanks,
Han


> Thanks
> Numan
>
>
>> 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 | 124 +++++++++++++++++++++++++++++++++++--
>>  tests/ovn.at        | 174
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 293 insertions(+), 5 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 81a00c8..819e8cf 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table
>> *flow_table,
>>      ovs_list_insert(&stf->flows, &sfr->flow_list);
>>  }
>>
>> +static bool
>> +flow_action_has_conj(const struct ovn_flow *f)
>> +{
>> +    const struct ofpact *a = NULL;
>> +
>> +    OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) {
>> +        if (a->type == OFPACT_CONJUNCTION) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +/* Returns true if the actions of 'f1' cannot be merged with the actions
>> of
>> + * 'f2'. This is the case when one of the flow contains action
>> "conjunction"
>> + * and the other doesn't. The function always populates 'f1_has_conj' and
>> + * 'f2_has_conj'.
>> + */
>> +static bool
>> +flow_actions_conflict(const struct ovn_flow *f1, const struct ovn_flow
>> *f2,
>> +                      bool *f1_has_conj, bool *f2_has_conj)
>> +{
>> +    *f1_has_conj = flow_action_has_conj(f1);
>> +    *f2_has_conj = flow_action_has_conj(f2);
>> +
>> +    if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) {
>> +        return true;
>> +    }
>> +
>> +    if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static void
>> +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1,
>> +                          const struct ovn_flow *f2)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +
>> +    char *f1_s = ovn_flow_to_string(f1);
>> +    char *f2_s = ovn_flow_to_string(f2);
>> +
>> +    VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s, flow-2:
>> %s",
>> +                 msg, f1_s, f2_s);
>> +    free(f1_s);
>> +    free(f2_s);
>> +}
>> +
>>  /* Flow table interfaces to the rest of ovn-controller. */
>>
>>  /* Adds a flow to 'desired_flows' with the specified 'match' and
>> 'actions' to
>> @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct
>> ovn_desired_flow_table *desired_flows,
>>      struct desired_flow *existing;
>>      existing = desired_flow_lookup(desired_flows, &f->flow, NULL);
>>      if (existing) {
>> -        /* There's already a flow with this particular match. Append the
>> -         * action to that flow rather than adding a new flow
>> +        /* There's already a flow with this particular match. Try to
>> append
>> +         * the action to that flow rather than adding a new flow.
>> +         *
>> +         * If exactly one of the existing or new flow includes a
>> conjunction
>> +         * action then we can't merge the actions. In that case keep the
>> flow
>> +         * without conjunction action as this is the least restrictive
>> one.
>> +         */
>> +        bool existing_conj = false;
>> +        bool new_conj = false;
>> +
>> +        if (flow_actions_conflict(&existing->flow, &f->flow,
>> &existing_conj,
>> +                                  &new_conj)) {
>> +            flow_log_actions_conflict("Cannot merge conj with non-conj
>> action",
>> +                                      &existing->flow, &f->flow);
>> +        }
>> +
>> +        /* If the existing flow is less restrictive (i.e., no conjunction
>> +         * action) then keep it installed. Store however the flow with
>> +         * conjunction action too as it will be installed when the
>> current
>> +         * one is removed.
>>           */
>> +        if (!existing_conj && new_conj) {
>> +            hmap_insert(&desired_flows->match_flow_table,
>> &f->match_hmap_node,
>> +                        f->flow.hash);
>> +            link_flow_to_sb(desired_flows, f, sb_uuid);
>> +            return;
>> +        }
>> +
>>          uint64_t compound_stub[64 / 8];
>>          struct ofpbuf compound;
>>          ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
>> -        ofpbuf_put(&compound, existing->flow.ofpacts,
>> -                   existing->flow.ofpacts_len);
>> +
>> +        /* Only merge actions if both flows either have action
>> conjunction
>> +         * or non-conjunction. Otherwise use the actions in the new flow,
>> +         * the least restrictive.
>> +         */
>> +        if (existing_conj == new_conj) {
>> +            ofpbuf_put(&compound, existing->flow.ofpacts,
>> +                       existing->flow.ofpacts_len);
>> +        }
>>
>
> In the case where 'existing_conj'  is true and 'new_conj' is false, we can
> actually avoid the compound_stub
> I'd do something like below
>
> <*************************************>
>
> static void
> ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b)
> {
>     struct ofpact *tmp = a->ofpacts;
>     size_t tmp_len = a->ofpacts_len;
>
>     a->ofpacts = b->ofpacts;
>     a->ofpacts_len = b->ofpacts_len;
>
>     b->ofpacts = tmp;
>     b->ofpacts_len = tmp_len;
> }
>
> void
> ofctrl_add_or_append_flow(.....)
> {
> ...
> ...
> ..
> if (!existing_conj && new_conj) {
>    ...
>    ...
>    return;
> }
>
> if (existing_conj && !new_conj) {
>     ofctrl_swap_flow_actions(f, existing);
> } else {
>     uint64_t compound_stub[64 / 8];
>     struct ofpbuf compound;
>     ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
>
>     ofpbuf_put(&compound, existing->flow.ofpacts,
> existing->flow.ofpacts_len);
>     ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
>
>     free(existing->flow.ofpacts);
>     existing->flow.ofpacts = xmemdup(compound.data, compound.size);
>     existing->flow.ofpacts_len = compound.size;
>
>     ofpbuf_uninit(&compound);
> }
> desired_flow_destroy(f);
> f = existing;
> ...
> <**********************************************>
>
> But I'm ok with the existing code too.
>
> Thanks
> Numan
>
>
>
>
>>          ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
>>
>>          free(existing->flow.ofpacts);
>> @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct
>> ovn_desired_flow_table *flow_table,
>>              /* Copy 'd' from 'flow_table' to installed_flows. */
>>              i = installed_flow_dup(d);
>>              hmap_insert(&installed_flows, &i->match_hmap_node,
>> i->flow.hash);
>> +        } else if (i->desired_flow != d) {
>> +            /* If a matching installed flow was found but its actions are
>> +             * conflicting with the desired flow actions, we should chose
>> +             * to install the least restrictive one (i.e., no conjunctive
>> +             * action).
>> +             */
>> +            bool i_conj = false;
>> +            bool d_conj = false;
>> +
>> +            if (flow_actions_conflict(&i->flow, &d->flow, &i_conj,
>> &d_conj) &&
>> +                    i_conj && !d_conj) {
>> +                flow_log_actions_conflict("Use new flow", &i->flow,
>> &d->flow);
>> +                installed_flow_mod(&i->flow, &d->flow, msgs);
>> +                ovn_flow_log(&i->flow, "updating installed");
>> +            }
>>          }
>>          link_installed_to_desired(i, d);
>>      }
>> @@ -1817,7 +1915,23 @@ 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. */
>> +                 * flow, so just add it to the link.
>> +                 *
>> +                 * However, if the existing installed flow is less
>> restrictive
>> +                 * (i.e., no conjunctive action), we should chose it
>> over the
>> +                 * existing installed flow.
>> +                 */
>> +                bool i_conj = false;
>> +                bool f_conj = false;
>> +
>> +                if (flow_actions_conflict(&i->flow, &f->flow, &i_conj,
>> +                                          &f_conj) &&
>> +                        i_conj && !f_conj) {
>> +                    flow_log_actions_conflict("Use new flow",
>> +                                              &i->flow, &f->flow);
>> +                    ovn_flow_log(&i->flow, "updating installed
>> (tracked)");
>> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>> +                }
>>                  link_installed_to_desired(i, f);
>>              }
>>              /* The track_list_node emptyness is used to check if the
>> node is
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 4e58722..118ed48 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -13709,6 +13709,180 @@ 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 --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 a 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=44 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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
>> +
>> +# 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=44 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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])
>> +
>> +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 Thu, Sep 10, 2020 at 2:37 PM 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 resolve conflicts between flows with the
> same match and different actions by giving precedence to less
> restrictive flows. This means that if the installed flow has action
> "conjunction" and the desired flow doesn't then we prefer the desired
> flow. Similarly, if the desired flow has action "conjunction" and the
> installed flow doesn't then we prefer the already installed flow.
>
> CC: Daniel Alvarez <dalvarez@redhat.com>
> CC: Han Zhou <hzhou@ovn.org>
> CC: Mark Michelson <mmichels@redhat.com>
> CC: Numan Siddique <numans@ovn.org>
> Reported-at: https://bugzilla.redhat.com/1871931
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
> 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 | 124 +++++++++++++++++++++++++++++++++++--
>  tests/ovn.at        | 174
++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 293 insertions(+), 5 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 81a00c8..819e8cf 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table
*flow_table,
>      ovs_list_insert(&stf->flows, &sfr->flow_list);
>  }
>
> +static bool
> +flow_action_has_conj(const struct ovn_flow *f)
> +{
> +    const struct ofpact *a = NULL;
> +
> +    OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) {
> +        if (a->type == OFPACT_CONJUNCTION) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}

I think we should be more strict here. The only situation that is not
considered as conflict, i.e. flows can be combined, is when both flows have
only conjunction actions. If any of the flows has any actions other than
conjunction, we should consider them conflicting.

> +
> +/* Returns true if the actions of 'f1' cannot be merged with the actions
of
> + * 'f2'. This is the case when one of the flow contains action
"conjunction"
> + * and the other doesn't. The function always populates 'f1_has_conj' and
> + * 'f2_has_conj'.
> + */
> +static bool
> +flow_actions_conflict(const struct ovn_flow *f1, const struct ovn_flow
*f2,
> +                      bool *f1_has_conj, bool *f2_has_conj)
> +{
> +    *f1_has_conj = flow_action_has_conj(f1);
> +    *f2_has_conj = flow_action_has_conj(f2);
> +
> +    if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) {
> +        return true;
> +    }
> +
> +    if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void
> +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1,
> +                          const struct ovn_flow *f2)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +
> +    char *f1_s = ovn_flow_to_string(f1);
> +    char *f2_s = ovn_flow_to_string(f2);
> +
> +    VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s, flow-2:
%s",
> +                 msg, f1_s, f2_s);
> +    free(f1_s);
> +    free(f2_s);
> +}
> +
>  /* Flow table interfaces to the rest of ovn-controller. */
>
>  /* Adds a flow to 'desired_flows' with the specified 'match' and
'actions' to
> @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct
ovn_desired_flow_table *desired_flows,
>      struct desired_flow *existing;
>      existing = desired_flow_lookup(desired_flows, &f->flow, NULL);

Here I thought about another problem. Consider the following scenario:
1. flow1 with match m1 and action "drop" added
2. flow2 with match m1 and action "conjunction(...)" added because it is
considered as conflict with flow1
3. Now a flow3 with match m1 and action "conjunction(...)" comes, the above
lookup may find flow1 or flow2, but not both. If it finds flow1, it will
add flow3 as a separate flow. If it finds flow2, it will append and combine
with flow2, which is a random behavior.

The desired behavior in this case would be to combine flow3 with flow2,
because when flow1 is removed by the user, we expect the two conjunction
flows work as expected. So, in this case we cannot rely on
desired_flow_lookup(), but instead we should do a loop using
HMAP_FOR_EACH_WITH_HASH to find all desired flows with the match, and there
should at most one flow that is not conflict with the new flow, and in that
case we should combine with that flow.

>      if (existing) {
> -        /* There's already a flow with this particular match. Append the
> -         * action to that flow rather than adding a new flow
> +        /* There's already a flow with this particular match. Try to
append
> +         * the action to that flow rather than adding a new flow.
> +         *
> +         * If exactly one of the existing or new flow includes a
conjunction
> +         * action then we can't merge the actions. In that case keep the
flow
> +         * without conjunction action as this is the least restrictive
one.

This comment needs to be updated, because we are keeping both flows.

> +         */
> +        bool existing_conj = false;
> +        bool new_conj = false;
> +
> +        if (flow_actions_conflict(&existing->flow, &f->flow,
&existing_conj,
> +                                  &new_conj)) {
> +            flow_log_actions_conflict("Cannot merge conj with non-conj
action",
> +                                      &existing->flow, &f->flow);
> +        }
> +
> +        /* If the existing flow is less restrictive (i.e., no conjunction
> +         * action) then keep it installed. Store however the flow with
> +         * conjunction action too as it will be installed when the
current
> +         * one is removed.
>           */
> +        if (!existing_conj && new_conj) {
> +            hmap_insert(&desired_flows->match_flow_table,
&f->match_hmap_node,
> +                        f->flow.hash);
> +            link_flow_to_sb(desired_flows, f, sb_uuid);
> +            return;
> +        }
> +
The logic doesn't look right here. If there is a conflict, we should add
the flow to match_flow_table regardless of which one is more restrictive,
because we want one of them to get installed when the other one is removed.

>          uint64_t compound_stub[64 / 8];
>          struct ofpbuf compound;
>          ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
> -        ofpbuf_put(&compound, existing->flow.ofpacts,
> -                   existing->flow.ofpacts_len);
> +
> +        /* Only merge actions if both flows either have action
conjunction
> +         * or non-conjunction. Otherwise use the actions in the new flow,
> +         * the least restrictive.
> +         */
> +        if (existing_conj == new_conj) {
> +            ofpbuf_put(&compound, existing->flow.ofpacts,
> +                       existing->flow.ofpacts_len);
> +        }
>          ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
>
>          free(existing->flow.ofpacts);
> @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct
ovn_desired_flow_table *flow_table,
>              /* Copy 'd' from 'flow_table' to installed_flows. */
>              i = installed_flow_dup(d);
>              hmap_insert(&installed_flows, &i->match_hmap_node,
i->flow.hash);
> +        } else if (i->desired_flow != d) {
> +            /* If a matching installed flow was found but its actions are
> +             * conflicting with the desired flow actions, we should chose
> +             * to install the least restrictive one (i.e., no conjunctive
> +             * action).
> +             */
> +            bool i_conj = false;
> +            bool d_conj = false;
> +
> +            if (flow_actions_conflict(&i->flow, &d->flow, &i_conj,
&d_conj) &&
> +                    i_conj && !d_conj) {
> +                flow_log_actions_conflict("Use new flow", &i->flow,
&d->flow);
> +                installed_flow_mod(&i->flow, &d->flow, msgs);
> +                ovn_flow_log(&i->flow, "updating installed");
> +            }
>          }
>          link_installed_to_desired(i, d);
>      }
> @@ -1817,7 +1915,23 @@ 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. */
> +                 * flow, so just add it to the link.
> +                 *
> +                 * However, if the existing installed flow is less
restrictive
> +                 * (i.e., no conjunctive action), we should chose it
over the
> +                 * existing installed flow.
> +                 */
> +                bool i_conj = false;
> +                bool f_conj = false;
> +
> +                if (flow_actions_conflict(&i->flow, &f->flow, &i_conj,
> +                                          &f_conj) &&
> +                        i_conj && !f_conj) {
> +                    flow_log_actions_conflict("Use new flow",
> +                                              &i->flow, &f->flow);
> +                    ovn_flow_log(&i->flow, "updating installed
(tracked)");
> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
> +                }
>                  link_installed_to_desired(i, f);
>              }
>              /* The track_list_node emptyness is used to check if the
node is
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4e58722..118ed48 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13709,6 +13709,180 @@ 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 --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 a 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=44 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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
> +
> +# 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=44 | \
> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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])
> +
> +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 9/24/20 4:29 AM, Han Zhou wrote:
> 
> 
> On Thu, Sep 10, 2020 at 2:37 PM 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 resolve conflicts between flows with the
>> same match and different actions by giving precedence to less
>> restrictive flows. This means that if the installed flow has action
>> "conjunction" and the desired flow doesn't then we prefer the desired
>> flow. Similarly, if the desired flow has action "conjunction" and the
>> installed flow doesn't then we prefer the already installed flow.
>>
>> CC: Daniel Alvarez <dalvarez@redhat.com <mailto:dalvarez@redhat.com>>
>> CC: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
>> CC: Mark Michelson <mmichels@redhat.com <mailto:mmichels@redhat.com>>
>> CC: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>>
>> Reported-at: https://bugzilla.redhat.com/1871931
>> Acked-by: Mark Michelson <mmichels@redhat.com
> <mailto:mmichels@redhat.com>>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>>
>> ---
>> 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 | 124 +++++++++++++++++++++++++++++++++++--
>>  tests/ovn.at <http://ovn.at>        | 174
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 293 insertions(+), 5 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 81a00c8..819e8cf 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table
> *flow_table,
>>      ovs_list_insert(&stf->flows, &sfr->flow_list);
>>  }
>>
>> +static bool
>> +flow_action_has_conj(const struct ovn_flow *f)
>> +{
>> +    const struct ofpact *a = NULL;
>> +
>> +    OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) {
>> +        if (a->type == OFPACT_CONJUNCTION) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
> 
> I think we should be more strict here. The only situation that is not
> considered as conflict, i.e. flows can be combined, is when both flows
> have only conjunction actions. If any of the flows has any actions other
> than conjunction, we should consider them conflicting.
> 
>> +
>> +/* Returns true if the actions of 'f1' cannot be merged with the
> actions of
>> + * 'f2'. This is the case when one of the flow contains action
> "conjunction"
>> + * and the other doesn't. The function always populates 'f1_has_conj' and
>> + * 'f2_has_conj'.
>> + */
>> +static bool
>> +flow_actions_conflict(const struct ovn_flow *f1, const struct
> ovn_flow *f2,
>> +                      bool *f1_has_conj, bool *f2_has_conj)
>> +{
>> +    *f1_has_conj = flow_action_has_conj(f1);
>> +    *f2_has_conj = flow_action_has_conj(f2);
>> +
>> +    if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) {
>> +        return true;
>> +    }
>> +
>> +    if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static void
>> +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1,
>> +                          const struct ovn_flow *f2)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +
>> +    char *f1_s = ovn_flow_to_string(f1);
>> +    char *f2_s = ovn_flow_to_string(f2);
>> +
>> +    VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s, flow-2:
> %s",
>> +                 msg, f1_s, f2_s);
>> +    free(f1_s);
>> +    free(f2_s);
>> +}
>> +
>>  /* Flow table interfaces to the rest of ovn-controller. */
>>
>>  /* Adds a flow to 'desired_flows' with the specified 'match' and
> 'actions' to
>> @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct
> ovn_desired_flow_table *desired_flows,
>>      struct desired_flow *existing;
>>      existing = desired_flow_lookup(desired_flows, &f->flow, NULL);
> 
> Here I thought about another problem. Consider the following scenario:
> 1. flow1 with match m1 and action "drop" added
> 2. flow2 with match m1 and action "conjunction(...)" added because it is
> considered as conflict with flow1
> 3. Now a flow3 with match m1 and action "conjunction(...)" comes, the
> above lookup may find flow1 or flow2, but not both. If it finds flow1,
> it will add flow3 as a separate flow. If it finds flow2, it will append
> and combine with flow2, which is a random behavior.
> 
> The desired behavior in this case would be to combine flow3 with flow2,
> because when flow1 is removed by the user, we expect the two conjunction
> flows work as expected. So, in this case we cannot rely on
> desired_flow_lookup(), but instead we should do a loop using
> HMAP_FOR_EACH_WITH_HASH to find all desired flows with the match, and
> there should at most one flow that is not conflict with the new flow,
> and in that case we should combine with that flow.

Hi Han,

You're right, this is the correct way to fix the issue. I sent a v4
implementing this:

http://patchwork.ozlabs.org/project/ovn/patch/1601451527-19334-1-git-send-email-dceara@redhat.com/

Most of the changes in ofctrl_add_or_append_flow() are not needed
anymore because we now just make sure that we append to conjunctive flows.

Thanks,
Dumitru

> 
>>      if (existing) {
>> -        /* There's already a flow with this particular match. Append the
>> -         * action to that flow rather than adding a new flow
>> +        /* There's already a flow with this particular match. Try to
> append
>> +         * the action to that flow rather than adding a new flow.
>> +         *
>> +         * If exactly one of the existing or new flow includes a
> conjunction
>> +         * action then we can't merge the actions. In that case keep
> the flow
>> +         * without conjunction action as this is the least
> restrictive one.
> 
> This comment needs to be updated, because we are keeping both flows.
> 
>> +         */
>> +        bool existing_conj = false;
>> +        bool new_conj = false;
>> +
>> +        if (flow_actions_conflict(&existing->flow, &f->flow,
> &existing_conj,
>> +                                  &new_conj)) {
>> +            flow_log_actions_conflict("Cannot merge conj with
> non-conj action",
>> +                                      &existing->flow, &f->flow);
>> +        }
>> +
>> +        /* If the existing flow is less restrictive (i.e., no conjunction
>> +         * action) then keep it installed. Store however the flow with
>> +         * conjunction action too as it will be installed when the
> current
>> +         * one is removed.
>>           */
>> +        if (!existing_conj && new_conj) {
>> +            hmap_insert(&desired_flows->match_flow_table,
> &f->match_hmap_node,
>> +                        f->flow.hash);
>> +            link_flow_to_sb(desired_flows, f, sb_uuid);
>> +            return;
>> +        }
>> +
> The logic doesn't look right here. If there is a conflict, we should add
> the flow to match_flow_table regardless of which one is more
> restrictive, because we want one of them to get installed when the other
> one is removed.
> 
>>          uint64_t compound_stub[64 / 8];
>>          struct ofpbuf compound;
>>          ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
>> -        ofpbuf_put(&compound, existing->flow.ofpacts,
>> -                   existing->flow.ofpacts_len);
>> +
>> +        /* Only merge actions if both flows either have action
> conjunction
>> +         * or non-conjunction. Otherwise use the actions in the new flow,
>> +         * the least restrictive.
>> +         */
>> +        if (existing_conj == new_conj) {
>> +            ofpbuf_put(&compound, existing->flow.ofpacts,
>> +                       existing->flow.ofpacts_len);
>> +        }
>>          ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
>>
>>          free(existing->flow.ofpacts);
>> @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct
> ovn_desired_flow_table *flow_table,
>>              /* Copy 'd' from 'flow_table' to installed_flows. */
>>              i = installed_flow_dup(d);
>>              hmap_insert(&installed_flows, &i->match_hmap_node,
> i->flow.hash);
>> +        } else if (i->desired_flow != d) {
>> +            /* If a matching installed flow was found but its actions are
>> +             * conflicting with the desired flow actions, we should chose
>> +             * to install the least restrictive one (i.e., no conjunctive
>> +             * action).
>> +             */
>> +            bool i_conj = false;
>> +            bool d_conj = false;
>> +
>> +            if (flow_actions_conflict(&i->flow, &d->flow, &i_conj,
> &d_conj) &&
>> +                    i_conj && !d_conj) {
>> +                flow_log_actions_conflict("Use new flow", &i->flow,
> &d->flow);
>> +                installed_flow_mod(&i->flow, &d->flow, msgs);
>> +                ovn_flow_log(&i->flow, "updating installed");
>> +            }
>>          }
>>          link_installed_to_desired(i, d);
>>      }
>> @@ -1817,7 +1915,23 @@ 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. */
>> +                 * flow, so just add it to the link.
>> +                 *
>> +                 * However, if the existing installed flow is less
> restrictive
>> +                 * (i.e., no conjunctive action), we should chose it
> over the
>> +                 * existing installed flow.
>> +                 */
>> +                bool i_conj = false;
>> +                bool f_conj = false;
>> +
>> +                if (flow_actions_conflict(&i->flow, &f->flow, &i_conj,
>> +                                          &f_conj) &&
>> +                        i_conj && !f_conj) {
>> +                    flow_log_actions_conflict("Use new flow",
>> +                                              &i->flow, &f->flow);
>> +                    ovn_flow_log(&i->flow, "updating installed
> (tracked)");
>> +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>> +                }
>>                  link_installed_to_desired(i, f);
>>              }
>>              /* The track_list_node emptyness is used to check if the
> node is
>> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
>> index 4e58722..118ed48 100644
>> --- a/tests/ovn.at <http://ovn.at>
>> +++ b/tests/ovn.at <http://ovn.at>
>> @@ -13709,6 +13709,180 @@ 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 --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 a 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=44 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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
>> +
>> +# 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=44 | \
>> +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
>> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,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])
>> +
>> +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 9/22/20 2:40 PM, Numan Siddique wrote:
> 
> 
> On Fri, Sep 11, 2020 at 3:07 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 resolve conflicts between flows with the
>     same match and different actions by giving precedence to less
>     restrictive flows. This means that if the installed flow has action
>     "conjunction" and the desired flow doesn't then we prefer the desired
>     flow. Similarly, if the desired flow has action "conjunction" and the
>     installed flow doesn't then we prefer the already installed flow.
> 
>     CC: Daniel Alvarez <dalvarez@redhat.com <mailto:dalvarez@redhat.com>>
>     CC: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
>     CC: Mark Michelson <mmichels@redhat.com <mailto:mmichels@redhat.com>>
>     CC: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>>
>     Reported-at: https://bugzilla.redhat.com/1871931
>     Acked-by: Mark Michelson <mmichels@redhat.com
>     <mailto:mmichels@redhat.com>>
>     Signed-off-by: Dumitru Ceara <dceara@redhat.com
>     <mailto:dceara@redhat.com>>
>     ---
> 
> 
> 
> 
> Thanks Dumitru for the patch.
> 
> The added test case fails  because of recent changes to the ACL table
> number. With the below changes, the test case passes.

Thanks Numan, I sent a v4 of this patch and I fixed the table numbers in
the test.

> 
> ***********************
> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
> index 9d8ce3bb2e..11e7dfd160 100644
> --- a/tests/ovn.at <http://ovn.at>
> +++ b/tests/ovn.at <http://ovn.at>
> @@ -13808,12 +13808,12 @@ 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=44 | \
> +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(,45)
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> -priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
> +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)
>  ])
>  
> @@ -13849,9 +13849,9 @@ 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=44 | \
> +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(,45)
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
>  priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> **********************
> 
> The patch LGTM with one minor comment.
> 
> Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>>
> 
> I will wait for Han if he has any comments before applying this patch.
> 
> Thanks
> Numan
>  
> 
>     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 | 124 +++++++++++++++++++++++++++++++++++--
>      tests/ovn.at <http://ovn.at>        | 174
>     ++++++++++++++++++++++++++++++++++++++++++++++++++++
>      2 files changed, 293 insertions(+), 5 deletions(-)
> 
>     diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>     index 81a00c8..819e8cf 100644
>     --- a/controller/ofctrl.c
>     +++ b/controller/ofctrl.c
>     @@ -916,6 +916,57 @@ link_flow_to_sb(struct ovn_desired_flow_table
>     *flow_table,
>          ovs_list_insert(&stf->flows, &sfr->flow_list);
>      }
> 
>     +static bool
>     +flow_action_has_conj(const struct ovn_flow *f)
>     +{
>     +    const struct ofpact *a = NULL;
>     +
>     +    OFPACT_FOR_EACH (a, f->ofpacts, f->ofpacts_len) {
>     +        if (a->type == OFPACT_CONJUNCTION) {
>     +            return true;
>     +        }
>     +    }
>     +    return false;
>     +}
>     +
>     +/* Returns true if the actions of 'f1' cannot be merged with the
>     actions of
>     + * 'f2'. This is the case when one of the flow contains action
>     "conjunction"
>     + * and the other doesn't. The function always populates
>     'f1_has_conj' and
>     + * 'f2_has_conj'.
>     + */
>     +static bool
>     +flow_actions_conflict(const struct ovn_flow *f1, const struct
>     ovn_flow *f2,
>     +                      bool *f1_has_conj, bool *f2_has_conj)
>     +{
>     +    *f1_has_conj = flow_action_has_conj(f1);
>     +    *f2_has_conj = flow_action_has_conj(f2);
>     +
>     +    if ((*f1_has_conj) && !(*f2_has_conj) && f2->ofpacts_len) {
>     +        return true;
>     +    }
>     +
>     +    if ((*f2_has_conj) && !(*f1_has_conj) && f1->ofpacts_len) {
>     +        return true;
>     +    }
>     +
>     +    return false;
>     +}
>     +
>     +static void
>     +flow_log_actions_conflict(const char *msg, const struct ovn_flow *f1,
>     +                          const struct ovn_flow *f2)
>     +{
>     +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>     +
>     +    char *f1_s = ovn_flow_to_string(f1);
>     +    char *f2_s = ovn_flow_to_string(f2);
>     +
>     +    VLOG_WARN_RL(&rl, "Flow actions conflict: %s, flow-1: %s,
>     flow-2: %s",
>     +                 msg, f1_s, f2_s);
>     +    free(f1_s);
>     +    free(f2_s);
>     +}
>     +
>      /* Flow table interfaces to the rest of ovn-controller. */
> 
>      /* Adds a flow to 'desired_flows' with the specified 'match' and
>     'actions' to
>     @@ -985,14 +1036,46 @@ ofctrl_add_or_append_flow(struct
>     ovn_desired_flow_table *desired_flows,
>          struct desired_flow *existing;
>          existing = desired_flow_lookup(desired_flows, &f->flow, NULL);
>          if (existing) {
>     -        /* There's already a flow with this particular match.
>     Append the
>     -         * action to that flow rather than adding a new flow
>     +        /* There's already a flow with this particular match. Try
>     to append
>     +         * the action to that flow rather than adding a new flow.
>     +         *
>     +         * If exactly one of the existing or new flow includes a
>     conjunction
>     +         * action then we can't merge the actions. In that case
>     keep the flow
>     +         * without conjunction action as this is the least
>     restrictive one.
>     +         */
>     +        bool existing_conj = false;
>     +        bool new_conj = false;
>     +
>     +        if (flow_actions_conflict(&existing->flow, &f->flow,
>     &existing_conj,
>     +                                  &new_conj)) {
>     +            flow_log_actions_conflict("Cannot merge conj with
>     non-conj action",
>     +                                      &existing->flow, &f->flow);
>     +        }
>     +
>     +        /* If the existing flow is less restrictive (i.e., no
>     conjunction
>     +         * action) then keep it installed. Store however the flow with
>     +         * conjunction action too as it will be installed when the
>     current
>     +         * one is removed.
>               */
>     +        if (!existing_conj && new_conj) {
>     +            hmap_insert(&desired_flows->match_flow_table,
>     &f->match_hmap_node,
>     +                        f->flow.hash);
>     +            link_flow_to_sb(desired_flows, f, sb_uuid);
>     +            return;
>     +        }
>     +
>              uint64_t compound_stub[64 / 8];
>              struct ofpbuf compound;
>              ofpbuf_use_stub(&compound, compound_stub,
>     sizeof(compound_stub));
>     -        ofpbuf_put(&compound, existing->flow.ofpacts,
>     -                   existing->flow.ofpacts_len);
>     +
>     +        /* Only merge actions if both flows either have action
>     conjunction
>     +         * or non-conjunction. Otherwise use the actions in the new
>     flow,
>     +         * the least restrictive.
>     +         */
>     +        if (existing_conj == new_conj) {
>     +            ofpbuf_put(&compound, existing->flow.ofpacts,
>     +                       existing->flow.ofpacts_len);
>     +        }
> 
> 
> In the case where 'existing_conj'  is true and 'new_conj' is false, we
> can actually avoid the compound_stub
> I'd do something like below

In v4 I went for a bit of a different approach as suggested by Han and
we don't need this part anymore. Thanks though for the suggestion!

Regards,
Dumitru

> 
> <*************************************>
> 
> static void
> ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b)
> {
>     struct ofpact *tmp = a->ofpacts;
>     size_t tmp_len = a->ofpacts_len;
> 
>     a->ofpacts = b->ofpacts;
>     a->ofpacts_len = b->ofpacts_len;
> 
>     b->ofpacts = tmp;
>     b->ofpacts_len = tmp_len;
> }
> 
> void
> ofctrl_add_or_append_flow(.....)
> {
> ...
> ...
> ..
> if (!existing_conj && new_conj) {
>    ...
>    ...
>    return;
> }
> 
> if (existing_conj && !new_conj) {
>     ofctrl_swap_flow_actions(f, existing);
> } else {
>     uint64_t compound_stub[64 / 8];
>     struct ofpbuf compound;
>     ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub));
> 
>     ofpbuf_put(&compound, existing->flow.ofpacts,
> existing->flow.ofpacts_len);
>     ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
> 
>     free(existing->flow.ofpacts);
>     existing->flow.ofpacts = xmemdup(compound.data, compound.size);
>     existing->flow.ofpacts_len = compound.size;
> 
>     ofpbuf_uninit(&compound);
> }
> desired_flow_destroy(f);
> f = existing;
> ...
> <**********************************************>
> 
> But I'm ok with the existing code too.
> 
> Thanks
> Numan
> 
> 
>  
> 
>              ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len);
> 
>              free(existing->flow.ofpacts);
>     @@ -1687,6 +1770,21 @@ update_installed_flows_by_compare(struct
>     ovn_desired_flow_table *flow_table,
>                  /* Copy 'd' from 'flow_table' to installed_flows. */
>                  i = installed_flow_dup(d);
>                  hmap_insert(&installed_flows, &i->match_hmap_node,
>     i->flow.hash);
>     +        } else if (i->desired_flow != d) {
>     +            /* If a matching installed flow was found but its
>     actions are
>     +             * conflicting with the desired flow actions, we should
>     chose
>     +             * to install the least restrictive one (i.e., no
>     conjunctive
>     +             * action).
>     +             */
>     +            bool i_conj = false;
>     +            bool d_conj = false;
>     +
>     +            if (flow_actions_conflict(&i->flow, &d->flow, &i_conj,
>     &d_conj) &&
>     +                    i_conj && !d_conj) {
>     +                flow_log_actions_conflict("Use new flow", &i->flow,
>     &d->flow);
>     +                installed_flow_mod(&i->flow, &d->flow, msgs);
>     +                ovn_flow_log(&i->flow, "updating installed");
>     +            }
>              }
>              link_installed_to_desired(i, d);
>          }
>     @@ -1817,7 +1915,23 @@ 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. */
>     +                 * flow, so just add it to the link.
>     +                 *
>     +                 * However, if the existing installed flow is less
>     restrictive
>     +                 * (i.e., no conjunctive action), we should chose
>     it over the
>     +                 * existing installed flow.
>     +                 */
>     +                bool i_conj = false;
>     +                bool f_conj = false;
>     +
>     +                if (flow_actions_conflict(&i->flow, &f->flow, &i_conj,
>     +                                          &f_conj) &&
>     +                        i_conj && !f_conj) {
>     +                    flow_log_actions_conflict("Use new flow",
>     +                                              &i->flow, &f->flow);
>     +                    ovn_flow_log(&i->flow, "updating installed
>     (tracked)");
>     +                    installed_flow_mod(&i->flow, &f->flow, msgs);
>     +                }
>                      link_installed_to_desired(i, f);
>                  }
>                  /* The track_list_node emptyness is used to check if
>     the node is
>     diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
>     index 4e58722..118ed48 100644
>     --- a/tests/ovn.at <http://ovn.at>
>     +++ b/tests/ovn.at <http://ovn.at>
>     @@ -13709,6 +13709,180 @@ 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 --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 a 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=44 | \
>     +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>     +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>     +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>     actions=conjunction(2,1/2)
>     +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>     actions=conjunction(2,1/2)
>     +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,45)
>     +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
>     actions=conjunction(2,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
>     +
>     +# 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=44 | \
>     +    grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
>     +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,45)
>     +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
>     actions=conjunction(2,1/2)
>     +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
>     actions=conjunction(2,1/2)
>     +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
>     actions=conjunction(2,2/2)
>     +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
>     actions=conjunction(2,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])
>     +
>     +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 <mailto:dev@openvswitch.org>
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>