[ovs-dev] [PATCH ovn v7] ofctrl.c: Add a predictable resolution for conflicting flow actions.
Mark Gray
mark.d.gray at redhat.com
Thu Oct 15 17:32:20 UTC 2020
On 15/10/2020 10:12, Dumitru Ceara wrote:
> Until now, in case the ACL configuration generates openflows that have
> the same match but different actions, ovn-controller was using the
> following approach:
> 1. If the flow being added contains conjunctive actions, merge its
> actions with the already existing flow.
> 2. Otherwise, if the flow is being added incrementally
> (update_installed_flows_by_track), don't install the new flow but
> instead keep the old one.
> 3. Otherwise, (update_installed_flows_by_compare), don't install the
> new flow but instead keep the old one.
>
> Even though one can argue that having an ACL with a match that includes
> the match of another ACL is a misconfiguration, it can happen that the
> users provision OVN like this. Depending on the order of reading and
> installing the logical flows, the above operations can yield
> unpredictable results, e.g., allow specific traffic but then after
> ovn-controller is restarted (or a recompute happens) that specific
> traffic starts being dropped.
>
> A simple example of ACL configuration is:
> ovn-nbctl acl-add ls to-lport 3 '(ip4.src==10.0.0.1 ||
> ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> ovn-nbctl acl-add ls to-lport 3 'ip4.src==10.0.0.1' allow
> ovn-nbctl acl-add ls to-lport 2 'arp' allow
> ovn-nbctl acl-add ls to-lport 1 'ip4' drop
>
> This is a pattern used by most CMSs:
> - define a default deny policy.
> - punch holes in the default deny policy based on user specific
> configurations.
>
> Without this commit the behavior for traffic from 10.0.0.1 to 10.0.0.5
> is unpredictable. Depending on the order of operations traffic might be
> dropped or allowed.
>
> It's also quite hard to force the CMS to ensure that such match overlaps
> never occur.
>
> To address this issue we now ensure that all desired flows refering the
> same installed flow are partially sorted in the following way:
> - first all flows with action "allow".
> - then all flows with action "drop".
> - then a single flow with action "conjunction" (resulting from merging
> all flows with the same match and action conjunction).
>
> This ensures that "allow" flows have precedence over "drop" flows which
> in turn have precedence over "conjunction" flows. Essentially less
> restrictive flows are always preferred over more restrictive flows whenever a match
> conflict happens.
>
> CC: Daniel Alvarez <dalvarez at redhat.com>
> Reported-at: https://bugzilla.redhat.com/1871931
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> v7:
> - Address Han and Mark Gray's comments:
> - Maintain the desired_flow list sorted.
> v6:
> - First 3 patches of the v5 series were already applied.
> - Send patch 4 as v6 addressing Han's comments:
> - Instead of maintaining a sorted list, select active flow by walking
> the list of desired flows.
> - Add missing ovn_flow_log() call.
> v5:
> - Turn the patch into a series.
> - Address Han's comments.
> - Ensure predictable flow ordering in all cases (during incremental
> procesing or full recomputation).
> v4:
> - Address Han's comments:
> - make sure only flows with action conjunction are combined.
> v3:
> - Add Mark's ack.
> - Add last missing pcap check in the test.
> v2:
> - Address Han's comments:
> - Do not delete desired flow that cannot be merged, it might be
> installed later.
> - Fix typos in the commit log.
> - Update the test to check the OVS flows.
> ---
> controller/ofctrl.c | 74 ++++++++++++++++--
> tests/ovn.at | 214 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 283 insertions(+), 5 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index f444cae..39ead96 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -188,6 +188,14 @@ struct sb_flow_ref {
> * relationship is 1 to N. A link is added when a flow addition is processed.
> * A link is removed when a flow deletion is processed, the desired flow
> * table is cleared, or the installed flow table is cleared.
> + *
> + * To ensure predictable behavior, the list of desired flows is maintained
> + * partially sorted in the following way (from least restrictive to most
> + * restrictive wrt. match):
> + * - allow flows without action conjunction.
> + * - drop flows without action conjunction.
> + * - a single flow with action conjunction.
> + *
> * The first desired_flow in the list is the active one, the one that is
> * actually installed.
> */
> @@ -796,6 +804,12 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
> }
>
> static bool
> +flow_action_has_drop(const struct ovn_flow *f)
> +{
> + return f->ofpacts_len == 0;
> +}
> +
> +static bool
> flow_action_has_conj(const struct ovn_flow *f)
> {
> const struct ofpact *a = NULL;
> @@ -808,6 +822,33 @@ flow_action_has_conj(const struct ovn_flow *f)
> return false;
> }
>
> +static bool
> +flow_action_has_allow(const struct ovn_flow *f)
> +{
> + return !flow_action_has_drop(f) && !flow_action_has_conj(f);
> +}
> +
> +/* Returns true if flow 'a' is preferred over flow 'b'. */
> +static bool
> +flow_is_preferred(const struct ovn_flow *a, const struct ovn_flow *b)
> +{
> + if (flow_action_has_allow(b)) {
> + return false;
> + }
> + if (flow_action_has_allow(a)) {
> + return true;
> + }
> + if (flow_action_has_drop(b)) {
> + return false;
> + }
> + if (flow_action_has_drop(a)) {
> + return true;
> + }
> +
> + /* Flows 'a' and 'b' should never both have action conjuntion. *
Typo .. conjunction
> + OVS_NOT_REACHED();
> +}
> +
> /* Adds the desired flow to the list of desired flows that have same match
> * conditions as the installed flow.
> *
> @@ -820,8 +861,18 @@ flow_action_has_conj(const struct ovn_flow *f)
> static bool
> link_installed_to_desired(struct installed_flow *i, struct desired_flow *d)
> {
> + struct desired_flow *f;
> +
> + /* Find first 'f' such that 'd' is preferred over 'f'. If no such desired
> + * flow exists then 'f' will point after the last element of the list.
> + */
> + LIST_FOR_EACH (f, installed_ref_list_node, &i->desired_refs) {
> + if (flow_is_preferred(&d->flow, &f->flow)) {
> + break;
> + }
> + }
> + ovs_list_insert(&f->installed_ref_list_node, &d->installed_ref_list_node);
> d->installed_flow = i;
> - ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
> return installed_flow_get_active(i) == d;
> }
>
> @@ -1789,8 +1840,14 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table,
> link_installed_to_desired(i, d);
> } else if (!d->installed_flow) {
> /* This is a desired_flow that conflicts with one installed
> - * previously but not linked yet. */
> - link_installed_to_desired(i, d);
> + * previously but not linked yet. However, if this flow becomes
> + * active, e.g., it is less restrictive than the previous active
> + * flow then modify the installed flow.
> + */
> + if (link_installed_to_desired(i, d)) {
> + installed_flow_mod(&i->flow, &d->flow, msgs);
> + ovn_flow_log(&i->flow, "updating installed (conflict)");
> + }
> }
> }
> }
> @@ -1919,8 +1976,15 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,
> ovn_flow_log(&i->flow, "updating installed (tracked)");
> } else {
> /* Adding a new flow that conflicts with an existing installed
> - * flow, so just add it to the link. */
> - link_installed_to_desired(i, f);
> + * flow, so add it to the link. If this flow becomes active,
> + * e.g., it is less restrictive than the previous active flow
> + * then modify the installed flow.
> + */
> + if (link_installed_to_desired(i, f)) {
> + installed_flow_mod(&i->flow, &f->flow, msgs);
> + ovn_flow_log(&i->flow,
> + "updating installed (tracked conflict)");
> + }
> }
> /* The track_list_node emptyness is used to check if the node is
> * already added to track list, so initialize it again here. */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 488fd11..9420b1e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13727,6 +13727,220 @@ grep conjunction.*conjunction.*conjunction | wc -l`])
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
> +AT_SETUP([ovn -- Superseeding ACLs with conjunction])
> +ovn_start
> +
> +ovn-nbctl ls-add ls1
> +
> +ovn-nbctl lsp-add ls1 ls1-lp1 \
> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01"
> +
> +ovn-nbctl lsp-add ls1 ls1-lp2 \
> +-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02"
> +
> +net_add n1
> +sim_add hv1
> +
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \
> + options:tx_pcap=hv1/vif1-tx.pcap \
> + options:rxq_pcap=hv1/vif1-rx.pcap \
> + ofport-request=1
> +
> +ovs-vsctl -- add-port br-int hv1-vif2 -- \
> + set interface hv1-vif2 external-ids:iface-id=ls1-lp2 \
> + options:tx_pcap=hv1/vif2-tx.pcap \
> + options:rxq_pcap=hv1/vif2-rx.pcap \
> + ofport-request=2
> +
> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...
> +#
> +# This shell function causes an ip packet to be received on INPORT.
> +# The packet's content has Ethernet destination DST and source SRC
> +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits).
> +# The OUTPORTs (zero or more) list the VIFs on which the packet should
> +# be received. INPORT and the OUTPORTs are specified as logical switch
> +# port numbers, e.g. 11 for vif11.
> +test_ip() {
> + # This packet has bad checksums but logical L3 routing doesn't check.
> + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> + local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\
> +${dst_ip}0035111100080000
> + shift; shift; shift; shift; shift
> + as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
> + for outport; do
> + echo $packet >> $outport.expected
> + done
> +}
> +
> +ip_to_hex() {
> + printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +reset_pcap_file() {
> + local iface=$1
> + local pcap_file=$2
> + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> +options:rxq_pcap=dummy-rx.pcap
> + rm -f ${pcap_file}*.pcap
> + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \
> +options:rxq_pcap=${pcap_file}-rx.pcap
> +}
> +
> +# Add a default deny ACL and an allow ACL for specific IP traffic.
> +ovn-nbctl acl-add ls1 to-lport 2 'arp' allow
> +ovn-nbctl acl-add ls1 to-lport 1 'ip4' drop
> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.42) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> +ovn-nbctl --wait=hv sync
> +
> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed.
> +for src in `seq 1 2`; do
> + for dst in `seq 3 4`; do
> + sip=`ip_to_hex 10 0 0 $src`
> + dip=`ip_to_hex 10 0 0 $dst`
> +
> + test_ip 1 f00000000001 f00000000002 $sip $dip 2
> + done
> +done
> +
> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped.
> +dip=`ip_to_hex 10 0 0 5`
> +for src in `seq 1 2`; do
> + sip=`ip_to_hex 10 0 0 $src`
> +
> + test_ip 1 f00000000001 f00000000002 $sip $dip
> +done
> +
> +cat 2.expected > expout
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> +AT_CHECK([cat 2.packets], [0], [expout])
> +reset_pcap_file hv1-vif2 hv1/vif2
> +rm -f 2.packets
> +> 2.expected
> +
> +# Add two less restrictive allow ACLs for src IP 10.0.0.1.
> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' allow
> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the less restrictive flows should have been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed.
> +for src in `seq 1 2`; do
> + for dst in `seq 3 4`; do
> + sip=`ip_to_hex 10 0 0 $src`
> + dip=`ip_to_hex 10 0 0 $dst`
> +
> + test_ip 1 f00000000001 f00000000002 $sip $dip 2
> + done
> +done
> +
> +# Traffic 10.0.0.2 -> 10.0.0.5 should be dropped.
> +sip=`ip_to_hex 10 0 0 2`
> +dip=`ip_to_hex 10 0 0 5`
> +test_ip 1 f00000000001 f00000000002 $sip $dip
> +
> +# Traffic 10.0.0.1 -> 10.0.0.5 should be allowed.
> +sip=`ip_to_hex 10 0 0 1`
> +dip=`ip_to_hex 10 0 0 5`
> +test_ip 1 f00000000001 f00000000002 $sip $dip 2
> +
> +cat 2.expected > expout
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> +AT_CHECK([cat 2.packets], [0], [expout])
> +reset_pcap_file hv1-vif2 hv1/vif2
> +rm -f 2.packets
> +> 2.expected
> +
> +#sleep infinity
> +
> +# Remove the first less restrictive allow ACL.
> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1'
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the second less restrictive allow ACL should have been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# Remove the less restrictive allow ACL.
> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1'
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2),conjunction(3,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed.
> +for src in `seq 1 2`; do
> + for dst in `seq 3 4`; do
> + sip=`ip_to_hex 10 0 0 $src`
> + dip=`ip_to_hex 10 0 0 $dst`
> +
> + test_ip 1 f00000000001 f00000000002 $sip $dip 2
> + done
> +done
> +
> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped.
> +dip=`ip_to_hex 10 0 0 5`
> +for src in `seq 1 2`; do
> + sip=`ip_to_hex 10 0 0 $src`
> +
> + test_ip 1 f00000000001 f00000000002 $sip $dip
> +done
> +
> +cat 2.expected > expout
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
> +AT_CHECK([cat 2.packets], [0], [expout])
> +
> +# Re-add the less restrictive allow ACL for src IP 10.0.0.1
> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow
> +ovn-nbctl --wait=hv sync
> +
> +# Check OVS flows, the less restrictive flows should have been installed.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \
> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl
> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +
> # 3 hypervisors, one logical switch, 3 logical ports per hypervisor
> AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL])
> ovn_start
>
Small typo, otherwise looks good.
Acked-by: Mark Gray <mark.d.gray at redhat.com>
More information about the dev
mailing list