On 6/9/25 11:42 AM, Aleksandr Smirnov wrote:
> The northd creates flows that restrict processing ARP requests:
> If ARP request comes from lswitch that has either localnet or
> l2gateway ports and router's attachment port is L3 dgw port such
> ARP will be allowed only on resident chassis.
>
> However, same lswitch could also have 'normal' aka virtual lports.
> For these lports restrictions described above should not be applied.
>
> The fix moves is_chassis_resident check from lrouter to lswitch level.
> The residence check is only applied for ARP requests that were
> originated from localnet/l2gateway ports. If such check is succeeded
> ARP request is allowed otherwise ARP request packet is dropped.
> For ARP requests coming from virtual ports no residence restrictions
> are applied.
>
> Co-authored-by: Alexandra Rukomoinikova <[email protected]>
> Signed-off-by: Aleksandr Smirnov <[email protected]>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
Hi Aleksandr, Alexandra,
First of all, sorry for the long delay in reviewing this patch.
I was initially thinking that this is a new feature to be added in 25.09
but after a closer look at the patch it's clear now (as you mentioned in
the commit message too) that this is a bug fix. So we don't have to
hurry to get it in before 25.09 branching (August 15th), bug fixes can
go in at any time in the development cycle.
> v2: Minor fixes, add co-authored-by
> ---
> northd/en-lr-nat.c | 25 +++++++
> northd/en-lr-nat.h | 3 +
> northd/en-ls-stateful.c | 36 +++++++++
> northd/en-ls-stateful.h | 2 +
> northd/inc-proc-northd.c | 1 +
> northd/northd.c | 135 ++++++++++++++++++++++++++++++---
> northd/northd.h | 4 +
> tests/ovn-northd.at | 10 +--
> tests/ovn.at | 156 +++++++++++++++++++++++++++++++++++++++
It would really make it easier to review and ensure we don't break this
behavior in the future if we had a system-ovn.at test too for this
functionality. Would it be possible to add one in v3?
> 9 files changed, 356 insertions(+), 16 deletions(-)
>
> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
> index 18a5a6255..9b6eb9ecd 100644
> --- a/northd/en-lr-nat.c
> +++ b/northd/en-lr-nat.c
> @@ -83,6 +83,7 @@ en_lr_nat_init(struct engine_node *node OVS_UNUSED,
> struct ed_type_lr_nat_data *data = xzalloc(sizeof *data);
> lr_nat_table_init(&data->lr_nats);
> hmapx_init(&data->trk_data.crupdated);
> + hmapx_init(&data->trk_data.mod_l3dgw);
> return data;
> }
>
> @@ -92,6 +93,7 @@ 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->trk_data.crupdated);
> + hmapx_destroy(&data->trk_data.mod_l3dgw);
> }
>
> void
> @@ -99,6 +101,7 @@ en_lr_nat_clear_tracked_data(void *data_)
> {
> struct ed_type_lr_nat_data *data = (struct ed_type_lr_nat_data *) data_;
> hmapx_clear(&data->trk_data.crupdated);
> + hmapx_clear(&data->trk_data.mod_l3dgw);
> }
>
> enum engine_node_state
> @@ -116,6 +119,23 @@ en_lr_nat_run(struct engine_node *node, void *data_)
> return EN_UPDATED;
> }
>
> +static void
> +collect_l3dgw_modified(struct lr_nat_record *lrnat_rec,
> + struct hmapx *portsmap)
> +{
> + for (int i = 0; i < lrnat_rec->n_nat_entries; i++) {
Nit: size_t i
> + struct ovn_nat *ent = &lrnat_rec->nat_entries[i];
> +
> + if (ent->is_valid
> + && ent->l3dgw_port
> + && ent->l3dgw_port->peer
> + && ent->l3dgw_port->peer->od
> + && !ent->is_distributed) {
> + hmapx_add(portsmap, ent->l3dgw_port->peer->od);
> + }
> + }
> +}
Maybe I'm missing something but I'm not sure I understand what this
function does. It's called "collect_l3dgw_modified()" but it actually
just populates a map of datapaths that have at least one non-distributed
NAT entry. Should we rename the function?
> +
> /* Handler functions. */
> enum engine_input_handler_result
> lr_nat_northd_handler(struct engine_node *node, void *data_)
> @@ -138,10 +158,15 @@ 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);
> +
> + collect_l3dgw_modified(lrnat_rec, &data->trk_data.mod_l3dgw);
> +
> 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);
> +
> + collect_l3dgw_modified(lrnat_rec, &data->trk_data.mod_l3dgw);
Hmm, we call collect_l3dgw_modified() twice. Once before
lr_nat_record_reinit() and once immediately after. Is the goal to end
up with a set of router datapaths that potentially had distributed nat
changes?
Although what we end up with in &data->trk_data.mod_l3dgw at this point
is all the router datapaths that had OR now have at least one
non-distributed NAT.
Was that your intention?
> }
>
> if (lr_nat_has_tracked_data(&data->trk_data)) {
> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h
> index df4eb0786..6d8320393 100644
> --- a/northd/en-lr-nat.h
> +++ b/northd/en-lr-nat.h
> @@ -94,6 +94,9 @@ struct lr_nat_record {
> struct lr_nat_tracked_data {
> /* Created or updated logical router with NAT data. */
> struct hmapx crupdated;
> +
> + /* Modified l3dgw_port values. */
This comment is very confusing to me. This map actually stores pointers
to struct ovn_datapath. Please see above.
> + struct hmapx mod_l3dgw;
> };
>
> struct lr_nat_table {
> diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c
> index b713b5bce..8345d0e16 100644
> --- a/northd/en-ls-stateful.c
> +++ b/northd/en-ls-stateful.c
> @@ -31,6 +31,7 @@
>
> /* OVN includes */
> #include "en-lb-data.h"
> +#include "en-lr-nat.h"
> #include "en-ls-stateful.h"
> #include "en-port-group.h"
> #include "lib/inc-proc-eng.h"
> @@ -235,6 +236,41 @@ ls_stateful_port_group_handler(struct engine_node *node,
> void *data_)
> return EN_HANDLED_UNCHANGED;
> }
>
> +enum engine_input_handler_result
> +ls_stateful_lr_nat_handler(struct engine_node *node, void *data_)
> +{
> + struct ed_type_lr_nat_data *lr_nat_data =
> + engine_get_input_data("lr_nat", node);
> +
> + if (!lr_nat_has_tracked_data(&lr_nat_data->trk_data)) {
> + return EN_UNHANDLED;
> + }
> +
> + struct ls_stateful_input input_data = ls_stateful_get_input_data(node);
> + struct ed_type_ls_stateful *data = data_;
> +
> + struct hmapx_node *hmapx_node;
> + HMAPX_FOR_EACH (hmapx_node, &lr_nat_data->trk_data.mod_l3dgw) {
> + const struct ovn_datapath *od = hmapx_node->data;
> +
> + struct ls_stateful_record *ls_stateful_rec = ls_stateful_table_find_(
> + &data->table, od->nbs);
> + ovs_assert(ls_stateful_rec);
> +
> + ls_stateful_record_reinit(ls_stateful_rec, od, NULL,
> + input_data.ls_port_groups);
> +
> + /* Add the ls_stateful_rec to the tracking data. */
> + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec);
> + }
> +
> + if (ls_stateful_has_tracked_data(&data->trk_data)) {
> + return EN_HANDLED_UPDATED;
> + }
> +
> + return EN_HANDLED_UNCHANGED;
I'm not sure I completely agree with reusing the en-ls-stateful
incremental processing node for this purpose. Shouldn't we have a
separate, small and well contained node, that computes any intermediate
data that we need for processing external ARPs on ha chassis?
> +}
> +
> /* static functions. */
> static void
> ls_stateful_table_init(struct ls_stateful_table *table)
> diff --git a/northd/en-ls-stateful.h b/northd/en-ls-stateful.h
> index 1d98b3695..40672281c 100644
> --- a/northd/en-ls-stateful.h
> +++ b/northd/en-ls-stateful.h
> @@ -109,6 +109,8 @@ enum engine_input_handler_result
> ls_stateful_northd_handler(struct engine_node *, void *data);
> enum engine_input_handler_result
> ls_stateful_port_group_handler(struct engine_node *, void *data);
> +enum engine_input_handler_result
> +ls_stateful_lr_nat_handler(struct engine_node *node, void *data);
>
> const struct ls_stateful_record *ls_stateful_table_find(
> const struct ls_stateful_table *, const struct nbrec_logical_switch *);
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index b1e4994a4..d7252a2c4 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -257,6 +257,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_ls_stateful, &en_northd,
> ls_stateful_northd_handler);
> engine_add_input(&en_ls_stateful, &en_port_group,
> ls_stateful_port_group_handler);
> + engine_add_input(&en_ls_stateful, &en_lr_nat,
> ls_stateful_lr_nat_handler);
>
> engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 2da94e7e1..b2fbe4e87 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -135,6 +135,7 @@ static bool vxlan_ic_mode;
> #define REGBIT_IP_FRAG "reg0[19]"
> #define REGBIT_ACL_PERSIST_ID "reg0[20]"
> #define REGBIT_ACL_HINT_ALLOW_PERSISTED "reg0[21]"
> +#define REGBIT_EXT_ARP "reg0[22]"
>
> /* Register definitions for switches and routers. */
>
> @@ -498,6 +499,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct
> uuid *key,
> hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
> od->lr_group = NULL;
> hmap_init(&od->ports);
> + hmapx_init(&od->ph_ports);
> sset_init(&od->router_ips);
> od->ls_peers = VECTOR_EMPTY_INITIALIZER(struct ovn_datapath *);
> od->router_ports = VECTOR_EMPTY_INITIALIZER(struct ovn_port *);
> @@ -532,6 +534,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> vector_destroy(&od->l3dgw_ports);
> destroy_mcast_info_for_datapath(od);
> destroy_ports_for_datapath(od);
> + hmapx_destroy(&od->ph_ports);
> sset_destroy(&od->router_ips);
> free(od);
> }
> @@ -1422,6 +1425,12 @@ lsp_is_vtep(const struct nbrec_logical_switch_port
> *nbsp)
> return !strcmp(nbsp->type, "vtep");
> }
>
> +static bool
> +lsp_is_l2gw(const struct nbrec_logical_switch_port *nbsp)
> +{
> + return !strcmp(nbsp->type, "l2gateway");
> +}
> +
> static bool
> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
> {
> @@ -2279,6 +2288,10 @@ join_logical_ports_lsp(struct hmap *ports,
> od->has_vtep_lports = true;
> }
>
> + if (lsp_is_localnet(nbsp) || lsp_is_l2gw(nbsp)) {
> + hmapx_add(&od->ph_ports, op);
> + }
> +
> parse_lsp_addrs(op);
>
> op->od = od;
> @@ -2672,6 +2685,15 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> }
> }
>
> +static bool
> +is_nat_distributed(const struct nbrec_nat *nat,
> + const struct ovn_datapath *od)
> +{
> + return !vector_is_empty(&od->l3dgw_ports)
> + && nat->logical_port && nat->external_mac
> + && !strcmp(nat->type, "dnat_and_snat");
> +}
> +
> /* Returns an array of strings, each consisting of a MAC address followed
> * by one or more IP addresses, and if the port is a distributed gateway
> * port, followed by 'is_chassis_resident("LPORT_NAME")', where the
> @@ -2730,9 +2752,7 @@ get_nat_addresses(const struct ovn_port *op, size_t *n,
> bool routable_only,
>
> /* Determine whether this NAT rule satisfies the conditions for
> * distributed NAT processing. */
> - if (!vector_is_empty(&op->od->l3dgw_ports) &&
> - !strcmp(nat->type, "dnat_and_snat") &&
> - nat->logical_port && nat->external_mac) {
> + if (is_nat_distributed(nat, op->od)) {
> /* Distributed NAT rule. */
> if (eth_addr_from_string(nat->external_mac, &mac)) {
> struct ds address = DS_EMPTY_INITIALIZER;
> @@ -9658,6 +9678,104 @@
> build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
> ds_destroy(&match);
> }
>
> +/*
> + * Check od, assumed lswitch, for router connections that
> + * requires chassis residence.
> + */
> +static bool
> +od_has_chassis_bound_lrps(const struct ovn_datapath *od)
The comment states it but the function name doesn't make it explicit.
This is only for switches. Furthermore, it's confusing to say "od has
chassis bound lrps" for a switch. Maybe we should call this
"switch_is_connected_to_l3dgw_router()" or something similar?
> +{
> + struct ovn_port *op;
> + VECTOR_FOR_EACH (&od->router_ports, op) {
> + struct ovn_port *op_r = op->peer;
> +
> + if (lrp_is_l3dgw(op_r)) {
> + return true;
> + }
> +
> + for (int ni = 0; ni < op_r->od->nbr->n_nat; ni++) {
Nit: size_t
> + const struct nbrec_nat *nat = op_r->od->nbr->nat[ni];
> +
> + /* Determine whether this NAT rule satisfies the
> + * conditions for distributed NAT processing. */
> + if (is_nat_gateway_port(nat, op_r)
> + && is_nat_distributed(nat, od)) {
> + return true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Create ARP filtering flow for od, assumed logical switch,
> + * for the following condition:
> + * Given lswitch has either localnet or l2gateway ports and
> + * router connection ports that requires chassis residence.
> + * ARP requests coming from localnet/l2gateway ports
> + * allowed for processing on resident chassis only.
> + */
> +static void
> +build_lswitch_arp_chassis_resident(const struct ovn_datapath *od,
> + struct lflow_table *lflows,
> + struct lflow_ref *lflow_ref)
> +{
> + if (hmapx_is_empty(&od->ph_ports) ||
> + !od_has_chassis_bound_lrps(od)) {
I don't think we should compute this in the en_lflow incremental
processing node. This is a property of the datapath (switch). It
should be computed in a previous I-P node.
> + return;
> + }
> +
> + struct ds match = DS_EMPTY_INITIALIZER;
> + struct hmapx_node *node;
> +
> + HMAPX_FOR_EACH (node, &od->ph_ports) {
> + struct ovn_port *op = node->data;
> +
> + ds_clear(&match);
> + ds_put_format(&match, "(arp.op == 1 || arp.op == 2) && inport == %s",
> + op->json_key);
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 75,
> + ds_cstr(&match), REGBIT_EXT_ARP" = 1; next;",
> lflow_ref);
> + }
> +
> + struct ovn_port *op;
> + VECTOR_FOR_EACH (&od->router_ports, op) {
> + struct ovn_port *op_r = op->peer;
> +
> + if (lrp_is_l3dgw(op_r)) {
> + ds_clear(&match);
> + ds_put_format(&match,
> + REGBIT_EXT_ARP" == 1 && is_chassis_resident(%s)",
> + op_r->cr_port->json_key);
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75,
> + ds_cstr(&match), "next;", lflow_ref);
> + }
> +
> + for (int ni = 0; ni < op_r->od->nbr->n_nat; ni++) {
> + const struct nbrec_nat *nat = op_r->od->nbr->nat[ni];
> +
> + /* Determine whether this NAT rule satisfies the
> + * conditions for distributed NAT processing. */
> + if (is_nat_gateway_port(nat, op_r)
This has the potential of being quite CPU heavy. The function calls
lrp_find_member_ip() internally and in this case it will be called for
each NAT record of routers connected to switches. A router can be
connected to multiple switches which means we'd be processing the same
NAT multiple times.
Should we do this processing in sync_pb_for_lsp() and store whatever we
need for flow generation in the struct ovn_port?
> + && is_nat_distributed(nat, op_r->od)) {
> + ds_clear(&match);
> + ds_put_format(&match,
> + REGBIT_EXT_ARP
> + " == 1 && is_chassis_resident(\"%s\")",
> + nat->logical_port);
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75,
> + ds_cstr(&match), "next;", lflow_ref);
Won't this create identical lflows, one for each NAT configured on a
neighbor router? It's a bit weird we don't match on 'op->json_key'
anywhere.
> + }
> + }
> + }
> +
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 70,
> + REGBIT_EXT_ARP" == 1", "drop;", lflow_ref);
> +
> + ds_destroy(&match);
> +}
> +
> static bool
> is_vlan_transparent(const struct ovn_datapath *od)
> {
> @@ -13904,10 +14022,6 @@ build_neigh_learning_flows_for_lrouter_port(
> op->lrp_networks.ipv4_addrs[i].network_s,
> op->lrp_networks.ipv4_addrs[i].plen,
> op->lrp_networks.ipv4_addrs[i].addr_s);
> - if (lrp_is_l3dgw(op)) {
> - ds_put_format(match, " && is_chassis_resident(%s)",
> - op->cr_port->json_key);
> - }
> const char *actions_s = REGBIT_LOOKUP_NEIGHBOR_RESULT
> " = lookup_arp(inport, arp.spa, arp.sha); "
> REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1;"
> @@ -13924,10 +14038,6 @@ build_neigh_learning_flows_for_lrouter_port(
> op->json_key,
> op->lrp_networks.ipv4_addrs[i].network_s,
> op->lrp_networks.ipv4_addrs[i].plen);
> - if (lrp_is_l3dgw(op)) {
> - ds_put_format(match, " && is_chassis_resident(%s)",
> - op->cr_port->json_key);
> - }
> ds_clear(actions);
> ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> " = lookup_arp(inport, arp.spa, arp.sha); %snext;",
> @@ -17796,6 +17906,9 @@ build_ls_stateful_flows(const struct
> ls_stateful_record *ls_stateful_rec,
> sampling_apps, features, ls_stateful_rec->lflow_ref,
> sbrec_acl_id_table);
> build_lb_hairpin(ls_stateful_rec, od, lflows,
> ls_stateful_rec->lflow_ref);
> +
> + build_lswitch_arp_chassis_resident(od, lflows,
> + ls_stateful_rec->lflow_ref);
> }
>
> struct lswitch_flow_build_info {
> diff --git a/northd/northd.h b/northd/northd.h
> index fc138aab5..f228b16ed 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -20,6 +20,7 @@
> #include "lib/ovn-util.h"
> #include "lib/ovs-atomic.h"
> #include "lib/sset.h"
> +#include "lib/hmapx.h"
> #include "northd/en-port-group.h"
> #include "northd/ipam.h"
> #include "openvswitch/hmap.h"
> @@ -418,6 +419,9 @@ struct ovn_datapath {
> * Valid only if it is logical router datapath. NULL otherwise. */
> struct lrouter_group *lr_group;
>
> + /* Set of localnet or l2gw ports. */
> + struct hmapx ph_ports;
> +
> /* Map of ovn_port objects belonging to this datapath.
> * This map doesn't include derived ports. */
> struct hmap ports;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 97eaa6a40..ab26272b2 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -7253,10 +7253,10 @@ AT_CHECK([grep lr_in_admission lrflows | grep cr-DR |
> ovn_strip_lflows], [0], [d
> table=??(lr_in_admission ), priority=50 , match=(eth.dst ==
> 04:ac:10:01:00:01 && inport == "DR-S3" && is_chassis_resident("cr-DR-S3")),
> action=(xreg0[[0..47]] = 04:ac:10:01:00:01; next;)
> ])
> # Check the flows in lr_in_lookup_neighbor stage
> -AT_CHECK([grep lr_in_lookup_neighbor lrflows | grep cr-DR |
> ovn_strip_lflows], [0], [dnl
> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S1"
> && arp.spa == 172.16.1.0/24 && arp.op == 1 &&
> is_chassis_resident("cr-DR-S1")), action=(reg9[[2]] = lookup_arp(inport,
> arp.spa, arp.sha); next;)
> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S2"
> && arp.spa == 172.16.2.0/24 && arp.op == 1 &&
> is_chassis_resident("cr-DR-S2")), action=(reg9[[2]] = lookup_arp(inport,
> arp.spa, arp.sha); next;)
> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S3"
> && arp.spa == 172.16.3.0/24 && arp.op == 1 &&
> is_chassis_resident("cr-DR-S3")), action=(reg9[[2]] = lookup_arp(inport,
> arp.spa, arp.sha); next;)
> +AT_CHECK([grep lr_in_lookup_neighbor lrflows | grep DR-S. |
> ovn_strip_lflows], [0], [dnl
> + table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S1"
> && arp.spa == 172.16.1.0/24 && arp.op == 1), action=(reg9[[2]] =
> lookup_arp(inport, arp.spa, arp.sha); next;)
> + table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S2"
> && arp.spa == 172.16.2.0/24 && arp.op == 1), action=(reg9[[2]] =
> lookup_arp(inport, arp.spa, arp.sha); next;)
> + table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S3"
> && arp.spa == 172.16.3.0/24 && arp.op == 1), action=(reg9[[2]] =
> lookup_arp(inport, arp.spa, arp.sha); next;)
> ])
> # Check the flows in lr_in_gw_redirect stage
> AT_CHECK([grep lr_in_gw_redirect lrflows | grep cr-DR | ovn_strip_lflows],
> [0], [dnl
> @@ -13989,7 +13989,7 @@ AT_CHECK([grep -e "172.168.0.110" -e "172.168.0.120"
> -e "10.0.0.3" -e "20.0.0.3"
> table=??(lr_out_undnat ), priority=100 , match=(ip && ip4.src ==
> 20.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")),
> action=(ct_dnat;)
> ])
>
> -AT_CHECK([grep -e "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e
> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" publicflows |
> ovn_strip_lflows], [0], [dnl
> +AT_CHECK([grep -e "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e
> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" publicflows |
> ovn_strip_lflows | grep -v "reg0.*22"], [0], [dnl
> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst ==
> 30:54:00:00:00:03 && is_chassis_resident("sw0-port1")), action=(outport =
> "public-lr0"; output;)
> table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
> {00:00:00:00:ff:02, 30:54:00:00:00:03} && eth.dst == ff:ff:ff:ff:ff:ff &&
> (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport = "_MC_flood_l2";
> output;)
> table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
> arp.op == 1 && arp.tpa == 172.168.0.110), action=(clone {outport =
> "public-lr0"; output; }; outport = "_MC_flood_l2"; output;)
Can you please also add an ovn-northd.at test that validates the newly
added logical flows? I think that would've made explicit the issue with
duplicate lflows (one per NAT) I listed above.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index dfa0399b4..d5ef2c81d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -43469,3 +43469,159 @@ AT_CHECK([grep -q "Postponing virtual lport
> sw0-vir" hv3/ovn-controller.log], [1
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([GARP delivery: gw and extenal ports])
> +ovn_start
> +#
> +# Configure initial environment
> +# LR1: down_link <-> LS1: up_link
> +# set lr_down: gateway port (chassis redirect) bound to hv1
> +# LS1: down_vif1 - vif port bound to hv1
> +# down_vif2 - vif port bound to hv2
> +# down_ext - outher port will be iteratred as localnet, l2gateway
> +#
> +# Test: throw ARP annonce request from vitrual ports (down_vif1, down_vif2)
> +# ensure mac_binding is always updated.
> +# (Fixing the issue: mac_binding is only updated for packets came from
> +# down_link's resident chassis)
> +# throw ARP annonce request from from localnet.
> +# ensure mac_binding is updated only if localnet bound to same hv as
> l3dgw
> +#
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl lrp-add lr1 down_link f0:00:00:00:00:f1 192.168.1.1/24
> +
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl lsp-add ls1 up_link
> +check ovn-nbctl lsp-add ls1 down_vif1
> +check ovn-nbctl lsp-add ls1 down_vif2
> +check ovn-nbctl lsp-add ls1 down_ext
> +
> +check ovn-nbctl set Logical_Switch_Port up_link \
> + type=router \
> + options:router-port=down_link \
> + addresses=router
> +
> +check ovn-nbctl lsp-set-addresses down_vif1 'f0:00:00:00:00:01 192.168.1.101'
> +check ovn-nbctl lsp-set-addresses down_vif2 'f0:00:00:00:00:02 192.168.1.102'
> +
> +check ovn-nbctl lsp-set-type down_ext localnet
> +check ovn-nbctl lsp-set-options down_ext network_name=physnet1
> +check ovn-nbctl lrp-set-gateway-chassis down_link hv1
> +
> +net_add n1
> +
> +# Create hypervisor hv1 connected to n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl add-port br-int vif1 -- \
> + set Interface vif1 external-ids:iface-id=down_vif1 \
> + options:tx_pcap=hv1/vif1-tx.pcap options:rxq_pcap=hv1/vif1-rx.pcap \
> + ofport_request=1
> +
> +# Create hypervisor hv2 connected to n1, add localnet here
> +sim_add hv2
> +as hv2
> +ovs-vsctl add-br br-phys
> +ovs-vsctl add-br br-eth0
> +ovn_attach n1 br-phys 192.168.0.2
> +ovs-vsctl add-port br-int vif2 -- \
> + set Interface vif2 external-ids:iface-id=down_vif2 \
> + options:tx_pcap=hv2/vif2-tx.pcap options:rxq_pcap=hv2/vif2-rx.pcap \
> + ofport_request=1
No need for ofport_request.
> +
> +ovs-vsctl set Open_vSwitch .
> external_ids:ovn-bridge-mappings="physnet1:br-eth0"
> +
> +ovs-vsctl add-port br-eth0 vif_ext -- \
> + set Interface vif_ext options:tx_pcap=hv2/vif_ext-tx.pcap \
> + options:rxq_pcap=hv2/vif_ext-rx.pcap
> +
> +# Pre-populate the hypervisors' ARP tables so that we don't lose any
> +# packets for ARP resolution (native tunneling doesn't queue packets
> +# for ARP resolution).
> +OVN_POPULATE_ARP
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +#
> +# Annonce 192.168.1.222 from localnet in hv2
> +# result: drop, hv2 is not gateway chassis for down_link
> +#
> +sha=0200000000ee
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 222`
> +tpa=$spa
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv2 ovs-appctl netdev-dummy/receive vif_ext $garp
> +
Please use fmt_pkt() instead of manually crafting packets. We actually
already have send_garp() for injecting GARPs.
> +#
> +# Make hv2 gateway chassis
> +# Annonce 192.168.1.223 from localnet in hv2
> +# result: ok, hv2 is gateway chassis for down_link
> +#
> +check ovn-nbctl lrp-set-gateway-chassis down_link hv2
> +
> +wait_row_count Port_Binding 1 logical_port=cr-down_link 'chassis!=[[]]'
> +
> +sha=0200000000ee
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 223`
> +tpa=$spa
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv2 ovs-appctl netdev-dummy/receive vif_ext $garp
> +
> +#
> +# Annonce 192.168.1.111, 112 from vif1, vif2 in hv1, hv2
> +# result: ok, vif1, vif2 are virtual ports, restrictions are not applied.
> +#
> +sha=f00000000001
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 111`
> +tpa=000000000000
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv1 ovs-appctl netdev-dummy/receive vif1 $garp
> +
> +sha=f00000000002
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 112`
> +tpa=000000000000
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv2 ovs-appctl netdev-dummy/receive vif2 $garp
> +
> +#
> +# Set down_ext type to l2gateway
> +# Annonce 192.168.1.113, 114 from vif1, vif2 in hv1, hv2
> +# result: ok, vif1, vif2 are virtual ports, restrictions are not applied.
> +#
> +check ovn-nbctl --wait=hv lsp-set-type down_ext l2gateway
> +
> +sha=f00000000001
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 113`
> +tpa=000000000000
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv1 ovs-appctl netdev-dummy/receive vif1 $garp
> +
> +sha=f00000000002
> +tha=000000000000
> +spa=`ip_to_hex 192 168 1 114`
> +tpa=000000000000
> +garp=ffffffffffff${sha}08060001080006040001${sha}${spa}${tha}${tpa}
> +as hv2 ovs-appctl netdev-dummy/receive vif2 $garp
> +
> +wait_row_count MAC_Binding 1 ip="192.168.1.111" mac='"f0:00:00:00:00:01"'
> logical_port='"down_link"'
> +wait_row_count MAC_Binding 1 ip="192.168.1.112" mac='"f0:00:00:00:00:02"'
> logical_port='"down_link"'
> +wait_row_count MAC_Binding 1 ip="192.168.1.113" mac='"f0:00:00:00:00:01"'
> logical_port='"down_link"'
> +wait_row_count MAC_Binding 1 ip="192.168.1.114" mac='"f0:00:00:00:00:02"'
> logical_port='"down_link"'
> +wait_row_count MAC_Binding 1 ip="192.168.1.223" mac='"02:00:00:00:00:ee"'
> logical_port='"down_link"'
> +wait_row_count MAC_Binding 0 ip="192.168.1.222" mac='"02:00:00:00:00:ee"'
> logical_port='"down_link"'
> +
> +check ovn-nbctl --wait=hv lsp-set-type down_ext localnet
> +
> +OVN_CLEANUP([hv1],[hv2])
> +AT_CLEANUP
> +])
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
