Hi Dumitru! I have prepared v3 with your comments addressed. Since the code has been sufficiently reworked some of your comments lost it's grounds. I will mention this fact as N/A in my answers.
On 8/4/25 1:30 PM, Dumitru Ceara wrote: > 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? > I do not think it i possible due to my heavy load. I plan to create system test later and send as follow-up. >> 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 Applied in all places affected in new code. > >> + 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? > N/A Renamed and commented in corresponding place in new code. >> + >> /* 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? > N/A This code is completely removed from ls-nat processing to new ls-arp processing. A logic here is to catch a change in the nat and trigger lflow regeneration for all lswitches that are affected. A change could be deletion or change type of a nat gateway that follows to loss a lr<->ls relationship that I am interested in. Thus, I need somehow know what were relationships BEFORE nat change and what are new relationships came with current change. In new version of the code this logic moves into ls-arp processing (nat_records struct member). >> } >> >> 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. > N/A >> + 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? > All processing code is now removed from both lr-nat and ls-stateful to new processing ls-arp. Unfortunately, I cannot process ALL conditions in new code but only most complicated case when a change in the nat triggers updates in lswitch'es lflow. Other cases not so difficult to process but they will require northd enabled to process all changes in lr, lrp, ls, lsp that is not working for now. >> +} >> + >> /* 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? > Function removed, decision is made in build_lswitch_arp_chassis_resident >> +{ >> + 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. > Check removed >> + 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? > This part is changed to rely on computation results stored in ls-arp processing data. >> + && 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. > Rewroked to omit duplicates. >> + } >> + } >> + } >> + >> + 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
