On 18/02/2021 14:30, Numan Siddique wrote: > On Wed, Feb 17, 2021 at 7:37 PM Mark Gray <[email protected]> wrote: >> >> Thanks Numan, some suggestions below! > > Hi Mark G, > > Thanks for the review. > > PSB for a few comments. > >> >> On 09/02/2021 18:44, [email protected] wrote: >>> From: Numan Siddique <[email protected]> >>> >>> 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? > > > Can you please elaborate more ? You mean ideally CMS should set > - router_ip ?
I was thinking that it could just be lb_force_snat_ip=true (default to remote_ip)? > > >>> >>> 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 <[email protected]> >>> Signed-off-by: Numan Siddique <[email protected]> >>> --- >>> 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? > > Great catch. I totally missed it. > >> >>> @@ -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." > > Ack. done > >> >>> + 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? > > Ack. I have added the log for this in v2. > > >> >>> + </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. > > In v2, (which I'll submit now), I'm not modifying this function at all. > > Also if "key_type' is NULL, then the code above this , which is > ---- > char *key = xasprintf("%s_force_snat_ip", key_type); > const char *addresses = smap_get(&od->nbr->options, key); > ----- > 'addresses' will be NULL and hence we will not hit this condition. > > >>> + 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? > > Ok. I get your point. My only reason to modify the function - > get_force_snat_ip() > was not to not log a warning if 'router_ip' is set. Ideally this > function should be called > if the option is a set of IP address(es). > > So in v2, I've not modified this function. But instead I first check > if lb_force_snat_ip > is configured with 'router_ip' or not. I felt there is probably no > need for a function just > for that. > >> >>> } >>> >>> @@ -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? > > I'm not too sure. I think we could. But we will end up with lots of macros. > > Please check out the v2. > > Thanks > Numan > >> >>> + 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 >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
