[ovs-dev] [PATCH v4 ovn 2/2] ovn-northd: Refactor processing of SNAT IPs.
Numan Siddique
numans at ovn.org
Tue Sep 29 08:48:34 UTC 2020
On Mon, Sep 28, 2020 at 3:39 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Instead of building string sets every time we need to generate logical flows
> for unique SNAT IPs we now prebuild the set of unique SNAT IPs and store the
> list of NAT entries that refer it.
>
> Acked-by: Han Zhou <hzhou at ovn.org>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> northd/ovn-northd.c | 322 ++++++++++++++++++++++++++++-----------------------
> 1 file changed, 174 insertions(+), 148 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 5fddc5e..a39cb0e 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -623,8 +623,9 @@ struct ovn_datapath {
> /* NAT entries configured on the router. */
> struct ovn_nat *nat_entries;
>
> - /* SNAT IPs used by the router. */
> - struct sset snat_ips;
> + /* SNAT IPs owned by the router (shash of 'struct ovn_snat_ip'). */
> + struct shash snat_ips;
> +
> struct lport_addresses dnat_force_snat_addrs;
> struct lport_addresses lb_force_snat_addrs;
>
> @@ -644,6 +645,18 @@ struct ovn_datapath {
> struct ovn_nat {
> const struct nbrec_nat *nb;
> struct lport_addresses ext_addrs;
> + struct ovs_list ext_addr_list_node; /* Linkage in the per-external IP
> + * list of nat entries. Currently
> + * only used for SNAT.
> + */
> +};
> +
> +/* Stores the list of SNAT entries referencing a unique SNAT IP address.
> + * The 'snat_entries' list will be empty if the SNAT IP is used only for
> + * dnat_force_snat_ip or lb_force_snat_ip.
> + */
> +struct ovn_snat_ip {
> + struct ovs_list snat_entries;
> };
>
> static bool
> @@ -670,32 +683,49 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
> }
>
> static void
> +snat_ip_add(struct ovn_datapath *od, const char *ip, struct ovn_nat *nat_entry)
> +{
> + struct ovn_snat_ip *snat_ip = shash_find_data(&od->snat_ips, ip);
> +
> + if (!snat_ip) {
> + snat_ip = xzalloc(sizeof *snat_ip);
> + ovs_list_init(&snat_ip->snat_entries);
> + shash_add(&od->snat_ips, ip, snat_ip);
> + }
> +
> + if (nat_entry) {
> + ovs_list_push_back(&snat_ip->snat_entries,
> + &nat_entry->ext_addr_list_node);
> + }
> +}
> +
> +static void
> init_nat_entries(struct ovn_datapath *od)
> {
> if (!od->nbr) {
> return;
> }
>
> - sset_init(&od->snat_ips);
> + shash_init(&od->snat_ips);
> if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) {
> if (od->dnat_force_snat_addrs.n_ipv4_addrs) {
> - sset_add(&od->snat_ips,
> - od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s);
> + snat_ip_add(od, od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> + NULL);
> }
> if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
> - sset_add(&od->snat_ips,
> - od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
> + snat_ip_add(od, od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> + NULL);
> }
> }
>
> if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) {
> if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> - sset_add(&od->snat_ips,
> - od->lb_force_snat_addrs.ipv4_addrs[0].addr_s);
> + snat_ip_add(od, od->lb_force_snat_addrs.ipv4_addrs[0].addr_s,
> + NULL);
> }
> if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> - sset_add(&od->snat_ips,
> - od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
> + snat_ip_add(od, od->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
> + NULL);
> }
> }
>
> @@ -721,10 +751,15 @@ init_nat_entries(struct ovn_datapath *od)
> continue;
> }
>
> - if (!nat_entry_is_v6(nat_entry)) {
> - sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv4_addrs[0].addr_s);
> - } else {
> - sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv6_addrs[0].addr_s);
> + /* If this is a SNAT rule add the IP to the set of unique SNAT IPs. */
> + if (!strcmp(nat->type, "snat")) {
> + if (!nat_entry_is_v6(nat_entry)) {
> + snat_ip_add(od, nat_entry->ext_addrs.ipv4_addrs[0].addr_s,
> + nat_entry);
> + } else {
> + snat_ip_add(od, nat_entry->ext_addrs.ipv6_addrs[0].addr_s,
> + nat_entry);
> + }
> }
> }
> }
> @@ -736,7 +771,7 @@ destroy_nat_entries(struct ovn_datapath *od)
> return;
> }
>
> - sset_destroy(&od->snat_ips);
> + shash_destroy_free_data(&od->snat_ips);
> destroy_lport_addresses(&od->dnat_force_snat_addrs);
> destroy_lport_addresses(&od->lb_force_snat_addrs);
>
> @@ -8728,6 +8763,65 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,
> }
>
> static void
> +build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
> + uint16_t priority, bool drop_snat,
> + struct hmap *lflows)
> +{
> + struct ds match_ips = DS_EMPTY_INITIALIZER;
> +
> + if (op->lrp_networks.n_ipv4_addrs) {
> + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> + const char *ip = op->lrp_networks.ipv4_addrs[i].addr_s;
> +
> + if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
> + continue;
> + } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
> + continue;
> + }
> + ds_put_format(&match_ips, "%s, ", ip);
Hi Dumitru,
The patch LGTM with a few minor comments.
I find the above if/else if and the continue a bit confusing. We could
also avoid calling shash_find twice.
How about the below changes on top of your patch. Does that sound good ?
******************************************************
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index a39cb0ebe..91da31941 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -8764,7 +8764,7 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,
static void
build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
- uint16_t priority, bool drop_snat,
+ uint16_t priority, bool drop_snat_ip,
struct hmap *lflows)
{
struct ds match_ips = DS_EMPTY_INITIALIZER;
@@ -8773,12 +8773,12 @@ build_lrouter_drop_own_dest(struct ovn_port
*op, enum ovn_stage stage,
for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
const char *ip = op->lrp_networks.ipv4_addrs[i].addr_s;
- if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
- continue;
- } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
- continue;
+ bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
+ bool drop_router_ip = (drop_snat_ip == router_ip_in_snat_ips);
+
+ if (drop_router_ip) {
+ ds_put_format(&match_ips, "%s, ", ip);
}
- ds_put_format(&match_ips, "%s, ", ip);
}
if (ds_last(&match_ips) != EOF) {
@@ -8799,12 +8799,12 @@ build_lrouter_drop_own_dest(struct ovn_port
*op, enum ovn_stage stage,
for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
const char *ip = op->lrp_networks.ipv6_addrs[i].addr_s;
- if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
- continue;
- } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
- continue;
+ bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
+ bool drop_router_ip = (drop_snat_ip == router_ip_in_snat_ips);
+
+ if (drop_router_ip) {
+ ds_put_format(&match_ips, "%s, ", ip);
}
- ds_put_format(&match_ips, "%s, ", ip);
}
if (ds_last(&match_ips) != EOF) {
**********************************************************
Thanks
Numan
> + }
> +
> + if (ds_last(&match_ips) != EOF) {
> + ds_chomp(&match_ips, ' ');
> + ds_chomp(&match_ips, ',');
> +
> + char *match = xasprintf("ip4.dst == {%s}", ds_cstr(&match_ips));
> + ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
> + match, "drop;",
> + &op->nbrp->header_);
> + free(match);
> + }
> + }
> +
> + if (op->lrp_networks.n_ipv6_addrs) {
> + ds_clear(&match_ips);
> +
> + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> + const char *ip = op->lrp_networks.ipv6_addrs[i].addr_s;
> +
> + if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
> + continue;
> + } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
> + continue;
> + }
> + ds_put_format(&match_ips, "%s, ", ip);
> + }
> +
> + if (ds_last(&match_ips) != EOF) {
> + ds_chomp(&match_ips, ' ');
> + ds_chomp(&match_ips, ',');
> +
> + char *match = xasprintf("ip6.dst == {%s}", ds_cstr(&match_ips));
> + ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
> + match, "drop;",
> + &op->nbrp->header_);
> + free(match);
> + }
> + }
> + ds_destroy(&match_ips);
> +}
> +
> +static void
> build_lrouter_force_snat_flows(struct hmap *lflows, struct ovn_datapath *od,
> const char *ip_version, const char *ip_addr,
> const char *context)
> @@ -8897,68 +8991,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> op, lflows, &match, &actions);
> }
>
> - /* Drop IP traffic destined to router owned IPs. Part of it is dropped
> - * in stage "lr_in_ip_input" but traffic that could have been unSNATed
> - * but didn't match any existing session might still end up here.
> - */
> - HMAP_FOR_EACH (op, key_node, ports) {
> - if (!op->nbrp) {
> - continue;
> - }
> -
> - if (op->lrp_networks.n_ipv4_addrs) {
> - ds_clear(&match);
> - for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> - if (!sset_find(&op->od->snat_ips,
> - op->lrp_networks.ipv4_addrs[i].addr_s)) {
> - continue;
> - }
> - ds_put_format(&match, "%s, ",
> - op->lrp_networks.ipv4_addrs[i].addr_s);
> - }
> -
> - if (ds_last(&match) != EOF) {
> - ds_chomp(&match, ' ');
> - ds_chomp(&match, ',');
> -
> - char *drop_match = xasprintf("ip4.dst == {%s}",
> - ds_cstr(&match));
> - /* Drop traffic with IP.dest == router-ip. */
> - ovn_lflow_add_with_hint(lflows, op->od,
> - S_ROUTER_IN_ARP_RESOLVE, 1,
> - drop_match, "drop;",
> - &op->nbrp->header_);
> - free(drop_match);
> - }
> - }
> -
> - if (op->lrp_networks.n_ipv6_addrs) {
> - ds_clear(&match);
> - for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> - if (!sset_find(&op->od->snat_ips,
> - op->lrp_networks.ipv6_addrs[i].addr_s)) {
> - continue;
> - }
> - ds_put_format(&match, "%s, ",
> - op->lrp_networks.ipv6_addrs[i].addr_s);
> - }
> -
> - if (ds_last(&match) != EOF) {
> - ds_chomp(&match, ' ');
> - ds_chomp(&match, ',');
> -
> - char *drop_match = xasprintf("ip6.dst == {%s}",
> - ds_cstr(&match));
> - /* Drop traffic with IP.dest == router-ip. */
> - ovn_lflow_add_with_hint(lflows, op->od,
> - S_ROUTER_IN_ARP_RESOLVE, 1,
> - drop_match, "drop;",
> - &op->nbrp->header_);
> - free(drop_match);
> - }
> - }
> - }
> -
> HMAP_FOR_EACH (od, key_node, datapaths) {
> if (!od->nbr) {
> continue;
> @@ -8972,29 +9004,37 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> * port to handle the special cases. In case we get the packet
> * on a regular port, just reply with the port's ETH address.
> */
> - struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
> for (int i = 0; i < od->nbr->n_nat; i++) {
> struct ovn_nat *nat_entry = &od->nat_entries[i];
> - const struct nbrec_nat *nat = nat_entry->nb;
>
> /* Skip entries we failed to parse. */
> if (!nat_entry_is_valid(nat_entry)) {
> continue;
> }
>
> - struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> - char *ext_addr = nat_entry_is_v6(nat_entry) ?
> - ext_addrs->ipv6_addrs[0].addr_s :
> - ext_addrs->ipv4_addrs[0].addr_s;
> + /* Skip SNAT entries for now, we handle unique SNAT IPs separately
> + * below.
> + */
> + if (!strcmp(nat_entry->nb->type, "snat")) {
> + continue;
> + }
> + build_lrouter_nat_arp_nd_flow(od, nat_entry, lflows);
> + }
>
> - if (!strcmp(nat->type, "snat")) {
> - if (!sset_add(&snat_ips, ext_addr)) {
> - continue;
> - }
> + /* Now handle SNAT entries too, one per unique SNAT IP. */
> + struct shash_node *snat_snode;
> + SHASH_FOR_EACH (snat_snode, &od->snat_ips) {
> + struct ovn_snat_ip *snat_ip = snat_snode->data;
> +
> + if (ovs_list_is_empty(&snat_ip->snat_entries)) {
> + continue;
> }
> +
> + struct ovn_nat *nat_entry =
> + CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
> + struct ovn_nat, ext_addr_list_node);
> build_lrouter_nat_arp_nd_flow(od, nat_entry, lflows);
> }
> - sset_destroy(&snat_ips);
> }
>
> HMAP_FOR_EACH (od, key_node, datapaths) {
> @@ -9193,82 +9233,59 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> }
> }
>
> - /* A gateway router can have 4 SNAT IP addresses to force DNATed and
> - * LBed traffic respectively to be SNATed. In addition, there can be
> - * a number of SNAT rules in the NAT table.
> - * Skip all of them for drop flows. */
> - ds_clear(&match);
> - ds_put_cstr(&match, "ip4.dst == {");
> - bool has_drop_ips = false;
> - for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> - if (sset_find(&op->od->snat_ips,
> - op->lrp_networks.ipv4_addrs[i].addr_s)) {
> - continue;
> - }
> - ds_put_format(&match, "%s, ",
> - op->lrp_networks.ipv4_addrs[i].addr_s);
> - has_drop_ips = true;
> - }
> - if (has_drop_ips) {
> - ds_chomp(&match, ' ');
> - ds_chomp(&match, ',');
> - ds_put_cstr(&match, "} || ip6.dst == {");
> - } else {
> - ds_clear(&match);
> - ds_put_cstr(&match, "ip6.dst == {");
> - }
> -
> - for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> - if (sset_find(&op->od->snat_ips,
> - op->lrp_networks.ipv6_addrs[i].addr_s)) {
> - continue;
> - }
> - ds_put_format(&match, "%s, ",
> - op->lrp_networks.ipv6_addrs[i].addr_s);
> - has_drop_ips = true;
> - }
> -
> - ds_chomp(&match, ' ');
> - ds_chomp(&match, ',');
> - ds_put_cstr(&match, "}");
> -
> - if (has_drop_ips) {
> - /* Drop IP traffic to this router. */
> - ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_INPUT, 60,
> - ds_cstr(&match), "drop;",
> - &op->nbrp->header_);
> - }
> + /* Drop IP traffic destined to router owned IPs except if the IP is
> + * also a SNAT IP. Those are dropped later, in stage
> + * "lr_in_arp_resolve", if unSNAT was unsuccessful.
> + *
> + * Priority 60.
> + */
> + build_lrouter_drop_own_dest(op, S_ROUTER_IN_IP_INPUT, 60, false,
> + lflows);
>
> - /* ARP/NS packets are taken care of per router. The only exception
> - * is on the l3dgw_port where we might need to use a different
> - * ETH address.
> + /* ARP / ND handling for external IP addresses.
> + *
> + * DNAT and SNAT IP addresses are external IP addresses that need ARP
> + * handling.
> + *
> + * These are already taken care globally, per router. The only
> + * exception is on the l3dgw_port where we might need to use a
> + * different ETH address.
> */
> if (op != op->od->l3dgw_port) {
> continue;
> }
>
> - struct sset sset_snat_ips = SSET_INITIALIZER(&sset_snat_ips);
> for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> - const struct nbrec_nat *nat = nat_entry->nb;
>
> /* Skip entries we failed to parse. */
> if (!nat_entry_is_valid(nat_entry)) {
> continue;
> }
>
> - struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> - char *ext_addr = (nat_entry_is_v6(nat_entry)
> - ? ext_addrs->ipv6_addrs[0].addr_s
> - : ext_addrs->ipv4_addrs[0].addr_s);
> - if (!strcmp(nat->type, "snat")) {
> - if (!sset_add(&sset_snat_ips, ext_addr)) {
> - continue;
> - }
> + /* Skip SNAT entries for now, we handle unique SNAT IPs separately
> + * below.
> + */
> + if (!strcmp(nat_entry->nb->type, "snat")) {
> + continue;
> + }
> + build_lrouter_port_nat_arp_nd_flow(op, nat_entry, lflows);
> + }
> +
> + /* Now handle SNAT entries too, one per unique SNAT IP. */
> + struct shash_node *snat_snode;
> + SHASH_FOR_EACH (snat_snode, &op->od->snat_ips) {
> + struct ovn_snat_ip *snat_ip = snat_snode->data;
> +
> + if (ovs_list_is_empty(&snat_ip->snat_entries)) {
> + continue;
> }
> +
> + struct ovn_nat *nat_entry =
> + CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
> + struct ovn_nat, ext_addr_list_node);
> build_lrouter_port_nat_arp_nd_flow(op, nat_entry, lflows);
> }
> - sset_destroy(&sset_snat_ips);
> }
>
> HMAP_FOR_EACH (op, key_node, ports) {
> @@ -10642,6 +10659,15 @@ build_arp_resolve_flows_for_lrouter_port(
> &op->nbrp->header_);
> }
> }
> +
> + /* Drop IP traffic destined to router owned IPs. Part of it is dropped
> + * in stage "lr_in_ip_input" but traffic that could have been unSNATed
> + * but didn't match any existing session might still end up here.
> + *
> + * Priority 1.
> + */
> + build_lrouter_drop_own_dest(op, S_ROUTER_IN_ARP_RESOLVE, 1, true,
> + lflows);
> } else if (op->od->n_router_ports && strcmp(op->nbsp->type, "router")
> && strcmp(op->nbsp->type, "virtual")) {
> /* This is a logical switch port that backs a VM or a container.
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list