[ovs-dev] [PATCH, v2] ovn: Allow SNAT traffic destined to router ip

[Chandra Sekhar Vejendla]

Guru Shetty <guru at ovn.org> wrote on 07/18/2016 08:03:52 AM:

> From: Guru Shetty <guru at ovn.org>
> To: Chandra Sekhar Vejendla/San Jose/IBM at IBMUS
> Cc: ovs dev <dev at openvswitch.org>
> Date: 07/18/2016 08:04 AM
> Subject: Re: [ovs-dev] [PATCH, v2] ovn: Allow SNAT traffic destined
> to router ip
>
> On 18 July 2016 at 05:22, Chandra S Vejendla <csvejend at us.ibm.com> wrote:
> When router ip is used as SNAT IP, traffic destined to router
> ip should not be dropped
>
> Thank you for the fix. You will need to add your Signed-off-by. Can
> you also add a "Fixes:" tag in commit message. Since this is a
> regression, I wonder whether we should add a simple unit test that
> looks at the generated logical flows to make sure that there is no
> "drop" for the SNAT IP address.
>

I'll send out patch v3 with 'Signed-off-by' & 'Fixes' tag.
Yes, i agree that we should have a unit test for this. If its fine with you
I would like to cover the unit test case in a different patch.

> I also wonder, whether we should disable ICMP response to these SNAT
> IP addresses from the router? Don't you see issues with that? I
> presume you will have a situation where the ICMP response flow added
> by the router will override the  SNAT flow and the router will
> respond to ICMP instead of the logical port.

Yes, i agree. ICMP responses should be disabled.
>
>
> ---
>  ovn/northd/ovn-northd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 7ce509d..78c3a7d 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2399,11 +2399,16 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
>          ds_put_cstr(&match, "ip4.dst == {");
>          bool has_drop_ips = false;
>          for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +            bool nat_ip_is_router_ip = false;
>              for (int j = 0; j < n_nat_ips; j++) {
>                  if (op->lrp_networks.ipv4_addrs[i].addr == nat_ips[j]) {
> -                    continue;
> +                    nat_ip_is_router_ip = true;
> +                    break;
>                  }
>              }
> +            if (nat_ip_is_router_ip) {
> +                continue;
> +            }
>              ds_put_format(&match, "%s, ",
>                            op->lrp_networks.ipv4_addrs[i].addr_s);
>              has_drop_ips = true;
> --
> 2.6.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev