On Wed, Jun 30, 2021 at 7:57 PM Mark Michelson wrote:
>
> Dealing with NAT and load balancer IPs has been a bit of a pain point.
> It requires creating static routes if east-west traffic to those
> addresses is desired. Further, it requires ARPs to be sent between the
> logical routers in order to create MAC Bindings.
>
> This commit seeks to make things easier. NAT and load balancer addresess
> automatically have IP routing logical flows and ARP resolution logical
> flows created for reachable routers. This eliminates the need to create
> static routes, and it also eliminates the need for ARPs to be sent
> between logical routers.
>
> In this commit, the behavior is not optional. The next commit will
> introduce configuration to make the behavior optional.
>
> Signed-off-by: Mark Michelson
Hi Mark,
There is one small problem. Please see below. With that addressed:
Acked-by: Numan Siddique
Numan
> ---
> northd/ovn-northd.c | 129 +-
> northd/ovn_northd.dl | 57
> tests/ovn-northd.at | 214 +++
> 3 files changed, 395 insertions(+), 5 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 694c3b2c4..58132bc5c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -1378,6 +1378,21 @@ build_datapaths(struct northd_context *ctx, struct
> hmap *datapaths,
> }
> }
>
> +/* Structure representing logical router port
> + * routable addresses. This includes DNAT and Load Balancer
> + * addresses. This structure will only be filled in if the
> + * router port is a gateway router port. Otherwise, all pointers
> + * will be NULL and n_addrs will be 0.
> + */
> +struct ovn_port_routable_addresses {
> +/* Array of address strings suitable for writing to a database table */
> +char **addresses;
> +/* The addresses field parsed into component parts */
> +struct lport_addresses *laddrs;
> +/* Number of items in each of the above arrays */
> +size_t n_addrs;
> +};
> +
> /* A logical switch port or logical router port.
> *
> * In steady state, an ovn_port points to a northbound Logical_Switch_Port
> @@ -1421,6 +1436,8 @@ struct ovn_port {
>
> struct lport_addresses lrp_networks;
>
> +struct ovn_port_routable_addresses routables;
> +
> /* Logical port multicast data. */
> struct mcast_port_info mcast_info;
>
> @@ -1447,6 +1464,44 @@ struct ovn_port {
> struct ovs_list list; /* In list of similar records. */
> };
>
> +static void
> +destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
> +{
> +for (size_t i = 0; i < ra->n_addrs; i++) {
> +free(ra->addresses[i]);
> +destroy_lport_addresses(&ra->laddrs[i]);
> +}
> +free(ra->addresses);
> +free(ra->laddrs);
> +}
> +
> +static char **get_nat_addresses(const struct ovn_port *op, size_t *n);
> +
> +static void
> +assign_routable_addresses(struct ovn_port *op)
> +{
> +size_t n;
> +char **nats = get_nat_addresses(op, &n);
> +
> +if (!nats) {
> +return;
> +}
> +
> +struct lport_addresses *laddrs = xcalloc(n, sizeof(*laddrs));
> +for (size_t i = 0; i < n; i++) {
> +int ofs;
> +if (!extract_addresses(nats[i], &laddrs[i], &ofs)){
> +continue;
> +}
> +}
> +
> +/* Everything seems to have worked out */
> +op->routables.addresses = nats;
> +op->routables.laddrs = laddrs;
> +op->routables.n_addrs = n;
The n_addrs would have the wrong value if 'extract_addresses()' fails.
You probably want to maintain another variable for n_addrs.
Thanks
Numan
> +}
> +
> +
> static void
> ovn_port_set_nb(struct ovn_port *op,
> const struct nbrec_logical_switch_port *nbsp,
> @@ -1496,6 +1551,8 @@ ovn_port_destroy(struct hmap *ports, struct ovn_port
> *port)
> }
> free(port->ps_addrs);
>
> +destroy_routable_addresses(&port->routables);
> +
> destroy_lport_addresses(&port->lrp_networks);
> free(port->json_key);
> free(port->key);
> @@ -2403,6 +2460,8 @@ join_logical_ports(struct northd_context *ctx,
> * use during flow creation. */
> od->l3dgw_port = op;
> od->l3redirect_port = crp;
> +
> +assign_routable_addresses(op);
> }
> }
> }
> @@ -2486,7 +2545,7 @@ get_nat_addresses(const struct ovn_port *op, size_t *n)
> {
> size_t n_nats = 0;
> struct eth_addr mac;
> -if (!op->nbrp || !op->od || !op->od->nbr
> +if (!op || !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)) {
> *n = n_nats;
> @@ -3067,7 +3126,6 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> } else {
> sbrec_port_binding_set_options(op->sb, NULL);
> }
>
