| Message ID | 20210920132001.3675665-1-mheib@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Virtual port add ND/ARP responder flows for IPv6 VIPs. | expand |
On 9/20/21 3:20 PM, mheib@redhat.com wrote: > From: Mohammad Heib <mheib@redhat.com> > > currently ovn-northd only handle virtual ports with VIP IPv4 and ignores > virtual ports with VIP IPv6. > > This patch adds support for virtual ports with VIP IPv6 by adding > lflows to the lsp_in_arp_rsp logical switch pipeline. > Those lflows handle Neighbor Solicitations and Neighbor Advertisement requests > that target the virtual port VIPs and bind the virtual port to the desired VIF. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2003091 > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- Hi Mohammad, Thanks for the patch! This is not a full review yet, just some stuff I noticed while glancing over the patch. It would be great if you could add a test case in ovn-northd.at to make sure the logical flows are generated as expected. This would also ensure that we don't miss adding the northd-ddlog part (which is not present in this patch). > northd/northd.c | 52 +++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 10 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index d1b87891c..d85036dcc 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7386,16 +7386,30 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > * - ARP reply from the virtual ip which belongs to a logical > * port of type 'virtual' and bind that port. > * */ > - ovs_be32 ip; > + > + union ip { > + ovs_be32 ip; > + ovs_u128 ipv6; > + }ip; > + I think we should probably use in6_add and in6_addr_mapped_ipv4 instead of coming up with a new type. Regards, Dumitru
Hi Dumitru, Thanks for the review:) On 9/21/21 5:14 PM, Dumitru Ceara wrote: > On 9/20/21 3:20 PM, mheib@redhat.com wrote: >> From: Mohammad Heib <mheib@redhat.com> >> >> currently ovn-northd only handle virtual ports with VIP IPv4 and ignores >> virtual ports with VIP IPv6. >> >> This patch adds support for virtual ports with VIP IPv6 by adding >> lflows to the lsp_in_arp_rsp logical switch pipeline. >> Those lflows handle Neighbor Solicitations and Neighbor Advertisement requests >> that target the virtual port VIPs and bind the virtual port to the desired VIF. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2003091 >> Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") >> Signed-off-by: Mohammad Heib <mheib@redhat.com> >> --- > Hi Mohammad, > > Thanks for the patch! > > This is not a full review yet, just some stuff I noticed while glancing > over the patch. > > It would be great if you could add a test case in ovn-northd.at to make > sure the logical flows are generated as expected. This would also > ensure that we don't miss adding the northd-ddlog part (which is not > present in this patch). i submitted patch v2 [1] that adds some test cases for vip with ipv6, there is a test that targets virtual port with ipv4 i added some cases to this test to cover virtual port with ipv6 vip. [1] http://patchwork.ozlabs.org/project/ovn/patch/20210922175755.822094-1-mheib@redhat.com/ > >> northd/northd.c | 52 +++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 42 insertions(+), 10 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index d1b87891c..d85036dcc 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -7386,16 +7386,30 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, >> * - ARP reply from the virtual ip which belongs to a logical >> * port of type 'virtual' and bind that port. >> * */ >> - ovs_be32 ip; >> + >> + union ip { >> + ovs_be32 ip; >> + ovs_u128 ipv6; >> + }ip; >> + > I think we should probably use in6_add and in6_addr_mapped_ipv4 instead > of coming up with a new type. thanks, the v2 patch use the in6_add instead of adding new type. > > Regards, > Dumitru > thanks, Mohammad
On 20/09/2021 14:20, mheib@redhat.com wrote: > From: Mohammad Heib <mheib@redhat.com> > > currently ovn-northd only handle virtual ports with VIP IPv4 and ignores > virtual ports with VIP IPv6. > > This patch adds support for virtual ports with VIP IPv6 by adding > lflows to the lsp_in_arp_rsp logical switch pipeline. > Those lflows handle Neighbor Solicitations and Neighbor Advertisement requests > that target the virtual port VIPs and bind the virtual port to the desired VIF. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2003091 > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > northd/northd.c | 52 +++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 10 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index d1b87891c..d85036dcc 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7386,16 +7386,30 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > * - ARP reply from the virtual ip which belongs to a logical > * port of type 'virtual' and bind that port. > * */ I think this comment^ needs to be updated to reference ND Also the comment above the function should be updated as the table number is incorrect. You did not make this change but it would be good to update. > - ovs_be32 ip; > + > + union ip { > + ovs_be32 ip; > + ovs_u128 ipv6; > + }ip; I think you need to run `./utilities/checkpatch.py`? There seems to be a few whitespace issues. For example: "s/}ip;/} ip;}" and "s/else{/else {". I think some of the line lengths are over the recommended characters. I couldn't double-check on my machine because checkpatch, for some reason, was hanging. > + > const char *virtual_ip = smap_get(&op->nbsp->options, > "virtual-ip"); > const char *virtual_parents = smap_get(&op->nbsp->options, > "virtual-parents"); > - if (!virtual_ip || !virtual_parents || > - !ip_parse(virtual_ip, &ip)) { > + if (!virtual_ip || !virtual_parents) { > return; > } > > + bool is_ipv4 = strchr(virtual_ip, '.') ? true : false; > + if (is_ipv4) { > + if (!ip_parse(virtual_ip, &ip.ip)) { > + return; > + } > + } else{ > + if (!ipv6_parse(virtual_ip, (struct in6_addr *)&ip.ipv6)) > + return; > + } > + > char *tokstr = xstrdup(virtual_parents); > char *save_ptr = NULL; > char *vparent; > @@ -7408,13 +7422,31 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > continue; > } > > - ds_clear(match); > - ds_put_format(match, "inport == \"%s\" && " > - "((arp.op == 1 && arp.spa == %s && " > - "arp.tpa == %s) || (arp.op == 2 && " > - "arp.spa == %s))", > - vparent, virtual_ip, virtual_ip, > - virtual_ip); > + if (is_ipv4) { > + ds_clear(match); > + ds_put_format(match, "inport == \"%s\" && " > + "((arp.op == 1 && arp.spa == %s && " > + "arp.tpa == %s) || (arp.op == 2 && " > + "arp.spa == %s))", > + vparent, virtual_ip, virtual_ip, > + virtual_ip); > + } else{ > + struct ipv6_netaddr na; > + /* Find VIP multicast group */ > + in6_addr_solicited_node(&na.sn_addr, (struct in6_addr *)&ip.ipv6); > + inet_ntop(AF_INET6, &na.sn_addr, na.sn_addr_s, sizeof na.sn_addr_s); > + > + ds_clear(match); > + ds_put_format(match, "inport == \"%s\" && " > + "((nd_ns && ip6.dst == {%s, %s} && nd.target == %s) ||" > + "(nd_na && nd.target == %s))", > + vparent, > + virtual_ip, > + na.sn_addr_s, > + virtual_ip, > + virtual_ip); > + } > + > ds_clear(actions); > ds_put_format(actions, > "bind_vport(%s, inport); " >
Hi Mark thanks for the review. On 9/23/21 1:25 PM, Mark Gray wrote: > On 20/09/2021 14:20, mheib@redhat.com wrote: >> From: Mohammad Heib <mheib@redhat.com> >> >> currently ovn-northd only handle virtual ports with VIP IPv4 and ignores >> virtual ports with VIP IPv6. >> >> This patch adds support for virtual ports with VIP IPv6 by adding >> lflows to the lsp_in_arp_rsp logical switch pipeline. >> Those lflows handle Neighbor Solicitations and Neighbor Advertisement requests >> that target the virtual port VIPs and bind the virtual port to the desired VIF. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2003091 >> Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") >> Signed-off-by: Mohammad Heib <mheib@redhat.com> >> --- >> northd/northd.c | 52 +++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 42 insertions(+), 10 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index d1b87891c..d85036dcc 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -7386,16 +7386,30 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, >> * - ARP reply from the virtual ip which belongs to a logical >> * port of type 'virtual' and bind that port. >> * */ > I think this comment^ needs to be updated to reference ND > > Also the comment above the function should be updated as the table > number is incorrect. You did not make this change but it would be good > to update. i added two the comments and updated the table id according the table definition on ./northd/ovn_northd.dl i hope that is the right place to take the table id from. > >> - ovs_be32 ip; >> + >> + union ip { >> + ovs_be32 ip; >> + ovs_u128 ipv6; >> + }ip; > I think you need to run `./utilities/checkpatch.py`? There seems to be a > few whitespace issues. For example: "s/}ip;/} ip;}" and "s/else{/else > {". I think some of the line lengths are over the recommended > characters. I couldn't double-check on my machine because checkpatch, > for some reason, was hanging. i executed the checkpatch script before submitting the patch but it didn't complain about the else braces, only about the line max len. anyway i fixed both issues and submitted v3 patch <http://patchwork.ozlabs.org/project/ovn/patch/20210923122508.1012397-1-mheib@redhat.com/>. Thanks > >> + >> const char *virtual_ip = smap_get(&op->nbsp->options, >> "virtual-ip"); >> const char *virtual_parents = smap_get(&op->nbsp->options, >> "virtual-parents"); >> - if (!virtual_ip || !virtual_parents || >> - !ip_parse(virtual_ip, &ip)) { >> + if (!virtual_ip || !virtual_parents) { >> return; >> } >> >> + bool is_ipv4 = strchr(virtual_ip, '.') ? true : false; >> + if (is_ipv4) { >> + if (!ip_parse(virtual_ip, &ip.ip)) { >> + return; >> + } >> + } else{ >> + if (!ipv6_parse(virtual_ip, (struct in6_addr *)&ip.ipv6)) >> + return; >> + } >> + >> char *tokstr = xstrdup(virtual_parents); >> char *save_ptr = NULL; >> char *vparent; >> @@ -7408,13 +7422,31 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, >> continue; >> } >> >> - ds_clear(match); >> - ds_put_format(match, "inport == \"%s\" && " >> - "((arp.op == 1 && arp.spa == %s && " >> - "arp.tpa == %s) || (arp.op == 2 && " >> - "arp.spa == %s))", >> - vparent, virtual_ip, virtual_ip, >> - virtual_ip); >> + if (is_ipv4) { >> + ds_clear(match); >> + ds_put_format(match, "inport == \"%s\" && " >> + "((arp.op == 1 && arp.spa == %s && " >> + "arp.tpa == %s) || (arp.op == 2 && " >> + "arp.spa == %s))", >> + vparent, virtual_ip, virtual_ip, >> + virtual_ip); >> + } else{ >> + struct ipv6_netaddr na; >> + /* Find VIP multicast group */ >> + in6_addr_solicited_node(&na.sn_addr, (struct in6_addr *)&ip.ipv6); >> + inet_ntop(AF_INET6, &na.sn_addr, na.sn_addr_s, sizeof na.sn_addr_s); >> + >> + ds_clear(match); >> + ds_put_format(match, "inport == \"%s\" && " >> + "((nd_ns && ip6.dst == {%s, %s} && nd.target == %s) ||" >> + "(nd_na && nd.target == %s))", >> + vparent, >> + virtual_ip, >> + na.sn_addr_s, >> + virtual_ip, >> + virtual_ip); >> + } >> + >> ds_clear(actions); >> ds_put_format(actions, >> "bind_vport(%s, inport); " >>
diff --git a/northd/northd.c b/northd/northd.c index d1b87891c..d85036dcc 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7386,16 +7386,30 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, * - ARP reply from the virtual ip which belongs to a logical * port of type 'virtual' and bind that port. * */ - ovs_be32 ip; + + union ip { + ovs_be32 ip; + ovs_u128 ipv6; + }ip; + const char *virtual_ip = smap_get(&op->nbsp->options, "virtual-ip"); const char *virtual_parents = smap_get(&op->nbsp->options, "virtual-parents"); - if (!virtual_ip || !virtual_parents || - !ip_parse(virtual_ip, &ip)) { + if (!virtual_ip || !virtual_parents) { return; } + bool is_ipv4 = strchr(virtual_ip, '.') ? true : false; + if (is_ipv4) { + if (!ip_parse(virtual_ip, &ip.ip)) { + return; + } + } else{ + if (!ipv6_parse(virtual_ip, (struct in6_addr *)&ip.ipv6)) + return; + } + char *tokstr = xstrdup(virtual_parents); char *save_ptr = NULL; char *vparent; @@ -7408,13 +7422,31 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, continue; } - ds_clear(match); - ds_put_format(match, "inport == \"%s\" && " - "((arp.op == 1 && arp.spa == %s && " - "arp.tpa == %s) || (arp.op == 2 && " - "arp.spa == %s))", - vparent, virtual_ip, virtual_ip, - virtual_ip); + if (is_ipv4) { + ds_clear(match); + ds_put_format(match, "inport == \"%s\" && " + "((arp.op == 1 && arp.spa == %s && " + "arp.tpa == %s) || (arp.op == 2 && " + "arp.spa == %s))", + vparent, virtual_ip, virtual_ip, + virtual_ip); + } else{ + struct ipv6_netaddr na; + /* Find VIP multicast group */ + in6_addr_solicited_node(&na.sn_addr, (struct in6_addr *)&ip.ipv6); + inet_ntop(AF_INET6, &na.sn_addr, na.sn_addr_s, sizeof na.sn_addr_s); + + ds_clear(match); + ds_put_format(match, "inport == \"%s\" && " + "((nd_ns && ip6.dst == {%s, %s} && nd.target == %s) ||" + "(nd_na && nd.target == %s))", + vparent, + virtual_ip, + na.sn_addr_s, + virtual_ip, + virtual_ip); + } + ds_clear(actions); ds_put_format(actions, "bind_vport(%s, inport); "