On Wed, Jun 30, 2021 at 7:57 PM Mark Michelson <mmich...@redhat.com> wrote: > > Load_Balancer and NAT entries have a new option, "add_route" that can be > set to automatically add routes to those addresses to neighbor routers, > therefore eliminating the need to create static routes. > > Signed-off-by: Mark Michelson <mmich...@redhat.com>
Acked-by: Numan Siddique <num...@ovn.org> There is one small comment which you can choose to ignore. Please see below. Numan > --- > northd/ovn-northd.8.xml | 7 ++++- > northd/ovn-northd.c | 57 +++++++++++++++++++++++++++++++++-------- > northd/ovn_northd.dl | 23 ++++++++++++----- > ovn-nb.xml | 33 +++++++++++++++++++++++- > tests/ovn-nbctl.at | 3 +++ > tests/ovn-northd.at | 40 ++++++++++++++++++++++++++--- > utilities/ovn-nbctl.c | 25 +++++++++++++----- > 7 files changed, 158 insertions(+), 30 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index b5c961e89..beaf5a183 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -3539,7 +3539,12 @@ outport = <var>P</var> > <ref column="external_mac" table="NAT" db="OVN_Northbound"/> column > of <ref table="NAT" db="OVN_Northbound"/> table for of type > <code>dnat_and_snat</code>, otherwise the Ethernet address of the > - distributed logical router port. > + distributed logical router port. Note that if the > + <ref column="external_ip" table="NAT" db="OVN_Northbound"/> is not > + within a subnet on the owning logical router, then OVN will only > + create ARP resolution flows if the <ref > column="options:add_route"/> > + is set to <code>true</code>. Otherwise, no ARP resolution flows > + will be added. > </p> > > <p> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 58132bc5c..f6fad281b 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -662,8 +662,14 @@ struct ovn_datapath { > struct lport_addresses dnat_force_snat_addrs; > struct lport_addresses lb_force_snat_addrs; > bool lb_force_snat_router_ip; > + /* The "routable" ssets are subsets of the load balancer > + * IPs for which IP routes and ARP resolution flows are automatically > + * added > + */ > struct sset lb_ips_v4; > + struct sset lb_ips_v4_routable; > struct sset lb_ips_v6; > + struct sset lb_ips_v6_routable; > > struct ovn_port **localnet_ports; > size_t n_localnet_ports; > @@ -834,7 +840,9 @@ static void > init_lb_ips(struct ovn_datapath *od) > { > sset_init(&od->lb_ips_v4); > + sset_init(&od->lb_ips_v4_routable); > sset_init(&od->lb_ips_v6); > + sset_init(&od->lb_ips_v6_routable); > } > > static void > @@ -845,7 +853,9 @@ destroy_lb_ips(struct ovn_datapath *od) > } > > sset_destroy(&od->lb_ips_v4); > + sset_destroy(&od->lb_ips_v4_routable); > sset_destroy(&od->lb_ips_v6); > + sset_destroy(&od->lb_ips_v6_routable); > } > > /* A group of logical router datapaths which are connected - either > @@ -1475,13 +1485,14 @@ destroy_routable_addresses(struct > ovn_port_routable_addresses *ra) > free(ra->laddrs); > } > > -static char **get_nat_addresses(const struct ovn_port *op, size_t *n); > +static char **get_nat_addresses(const struct ovn_port *op, size_t *n, > + bool routable_only); > > static void > assign_routable_addresses(struct ovn_port *op) > { > size_t n; > - char **nats = get_nat_addresses(op, &n); > + char **nats = get_nat_addresses(op, &n, true); > > if (!nats) { > return; > @@ -2541,7 +2552,7 @@ join_logical_ports(struct northd_context *ctx, > * The caller must free each of the n returned strings with free(), > * and must free the returned array when it is no longer needed. */ > static char ** > -get_nat_addresses(const struct ovn_port *op, size_t *n) > +get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only) > { > size_t n_nats = 0; > struct eth_addr mac; > @@ -2564,6 +2575,12 @@ get_nat_addresses(const struct ovn_port *op, size_t *n) > const struct nbrec_nat *nat = op->od->nbr->nat[i]; > ovs_be32 ip, mask; > > + if (routable_only && > + (!strcmp(nat->type, "snat") || > + !smap_get_bool(&nat->options, "add_route", false))) { > + continue; > + } > + > char *error = ip_parse_masked(nat->external_ip, &ip, &mask); > if (error || mask != OVS_BE32_MAX) { > free(error); > @@ -2615,13 +2632,24 @@ get_nat_addresses(const struct ovn_port *op, size_t > *n) > } > > const char *ip_address; > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) { > - ds_put_format(&c_addresses, " %s", ip_address); > - central_ip_address = true; > - } > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { > - ds_put_format(&c_addresses, " %s", ip_address); > - central_ip_address = true; > + if (routable_only) { > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) { > + ds_put_format(&c_addresses, " %s", ip_address); > + central_ip_address = true; > + } > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) { > + ds_put_format(&c_addresses, " %s", ip_address); > + central_ip_address = true; > + } > + } else { > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) { > + ds_put_format(&c_addresses, " %s", ip_address); > + central_ip_address = true; > + } > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { > + ds_put_format(&c_addresses, " %s", ip_address); > + central_ip_address = true; > + } > } > > if (central_ip_address) { > @@ -3133,7 +3161,7 @@ ovn_port_update_sbrec(struct northd_context *ctx, > if (nat_addresses && !strcmp(nat_addresses, "router")) { > if (op->peer && op->peer->od > && (chassis || op->peer->od->l3redirect_port)) { > - nats = get_nat_addresses(op->peer, &n_nats); > + nats = get_nat_addresses(op->peer, &n_nats, false); > } > /* Only accept manual specification of ethernet address > * followed by IPv4 addresses on type "l3gateway" ports. */ > @@ -3606,11 +3634,18 @@ build_lrouter_lbs(struct hmap *datapaths, struct hmap > *lbs) > ovn_northd_lb_find(lbs, > &od->nbr->load_balancer[i]->header_.uuid); > const char *ip_address; > + bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", > false); > SSET_FOR_EACH (ip_address, &lb->ips_v4) { > sset_add(&od->lb_ips_v4, ip_address); > + if (is_routable) { > + sset_add(&od->lb_ips_v4_routable, ip_address); > + } > } > SSET_FOR_EACH (ip_address, &lb->ips_v6) { > sset_add(&od->lb_ips_v6, ip_address); > + if (is_routable) { > + sset_add(&od->lb_ips_v6_routable, ip_address); > + } > } > } > } > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index 35d40ff5a..c9ee742d1 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -201,7 +201,7 @@ OutProxy_Port_Binding(._uuid = lsp._uuid, > Some{"router"} -> match ((l3dgw_port, opt_chassis, peer)) { > (None, None, _) -> set_empty(), > (_, _, None) -> set_empty(), > - (_, _, Some{rport}) -> > get_nat_addresses(rport) > + (_, _, Some{rport}) -> > get_nat_addresses(rport, false) > }, > Some{nat_addresses} -> { > /* Only accept manual specification of ethernet address > @@ -298,12 +298,16 @@ OutProxy_Port_Binding(._uuid = lrp._uuid, > }. > /* > */ > -function get_router_load_balancer_ips(router: Intern<Router>) : > +function get_router_load_balancer_ips(router: Intern<Router>, > + routable_only: bool) : > (Set<string>, Set<string>) = > { > var all_ips_v4 = set_empty(); > var all_ips_v6 = set_empty(); > for (lb in router.lbs) { > + if (routable_only and not lb.options.get_bool_def("add_route", > false)) { > + continue; > + }; > for (kv in lb.vips) { > (var vip, _) = kv; > /* node->key contains IP:port or just IP. */ > @@ -325,7 +329,7 @@ function get_router_load_balancer_ips(router: > Intern<Router>) : > * external IP addresses of all NAT rules defined on that router, and all > * of the IP addresses used in load balancer VIPs defined on that router. > */ > -function get_nat_addresses(rport: Intern<RouterPort>): Set<string> = > +function get_nat_addresses(rport: Intern<RouterPort>, routable_only: bool): > Set<string> = > { > var addresses = set_empty(); > var has_redirect = rport.router.l3dgw_port.is_some(); > @@ -337,6 +341,11 @@ function get_nat_addresses(rport: Intern<RouterPort>): > Set<string> = > > /* Get NAT IP addresses. */ > for (nat in rport.router.nats) { > + if (routable_only and > + (nat.nat.__type == "snat" or > + not nat.nat.options.get_bool_def("add_route", false))) { > + continue; > + }; > /* Determine whether this NAT rule satisfies the conditions > for > * distributed NAT processing. */ > if (has_redirect and nat.nat.__type == "dnat_and_snat" and > @@ -379,7 +388,7 @@ function get_nat_addresses(rport: Intern<RouterPort>): > Set<string> = > }; > > /* A set to hold all load-balancer vips. */ > - (var all_ips_v4, var all_ips_v6) = > get_router_load_balancer_ips(rport.router); > + (var all_ips_v4, var all_ips_v6) = > get_router_load_balancer_ips(rport.router, routable_only); > > for (ip_address in set_union(all_ips_v4, all_ips_v6)) { > c_addresses = c_addresses ++ " ${ip_address}"; > @@ -4105,7 +4114,7 @@ function get_arp_forward_ips(rp: Intern<RouterPort>): > (Set<string>, Set<string>) > var all_ips_v6 = set_empty(); > > (var lb_ips_v4, var lb_ips_v6) > - = get_router_load_balancer_ips(rp.router); > + = get_router_load_balancer_ips(rp.router, false); > for (a in lb_ips_v4) { > /* Check if the ovn port has a network configured on which we could > * expect ARP requests for the LB VIP. > @@ -5090,7 +5099,7 @@ var residence_check = match (is_redirect) { > true -> Some{"is_chassis_resident(${router.redirect_port_name})"}, > false -> None > } in { > - (var all_ips_v4, _) = get_router_load_balancer_ips(router) in { > + (var all_ips_v4, _) = get_router_load_balancer_ips(router, false) in { > if (not all_ips_v4.is_empty()) { > LogicalRouterArpFlow(.lr = router, > .lrp = Some{lrp}, > @@ -6504,7 +6513,7 @@ relation RouterPortRoutableAddresses( > addresses: Set<lport_addresses>) > RouterPortRoutableAddresses(port.lrp._uuid, addresses) :- > port in &RouterPort(.is_redirect = true), > - var addresses = get_nat_addresses(port).filter_map(extract_addresses), > + var addresses = get_nat_addresses(port, > true).filter_map(extract_addresses), > addresses != set_empty(). > > /* Return a vector of pairs (1, set[0]), ... (n, set[n - 1]). */ > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 36a77097c..ad6deb059 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -1715,6 +1715,18 @@ > option, the force_snat_for_lb option configured for the router > pipeline will not be applied for this load balancer. > </column> > + > + <column name="options" key="add_route"> > + If set to <code>true</code>, then neighbor routers will have logical > + flows added that will allow for routing to the VIP IP. It also will > + have ARP resolution logical flows added. By setting this option, it > + means there is no reason to create a > + <ref table="Logical_Router_Static_Route"/> from neighbor routers to > + this NAT address. It also means that no ARP request is required for > + neighbor routers to learn the IP-MAC mapping for this VIP IP. For > + more information about what flows are added for IP routes, please > + see the <code>ovn-northd</code> manpage section on IP Routing. > + </column> > </group> > </table> > > @@ -2059,7 +2071,13 @@ > <code>false</code> by default. It is recommended to set to > <code>true</code> when a large number of logical routers are > connected to the same logical switch but most of them never need to > - send traffic between each other. > + send traffic between each other. By default, ovn-northd does not > + create mappings to NAT and load balancer addresess. However, for > NAT > + and load balancer addresses that have the <code>add_route</code> > + option added, ovn-northd will create logical flows that map NAT and > + load balancer IP addresses to the appropriate MAC address. Setting > + <var>dynamic_neigh_routers</var> to <code>true</code> will prevent > + the automatic creation of these logical flows. > </p> > </column> > <column name="options" key="always_learn_from_arp_request" > type='{"type": "boolean"}'> > @@ -3032,6 +3050,19 @@ > tracking state or not. > </column> > > + <column name="options" key="add_route"> > + If set to <code>true</code>, then neighbor routers will have logical > + flows added that will allow for routing to the NAT address. It also > will > + have ARP resolution logical flows added. By setting this option, it > means > + there is no reason to create a <ref > table="Logical_Router_Static_Route"/> > + from neighbor routers to this NAT address. It also means that no ARP > + request is required for neighbor routers to learn the IP-MAC mapping > for > + this NAT address. This option only applies to NATs of type > + <code>dnat</code> and <code>dnat_and_snat</code>. For more information > + about what flows are added for IP routes, please see the > + <code>ovn-northd</code> manpage section on IP Routing. > + </column> > + > <group title="Common Columns"> > <column name="external_ids"> > See <em>External IDs</em> at the beginning of this document. > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 1058d418a..828777b82 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -465,6 +465,9 @@ AT_CHECK([ovn-nbctl lsp-add ls0 lp0]) > AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat_and_snat 30.0.0.2 192.168.1.2 lp0 > 00:00:00:01:02], [1], [], > [ovn-nbctl: invalid mac address 00:00:00:01:02. > ]) > +AT_CHECK([ovn-nbctl --add-route lr-nat-add lr0 snat 30.0.0.2 192.168.1.2], > [1], [], > +[ovn-nbctl: routes cannot be added for snat types. > +]) > > dnl Add snat and dnat > AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 30.0.0.1 192.168.1.0/24]) > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index feb38ea5e..0c8a09ced 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -3887,11 +3887,20 @@ check ovn-nbctl lr-nat-add ro2 dnat 20.0.0.100 > 192.168.2.100 > > check_lflows 0 > > -AS_BOX([Checking that NAT flows are installed for gateway routers]) > +AS_BOX([Checking that non-routable NAT flows are not installed for gateway > routers]) > > check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100 > check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100 > > +check_lflows 0 > + > +AS_BOX([Checking that routable NAT flows are installed when gateway chassis > exists]) > + > +check ovn-nbctl lr-nat-del ro1 > +check ovn-nbctl lr-nat-del ro2 > +check ovn-nbctl --add-route lr-nat-add ro1 dnat 10.0.0.100 192.168.1.100 > +check ovn-nbctl --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100 > + > check_lflows 1 > > AS_BOX([Checking that NAT flows are not installed for routers with gateway > chassis removed]) > @@ -3925,11 +3934,20 @@ check ovn-nbctl lr-nat-add ro2 dnat_and_snat > 20.0.0.100 192.168.2.2 vm2 00:00:00 > > check_lflows 0 > > -AS_BOX([Checking that Floating IP NAT flows are installed for gateway > routers]) > +AS_BOX([Checking that non-routable Floating IP NAT flows are not installed > for gateway routers]) > > check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100 > check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100 > > +check_lflows 0 > + > +AS_BOX([Checking that routable Floating IP NAT flows are installed for > gateway routers]) > +check ovn-nbctl lr-nat-del ro1 > +check ovn-nbctl lr-nat-del ro2 > + > +check ovn-nbctl --add-route lr-nat-add ro1 dnat_and_snat 10.0.0.100 > 192.168.1.2 vm1 00:00:00:00:00:01 > +check ovn-nbctl --add-route lr-nat-add ro2 dnat_and_snat 20.0.0.100 > 192.168.2.2 vm2 00:00:00:00:00:02 > + > check_lflows 1 > > AS_BOX([Checking that Floating IP NAT flows are not installed for routers > with gateway chassis removed]) > @@ -3965,15 +3983,29 @@ check ovn-nbctl lb-add lb1 10.0.0.100 192.168.1.2 > check ovn-nbctl lr-lb-add ro1 lb1 > > check ovn-nbctl lb-add lb2 20.0.0.100 192.168.2.2 > -check ovn-nbctl lr-lb-add ro2 lb2 > +check ovn-nbctl --wait=sb lr-lb-add ro2 lb2 > > check_lflows 0 > > -AS_BOX([Checking that Load Balancer VIP flows are installed for gateway > routers]) > +AS_BOX([Checking that non-routable Load Balancer VIP flows are not installed > for gateway routers]) > > check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100 > check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100 > > +check_lflows 0 > + > +AS_BOX([Checking that routable Load Balancer VIP flows are installed for > gateway routers]) > + > +check ovn-nbctl lr-lb-del ro1 lb1 > +check ovn-nbctl lr-lb-del ro2 lb2 > +check ovn-nbctl lb-del lb1 > +check ovn-nbctl lb-del lb2 > + > +check ovn-nbctl --add-route lb-add lb1 10.0.0.100 192.168.1.2 > +check ovn-nbctl --add-route lb-add lb2 20.0.0.100 192.168.2.2 > +check ovn-nbctl lr-lb-add ro1 lb1 > +check ovn-nbctl --wait=sb lr-lb-add ro2 lb2 > + > check_lflows 1 > > AS_BOX([Checking that Load Balancer VIP flows are not installed for routers > with gateway chassis removed]) > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index dc13fa9ca..f4a859495 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -367,6 +367,7 @@ Policy commands:\n\ > NAT commands:\n\ > [--stateless]\n\ > [--portrange]\n\ > + [--add-route]\n\ > lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT > EXTERNAL_MAC]\n\ > [EXTERNAL_PORT_RANGE]\n\ > add a NAT to ROUTER\n\ > @@ -2703,6 +2704,7 @@ nbctl_lb_add(struct ctl_context *ctx) > bool add_duplicate = shash_find(&ctx->options, "--add-duplicate") != > NULL; > bool empty_backend_rej = shash_find(&ctx->options, "--reject") != NULL; > bool empty_backend_event = shash_find(&ctx->options, "--event") != NULL; > + bool add_route = shash_find(&ctx->options, "--add-route") != NULL; > > if (empty_backend_event && empty_backend_rej) { > ctl_error(ctx, > @@ -2822,14 +2824,18 @@ nbctl_lb_add(struct ctl_context *ctx) > smap_add(CONST_CAST(struct smap *, &lb->vips), > lb_vip_normalized, ds_cstr(&lb_ips_new)); > nbrec_load_balancer_set_vips(lb, &lb->vips); > + struct smap options = SMAP_INITIALIZER(&options); > if (empty_backend_rej) { > - const struct smap options = SMAP_CONST1(&options, "reject", "true"); > - nbrec_load_balancer_set_options(lb, &options); > + smap_add(&options, "reject", "true"); > } > if (empty_backend_event) { > - const struct smap options = SMAP_CONST1(&options, "event", "true"); > - nbrec_load_balancer_set_options(lb, &options); > + smap_add(&options, "event", "true"); > + } > + if (add_route) { > + smap_add(&options, "add_route", "true"); > } > + nbrec_load_balancer_set_options(lb, &options); > + smap_destroy(&options); > out: > ds_destroy(&lb_ips_new); > > @@ -4400,6 +4406,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx) > char *new_logical_ip = NULL; > char *new_external_ip = NULL; > bool is_portrange = shash_find(&ctx->options, "--portrange") != NULL; > + bool add_route = shash_find(&ctx->options, "--add-route") != NULL; > > char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr); > if (error) { > @@ -4516,6 +4523,11 @@ nbctl_lr_nat_add(struct ctl_context *ctx) > } > > int is_snat = !strcmp("snat", nat_type); > + > + if (is_snat && add_route) { > + ctl_error(ctx, "routes cannot be added for snat types."); > + goto cleanup; > + } > for (size_t i = 0; i < lr->n_nat; i++) { > const struct nbrec_nat *nat = lr->nat[i]; > > @@ -4596,6 +4608,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx) > } > > smap_add(&nat_options, "stateless", stateless ? "true":"false"); > + smap_add(&nat_options, "add_route", add_route ? "true": "false"); We can add the option "add_route=true" only if "--add-route" flag is set. ovn-northd anyway defaults to false if not set. This would save some NB DB space. Thanks Numan > nbrec_nat_set_options(nat, &nat_options); > > smap_destroy(&nat_options); > @@ -6714,7 +6727,7 @@ static const struct ctl_command_syntax nbctl_commands[] > = { > "ROUTER TYPE EXTERNAL_IP LOGICAL_IP" > "[LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]", > nbctl_pre_lr_nat_add, nbctl_lr_nat_add, > - NULL, "--may-exist,--stateless,--portrange", RW }, > + NULL, "--may-exist,--stateless,--portrange,--add-route", RW }, > { "lr-nat-del", 1, 3, "ROUTER [TYPE [IP]]", > nbctl_pre_lr_nat_del, nbctl_lr_nat_del, NULL, "--if-exists", RW }, > { "lr-nat-list", 1, 1, "ROUTER", nbctl_pre_lr_nat_list, > @@ -6725,7 +6738,7 @@ static const struct ctl_command_syntax nbctl_commands[] > = { > /* load balancer commands. */ > { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]", > nbctl_pre_lb_add, nbctl_lb_add, NULL, > - "--may-exist,--add-duplicate,--reject,--event", RW }, > + "--may-exist,--add-duplicate,--reject,--event,--add-route", RW }, > { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del, NULL, > "--if-exists", RW }, > { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL, "", > RO }, > -- > 2.31.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev