[ovs-dev] [PATCH ovn v3] ofctrl: Add a predictable resolution for conflicting flow actions.
Numan Siddique
numans at ovn.org
Tue Sep 22 12:40:45 UTC 2020
On Fri, Sep 11, 2020 at 3:07 AM Dumitru Ceara <dceara at redhat.com> wrote:
> Until now, in case the ACL configuration generates openflows that have
> the same match but different actions, ovn-controller was using the
> following approach:
> 1. If the flow being added contains conjunctive actions, merge its
> actions with the already existing flow.
> 2. Otherwise, if the flow is being added incrementally
> (update_installed_flows_by_track), don't install the new flow but
> instead keep the old one.
> 3. Otherwise, (update_installed_flows_by_compare), don't install the
> new flow but instead keep the old one.
>
> Even though one can argue that having an ACL with a match that includes
> the match of another ACL is a misconfiguration, it can happen that the
> users provision OVN like this. Depending on the order of reading and
> installing the logical flows, the above operations can yield
> unpredictable results, e.g., allow specific traffic but then after
> ovn-controller is restarted (or a recompute happens) that specific
> traffic starts being dropped.
>
> A simple example of ACL configuration is:
> ovn-nbctl acl-add ls to-lport 3 '(ip4.src==10.0.0.1 ||
> ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> ovn-nbctl acl-add ls to-lport 3 'ip4.src==10.0.0.1' allow
> ovn-nbctl acl-add ls to-lport 2 'arp' allow
> ovn-nbctl acl-add ls to-lport 1 'ip4' drop
>
> This is a pattern used by most CMSs:
> - define a default deny policy.
> - punch holes in the default deny policy based on user specific
> configurations.
>
> Without this commit the behavior for traffic from 10.0.0.1 to 10.0.0.5
> is unpredictable. Depending on the order of operations traffic might be
> dropped or allowed.
>
> It's also quite hard to force the CMS to ensure that such match overlaps
> never occur.
>
> To address this issue we now 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 at redhat.com>
> CC: Han Zhou <hzhou at ovn.org>
> CC: Mark Michelson <mmichels at redhat.com>
> CC: Numan Siddique <numans at ovn.org>
> Reported-at: https://bugzilla.redhat.com/1871931
> Acked-by: Mark Michelson <mmichels at redhat.com>
> Signed-off-by: Dumitru Ceara <dceara at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list