[ovs-dev] ovn: Add LB flows for logical router with gateway port

From: Numan Siddique <nusiddiq@redhat.com>

This patch adds support for associating a load balancer to a
logical router with gateway router port which was missing earlier.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/northd/ovn-northd.8.xml |  70 +++++++++++++++-------
 ovn/northd/ovn-northd.c     |  82 +++++++++++++++++++++++---
 tests/system-ovn.at         | 141 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+), 29 deletions(-)

On Thu, Sep 21, 2017 at 09:22:16PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> This patch adds support for associating a load balancer to a
> logical router with gateway router port which was missing earlier.
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

It doesn't look like this patch got any reviews.  It's not my area of
expertise.  Guru, is this something that you could look at?

I've had a look over this and it looks fine by me. Ben, do you plan to
merge this soon? If so, I'll need to update my IPv6 load balancer patch
since it will conflict with this a bit.

On Thu, Sep 21, 2017 at 10:53 AM <nusiddiq@redhat.com> wrote:

> From: Numan Siddique <nusiddiq@redhat.com>
>
> This patch adds support for associating a load balancer to a
> logical router with gateway router port which was missing earlier.
>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>
Acked-by: Mark Michelson <mmichels@redhat.com>

> ---
>  ovn/northd/ovn-northd.8.xml |  70 +++++++++++++++-------
>  ovn/northd/ovn-northd.c     |  82 +++++++++++++++++++++++---
>  tests/system-ovn.at         | 141
> ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+), 29 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 0d85ec0d2..2798369ce 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -1459,61 +1459,69 @@ icmp4 {
>        in the reverse direction needs to be unDNATed.
>      </p>
>
> -    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> +    <p> Ingress Table 4: Load balancing DNAT rules </p>
> +
> +    <p>
> +      Following load balancing DNAT flows are added for Gateway router or
> +      Router with gateway port. These flows are programmed only on the
> +      <code>redirect-chassis</code>.
> +    </p>
>
>      <ul>
>        <li>
> -        For all the configured load balancing rules for Gateway router in
> -        <code>OVN_Northbound</code> database that includes a L4 port
> -        <var>PORT</var> of protocol <var>P</var> and IPv4 address
> -        <var>VIP</var>, a priority-120 flow that matches on
> +        For all the configured load balancing rules for a Gateway router
> or
> +        Router with gateway port in <code>OVN_Northbound</code> database
> that
> +        includes a L4 port <var>PORT</var> of protocol <var>P</var> and
> IPv4
> +        address <var>VIP</var>, a priority-120 flow that matches on
>          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst == <var>VIP</var>
>          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
>          </var></code> with an action of
> <code>ct_lb(<var>args</var>)</code>,
>          where <var>args</var> contains comma separated IPv4 addresses (and
> -        optional port numbers) to load balance to.  If the Gateway router
> -        is configured to force SNAT any load-balanced packets, the above
> -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> +        optional port numbers) to load balance to.  If the router is
> configured
> +        to force SNAT any load-balanced packets, the above action will be
> +        replaced by <code>flags.force_snat_for_lb = 1;
>          ct_lb(<var>args</var>);</code>.
>        </li>
>
>        <li>
> -        For all the configured load balancing rules for Gateway router in
> +        For all the configured load balancing rules for a router in
>          <code>OVN_Northbound</code> database that includes a L4 port
>          <var>PORT</var> of protocol <var>P</var> and IPv4 address
>          <var>VIP</var>, a priority-120 flow that matches on
>          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst == <var>VIP</var>
>          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
> -        </var></code> with an action of <code>ct_dnat;</code>.
> -        If the Gateway router is configured to force SNAT any
> load-balanced
> -        packets, the above action will be replaced by
> -        <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> +        </var></code> with an action of <code>ct_dnat;</code>. If the
> router is
> +        configured to force SNAT any load-balanced packets, the above
> action
> +        will be replaced by <code>flags.force_snat_for_lb = 1;
> ct_dnat;</code>.
>        </li>
>
>        <li>
> -        For all the configured load balancing rules for Gateway router in
> +        For all the configured load balancing rules for a router in
>          <code>OVN_Northbound</code> database that includes just an IP
> address
>          <var>VIP</var> to match on, a priority-110 flow that matches on
>          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
>          <var>VIP</var></code> with an action of
>          <code>ct_lb(<var>args</var>)</code>, where <var>args</var>
> contains
> -        comma separated IPv4 addresses.  If the Gateway router
> -        is configured to force SNAT any load-balanced packets, the above
> -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> -        ct_lb(<var>args</var>);</code>.
> +        comma separated IPv4 addresses.  If the router is configured to
> force
> +        SNAT any load-balanced packets, the above action will be replaced
> by
> +        <code>flags.force_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
>        </li>
>
>        <li>
> -        For all the configured load balancing rules for Gateway router in
> +        For all the configured load balancing rules for a router in
>          <code>OVN_Northbound</code> database that includes just an IP
> address
>          <var>VIP</var> to match on, a priority-110 flow that matches on
>          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
>          <var>VIP</var></code> with an action of <code>ct_dnat;</code>.
> -        If the Gateway router is configured to force SNAT any
> load-balanced
> +        If the router is configured to force SNAT any load-balanced
>          packets, the above action will be replaced by
>          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
>        </li>
> +    </ul>
>
> +    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> +
> +    <ul>
>        <li>
>          For each configuration in the OVN Northbound database, that asks
>          to change the destination IP address of a packet from
> <var>A</var> to
> @@ -1892,6 +1900,28 @@ arp {
>      <ul>
>        <li>
>          <p>
> +          For all the configured load balancing rules for a router with
> gateway
> +          port in <code>OVN_Northbound</code> database that includes an
> IPv4
> +          address <code>VIP</code>, for every backend IPv4 address
> <var>B</var>
> +          defined for the <code>VIP</code> a priority-120 flow is
> programmed on
> +          <code>redirect-chassis</code> that matches
> +          <code>ip &amp;&amp; ip4.src == <var>B</var> &amp;&amp;
> +          outport == <var>GW</var></code>, where <var>GW</var> is the
> logical
> +          router gateway port with an action <code>ct_dnat;</code>. If the
> +          backend IPv4 address <var>B</var> is also configured with L4
> port
> +          <var>PORT</var> of protocol <var>P</var>, then the
> +          match also includes <code>P.src</code> == <var>PORT</var>.
> +        </p>
> +
> +        <p>
> +          If the router is configured to force SNAT  any load-balanced
> packets,
> +          above action will be replaced by
> +          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> +        </p>
> +      </li>
> +
> +      <li>
> +        <p>
>            For each configuration in the OVN Northbound database that asks
>            to change the destination IP address of a packet from an IP
>            address of <var>A</var> to <var>B</var>, a priority-100 flow
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 49e4ac338..7cc6aab73 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -4338,7 +4338,8 @@ get_force_snat_ip(struct ovn_datapath *od, const
> char *key_type, ovs_be32 *ip)
>  static void
>  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>                     struct ds *match, struct ds *actions, int priority,
> -                   const char *lb_force_snat_ip)
> +                   const char *lb_force_snat_ip, char *backend_ips,
> +                   bool is_udp)
>  {
>      /* A match and actions for new connections. */
>      char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> @@ -4365,6 +4366,63 @@ add_router_lb_flow(struct hmap *lflows, struct
> ovn_datapath *od,
>
>      free(new_match);
>      free(est_match);
> +
> +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> +        return;
> +    }
> +
> +    /* Add logical flows to UNDNAT the load balanced reverse traffic in
> +     * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the
> logical
> +     * router has a gateway router port associated.
> +     */
> +    struct ds undnat_match = DS_EMPTY_INITIALIZER;
> +    ds_put_cstr(&undnat_match, "ip4 && (");
> +    char *start, *next, *ip_str;
> +    start = next = xstrdup(backend_ips);
> +    ip_str = strsep(&next, ",");
> +    bool backend_ips_found = false;
> +    while (ip_str && ip_str[0]) {
> +        char *ip_address = NULL;
> +        uint16_t port = 0;
> +        ip_address_and_port_from_lb_key(ip_str, &ip_address, &port);
> +        if (!ip_address) {
> +            break;
> +        }
> +
> +        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> +        free(ip_address);
> +        if (port) {
> +            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> +                          is_udp ? "udp" : "tcp", port);
> +        } else {
> +            ds_put_cstr(&undnat_match, ") || ");
> +        }
> +        ip_str = strsep(&next, ",");
> +        backend_ips_found = true;
> +    }
> +
> +    free(start);
> +    if (!backend_ips_found) {
> +        ds_destroy(&undnat_match);
> +        return;
> +    }
> +    ds_chomp(&undnat_match, ' ');
> +    ds_chomp(&undnat_match, '|');
> +    ds_chomp(&undnat_match, '|');
> +    ds_chomp(&undnat_match, ' ');
> +    ds_put_format(&undnat_match, ") && outport == %s && "
> +                 "is_chassis_resident(%s)", od->l3dgw_port->json_key,
> +                 od->l3redirect_port->json_key);
> +    if (lb_force_snat_ip) {
> +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> +                      ds_cstr(&undnat_match),
> +                      "flags.force_snat_for_lb = 1; ct_dnat;");
> +    } else {
> +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> +                      ds_cstr(&undnat_match), "ct_dnat;");
> +    }
> +
> +    ds_destroy(&undnat_match);
>  }
>
>  static void
> @@ -5242,8 +5300,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>          }
>
>          /* Load balancing and packet defrag are only valid on
> -         * Gateway routers. */
> -        if (!smap_get(&od->nbr->options, "chassis")) {
> +         * Gateway routers or router with gateway port. */
> +        if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) {
>              continue;
>          }
>
> @@ -5282,20 +5340,26 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                ip_address);
>                  free(ip_address);
>
> +                int prio = 110;
> +                bool is_udp = lb->protocol && !strcmp(lb->protocol,
> "udp") ?
> +                    true : false;
>                  if (port) {
> -                    if (lb->protocol && !strcmp(lb->protocol, "udp")) {
> +                    if (is_udp) {
>                          ds_put_format(&match, " && udp && udp.dst == %d",
>                                        port);
>                      } else {
>                          ds_put_format(&match, " && tcp && tcp.dst == %d",
>                                        port);
>                      }
> -                    add_router_lb_flow(lflows, od, &match, &actions, 120,
> -                                       lb_force_snat_ip);
> -                } else {
> -                    add_router_lb_flow(lflows, od, &match, &actions, 110,
> -                                       lb_force_snat_ip);
> +                    prio = 120;
> +                }
> +
> +                if (od->l3redirect_port) {
> +                    ds_put_format(&match, " && is_chassis_resident(%s)",
> +                                  od->l3redirect_port->json_key);
>                  }
> +                add_router_lb_flow(lflows, od, &match, &actions, prio,
> +                                   lb_force_snat_ip, node->value, is_udp);
>              }
>          }
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index dd62bd116..638c0b661 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -1075,6 +1075,147 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/d
>  /connection dropped.*/d"])
>  AT_CLEANUP
>
> +AT_SETUP([ovn -- load balancing in router with gateway router port])
> +AT_KEYWORDS([ovnlb])
> +
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +# Logical network:
> +# One LR R1 with switches foo (192.168.1.0/24), bar (192.168.2.0/24),
> +# and alice (172.16.1.0/24) connected to it.  The port between R1 and
> +# alice is the router gateway port where the R1 LB rules are applied.
> +#
> +#    foo -- R1 -- bar
> +#           |
> +#    alice ----
> +
> +ovn-nbctl lr-add R1
> +
> +ovn-nbctl ls-add foo
> +ovn-nbctl ls-add bar
> +ovn-nbctl ls-add alice
> +
> +ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> +ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24
> +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
> +    -- set Logical_Router_Port alice options:redirect-chassis=hv1
> +
> +# Connect foo to R1
> +ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> +    type=router options:router-port=foo \
> +    -- lsp-set-addresses rp-foo router
> +
> +# Connect bar to R1
> +ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
> +    type=router options:router-port=bar \
> +    -- lsp-set-addresses rp-bar router
> +
> +# Connect alice to R1
> +ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> +    type=router options:router-port=alice \
> +    -- lsp-set-addresses rp-alice router
> +
> +# Logical port 'foo1' in switch 'foo'.
> +ADD_NAMESPACES(foo1)
> +ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> +         "192.168.1.1")
> +ovn-nbctl lsp-add foo foo1 \
> +-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
> +
> +# Logical port 'foo2' in switch 'foo'.
> +ADD_NAMESPACES(foo2)
> +ADD_VETH(foo2, foo2, br-int, "192.168.1.3/24", "f0:00:00:01:02:06", \
> +         "192.168.1.1")
> +ovn-nbctl lsp-add foo foo2 \
> +-- lsp-set-addresses foo2 "f0:00:00:01:02:06 192.168.1.3"
> +
> +# Logical port 'bar1' in switch 'bar'.
> +ADD_NAMESPACES(bar1)
> +ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:04", \
> +         "192.168.2.1")
> +ovn-nbctl lsp-add bar bar1 \
> +-- lsp-set-addresses bar1 "f0:00:00:01:02:04 192.168.2.2"
> +
> +# Logical port 'alice1' in switch 'alice'.
> +ADD_NAMESPACES(alice1)
> +ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:05", \
> +         "172.16.1.1")
> +ovn-nbctl lsp-add alice alice1 \
> +-- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
> +
> +# Config OVN load-balancer with a VIP.
> +uuid=`ovn-nbctl  create load_balancer
> vips:172.16.1.10="192.168.1.2,192.168.2.2"`
> +ovn-nbctl set logical_router R1 load_balancer=$uuid
> +
> +# Config OVN load-balancer with another VIP (this time with ports).
> +ovn-nbctl set load_balancer $uuid vips:'"172.16.1.11:8000"'='"
> 192.168.1.2:80,192.168.2.2:80"'
> +
> +# Wait for ovn-controller to catch up.
> +ovn-nbctl --wait=hv sync
> +OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \
> +grep 'nat(dst=192.168.2.2:80)'])
> +
> +# Start webservers in 'foo1', 'bar1'.
> +OVS_START_L7([foo1], [http])
> +OVS_START_L7([bar1], [http])
> +
> +dnl Should work with the virtual IP address through NAT
> +for i in `seq 1 20`; do
> +    echo Request $i
> +    NS_CHECK_EXEC([alice1], [wget 172.16.1.10 -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> +done
> +
> +dnl Each server should have at least one connection.
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.10) |
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
>
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> +])
> +
> +dnl Test load-balancing that includes L4 ports in NAT.
> +for i in `seq 1 20`; do
> +    echo Request $i
> +    NS_CHECK_EXEC([alice1], [wget 172.16.1.11:8000 -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> +done
> +
> +dnl Each server should have at least one connection.
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.11) |
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
>
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> +])
> +
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +
>  AT_SETUP([ovn -- DNAT and SNAT on distributed router - N/S])
>  AT_KEYWORDS([ovnnat])
>
> --
> 2.13.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

This patch is one where I need help.  I asked Guru because it's an area
he knows, but if you feel confident about it, Mark, then I'll merge it.

Do you have a preference on ordering?

On Fri, Oct 27, 2017 at 03:26:56PM +0000, Mark Michelson wrote:
> I've had a look over this and it looks fine by me. Ben, do you plan to
> merge this soon? If so, I'll need to update my IPv6 load balancer patch
> since it will conflict with this a bit.
> 
> On Thu, Sep 21, 2017 at 10:53 AM <nusiddiq@redhat.com> wrote:
> 
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > This patch adds support for associating a load balancer to a
> > logical router with gateway router port which was missing earlier.
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> >
> Acked-by: Mark Michelson <mmichels@redhat.com>
> 
> > ---
> >  ovn/northd/ovn-northd.8.xml |  70 +++++++++++++++-------
> >  ovn/northd/ovn-northd.c     |  82 +++++++++++++++++++++++---
> >  tests/system-ovn.at         | 141
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 264 insertions(+), 29 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 0d85ec0d2..2798369ce 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -1459,61 +1459,69 @@ icmp4 {
> >        in the reverse direction needs to be unDNATed.
> >      </p>
> >
> > -    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > +    <p> Ingress Table 4: Load balancing DNAT rules </p>
> > +
> > +    <p>
> > +      Following load balancing DNAT flows are added for Gateway router or
> > +      Router with gateway port. These flows are programmed only on the
> > +      <code>redirect-chassis</code>.
> > +    </p>
> >
> >      <ul>
> >        <li>
> > -        For all the configured load balancing rules for Gateway router in
> > -        <code>OVN_Northbound</code> database that includes a L4 port
> > -        <var>PORT</var> of protocol <var>P</var> and IPv4 address
> > -        <var>VIP</var>, a priority-120 flow that matches on
> > +        For all the configured load balancing rules for a Gateway router
> > or
> > +        Router with gateway port in <code>OVN_Northbound</code> database
> > that
> > +        includes a L4 port <var>PORT</var> of protocol <var>P</var> and
> > IPv4
> > +        address <var>VIP</var>, a priority-120 flow that matches on
> >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst == <var>VIP</var>
> >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
> >          </var></code> with an action of
> > <code>ct_lb(<var>args</var>)</code>,
> >          where <var>args</var> contains comma separated IPv4 addresses (and
> > -        optional port numbers) to load balance to.  If the Gateway router
> > -        is configured to force SNAT any load-balanced packets, the above
> > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > +        optional port numbers) to load balance to.  If the router is
> > configured
> > +        to force SNAT any load-balanced packets, the above action will be
> > +        replaced by <code>flags.force_snat_for_lb = 1;
> >          ct_lb(<var>args</var>);</code>.
> >        </li>
> >
> >        <li>
> > -        For all the configured load balancing rules for Gateway router in
> > +        For all the configured load balancing rules for a router in
> >          <code>OVN_Northbound</code> database that includes a L4 port
> >          <var>PORT</var> of protocol <var>P</var> and IPv4 address
> >          <var>VIP</var>, a priority-120 flow that matches on
> >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst == <var>VIP</var>
> >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
> > -        </var></code> with an action of <code>ct_dnat;</code>.
> > -        If the Gateway router is configured to force SNAT any
> > load-balanced
> > -        packets, the above action will be replaced by
> > -        <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > +        </var></code> with an action of <code>ct_dnat;</code>. If the
> > router is
> > +        configured to force SNAT any load-balanced packets, the above
> > action
> > +        will be replaced by <code>flags.force_snat_for_lb = 1;
> > ct_dnat;</code>.
> >        </li>
> >
> >        <li>
> > -        For all the configured load balancing rules for Gateway router in
> > +        For all the configured load balancing rules for a router in
> >          <code>OVN_Northbound</code> database that includes just an IP
> > address
> >          <var>VIP</var> to match on, a priority-110 flow that matches on
> >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
> >          <var>VIP</var></code> with an action of
> >          <code>ct_lb(<var>args</var>)</code>, where <var>args</var>
> > contains
> > -        comma separated IPv4 addresses.  If the Gateway router
> > -        is configured to force SNAT any load-balanced packets, the above
> > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > -        ct_lb(<var>args</var>);</code>.
> > +        comma separated IPv4 addresses.  If the router is configured to
> > force
> > +        SNAT any load-balanced packets, the above action will be replaced
> > by
> > +        <code>flags.force_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
> >        </li>
> >
> >        <li>
> > -        For all the configured load balancing rules for Gateway router in
> > +        For all the configured load balancing rules for a router in
> >          <code>OVN_Northbound</code> database that includes just an IP
> > address
> >          <var>VIP</var> to match on, a priority-110 flow that matches on
> >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
> >          <var>VIP</var></code> with an action of <code>ct_dnat;</code>.
> > -        If the Gateway router is configured to force SNAT any
> > load-balanced
> > +        If the router is configured to force SNAT any load-balanced
> >          packets, the above action will be replaced by
> >          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> >        </li>
> > +    </ul>
> >
> > +    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > +
> > +    <ul>
> >        <li>
> >          For each configuration in the OVN Northbound database, that asks
> >          to change the destination IP address of a packet from
> > <var>A</var> to
> > @@ -1892,6 +1900,28 @@ arp {
> >      <ul>
> >        <li>
> >          <p>
> > +          For all the configured load balancing rules for a router with
> > gateway
> > +          port in <code>OVN_Northbound</code> database that includes an
> > IPv4
> > +          address <code>VIP</code>, for every backend IPv4 address
> > <var>B</var>
> > +          defined for the <code>VIP</code> a priority-120 flow is
> > programmed on
> > +          <code>redirect-chassis</code> that matches
> > +          <code>ip &amp;&amp; ip4.src == <var>B</var> &amp;&amp;
> > +          outport == <var>GW</var></code>, where <var>GW</var> is the
> > logical
> > +          router gateway port with an action <code>ct_dnat;</code>. If the
> > +          backend IPv4 address <var>B</var> is also configured with L4
> > port
> > +          <var>PORT</var> of protocol <var>P</var>, then the
> > +          match also includes <code>P.src</code> == <var>PORT</var>.
> > +        </p>
> > +
> > +        <p>
> > +          If the router is configured to force SNAT  any load-balanced
> > packets,
> > +          above action will be replaced by
> > +          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > +        </p>
> > +      </li>
> > +
> > +      <li>
> > +        <p>
> >            For each configuration in the OVN Northbound database that asks
> >            to change the destination IP address of a packet from an IP
> >            address of <var>A</var> to <var>B</var>, a priority-100 flow
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 49e4ac338..7cc6aab73 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -4338,7 +4338,8 @@ get_force_snat_ip(struct ovn_datapath *od, const
> > char *key_type, ovs_be32 *ip)
> >  static void
> >  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> >                     struct ds *match, struct ds *actions, int priority,
> > -                   const char *lb_force_snat_ip)
> > +                   const char *lb_force_snat_ip, char *backend_ips,
> > +                   bool is_udp)
> >  {
> >      /* A match and actions for new connections. */
> >      char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > @@ -4365,6 +4366,63 @@ add_router_lb_flow(struct hmap *lflows, struct
> > ovn_datapath *od,
> >
> >      free(new_match);
> >      free(est_match);
> > +
> > +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > +        return;
> > +    }
> > +
> > +    /* Add logical flows to UNDNAT the load balanced reverse traffic in
> > +     * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the
> > logical
> > +     * router has a gateway router port associated.
> > +     */
> > +    struct ds undnat_match = DS_EMPTY_INITIALIZER;
> > +    ds_put_cstr(&undnat_match, "ip4 && (");
> > +    char *start, *next, *ip_str;
> > +    start = next = xstrdup(backend_ips);
> > +    ip_str = strsep(&next, ",");
> > +    bool backend_ips_found = false;
> > +    while (ip_str && ip_str[0]) {
> > +        char *ip_address = NULL;
> > +        uint16_t port = 0;
> > +        ip_address_and_port_from_lb_key(ip_str, &ip_address, &port);
> > +        if (!ip_address) {
> > +            break;
> > +        }
> > +
> > +        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > +        free(ip_address);
> > +        if (port) {
> > +            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> > +                          is_udp ? "udp" : "tcp", port);
> > +        } else {
> > +            ds_put_cstr(&undnat_match, ") || ");
> > +        }
> > +        ip_str = strsep(&next, ",");
> > +        backend_ips_found = true;
> > +    }
> > +
> > +    free(start);
> > +    if (!backend_ips_found) {
> > +        ds_destroy(&undnat_match);
> > +        return;
> > +    }
> > +    ds_chomp(&undnat_match, ' ');
> > +    ds_chomp(&undnat_match, '|');
> > +    ds_chomp(&undnat_match, '|');
> > +    ds_chomp(&undnat_match, ' ');
> > +    ds_put_format(&undnat_match, ") && outport == %s && "
> > +                 "is_chassis_resident(%s)", od->l3dgw_port->json_key,
> > +                 od->l3redirect_port->json_key);
> > +    if (lb_force_snat_ip) {
> > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > +                      ds_cstr(&undnat_match),
> > +                      "flags.force_snat_for_lb = 1; ct_dnat;");
> > +    } else {
> > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > +                      ds_cstr(&undnat_match), "ct_dnat;");
> > +    }
> > +
> > +    ds_destroy(&undnat_match);
> >  }
> >
> >  static void
> > @@ -5242,8 +5300,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> >          }
> >
> >          /* Load balancing and packet defrag are only valid on
> > -         * Gateway routers. */
> > -        if (!smap_get(&od->nbr->options, "chassis")) {
> > +         * Gateway routers or router with gateway port. */
> > +        if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) {
> >              continue;
> >          }
> >
> > @@ -5282,20 +5340,26 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> >                                ip_address);
> >                  free(ip_address);
> >
> > +                int prio = 110;
> > +                bool is_udp = lb->protocol && !strcmp(lb->protocol,
> > "udp") ?
> > +                    true : false;
> >                  if (port) {
> > -                    if (lb->protocol && !strcmp(lb->protocol, "udp")) {
> > +                    if (is_udp) {
> >                          ds_put_format(&match, " && udp && udp.dst == %d",
> >                                        port);
> >                      } else {
> >                          ds_put_format(&match, " && tcp && tcp.dst == %d",
> >                                        port);
> >                      }
> > -                    add_router_lb_flow(lflows, od, &match, &actions, 120,
> > -                                       lb_force_snat_ip);
> > -                } else {
> > -                    add_router_lb_flow(lflows, od, &match, &actions, 110,
> > -                                       lb_force_snat_ip);
> > +                    prio = 120;
> > +                }
> > +
> > +                if (od->l3redirect_port) {
> > +                    ds_put_format(&match, " && is_chassis_resident(%s)",
> > +                                  od->l3redirect_port->json_key);
> >                  }
> > +                add_router_lb_flow(lflows, od, &match, &actions, prio,
> > +                                   lb_force_snat_ip, node->value, is_udp);
> >              }
> >          }
> >
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index dd62bd116..638c0b661 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -1075,6 +1075,147 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> > patch-.*/d
> >  /connection dropped.*/d"])
> >  AT_CLEANUP
> >
> > +AT_SETUP([ovn -- load balancing in router with gateway router port])
> > +AT_KEYWORDS([ovnlb])
> > +
> > +CHECK_CONNTRACK()
> > +CHECK_CONNTRACK_NAT()
> > +ovn_start
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +ADD_BR([br-int])
> > +
> > +# Set external-ids in br-int needed for ovn-controller
> > +ovs-vsctl \
> > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > +        -- set Open_vSwitch .
> > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > +        -- set bridge br-int fail-mode=secure
> > other-config:disable-in-band=true
> > +
> > +# Start ovn-controller
> > +start_daemon ovn-controller
> > +
> > +# Logical network:
> > +# One LR R1 with switches foo (192.168.1.0/24), bar (192.168.2.0/24),
> > +# and alice (172.16.1.0/24) connected to it.  The port between R1 and
> > +# alice is the router gateway port where the R1 LB rules are applied.
> > +#
> > +#    foo -- R1 -- bar
> > +#           |
> > +#    alice ----
> > +
> > +ovn-nbctl lr-add R1
> > +
> > +ovn-nbctl ls-add foo
> > +ovn-nbctl ls-add bar
> > +ovn-nbctl ls-add alice
> > +
> > +ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> > +ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24
> > +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
> > +    -- set Logical_Router_Port alice options:redirect-chassis=hv1
> > +
> > +# Connect foo to R1
> > +ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> > +    type=router options:router-port=foo \
> > +    -- lsp-set-addresses rp-foo router
> > +
> > +# Connect bar to R1
> > +ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
> > +    type=router options:router-port=bar \
> > +    -- lsp-set-addresses rp-bar router
> > +
> > +# Connect alice to R1
> > +ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> > +    type=router options:router-port=alice \
> > +    -- lsp-set-addresses rp-alice router
> > +
> > +# Logical port 'foo1' in switch 'foo'.
> > +ADD_NAMESPACES(foo1)
> > +ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> > +         "192.168.1.1")
> > +ovn-nbctl lsp-add foo foo1 \
> > +-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
> > +
> > +# Logical port 'foo2' in switch 'foo'.
> > +ADD_NAMESPACES(foo2)
> > +ADD_VETH(foo2, foo2, br-int, "192.168.1.3/24", "f0:00:00:01:02:06", \
> > +         "192.168.1.1")
> > +ovn-nbctl lsp-add foo foo2 \
> > +-- lsp-set-addresses foo2 "f0:00:00:01:02:06 192.168.1.3"
> > +
> > +# Logical port 'bar1' in switch 'bar'.
> > +ADD_NAMESPACES(bar1)
> > +ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:04", \
> > +         "192.168.2.1")
> > +ovn-nbctl lsp-add bar bar1 \
> > +-- lsp-set-addresses bar1 "f0:00:00:01:02:04 192.168.2.2"
> > +
> > +# Logical port 'alice1' in switch 'alice'.
> > +ADD_NAMESPACES(alice1)
> > +ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:05", \
> > +         "172.16.1.1")
> > +ovn-nbctl lsp-add alice alice1 \
> > +-- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
> > +
> > +# Config OVN load-balancer with a VIP.
> > +uuid=`ovn-nbctl  create load_balancer
> > vips:172.16.1.10="192.168.1.2,192.168.2.2"`
> > +ovn-nbctl set logical_router R1 load_balancer=$uuid
> > +
> > +# Config OVN load-balancer with another VIP (this time with ports).
> > +ovn-nbctl set load_balancer $uuid vips:'"172.16.1.11:8000"'='"
> > 192.168.1.2:80,192.168.2.2:80"'
> > +
> > +# Wait for ovn-controller to catch up.
> > +ovn-nbctl --wait=hv sync
> > +OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \
> > +grep 'nat(dst=192.168.2.2:80)'])
> > +
> > +# Start webservers in 'foo1', 'bar1'.
> > +OVS_START_L7([foo1], [http])
> > +OVS_START_L7([bar1], [http])
> > +
> > +dnl Should work with the virtual IP address through NAT
> > +for i in `seq 1 20`; do
> > +    echo Request $i
> > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.10 -t 5 -T 1
> > --retry-connrefused -v -o wget$i.log])
> > +done
> > +
> > +dnl Each server should have at least one connection.
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.10) |
> > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> >
> > +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> >
> > +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > +])
> > +
> > +dnl Test load-balancing that includes L4 ports in NAT.
> > +for i in `seq 1 20`; do
> > +    echo Request $i
> > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.11:8000 -t 5 -T 1
> > --retry-connrefused -v -o wget$i.log])
> > +done
> > +
> > +dnl Each server should have at least one connection.
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.11) |
> > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> >
> > +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> >
> > +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > +])
> > +
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > +/connection dropped.*/d"])
> > +AT_CLEANUP
> > +
> >  AT_SETUP([ovn -- DNAT and SNAT on distributed router - N/S])
> >  AT_KEYWORDS([ovnnat])
> >
> > --
> > 2.13.3
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Go ahead and do this one first. It won't be too difficult for me to update
the IPv6 one.

On Fri, Oct 27, 2017 at 10:36 AM Ben Pfaff <blp@ovn.org> wrote:

> This patch is one where I need help.  I asked Guru because it's an area
> he knows, but if you feel confident about it, Mark, then I'll merge it.
>
> Do you have a preference on ordering?
>
> On Fri, Oct 27, 2017 at 03:26:56PM +0000, Mark Michelson wrote:
> > I've had a look over this and it looks fine by me. Ben, do you plan to
> > merge this soon? If so, I'll need to update my IPv6 load balancer patch
> > since it will conflict with this a bit.
> >
> > On Thu, Sep 21, 2017 at 10:53 AM <nusiddiq@redhat.com> wrote:
> >
> > > From: Numan Siddique <nusiddiq@redhat.com>
> > >
> > > This patch adds support for associating a load balancer to a
> > > logical router with gateway router port which was missing earlier.
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > > ---
> > >  ovn/northd/ovn-northd.8.xml |  70 +++++++++++++++-------
> > >  ovn/northd/ovn-northd.c     |  82 +++++++++++++++++++++++---
> > >  tests/system-ovn.at         | 141
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 264 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > > index 0d85ec0d2..2798369ce 100644
> > > --- a/ovn/northd/ovn-northd.8.xml
> > > +++ b/ovn/northd/ovn-northd.8.xml
> > > @@ -1459,61 +1459,69 @@ icmp4 {
> > >        in the reverse direction needs to be unDNATed.
> > >      </p>
> > >
> > > -    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > > +    <p> Ingress Table 4: Load balancing DNAT rules </p>
> > > +
> > > +    <p>
> > > +      Following load balancing DNAT flows are added for Gateway
> router or
> > > +      Router with gateway port. These flows are programmed only on the
> > > +      <code>redirect-chassis</code>.
> > > +    </p>
> > >
> > >      <ul>
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > -        <code>OVN_Northbound</code> database that includes a L4 port
> > > -        <var>PORT</var> of protocol <var>P</var> and IPv4 address
> > > -        <var>VIP</var>, a priority-120 flow that matches on
> > > +        For all the configured load balancing rules for a Gateway
> router
> > > or
> > > +        Router with gateway port in <code>OVN_Northbound</code>
> database
> > > that
> > > +        includes a L4 port <var>PORT</var> of protocol <var>P</var>
> and
> > > IPv4
> > > +        address <var>VIP</var>, a priority-120 flow that matches on
> > >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
> <var>VIP</var>
> > >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst ==
> <var>PORT
> > >          </var></code> with an action of
> > > <code>ct_lb(<var>args</var>)</code>,
> > >          where <var>args</var> contains comma separated IPv4 addresses
> (and
> > > -        optional port numbers) to load balance to.  If the Gateway
> router
> > > -        is configured to force SNAT any load-balanced packets, the
> above
> > > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > > +        optional port numbers) to load balance to.  If the router is
> > > configured
> > > +        to force SNAT any load-balanced packets, the above action
> will be
> > > +        replaced by <code>flags.force_snat_for_lb = 1;
> > >          ct_lb(<var>args</var>);</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes a L4 port
> > >          <var>PORT</var> of protocol <var>P</var> and IPv4 address
> > >          <var>VIP</var>, a priority-120 flow that matches on
> > >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
> <var>VIP</var>
> > >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst ==
> <var>PORT
> > > -        </var></code> with an action of <code>ct_dnat;</code>.
> > > -        If the Gateway router is configured to force SNAT any
> > > load-balanced
> > > -        packets, the above action will be replaced by
> > > -        <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > > +        </var></code> with an action of <code>ct_dnat;</code>. If the
> > > router is
> > > +        configured to force SNAT any load-balanced packets, the above
> > > action
> > > +        will be replaced by <code>flags.force_snat_for_lb = 1;
> > > ct_dnat;</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes just an IP
> > > address
> > >          <var>VIP</var> to match on, a priority-110 flow that matches
> on
> > >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
> > >          <var>VIP</var></code> with an action of
> > >          <code>ct_lb(<var>args</var>)</code>, where <var>args</var>
> > > contains
> > > -        comma separated IPv4 addresses.  If the Gateway router
> > > -        is configured to force SNAT any load-balanced packets, the
> above
> > > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > > -        ct_lb(<var>args</var>);</code>.
> > > +        comma separated IPv4 addresses.  If the router is configured
> to
> > > force
> > > +        SNAT any load-balanced packets, the above action will be
> replaced
> > > by
> > > +        <code>flags.force_snat_for_lb = 1;
> ct_lb(<var>args</var>);</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes just an IP
> > > address
> > >          <var>VIP</var> to match on, a priority-110 flow that matches
> on
> > >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
> > >          <var>VIP</var></code> with an action of <code>ct_dnat;</code>.
> > > -        If the Gateway router is configured to force SNAT any
> > > load-balanced
> > > +        If the router is configured to force SNAT any load-balanced
> > >          packets, the above action will be replaced by
> > >          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > >        </li>
> > > +    </ul>
> > >
> > > +    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > > +
> > > +    <ul>
> > >        <li>
> > >          For each configuration in the OVN Northbound database, that
> asks
> > >          to change the destination IP address of a packet from
> > > <var>A</var> to
> > > @@ -1892,6 +1900,28 @@ arp {
> > >      <ul>
> > >        <li>
> > >          <p>
> > > +          For all the configured load balancing rules for a router
> with
> > > gateway
> > > +          port in <code>OVN_Northbound</code> database that includes
> an
> > > IPv4
> > > +          address <code>VIP</code>, for every backend IPv4 address
> > > <var>B</var>
> > > +          defined for the <code>VIP</code> a priority-120 flow is
> > > programmed on
> > > +          <code>redirect-chassis</code> that matches
> > > +          <code>ip &amp;&amp; ip4.src == <var>B</var> &amp;&amp;
> > > +          outport == <var>GW</var></code>, where <var>GW</var> is the
> > > logical
> > > +          router gateway port with an action <code>ct_dnat;</code>.
> If the
> > > +          backend IPv4 address <var>B</var> is also configured with L4
> > > port
> > > +          <var>PORT</var> of protocol <var>P</var>, then the
> > > +          match also includes <code>P.src</code> == <var>PORT</var>.
> > > +        </p>
> > > +
> > > +        <p>
> > > +          If the router is configured to force SNAT  any load-balanced
> > > packets,
> > > +          above action will be replaced by
> > > +          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > > +        </p>
> > > +      </li>
> > > +
> > > +      <li>
> > > +        <p>
> > >            For each configuration in the OVN Northbound database that
> asks
> > >            to change the destination IP address of a packet from an IP
> > >            address of <var>A</var> to <var>B</var>, a priority-100 flow
> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > index 49e4ac338..7cc6aab73 100644
> > > --- a/ovn/northd/ovn-northd.c
> > > +++ b/ovn/northd/ovn-northd.c
> > > @@ -4338,7 +4338,8 @@ get_force_snat_ip(struct ovn_datapath *od, const
> > > char *key_type, ovs_be32 *ip)
> > >  static void
> > >  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > >                     struct ds *match, struct ds *actions, int priority,
> > > -                   const char *lb_force_snat_ip)
> > > +                   const char *lb_force_snat_ip, char *backend_ips,
> > > +                   bool is_udp)
> > >  {
> > >      /* A match and actions for new connections. */
> > >      char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > > @@ -4365,6 +4366,63 @@ add_router_lb_flow(struct hmap *lflows, struct
> > > ovn_datapath *od,
> > >
> > >      free(new_match);
> > >      free(est_match);
> > > +
> > > +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > > +        return;
> > > +    }
> > > +
> > > +    /* Add logical flows to UNDNAT the load balanced reverse traffic
> in
> > > +     * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the
> > > logical
> > > +     * router has a gateway router port associated.
> > > +     */
> > > +    struct ds undnat_match = DS_EMPTY_INITIALIZER;
> > > +    ds_put_cstr(&undnat_match, "ip4 && (");
> > > +    char *start, *next, *ip_str;
> > > +    start = next = xstrdup(backend_ips);
> > > +    ip_str = strsep(&next, ",");
> > > +    bool backend_ips_found = false;
> > > +    while (ip_str && ip_str[0]) {
> > > +        char *ip_address = NULL;
> > > +        uint16_t port = 0;
> > > +        ip_address_and_port_from_lb_key(ip_str, &ip_address, &port);
> > > +        if (!ip_address) {
> > > +            break;
> > > +        }
> > > +
> > > +        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > > +        free(ip_address);
> > > +        if (port) {
> > > +            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> > > +                          is_udp ? "udp" : "tcp", port);
> > > +        } else {
> > > +            ds_put_cstr(&undnat_match, ") || ");
> > > +        }
> > > +        ip_str = strsep(&next, ",");
> > > +        backend_ips_found = true;
> > > +    }
> > > +
> > > +    free(start);
> > > +    if (!backend_ips_found) {
> > > +        ds_destroy(&undnat_match);
> > > +        return;
> > > +    }
> > > +    ds_chomp(&undnat_match, ' ');
> > > +    ds_chomp(&undnat_match, '|');
> > > +    ds_chomp(&undnat_match, '|');
> > > +    ds_chomp(&undnat_match, ' ');
> > > +    ds_put_format(&undnat_match, ") && outport == %s && "
> > > +                 "is_chassis_resident(%s)", od->l3dgw_port->json_key,
> > > +                 od->l3redirect_port->json_key);
> > > +    if (lb_force_snat_ip) {
> > > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > > +                      ds_cstr(&undnat_match),
> > > +                      "flags.force_snat_for_lb = 1; ct_dnat;");
> > > +    } else {
> > > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > > +                      ds_cstr(&undnat_match), "ct_dnat;");
> > > +    }
> > > +
> > > +    ds_destroy(&undnat_match);
> > >  }
> > >
> > >  static void
> > > @@ -5242,8 +5300,8 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >          }
> > >
> > >          /* Load balancing and packet defrag are only valid on
> > > -         * Gateway routers. */
> > > -        if (!smap_get(&od->nbr->options, "chassis")) {
> > > +         * Gateway routers or router with gateway port. */
> > > +        if (!smap_get(&od->nbr->options, "chassis") &&
> !od->l3dgw_port) {
> > >              continue;
> > >          }
> > >
> > > @@ -5282,20 +5340,26 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >                                ip_address);
> > >                  free(ip_address);
> > >
> > > +                int prio = 110;
> > > +                bool is_udp = lb->protocol && !strcmp(lb->protocol,
> > > "udp") ?
> > > +                    true : false;
> > >                  if (port) {
> > > -                    if (lb->protocol && !strcmp(lb->protocol, "udp"))
> {
> > > +                    if (is_udp) {
> > >                          ds_put_format(&match, " && udp && udp.dst ==
> %d",
> > >                                        port);
> > >                      } else {
> > >                          ds_put_format(&match, " && tcp && tcp.dst ==
> %d",
> > >                                        port);
> > >                      }
> > > -                    add_router_lb_flow(lflows, od, &match, &actions,
> 120,
> > > -                                       lb_force_snat_ip);
> > > -                } else {
> > > -                    add_router_lb_flow(lflows, od, &match, &actions,
> 110,
> > > -                                       lb_force_snat_ip);
> > > +                    prio = 120;
> > > +                }
> > > +
> > > +                if (od->l3redirect_port) {
> > > +                    ds_put_format(&match, " &&
> is_chassis_resident(%s)",
> > > +                                  od->l3redirect_port->json_key);
> > >                  }
> > > +                add_router_lb_flow(lflows, od, &match, &actions, prio,
> > > +                                   lb_force_snat_ip, node->value,
> is_udp);
> > >              }
> > >          }
> > >
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index dd62bd116..638c0b661 100644
> > > --- a/tests/system-ovn.at
> > > +++ b/tests/system-ovn.at
> > > @@ -1075,6 +1075,147 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> port
> > > patch-.*/d
> > >  /connection dropped.*/d"])
> > >  AT_CLEANUP
> > >
> > > +AT_SETUP([ovn -- load balancing in router with gateway router port])
> > > +AT_KEYWORDS([ovnlb])
> > > +
> > > +CHECK_CONNTRACK()
> > > +CHECK_CONNTRACK_NAT()
> > > +ovn_start
> > > +OVS_TRAFFIC_VSWITCHD_START()
> > > +ADD_BR([br-int])
> > > +
> > > +# Set external-ids in br-int needed for ovn-controller
> > > +ovs-vsctl \
> > > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > > +        -- set Open_vSwitch .
> > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > > +        -- set bridge br-int fail-mode=secure
> > > other-config:disable-in-band=true
> > > +
> > > +# Start ovn-controller
> > > +start_daemon ovn-controller
> > > +
> > > +# Logical network:
> > > +# One LR R1 with switches foo (192.168.1.0/24), bar (192.168.2.0/24),
> > > +# and alice (172.16.1.0/24) connected to it.  The port between R1 and
> > > +# alice is the router gateway port where the R1 LB rules are applied.
> > > +#
> > > +#    foo -- R1 -- bar
> > > +#           |
> > > +#    alice ----
> > > +
> > > +ovn-nbctl lr-add R1
> > > +
> > > +ovn-nbctl ls-add foo
> > > +ovn-nbctl ls-add bar
> > > +ovn-nbctl ls-add alice
> > > +
> > > +ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> > > +ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24
> > > +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
> > > +    -- set Logical_Router_Port alice options:redirect-chassis=hv1
> > > +
> > > +# Connect foo to R1
> > > +ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> > > +    type=router options:router-port=foo \
> > > +    -- lsp-set-addresses rp-foo router
> > > +
> > > +# Connect bar to R1
> > > +ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
> > > +    type=router options:router-port=bar \
> > > +    -- lsp-set-addresses rp-bar router
> > > +
> > > +# Connect alice to R1
> > > +ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> > > +    type=router options:router-port=alice \
> > > +    -- lsp-set-addresses rp-alice router
> > > +
> > > +# Logical port 'foo1' in switch 'foo'.
> > > +ADD_NAMESPACES(foo1)
> > > +ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> > > +         "192.168.1.1")
> > > +ovn-nbctl lsp-add foo foo1 \
> > > +-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
> > > +
> > > +# Logical port 'foo2' in switch 'foo'.
> > > +ADD_NAMESPACES(foo2)
> > > +ADD_VETH(foo2, foo2, br-int, "192.168.1.3/24", "f0:00:00:01:02:06", \
> > > +         "192.168.1.1")
> > > +ovn-nbctl lsp-add foo foo2 \
> > > +-- lsp-set-addresses foo2 "f0:00:00:01:02:06 192.168.1.3"
> > > +
> > > +# Logical port 'bar1' in switch 'bar'.
> > > +ADD_NAMESPACES(bar1)
> > > +ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:04", \
> > > +         "192.168.2.1")
> > > +ovn-nbctl lsp-add bar bar1 \
> > > +-- lsp-set-addresses bar1 "f0:00:00:01:02:04 192.168.2.2"
> > > +
> > > +# Logical port 'alice1' in switch 'alice'.
> > > +ADD_NAMESPACES(alice1)
> > > +ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24",
> "f0:00:00:01:02:05", \
> > > +         "172.16.1.1")
> > > +ovn-nbctl lsp-add alice alice1 \
> > > +-- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
> > > +
> > > +# Config OVN load-balancer with a VIP.
> > > +uuid=`ovn-nbctl  create load_balancer
> > > vips:172.16.1.10="192.168.1.2,192.168.2.2"`
> > > +ovn-nbctl set logical_router R1 load_balancer=$uuid
> > > +
> > > +# Config OVN load-balancer with another VIP (this time with ports).
> > > +ovn-nbctl set load_balancer $uuid vips:'"172.16.1.11:8000"'='"
> > > 192.168.1.2:80,192.168.2.2:80"'
> > > +
> > > +# Wait for ovn-controller to catch up.
> > > +ovn-nbctl --wait=hv sync
> > > +OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \
> > > +grep 'nat(dst=192.168.2.2:80)'])
> > > +
> > > +# Start webservers in 'foo1', 'bar1'.
> > > +OVS_START_L7([foo1], [http])
> > > +OVS_START_L7([bar1], [http])
> > > +
> > > +dnl Should work with the virtual IP address through NAT
> > > +for i in `seq 1 20`; do
> > > +    echo Request $i
> > > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.10 -t 5 -T 1
> > > --retry-connrefused -v -o wget$i.log])
> > > +done
> > > +
> > > +dnl Each server should have at least one connection.
> > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.10) |
> > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> > >
> > >
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > >
> > >
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > > +])
> > > +
> > > +dnl Test load-balancing that includes L4 ports in NAT.
> > > +for i in `seq 1 20`; do
> > > +    echo Request $i
> > > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.11:8000 -t 5 -T 1
> > > --retry-connrefused -v -o wget$i.log])
> > > +done
> > > +
> > > +dnl Each server should have at least one connection.
> > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.11) |
> > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> > >
> > >
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > >
> > >
> +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
> > > +])
> > > +
> > > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > > +
> > > +as ovn-sb
> > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > +
> > > +as ovn-nb
> > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > +
> > > +as northd
> > > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > > +
> > > +as
> > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > > +/connection dropped.*/d"])
> > > +AT_CLEANUP
> > > +
> > >  AT_SETUP([ovn -- DNAT and SNAT on distributed router - N/S])
> > >  AT_KEYWORDS([ovnnat])
> > >
> > > --
> > > 2.13.3
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 27 October 2017 at 08:35, Ben Pfaff <blp@ovn.org> wrote:

> This patch is one where I need help.  I asked Guru because it's an area
> he knows, but if you feel confident about it, Mark, then I'll merge it.
>

No objections from me. If I find issues, I will let know even after the
patch is merged.


>
> Do you have a preference on ordering?
>
> On Fri, Oct 27, 2017 at 03:26:56PM +0000, Mark Michelson wrote:
> > I've had a look over this and it looks fine by me. Ben, do you plan to
> > merge this soon? If so, I'll need to update my IPv6 load balancer patch
> > since it will conflict with this a bit.
> >
> > On Thu, Sep 21, 2017 at 10:53 AM <nusiddiq@redhat.com> wrote:
> >
> > > From: Numan Siddique <nusiddiq@redhat.com>
> > >
> > > This patch adds support for associating a load balancer to a
> > > logical router with gateway router port which was missing earlier.
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > > ---
> > >  ovn/northd/ovn-northd.8.xml |  70 +++++++++++++++-------
> > >  ovn/northd/ovn-northd.c     |  82 +++++++++++++++++++++++---
> > >  tests/system-ovn.at         | 141
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 264 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > > index 0d85ec0d2..2798369ce 100644
> > > --- a/ovn/northd/ovn-northd.8.xml
> > > +++ b/ovn/northd/ovn-northd.8.xml
> > > @@ -1459,61 +1459,69 @@ icmp4 {
> > >        in the reverse direction needs to be unDNATed.
> > >      </p>
> > >
> > > -    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > > +    <p> Ingress Table 4: Load balancing DNAT rules </p>
> > > +
> > > +    <p>
> > > +      Following load balancing DNAT flows are added for Gateway
> router or
> > > +      Router with gateway port. These flows are programmed only on the
> > > +      <code>redirect-chassis</code>.
> > > +    </p>
> > >
> > >      <ul>
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > -        <code>OVN_Northbound</code> database that includes a L4 port
> > > -        <var>PORT</var> of protocol <var>P</var> and IPv4 address
> > > -        <var>VIP</var>, a priority-120 flow that matches on
> > > +        For all the configured load balancing rules for a Gateway
> router
> > > or
> > > +        Router with gateway port in <code>OVN_Northbound</code>
> database
> > > that
> > > +        includes a L4 port <var>PORT</var> of protocol <var>P</var>
> and
> > > IPv4
> > > +        address <var>VIP</var>, a priority-120 flow that matches on
> > >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
> <var>VIP</var>
> > >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst ==
> <var>PORT
> > >          </var></code> with an action of
> > > <code>ct_lb(<var>args</var>)</code>,
> > >          where <var>args</var> contains comma separated IPv4 addresses
> (and
> > > -        optional port numbers) to load balance to.  If the Gateway
> router
> > > -        is configured to force SNAT any load-balanced packets, the
> above
> > > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > > +        optional port numbers) to load balance to.  If the router is
> > > configured
> > > +        to force SNAT any load-balanced packets, the above action
> will be
> > > +        replaced by <code>flags.force_snat_for_lb = 1;
> > >          ct_lb(<var>args</var>);</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes a L4 port
> > >          <var>PORT</var> of protocol <var>P</var> and IPv4 address
> > >          <var>VIP</var>, a priority-120 flow that matches on
> > >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
> <var>VIP</var>
> > >          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst ==
> <var>PORT
> > > -        </var></code> with an action of <code>ct_dnat;</code>.
> > > -        If the Gateway router is configured to force SNAT any
> > > load-balanced
> > > -        packets, the above action will be replaced by
> > > -        <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > > +        </var></code> with an action of <code>ct_dnat;</code>. If the
> > > router is
> > > +        configured to force SNAT any load-balanced packets, the above
> > > action
> > > +        will be replaced by <code>flags.force_snat_for_lb = 1;
> > > ct_dnat;</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes just an IP
> > > address
> > >          <var>VIP</var> to match on, a priority-110 flow that matches
> on
> > >          <code>ct.new &amp;&amp; ip &amp;&amp; ip4.dst ==
> > >          <var>VIP</var></code> with an action of
> > >          <code>ct_lb(<var>args</var>)</code>, where <var>args</var>
> > > contains
> > > -        comma separated IPv4 addresses.  If the Gateway router
> > > -        is configured to force SNAT any load-balanced packets, the
> above
> > > -        action will be replaced by <code>flags.force_snat_for_lb = 1;
> > > -        ct_lb(<var>args</var>);</code>.
> > > +        comma separated IPv4 addresses.  If the router is configured
> to
> > > force
> > > +        SNAT any load-balanced packets, the above action will be
> replaced
> > > by
> > > +        <code>flags.force_snat_for_lb = 1;
> ct_lb(<var>args</var>);</code>.
> > >        </li>
> > >
> > >        <li>
> > > -        For all the configured load balancing rules for Gateway
> router in
> > > +        For all the configured load balancing rules for a router in
> > >          <code>OVN_Northbound</code> database that includes just an IP
> > > address
> > >          <var>VIP</var> to match on, a priority-110 flow that matches
> on
> > >          <code>ct.est &amp;&amp; ip &amp;&amp; ip4.dst ==
> > >          <var>VIP</var></code> with an action of <code>ct_dnat;</code>.
> > > -        If the Gateway router is configured to force SNAT any
> > > load-balanced
> > > +        If the router is configured to force SNAT any load-balanced
> > >          packets, the above action will be replaced by
> > >          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > >        </li>
> > > +    </ul>
> > >
> > > +    <p>Ingress Table 4: DNAT on Gateway Routers</p>
> > > +
> > > +    <ul>
> > >        <li>
> > >          For each configuration in the OVN Northbound database, that
> asks
> > >          to change the destination IP address of a packet from
> > > <var>A</var> to
> > > @@ -1892,6 +1900,28 @@ arp {
> > >      <ul>
> > >        <li>
> > >          <p>
> > > +          For all the configured load balancing rules for a router
> with
> > > gateway
> > > +          port in <code>OVN_Northbound</code> database that includes
> an
> > > IPv4
> > > +          address <code>VIP</code>, for every backend IPv4 address
> > > <var>B</var>
> > > +          defined for the <code>VIP</code> a priority-120 flow is
> > > programmed on
> > > +          <code>redirect-chassis</code> that matches
> > > +          <code>ip &amp;&amp; ip4.src == <var>B</var> &amp;&amp;
> > > +          outport == <var>GW</var></code>, where <var>GW</var> is the
> > > logical
> > > +          router gateway port with an action <code>ct_dnat;</code>.
> If the
> > > +          backend IPv4 address <var>B</var> is also configured with L4
> > > port
> > > +          <var>PORT</var> of protocol <var>P</var>, then the
> > > +          match also includes <code>P.src</code> == <var>PORT</var>.
> > > +        </p>
> > > +
> > > +        <p>
> > > +          If the router is configured to force SNAT  any load-balanced
> > > packets,
> > > +          above action will be replaced by
> > > +          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
> > > +        </p>
> > > +      </li>
> > > +
> > > +      <li>
> > > +        <p>
> > >            For each configuration in the OVN Northbound database that
> asks
> > >            to change the destination IP address of a packet from an IP
> > >            address of <var>A</var> to <var>B</var>, a priority-100 flow
> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > index 49e4ac338..7cc6aab73 100644
> > > --- a/ovn/northd/ovn-northd.c
> > > +++ b/ovn/northd/ovn-northd.c
> > > @@ -4338,7 +4338,8 @@ get_force_snat_ip(struct ovn_datapath *od, const
> > > char *key_type, ovs_be32 *ip)
> > >  static void
> > >  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > >                     struct ds *match, struct ds *actions, int priority,
> > > -                   const char *lb_force_snat_ip)
> > > +                   const char *lb_force_snat_ip, char *backend_ips,
> > > +                   bool is_udp)
> > >  {
> > >      /* A match and actions for new connections. */
> > >      char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > > @@ -4365,6 +4366,63 @@ add_router_lb_flow(struct hmap *lflows, struct
> > > ovn_datapath *od,
> > >
> > >      free(new_match);
> > >      free(est_match);
> > > +
> > > +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > > +        return;
> > > +    }
> > > +
> > > +    /* Add logical flows to UNDNAT the load balanced reverse traffic
> in
> > > +     * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the
> > > logical
> > > +     * router has a gateway router port associated.
> > > +     */
> > > +    struct ds undnat_match = DS_EMPTY_INITIALIZER;
> > > +    ds_put_cstr(&undnat_match, "ip4 && (");
> > > +    char *start, *next, *ip_str;
> > > +    start = next = xstrdup(backend_ips);
> > > +    ip_str = strsep(&next, ",");
> > > +    bool backend_ips_found = false;
> > > +    while (ip_str && ip_str[0]) {
> > > +        char *ip_address = NULL;
> > > +        uint16_t port = 0;
> > > +        ip_address_and_port_from_lb_key(ip_str, &ip_address, &port);
> > > +        if (!ip_address) {
> > > +            break;
> > > +        }
> > > +
> > > +        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > > +        free(ip_address);
> > > +        if (port) {
> > > +            ds_put_format(&undnat_match, " && %s.src == %d) || ",
> > > +                          is_udp ? "udp" : "tcp", port);
> > > +        } else {
> > > +            ds_put_cstr(&undnat_match, ") || ");
> > > +        }
> > > +        ip_str = strsep(&next, ",");
> > > +        backend_ips_found = true;
> > > +    }
> > > +
> > > +    free(start);
> > > +    if (!backend_ips_found) {
> > > +        ds_destroy(&undnat_match);
> > > +        return;
> > > +    }
> > > +    ds_chomp(&undnat_match, ' ');
> > > +    ds_chomp(&undnat_match, '|');
> > > +    ds_chomp(&undnat_match, '|');
> > > +    ds_chomp(&undnat_match, ' ');
> > > +    ds_put_format(&undnat_match, ") && outport == %s && "
> > > +                 "is_chassis_resident(%s)", od->l3dgw_port->json_key,
> > > +                 od->l3redirect_port->json_key);
> > > +    if (lb_force_snat_ip) {
> > > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > > +                      ds_cstr(&undnat_match),
> > > +                      "flags.force_snat_for_lb = 1; ct_dnat;");
> > > +    } else {
> > > +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> > > +                      ds_cstr(&undnat_match), "ct_dnat;");
> > > +    }
> > > +
> > > +    ds_destroy(&undnat_match);
> > >  }
> > >
> > >  static void
> > > @@ -5242,8 +5300,8 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >          }
> > >
> > >          /* Load balancing and packet defrag are only valid on
> > > -         * Gateway routers. */
> > > -        if (!smap_get(&od->nbr->options, "chassis")) {
> > > +         * Gateway routers or router with gateway port. */
> > > +        if (!smap_get(&od->nbr->options, "chassis") &&
> !od->l3dgw_port) {
> > >              continue;
> > >          }
> > >
> > > @@ -5282,20 +5340,26 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >                                ip_address);
> > >                  free(ip_address);
> > >
> > > +                int prio = 110;
> > > +                bool is_udp = lb->protocol && !strcmp(lb->protocol,
> > > "udp") ?
> > > +                    true : false;
> > >                  if (port) {
> > > -                    if (lb->protocol && !strcmp(lb->protocol, "udp"))
> {
> > > +                    if (is_udp) {
> > >                          ds_put_format(&match, " && udp && udp.dst ==
> %d",
> > >                                        port);
> > >                      } else {
> > >                          ds_put_format(&match, " && tcp && tcp.dst ==
> %d",
> > >                                        port);
> > >                      }
> > > -                    add_router_lb_flow(lflows, od, &match, &actions,
> 120,
> > > -                                       lb_force_snat_ip);
> > > -                } else {
> > > -                    add_router_lb_flow(lflows, od, &match, &actions,
> 110,
> > > -                                       lb_force_snat_ip);
> > > +                    prio = 120;
> > > +                }
> > > +
> > > +                if (od->l3redirect_port) {
> > > +                    ds_put_format(&match, " &&
> is_chassis_resident(%s)",
> > > +                                  od->l3redirect_port->json_key);
> > >                  }
> > > +                add_router_lb_flow(lflows, od, &match, &actions, prio,
> > > +                                   lb_force_snat_ip, node->value,
> is_udp);
> > >              }
> > >          }
> > >
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index dd62bd116..638c0b661 100644
> > > --- a/tests/system-ovn.at
> > > +++ b/tests/system-ovn.at
> > > @@ -1075,6 +1075,147 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> port
> > > patch-.*/d
> > >  /connection dropped.*/d"])
> > >  AT_CLEANUP
> > >
> > > +AT_SETUP([ovn -- load balancing in router with gateway router port])
> > > +AT_KEYWORDS([ovnlb])
> > > +
> > > +CHECK_CONNTRACK()
> > > +CHECK_CONNTRACK_NAT()
> > > +ovn_start
> > > +OVS_TRAFFIC_VSWITCHD_START()
> > > +ADD_BR([br-int])
> > > +
> > > +# Set external-ids in br-int needed for ovn-controller
> > > +ovs-vsctl \
> > > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > > +        -- set Open_vSwitch .
> > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > > +        -- set bridge br-int fail-mode=secure
> > > other-config:disable-in-band=true
> > > +
> > > +# Start ovn-controller
> > > +start_daemon ovn-controller
> > > +
> > > +# Logical network:
> > > +# One LR R1 with switches foo (192.168.1.0/24), bar (192.168.2.0/24),
> > > +# and alice (172.16.1.0/24) connected to it.  The port between R1 and
> > > +# alice is the router gateway port where the R1 LB rules are applied.
> > > +#
> > > +#    foo -- R1 -- bar
> > > +#           |
> > > +#    alice ----
> > > +
> > > +ovn-nbctl lr-add R1
> > > +
> > > +ovn-nbctl ls-add foo
> > > +ovn-nbctl ls-add bar
> > > +ovn-nbctl ls-add alice
> > > +
> > > +ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> > > +ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24
> > > +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
> > > +    -- set Logical_Router_Port alice options:redirect-chassis=hv1
> > > +
> > > +# Connect foo to R1
> > > +ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> > > +    type=router options:router-port=foo \
> > > +    -- lsp-set-addresses rp-foo router
> > > +
> > > +# Connect bar to R1
> > > +ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
> > > +    type=router options:router-port=bar \
> > > +    -- lsp-set-addresses rp-bar router
> > > +
> > > +# Connect alice to R1
> > > +ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> > > +    type=router options:router-port=alice \
> > > +    -- lsp-set-addresses rp-alice router
> > > +
> > > +# Logical port 'foo1' in switch 'foo'.
> > > +ADD_NAMESPACES(foo1)
> > > +ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> > > +         "192.168.1.1")
> > > +ovn-nbctl lsp-add foo foo1 \
> > > +-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
> > > +
> > > +# Logical port 'foo2' in switch 'foo'.
> > > +ADD_NAMESPACES(foo2)
> > > +ADD_VETH(foo2, foo2, br-int, "192.168.1.3/24", "f0:00:00:01:02:06", \
> > > +         "192.168.1.1")
> > > +ovn-nbctl lsp-add foo foo2 \
> > > +-- lsp-set-addresses foo2 "f0:00:00:01:02:06 192.168.1.3"
> > > +
> > > +# Logical port 'bar1' in switch 'bar'.
> > > +ADD_NAMESPACES(bar1)
> > > +ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:04", \
> > > +         "192.168.2.1")
> > > +ovn-nbctl lsp-add bar bar1 \
> > > +-- lsp-set-addresses bar1 "f0:00:00:01:02:04 192.168.2.2"
> > > +
> > > +# Logical port 'alice1' in switch 'alice'.
> > > +ADD_NAMESPACES(alice1)
> > > +ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24",
> "f0:00:00:01:02:05", \
> > > +         "172.16.1.1")
> > > +ovn-nbctl lsp-add alice alice1 \
> > > +-- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
> > > +
> > > +# Config OVN load-balancer with a VIP.
> > > +uuid=`ovn-nbctl  create load_balancer
> > > vips:172.16.1.10="192.168.1.2,192.168.2.2"`
> > > +ovn-nbctl set logical_router R1 load_balancer=$uuid
> > > +
> > > +# Config OVN load-balancer with another VIP (this time with ports).
> > > +ovn-nbctl set load_balancer $uuid vips:'"172.16.1.11:8000"'='"
> > > 192.168.1.2:80,192.168.2.2:80"'
> > > +
> > > +# Wait for ovn-controller to catch up.
> > > +ovn-nbctl --wait=hv sync
> > > +OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \
> > > +grep 'nat(dst=192.168.2.2:80)'])
> > > +
> > > +# Start webservers in 'foo1', 'bar1'.
> > > +OVS_START_L7([foo1], [http])
> > > +OVS_START_L7([bar1], [http])
> > > +
> > > +dnl Should work with the virtual IP address through NAT
> > > +for i in `seq 1 20`; do
> > > +    echo Request $i
> > > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.10 -t 5 -T 1
> > > --retry-connrefused -v -o wget$i.log])
> > > +done
> > > +
> > > +dnl Each server should have at least one connection.
> > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.10) |
> > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> > >
> > > +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>,
> protoinfo=(state=<cleared>)
> > >
> > > +tcp,orig=(src=172.16.1.2,dst=172.16.1.10,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>,
> protoinfo=(state=<cleared>)
> > > +])
> > > +
> > > +dnl Test load-balancing that includes L4 ports in NAT.
> > > +for i in `seq 1 20`; do
> > > +    echo Request $i
> > > +    NS_CHECK_EXEC([alice1], [wget 172.16.1.11:8000 -t 5 -T 1
> > > --retry-connrefused -v -o wget$i.log])
> > > +done
> > > +
> > > +dnl Each server should have at least one connection.
> > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.11) |
> > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> > >
> > > +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>,
> protoinfo=(state=<cleared>)
> > >
> > > +tcp,orig=(src=172.16.1.2,dst=172.16.1.11,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>,
> protoinfo=(state=<cleared>)
> > > +])
> > > +
> > > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > > +
> > > +as ovn-sb
> > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > +
> > > +as ovn-nb
> > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > +
> > > +as northd
> > > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > > +
> > > +as
> > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > > +/connection dropped.*/d"])
> > > +AT_CLEANUP
> > > +
> > >  AT_SETUP([ovn -- DNAT and SNAT on distributed router - N/S])
> > >  AT_KEYWORDS([ovnnat])
> > >
> > > --
> > > 2.13.3
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Fri, Oct 27, 2017 at 10:03:07AM -0700, Guru Shetty wrote:
> On 27 October 2017 at 08:35, Ben Pfaff <blp@ovn.org> wrote:
> 
> > This patch is one where I need help.  I asked Guru because it's an area
> > he knows, but if you feel confident about it, Mark, then I'll merge it.
> >
> 
> No objections from me. If I find issues, I will let know even after the
> patch is merged.
> 
> 
> >
> > Do you have a preference on ordering?

OK, thanks Numan, Guru, Mark.  I applied this to master.  It is not
obviously (to me) a bug fix, so I did not backport it.

On Fri, Oct 27, 2017 at 11:19 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Fri, Oct 27, 2017 at 10:03:07AM -0700, Guru Shetty wrote:
> > On 27 October 2017 at 08:35, Ben Pfaff <blp@ovn.org> wrote:
> >
> > > This patch is one where I need help.  I asked Guru because it's an area
> > > he knows, but if you feel confident about it, Mark, then I'll merge it.
> > >
> >
> > No objections from me. If I find issues, I will let know even after the
> > patch is merged.
> >
> >
> > >
> > > Do you have a preference on ordering?
>
> OK, thanks Numan, Guru, Mark.  I applied this to master.  It is not
> obviously (to me) a bug fix, so I did not backport it.
>

Thanks Ben.
Yeah its not a bug fix. So I think it's fine not to backport.

Numan



> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>