Thanks Numan, some suggestions below!
On 09/02/2021 18:44, num...@ovn.org wrote:
> From: Numan Siddique <num...@ovn.org>
>
> When a Gateway router is configured with a load balancer
> and it is also configured with options:lb_force_snat_ip=<IP>,
> OVN after load balancing the destination IP to one of the
> backend also does a NAT on the source ip with the
> lb_force_snat_ip if the packet is destined to a load balancer
> VIP.
>
> There is a problem with the snat of source ip to 'lb_force_snat_ip'
> in one particular usecase. When the packet enters the Gateway router
> from a provider logical switch destined to the load balancer VIP,
> then it is first load balanced to one of the backend and then
> the source ip is snatted to 'lb_force_snat_ip'. If the chosen
> backend is reachable via the provider logical switch, then the
> packet is hairpinned back and it may hit the wire with
> the source ip 'lb_force_snat_ip'. If 'lb_force_snat_ip' happens
> to be an OVN internal IP then the packet may be dropped.
>
> This patch addresses this issue by providing the option to
> set the option - 'lb_force_snat_ip=router_ip'. If 'router_ip'
> is set, then OVN will snat the load balanced packet to the
> router ip of the logical router port which chosen as 'outport'
> in lr_in_ip_routing stage.
It almost feels like this should be the default behaviour?
>
> Example.
>
> If the gateway router is
>
> ovn-nbctl show lr0
> router 68f20092-5563-44b8-9ccb-b11de3e3a66c (lr0)
> port lr0-sw0
> mac: "00:00:00:00:ff:01"
> networks: ["10.0.0.1/24"]
> port lr0-public
> mac: "00:00:20:20:12:13"
> networks: ["172.168.0.100/24"]
>
> Then the below logical flows are added if 'lb_force_snat_ip'
> is configured to 'router_ip'.
>
> table=1 (lr_out_snat), priority=110
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"),
> action=(ct_snat(172.168.0.100);)
>
> table=1 (lr_out_snat), priority=110
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0")
> action=(ct_snat(10.0.0.1);)
>
> For the above described scenario, the packet will have source ip as
> 172.168.0.100 which belongs to the provider logical switch CIDR.
>
> Reported-by: Tim Rozet <tro...@redhat.com>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> northd/ovn-northd.8.xml | 35 ++++++++++++++++++
> northd/ovn-northd.c | 66 ++++++++++++++++++++++++++++++++--
> tests/ovn-northd.at | 79 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 177 insertions(+), 3 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 70065a36d9..27b28aff93 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
Should 'ovn-nb.xml' also be updated?
> @@ -3653,6 +3653,32 @@ nd_ns {
> <code>flags.force_snat_for_dnat == 1 && ip</code> with an
> action <code>ct_snat(<var>B</var>);</code>.
> </p>
> + </li>
> +
> + <li>
> + <p>
> + If the Gateway router in the OVN Northbound database has been
> + configured to force SNAT a packet (that has been previously
> + load-balanced) using router IP (i.e <ref column="options"
> + table="Logical_Router"/>:lb_force_snat_ip=router_ip), then for
> + each logical router port <var>P</var> attached to the Gateway
> + router, a priority-110 flow matches
> + <code>flags.force_snat_for_lb == 1 && outport ==
> <var>P</var>
> + </code> with an action <code>ct_snat(<var>R</var>);</code>
> + where <var>R</var> is the router port IP configured.
maybe rephrase to "is the IP configured on the router port."
> + If <code>R</code> is an IPv4 address then the match will also
> + include <code>ip4</code> and if it is an IPv6 address, then the
> + match will also include <code>ip6</code>.
> + </p>
> +
> + <p>
> + If the logical router port <var>P</var> is configured with multiple
> + IPv4 and multiple IPv6 addresses, only the first IPv4 and first
> IPv6
> + address is considered.
Should we log this condition?
> + </p>
> + </li>
> +
> + <li>
> <p>
> If the Gateway router in the OVN Northbound database has been
> configured to force SNAT a packet (that has been previously
> @@ -3660,6 +3686,9 @@ nd_ns {
> <code>flags.force_snat_for_lb == 1 && ip</code> with an
> action <code>ct_snat(<var>B</var>);</code>.
> </p>
> + </li>
> +
> + <li>
> <p>
> For each configuration in the OVN Northbound database, that asks
> to change the source IP address of a packet from an IP address of
> @@ -3673,14 +3702,18 @@ nd_ns {
> options, then the action would be <code>ip4/6.src=
> (<var>B</var>)</code>.
> </p>
> + </li>
>
> + <li>
> <p>
> If the NAT rule has <code>allowed_ext_ips</code> configured, then
> there is an additional match <code>ip4.dst == <var>allowed_ext_ips
> </var></code>. Similarly, for IPV6, match would be <code>ip6.dst ==
> <var>allowed_ext_ips</var></code>.
> </p>
> + </li>
>
> + <li>
> <p>
> If the NAT rule has <code>exempted_ext_ips</code> set, then
> there is an additional flow configured at the priority + 1 of
> @@ -3689,7 +3722,9 @@ nd_ns {
> </code>. This flow is used to bypass the ct_snat action for a
> packet
> which is destinted to <code>exempted_ext_ips</code>.
> </p>
> + </li>
>
> + <li>
> <p>
> A priority-0 logical flow with match <code>1</code> has actions
> <code>next;</code>.
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index db6572a62b..ece158b71e 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -622,6 +622,7 @@ struct ovn_datapath {
>
> struct lport_addresses dnat_force_snat_addrs;
> struct lport_addresses lb_force_snat_addrs;
> + bool lb_force_snat_router_ip;
>
> struct ovn_port **localnet_ports;
> size_t n_localnet_ports;
> @@ -721,6 +722,17 @@ init_nat_entries(struct ovn_datapath *od)
> snat_ip_add(od, od->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
> NULL);
> }
> + } else {
> + const char *lb_force_snat =
> + smap_get(&od->nbr->options, "lb_force_snat_ip");
> + if (lb_force_snat && !strcmp(lb_force_snat, "router_ip")
> + && smap_get(&od->nbr->options, "chassis")) {
> + /* Set it to true only if its gateway router and
> + * options:lb_force_snat_ip=router_ip. */
> + od->lb_force_snat_router_ip = true;
> + } else {
> + od->lb_force_snat_router_ip = false;
> + }
> }
>
> if (!od->nbr->n_nat) {
> @@ -8365,9 +8377,12 @@ get_force_snat_ip(struct ovn_datapath *od, const char
> *key_type,
> }
>
> if (!extract_ip_address(addresses, laddrs)) {
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> - VLOG_WARN_RL(&rl, "bad ip %s in options of router "UUID_FMT"",
> - addresses, UUID_ARGS(&od->key));
> + if (strcmp(addresses, "router_ip") || strcmp(key_type, "lb")) {
Also, probably good to check or assert 'key_type' for NULL even though,
currently, all callers of get_force_snat_ip() cant pass a NULL value.
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_WARN_RL(&rl, "bad ip %s in options of router "UUID_FMT"",
> + addresses, UUID_ARGS(&od->key));
> + }
> +
> return false;
I think finding an IP or 'router_ip' should be the successful case and
not finding them should be unsuccessful. However, this would change the
logic for callers. Or maybe the name of this function could change and
another function to check for router_ip could be added. What do you think?
> }
>
> @@ -8943,6 +8958,48 @@ build_lrouter_force_snat_flows(struct hmap *lflows,
> struct ovn_datapath *od,
> ds_destroy(&actions);
> }
>
> +static void
> +build_lrouter_force_snat_flows_op(struct ovn_port *op,
> + struct hmap *lflows,
> + struct ds *match, struct ds *actions)
> +{
> + if (!op->nbrp || !op->peer || !op->od->lb_force_snat_router_ip) {
> + return;
> + }
> +
> + if (op->lrp_networks.n_ipv4_addrs) {
> + ds_clear(match);
> + ds_clear(actions);
> +
> + /* Higher priority rules to force SNAT with the router port ip.
> + * This only takes effect when the packet has already been
> + * load balanced once. */
> + ds_put_format(match, "flags.force_snat_for_lb == 1 && ip4 && "
> + "outport == %s", op->json_key);
> + ds_put_format(actions, "ct_snat(%s);",
> + op->lrp_networks.ipv4_addrs[0].addr_s);
> + ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110,
General musing that doesn't need to be addressed here. I wonder should
we have a macro definition for priorities for logical flows?
> + ds_cstr(match), ds_cstr(actions));
> + }
> +
> + /* op->lrp_networks.ipv6_addrs will always have LLA and that will be
> + * last in the list. So add the flows only if n_ipv6_addrs > 1. */
> + if (op->lrp_networks.n_ipv6_addrs > 1) {
> + ds_clear(match);
> + ds_clear(actions);
> +
> + /* Higher priority rules to force SNAT with the router port ip.
> + * This only takes effect when the packet has already been
> + * load balanced once. */
> + ds_put_format(match, "flags.force_snat_for_lb == 1 && ip6 && "
> + "outport == %s", op->json_key);
> + ds_put_format(actions, "ct_snat(%s);",
> + op->lrp_networks.ipv6_addrs[0].addr_s);
> + ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110,
> + ds_cstr(match), ds_cstr(actions));
> + }
> +}
> +
> static void
> build_lrouter_bfd_flows(struct hmap *lflows, struct ovn_port *op)
> {
> @@ -11278,6 +11335,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath
> *od,
> "dnat");
> }
> }
> +
> if (lb_force_snat_ip) {
> if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> build_lrouter_force_snat_flows(lflows, od, "4",
> @@ -11490,6 +11548,8 @@ build_lswitch_and_lrouter_iterate_by_op(struct
> ovn_port *op,
> &lsi->match, &lsi->actions);
> build_lrouter_ipv4_ip_input(op, lsi->lflows,
> &lsi->match, &lsi->actions);
> + build_lrouter_force_snat_flows_op(op, lsi->lflows, &lsi->match,
> + &lsi->actions);
> }
>
> static void
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 7240e22baf..fd03b1fb66 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2443,3 +2443,82 @@ check ovn-sbctl set chassis hv1
> other_config:port-up-notif=true
> wait_row_count nb:Logical_Switch_Port 1 up=false name=lsp1
>
> AT_CLEANUP
> +
> +AT_SETUP([ovn -- lb_force_snat_ip for Gateway Routers])
> +ovn_start
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl ls-add sw1
> +
> +# Create a logical router and attach both logical switches
> +check ovn-nbctl lr-add lr0
> +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> +check ovn-nbctl lsp-add sw0 sw0-lr0
> +check ovn-nbctl lsp-set-type sw0-lr0 router
> +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> +
> +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24
> +check ovn-nbctl lsp-add sw1 sw1-lr0
> +check ovn-nbctl lsp-set-type sw1-lr0 router
> +check ovn-nbctl lsp-set-addresses sw1-lr0 00:00:00:00:ff:02
> +check ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
> +
> +check ovn-nbctl ls-add public
> +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> +check ovn-nbctl lsp-add public public-lr0
> +check ovn-nbctl lsp-set-type public-lr0 router
> +check ovn-nbctl lsp-set-addresses public-lr0 router
> +check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> +
> +check ovn-nbctl set logical_router lr0 options:chassis=ch1
> +
> +ovn-sbctl dump-flows lr0 > lr0flows
> +AT_CAPTURE_FILE([lr0flows])
> +
> +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0],
> [dnl
> +])
> +
> +check ovn-nbctl --wait=sb set logical_router lr0
> options:lb_force_snat_ip="20.0.0.4 aef0::4"
> +
> +ovn-sbctl dump-flows lr0 > lr0flows
> +AT_CAPTURE_FILE([lr0flows])
> +
> +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0],
> [dnl
> + table=1 (lr_out_snat ), priority=100 ,
> match=(flags.force_snat_for_lb == 1 && ip4), action=(ct_snat(20.0.0.4);)
> + table=1 (lr_out_snat ), priority=100 ,
> match=(flags.force_snat_for_lb == 1 && ip6), action=(ct_snat(aef0::4);)
> +])
> +
> +check ovn-nbctl --wait=sb set logical_router lr0
> options:lb_force_snat_ip="router_ip"
> +
> +ovn-sbctl dump-flows lr0 > lr0flows
> +AT_CAPTURE_FILE([lr0flows])
> +
> +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0],
> [dnl
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"),
> action=(ct_snat(172.168.0.100);)
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"),
> action=(ct_snat(10.0.0.1);)
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"),
> action=(ct_snat(20.0.0.1);)
> +])
> +
> +check ovn-nbctl --wait=sb remove logical_router lr0 options chassis
> +
> +ovn-sbctl dump-flows lr0 > lr0flows
> +AT_CAPTURE_FILE([lr0flows])
> +
> +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0],
> [dnl
> +])
> +
> +check ovn-nbctl set logical_router lr0 options:chassis=ch1
> +check ovn-nbctl --wait=sb add logical_router_port lr0-sw1 networks
> "bef0\:\:1/64"
> +
> +ovn-sbctl dump-flows lr0 > lr0flows
> +AT_CAPTURE_FILE([lr0flows])
> +
> +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort], [0],
> [dnl
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"),
> action=(ct_snat(172.168.0.100);)
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"),
> action=(ct_snat(10.0.0.1);)
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"),
> action=(ct_snat(20.0.0.1);)
> + table=1 (lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw1"),
> action=(ct_snat(bef0::1);)
> +])
> +
> +AT_CLEANUP
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
