On 2/14/25 10:03 AM, Ales Musil wrote:
> On Fri, Feb 14, 2025 at 7:34 AM Martin Kalcok <[email protected]>
> wrote:
>
>> From: Dumitru Ceara <[email protected]>
>>
>> We can do this early, in the en_lr_nat node. That allows future commits
>> to reuse the parsed information later in the I-P run without having to,
>> reprocess NB contents.
>>
>> While at it clean up a bit the nat entry parsing and checking. We were
>> re-parsing external_ips, for example, twice even though we already had
>> the parsed IPs available in the internal record. Also the code now
>> tries to separate parsing and checking a bit as they were inter-twined
>> and very hard to understand.
>>
>> Signed-off-by: Dumitru Ceara <[email protected]>
>> ---
>>
>
> Hi Martin, Dumitru,
>
Hi Ales,
> we have talked about this offline but we should really remove invalid
> NAT entries, I'm fine with this approach but it would be a nice thing
> to do as a follow up.
>
Yes, I agree, but I didn't want to make even more "unrelated" changes so
close to branching for 25.03. I'll follow up after branching. This
code definitely needs more refactor than what I tried to do in this patch.
>
> northd/en-lr-nat.c | 234 +++++++++++++++++++++++++++++++++++----
>> northd/en-lr-nat.h | 24 ++--
>> northd/northd.c | 270 ++++++++++++---------------------------------
>> northd/northd.h | 3 +
>> 4 files changed, 292 insertions(+), 239 deletions(-)
>>
>> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
>> index bdbb2c860..44009ca6f 100644
>> --- a/northd/en-lr-nat.c
>> +++ b/northd/en-lr-nat.c
>> @@ -43,17 +43,21 @@ static void lr_nat_table_init(struct lr_nat_table *);
>> static void lr_nat_table_clear(struct lr_nat_table *);
>> static void lr_nat_table_destroy(struct lr_nat_table *);
>> static void lr_nat_table_build(struct lr_nat_table *,
>> - const struct ovn_datapaths *lr_datapaths);
>> + const struct ovn_datapaths *lr_datapaths,
>> + const struct hmap *lr_ports);
>> static struct lr_nat_record *lr_nat_table_find_by_index_(
>> const struct lr_nat_table *, size_t od_index);
>>
>> -static struct lr_nat_record *lr_nat_record_create(
>> - struct lr_nat_table *, const struct ovn_datapath *);
>> +static struct lr_nat_record *lr_nat_record_create(struct lr_nat_table *,
>> + const struct
>> ovn_datapath *,
>> + const struct hmap
>> *lr_ports);
>> static void lr_nat_record_init(struct lr_nat_record *,
>> - const struct ovn_datapath *);
>> + const struct ovn_datapath *,
>> + const struct hmap *lr_ports);
>> static void lr_nat_record_clear(struct lr_nat_record *);
>> static void lr_nat_record_reinit(struct lr_nat_record *,
>> - const struct ovn_datapath *);
>> + const struct ovn_datapath *,
>> + const struct hmap *lr_ports);
>> static void lr_nat_record_destroy(struct lr_nat_record *);
>>
>> static bool get_force_snat_ip(const struct ovn_datapath *,
>> @@ -105,7 +109,8 @@ en_lr_nat_run(struct engine_node *node, void *data_)
>>
>> stopwatch_start(LR_NAT_RUN_STOPWATCH_NAME, time_msec());
>> lr_nat_table_clear(&data->lr_nats);
>> - lr_nat_table_build(&data->lr_nats, &northd_data->lr_datapaths);
>> + lr_nat_table_build(&data->lr_nats, &northd_data->lr_datapaths,
>> + &northd_data->lr_ports);
>>
>> stopwatch_stop(LR_NAT_RUN_STOPWATCH_NAME, time_msec());
>> engine_set_node_state(node, EN_UPDATED);
>> @@ -133,7 +138,7 @@ lr_nat_northd_handler(struct engine_node *node, void
>> *data_)
>> od = hmapx_node->data;
>> lrnat_rec = lr_nat_table_find_by_index_(&data->lr_nats,
>> od->index);
>> ovs_assert(lrnat_rec);
>> - lr_nat_record_reinit(lrnat_rec, od);
>> + lr_nat_record_reinit(lrnat_rec, od, &northd_data->lr_ports);
>>
>> /* Add the lrnet rec to the tracking data. */
>> hmapx_add(&data->trk_data.crupdated, lrnat_rec);
>> @@ -169,14 +174,15 @@ lr_nat_table_clear(struct lr_nat_table *table)
>>
>> static void
>> lr_nat_table_build(struct lr_nat_table *table,
>> - const struct ovn_datapaths *lr_datapaths)
>> + const struct ovn_datapaths *lr_datapaths,
>> + const struct hmap *lr_ports)
>> {
>> table->array = xrealloc(table->array,
>> ods_size(lr_datapaths) * sizeof
>> *table->array);
>>
>> const struct ovn_datapath *od;
>> HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>> - lr_nat_record_create(table, od);
>> + lr_nat_record_create(table, od, lr_ports);
>> }
>> }
>>
>> @@ -198,12 +204,13 @@ lr_nat_table_find_by_index_(const struct
>> lr_nat_table *table,
>>
>> static struct lr_nat_record *
>> lr_nat_record_create(struct lr_nat_table *table,
>> - const struct ovn_datapath *od)
>> + const struct ovn_datapath *od,
>> + const struct hmap *lr_ports)
>> {
>> ovs_assert(od->nbr);
>>
>> struct lr_nat_record *lrnat_rec = xzalloc(sizeof *lrnat_rec);
>> - lr_nat_record_init(lrnat_rec, od);
>> + lr_nat_record_init(lrnat_rec, od, lr_ports);
>>
>> hmap_insert(&table->entries, &lrnat_rec->key_node,
>> uuid_hash(&od->nbr->header_.uuid));
>> @@ -211,9 +218,142 @@ lr_nat_record_create(struct lr_nat_table *table,
>> return lrnat_rec;
>> }
>>
>> +/* Returns true if a 'nat_entry' is valid, i.e.:
>> + * - parsing was successful.
>> + * - the string yielded exactly one IPv4 address or exactly one IPv6
>> address.
>> + */
>> +static bool
>> +lr_nat_entry_ext_addrs_valid(const struct ovn_nat *nat_entry)
>> +{
>> + const struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
>> +
>> + return (ext_addrs->n_ipv4_addrs == 1 && ext_addrs->n_ipv6_addrs == 0)
>> ||
>> + (ext_addrs->n_ipv4_addrs == 0 && ext_addrs->n_ipv6_addrs == 1);
>> +}
>> +
>> +/* Populates 'nat_entry->logical_ip_cidr_bits'. SNAT rules can have
>> + * subnets as logical IPs. Their prefix length is used to generate
>> + * NAT priority (LPM). */
>> +static bool
>> +lr_nat_entry_set_logical_ip_cidr_bits(const struct ovn_datapath *od,
>> + struct ovn_nat *nat_entry)
>> +{
>> + struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT;
>> + const struct nbrec_nat *nat = nat_entry->nb;
>> + bool is_v6 = nat_entry_is_v6(nat_entry);
>> + ovs_be32 ip, mask;
>> + char *error = NULL;
>> + if (is_v6) {
>> + error = ipv6_parse_masked(nat->logical_ip, &ipv6, &mask_v6);
>> + nat_entry->logical_ip_cidr_bits = ipv6_count_cidr_bits(&mask_v6);
>> + } else {
>> + error = ip_parse_masked(nat->logical_ip, &ip, &mask);
>> + nat_entry->logical_ip_cidr_bits = ip_count_cidr_bits(mask);
>> + }
>> + if (nat_entry->type == SNAT) {
>> + if (error) {
>> + /* Invalid for both IPv4 and IPv6 */
>> + static struct vlog_rate_limit rl =
>> + VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "bad ip network or ip %s for snat "
>> + "in router " UUID_FMT,
>> + nat->logical_ip, UUID_ARGS(&od->key));
>> + free(error);
>> + return false;
>> + }
>> + } else {
>> + if (error || (!is_v6 && mask != OVS_BE32_MAX)
>> + || (is_v6 && memcmp(&mask_v6, &v6_exact, sizeof mask_v6))) {
>> + /* Invalid for both IPv4 and IPv6 */
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "bad ip %s for dnat in router "
>> + UUID_FMT,
>> + nat->logical_ip, UUID_ARGS(&od->key));
>> + free(error);
>> + return false;
>> + }
>> + }
>> + return true;
>> +}
>> +
>> +/* Populates 'nat_entry->l3dgw_port' with the corresponding DGW port
>> + * (distributed gateway port) for the NAT entry, if any. Sets
>> + * 'nat_entry->is_distributed' to true if the NAT entry is fully
>> + * distributed, that is, dnat_and_snat with valid DGW port and mac set.
>> + * It sets 'nat_entry->is_distributed' to false otherwise.
>> + *
>> + * Returns false on failure to find an adequate DGW port.
>> + * Returns true otherwise.
>> + * */
>> +static bool
>> +lr_nat_entry_set_dgw_port(const struct ovn_datapath *od,
>> + struct ovn_nat *nat_entry,
>> + const struct hmap *lr_ports)
>> +{
>> + const struct nbrec_nat *nat = nat_entry->nb;
>> +
>> + /* Validate gateway_port of NAT rule. */
>> + nat_entry->l3dgw_port = NULL;
>> + if (nat->gateway_port == NULL) {
>> + if (od->n_l3dgw_ports == 1) {
>> + nat_entry->l3dgw_port = od->l3dgw_ports[0];
>> + } else if (od->n_l3dgw_ports > 1) {
>> + /* Find the DGP reachable for the NAT external IP. */
>> + for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
>> + if (lrp_find_member_ip(od->l3dgw_ports[i],
>> nat->external_ip)) {
>> + nat_entry->l3dgw_port = od->l3dgw_ports[i];
>> + break;
>> + }
>> + }
>> + if (nat_entry->l3dgw_port == NULL) {
>> + static struct vlog_rate_limit rl =
>> VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "Unable to determine gateway_port for
>> NAT "
>> + "with external_ip: %s configured on logical "
>> + "router: %s with multiple distributed
>> gateway "
>> + "ports", nat->external_ip, od->nbr->name);
>> + return false;
>> + }
>> + }
>> + } else {
>> + nat_entry->l3dgw_port =
>> + ovn_port_find(lr_ports, nat->gateway_port->name);
>> +
>> + if (!nat_entry->l3dgw_port || nat_entry->l3dgw_port->od != od ||
>> + !lrp_is_l3dgw(nat_entry->l3dgw_port)) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "gateway_port: %s of NAT configured on "
>> + "logical router: %s is not a valid distributed "
>> + "gateway port on that router",
>> + nat->gateway_port->name, od->nbr->name);
>> + return false;
>> + }
>> + }
>> +
>> + /* For distributed router NAT, determine whether this NAT rule
>> + * satisfies the conditions for distributed NAT processing. */
>> + nat_entry->is_distributed = false;
>> +
>> + /* NAT cannnot be distributed if the DGP's peer
>> + * has a chassisredirect port (as the routing is centralized
>> + * on the gateway chassis for the DGP's networks/subnets.)
>> + */
>> + struct ovn_port *l3dgw_port = nat_entry->l3dgw_port;
>> + if (l3dgw_port && l3dgw_port->peer && l3dgw_port->peer->cr_port) {
>> + return true;
>> + }
>> +
>> + if (od->n_l3dgw_ports && nat_entry->type == DNAT_AND_SNAT &&
>> + nat->logical_port && nat->external_mac) {
>> + nat_entry->is_distributed = true;
>> + }
>> +
>> + return true;
>> +}
>> +
>> static void
>> lr_nat_record_init(struct lr_nat_record *lrnat_rec,
>> - const struct ovn_datapath *od)
>> + const struct ovn_datapath *od,
>> + const struct hmap *lr_ports)
>> {
>> lrnat_rec->lr_index = od->index;
>> lrnat_rec->nbr_uuid = od->nbr->header_.uuid;
>> @@ -285,16 +425,64 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec,
>>
>> nat_entry->nb = nat;
>> nat_entry->is_router_ip = false;
>> + nat_entry->is_valid = true;
>> +
>> + if (!strcmp(nat->type, "snat")) {
>> + nat_entry->type = SNAT;
>> + } else if (!strcmp(nat->type, "dnat_and_snat")) {
>> + nat_entry->type = DNAT_AND_SNAT;
>> + } else {
>> + nat_entry->type = DNAT;
>> + }
>> +
>> + if (!extract_ip_addresses(nat->external_ip,
>> &nat_entry->ext_addrs)) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +
>> + VLOG_WARN_RL(&rl, "Failed to extract ip address %s "
>> + "in nat configuration for router %s",
>> + nat->external_ip, od->nbr->name);
>> + nat_entry->is_valid = false;
>> + continue;
>> + }
>> +
>> + if (nat->external_mac && !eth_addr_from_string(nat->external_mac,
>> + &nat_entry->mac)) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "bad mac %s for dnat in router "
>> + ""UUID_FMT"", nat->external_mac,
>> UUID_ARGS(&od->key));
>> + nat_entry->is_valid = false;
>> + continue;
>> + }
>>
>> - if (!extract_ip_addresses(nat->external_ip,
>> - &nat_entry->ext_addrs) ||
>> - !nat_entry_is_valid(nat_entry)) {
>> + if (!lr_nat_entry_ext_addrs_valid(nat_entry)) {
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>>
>> - VLOG_WARN_RL(&rl,
>> - "Bad ip address %s in nat configuration "
>> - "for router %s", nat->external_ip,
>> - od->nbr->name);
>> + VLOG_WARN_RL(&rl, "Invalid ip address %s in nat configuration
>> "
>> + "for router %s",
>> + nat->external_ip, od->nbr->name);
>> + nat_entry->is_valid = false;
>> + continue;
>> + }
>> +
>> + if (nat->allowed_ext_ips && nat->exempted_ext_ips) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> + VLOG_WARN_RL(&rl, "NAT rule: " UUID_FMT " not applied, since "
>> + "both allowed and exempt external ips set",
>> + UUID_ARGS(&(nat->header_.uuid)));
>> + nat_entry->is_valid = false;
>> + continue;
>> + }
>> +
>> + /* Set nat_entry->logical_ip_cidr_bits (SNAT logical_ip can be
>> + * a subnet). */
>> + if (!lr_nat_entry_set_logical_ip_cidr_bits(od, nat_entry)) {
>> + nat_entry->is_valid = false;
>> + continue;
>> + }
>> +
>> + /* Set nat_entry->l3dgw_port and nat_entry->is_distributed. */
>> + if (!lr_nat_entry_set_dgw_port(od, nat_entry, lr_ports)) {
>> + nat_entry->is_valid = false;
>> continue;
>> }
>>
>> @@ -313,11 +501,8 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec,
>> nat_entry->ext_addrs.ipv6_addrs[0].addr_s,
>> nat_entry);
>> }
>> - nat_entry->type = SNAT;
>> } else {
>> - nat_entry->type = DNAT;
>> if (!strcmp(nat->type, "dnat_and_snat")) {
>> - nat_entry->type = DNAT_AND_SNAT;
>> if (nat->logical_port && nat->external_mac) {
>> lrnat_rec->has_distributed_nat = true;
>> }
>> @@ -348,10 +533,11 @@ lr_nat_record_clear(struct lr_nat_record *lrnat_rec)
>>
>> static void
>> lr_nat_record_reinit(struct lr_nat_record *lrnat_rec,
>> - const struct ovn_datapath *od)
>> + const struct ovn_datapath *od,
>> + const struct hmap *lr_ports)
>> {
>> lr_nat_record_clear(lrnat_rec);
>> - lr_nat_record_init(lrnat_rec, od);
>> + lr_nat_record_init(lrnat_rec, od, lr_ports);
>> }
>>
>> static void
>> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h
>> index 120ceeca9..a73933390 100644
>> --- a/northd/en-lr-nat.h
>> +++ b/northd/en-lr-nat.h
>> @@ -38,13 +38,22 @@ enum ovn_nat_type {
>> /* Contains a NAT entry with the external addresses pre-parsed. */
>> struct ovn_nat {
>> const struct nbrec_nat *nb;
>> - struct lport_addresses ext_addrs;
>> + struct lport_addresses ext_addrs; /* Parsed NB.NAT.external_ip. */
>> + struct eth_addr mac; /* Parsed NB.NAT.external_mac. */
>> + int logical_ip_cidr_bits; /* Parsed NB.NATlogical_ip prefix
>> len. */
>> struct ovs_list ext_addr_list_node; /* Linkage in the per-external IP
>> * list of nat entries. Currently
>> * only used for SNAT.
>> */
>> bool is_router_ip; /* Indicates if the NAT external_ip is also one of
>> * router's lrp ip. Can be 'true' only for SNAT.
>> */
>> +
>> + struct ovn_port *l3dgw_port; /* If non-NULL, the distributed gateway
>> port
>> + * this NAT will use. NULL for gateway
>> + * routers. */
>> + bool is_distributed; /* True if this NAT record is fully
>> + * distributed. */
>> + bool is_valid; /* True if the configuration of this entry is valid. */
>> enum ovn_nat_type type;
>> };
>>
>> @@ -114,19 +123,6 @@ void en_lr_nat_run(struct engine_node *, void *data);
>> bool lr_nat_logical_router_handler(struct engine_node *, void *data);
>> bool lr_nat_northd_handler(struct engine_node *, void *data);
>>
>> -/* Returns true if a 'nat_entry' is valid, i.e.:
>> - * - parsing was successful.
>> - * - the string yielded exactly one IPv4 address or exactly one IPv6
>> address.
>> - */
>> -static inline bool
>> -nat_entry_is_valid(const struct ovn_nat *nat_entry)
>> -{
>> - const struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
>> -
>> - return (ext_addrs->n_ipv4_addrs == 1 && ext_addrs->n_ipv6_addrs == 0)
>> ||
>> - (ext_addrs->n_ipv4_addrs == 0 && ext_addrs->n_ipv6_addrs == 1);
>> -}
>> -
>> static inline bool
>> nat_entry_is_v6(const struct ovn_nat *nat_entry)
>> {
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 11f4131aa..1d9f0675e 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -1553,9 +1553,6 @@ ipam_add_port_addresses(struct ovn_datapath *od,
>> struct ovn_port *op)
>> }
>> }
>>
>> -static const char *find_lrp_member_ip(const struct ovn_port *op,
>> - const char *ip_s);
>> -
>> /* Returns true if the given router port 'op' (assumed to be a distributed
>> * gateway port) is the relevant DGP where the NAT rule of the router
>> needs to
>> * be applied. */
>> @@ -1563,7 +1560,7 @@ static bool
>> is_nat_gateway_port(const struct nbrec_nat *nat, const struct ovn_port
>> *op)
>> {
>> if (op->od->n_l3dgw_ports > 1
>> - && ((!nat->gateway_port && !find_lrp_member_ip(op,
>> nat->external_ip))
>> + && ((!nat->gateway_port && !lrp_find_member_ip(op,
>> nat->external_ip))
>> || (nat->gateway_port && nat->gateway_port != op->nbrp))) {
>> return false;
>> }
>> @@ -8842,7 +8839,7 @@ build_lswitch_rport_arp_req_flows_for_lbnats(
>> &lr_stateful_rec->lrnat_rec->nat_entries[i];
>> const struct nbrec_nat *nat = nat_entry->nb;
>>
>> - if (!nat_entry_is_valid(nat_entry)) {
>> + if (!nat_entry->is_valid) {
>> continue;
>> }
>>
>> @@ -10569,8 +10566,8 @@ build_bfd_map(const struct nbrec_bfd_table
>> *nbrec_bfd_table,
>> * overlaps with 'ip_s". If one is not found, returns NULL.
>> *
>> * The caller must not free the returned string. */
>> -static const char *
>> -find_lrp_member_ip(const struct ovn_port *op, const char *ip_s)
>> +const char *
>> +lrp_find_member_ip(const struct ovn_port *op, const char *ip_s)
>> {
>> return find_lport_address(&op->lrp_networks, ip_s);
>> }
>> @@ -10589,7 +10586,7 @@ get_outport_for_routing_policy_nexthop(struct
>> ovn_datapath *od,
>> struct nbrec_logical_router_port *lrp = od->nbr->ports[i];
>>
>> struct ovn_port *out_port = ovn_port_find(lr_ports, lrp->name);
>> - if (out_port && find_lrp_member_ip(out_port, nexthop)) {
>> + if (out_port && lrp_find_member_ip(out_port, nexthop)) {
>> return out_port;
>> }
>> }
>> @@ -10679,7 +10676,7 @@ build_routing_policy_flow(struct lflow_table
>> *lflows, struct ovn_datapath *od,
>> return;
>> }
>>
>> - const char *lrp_addr_s = find_lrp_member_ip(out_port, nexthop);
>> + const char *lrp_addr_s = lrp_find_member_ip(out_port, nexthop);
>> if (!lrp_addr_s) {
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
>> @@ -10780,7 +10777,7 @@ build_ecmp_routing_policy_flows(struct lflow_table
>> *lflows,
>> }
>>
>> const char *lrp_addr_s =
>> - find_lrp_member_ip(out_port, rp->valid_nexthops[i]);
>> + lrp_find_member_ip(out_port, rp->valid_nexthops[i]);
>> if (!lrp_addr_s) {
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
>> @@ -11525,7 +11522,7 @@ find_route_outport(const struct hmap *lr_ports,
>> const char *output_port,
>> return false;
>> }
>> if (nexthop[0]) {
>> - *lrp_addr_s = find_lrp_member_ip(*out_port, nexthop);
>> + *lrp_addr_s = lrp_find_member_ip(*out_port, nexthop);
>> }
>> if (!*lrp_addr_s) {
>> if (!force_out_port) {
>> @@ -11570,7 +11567,7 @@ find_static_route_outport(const struct
>> ovn_datapath *od,
>> * router port matching the next hop. */
>> HMAP_FOR_EACH (out_port, dp_node, &od->ports) {
>> if (route->nexthop[0]) {
>> - lrp_addr_s = find_lrp_member_ip(out_port, route->nexthop);
>> + lrp_addr_s = lrp_find_member_ip(out_port, route->nexthop);
>> }
>> if (lrp_addr_s) {
>> break;
>> @@ -14514,7 +14511,7 @@ build_arp_resolve_flows_for_lsp(
>> continue;
>> }
>>
>> - if (!find_lrp_member_ip(peer, ip_s)) {
>> + if (!lrp_find_member_ip(peer, ip_s)) {
>> continue;
>> }
>>
>> @@ -14546,7 +14543,7 @@ build_arp_resolve_flows_for_lsp(
>> continue;
>> }
>>
>> - if (!find_lrp_member_ip(peer, ip_s)) {
>> + if (!lrp_find_member_ip(peer, ip_s)) {
>> continue;
>> }
>>
>> @@ -15538,7 +15535,7 @@ build_lrouter_arp_nd_for_datapath(const struct
>> ovn_datapath *od,
>> struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i];
>>
>> /* Skip entries we failed to parse. */
>> - if (!nat_entry_is_valid(nat_entry)) {
>> + if (!nat_entry->is_valid) {
>> continue;
>> }
>>
>> @@ -15845,7 +15842,7 @@ build_lrouter_ipv4_ip_input_for_lbnats(
>> &lr_stateful_rec->lrnat_rec->nat_entries[i];
>>
>> /* Skip entries we failed to parse. */
>> - if (!nat_entry_is_valid(nat_entry)) {
>> + if (!nat_entry->is_valid) {
>> continue;
>> }
>>
>> @@ -16487,145 +16484,6 @@ build_lrouter_ingress_flow(struct lflow_table
>> *lflows,
>> }
>> }
>>
>> -static int
>> -lrouter_check_nat_entry(const struct ovn_datapath *od,
>> - const struct ovn_nat *nat_entry,
>> - const struct hmap *lr_ports, ovs_be32 *mask,
>> - bool *is_v6, int *cidr_bits, struct eth_addr *mac,
>> - bool *distributed, struct ovn_port
>> **nat_l3dgw_port)
>> -{
>> - const struct nbrec_nat *nat = nat_entry->nb;
>> - struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT;
>> - ovs_be32 ip;
>> -
>> - if (nat->allowed_ext_ips && nat->exempted_ext_ips) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> - VLOG_WARN_RL(&rl, "NAT rule: "UUID_FMT" not applied, since "
>> - "both allowed and exempt external ips set",
>> - UUID_ARGS(&(nat->header_.uuid)));
>> - return -EINVAL;
>> - }
>> -
>> - char *error = ip_parse_masked(nat->external_ip, &ip, mask);
>> - *is_v6 = false;
>> -
>> - if (error || *mask != OVS_BE32_MAX) {
>> - free(error);
>> - error = ipv6_parse_masked(nat->external_ip, &ipv6, &mask_v6);
>> - if (error || memcmp(&mask_v6, &v6_exact, sizeof(mask_v6))) {
>> - /* Invalid for both IPv4 and IPv6 */
>> - static struct vlog_rate_limit rl =
>> - VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad external ip %s for nat",
>> - nat->external_ip);
>> - free(error);
>> - return -EINVAL;
>> - }
>> - /* It was an invalid IPv4 address, but valid IPv6.
>> - * Treat the rest of the handling of this NAT rule
>> - * as IPv6. */
>> - *is_v6 = true;
>> - }
>> -
>> - /* Validate gateway_port of NAT rule. */
>> - *nat_l3dgw_port = NULL;
>> - if (nat->gateway_port == NULL) {
>> - if (od->n_l3dgw_ports == 1) {
>> - *nat_l3dgw_port = od->l3dgw_ports[0];
>> - } else if (od->n_l3dgw_ports > 1) {
>> - /* Find the DGP reachable for the NAT external IP. */
>> - for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
>> - if (find_lrp_member_ip(od->l3dgw_ports[i],
>> nat->external_ip)) {
>> - *nat_l3dgw_port = od->l3dgw_ports[i];
>> - break;
>> - }
>> - }
>> - if (*nat_l3dgw_port == NULL) {
>> - static struct vlog_rate_limit rl =
>> VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "Unable to determine gateway_port for
>> NAT "
>> - "with external_ip: %s configured on logical "
>> - "router: %s with multiple distributed
>> gateway "
>> - "ports", nat->external_ip, od->nbr->name);
>> - return -EINVAL;
>> - }
>> - }
>> - } else {
>> - *nat_l3dgw_port = ovn_port_find(lr_ports,
>> nat->gateway_port->name);
>> -
>> - if (!(*nat_l3dgw_port) || (*nat_l3dgw_port)->od != od ||
>> - !lrp_is_l3dgw(*nat_l3dgw_port)) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "gateway_port: %s of NAT configured on "
>> - "logical router: %s is not a valid distributed "
>> - "gateway port on that router",
>> - nat->gateway_port->name, od->nbr->name);
>> - return -EINVAL;
>> - }
>> - }
>> -
>> - /* Check the validity of nat->logical_ip. 'logical_ip' can
>> - * be a subnet when the type is "snat". */
>> - if (*is_v6) {
>> - error = ipv6_parse_masked(nat->logical_ip, &ipv6, &mask_v6);
>> - *cidr_bits = ipv6_count_cidr_bits(&mask_v6);
>> - } else {
>> - error = ip_parse_masked(nat->logical_ip, &ip, mask);
>> - *cidr_bits = ip_count_cidr_bits(*mask);
>> - }
>> - if (nat_entry->type == SNAT) {
>> - if (error) {
>> - /* Invalid for both IPv4 and IPv6 */
>> - static struct vlog_rate_limit rl =
>> - VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad ip network or ip %s for snat "
>> - "in router "UUID_FMT"",
>> - nat->logical_ip, UUID_ARGS(&od->key));
>> - free(error);
>> - return -EINVAL;
>> - }
>> - } else {
>> - if (error || (*is_v6 == false && *mask != OVS_BE32_MAX)
>> - || (*is_v6 && memcmp(&mask_v6, &v6_exact,
>> - sizeof mask_v6))) {
>> - /* Invalid for both IPv4 and IPv6 */
>> - static struct vlog_rate_limit rl =
>> - VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad ip %s for dnat in router "
>> - ""UUID_FMT"", nat->logical_ip, UUID_ARGS(&od->key));
>> - free(error);
>> - return -EINVAL;
>> - }
>> - }
>> -
>> - /* For distributed router NAT, determine whether this NAT rule
>> - * satisfies the conditions for distributed NAT processing. */
>> - *distributed = false;
>> -
>> - /* NAT cannnot be distributed if the DGP's peer
>> - * has a chassisredirect port (as the routing is centralized
>> - * on the gateway chassis for the DGP's networks/subnets.)
>> - */
>> - struct ovn_port *l3dgw_port = *nat_l3dgw_port;
>> - if (l3dgw_port && l3dgw_port->peer && l3dgw_port->peer->cr_port) {
>> - return 0;
>> - }
>> -
>> - if (od->n_l3dgw_ports && nat_entry->type == DNAT_AND_SNAT &&
>> - nat->logical_port && nat->external_mac) {
>> - if (eth_addr_from_string(nat->external_mac, mac)) {
>> - *distributed = true;
>> - } else {
>> - static struct vlog_rate_limit rl =
>> - VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad mac %s for dnat in router "
>> - ""UUID_FMT"", nat->external_mac, UUID_ARGS(&od->key));
>> - return -EINVAL;
>> - }
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /* NAT, Defrag and load balancing. */
>> static void build_lr_nat_defrag_and_lb_default_flows(
>> struct ovn_datapath *od, struct lflow_table *lflows,
>> @@ -16668,7 +16526,7 @@ static void
>> build_lrouter_nat_defrag_and_lb(
>> const struct lr_stateful_record *lr_stateful_rec,
>> const struct ovn_datapath *od, struct lflow_table *lflows,
>> - const struct hmap *ls_ports, const struct hmap *lr_ports,
>> + const struct hmap *ls_ports,
>> struct ds *match, struct ds *actions,
>> const struct shash *meter_groups,
>> const struct chassis_features *features,
>> @@ -16772,20 +16630,16 @@ build_lrouter_nat_defrag_and_lb(
>> for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) {
>> struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i];
>> const struct nbrec_nat *nat = nat_entry->nb;
>> - struct eth_addr mac = eth_addr_broadcast;
>> - bool is_v6, distributed_nat;
>> - ovs_be32 mask;
>> - int cidr_bits;
>> - struct ovn_port *l3dgw_port;
>>
>> - bool stateless = lrouter_dnat_and_snat_is_stateless(nat_entry);
>> -
>> - if (lrouter_check_nat_entry(od, nat_entry, lr_ports, &mask,
>> &is_v6,
>> - &cidr_bits,
>> - &mac, &distributed_nat, &l3dgw_port)
>> < 0) {
>> + /* Skip invalid entries. */
>> + if (!nat_entry->is_valid) {
>> continue;
>> }
>>
>> + bool stateless = lrouter_dnat_and_snat_is_stateless(nat_entry);
>> + bool is_v6 = nat_entry_is_v6(nat_entry);
>> + unsigned int cidr_bits = nat_entry->logical_ip_cidr_bits;
>> +
>> /* S_ROUTER_IN_UNSNAT
>> * Ingress UNSNAT table: It is for already established
>> connections'
>> * reverse traffic. i.e., SNAT has already been done in egress
>> @@ -16798,22 +16652,27 @@ build_lrouter_nat_defrag_and_lb(
>> * egress pipeline. */
>> if (stateless) {
>> build_lrouter_in_unsnat_stateless_flow(lflows, od, nat_entry,
>> - match, distributed_nat,
>> - is_v6, l3dgw_port,
>> + match,
>> +
>> nat_entry->is_distributed,
>> + is_v6,
>> + nat_entry->l3dgw_port,
>> lflow_ref);
>> } else if (lrouter_use_common_zone(od)) {
>> build_lrouter_in_unsnat_in_czone_flow(lflows, od, nat_entry,
>> match,
>> - distributed_nat, is_v6,
>> - l3dgw_port, lflow_ref);
>> +
>> nat_entry->is_distributed,
>> + is_v6,
>> nat_entry->l3dgw_port,
>> + lflow_ref);
>> } else {
>> build_lrouter_in_unsnat_flow(lflows, od, nat_entry, match,
>> - distributed_nat, is_v6,
>> l3dgw_port,
>> + nat_entry->is_distributed,
>> + is_v6, nat_entry->l3dgw_port,
>> lflow_ref);
>> }
>> /* S_ROUTER_IN_DNAT */
>> build_lrouter_in_dnat_flow(lflows, od, lrnat_rec, nat_entry,
>> match,
>> - actions, distributed_nat, cidr_bits,
>> is_v6,
>> - l3dgw_port, stateless, lflow_ref);
>> + actions, nat_entry->is_distributed,
>> + cidr_bits, is_v6,
>> nat_entry->l3dgw_port,
>> + stateless, lflow_ref);
>>
>> /* ARP resolve for NAT IPs. */
>> if (!od->is_gw_router) {
>> @@ -16825,7 +16684,8 @@ build_lrouter_nat_defrag_and_lb(
>> ds_clear(match);
>> ds_put_format(
>> match, "inport == %s && outport == %s && ip%s.dst ==
>> %s",
>> - l3dgw_port->json_key, l3dgw_port->json_key,
>> + nat_entry->l3dgw_port->json_key,
>> + nat_entry->l3dgw_port->json_key,
>> is_v6 ? "6" : "4", nat->external_ip);
>> ovn_lflow_add_with_hint(lflows, od,
>> S_ROUTER_IN_ARP_RESOLVE,
>> @@ -16839,21 +16699,21 @@ build_lrouter_nat_defrag_and_lb(
>> ds_clear(match);
>> ds_put_format(
>> match, "outport == %s && %s == %s",
>> - l3dgw_port->json_key,
>> + nat_entry->l3dgw_port->json_key,
>> is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4,
>> nat->external_ip);
>> ds_clear(actions);
>> ds_put_format(
>> actions, "eth.dst = %s; next;",
>> - distributed_nat ? nat->external_mac :
>> - l3dgw_port->lrp_networks.ea_s);
>> + nat_entry->is_distributed ? nat->external_mac :
>> + nat_entry->l3dgw_port->lrp_networks.ea_s);
>> ovn_lflow_add_with_hint(lflows, od,
>> S_ROUTER_IN_ARP_RESOLVE,
>> 100, ds_cstr(match),
>> ds_cstr(actions),
>> &nat->header_,
>> lflow_ref);
>> - if (od->redirect_bridged && distributed_nat) {
>> + if (od->redirect_bridged && nat_entry->is_distributed) {
>> ds_clear(match);
>> ds_put_format(
>> match,
>> @@ -16883,13 +16743,16 @@ build_lrouter_nat_defrag_and_lb(
>> if (use_common_zone) {
>> /* S_ROUTER_OUT_DNAT_LOCAL */
>> build_lrouter_out_is_dnat_local(lflows, od, nat, match,
>> actions,
>> - distributed_nat, is_v6,
>> - l3dgw_port, lflow_ref);
>> + nat_entry->is_distributed,
>> + is_v6, nat_entry->l3dgw_port,
>> + lflow_ref);
>> }
>>
>> /* S_ROUTER_OUT_UNDNAT */
>> build_lrouter_out_undnat_flow(lflows, od, nat_entry, match,
>> actions,
>> - distributed_nat, mac, is_v6,
>> l3dgw_port,
>> + nat_entry->is_distributed,
>> + nat_entry->mac, is_v6,
>> + nat_entry->l3dgw_port,
>> stateless, lflow_ref);
>> /* S_ROUTER_OUT_SNAT
>> * Egress SNAT table: Packets enter the egress pipeline with
>> @@ -16897,23 +16760,30 @@ build_lrouter_nat_defrag_and_lb(
>> * address. */
>> if (stateless) {
>> build_lrouter_out_snat_stateless_flow(lflows, od, nat_entry,
>> match,
>> - actions,
>> distributed_nat,
>> - mac, cidr_bits, is_v6,
>> - l3dgw_port, lflow_ref);
>> + actions,
>> +
>> nat_entry->is_distributed,
>> + nat_entry->mac,
>> cidr_bits,
>> + is_v6,
>> nat_entry->l3dgw_port,
>> + lflow_ref);
>> } else if (lrouter_use_common_zone(od)) {
>> build_lrouter_out_snat_in_czone_flow(lflows, od, nat_entry,
>> match,
>> - actions,
>> distributed_nat, mac,
>> - cidr_bits, is_v6,
>> l3dgw_port,
>> + actions,
>> +
>> nat_entry->is_distributed,
>> + nat_entry->mac,
>> cidr_bits,
>> + is_v6,
>> nat_entry->l3dgw_port,
>> lflow_ref);
>> } else {
>> build_lrouter_out_snat_flow(lflows, od, nat_entry, match,
>> actions,
>> - distributed_nat, mac, cidr_bits,
>> is_v6,
>> - l3dgw_port, lflow_ref, features);
>> + nat_entry->is_distributed,
>> + nat_entry->mac, cidr_bits,
>> + is_v6, nat_entry->l3dgw_port,
>> + lflow_ref, features);
>> }
>>
>> /* S_ROUTER_IN_ADMISSION - S_ROUTER_IN_IP_INPUT */
>> - build_lrouter_ingress_flow(lflows, od, nat_entry, match, actions,
>> mac,
>> - distributed_nat, is_v6, l3dgw_port,
>> + build_lrouter_ingress_flow(lflows, od, nat_entry, match, actions,
>> + nat_entry->mac,
>> nat_entry->is_distributed,
>> + is_v6, nat_entry->l3dgw_port,
>> meter_groups, lflow_ref);
>>
>> /* Ingress Gateway Redirect Table: For NAT on a distributed
>> @@ -16925,13 +16795,13 @@ build_lrouter_nat_defrag_and_lb(
>> * generated in the following stage is sent out with proper IP/MAC
>> * src addresses.
>> */
>> - if (distributed_nat) {
>> + if (nat_entry->is_distributed) {
>> ds_clear(match);
>> ds_clear(actions);
>> ds_put_format(match,
>> "ip%s.src == %s && outport == %s",
>> is_v6 ? "6" : "4", nat->logical_ip,
>> - l3dgw_port->json_key);
>> + nat_entry->l3dgw_port->json_key);
>> /* Add a rule to drop traffic from a distributed NAT if
>> * the virtual port has not claimed yet becaused otherwise
>> * the traffic will be centralized misconfiguring the TOR
>> switch.
>> @@ -16967,10 +16837,10 @@ build_lrouter_nat_defrag_and_lb(
>> ds_put_format(match, "ip%s.dst == %s && outport == %s",
>> is_v6 ? "6" : "4",
>> nat->external_ip,
>> - l3dgw_port->json_key);
>> - if (!distributed_nat) {
>> + nat_entry->l3dgw_port->json_key);
>> + if (!nat_entry->is_distributed) {
>> ds_put_format(match, " && is_chassis_resident(%s)",
>> - l3dgw_port->cr_port->json_key);
>> + nat_entry->l3dgw_port->cr_port->json_key);
>> } else {
>> ds_put_format(match, " && is_chassis_resident(\"%s\")",
>> nat->logical_port);
>> @@ -17268,7 +17138,6 @@ build_lr_stateful_flows(const struct
>> lr_stateful_record *lr_stateful_rec,
>> const struct ovn_datapaths *lr_datapaths,
>> struct lflow_table *lflows,
>> const struct hmap *ls_ports,
>> - const struct hmap *lr_ports,
>> struct ds *match,
>> struct ds *actions,
>> const struct shash *meter_groups,
>> @@ -17280,7 +17149,7 @@ build_lr_stateful_flows(const struct
>> lr_stateful_record *lr_stateful_rec,
>> ovs_assert(uuid_equals(&od->nbr->header_.uuid,
>> &lr_stateful_rec->nbr_uuid));
>> build_lrouter_nat_defrag_and_lb(lr_stateful_rec, od, lflows, ls_ports,
>> - lr_ports, match, actions,
>> meter_groups,
>> + match, actions, meter_groups,
>> features, lr_stateful_rec->lflow_ref);
>> build_lr_gateway_redirect_flows_for_nats(od,
>> lr_stateful_rec->lrnat_rec,
>> lflows, match, actions,
>> @@ -17608,8 +17477,7 @@ build_lflows_thread(void *arg)
>> }
>> build_lr_stateful_flows(lr_stateful_rec,
>> lsi->lr_datapaths,
>> lsi->lflows, lsi->ls_ports,
>> - lsi->lr_ports, &lsi->match,
>> - &lsi->actions,
>> + &lsi->match, &lsi->actions,
>> lsi->meter_groups,
>> lsi->features);
>> }
>> @@ -17835,7 +17703,7 @@ build_lswitch_and_lrouter_flows(
>> stopwatch_start(LFLOWS_LR_STATEFUL_STOPWATCH_NAME, time_msec());
>> LR_STATEFUL_TABLE_FOR_EACH (lr_stateful_rec, lr_stateful_table) {
>> build_lr_stateful_flows(lr_stateful_rec, lsi.lr_datapaths,
>> - lsi.lflows, lsi.ls_ports,
>> lsi.lr_ports,
>> + lsi.lflows, lsi.ls_ports,
>> &lsi.match, &lsi.actions,
>> lsi.meter_groups, lsi.features);
>> }
>> @@ -18238,7 +18106,7 @@ lflow_handle_lr_stateful_changes(struct
>> ovsdb_idl_txn *ovnsb_txn,
>> /* Generate new lflows. */
>> build_lr_stateful_flows(lr_stateful_rec,
>> lflow_input->lr_datapaths,
>> lflows, lflow_input->ls_ports,
>> - lflow_input->lr_ports, &match, &actions,
>> + &match, &actions,
>> lflow_input->meter_groups,
>> lflow_input->features);
>>
>> diff --git a/northd/northd.h b/northd/northd.h
>> index eaf3d9455..b984e124d 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -967,6 +967,8 @@ lsp_is_router(const struct nbrec_logical_switch_port
>> *nbsp)
>> return !strcmp(nbsp->type, "router");
>> }
>>
>> +const char *lrp_find_member_ip(const struct ovn_port *op, const char
>> *ip_s);
>> +
>> /* This function returns true if 'op' is a gateway router port.
>> * False otherwise.
>> * For 'op' to be a gateway router port.
>> @@ -985,6 +987,7 @@ lrp_is_l3dgw(const struct ovn_port *op)
>> }
>>
>> struct ovn_port *ovn_port_find(const struct hmap *ports, const char
>> *name);
>> +
>> void build_igmp_lflows(struct hmap *igmp_groups,
>> const struct hmap *ls_datapaths,
>> struct lflow_table *lflows,
>> --
>> 2.43.0
>>
>>
> Otherwise it looks good to me, thanks.
>
> Acked-by: Ales Musil <[email protected]>
>
Thanks all! I also added Felix's ack from v6 (because this patch hasn't
changed since v6) and then pushed this patch to main.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
