Thanks for the review. On Fri, Jan 27, 2017 at 10:20 AM, Guru Shetty <[email protected]> wrote:
> > > On 26 January 2017 at 01:20, Mickey Spiegel <[email protected]> wrote: > >> Currently in OVN, the "nat-addresses" in the "options" column of a >> logical switch port of type "router" must be specified manually. >> Typically the user would specify as "nat-addresses" all of the NAT >> external IP addresses and load balancer IP addresses that have >> already been specified separately on the router. >> >> This patch allows the logical switch port's "nat-addresses" to be >> specified as the string "router". When ovn-northd sees this string, >> it automatically copies the following into the southbound >> Port_Binding's "nat-addresses" in the "options" column: >> The options:router-port's MAC address. >> Each NAT external IP address (of any NAT type) specified on the >> logical router of options:router-port. >> Each load balancer IP address specified on the logical router of >> options:router-port. >> This will cause the controller where the gateway router resides to >> issue gratuitous ARPs for each NAT external IP address and for each >> load balancer IP address specified on the gateway router. >> >> This patch is written as if it will be included in OVS 2.7. If it >> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml >> will need to be updated from OVS 2.7 to OVS 2.8. >> >> Signed-off-by: Mickey Spiegel <[email protected]> >> --- >> ovn/northd/ovn-northd.c | 114 ++++++++++++++++++++++++++++++ >> ++++++++---------- >> ovn/ovn-nb.xml | 42 +++++++++++++++--- >> tests/ovn.at | 60 +++++++++++++++++++++++++ >> 3 files changed, 185 insertions(+), 31 deletions(-) >> >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index e24ff8f..efb8a74 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx, >> } >> >> static void >> +ip_address_and_port_from_lb_key(const char *key, char **ip_address, >> + uint16_t *port); >> + >> +static void >> +get_router_load_balancer_ips(const struct ovn_datapath *od, >> + struct sset *all_ips) >> +{ >> + if (!od->nbr) { >> + return; >> + } >> + >> + for (int i = 0; i < od->nbr->n_load_balancer; i++) { >> + struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; >> + struct smap *vips = &lb->vips; >> + struct smap_node *node; >> + >> + SMAP_FOR_EACH (node, vips) { >> + /* node->key contains IP:port or just IP. */ >> + char *ip_address = NULL; >> + uint16_t port; >> + >> + ip_address_and_port_from_lb_key(node->key, &ip_address, >> &port); >> + if (!ip_address) { >> + continue; >> + } >> + >> + if (!sset_contains(all_ips, ip_address)) { >> + sset_add(all_ips, ip_address); >> + } >> + >> + free(ip_address); >> + } >> + } >> +} >> + >> +/* Returns a string consisting of the port's MAC address followed by the >> + * external IP addresses of all NAT rules defined on that router and the >> + * VIPs of all load balancers defined on that router. */ >> > A comment that the called has to free the returned string is useful. > I can add that. > > Also, the load balancer IP address can also be the IP address of the > router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a > problem when a GARP is sent for the router IP too. > If the router IP is used as a load balancer IP address or as a SNAT address, then sending GARP for the router IP is a good thing. Mickey > > Acked-by: Gurucharan Shetty <[email protected]> > > > > >> +static char * >> +get_nat_addresses(const struct ovn_port *op) >> +{ >> + struct eth_addr mac; >> + if (!op->nbrp || !op->od || !op->od->nbr >> + || (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer) >> + || !eth_addr_from_string(op->nbrp->mac, &mac)) { >> + return NULL; >> + } >> + >> + struct ds addresses = DS_EMPTY_INITIALIZER; >> + ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); >> + >> + /* Get NAT IP addresses. */ >> + for (int i = 0; i < op->od->nbr->n_nat; i++) { >> + const struct nbrec_nat *nat; >> + nat = op->od->nbr->nat[i]; >> + >> + ovs_be32 ip, mask; >> + >> + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); >> + if (error || mask != OVS_BE32_MAX) { >> + free(error); >> + continue; >> + } >> + ds_put_format(&addresses, " %s", nat->external_ip); >> + } >> + >> + /* A set to hold all load-balancer vips. */ >> + struct sset all_ips = SSET_INITIALIZER(&all_ips); >> + get_router_load_balancer_ips(op->od, &all_ips); >> + >> + const char *ip_address; >> + SSET_FOR_EACH(ip_address, &all_ips) { >> + ds_put_format(&addresses, " %s", ip_address); >> + } >> + sset_destroy(&all_ips); >> + >> + return ds_steal_cstr(&addresses); >> +} >> + >> +static void >> ovn_port_update_sbrec(const struct ovn_port *op, >> struct hmap *chassis_qdisc_queues) >> { >> @@ -1524,7 +1604,15 @@ ovn_port_update_sbrec(const struct ovn_port *op, >> >> const char *nat_addresses = smap_get(&op->nbsp->options, >> "nat-addresses"); >> - if (nat_addresses) { >> + if (nat_addresses && !strcmp(nat_addresses, "router")) { >> + if (op->peer && op->peer->nbrp) { >> + char *nats = get_nat_addresses(op->peer); >> + if (nats) { >> + smap_add(&new, "nat-addresses", nats); >> + free(nats); >> + } >> + } >> + } else if (nat_addresses) { >> struct lport_addresses laddrs; >> if (!extract_lsp_addresses(nat_addresses, &laddrs)) { >> static struct vlog_rate_limit rl = >> @@ -3869,29 +3957,7 @@ 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); >> - >> - for (int i = 0; i < op->od->nbr->n_load_balancer; i++) { >> - struct nbrec_load_balancer *lb = >> op->od->nbr->load_balancer[i]; >> - struct smap *vips = &lb->vips; >> - struct smap_node *node; >> - >> - SMAP_FOR_EACH (node, vips) { >> - /* node->key contains IP:port or just IP. */ >> - char *ip_address = NULL; >> - uint16_t port; >> - >> - ip_address_and_port_from_lb_key(node->key, &ip_address, >> &port); >> - if (!ip_address) { >> - continue; >> - } >> - >> - if (!sset_contains(&all_ips, ip_address)) { >> - sset_add(&all_ips, ip_address); >> - } >> - >> - free(ip_address); >> - } >> - } >> + get_router_load_balancer_ips(op->od, &all_ips); >> >> const char *ip_address; >> SSET_FOR_EACH(ip_address, &all_ips) { >> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml >> index 2af46b6..57fbc98 100644 >> --- a/ovn/ovn-nb.xml >> +++ b/ovn/ovn-nb.xml >> @@ -242,13 +242,41 @@ >> </column> >> >> <column name="options" key="nat-addresses"> >> - MAC address of the <code>router-port</code> followed by a list >> of >> - SNAT and DNAT IP addresses. This is used to send gratuitous >> ARPs for >> - SNAT and DNAT IP addresses via <code>localnet</code> and is >> valid for >> - only L3 gateway ports. Example: <code>80:fa:5b:06:72:b7 >> 158.36.44.22 >> - 158.36.44.24</code>. This would result in generation of >> gratuitous >> - ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC >> - address of 80:fa:5b:06:72:b7. >> + <p> >> + This is used to send gratuitous ARPs for SNAT and DNAT IP >> + addresses via <code>localnet</code> and is valid for only L3 >> + gateway ports. >> + </p> >> + >> + <p> >> + This must take one of the following forms: >> + </p> >> + >> + <dl> >> + <dt><code>router</code></dt> >> + <dd> >> + <p> >> + Gratuitous ARPs will be sent for all SNAT and DNAT >> external IP >> + addresses and for all load balancer IP addresses defined >> on the >> + <ref column="options" key="router-port"/>'s logical >> router, >> + using the <ref column="options" key="router-port"/>'s MAC >> + address. >> + </p> >> + >> + <p> >> + Supported only in OVN 2.7 and later. Earlier versions >> required >> + NAT addresses to be manually synchronized. >> + </p> >> + </dd> >> + >> + <dt><code>Ethernet address followed by one or more IPv4 >> addresses</code></dt> >> + <dd> >> + Example: <code>80:fa:5b:06:72:b7 158.36.44.22 >> + 158.36.44.24</code>. This would result in generation of >> + gratuitous ARPs for IP addresses 158.36.44.22 and >> 158.36.44.24 >> + with a MAC address of 80:fa:5b:06:72:b7. >> + </dd> >> + </dl> >> </column> >> </group> >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 6eed020..febe00a 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -5219,6 +5219,66 @@ OVN_CLEANUP([hv1]) >> >> AT_CLEANUP >> >> +AT_SETUP([ovn -- send gratuitous arp with nat-addresses router in >> localnet]) >> +AT_SKIP_IF([test $HAVE_PYTHON = no]) >> +ovn_start >> +# Create logical switch >> +ovn-nbctl ls-add ls0 >> +# Create gateway router >> +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1 >> +# Add router port to gateway router >> +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.1/24 >> +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \ >> + type=router options:router-port=lrp0-rp >> addresses='"f0:00:00:00:00:01"' >> +# Add nat-address option >> +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router" >> +# Add NAT rules >> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.0.1 10.0.0.0/24]) >> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 192.168.0.2 10.0.0.1]) >> +# Add load balancers >> +AT_CHECK([ovn-nbctl lb-add lb0 192.168.0.3:80 10.0.0.2:80,10.0.0.3:80]) >> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0]) >> +AT_CHECK([ovn-nbctl lb-add lb1 192.168.0.3:8080 10.0.0.2:8080, >> 10.0.0.3:8080]) >> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1]) >> + >> +net_add n1 >> +sim_add hv1 >> +as hv1 >> +ovs-vsctl \ >> + -- add-br br-phys \ >> + -- add-br br-eth0 >> + >> +ovn_attach n1 br-phys 192.168.0.1 >> + >> +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappin >> gs=physnet1:br-eth0]) >> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif >> options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif- >> rx.pcap]) >> + >> +# Create a localnet port. >> +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port]) >> +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown]) >> +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet]) >> +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1]) >> + >> + >> +# Wait for packet to be received. >> +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 50]) >> +trim_zeros() { >> + sed 's/\(00\)\{1,\}$//' >> +} >> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | >> trim_zeros > packets >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80001000000000000c0a80001" >> +echo $expected > expout >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80002000000000000c0a80002" >> +echo $expected >> expout >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80003000000000000c0a80003" >> +echo $expected >> expout >> +AT_CHECK([sort packets], [0], [expout]) >> +cat packets >> + >> +OVN_CLEANUP([hv1]) >> + >> +AT_CLEANUP >> + >> AT_SETUP([ovn -- delete mac bindings]) >> ovn_start >> net_add n1 >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
