On 12/6/23 04:03, Numan Siddique wrote:
> On Thu, Nov 23, 2023 at 9:04 AM Dumitru Ceara <[email protected]> wrote:
>>
>> On 11/17/23 23:05, Numan Siddique wrote:
>>> On Wed, Nov 15, 2023 at 1:27 AM Han Zhou <[email protected]> wrote:
>>>>
>>>> On Thu, Oct 26, 2023 at 11:15 AM <[email protected]> wrote:
>>>>>
>>>>> From: Numan Siddique <[email protected]>
>>>>>
>>>>> This new engine now maintains the NAT related data for each
>>>>> logical router which was earlier maintained by the northd
>>>>> engine node in the 'struct ovn_datapath'. Main inputs to
>>>>> this engine node are:
>>>>> - northd
>>>>> - NB logical router
>>>>>
>>>>> A record for each logical router is maintained in the 'lr_nats'
>>>>> hmap table and this record
>>>>> - stores the ovn_nat's
>>>>
>>>> It seems the sentence is incomplete.
>>>>
>>>>>
>>>>> Handlers are also added to handle the changes to both these
>>>>> inputs. This engine node becomes an input to 'lflow' node.
>>>>> This essentially decouples the lr NAT data from the northd
>>>>> engine node.
>>>>>
>>>>> Signed-off-by: Numan Siddique <[email protected]>
>>>>> ---
>>>>> lib/ovn-util.c | 6 +-
>>>>> lib/ovn-util.h | 2 +-
>>>>> lib/stopwatch-names.h | 1 +
>>>>> northd/automake.mk | 2 +
>>>>> northd/en-lflow.c | 5 +
>>>>> northd/en-lr-nat.c | 498 +++++++++++++++++++++++++++++++++++++
>>>>> northd/en-lr-nat.h | 134 ++++++++++
>>>>> northd/en-sync-sb.c | 11 +-
>>>>> northd/inc-proc-northd.c | 9 +
>>>>> northd/northd.c | 514 ++++++++++++++-------------------------
>>>>> northd/northd.h | 32 ++-
>>>>> tests/ovn-northd.at | 18 ++
>>>>> 12 files changed, 877 insertions(+), 355 deletions(-)
>>>>> create mode 100644 northd/en-lr-nat.c
>>>>> create mode 100644 northd/en-lr-nat.h
>>
>> I would call these en-nat.{hc}, I think. There's no NAT for switches.
>
>
> In v3 I haven't renamed it. I can rename it in the next version once
> most of the review comments
> are addressed (it's a bit of a pain to rename and fix all the compiler
> errors).
>
> But in this case I still prefer lr-nat (and the structures -
> lr_nat_record, ... ) because,
> - This engine node maintains the data for the NATs associated
> with a logical router. A router
> can have 0 or more NATs
> - We have a NB NAT table which is for each NAT entry, whereas
> this engine node is for NATs associated with a router
> and it could confuse and not convey the relationship between
> router and NATs.
> - This patch creates a 'lr_nat_record' entry for every logical
> router even if it doesn't have any NATs.
>
>
OK, this seems reasonable.
>>
>>>>>
>>>>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>>>>> index 33105202f2..05e635a6b4 100644
>>>>> --- a/lib/ovn-util.c
>>>>> +++ b/lib/ovn-util.c
>>>>> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
>>>> sbrec_port_binding *binding,
>>>>> }
>>>>>
>>>>> bool
>>>>> -lport_addresses_is_empty(struct lport_addresses *laddrs)
>>>>> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
>>>>> {
>>>>> return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
>>>>> }
>>>>> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
>>>> *laddrs)
>>>>> {
>>>>> free(laddrs->ipv4_addrs);
>>>>> free(laddrs->ipv6_addrs);
>>>>> + laddrs->ipv4_addrs = NULL;
>>>>> + laddrs->ipv6_addrs = NULL;
>>>>> + laddrs->n_ipv4_addrs = 0;
>>>>> + laddrs->n_ipv6_addrs = 0;
>>>>> }
>>>>>
>>>>> /* Returns a string of the IP address of 'laddrs' that overlaps with
>>>> 'ip_s'.
>>>>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>>>>> index bff50dbde9..5805415885 100644
>>>>> --- a/lib/ovn-util.h
>>>>> +++ b/lib/ovn-util.h
>>>>> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
>>>> sbrec_port_binding *binding,
>>>>> bool extract_lrp_networks__(char *mac, char **networks, size_t
>>>> n_networks,
>>>>> struct lport_addresses *laddrs);
>>>>>
>>>>> -bool lport_addresses_is_empty(struct lport_addresses *);
>>>>> +bool lport_addresses_is_empty(const struct lport_addresses *);
>>>>> void destroy_lport_addresses(struct lport_addresses *);
>>>>> const char *find_lport_address(const struct lport_addresses *laddrs,
>>>>> const char *ip_s);
>>>>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
>>>>> index 3452cc71cf..0a16da211e 100644
>>>>> --- a/lib/stopwatch-names.h
>>>>> +++ b/lib/stopwatch-names.h
>>>>> @@ -32,5 +32,6 @@
>>>>> #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
>>>>> #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>>>>> #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
>>>>> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>>>>>
>>>>> #endif
>>>>> diff --git a/northd/automake.mk b/northd/automake.mk
>>>>> index cf622fc3c9..ae367a2a8b 100644
>>>>> --- a/northd/automake.mk
>>>>> +++ b/northd/automake.mk
>>>>> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
>>>>> northd/en-sync-from-sb.h \
>>>>> northd/en-lb-data.c \
>>>>> northd/en-lb-data.h \
>>>>> + northd/en-lr-nat.c \
>>>>> + northd/en-lr-nat.h \
>>>>> northd/inc-proc-northd.c \
>>>>> northd/inc-proc-northd.h \
>>>>> northd/ipam.c \
>>>>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
>>>>> index 96d03b7ada..22f398d419 100644
>>>>> --- a/northd/en-lflow.c
>>>>> +++ b/northd/en-lflow.c
>>>>> @@ -19,6 +19,7 @@
>>>>> #include <stdio.h>
>>>>>
>>>>> #include "en-lflow.h"
>>>>> +#include "en-lr-nat.h"
>>>>> #include "en-northd.h"
>>>>> #include "en-meters.h"
>>>>>
>>>>> @@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
>>>>> engine_get_input_data("port_group", node);
>>>>> struct sync_meters_data *sync_meters_data =
>>>>> engine_get_input_data("sync_meters", node);
>>>>> + struct ed_type_lr_nat_data *lr_nat_data =
>>>>> + engine_get_input_data("lr_nat", node);
>>>>> +
>>>>> lflow_input->nbrec_bfd_table =
>>>>> EN_OVSDB_GET(engine_get_input("NB_bfd", node));
>>>>> lflow_input->sbrec_bfd_table =
>>>>> @@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node *node,
>>>>> lflow_input->ls_ports = &northd_data->ls_ports;
>>>>> lflow_input->lr_ports = &northd_data->lr_ports;
>>>>> lflow_input->ls_port_groups = &pg_data->ls_port_groups;
>>>>> + lflow_input->lr_nats = &lr_nat_data->lr_nats;
>>>>> lflow_input->meter_groups = &sync_meters_data->meter_groups;
>>>>> lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map;
>>>>> lflow_input->svc_monitor_map = &northd_data->svc_monitor_map;
>>>>> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
>>>>> new file mode 100644
>>>>> index 0000000000..0d332d300b
>>>>> --- /dev/null
>>>>> +++ b/northd/en-lr-nat.c
>>>>> @@ -0,0 +1,498 @@
>>>>> +/*
>>>>> + * Copyright (c) 2023, Red Hat, Inc.
>>>>> + *
>>>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>>>> + * you may not use this file except in compliance with the License.
>>>>> + * You may obtain a copy of the License at:
>>>>> + *
>>>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>>>> + *
>>>>> + * Unless required by applicable law or agreed to in writing, software
>>>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>>>> implied.
>>>>> + * See the License for the specific language governing permissions and
>>>>> + * limitations under the License.
>>>>> + */
>>>>> +
>>>>> +#include <config.h>
>>>>> +
>>>>> +#include <getopt.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <stdio.h>
>>>>> +
>>>>> +/* OVS includes */
>>>>> +#include "include/openvswitch/hmap.h"
>>>>> +#include "openvswitch/util.h"
>>>>> +#include "openvswitch/vlog.h"
>>>>> +#include "stopwatch.h"
>>>>> +
>>>>> +/* OVN includes */
>>>>> +#include "en-lr-nat.h"
>>>>> +#include "lib/inc-proc-eng.h"
>>>>> +#include "lib/lb.h"
>>>>> +#include "lib/ovn-nb-idl.h"
>>>>> +#include "lib/ovn-sb-idl.h"
>>>>> +#include "lib/ovn-util.h"
>>>>> +#include "lib/stopwatch-names.h"
>>>>> +#include "northd.h"
>>>>> +
>>>>> +VLOG_DEFINE_THIS_MODULE(en_lr_nat);
>>
>> Globally, throughout the whole patch, I think we should replace
>> "lr_nat_*" with "nat_*". There's no NAT on switches.
>>
>>>>> +
>>>>> +/* Static function declarations. */
>>>>> +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);
>>>>> +static struct lr_nat_record *lr_nat_table_find_(const struct
>>>> lr_nat_table *,
>>>>> + const struct
>>>> nbrec_logical_router *);
>>>>> +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 void lr_nat_record_init(struct lr_nat_record *);
>>>>> +static void lr_nat_record_reinit(struct lr_nat_record *);
>>>>> +static void lr_nat_record_destroy(struct lr_nat_record *);
>>>>> +
>>>>> +static void lr_nat_entries_init(struct lr_nat_record *);
>>>>> +static void lr_nat_entries_destroy(struct lr_nat_record *);
>>>>> +static void lr_nat_external_ips_init(struct lr_nat_record *);
>>>>> +static void lr_nat_external_ips_destroy(struct lr_nat_record *);
>>>>> +static bool get_force_snat_ip(struct lr_nat_record *, const char
>>>> *key_type,
>>>>> + struct lport_addresses *);
>>>>> +static struct lr_nat_input lr_nat_get_input_data(struct engine_node *);
>>>>> +static bool is_lr_nats_changed(const struct nbrec_logical_router *);
>>>>> +static bool is_lr_nats_seqno_changed(const struct nbrec_logical_router
>>>> *nbr);
>>>>> +
>>>>> +
>>>>> +const struct lr_nat_record *
>>>>> +lr_nat_table_find_by_index(const struct lr_nat_table *table,
>>>>> + size_t od_index)
>>>>> +{
>>>>> + return lr_nat_table_find_by_index_(table, od_index);
>>>>> +}
>>>>> +
>>>>> +/* 'lr_nat' engine node manages the NB logical router NAT data.
>>>>> + */
>>>>> +void *
>>>>> +en_lr_nat_init(struct engine_node *node OVS_UNUSED,
>>>>> + struct engine_arg *arg OVS_UNUSED)
>>>>> +{
>>>>> + struct ed_type_lr_nat_data *data = xzalloc(sizeof *data);
>>>>> + lr_nat_table_init(&data->lr_nats);
>>>>> + hmapx_init(&data->tracked_data.crupdated);
>>>>> + hmapx_init(&data->tracked_data.deleted);
>>>>> + return data;
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +en_lr_nat_cleanup(void *data_)
>>>>> +{
>>>>> + struct ed_type_lr_nat_data *data = (struct ed_type_lr_nat_data *)
>>>> data_;
>>>>> + lr_nat_table_destroy(&data->lr_nats);
>>>>> + hmapx_destroy(&data->tracked_data.crupdated);
>>>>> + hmapx_destroy(&data->tracked_data.deleted);
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +en_lr_nat_clear_tracked_data(void *data_)
>>>>> +{
>>>>> + struct ed_type_lr_nat_data *data = (struct ed_type_lr_nat_data *)
>>>> data_;
>>>>> +
>>>>> + struct hmapx_node *hmapx_node;
>>>>> + HMAPX_FOR_EACH_SAFE (hmapx_node, &data->tracked_data.deleted) {
>>>>> + lr_nat_record_destroy(hmapx_node->data);
>>>>> + hmapx_delete(&data->tracked_data.deleted, hmapx_node);
>>>>> + }
>>>>> +
>>>>> + hmapx_clear(&data->tracked_data.crupdated);
>>>>> + data->tracked = false;
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +en_lr_nat_run(struct engine_node *node, void *data_)
>>>>> +{
>>>>> + struct lr_nat_input input_data = lr_nat_get_input_data(node);
>>>>> + struct ed_type_lr_nat_data *data = data_;
>>>>> +
>>>>> + stopwatch_start(LR_NAT_RUN_STOPWATCH_NAME, time_msec());
>>>>> + data->tracked = false;
>>>>> + lr_nat_table_clear(&data->lr_nats);
>>>>> + lr_nat_table_build(&data->lr_nats, input_data.lr_datapaths);
>>>>> +
>>>>> + stopwatch_stop(LR_NAT_RUN_STOPWATCH_NAME, time_msec());
>>>>> + engine_set_node_state(node, EN_UPDATED);
>>>>> +}
>>>>> +
>>>>> +/* Handler functions. */
>>>>> +bool
>>>>> +lr_nat_northd_handler(struct engine_node *node, void *data OVS_UNUSED)
>>>>> +{
>>>>> + struct northd_data *northd_data = engine_get_input_data("northd",
>>>> node);
>>>>> + if (!northd_data->change_tracked) {
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>
>>>> I wonder how can we return true here without checking the actual changes in
>>>> north? We should check if there is any change in northd that would affect
>>>> the data of this node, right?
>>>>
>>>
>>> This (lr_nat) node is interested only in the logical router changes
>>> related to NAT. And any changes related to NAT are handled below
>>> in the lr_nat_logical_router_handler() function. So we don't have to
>>> do anything for northd node changes. Actually we don't need to have
>>> northd node as input. I only added it so that lr_nat_record can store
>>> 'struct ovn_datapath *'.
>>
>> We're making quite an assumption here, we're essentially saying: "if the
>> northd_data was 'change tracked' then all 'ovn_datapath *' we stored in
>> lr_nat_records are still valid". That seems very fragile to me. What
>> happens when in the future router deletion is handled incrementally in
>> northd?
>
>
> This is very similar to having 'struct nbrec_logical_switch' or
> 'struct nbrec_logical_router'
> referenced in the 'struct ovn_datapath'. In the future if we add I-P
> for logical switch/router
> deletion we will include that in the tracking data and the 'en_lr_nat'
> engine node will
> delete the entry in its lr_nat table.
>
> I don't think its fragile to me.
>
> However, in v3 I changed the inputs to lr_nat engine node. It's only
> input is now 'northd' engine node
> and the handler 'lr_nat_northd_handler' handles the changes from the
> northd_tracked_data ->lr_with_changed_nats.
>
> Request to take a look at v3.
>
>
OK, will do.
>
>>
>>> This becomes easier for lflow engine node to easily access
>>> lr_nat_record from lr_nats array using
>>> the od->index.
>>> If we don't want this, then we need to do a lookup for the lr_nat
>>> record in the lr_nats hmap.
>>> I wanted to avoid this lookup.
>>>
>>
>> Do we know if this lookup is expensive on large scale realistic topologies?
>
> I haven't measured it, but it can be expensive if there are many hash
> collisions in the hmap.
>
>
I agree.
>>
>>> Thanks
>>> Numan
>>>
>>>> Thanks,
>>>> Han
>>>>
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> +bool
>>>>> +lr_nat_logical_router_handler(struct engine_node *node, void *data_)
>>>>> +{
>>>>> + struct lr_nat_input input_data = lr_nat_get_input_data(node);
>>>>> + struct ed_type_lr_nat_data *data = data_;
>>>>> + const struct nbrec_logical_router *nbr;
>>>>> +
>>>>> + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (
>>>>> + nbr, input_data.nbrec_logical_router_table) {
>>>>> + if (!is_lr_nats_changed(nbr)) {
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + struct lr_nat_record *lrnat_rec =
>>>> lr_nat_table_find_(&data->lr_nats,
>>>>> + nbr);
>>>>> +
>>>>> + if (nbrec_logical_router_is_deleted(nbr)) {
>>>>> + if (lrnat_rec) {
>>>>> + /* Remove the record from the entries. */
>>>>> + hmap_remove(&data->lr_nats.entries,
>>>> &lrnat_rec->key_node);
>>>>> +
>>>>> + /* Add the lrnet rec to the tracking data. */
>>>>> + hmapx_add(&data->tracked_data.deleted, lrnat_rec);
>>>>> + }
>>>>> + } else {
>>>>> + if (!lrnat_rec) {
>>>>> + const struct ovn_datapath *od;
>>>>> + od =
>>>> ovn_datapath_find(&input_data.lr_datapaths->datapaths,
>>>>> + &nbr->header_.uuid);
>>>>> + ovs_assert(od);
>>>>> + lrnat_rec = lr_nat_record_create(&data->lr_nats, od);
>>>>> + } else {
>>>>> + lr_nat_record_reinit(lrnat_rec);
>>>>> + }
>>>>> +
>>>>> + /* Add the lrnet rec to the tracking data. */
>>>>> + hmapx_add(&data->tracked_data.crupdated, lrnat_rec);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (!hmapx_is_empty(&data->tracked_data.deleted)
>>>>> + || !hmapx_is_empty(&data->tracked_data.crupdated)) {
>>>>> + data->tracked = true;
>>>>> + engine_set_node_state(node, EN_UPDATED);
>>>>> + }
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> +/* static functions. */
>>>>> +static void
>>>>> +lr_nat_table_init(struct lr_nat_table *table)
>>>>> +{
>>>>> + *table = (struct lr_nat_table) {
>>>>> + .entries = HMAP_INITIALIZER(&table->entries),
>>>>> + };
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_table_clear(struct lr_nat_table *table)
>>>>> +{
>>>>> + struct lr_nat_record *lrnat_rec;
>>>>> + HMAP_FOR_EACH_POP (lrnat_rec, key_node, &table->entries) {
>>>>> + lr_nat_record_destroy(lrnat_rec);
>>>>> + }
>>>>> +
>>>>> + free(table->array);
>>>>> + table->array = NULL;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_table_build(struct lr_nat_table *table,
>>>>> + const struct ovn_datapaths *lr_datapaths)
>>>>> +{
>>>>> + 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);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_table_destroy(struct lr_nat_table *table)
>>>>> +{
>>>>> + lr_nat_table_clear(table);
>>>>> + hmap_destroy(&table->entries);
>>>>> +}
>>>>> +
>>>>> +struct lr_nat_record *
>>>>> +lr_nat_table_find_(const struct lr_nat_table *table,
>>>>> + const struct nbrec_logical_router *nbr)
>>>>> +{
>>>>> + struct lr_nat_record *lrnat_rec;
>>>>> +
>>>>> + HMAP_FOR_EACH_WITH_HASH (lrnat_rec, key_node,
>>>>> + uuid_hash(&nbr->header_.uuid),
>>>> &table->entries) {
>>>>> + if (nbr == lrnat_rec->od->nbr) {
>>>>> + return lrnat_rec;
>>>>> + }
>>>>> + }
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +
>>>>> +struct lr_nat_record *
>>>>> +lr_nat_table_find_by_index_(const struct lr_nat_table *table,
>>>>> + size_t od_index)
>>>>> +{
>>>>> + ovs_assert(od_index <= hmap_count(&table->entries));
>>>>> +
>>>>> + return table->array[od_index];
>>>>> +}
>>>>> +
>>>>> +static struct lr_nat_record *
>>>>> +lr_nat_record_create(struct lr_nat_table *table,
>>>>> + const struct ovn_datapath *od)
>>>>> +{
>>>>> + ovs_assert(od->nbr);
>>>>> +
>>>>> + struct lr_nat_record *lrnat_rec = xzalloc(sizeof *lrnat_rec);
>>>>> + lrnat_rec->od = od;
>>>>> + lr_nat_record_init(lrnat_rec);
>>>>> +
>>>>> + hmap_insert(&table->entries, &lrnat_rec->key_node,
>>>>> + uuid_hash(&od->nbr->header_.uuid));
>>>>> + table->array[od->index] = lrnat_rec;
>>>>> + return lrnat_rec;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_record_init(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + lr_nat_entries_init(lrnat_rec);
>>>>> + lr_nat_external_ips_init(lrnat_rec);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_record_reinit(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + lr_nat_entries_destroy(lrnat_rec);
>>>>> + lr_nat_external_ips_destroy(lrnat_rec);
>>>>> + lr_nat_record_init(lrnat_rec);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_record_destroy(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + lr_nat_entries_destroy(lrnat_rec);
>>>>> + lr_nat_external_ips_destroy(lrnat_rec);
>>>>> + free(lrnat_rec);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_external_ips_init(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + sset_init(&lrnat_rec->external_ips);
>>>>> + for (size_t i = 0; i < lrnat_rec->od->nbr->n_nat; i++) {
>>>>> + sset_add(&lrnat_rec->external_ips,
>>>>> + lrnat_rec->od->nbr->nat[i]->external_ip);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_external_ips_destroy(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + sset_destroy(&lrnat_rec->external_ips);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +snat_ip_add(struct lr_nat_record *lrnat_rec, const char *ip,
>>>>> + struct ovn_nat *nat_entry)
>>>>> +{
>>>>> + struct ovn_snat_ip *snat_ip = shash_find_data(&lrnat_rec->snat_ips,
>>>> ip);
>>>>> +
>>>>> + if (!snat_ip) {
>>>>> + snat_ip = xzalloc(sizeof *snat_ip);
>>>>> + ovs_list_init(&snat_ip->snat_entries);
>>>>> + shash_add(&lrnat_rec->snat_ips, ip, snat_ip);
>>>>> + }
>>>>> +
>>>>> + if (nat_entry) {
>>>>> + ovs_list_push_back(&snat_ip->snat_entries,
>>>>> + &nat_entry->ext_addr_list_node);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_entries_init(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + shash_init(&lrnat_rec->snat_ips);
>>>>> + sset_init(&lrnat_rec->external_macs);
>>>>> + lrnat_rec->has_distributed_nat = false;
>>>>> +
>>>>> + if (get_force_snat_ip(lrnat_rec, "dnat",
>>>>> + &lrnat_rec->dnat_force_snat_addrs)) {
>>>>> + if (lrnat_rec->dnat_force_snat_addrs.n_ipv4_addrs) {
>>>>> + snat_ip_add(lrnat_rec,
>>>>> +
>>>> lrnat_rec->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
>>>>> + NULL);
>>>>> + }
>>>>> + if (lrnat_rec->dnat_force_snat_addrs.n_ipv6_addrs) {
>>>>> + snat_ip_add(lrnat_rec,
>>>>> +
>>>> lrnat_rec->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
>>>>> + NULL);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /* Check if 'lb_force_snat_ip' is configured with 'router_ip'. */
>>>>> + const char *lb_force_snat =
>>>>> + smap_get(&lrnat_rec->od->nbr->options, "lb_force_snat_ip");
>>>>> + if (lb_force_snat && !strcmp(lb_force_snat, "router_ip")
>>>>> + && smap_get(&lrnat_rec->od->nbr->options, "chassis")) {
>>>>> +
>>>>> + /* Set it to true only if its gateway router and
>>>>> + * options:lb_force_snat_ip=router_ip. */
>>>>> + lrnat_rec->lb_force_snat_router_ip = true;
>>>>> + } else {
>>>>> + lrnat_rec->lb_force_snat_router_ip = false;
>>>>> +
>>>>> + /* Check if 'lb_force_snat_ip' is configured with a set of
>>>>> + * IP address(es). */
>>>>> + if (get_force_snat_ip(lrnat_rec, "lb",
>>>>> + &lrnat_rec->lb_force_snat_addrs)) {
>>>>> + if (lrnat_rec->lb_force_snat_addrs.n_ipv4_addrs) {
>>>>> + snat_ip_add(lrnat_rec,
>>>>> +
>>>> lrnat_rec->lb_force_snat_addrs.ipv4_addrs[0].addr_s,
>>>>> + NULL);
>>>>> + }
>>>>> + if (lrnat_rec->lb_force_snat_addrs.n_ipv6_addrs) {
>>>>> + snat_ip_add(lrnat_rec,
>>>>> +
>>>> lrnat_rec->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
>>>>> + NULL);
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (!lrnat_rec->od->nbr->n_nat) {
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + lrnat_rec->nat_entries =
>>>>> + xmalloc(lrnat_rec->od->nbr->n_nat * sizeof
>>>> *lrnat_rec->nat_entries);
>>>>> +
>>>>> + for (size_t i = 0; i < lrnat_rec->od->nbr->n_nat; i++) {
>>>>> + const struct nbrec_nat *nat = lrnat_rec->od->nbr->nat[i];
>>>>> + struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i];
>>>>> +
>>>>> + nat_entry->nb = nat;
>>>>> + if (!extract_ip_addresses(nat->external_ip,
>>>>> + &nat_entry->ext_addrs) ||
>>>>> + !nat_entry_is_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,
>>>>> + lrnat_rec->od->nbr->name);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + /* 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(lrnat_rec,
>>>>> + nat_entry->ext_addrs.ipv4_addrs[0].addr_s,
>>>>> + nat_entry);
>>>>> + } else {
>>>>> + snat_ip_add(lrnat_rec,
>>>>> + nat_entry->ext_addrs.ipv6_addrs[0].addr_s,
>>>>> + nat_entry);
>>>>> + }
>>>>> + } else {
>>>>> + if (!strcmp(nat->type, "dnat_and_snat")
>>>>> + && nat->logical_port && nat->external_mac) {
>>>>> + lrnat_rec->has_distributed_nat = true;
>>>>> + }
>>>>> +
>>>>> + if (nat->external_mac) {
>>>>> + sset_add(&lrnat_rec->external_macs, nat->external_mac);
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> + lrnat_rec->n_nat_entries = lrnat_rec->od->nbr->n_nat;
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +get_force_snat_ip(struct lr_nat_record *lrnat_rec, const char *key_type,
>>>>> + struct lport_addresses *laddrs)
>>>>> +{
>>>>> + char *key = xasprintf("%s_force_snat_ip", key_type);
>>>>> + const char *addresses = smap_get(&lrnat_rec->od->nbr->options, key);
>>>>> + free(key);
>>>>> +
>>>>> + if (!addresses) {
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>> + if (!extract_ip_address(addresses, laddrs)) {
>>>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>>>>> + VLOG_WARN_RL(&rl, "bad ip %s in options of router "UUID_FMT"",
>>>>> + addresses,
>>>> UUID_ARGS(&lrnat_rec->od->nbr->header_.uuid));
>>>>> + return false;
>>>>> + }
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +lr_nat_entries_destroy(struct lr_nat_record *lrnat_rec)
>>>>> +{
>>>>> + shash_destroy_free_data(&lrnat_rec->snat_ips);
>>>>> + destroy_lport_addresses(&lrnat_rec->dnat_force_snat_addrs);
>>>>> + destroy_lport_addresses(&lrnat_rec->lb_force_snat_addrs);
>>>>> +
>>>>> + for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) {
>>>>> + destroy_lport_addresses(&lrnat_rec->nat_entries[i].ext_addrs);
>>>>> + }
>>>>> +
>>>>> + free(lrnat_rec->nat_entries);
>>>>> + lrnat_rec->nat_entries = NULL;
>>>>> + lrnat_rec->n_nat_entries = 0;
>>>>> + sset_destroy(&lrnat_rec->external_macs);
>>>>> +}
>>>>> +
>>>>> +static struct lr_nat_input
>>>>> +lr_nat_get_input_data(struct engine_node *node)
>>>>> +{
>>>>> + struct northd_data *northd_data = engine_get_input_data("northd",
>>>> node);
>>>>> + return (struct lr_nat_input) {
>>>>> + .nbrec_logical_router_table =
>>>>> + EN_OVSDB_GET(engine_get_input("NB_logical_router", node)),
>>>>> + .lr_datapaths = &northd_data->lr_datapaths,
>>>>> + };
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +is_lr_nats_seqno_changed(const struct nbrec_logical_router *nbr)
>>>>> +{
>>>>> + for (size_t i = 0; i < nbr->n_nat; i++) {
>>>>> + if (nbrec_nat_row_get_seqno(nbr->nat[i],
>>>>> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
>>>>> + return true;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> +static bool
>>>>> +is_lr_nats_changed(const struct nbrec_logical_router *nbr) {
>>>>> + return (nbrec_logical_router_is_new(nbr)
>>>>> + || nbrec_logical_router_is_deleted(nbr)
>>>>> + || nbrec_logical_router_is_updated(nbr,
>>>>> +
>>>> NBREC_LOGICAL_ROUTER_COL_NAT)
>>>>> + || nbrec_logical_router_is_updated(
>>>>> + nbr, NBREC_LOGICAL_ROUTER_COL_OPTIONS)
>>>>> + || is_lr_nats_seqno_changed(nbr));
>>>>> +}
>>>>> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h
>>>>> new file mode 100644
>>>>> index 0000000000..01a16a21aa
>>>>> --- /dev/null
>>>>> +++ b/northd/en-lr-nat.h
>>>>> @@ -0,0 +1,134 @@
>>>>> +/*
>>>>> + * Copyright (c) 2023, Red Hat, Inc.
>>>>> + *
>>>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>>>> + * you may not use this file except in compliance with the License.
>>>>> + * You may obtain a copy of the License at:
>>>>> + *
>>>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>>>> + *
>>>>> + * Unless required by applicable law or agreed to in writing, software
>>>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>>>> implied.
>>>>> + * See the License for the specific language governing permissions and
>>>>> + * limitations under the License.
>>>>> + */
>>>>> +#ifndef EN_LR_NAT_H
>>>>> +#define EN_LR_NAT_H 1
>>>>> +
>>>>> +#include <stdint.h>
>>>>> +
>>>>> +/* OVS includes. */
>>>>> +#include "lib/hmapx.h"
>>>>> +#include "openvswitch/hmap.h"
>>>>> +#include "sset.h"
>>>>> +
>>>>> +/* OVN includes. */
>>>>> +#include "lib/inc-proc-eng.h"
>>>>> +#include "lib/ovn-nb-idl.h"
>>>>> +#include "lib/ovn-sb-idl.h"
>>>>> +#include "lib/ovn-util.h"
>>>>> +
>>>>> +/* Contains a NAT entry with the external addresses pre-parsed. */
>>>>> +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;
>>>>> +};
>>>>> +
>>>>> +struct lr_nat_record {
>>>>> + struct hmap_node key_node; /* Index on 'nbr->header_.uuid'. */
>>>>> +
>>>>> + const struct ovn_datapath *od;
>>>>> +
>>>>> + struct ovn_nat *nat_entries;
>>>>> + size_t n_nat_entries;
>>>>> +
>>>>> + bool has_distributed_nat;
>>>>> +
>>>>> + /* Set of nat external ips on the router. */
>>>>> + struct sset external_ips;
>>>>> +
>>>>> + /* Set of nat external macs on the router. */
>>>>> + struct sset external_macs;
>>>>> +
>>>>> + /* 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;
>>>>> + bool lb_force_snat_router_ip;
>>>>> +};
>>>>> +
>>>>> +struct lr_nat_tracked_data {
>>>>> + /* Created or updated logical router with NAT data. */
>>>>> + struct hmapx crupdated;
>>>>> +
>>>>> + /* Deleted logical router with NAT data. */
>>>>> + struct hmapx deleted; /* Stores 'struct lr_nat_record'. */
>>>>> +};
>>>>> +
>>>>> +struct lr_nat_table {
>>>>> + struct hmap entries; /* Stores struct lr_nat_record. */
>>>>> +
>>>>> + /* The array index of each element in 'entries'. */
>>>>> + struct lr_nat_record **array;
>>>>> +};
>>>>> +
>>>>> +const struct lr_nat_record * lr_nat_table_find_by_index(
>>>>> + const struct lr_nat_table *, size_t od_index);
>>>>> +
>>>>> +/* Incremental processing implementation. */
>>>>> +struct lr_nat_input {
>>>>> + /* Northbound table references. */
>>>>> + const struct nbrec_logical_router_table *nbrec_logical_router_table;
>>>>> +
>>>>> + const struct ovn_datapaths *lr_datapaths;
>>>>> +};
>>>>> +
>>>>> +struct ed_type_lr_nat_data {
>>>>> + struct lr_nat_table lr_nats;
>>>>> +
>>>>> + bool tracked;
>>>>> + struct lr_nat_tracked_data tracked_data;
>>
>> We don't really need the "bool tracked". We can quite easily have an
>> inline function along the lines of:
>>
>> static inline bool
>> lr_nat_is_tracked_data(const struct lr_nat_tracked_data *td)
>> {
>> return !hmapx_is_empty(td->deleted)
>> || !hmapx_is_empty(td->crupdated);
>> }
>
> Ack. Addressed in v3.
>
>>
>> This is actually true for struct ed_type_lb_data->tracked as well but
>> that must be handled in a different patch.
>
> I haven't included this change in this series. Do you want me to
> include it in v4 as a separate patch ?
>
That would be great; it would make the code a bit more consistent I think.
Thanks for your replies!
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
