On Thu, Feb 18, 2021 at 11:56 PM Mark Gray <[email protected]> wrote: > > On 18/02/2021 17:26, Numan Siddique wrote: > > On Thu, Feb 18, 2021 at 9:15 PM Mark Gray <[email protected]> wrote: > >> > >> 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)? > > > > You mean 'router_ip' ? If it is 'true' then, IMHO, it is not clear with > > what IP > > to snat with. > > > > Since lb_force_snat_ip can take other IP addresses (which is the present > > case), > > I am not sure if mixing bool/string would be clear enough for the user. > > > > Let me know if you think it is obvious that it will be router ip if set to > > true. > > Let me rephrase. Is there a use case in which IP should be specified? > However, thinking about it more, I think I have answered myself, it > could be that if a link had multiple IPs, you would want to specify > which IP it should take.
Also right now with the present master, we do support IP for lb_force_snat_ip An example - https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L22669 Ok. Let me know if you are fine with the patch If I do the other changes ? I can definitely spin up another version if you prefer. Thanks Numan > > > > > Thanks > > Numan > > > > > >>> > >>> > >>>>> > >>>>> 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 > >> > > > > _______________________________________________ > 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
