[ovs-dev] [PATCH 2/2] ovn: Support address sets generated from port groups
Daniel Alvarez Sanchez
dalvarez at redhat.com
Mon Mar 12 10:08:05 UTC 2018
LGTM! Just one tiny comment inline in the tests.
On Thu, Mar 1, 2018 at 4:37 AM, Han Zhou <zhouhan at gmail.com> wrote:
> Address sets are automatically generated from corresponding port
> groups, and can be used directly in ACL match conditions.
>
> There are two address sets generated for each port group:
>
> <port group name>_ip4
> <port group name>_ip6
>
> For example, if port_group1 is created, we can directly use below
> match condition in ACL:
> "outport == @port_group1 && ip4.src == $port_group1_ip4"
>
> This will simplify OVN client implementation, and avoid some tricky
> problems such as race conditions when maintaining address set
> memberships as discussed in the link below.
>
> Reported-by: Lucas Alvares Gomes <lucasagomes at gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> February/046174.html
> Signed-off-by: Han Zhou <hzhou8 at ebay.com>
> ---
> NEWS | 3 +-
> ovn/northd/ovn-northd.c | 87 ++++++++++++++++---
> ovn/ovn-nb.xml | 18 ++++
> ovn/ovn-sb.xml | 23 ++++-
> tests/ovn.at | 226 ++++++++++++++++++++++++++++++
> ++++++++++++++++++
> 5 files changed, 340 insertions(+), 17 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index da2c3ec..db98282 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,7 +15,8 @@ Post-v2.9.0
> - Linux kernel 4.14
> * Add support for compiling OVS with the latest Linux 4.14 kernel
> - OVN:
> - * Port_Group is supported in ACL match conditions.
> + * Port_Group and generated address sets are supported in ACL match
> + conditions. See ovn-nb(5) and ovn-sb(5) for details.
>
> v2.9.0 - 19 Feb 2018
> --------------------
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 2924d8f..11b9ab0 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -6098,9 +6098,32 @@ build_lflows(struct northd_context *ctx, struct
> hmap *datapaths,
> hmap_destroy(&mcgroups);
> }
>
> -/* OVN_Northbound and OVN_Southbound have an identical Address_Set table.
> - * We always update OVN_Southbound to match the current data in
> - * OVN_Northbound, so that the address sets used in Logical_Flows in
> +static void
> +sync_address_set(struct northd_context *ctx, const char *name,
> + const char **addrs, size_t n_addrs,
> + struct shash *sb_address_sets)
> +{
> + const struct sbrec_address_set *sb_address_set;
> + sb_address_set = shash_find_and_delete(sb_address_sets,
> + name);
> + if (!sb_address_set) {
> + sb_address_set = sbrec_address_set_insert(ctx->ovnsb_txn);
> + sbrec_address_set_set_name(sb_address_set, name);
> + }
> +
> + sbrec_address_set_set_addresses(sb_address_set,
> + addrs, n_addrs);
> +}
> +
> +/* OVN_Southbound Address_Set table contains same records as in north
> + * bound, plus the records generated from Port_Group table in north bound.
> + *
> + * There are 2 records generated from each port group, one for IPv4, and
> + * one for IPv6, named in the format: <port group name>_ip4 and
> + * <port group name>_ip6 respectively. MAC addresses are ignored.
> + *
> + * We always update OVN_Southbound to match the Address_Set and Port_Group
> + * in OVN_Northbound, so that the address sets used in Logical_Flows in
> * OVN_Southbound is checked against the proper set.*/
> static void
> sync_address_sets(struct northd_context *ctx)
> @@ -6112,19 +6135,55 @@ sync_address_sets(struct northd_context *ctx)
> shash_add(&sb_address_sets, sb_address_set->name, sb_address_set);
> }
>
> - const struct nbrec_address_set *nb_address_set;
> - NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
> - sb_address_set = shash_find_and_delete(&sb_address_sets,
> - nb_address_set->name);
> - if (!sb_address_set) {
> - sb_address_set = sbrec_address_set_insert(ctx->ovnsb_txn);
> - sbrec_address_set_set_name(sb_address_set,
> nb_address_set->name);
> + /* sync port group generated address sets first */
> + const struct nbrec_port_group *nb_port_group;
> + NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) {
> + const char **ipv4_addrs = xcalloc(1, sizeof *ipv4_addrs);
> + size_t n_ipv4_addrs = 0;
> + const char **ipv6_addrs = xcalloc(1, sizeof *ipv6_addrs);
> + size_t n_ipv6_addrs = 0;
> + for (size_t i = 0; i < nb_port_group->n_ports; i++) {
> + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses;
> j++) {
> + struct lport_addresses laddrs;
> + extract_lsp_addresses(nb_port_
> group->ports[i]->addresses[j],
> + &laddrs);
> + ipv4_addrs = xrealloc(ipv4_addrs,
> + (n_ipv4_addrs + laddrs.n_ipv4_addrs)
> + * sizeof *ipv4_addrs);
> + for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) {
> + ipv4_addrs[n_ipv4_addrs++] =
> + xstrdup(laddrs.ipv4_addrs[k].addr_s);
> + }
> + ipv6_addrs = xrealloc(ipv6_addrs,
> + (n_ipv6_addrs + laddrs.n_ipv6_addrs)
> + * sizeof *ipv6_addrs);
> + for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) {
> + ipv6_addrs[n_ipv6_addrs++] =
> + xstrdup(laddrs.ipv6_addrs[k].addr_s);
> + }
> + destroy_lport_addresses(&laddrs);
> + }
> }
> + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name);
> + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name);
> + sync_address_set(ctx, ipv4_addrs_name, ipv4_addrs, n_ipv4_addrs,
> + &sb_address_sets);
> + sync_address_set(ctx, ipv6_addrs_name, ipv6_addrs, n_ipv6_addrs,
> + &sb_address_sets);
> + free(ipv4_addrs_name);
> + free(ipv6_addrs_name);
> + free(ipv4_addrs);
> + free(ipv6_addrs);
> + }
>
> - sbrec_address_set_set_addresses(sb_address_set,
> - /* "char **" is not compatible with "const char **" */
> - (const char **) nb_address_set->addresses,
> - nb_address_set->n_addresses);
> + /* sync user defined address sets, which may overwrite port group
> + * generated address sets if same name is used */
> + const struct nbrec_address_set *nb_address_set;
> + NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
> + sync_address_set(ctx, nb_address_set->name,
> + /* "char **" is not compatible with "const char **" */
> + (const char **)nb_address_set->addresses,
> + nb_address_set->n_addresses, &sb_address_sets);
> }
>
> struct shash_node *node, *next;
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 83727c5..11b3e2b 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -936,6 +936,24 @@
> db="OVN_Southbound"/> database.
> </p>
>
> + <p>
> + For each port group, there are two address sets generated to the
> + <ref table="Address_Set" db="OVN_Southbound"/> table of the
> + <ref db="OVN_Southbound"/> database, containing the IP addresses
> + of the group of ports, one for IPv4, and the other for IPv6, with
> + <ref column="name" table="Address_Set" db="OVN_Southbound"/> being
> + the <ref column="name" table="Port_Group" db="OVN_Northbound"/>
> + of the <ref table="Port_Group" db="OVN_Northbound"/> followed by
> + a suffix <code>_ip4</code> for IPv4 and <code>_ip6</code> for IPv6.
> + The generated address sets can be used in the same way as regular
> + address sets in the <ref column="match" table="ACL"/> column
> + of the <ref table="ACL"/> table. For syntax information, see the
> details
> + of the expression language used for the <ref column="match"
> + table="Logical_Flow" db="OVN_Southbound"/> column in the <ref
> + table="Logical_Flow" db="OVN_Southbound"/> table of the <ref
> + db="OVN_Southbound"/> database.
> + </p>
> +
> <column name="name">
> A name for the port group. Names are ASCII and must match
> <code>[a-zA-Z_.][a-zA-Z_.0-9]*</code>.
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 2eac943..702ebef 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -368,9 +368,17 @@
>
> <table name="Address_Set" title="Address Sets">
> <p>
> - See the documentation for the <ref table="Address_Set"
> + This table contains address sets synced from the <ref
> table="Address_Set"
> db="OVN_Northbound"/> table in the <ref db="OVN_Northbound"/>
> database
> - for details.
> + and address sets generated from the <ref table="Port_Group"
> + db="OVN_Northbound"/> table in the <ref db="OVN_Northbound"/>
> database.
> + </p>
> +
> + <p>
> + See the documentation for the <ref table="Address_Set"
> + db="OVN_Northbound"/> table and <ref table="Port_Group"
> + db="OVN_Northbound"/> table in the <ref db="OVN_Northbound"/>
> + database for details.
> </p>
>
> <column name="name"/>
> @@ -790,6 +798,17 @@
> <code>@port_group1</code>.
> </p>
>
> + <p>
> + Additionally, you may refer to the set of addresses belonging to a
> + group of logical switch ports stored in the <ref
> table="Port_Group"/>
> + table by its <ref column="name" table="Port_Group"/> followed by
> + a suffix '_ip4'/'_ip6'. The IPv4 address set of a
> + <ref table="Port_Group"/> with a name of <code>port_group1</code>
> + can be referred to as <code>$port_group1_ip4</code>, and the IPv6
> + address set of the same <ref table="Port_Group"/> can be referred
> to
> + as <code>$port_group1_ip6</code>
> + </p>
> +
> <p><em>Miscellaneous</em></p>
>
> <p>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index c8ab7b7..16993de 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9363,3 +9363,229 @@ ra_test 000005dc 40 80 40
> aef00000000000000000000000000000 30 fd0f00000000000000
>
> OVN_CLEANUP([hv1],[hv2])
> AT_CLEANUP
> +
> +AT_SETUP([ovn -- Port Groups])
> +AT_KEYWORDS([ovnpg])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +# Logical network:
> +#
> +# Three logical switches ls1, ls2, ls3.
> +# One logical router lr0 connected to ls[123],
> +# with nine subnets, three per logical switch:
> +#
> +# lrp11 on ls1 for subnet 192.168.11.0/24
> +# lrp12 on ls1 for subnet 192.168.12.0/24
> +# lrp13 on ls1 for subnet 192.168.13.0/24
> +# ...
> +# lrp33 on ls3 for subnet 192.168.33.0/24
> +#
> +# 27 VIFs, 9 per LS, 3 per subnet: lp[123][123][123], where the first two
> +# digits are the subnet and the last digit distinguishes the VIF.
> +#
> +# This test will create two port groups and uses them in ACL.
> +
> +get_lsp_uuid () {
> + ovn-nbctl lsp-list ls${1%??} | grep lp$1 | awk '{ print $1 }'
> +}
> +
> +pg1_ports=
> +pg2_ports=
> +for i in 1 2 3; do
> + ovn-nbctl ls-add ls$i
> + for j in 1 2 3; do
> + for k in 1 2 3; do
> + ovn-nbctl \
> + -- lsp-add ls$i lp$i$j$k \
> + -- lsp-set-addresses lp$i$j$k "f0:00:00:00:0$i:$j$k \
> + 192.168.$i$j.$k"
> + # logical ports lp[12]?1 belongs to port group pg1
> + if test $i != 3 && test $k == 1; then
> + pg1_ports="$pg1_ports `get_lsp_uuid $i$j$k`"
> + fi
> + # logical ports lp[23]?2 belongs to port group pg2
> + if test $i != 1 && test $k == 2; then
> + pg2_ports="$pg2_ports `get_lsp_uuid $i$j$k`"
> + fi
> + done
> + done
> +done
> +
> +ovn-nbctl lr-add lr0
> +for i in 1 2 3; do
> + for j in 1 2 3; do
> + ovn-nbctl lrp-add lr0 lrp$i$j 00:00:00:00:ff:$i$j
> 192.168.$i$j.254/24
> + ovn-nbctl \
> + -- lsp-add ls$i lrp$i$j-attachment \
> + -- set Logical_Switch_Port lrp$i$j-attachment type=router \
> + options:router-port=lrp$i$j \
> + addresses='"00:00:00:00:ff:'$i$j'"'
> + done
> +done
> +
> +ovn-nbctl create Port_Group name=pg1 ports="$pg1_ports"
> +ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
> +
> +# create ACLs on all lswitches to drop traffic from pg2 to pg1
> +ovn-nbctl acl-add ls1 to-lport 1001 'outport == @pg1 && ip4.src ==
> $pg2_ip4' drop
> +ovn-nbctl acl-add ls2 to-lport 1001 'outport == @pg1 && ip4.src ==
> $pg2_ip4' drop
> +ovn-nbctl acl-add ls3 to-lport 1001 'outport == @pg1 && ip4.src ==
> $pg2_ip4' drop
> +
> +# Physical network:
> +#
> +# Three hypervisors hv[123].
> +# lp?1[123] spread across hv[123]: lp?11 on hv1, lp?12 on hv2, lp?13 on
> hv3.
> +# lp?2[123] spread across hv[23]: lp?21 and lp?22 on hv2, lp?23 on hv3.
> +# lp?3[123] all on hv3.
> +
> +# Given the name of a logical port, prints the name of the hypervisor
> +# on which it is located.
> +vif_to_hv() {
> + case $1 in dnl (
> + ?11) echo 1 ;; dnl (
> + ?12 | ?21 | ?22) echo 2 ;; dnl (
> + ?13 | ?23 | ?3?) echo 3 ;;
> + esac
> +}
> +
> +# Given the name of a logical port, prints the name of its logical router
> +# port, e.g. "vif_to_lrp 123" yields 12.
> +vif_to_lrp() {
> + echo ${1%?}
> +}
> +
> +# Given the name of a logical port, prints the name of its logical
> +# switch, e.g. "vif_to_ls 123" yields 1.
> +vif_to_ls() {
> + echo ${1%??}
> +}
> +
> +net_add n1
> +for i in 1 2 3; do
> + sim_add hv$i
> + as hv$i
> + ovs-vsctl add-br br-phys
> + ovn_attach n1 br-phys 192.168.0.$i
> +done
> +for i in 1 2 3; do
> + for j in 1 2 3; do
> + for k in 1 2 3; do
> + hv=`vif_to_hv $i$j$k`
> + as hv$hv ovs-vsctl \
> + -- add-port br-int vif$i$j$k \
> + -- set Interface vif$i$j$k \
> + external-ids:iface-id=lp$i$j$k \
> + options:tx_pcap=hv$hv/vif$i$j$k-tx.pcap \
> + options:rxq_pcap=hv$hv/vif$i$j$k-rx.pcap \
> + ofport-request=$i$j$k
> + done
> + done
> +done
> +
> +# Pre-populate the hypervisors' ARP tables so that we don't lose any
> +# packets for ARP resolution (native tunneling doesn't queue packets
> +# for ARP resolution).
> +OVN_POPULATE_ARP
> +
> +# Allow some time for ovn-northd and ovn-controller to catch up.
> +# XXX This should be more systematic.
> +sleep 1
> +
> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...
> +#
> +# This shell function causes a 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. 123 for
> vif123.
> +for i in 1 2 3; do
> + for j in 1 2 3; do
> + for k in 1 2 3; do
> + : > $i$j$k.expected
> + done
> + done
> +done
> +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
> + hv=hv`vif_to_hv $inport`
> + as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> + #as $hv ovs-appctl ofproto/trace br-int in_port=$inport $packet
> + in_ls=`vif_to_ls $inport`
> + in_lrp=`vif_to_lrp $inport`
> + for outport; do
> + out_ls=`vif_to_ls $outport`
> + if test $in_ls = $out_ls; then
> + # Ports on the same logical switch receive exactly the same
> packet.
> + echo $packet
> + else
> + # Routing decrements TTL and updates source and dest MAC
> + # (and checksum).
> + out_lrp=`vif_to_lrp $outport`
> + echo f00000000${outport}00000000ff$
> {out_lrp}08004500001c00000000"3f1101"00${src_ip}${dst_ip}0035111100080000
> + fi >> $outport.expected
> + done
> +}
>
Neat! this and test_arp are really useful functions to tests :)
> +
> +as hv1 ovs-vsctl --columns=name,ofport list interface
> +as hv1 ovn-sbctl list port_binding
> +as hv1 ovn-sbctl list datapath_binding
> +as hv1 ovn-sbctl list port_group
> +as hv1 ovn-sbctl list address_set
> +as hv1 ovn-sbctl dump-flows
> +as hv1 ovs-ofctl dump-flows br-int
>
nit: We can remove this block.
> +
> +# Send IP packets between all pairs of source and destination ports,
> +# packets matches ACL (pg2 to pg1) should be dropped
> +ip_to_hex() {
> + printf "%02x%02x%02x%02x" "$@"
> +}
> +for is in 1 2 3; do
> + for js in 1 2 3; do
> + for ks in 1 2 3; do
> + bcast=
> + s=$is$js$ks
> + smac=f00000000$s
> + sip=`ip_to_hex 192 168 $is$js $ks`
> + for id in 1 2 3; do
> + for jd in 1 2 3; do
> + for kd in 1 2 3; do
> + d=$id$jd$kd
> + dip=`ip_to_hex 192 168 $id$jd $kd`
> + if test $is = $id; then dmac=f00000000$d; else
> dmac=00000000ff$is$js; fi
> + if test $d != $s; then unicast=$d; else unicast=; fi
> +
> + # packets matches ACL should be dropped
> + if test $id != 3 && test $kd == 1; then
> + if test $is != 1 && test $ks == 2; then
> + unicast=
> + fi
> + fi
> + test_ip $s $smac $dmac $sip $dip $unicast #1
> + done
> + done
> + done
> + done
> + done
> +done
> +
> +# Allow some time for packet forwarding.
> +# XXX This can be improved.
> +sleep 1
> +
> +# Now check the packets actually received against the ones expected.
> +for i in 1 2 3; do
> + for j in 1 2 3; do
> + for k in 1 2 3; do
> + OVN_CHECK_PACKETS([hv`vif_to_hv $i$j$k`/vif$i$j$k-tx.pcap],
> + [$i$j$k.expected])
> + done
> + done
> +done
> +
> +# Gracefully terminate daemons
> +OVN_CLEANUP([hv1], [hv2], [hv3])
> +AT_CLEANUP
> --
> 2.1.0
>
Acked-by: Daniel Alvarez <dalvarez at redhat.com>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list