[ovs-dev] [PATCH ovn v7] ofctrl.c: Add a predictable resolution for conflicting flow actions.
Han Zhou
hzhou at ovn.org
Thu Oct 15 18:05:21 UTC 2020
On Thu, Oct 15, 2020 at 10:32 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> 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>
>
Thanks Dumitru and Mark. I applied this to master with the small typo
mentioned by Mark fixed.
Han
....... 8><
.................................................................. ><8
..............
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 39ead968d..79529d13c 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -845,7 +845,7 @@ flow_is_preferred(const struct ovn_flow *a, const
struct ovn_flow *b)
return true;
}
- /* Flows 'a' and 'b' should never both have action conjuntion. */
+ /* Flows 'a' and 'b' should never both have action conjunction. */
OVS_NOT_REACHED();
}
More information about the dev
mailing list