[ovs-dev,2/2] ovn: Support address sets generated from port groups

Message ID 1519875463-65690-2-git-send-email-hzhou8@ebay.com
State Superseded
Headers show
Series
  • [ovs-dev,1/2] ovn: Support port groups in ACLs
Related show

Commit Message

Han Zhou March 1, 2018, 3:37 a.m.
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@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-February/046174.html
Signed-off-by: Han Zhou <hzhou8@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(-)

Comments

Daniel Alvarez Sanchez March 1, 2018, 3:22 p.m. | #1
Hi Han,

I've tested both patches today on a lab setup and from a performance
perspective there's no big difference after merging my IDL patch with
800 ports. However, I must say that this it's greatly improving the
reconnection times to OVSDB (now the dumps from ovsdb-server upon
reconnection are much smaller since we're cutting down the number
of ACL's *a lot*). Over all, it works *GREAT*! Thanks a lot.

I'll do another pass focusing only on the code now that I tested the
behavior.

In order to help other reviewers, I'm sharing a script below based
on your tests that I used to play around, inspect inserted flows, stop
northd/controller to check if the updates took place, etc..

-----------------------------------

add_phys_port() {
    name=$1
    mac=$2
    ip=$3
    mask=$4
    gw=$5
    iface_id=$6
    ip netns add $name
    ovs-vsctl add-port br-int $name -- set interface $name type=internal
    ip link set $name netns $name
    ip netns exec $name ip link set $name address $mac
    ip netns exec $name ip addr add $ip/$mask dev $name
    ip netns exec $name ip link set $name up
    ip netns exec $name ip route add default via $gw
    ovs-vsctl set Interface $name external_ids:iface-id=$iface_id
}


ovn-nbctl ls-add lsw0

for i in 1 2 3; do
    for j in 1 2 3; do
        ovn-nbctl lsp-add lsw0 lp$i$j
        add_phys_port lp$i$j 02:00:00:00:00:$i$j 192.168.0.$i$j 24
192.168.0.254 lp$i$j
        if test $j = 1; then
            ovn-nbctl lsp-set-addresses lp$i$j "02:00:00:00:00:$i$j
192.168.0.$i$j" unknown
        else
            if test $j = 3; then
                ip_addres="192.168.0.$i$j fe80::ea2a:eaff:fe28:$i$j/64
192.169.0.$i$j"
                ip netns exec lp$i$j ip addr add 192.169.0.$i$j/24 dev
lp$i$j
    else
                ip_addrs="192.168.0.$i$j"
            fi
            ovn-nbctl lsp-set-addresses lp$i$j "02:00:00:00:00:$i$j
$ip_addrs"
            ovn-nbctl lsp-set-port-security lp$i$j 02:00:00:00:00:$i$j
        fi
    done
done

get_lsp_uuid () {
    ovn-nbctl lsp-list lsw0 | grep $1 | awk '{ print $1 }'
}

ovn-nbctl create Port_Group name=pg1 ports=`get_lsp_uuid
lp22`,`get_lsp_uuid lp33`
ovn-nbctl create Port_Group name=pg2 ports=`get_lsp_uuid
lp11`,`get_lsp_uuid lp31`

ovn-nbctl acl-add lsw0 to-lport 1077 'outport == @pg1 && ip && icmp' drop
ovn-nbctl acl-add lsw0 to-lport 1077 'ip4.src == $pg2_ip4 && ip && icmp'
drop

-----------------------------------

These are the generated flows associated for the installed ACLs:

 cookie=0x68b73f3d, duration=219.014s, table=44, n_packets=0, n_bytes=0,
idle_age=219, priority=2002,icmp6,reg15=0x9,metadata=0xf actions=drop
 cookie=0x68b73f3d, duration=219.014s, table=44, n_packets=8, n_bytes=784,
idle_age=2, priority=2002,icmp,reg15=0x9,metadata=0xf actions=drop
 cookie=0x68b73f3d, duration=219.014s, table=44, n_packets=6, n_bytes=588,
idle_age=198, priority=2002,icmp,reg15=0x5,metadata=0xf actions=drop
 cookie=0x68b73f3d, duration=219.014s, table=44, n_packets=0, n_bytes=0,
idle_age=219, priority=2002,icmp6,reg15=0x5,metadata=0xf actions=drop

 cookie=0xa4e8c7cc, duration=118.575s, table=44, n_packets=1, n_bytes=98,
idle_age=105, priority=2077,icmp,metadata=0xf,nw_src=192.168.0.11
actions=drop
 cookie=0xa4e8c7cc, duration=118.575s, table=44, n_packets=0, n_bytes=0,
idle_age=118, priority=2077,icmp,metadata=0xf,nw_src=192.168.0.31
actions=drop


Excellent work, really!
Daniel

On Thu, Mar 1, 2018 at 4:37 AM, Han Zhou <zhouhan@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@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> February/046174.html
> Signed-off-by: Han Zhou <hzhou8@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
> +}
> +
> +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
> +
> +# 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
>

Tested-By: Daniel Alvarez <dalvarez@redhat.com>

>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Daniel Alvarez Sanchez March 12, 2018, 10:08 a.m. | #2
LGTM! Just one tiny comment inline in the tests.


On Thu, Mar 1, 2018 at 4:37 AM, Han Zhou <zhouhan@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@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> February/046174.html
> Signed-off-by: Han Zhou <hzhou8@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@redhat.com>

>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Han Zhou March 12, 2018, 7:49 p.m. | #3
On Mon, Mar 12, 2018 at 3:08 AM, Daniel Alvarez Sanchez <dalvarez@redhat.com>
wrote:
>
> LGTM! Just one tiny comment inline in the tests.
>
>
> On Thu, Mar 1, 2018 at 4:37 AM, Han Zhou <zhouhan@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@gmail.com>
>> Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-February/046174.html
>> Signed-off-by: Han Zhou <hzhou8@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.
It was for debugging. I can remove it, but it may still be helpful for
debugging in case it fails.
>
>
> Acked-by: Daniel Alvarez <dalvarez@redhat.com>
>>
Thanks for the review! :)

Patch

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
+}
+
+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
+
+# 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