| Message ID | 20180626184232.10399-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn: Add router load balancer undnat rule for IPv6 | expand |
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) || ", >
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) || ", >> >
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) || ", > >> > > >
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
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
>
>
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) || ",
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(-)