[ovs-dev] [PATCH ovn] northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.

Mark Gray mark.d.gray at redhat.com
Thu Feb 18 18:25:49 UTC 2021


On 18/02/2021 17:26, Numan Siddique wrote:
> On Thu, Feb 18, 2021 at 9:15 PM Mark Gray <mark.d.gray at redhat.com> wrote:
>>
>> On 18/02/2021 14:30, Numan Siddique wrote:
>>> On Wed, Feb 17, 2021 at 7:37 PM Mark Gray <mark.d.gray at redhat.com> wrote:
>>>>
>>>> Thanks Numan, some suggestions below!
>>>
>>> Hi Mark G,
>>>
>>> Thanks for the review.
>>>
>>> PSB for a few comments.
>>>
>>>>
>>>> On 09/02/2021 18:44, numans at ovn.org wrote:
>>>>> From: Numan Siddique <numans at 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?
>>>
>>>
>>> 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.

> 
> 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 <trozet at redhat.com>
>>>>> Signed-off-by: Numan Siddique <numans at 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?
>>>
>>> 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
>>>> dev at openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> 



More information about the dev mailing list