[ovs-dev] ovn: Add router load balancer undnat rule for IPv6

Message ID 20180626184232.10399-1-mmichels@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev] ovn: Add router load balancer undnat rule for IPv6
Related show

Commit Message

Mark Michelson June 26, 2018, 6:42 p.m.
When configuring a router port to have a redirect-chassis and using an
IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
would not work as expected. This is because a rule to un-dnat the return
traffic from the load balancer destination was not installed. This is
because this rule was only being installed for IPv4 load balancers.

This change adds the same rule for IPv6 load balancers as well.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 ovn/northd/ovn-northd.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Mark Michelson June 26, 2018, 6:45 p.m. | #1
A note: if approved, this patch will also need to go into version 2.9

On 06/26/2018 02:42 PM, Mark Michelson wrote:
> When configuring a router port to have a redirect-chassis and using an
> IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
> would not work as expected. This is because a rule to un-dnat the return
> traffic from the load balancer destination was not installed. This is
> because this rule was only being installed for IPv4 load balancers.
> 
> This change adds the same rule for IPv6 load balancers as well.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
>   ovn/northd/ovn-northd.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 72fe4e795..2ca439b39 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -4641,8 +4641,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>       free(new_match);
>       free(est_match);
>   
> -    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
> -            || addr_family != AF_INET) {
> +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
>           return;
>       }
>   
> @@ -4651,7 +4650,11 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>        * router has a gateway router port associated.
>        */
>       struct ds undnat_match = DS_EMPTY_INITIALIZER;
> -    ds_put_cstr(&undnat_match, "ip4 && (");
> +    if (addr_family == AF_INET) {
> +        ds_put_cstr(&undnat_match, "ip4 && (");
> +    } else {
> +        ds_put_cstr(&undnat_match, "ip6 && (");
> +    }
>       char *start, *next, *ip_str;
>       start = next = xstrdup(backend_ips);
>       ip_str = strsep(&next, ",");
> @@ -4666,7 +4669,11 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>               break;
>           }
>   
> -        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> +        if (addr_family_ == AF_INET) {
> +            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> +        } else {
> +            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
> +        }
>           free(ip_address);
>           if (port) {
>               ds_put_format(&undnat_match, " && %s.src == %d) || ",
>
Mark Michelson July 10, 2018, 2:41 p.m. | #2
Hi Ben,

Thanks for applying this patch to master. Can you please apply the patch 
to the 2.9 branch as well?

Thank you,
Mark

On 06/26/2018 02:45 PM, Mark Michelson wrote:
> A note: if approved, this patch will also need to go into version 2.9
> 
> On 06/26/2018 02:42 PM, Mark Michelson wrote:
>> When configuring a router port to have a redirect-chassis and using an
>> IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
>> would not work as expected. This is because a rule to un-dnat the return
>> traffic from the load balancer destination was not installed. This is
>> because this rule was only being installed for IPv4 load balancers.
>>
>> This change adds the same rule for IPv6 load balancers as well.
>>
>> Signed-off-by: Mark Michelson <mmichels@redhat.com>
>> ---
>>   ovn/northd/ovn-northd.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index 72fe4e795..2ca439b39 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -4641,8 +4641,7 @@ add_router_lb_flow(struct hmap *lflows, struct 
>> ovn_datapath *od,
>>       free(new_match);
>>       free(est_match);
>> -    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
>> -            || addr_family != AF_INET) {
>> +    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
>>           return;
>>       }
>> @@ -4651,7 +4650,11 @@ add_router_lb_flow(struct hmap *lflows, struct 
>> ovn_datapath *od,
>>        * router has a gateway router port associated.
>>        */
>>       struct ds undnat_match = DS_EMPTY_INITIALIZER;
>> -    ds_put_cstr(&undnat_match, "ip4 && (");
>> +    if (addr_family == AF_INET) {
>> +        ds_put_cstr(&undnat_match, "ip4 && (");
>> +    } else {
>> +        ds_put_cstr(&undnat_match, "ip6 && (");
>> +    }
>>       char *start, *next, *ip_str;
>>       start = next = xstrdup(backend_ips);
>>       ip_str = strsep(&next, ",");
>> @@ -4666,7 +4669,11 @@ add_router_lb_flow(struct hmap *lflows, struct 
>> ovn_datapath *od,
>>               break;
>>           }
>> -        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
>> +        if (addr_family_ == AF_INET) {
>> +            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
>> +        } else {
>> +            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
>> +        }
>>           free(ip_address);
>>           if (port) {
>>               ds_put_format(&undnat_match, " && %s.src == %d) || ",
>>
>
Ben Pfaff July 10, 2018, 9:24 p.m. | #3
Done.

On Tue, Jul 10, 2018 at 10:41:32AM -0400, Mark Michelson wrote:
> Hi Ben,
> 
> Thanks for applying this patch to master. Can you please apply the patch to
> the 2.9 branch as well?
> 
> Thank you,
> Mark
> 
> On 06/26/2018 02:45 PM, Mark Michelson wrote:
> >A note: if approved, this patch will also need to go into version 2.9
> >
> >On 06/26/2018 02:42 PM, Mark Michelson wrote:
> >>When configuring a router port to have a redirect-chassis and using an
> >>IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
> >>would not work as expected. This is because a rule to un-dnat the return
> >>traffic from the load balancer destination was not installed. This is
> >>because this rule was only being installed for IPv4 load balancers.
> >>
> >>This change adds the same rule for IPv6 load balancers as well.
> >>
> >>Signed-off-by: Mark Michelson <mmichels@redhat.com>
> >>---
> >>  ovn/northd/ovn-northd.c | 15 +++++++++++----
> >>  1 file changed, 11 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> >>index 72fe4e795..2ca439b39 100644
> >>--- a/ovn/northd/ovn-northd.c
> >>+++ b/ovn/northd/ovn-northd.c
> >>@@ -4641,8 +4641,7 @@ add_router_lb_flow(struct hmap *lflows, struct
> >>ovn_datapath *od,
> >>      free(new_match);
> >>      free(est_match);
> >>-    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
> >>-            || addr_family != AF_INET) {
> >>+    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> >>          return;
> >>      }
> >>@@ -4651,7 +4650,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> >>ovn_datapath *od,
> >>       * router has a gateway router port associated.
> >>       */
> >>      struct ds undnat_match = DS_EMPTY_INITIALIZER;
> >>-    ds_put_cstr(&undnat_match, "ip4 && (");
> >>+    if (addr_family == AF_INET) {
> >>+        ds_put_cstr(&undnat_match, "ip4 && (");
> >>+    } else {
> >>+        ds_put_cstr(&undnat_match, "ip6 && (");
> >>+    }
> >>      char *start, *next, *ip_str;
> >>      start = next = xstrdup(backend_ips);
> >>      ip_str = strsep(&next, ",");
> >>@@ -4666,7 +4669,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> >>ovn_datapath *od,
> >>              break;
> >>          }
> >>-        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> >>+        if (addr_family_ == AF_INET) {
> >>+            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> >>+        } else {
> >>+            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
> >>+        }
> >>          free(ip_address);
> >>          if (port) {
> >>              ds_put_format(&undnat_match, " && %s.src == %d) || ",
> >>
> >
>
Flavio Leitner July 10, 2018, 9:44 p.m. | #4
Hi Ben and Mark,

This patch broke 2.9 build over here, see below.

On Tue, Jul 10, 2018 at 02:24:46PM -0700, Ben Pfaff wrote:
> Done.
> 
> On Tue, Jul 10, 2018 at 10:41:32AM -0400, Mark Michelson wrote:
> > Hi Ben,
> > 
> > Thanks for applying this patch to master. Can you please apply the patch to
> > the 2.9 branch as well?
> > 
> > Thank you,
> > Mark
> > 
> > On 06/26/2018 02:45 PM, Mark Michelson wrote:
> > >A note: if approved, this patch will also need to go into version 2.9
> > >
> > >On 06/26/2018 02:42 PM, Mark Michelson wrote:
> > >>When configuring a router port to have a redirect-chassis and using an
> > >>IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
> > >>would not work as expected. This is because a rule to un-dnat the return
> > >>traffic from the load balancer destination was not installed. This is
> > >>because this rule was only being installed for IPv4 load balancers.
> > >>
> > >>This change adds the same rule for IPv6 load balancers as well.
> > >>
> > >>Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > >>---
> > >>  ovn/northd/ovn-northd.c | 15 +++++++++++----
> > >>  1 file changed, 11 insertions(+), 4 deletions(-)
> > >>
> > >>diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > >>index 72fe4e795..2ca439b39 100644
> > >>--- a/ovn/northd/ovn-northd.c
> > >>+++ b/ovn/northd/ovn-northd.c
> > >>@@ -4641,8 +4641,7 @@ add_router_lb_flow(struct hmap *lflows, struct
> > >>ovn_datapath *od,
> > >>      free(new_match);
> > >>      free(est_match);
> > >>-    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
> > >>-            || addr_family != AF_INET) {
> > >>+    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > >>          return;
> > >>      }
> > >>@@ -4651,7 +4650,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> > >>ovn_datapath *od,
> > >>       * router has a gateway router port associated.
> > >>       */
> > >>      struct ds undnat_match = DS_EMPTY_INITIALIZER;
> > >>-    ds_put_cstr(&undnat_match, "ip4 && (");
> > >>+    if (addr_family == AF_INET) {
> > >>+        ds_put_cstr(&undnat_match, "ip4 && (");
> > >>+    } else {
> > >>+        ds_put_cstr(&undnat_match, "ip6 && (");
> > >>+    }
> > >>      char *start, *next, *ip_str;
> > >>      start = next = xstrdup(backend_ips);
> > >>      ip_str = strsep(&next, ",");
> > >>@@ -4666,7 +4669,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> > >>ovn_datapath *od,
> > >>              break;
> > >>          }
> > >>-        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > >>+        if (addr_family_ == AF_INET) {
                   ^^^^^^^^^^^^ extra _?      


> > >>+            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > >>+        } else {
> > >>+            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
> > >>+        }
> > >>          free(ip_address);
> > >>          if (port) {
> > >>              ds_put_format(&undnat_match, " && %s.src == %d) || ",
> > >>
> > >
> > 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff July 10, 2018, 10:25 p.m. | #5
Oops.  I sent a fix:
        https://patchwork.ozlabs.org/patch/942271/
I forgot to add you in Reported-by but I'll do that before pushing.

On Tue, Jul 10, 2018 at 06:44:15PM -0300, Flavio Leitner wrote:
> 
> Hi Ben and Mark,
> 
> This patch broke 2.9 build over here, see below.
> 
> On Tue, Jul 10, 2018 at 02:24:46PM -0700, Ben Pfaff wrote:
> > Done.
> > 
> > On Tue, Jul 10, 2018 at 10:41:32AM -0400, Mark Michelson wrote:
> > > Hi Ben,
> > > 
> > > Thanks for applying this patch to master. Can you please apply the patch to
> > > the 2.9 branch as well?
> > > 
> > > Thank you,
> > > Mark
> > > 
> > > On 06/26/2018 02:45 PM, Mark Michelson wrote:
> > > >A note: if approved, this patch will also need to go into version 2.9
> > > >
> > > >On 06/26/2018 02:42 PM, Mark Michelson wrote:
> > > >>When configuring a router port to have a redirect-chassis and using an
> > > >>IPv6 load balancer rule that specifies a TCP/UDP port, load balancing
> > > >>would not work as expected. This is because a rule to un-dnat the return
> > > >>traffic from the load balancer destination was not installed. This is
> > > >>because this rule was only being installed for IPv4 load balancers.
> > > >>
> > > >>This change adds the same rule for IPv6 load balancers as well.
> > > >>
> > > >>Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > > >>---
> > > >>  ovn/northd/ovn-northd.c | 15 +++++++++++----
> > > >>  1 file changed, 11 insertions(+), 4 deletions(-)
> > > >>
> > > >>diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > >>index 72fe4e795..2ca439b39 100644
> > > >>--- a/ovn/northd/ovn-northd.c
> > > >>+++ b/ovn/northd/ovn-northd.c
> > > >>@@ -4641,8 +4641,7 @@ add_router_lb_flow(struct hmap *lflows, struct
> > > >>ovn_datapath *od,
> > > >>      free(new_match);
> > > >>      free(est_match);
> > > >>-    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
> > > >>-            || addr_family != AF_INET) {
> > > >>+    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > > >>          return;
> > > >>      }
> > > >>@@ -4651,7 +4650,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> > > >>ovn_datapath *od,
> > > >>       * router has a gateway router port associated.
> > > >>       */
> > > >>      struct ds undnat_match = DS_EMPTY_INITIALIZER;
> > > >>-    ds_put_cstr(&undnat_match, "ip4 && (");
> > > >>+    if (addr_family == AF_INET) {
> > > >>+        ds_put_cstr(&undnat_match, "ip4 && (");
> > > >>+    } else {
> > > >>+        ds_put_cstr(&undnat_match, "ip6 && (");
> > > >>+    }
> > > >>      char *start, *next, *ip_str;
> > > >>      start = next = xstrdup(backend_ips);
> > > >>      ip_str = strsep(&next, ",");
> > > >>@@ -4666,7 +4669,11 @@ add_router_lb_flow(struct hmap *lflows, struct
> > > >>ovn_datapath *od,
> > > >>              break;
> > > >>          }
> > > >>-        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > > >>+        if (addr_family_ == AF_INET) {
>                    ^^^^^^^^^^^^ extra _?      
> 
> 
> > > >>+            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
> > > >>+        } else {
> > > >>+            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
> > > >>+        }
> > > >>          free(ip_address);
> > > >>          if (port) {
> > > >>              ds_put_format(&undnat_match, " && %s.src == %d) || ",
> > > >>
> > > >
> > > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> -- 
> Flavio
> 
>

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 72fe4e795..2ca439b39 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -4641,8 +4641,7 @@  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
     free(new_match);
     free(est_match);
 
-    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips
-            || addr_family != AF_INET) {
+    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
         return;
     }
 
@@ -4651,7 +4650,11 @@  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
      * router has a gateway router port associated.
      */
     struct ds undnat_match = DS_EMPTY_INITIALIZER;
-    ds_put_cstr(&undnat_match, "ip4 && (");
+    if (addr_family == AF_INET) {
+        ds_put_cstr(&undnat_match, "ip4 && (");
+    } else {
+        ds_put_cstr(&undnat_match, "ip6 && (");
+    }
     char *start, *next, *ip_str;
     start = next = xstrdup(backend_ips);
     ip_str = strsep(&next, ",");
@@ -4666,7 +4669,11 @@  add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
             break;
         }
 
-        ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
+        if (addr_family_ == AF_INET) {
+            ds_put_format(&undnat_match, "(ip4.src == %s", ip_address);
+        } else {
+            ds_put_format(&undnat_match, "(ip6.src == %s", ip_address);
+        }
         free(ip_address);
         if (port) {
             ds_put_format(&undnat_match, " && %s.src == %d) || ",