On Tue, Nov 12, 2019 at 9:02 AM Han Zhou <zhou...@gmail.com> wrote:
>
>
>
> On Tue, Nov 12, 2019 at 2:28 AM Dumitru Ceara <dce...@redhat.com> wrote:
> >
> > Function get_router_load_balancer_ips() was incorrectly returning a
> > single address_family even though the IP set could contain a mix of
> > IPv4 and IPv6 addresses.
> >
> > Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> > ---
> > northd/ovn-northd.c | 126
+++++++++++++++++++++++++++++----------------------
> > 1 file changed, 72 insertions(+), 54 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 2f0f501..32f3200 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -2184,7 +2184,7 @@ ip_address_and_port_from_lb_key(const char *key,
char **ip_address,
> >
> > static void
> > get_router_load_balancer_ips(const struct ovn_datapath *od,
> > - struct sset *all_ips, int *addr_family)
> > + struct sset *all_ips_v4, struct sset
*all_ips_v6)
> > {
> > if (!od->nbr) {
> > return;
> > @@ -2199,13 +2199,21 @@ get_router_load_balancer_ips(const struct
ovn_datapath *od,
> > /* node->key contains IP:port or just IP. */
> > char *ip_address = NULL;
> > uint16_t port;
> > + int addr_family;
> >
> > ip_address_and_port_from_lb_key(node->key, &ip_address,
&port,
> > - addr_family);
> > + &addr_family);
> > if (!ip_address) {
> > continue;
> > }
> >
> > + struct sset *all_ips;
> > + if (addr_family == AF_INET) {
> > + all_ips = all_ips_v4;
> > + } else {
> > + all_ips = all_ips_v6;
> > + }
> > +
> > if (!sset_contains(all_ips, ip_address)) {
> > sset_add(all_ips, ip_address);
> > }
> > @@ -2299,17 +2307,22 @@ get_nat_addresses(const struct ovn_port *op,
size_t *n)
> > }
> > }
> >
> > - /* A set to hold all load-balancer vips. */
> > - struct sset all_ips = SSET_INITIALIZER(&all_ips);
> > - int addr_family;
> > - get_router_load_balancer_ips(op->od, &all_ips, &addr_family);
> > + /* Two sets to hold all load-balancer vips. */
> > + struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4);
> > + struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6);
> > + get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
> >
> > const char *ip_address;
> > - SSET_FOR_EACH (ip_address, &all_ips) {
> > + SSET_FOR_EACH (ip_address, &all_ips_v4) {
> > ds_put_format(&c_addresses, " %s", ip_address);
> > central_ip_address = true;
> > }
> > - sset_destroy(&all_ips);
> > + SSET_FOR_EACH (ip_address, &all_ips_v6) {
> > + ds_put_format(&c_addresses, " %s", ip_address);
> > + central_ip_address = true;
> > + }
> > + sset_destroy(&all_ips_v4);
> > + sset_destroy(&all_ips_v6);
> >
> > if (central_ip_address) {
> > /* Gratuitous ARP for centralized NAT rules on distributed
gateway
> > @@ -6911,61 +6924,66 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
> > }
> >
> > /* A set to hold all load-balancer vips that need ARP
responses. */
> > - struct sset all_ips = SSET_INITIALIZER(&all_ips);
> > - int addr_family;
> > - get_router_load_balancer_ips(op->od, &all_ips, &addr_family);
> > + struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4);
> > + struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6);
> > + get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
> >
> > const char *ip_address;
> > - SSET_FOR_EACH(ip_address, &all_ips) {
> > + SSET_FOR_EACH (ip_address, &all_ips_v4) {
> > ds_clear(&match);
> > - if (addr_family == AF_INET) {
> > - ds_put_format(&match,
> > - "inport == %s && arp.tpa == %s && arp.op
== 1",
> > - op->json_key, ip_address);
> > - } else {
> > - ds_put_format(&match,
> > - "inport == %s && nd_ns && nd.target ==
%s",
> > - op->json_key, ip_address);
> > - }
> > + ds_put_format(&match,
> > + "inport == %s && arp.tpa == %s && arp.op ==
1",
> > + op->json_key, ip_address);
> >
> > ds_clear(&actions);
> > - if (addr_family == AF_INET) {
> > - ds_put_format(&actions,
> > - "eth.dst = eth.src; "
> > - "eth.src = %s; "
> > - "arp.op = 2; /* ARP reply */ "
> > - "arp.tha = arp.sha; "
> > - "arp.sha = %s; "
> > - "arp.tpa = arp.spa; "
> > - "arp.spa = %s; "
> > - "outport = %s; "
> > - "flags.loopback = 1; "
> > - "output;",
> > - op->lrp_networks.ea_s,
> > - op->lrp_networks.ea_s,
> > - ip_address,
> > - op->json_key);
> > - } else {
> > - ds_put_format(&actions,
> > - "nd_na { "
> > - "eth.src = %s; "
> > - "ip6.src = %s; "
> > - "nd.target = %s; "
> > - "nd.tll = %s; "
> > - "outport = inport; "
> > - "flags.loopback = 1; "
> > - "output; "
> > - "};",
> > - op->lrp_networks.ea_s,
> > - ip_address,
> > - ip_address,
> > - op->lrp_networks.ea_s);
> > - }
> > + ds_put_format(&actions,
> > + "eth.dst = eth.src; "
> > + "eth.src = %s; "
> > + "arp.op = 2; /* ARP reply */ "
> > + "arp.tha = arp.sha; "
> > + "arp.sha = %s; "
> > + "arp.tpa = arp.spa; "
> > + "arp.spa = %s; "
> > + "outport = %s; "
> > + "flags.loopback = 1; "
> > + "output;",
> > + op->lrp_networks.ea_s,
> > + op->lrp_networks.ea_s,
> > + ip_address,
> > + op->json_key);
> > +
> > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
> > ds_cstr(&match), ds_cstr(&actions));
> > }
> >
> > - sset_destroy(&all_ips);
> > + SSET_FOR_EACH (ip_address, &all_ips_v6) {
> > + ds_clear(&match);
> > + ds_put_format(&match,
> > + "inport == %s && nd_ns && nd.target == %s",
> > + op->json_key, ip_address);
> > +
> > + ds_clear(&actions);
> > + ds_put_format(&actions,
> > + "nd_na { "
> > + "eth.src = %s; "
> > + "ip6.src = %s; "
> > + "nd.target = %s; "
> > + "nd.tll = %s; "
> > + "outport = inport; "
> > + "flags.loopback = 1; "
> > + "output; "
> > + "};",
> > + op->lrp_networks.ea_s,
> > + ip_address,
> > + ip_address,
> > + op->lrp_networks.ea_s);
> > +
> > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
> > + ds_cstr(&match), ds_cstr(&actions));
> > + }
> > +
> > + sset_destroy(&all_ips_v4);
> > + sset_destroy(&all_ips_v6);
> >
> > /* A gateway router can have 2 SNAT IP addresses to force
DNATed and
> > * LBed traffic respectively to be SNATed. In addition, there
can be
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Thanks Dumitru. I applied this to master.
I am so sorry that when applying this, I made a terrible mistake and used
the commit message from the 2/2 patch in this series. I realized it after
~3 hours.
Since the commit was right and only the message was wrong, I think
reverting it and recommitting a new patch just with message changed may
cause more confusion, so I just corrected it with a --force commit.
If there were pull request during this timeframe then the next pull will
encounter a conflict. I'll take the blame for that.
Thanks,
Han
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
