[ovs-dev] [PATCH] ovn: Allow IP packets destined to router ip for SNAT

[Guru Shetty]

On 21 June 2016 at 18:36, Chandra S Vejendla <csvejend at us.ibm.com> wrote:

> By default all the ip traffic destined to router ip is dropped in
> lr_in_ip_input stage. When the router ip is used as snat ip, allow
> reverse snat traffic destined to the router ip.
>
> Signed-off-by: Chandra Sekhar Vejendla <csvejend at us.ibm.com>
>

Thank you for the fix! This needs an update to ovn-northd.8.xml. If you are
happy with the following incremental which does that (and also adds your
name to AUTHORS and makes a couple of stylistic changes), I will apply it.

diff --git a/AUTHORS b/AUTHORS
index e2ac267..c39fdd3 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -39,6 +39,7 @@ Bruce Davie             bsd at nicira.com
 Bryan Phillippe         bp at toroki.com
 Carlo Andreotti         c.andreotti at m3s.it
 Casey Barker            crbarker at google.com
+Chandra Sekhar Vejendla csvejend at us.ibm.com
 Christoph Jaeger        cj at linux.com
 Chris Wright            chrisw at sous-sol.org
 Chuck Short             zulcss at ubuntu.com
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 22edba9..6d52f7e 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -631,7 +631,10 @@ output;
         handled by one of the flows above, which amounts to ICMP (other
than
         echo requests) and fragments with nonzero offsets.  For each IP
address
         <var>A</var> owned by the router, a priority-60 flow matches
-        <code>ip4.dst == <var>A</var></code> and drops the traffic.
+        <code>ip4.dst == <var>A</var></code> and drops the traffic.  An
+        exception is made and the above flow is not added if the router
+        port's own IP address is used to SNAT packets passing through that
+        router.
       </li>
     </ul>

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 6ff303e..1599e18 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2047,9 +2047,9 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
         }

         /* Drop IP traffic to this router, unless the router ip is used as
-         * snat ip. */
+         * SNAT ip. */
         bool snat_ip_is_router_ip = false;
-        for (int i = 0; i < op->od->nbr->n_nat && !snat_ip_is_router_ip;
i++) {
+        for (int i = 0; i < op->od->nbr->n_nat; i++) {
             const struct nbrec_nat *nat;
             ovs_be32 ip;

@@ -2057,14 +2057,17 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
             if (strcmp(nat->type, "snat")) {
                 continue;
             }
+
             if (!ip_parse(nat->external_ip, &ip) || !ip) {
                 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
                 VLOG_WARN_RL(&rl, "bad ip address %s in snat configuration
"
                          "for router %s", nat->external_ip, op->key);
                 continue;
             }
+
             if (ip == op->ip) {
                 snat_ip_is_router_ip = true;
+                break;
             }
         }




> ---
>  ovn/northd/ovn-northd.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 17713ec..6ff303e 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2046,11 +2046,34 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>              free(actions);
>          }
>
> -        /* Drop IP traffic to this router. */
> -        match = xasprintf("ip4.dst == "IP_FMT, IP_ARGS(op->ip));
> -        ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 60,
> -                      match, "drop;");
> -        free(match);
> +        /* Drop IP traffic to this router, unless the router ip is used as
> +         * snat ip. */
> +        bool snat_ip_is_router_ip = false;
> +        for (int i = 0; i < op->od->nbr->n_nat && !snat_ip_is_router_ip;
> i++) {
> +            const struct nbrec_nat *nat;
> +            ovs_be32 ip;
> +
> +            nat = op->od->nbr->nat[i];
> +            if (strcmp(nat->type, "snat")) {
> +                continue;
> +            }
> +            if (!ip_parse(nat->external_ip, &ip) || !ip) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "bad ip address %s in snat
> configuration "
> +                         "for router %s", nat->external_ip, op->key);
> +                continue;
> +            }
> +            if (ip == op->ip) {
> +                snat_ip_is_router_ip = true;
> +            }
> +        }
> +
> +        if (!snat_ip_is_router_ip) {
> +            match = xasprintf("ip4.dst == "IP_FMT, IP_ARGS(op->ip));
> +            ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 60, match,
> +                          "drop;");
> +            free(match);
> +        }
>      }
>
>      /* NAT in Gateway routers. */
> --
> 2.6.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>