On 10/2/20 10:28 PM, Han Zhou wrote: > > Hi Dumitru, > Hi Han,
> Thanks for the revision. It looks good overall. The major problems left are: > 1. it missed updating the active desired_flow in the installed_flow. > 2. when a tracked flow deletion is handled, if there are other desired > flows linked to the same installed flow, it should use the same criteria > to decide which flow should become active from the candidate flows. > I see, you're right, thanks. I'll fix it in the next revision. > I also noticed 2 problems of the existing code while reviewing this > patch. I submitted 2 patches: > 1. > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > 2. > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > > 1) is required for your solution to work properly. 2) is not directly > related but will cause a merge conflict. Please help review both since > they are closely related to your patch. > I'll try to review your patches as soon as possible and I'll respin mine afterwards. > Please see more detailed comments below. > > On Wed, Sep 30, 2020 at 12:39 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >> CC: Han Zhou <[email protected] <mailto:[email protected]>> >> Reported-at: https://bugzilla.redhat.com/1871931 >> Acked-by: Mark Michelson <[email protected] > <mailto:[email protected]>> >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> >> --- >> 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 | 163 ++++++++++++++++++++++++++++++++++++------- >> tests/ovn.at <http://ovn.at> | 195 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 332 insertions(+), 26 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 81a00c8..4577413 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -206,6 +206,9 @@ struct installed_flow { >> struct desired_flow *desired_flow; >> }; >> >> +typedef bool >> +(*desired_flow_match_cb)(const struct desired_flow *candidate, >> + const void *arg); >> static struct desired_flow *desired_flow_alloc( >> uint8_t table_id, >> uint16_t priority, >> @@ -214,8 +217,14 @@ static struct desired_flow *desired_flow_alloc( >> const struct ofpbuf *actions); >> static struct desired_flow *desired_flow_lookup( >> struct ovn_desired_flow_table *, >> + const struct ovn_flow *target); >> +static struct desired_flow *desired_flow_lookup_by_uuid( > > The name "by_uuid" is a little misleading. It sounds like it only looks > uuid. But instead, it still looks up by match which it will also compare > uuid. How about: desired_flow_lookup_check_uuid? > >> + struct ovn_desired_flow_table *, >> const struct ovn_flow *target, >> - const struct uuid *sb_uuid); >> + const struct uuid *); >> +static struct desired_flow *desired_flow_lookup_conjunctive( >> + struct ovn_desired_flow_table *, >> + const struct ovn_flow *target); >> static void desired_flow_destroy(struct desired_flow *); >> >> static struct installed_flow *installed_flow_lookup( >> @@ -916,6 +925,33 @@ 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; >> +} >> + >> +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); >> +} >> + > > In this log it would be better to mention which one is selected. It > would be better not only abstracting the logging into a function but > also the logic of comparing flows and determining which one is selected. > Ack, sounds better indeed. >> /* Flow table interfaces to the rest of ovn-controller. */ >> >> /* Adds a flow to 'desired_flows' with the specified 'match' and > 'actions' to >> @@ -939,7 +975,7 @@ ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *flow_table, >> struct desired_flow *f = desired_flow_alloc(table_id, priority, > cookie, >> match, actions); >> >> - if (desired_flow_lookup(flow_table, &f->flow, sb_uuid)) { >> + if (desired_flow_lookup_by_uuid(flow_table, &f->flow, sb_uuid)) { >> if (log_duplicate_flow) { >> static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); >> if (!VLOG_DROP_DBG(&rl)) { >> @@ -979,14 +1015,15 @@ ofctrl_add_or_append_flow(struct > ovn_desired_flow_table *desired_flows, >> const struct ofpbuf *actions, >> const struct uuid *sb_uuid) >> { >> - struct desired_flow *f = desired_flow_alloc(table_id, priority, > cookie, >> - match, actions); >> - >> struct desired_flow *existing; >> - existing = desired_flow_lookup(desired_flows, &f->flow, NULL); >> + struct desired_flow *f; >> + >> + f = desired_flow_alloc(table_id, priority, cookie, match, actions); >> + existing = desired_flow_lookup_conjunctive(desired_flows, &f->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 and action >> + * 'conjunction'. Append the action to that flow rather than >> + * adding a new flow. >> */ >> uint64_t compound_stub[64 / 8]; >> struct ofpbuf compound; >> @@ -1225,15 +1262,11 @@ installed_flow_dup(struct desired_flow *src) >> return dst; >> } >> >> -/* Finds and returns a desired_flow in 'flow_table' whose key is > identical to >> - * 'target''s key, or NULL if there is none. >> - * >> - * If sb_uuid is not NULL, the function will also check if the found > flow is >> - * referenced by the sb_uuid. */ >> static struct desired_flow * >> -desired_flow_lookup(struct ovn_desired_flow_table *flow_table, >> - const struct ovn_flow *target, >> - const struct uuid *sb_uuid) >> +desired_flow_lookup__(struct ovn_desired_flow_table *flow_table, >> + const struct ovn_flow *target, >> + desired_flow_match_cb match_cb, >> + const void *arg) >> { >> struct desired_flow *d; >> HMAP_FOR_EACH_WITH_HASH (d, match_hmap_node, target->hash, >> @@ -1242,20 +1275,75 @@ desired_flow_lookup(struct > ovn_desired_flow_table *flow_table, >> if (f->table_id == target->table_id >> && f->priority == target->priority >> && minimatch_equal(&f->match, &target->match)) { >> - if (!sb_uuid) { >> + >> + if (!match_cb || match_cb(d, arg)) { >> return d; >> } >> - struct sb_flow_ref *sfr; >> - LIST_FOR_EACH (sfr, sb_list, &d->references) { >> - if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { >> - return d; >> - } >> - } >> } >> } >> return NULL; >> } >> >> +/* Finds and returns a desired_flow in 'flow_table' whose key is > identical to >> + * 'target''s key, or NULL if there is none. >> + */ >> +static struct desired_flow * >> +desired_flow_lookup(struct ovn_desired_flow_table *flow_table, >> + const struct ovn_flow *target) >> +{ >> + return desired_flow_lookup__(flow_table, target, NULL, NULL); >> +} >> + >> +static bool >> +flow_lookup_match_uuid(const struct desired_flow *candidate, const > void *arg) > > This function is used as a callback, so it would be better to have _cb > in its name to help code reading, e.g. flow_lookup_cb_match_uuid > Sure. >> +{ >> + const struct uuid *sb_uuid = arg; >> + struct sb_flow_ref *sfr; >> + >> + LIST_FOR_EACH (sfr, sb_list, &candidate->references) { >> + if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { >> + return true; >> + } >> + } >> + return false; >> +} >> + >> +/* Finds and returns a desired_flow in 'flow_table' whose key is > identical to >> + * 'target''s key, or NULL if there is none. >> + * >> + * The function will also check if the found flow is referenced by the >> + * 'sb_uuid'. >> + */ >> +static struct desired_flow * >> +desired_flow_lookup_by_uuid(struct ovn_desired_flow_table *flow_table, >> + const struct ovn_flow *target, >> + const struct uuid *sb_uuid) >> +{ >> + return desired_flow_lookup__(flow_table, target, > flow_lookup_match_uuid, >> + sb_uuid); >> +} >> + >> +static bool >> +flow_lookup_match_conj(const struct desired_flow *candidate, >> + const void *arg OVS_UNUSED) > > Same as above of the naming. > Ack. >> +{ >> + return flow_action_has_conj(&candidate->flow); >> +} >> + >> +/* Finds and returns a desired_flow in 'flow_table' whose key is > identical to >> + * 'target''s key, or NULL if there is none. >> + * >> + * The function will only return a matching flow if it contains action >> + * 'conjunction'. >> + */ >> +static struct desired_flow * >> +desired_flow_lookup_conjunctive(struct ovn_desired_flow_table > *flow_table, >> + const struct ovn_flow *target) >> +{ >> + return desired_flow_lookup__(flow_table, target, > flow_lookup_match_conj, >> + NULL); >> +} >> + >> /* Finds and returns an installed_flow in installed_flows whose key is >> * identical to 'target''s key, or NULL if there is none. */ >> static struct installed_flow * >> @@ -1653,8 +1741,7 @@ update_installed_flows_by_compare(struct > ovn_desired_flow_table *flow_table, >> struct installed_flow *i, *next; >> HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { >> unlink_all_refs_for_installed_flow(i); >> - struct desired_flow *d = >> - desired_flow_lookup(flow_table, &i->flow, NULL); >> + struct desired_flow *d = desired_flow_lookup(flow_table, > &i->flow); >> if (!d) { >> /* Installed flow is no longer desirable. Delete it from the >> * switch and from installed_flows. */ >> @@ -1687,6 +1774,18 @@ 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). >> + */ >> + if (flow_action_has_conj(&i->flow) && >> + !flow_action_has_conj(&d->flow)) { >> + 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"); >> + } > > Here is a problem. Before this change, the flow actually installed in > OVS is the one found in the previous loop. Now in this case we change > the flow installed, we should update the i->desired_flow to match the > one selected. > Right, thanks for spotting this. >> } >> link_installed_to_desired(i, d); >> } >> @@ -1817,7 +1916,19 @@ 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. >> + */ >> + if (flow_action_has_conj(&i->flow) && >> + !flow_action_has_conj(&f->flow)) { >> + 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); >> + } > > We should make sure it is set as the active flow. > > In addition, in the other branch in this function when the flow is > deleted, we need to use the same criteria to select the flow to be > actively installed. Currently unlink_installed_to_desired() just pick > the first one from the rest of desired_flows in the linked list. This > patch should go through the list and figure out which one should be > selected using the same criteria. > I think I also need to double check the self test. I thought I was covering this case too but I guess I was wrong. Thanks, Dumitru > Thanks, > Han > >> 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 7769b69..6b77057 100644 >> --- a/tests/ovn.at <http://ovn.at> >> +++ b/tests/ovn.at <http://ovn.at> >> @@ -13709,6 +13709,201 @@ grep conjunction.*conjunction.*conjunction | > wc -l`]) >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> >> +AT_SETUP([ovn -- Superseeding ACLs with conjunction]) >> +ovn_start >> + >> +ovn-nbctl ls-add ls1 >> + >> +ovn-nbctl lsp-add ls1 ls1-lp1 \ >> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" >> + >> +ovn-nbctl lsp-add ls1 ls1-lp2 \ >> +-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02" >> + >> +net_add n1 >> +sim_add hv1 >> + >> +as hv1 >> +ovs-vsctl add-br br-phys >> +ovn_attach n1 br-phys 192.168.0.1 >> +ovs-vsctl -- add-port br-int hv1-vif1 -- \ >> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap \ >> + options:rxq_pcap=hv1/vif1-rx.pcap \ >> + ofport-request=1 >> + >> +ovs-vsctl -- add-port br-int hv1-vif2 -- \ >> + set interface hv1-vif2 external-ids:iface-id=ls1-lp2 \ >> + options:tx_pcap=hv1/vif2-tx.pcap \ >> + options:rxq_pcap=hv1/vif2-rx.pcap \ >> + ofport-request=2 >> + >> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... >> +# >> +# This shell function causes an ip packet to be received on INPORT. >> +# The packet's content has Ethernet destination DST and source SRC >> +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits). >> +# The OUTPORTs (zero or more) list the VIFs on which the packet should >> +# be received. INPORT and the OUTPORTs are specified as logical switch >> +# port numbers, e.g. 11 for vif11. >> +test_ip() { >> + # This packet has bad checksums but logical L3 routing doesn't check. >> + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 >> + local > packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\ >> +${dst_ip}0035111100080000 >> + shift; shift; shift; shift; shift >> + as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet >> + for outport; do >> + echo $packet >> $outport.expected >> + done >> +} >> + >> +ip_to_hex() { >> + printf "%02x%02x%02x%02x" "$@" >> +} >> + >> +reset_pcap_file() { >> + local iface=$1 >> + local pcap_file=$2 >> + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \ >> +options:rxq_pcap=dummy-rx.pcap >> + rm -f ${pcap_file}*.pcap >> + ovs-vsctl -- set Interface $iface > options:tx_pcap=${pcap_file}-tx.pcap \ >> +options:rxq_pcap=${pcap_file}-rx.pcap >> +} >> + >> +# Add a default deny ACL and an allow ACL for specific IP traffic. >> +ovn-nbctl acl-add ls1 to-lport 2 'arp' allow >> +ovn-nbctl acl-add ls1 to-lport 1 'ip4' drop >> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || > ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow >> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || > ip4.src==10.0.0.42) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow >> +ovn-nbctl --wait=hv sync >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. >> +dip=`ip_to_hex 10 0 0 5` >> +for src in `seq 1 2`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip >> +done >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> +reset_pcap_file hv1-vif2 hv1/vif2 >> +rm -f 2.packets >> +> 2.expected >> + >> +# Add 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=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.2 -> 10.0.0.5 should be dropped. >> +sip=`ip_to_hex 10 0 0 2` >> +dip=`ip_to_hex 10 0 0 5` >> +test_ip 1 f00000000001 f00000000002 $sip $dip >> + >> +# Traffic 10.0.0.1 -> 10.0.0.5 should be allowed. >> +sip=`ip_to_hex 10 0 0 1` >> +dip=`ip_to_hex 10 0 0 5` >> +test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> +reset_pcap_file hv1-vif2 hv1/vif2 >> +rm -f 2.packets >> +> 2.expected >> + >> +# Remove the less restrictive allow ACL. >> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1' >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 > actions=conjunction(2,2/2),conjunction(3,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. >> +dip=`ip_to_hex 10 0 0 5` >> +for src in `seq 1 2`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip >> +done >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> + >> +# Re-add the less restrictive allow ACL for src IP 10.0.0.1 >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the less restrictive flows should have been installed. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +OVN_CLEANUP([hv1]) >> +AT_CLEANUP >> + >> # 3 hypervisors, one logical switch, 3 logical ports per hypervisor >> AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL]) >> ovn_start >> -- >> 1.8.3.1 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
