Hi Rosemarie, On 3/11/25 10:29 PM, Rosemarie O'Riorden wrote: > When a gateway router has a load balancer configured, the option > lb_force_snat_ip=router_ip can be set so that OVN SNATs load balanced > packets to the logical router's egress interface's IP address, that is, the > port chosen as "outport" in the lr_in_ip_routing stage. > > However, this was only designed to work when one network was configured > on the logical router outport. When multiple networks were configured, > OVN's behavior was to simply choose the lexicographically first IP address for > SNAT. This often lead to an incorrect address being used for SNAT. > > To fix this, two main components have been added: > 1. A new flag, flags.network_id. It is 4 bits and stores an index. > 2. A new stage in the router ingress pipeline, lr_in_network_id.
The discussion we had with Aleksandra and Ilya here reminded me that we should increment OVN_INTERNAL_MINOR_VER every time we add new logical stages: https://mail.openvswitch.org/pipermail/ovs-dev/2025-March/421992.html > > Now in the stage lr_in_network_id, OVN generates flows that assign > flags.network_id with an index, which is chosen by looping through the > networks > on the port, and assigning flags.network_id = i. flags.network_id is then > matched on later in lr_out_snat and the network at that index will be chosen > for SNAT. > > However, if there are more than 16 networks, flags.network_id will be 0 > for networks 17 and up. Then, for those networks, the first > lexicographical network will be chosen for SNAT. > > There is also a lower priority flow with the old behavior to make upgrades > smooth. > > Two tests have been added to verify that: > 1. The correct network is chosen for SNAT. > 2. The new and updated flows with flags.network_id are correct. > > And tests that were broken by this new behavior have been updated. > > Reported-at: https://issues.redhat.com/browse/FDP-871 > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2024-October/417717.html > Signed-off-by: Rosemarie O'Riorden <rosema...@redhat.com> > --- > v4: > - Change "op->lrp_networks.ipv4_addrs" to "op->lrp_networks.ipv6_addrs" in > error message for IPv6 > - Add flow with old behavior only if the router port has at least one IPv4 > address, or 2 IPv6 addresses (2 because LLA is always there). > - Add ovn-northd.at test to cover case when LRP has just a single IPv6 address > (LLA) and no IPv4. Make sure priority-105 flow is not added. > - Fixed tests where flows were affected by changes > v3: > - Fix "16 bits" typo, flags.network_id 4 bits (fits 16 networks) > - Replace inconsistent comments (networks "17 and up" get network_id=0) > - Define OVN_MAX_NETWORK_ID macro, use instead of hardcoding 15 or 16 > - Use OVN_MAX_NETWORK_ID while defining MLF_NETWORK_ID > - Fix incorrect expression resulting in wrong hex value > - Remove extra quotations around port name, resulting in double quotes and > thus > invalid flows > - Make priority-100 flow (with old behavior) 105 instead > - Remove "flags.force_snat_for_lb == 1" from match in new stage, set > network_id > for all IP packets instead > - Change function name for new stage since it's no longer restricted to "force > snat for lb" case > - Update affected test flows > - Fix comment that was too long, improve wording > - Replace "routerip" with "router_ip" > v2: > - Improve wording of commit message and add missing explanation > - Add Reported-at tag > - Correct hex value for MLF_NETWORK_ID in include/ovn/logical-fields.h > - In northd/northd.c: > - Change behavior to set flags.network_id=0 for networks 16 and up > - Add flow with old behavior for consistency during upgrades > - Fix redundant loop conditions > - Remove code I accidentally included in v1 > - Fix indentation in multiple spots > - Remove incorrect changes in build_lrouter_nat_defrag_and_lb() > - Fix documentation, add new stage > - Move end-to-end test from ovn-northd.at to ovn.at > - Add check before all ovn and ovs commands in test > - Remove unnecessary section included in test > - Updated all flows in tests that were affected by new changes > - Added more IP addresses to test to test behavior with >16 networks > --- > include/ovn/logical-fields.h | 12 ++- > lib/logical-fields.c | 3 + > northd/northd.c | 184 +++++++++++++++++++++++++++++------ > northd/northd.h | 3 +- > northd/ovn-northd.8.xml | 59 +++++++++-- > tests/ovn-northd.at | 146 ++++++++++++++++++++++++--- > tests/ovn.at | 71 ++++++++++++++ > 7 files changed, 424 insertions(+), 54 deletions(-) > > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > index 196ac9dd8..b9446a042 100644 > --- a/include/ovn/logical-fields.h > +++ b/include/ovn/logical-fields.h > @@ -71,6 +71,10 @@ enum ovn_controller_event { > #define MFF_LOG_LB_AFF_MATCH_PORT_OLD MFF_REG8 > #define MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD MFF_XXREG0 > > +/* Maximum number of networks supported by 4-bit flags.network_id. */ > +#define OVN_MAX_NETWORK_ID \ > + ((1 << (MLF_NETWORK_ID_END_BIT - MLF_NETWORK_ID_START_BIT + 1)) - 1) > + > void ovn_init_symtab(struct shash *symtab); > > /* MFF_LOG_FLAGS_REG bit assignments */ > @@ -97,6 +101,8 @@ enum mff_log_flags_bits { > MLF_FROM_CTRL_BIT = 19, > MLF_UNSNAT_NEW_BIT = 20, > MLF_UNSNAT_NOT_TRACKED_BIT = 21, > + MLF_NETWORK_ID_START_BIT = 28, > + MLF_NETWORK_ID_END_BIT = 31, > }; > > /* MFF_LOG_FLAGS_REG flag assignments */ > @@ -159,7 +165,11 @@ enum mff_log_flags { > MLF_UNSNAT_NEW = (1 << MLF_UNSNAT_NEW_BIT), > > /* Indicate that the packet didn't go through unSNAT. */ > - MLF_UNSNAT_NOT_TRACKED = (1 << MLF_UNSNAT_NOT_TRACKED_BIT) > + MLF_UNSNAT_NOT_TRACKED = (1 << MLF_UNSNAT_NOT_TRACKED_BIT), > + > + /* Assign network ID to packet to choose correct network for snat when > + * lb_force_snat_ip=router_ip. */ > + MLF_NETWORK_ID = (OVN_MAX_NETWORK_ID << MLF_NETWORK_ID_START_BIT), > }; > > /* OVN logical fields > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > index ed287f42b..db2d08ada 100644 > --- a/lib/logical-fields.c > +++ b/lib/logical-fields.c > @@ -147,6 +147,9 @@ ovn_init_symtab(struct shash *symtab) > MLF_UNSNAT_NOT_TRACKED_BIT); > expr_symtab_add_subfield(symtab, "flags.unsnat_not_tracked", NULL, > flags_str); > + snprintf(flags_str, sizeof flags_str, "flags[%d..%d]", > + MLF_NETWORK_ID_START_BIT, MLF_NETWORK_ID_END_BIT); > + expr_symtab_add_subfield(symtab, "flags.network_id", NULL, flags_str); > > snprintf(flags_str, sizeof flags_str, "flags[%d]", MLF_FROM_CTRL_BIT); > expr_symtab_add_subfield(symtab, "flags.from_ctrl", NULL, flags_str); > diff --git a/northd/northd.c b/northd/northd.c > index 1d3e132d4..c94f83e32 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -12985,73 +12985,118 @@ build_lrouter_force_snat_flows_op(struct ovn_port > *op, > struct ds *match, struct ds *actions, > struct lflow_ref *lflow_ref) > { > + size_t network_id; > ovs_assert(op->nbrp); > if (!op->peer || !lrnat_rec->lb_force_snat_router_ip) { > return; > } > > - if (op->lrp_networks.n_ipv4_addrs) { > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > ds_clear(match); > ds_clear(actions); > > ds_put_format(match, "inport == %s && ip4.dst == %s", > - op->json_key, op->lrp_networks.ipv4_addrs[0].addr_s); > + op->json_key, op->lrp_networks.ipv4_addrs[i].addr_s); > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_UNSNAT, 110, > ds_cstr(match), "ct_snat;", lflow_ref); > > ds_clear(match); > > + /* Since flags.network_id is 4 bits, assign flags.network_id = 0 for > + * networks 17 and up. */ > + if (i > OVN_MAX_NETWORK_ID) { > + network_id = 0; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_WARN_RL(&rl, "Logical router port %s already has the max of > " > + "%d networks configured, so for network " > + "\"%s/%d\" the first IP [%s] will be > considered " > + "as SNAT for load balancer.", op->json_key, > + OVN_MAX_NETWORK_ID + 1, > + op->lrp_networks.ipv4_addrs[i].addr_s, > + op->lrp_networks.ipv4_addrs[i].plen, > + op->lrp_networks.ipv4_addrs[0].addr_s); > + } else { > + network_id = i; > + } > + > /* Higher priority rules to force SNAT with the router port ip. > * This only takes effect when the packet has already been > * load balanced once. */ > - ds_put_format(match, "flags.force_snat_for_lb == 1 && ip4 && " > - "outport == %s", op->json_key); > + ds_put_format(match, "flags.force_snat_for_lb == 1 && " > + "flags.network_id == %"PRIuSIZE" && ip4 && " > + "outport == %s", network_id, op->json_key); > ds_put_format(actions, "ct_snat(%s);", > - op->lrp_networks.ipv4_addrs[0].addr_s); > + op->lrp_networks.ipv4_addrs[network_id].addr_s); > ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110, > ds_cstr(match), ds_cstr(actions), > lflow_ref); > - if (op->lrp_networks.n_ipv4_addrs > 1) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - VLOG_WARN_RL(&rl, "Logical router port %s is configured with " > - "multiple IPv4 addresses. Only the first " > - "IP [%s] is considered as SNAT for load " > - "balancer", op->json_key, > - op->lrp_networks.ipv4_addrs[0].addr_s); > - } > } > > /* op->lrp_networks.ipv6_addrs will always have LLA and that will be > - * last in the list. So add the flows only if n_ipv6_addrs > 1. */ > - if (op->lrp_networks.n_ipv6_addrs > 1) { > + * last in the list. So loop to add flows n_ipv6_addrs - 1 times. */ > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs - 1; i++) { > ds_clear(match); > ds_clear(actions); > > ds_put_format(match, "inport == %s && ip6.dst == %s", > - op->json_key, op->lrp_networks.ipv6_addrs[0].addr_s); > + op->json_key, op->lrp_networks.ipv6_addrs[i].addr_s); > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_UNSNAT, 110, > ds_cstr(match), "ct_snat;", lflow_ref); > - > ds_clear(match); > > + /* Since flags.network_id is 4 bits, assign flags.network_id = 0 for > + * networks 17 and up. */ > + if (i > OVN_MAX_NETWORK_ID) { > + network_id = 0; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_WARN_RL(&rl, "Logical router port %s already has the max of > " > + "%d networks configured, so for network " > + "\"%s/%d\" the first IP [%s] will be > considered " > + "as SNAT for load balancer.", op->json_key, > + OVN_MAX_NETWORK_ID + 1, > + op->lrp_networks.ipv6_addrs[i].addr_s, > + op->lrp_networks.ipv6_addrs[i].plen, > + op->lrp_networks.ipv6_addrs[0].addr_s); > + } else { > + network_id = i; > + } > + > /* Higher priority rules to force SNAT with the router port ip. > * This only takes effect when the packet has already been > * load balanced once. */ > - ds_put_format(match, "flags.force_snat_for_lb == 1 && ip6 && " > - "outport == %s", op->json_key); > + ds_put_format(match, "flags.force_snat_for_lb == 1 && " > + "flags.network_id == %"PRIuSIZE" && ip6 && " > + "outport == %s", network_id, op->json_key); > ds_put_format(actions, "ct_snat(%s);", > - op->lrp_networks.ipv6_addrs[0].addr_s); > + op->lrp_networks.ipv6_addrs[network_id].addr_s); > ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110, > ds_cstr(match), ds_cstr(actions), > lflow_ref); > - if (op->lrp_networks.n_ipv6_addrs > 2) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - VLOG_WARN_RL(&rl, "Logical router port %s is configured with " > - "multiple IPv6 addresses. Only the first " > - "IP [%s] is considered as SNAT for load " > - "balancer", op->json_key, > - op->lrp_networks.ipv6_addrs[0].addr_s); > - } > + } > + > + /* This lower-priority flow matches the old behavior for if northd is > + * upgraded before controller and flags.network_id is not recognized. */ > + if (op->lrp_networks.n_ipv4_addrs > 0) { > + ds_clear(match); > + ds_clear(actions); > + ds_put_format(match, "flags.force_snat_for_lb == 1 && ip4 && " > + "outport == %s", op->json_key); > + ds_put_format(actions, "ct_snat(%s);", > + op->lrp_networks.ipv4_addrs[0].addr_s); > + ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 105, > + ds_cstr(match), ds_cstr(actions), lflow_ref); > + } Nit: I'd add an empty line here. > + /* op->lrp_networks.ipv6_addrs will always have LLA, so only add flow if > + * there is more than 1. */ > + if (op->lrp_networks.n_ipv6_addrs > 1) { > + ds_clear(match); > + ds_clear(actions); > + ds_put_format(match, "flags.force_snat_for_lb == 1 && ip6 && " > + "outport == %s", op->json_key); > + ds_put_format(actions, "ct_snat(%s);", > + op->lrp_networks.ipv6_addrs[0].addr_s); > + ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 105, > + ds_cstr(match), ds_cstr(actions), lflow_ref); > } > } > > @@ -14930,6 +14975,87 @@ build_arp_request_flows_for_lrouter( > lflow_ref); > } > > +static void > +build_lrouter_network_id_flows( > + struct ovn_datapath *od, struct lflow_table *lflows, > + struct ds *match, struct ds *actions, struct lflow_ref > *lflow_ref) Nit: most other functions around this one that have multi-line argument lists use 8 spaces as indent. > +{ > + const struct ovn_port *op; > + size_t network_id; > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + /* Since flags.network_id is 4 bits and can store 16 network IDs, > + * assign a value of 0 for networks 17 and up. */ > + if (i > OVN_MAX_NETWORK_ID) { > + network_id = 0; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > + VLOG_WARN_RL(&rl, "Logical router port %s already has the > max " > + "of %d networks configured, so network " > + "\"%s/%d\" is assigned " > + "flags.network_id = 0.", op->json_key, > + OVN_MAX_NETWORK_ID + 1, > + op->lrp_networks.ipv4_addrs[i].addr_s, > + op->lrp_networks.ipv4_addrs[i].plen); > + } else { > + network_id = i; > + } > + > + ds_clear(match); > + ds_clear(actions); > + > + ds_put_format(match, "outport == %s && " > + REG_NEXT_HOP_IPV4 " == %s/%d", op->json_key, > + op->lrp_networks.ipv4_addrs[i].addr_s, > + op->lrp_networks.ipv4_addrs[i].plen); > + > + ds_put_format(actions, "flags.network_id = %"PRIuSIZE"; ", > + network_id); > + ds_put_format(actions, "next;"); > + > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_NETWORK_ID, 110, > + ds_cstr(match), ds_cstr(actions), > + lflow_ref); > + } > + > + /* op->lrp_networks.ipv6_addrs will always have LLA and that will be > + * last in the list. So add the flows only if n_ipv6_addrs > 1, and > + * loop n_ipv6_addrs - 1 times. */ > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs - 1; i++) { > + /* Since flags.network_id is 4 bits and can store 16 network IDs, > + * assign a value of 0 for networks 17 and up. */ > + if (i > OVN_MAX_NETWORK_ID) { > + network_id = 0; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > + VLOG_WARN_RL(&rl, "Logical router port %s already has the > max " > + "of %d networks configured, so network " > + "\"%s/%d\" is assigned " > + "flags.network_id = 0.", op->json_key, > + OVN_MAX_NETWORK_ID + 1, > + op->lrp_networks.ipv6_addrs[i].addr_s, > + op->lrp_networks.ipv6_addrs[i].plen); > + } else { > + network_id = i; > + } > + > + ds_clear(match); > + ds_clear(actions); > + > + ds_put_format(match, "outport == %s && " > + REG_NEXT_HOP_IPV6 " == %s/%d", op->json_key, > + op->lrp_networks.ipv6_addrs[i].addr_s, > + op->lrp_networks.ipv6_addrs[i].plen); > + > + ds_put_format(actions, "flags.network_id = %"PRIuSIZE"; ", i); > + ds_put_format(actions, "next;"); > + > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_NETWORK_ID, 110, > + ds_cstr(match), ds_cstr(actions), lflow_ref); > + } > + } > + ovn_lflow_add(lflows, od, S_ROUTER_IN_NETWORK_ID, 0, > + "1", "next;", lflow_ref); > +} > + > /* Logical router egress table DELIVERY: Delivery (priority 100-110). > * > * Priority 100 rules deliver packets to enabled logical ports. > @@ -17328,6 +17454,8 @@ build_lswitch_and_lrouter_iterate_by_lr(struct > ovn_datapath *od, > &lsi->actions, > lsi->meter_groups, > NULL); > + build_lrouter_network_id_flows(od, lsi->lflows, &lsi->match, > + &lsi->actions, NULL); Nit: indentation. > build_misc_local_traffic_drop_flows_for_lrouter(od, lsi->lflows, NULL); > > build_lr_nat_defrag_and_lb_default_flows(od, lsi->lflows, NULL); > diff --git a/northd/northd.h b/northd/northd.h > index 1a7afe902..388bac6df 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -547,7 +547,8 @@ enum ovn_stage { > PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 22, "lr_in_chk_pkt_len") > \ > PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 23, "lr_in_larger_pkts") > \ > PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 24, "lr_in_gw_redirect") > \ > - PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 25, "lr_in_arp_request") > \ > + PIPELINE_STAGE(ROUTER, IN, NETWORK_ID, 25, "lr_in_network_id") > \ > + PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 26, "lr_in_arp_request") > \ > \ > /* Logical router egress stages. */ \ > PIPELINE_STAGE(ROUTER, OUT, CHECK_DNAT_LOCAL, 0, > \ > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 155ba8a49..5dd0a004b 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -5066,7 +5066,44 @@ icmp6 { > </li> > </ul> > > - <h3>Ingress Table 25: ARP Request</h3> > + <h3>Ingress Table 25: Network ID</h3> > + > + <p> > + This table generates flows that set flags.network_id. It holds the > + following flows: > + </p> > + > + <ul> > + <li> > + <p> > + A priority-110 flow for IPv4 packets with match <code>outport == > + <var>P</var> && REG_NEXT_HOP_IPV4 == > + <var>I</var>/<var>C</var></code>, and actions > <code>flags.network_id > + = <var>N</var>; next;</code>. > + </p> > + > + <p> > + Where <var>P</var> is the outport, <var>I</var> is the next-hop IP, > + <var>C</var> is the next-hop network CIDR, and <var>N</var> is the > + network id (index). > + </p> > + > + <p> > + <code>flags.network_id</code> is 4 bits, and thus only 16 networks > can > + be indexed. If the number of networks is greater than 16, networks 17 > + and up will have the actions <code>flags.network_id = 0; next;</code> > + and only the first lexicographical IP will be considered for SNAT for > + those networks. > + </p> > + </li> > + > + <li> > + Catch-all: A priority-0 flow with match <code>1</code> has > + actions <code>next;</code>. > + </li> > + </ul> > + > + <h3>Ingress Table 26: ARP Request</h3> > > <p> > In the common case where the Ethernet destination has been resolved, > this > @@ -5330,18 +5367,22 @@ nd_ns { > table="Logical_Router"/>:lb_force_snat_ip=router_ip), then for > each logical router port <var>P</var> attached to the Gateway > router, a priority-110 flow matches > - <code>flags.force_snat_for_lb == 1 && outport == > <var>P</var> > - </code> with an action <code>ct_snat(<var>R</var>);</code> > - where <var>R</var> is the IP configured on the router port. > - If <code>R</code> is an IPv4 address then the match will also > - include <code>ip4</code> and if it is an IPv6 address, then the > - match will also include <code>ip6</code>. > + <code>flags.force_snat_for_lb == 1 && flags.network_id == > + <var>N</var> && outport == <var>P</var></code>, where > + <var>N</var> is the network index, with an action > + <code>ct_snat(<var>R</var>);</code> where <var>R</var> is the IP > + configured on the router port. If <code>R</code> is an IPv4 address > + then the match will also include <code>ip4</code> and if it is an > + IPv6 address, then the match will also include <code>ip6</code>. > + <var>N</var>, the network index, will be 0 for networks 17 and up. > </p> > > <p> > If the logical router port <var>P</var> is configured with multiple > - IPv4 and multiple IPv6 addresses, only the first IPv4 and first > IPv6 > - address is considered. > + IPv4 and multiple IPv6 addresses, the IPv4 and IPv6 address within > + the same network as the next-hop will be chosen. However, if there > + are more than 16 networks configured, the first lexicographical IP > + will be considered for SNAT for networks 17 and up. > </p> > </li> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index cfaba19bf..6b981fd89 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -4493,9 +4493,12 @@ AT_CHECK([grep "lr_in_dnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.100);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), > action=(ct_snat(20.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.100);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), > action=(ct_snat(20.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-public"), action=(ct_snat(172.168.0.100);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw1"), action=(ct_snat(20.0.0.1);) > table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > ]) > > @@ -4558,10 +4561,14 @@ AT_CHECK([grep "lr_in_dnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.100);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), > action=(ct_snat(20.0.0.1);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw1"), > action=(ct_snat(bef0::1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.100);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"), > action=(ct_snat(20.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw1"), > action=(ct_snat(bef0::1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-public"), action=(ct_snat(172.168.0.100);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw1"), action=(ct_snat(20.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 && > outport == "lr0-sw1"), action=(ct_snat(bef0::1);) > table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > ]) > > @@ -6268,8 +6275,10 @@ AT_CHECK([grep "lr_out_post_undnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-public"), action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);) > table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > table=??(lr_out_snat ), priority=25 , match=(ip && ip4.src == > 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);) > table=??(lr_out_snat ), priority=33 , match=(ip && ip4.src == > 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);) > @@ -6334,8 +6343,10 @@ AT_CHECK([grep "lr_out_post_undnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-public"), action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);) > table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > table=??(lr_out_snat ), priority=25 , match=(ip && ip4.src == > 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);) > table=??(lr_out_snat ), priority=33 , match=(ip && ip4.src == > 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);) > @@ -6412,10 +6423,14 @@ AT_CHECK([grep "lr_out_post_undnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public"), > action=(ct_snat(def0::10);) > - table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw0"), > action=(ct_snat(aef0::1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), > action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), > action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public"), > action=(ct_snat(def0::10);) > + table=??(lr_out_snat ), priority=105 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw0"), > action=(ct_snat(aef0::1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-public"), action=(ct_snat(172.168.0.10);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 && > outport == "lr0-public"), action=(ct_snat(def0::10);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 && > outport == "lr0-sw0"), action=(ct_snat(aef0::1);) > table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > table=??(lr_out_snat ), priority=25 , match=(ip && ip4.src == > 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);) > table=??(lr_out_snat ), priority=33 , match=(ip && ip4.src == > 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);) > @@ -16748,3 +16763,104 @@ check grep -q "Bad configuration: The peer of the > switch port 'ls-lrp1' (LRP pee > > AT_CLEANUP > ]) > + > +AT_SETUP([lb_force_snat_ip=router_ip generate flags.network_id flows]) This should be wrapped in OVN_FOR_EACH_NORTHD_NO_HV. > +ovn_start > + > +check ovn-nbctl lr-add lr > +check ovn-nbctl set logical_router lr options:chassis=hv1 > +check ovn-nbctl set logical_router lr options:lb_force_snat_ip=router_ip > +check ovn-nbctl lrp-add lr lrp-client 02:00:00:00:00:02 1.1.1.1/24 ff01::01 > +check ovn-nbctl lrp-add lr lrp-server 02:00:00:00:00:03 1.1.2.1/24 > 7.7.7.1/24 \ > + 8.8.8.1/24 1.2.1.1/24 1.2.2.1/24 3.3.3.1/24 4.4.4.1/24 > 5.5.5.1/24 \ > + 6.6.6.1/24 6.2.1.1/24 6.3.1.1/24 6.4.1.1/24 6.5.1.1/24 > 6.6.1.1/24 \ > + 6.7.1.1/24 6.8.1.1/24 6.9.1.1/24 > 7.2.1.1/24 \ > + ff01::02 ff01::03 ff01::06 > +check ovn-nbctl ls-add ls-client > +check ovn-nbctl ls-add ls-server > +check ovn-nbctl lsp-add ls-client lsp-client-router > +check ovn-nbctl lsp-set-type lsp-client-router router > +check ovn-nbctl lsp-add ls-server lsp-server-router > +check ovn-nbctl lsp-set-type lsp-server-router router > +check ovn-nbctl set logical_switch_port lsp-client-router > options:router-port=lrp-client > +check ovn-nbctl set logical_switch_port lsp-server-router > options:router-port=lrp-server > +check ovn-nbctl lsp-add ls-client client > +check ovn-nbctl lsp-add ls-server server > +check ovn-nbctl lsp-set-addresses client "02:00:00:00:00:01 1.1.1.10" > +check ovn-nbctl lsp-set-addresses server "02:00:00:00:00:04 2.2.2.10" > +Andover, Massachusettscheck ovn-nbctl lsp-set-addresses lsp-client-router > router Hmmm, Andover, Massachusetts. :) > +check ovn-nbctl lsp-set-addresses lsp-server-router router > +check ovn-nbctl lb-add lb 42.42.42.42:80 2.2.2.10:80 udp > +check ovn-nbctl lr-lb-add lr lb > +check ovn-nbctl --wait=hv sync Nit: this can be --wait=sb. > + > +ovn-sbctl dump-flows lr > lrflows > +AT_CAPTURE_FILE([lrflows]) > + > +AT_CHECK([grep -E flags.network_id lrflows | ovn_strip_lflows], [0], [dnl > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-client" && reg0 == 1.1.1.1/24), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-client" && xxreg0 == ff01::1/128), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 1.1.2.1/24), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 1.2.1.1/24), action=(flags.network_id = 1; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 1.2.2.1/24), action=(flags.network_id = 2; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 3.3.3.1/24), action=(flags.network_id = 3; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 4.4.4.1/24), action=(flags.network_id = 4; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 5.5.5.1/24), action=(flags.network_id = 5; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.2.1.1/24), action=(flags.network_id = 6; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.3.1.1/24), action=(flags.network_id = 7; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.4.1.1/24), action=(flags.network_id = 8; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.5.1.1/24), action=(flags.network_id = 9; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.6.1.1/24), action=(flags.network_id = 10; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.6.6.1/24), action=(flags.network_id = 11; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.7.1.1/24), action=(flags.network_id = 12; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.8.1.1/24), action=(flags.network_id = 13; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 6.9.1.1/24), action=(flags.network_id = 14; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 7.2.1.1/24), action=(flags.network_id = 15; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 7.7.7.1/24), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && reg0 == 8.8.8.1/24), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && xxreg0 == ff01::2/128), action=(flags.network_id = 0; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && xxreg0 == ff01::3/128), action=(flags.network_id = 1; next;) > + table=??(lr_in_network_id ), priority=110 , match=(outport == > "lrp-server" && xxreg0 == ff01::6/128), action=(flags.network_id = 2; next;) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lrp-client"), action=(ct_snat(1.1.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 && > outport == "lrp-server"), action=(ct_snat(1.1.2.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 && > outport == "lrp-client"), action=(ct_snat(ff01::1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 && > outport == "lrp-server"), action=(ct_snat(ff01::2);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 1 && ip4 && > outport == "lrp-server"), action=(ct_snat(1.2.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 1 && ip6 && > outport == "lrp-server"), action=(ct_snat(ff01::3);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 10 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.6.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 11 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.6.6.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 12 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.7.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 13 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.8.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 14 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.9.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 15 && ip4 && > outport == "lrp-server"), action=(ct_snat(7.2.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 2 && ip4 && > outport == "lrp-server"), action=(ct_snat(1.2.2.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 2 && ip6 && > outport == "lrp-server"), action=(ct_snat(ff01::6);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 3 && ip4 && > outport == "lrp-server"), action=(ct_snat(3.3.3.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 4 && ip4 && > outport == "lrp-server"), action=(ct_snat(4.4.4.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 5 && ip4 && > outport == "lrp-server"), action=(ct_snat(5.5.5.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 6 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.2.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 7 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.3.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 8 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.4.1.1);) > + table=??(lr_out_snat ), priority=110 , > match=(flags.force_snat_for_lb == 1 && flags.network_id == 9 && ip4 && > outport == "lrp-server"), action=(ct_snat(6.5.1.1);) > +]) > +AT_CLEANUP > + > +AT_SETUP([lb_force_snat_ip=router_ip one IPv6 address]) This should be wrapped in OVN_FOR_EACH_NORTHD_NO_HV. > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl set logical_router lr0 options:chassis=hv1 > +check ovn-nbctl set logical_router lr0 options:lb_force_snat_ip=router_ip > +check ovn-nbctl lrp-add lr0 lrp0 02:00:00:00:00:01 > +check ovn-nbctl lsp-add sw0 lrp0-attachment > +check ovn-nbctl lsp-set-type lrp0-attachment router > +check ovn-nbctl lsp-set-addresses lrp0-attachment 02:00:00:00:00:01 > +check ovn-nbctl lsp-set-options lrp0-attachment router-port=lrp0 > +check ovn-nbctl --wait=hv sync Nit: this could be just --wait=sb. > + > +ovn-sbctl dump-flows lr0 > lrflows > +AT_CAPTURE_FILE([lrflows]) > +AT_CHECK([grep -E "flags.force_snat_for_lb == 1" lrflows | grep priority=105 > | ovn_strip_lflows], [0], [dnl "-E" is extended regex, we don't use any here. I would change this to: AT_CHECK([! ovn_strip_lflows < lrflows | grep priority=105 | grep -q "flags.force_snat_for_lb == 1"]) > +]) For completeness I think we should check what happens when we add an explicit IPv6 address. Something like: check ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 02:00:00:00:00:01 4242::4242/64 check ovn-nbctl --wait=sb sync ovn-sbctl dump-flows lr0 > lrflows AT_CAPTURE_FILE([lrflows]) AT_CHECK([ovn_strip_lflows < lrflows | grep priority=105 | grep -c "flags.force_snat_for_lb == 1"], [0], [dnl 1 ]) > +AT_CLEANUP > diff --git a/tests/ovn.at b/tests/ovn.at > index d8c06cd73..4156cc376 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -42163,3 +42163,74 @@ wait_row_count ACL_ID 0 > > AT_CLEANUP > ]) > + > +AT_SETUP([lb_force_snat_ip=router_ip select correct network for snat]) This should be wrapped in OVN_FOR_EACH_NORTHD. > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > +ovn_start > + > +check ovn-nbctl lr-add lr > +check ovn-nbctl set logical_router lr options:chassis=hv1 > +check ovn-nbctl set logical_router lr options:lb_force_snat_ip=router_ip > +check ovn-nbctl lrp-add lr lrp-client 02:00:00:00:00:02 1.1.1.1/24 > +check ovn-nbctl lrp-add lr lrp-server 02:00:00:00:00:03 1.1.2.1/24 > 1.2.1.1/24 \ > + 1.1.3.1/24 2.2.2.1/24 7.7.7.1/24 8.8.8.1/24 6.7.1.1/24 > 6.8.1.1/24 \ > + 8.8.8.1/24 1.2.1.1/24 1.2.2.1/24 3.3.3.1/24 4.4.4.1/24 > 5.5.5.1/24 \ > + 6.6.6.1/24 6.2.1.1/24 6.3.1.1/24 6.4.1.1/24 6.5.1.1/24 6.6.1.1/24 This adds 20 networks, out of which 2 are duplicated (8.8.8.1/24 and 1.2.1.1/24), so we could add a check that a warning is generated for the last 2. > +check ovn-nbctl ls-add ls-client > +check ovn-nbctl ls-add ls-server > +check ovn-nbctl lsp-add ls-client lsp-client-router > +check ovn-nbctl lsp-set-type lsp-client-router router > +check ovn-nbctl lsp-add ls-server lsp-server-router > +check ovn-nbctl lsp-set-type lsp-server-router router > +check ovn-nbctl set logical_switch_port lsp-client-router > options:router-port=lrp-client > +check ovn-nbctl set logical_switch_port lsp-server-router > options:router-port=lrp-server > +check ovn-nbctl lsp-add ls-client client > +check ovn-nbctl lsp-add ls-server server > +check ovn-nbctl lsp-set-addresses client "02:00:00:00:00:01 1.1.1.10" > +check ovn-nbctl lsp-set-addresses server "02:00:00:00:00:04 2.2.2.10" > +check ovn-nbctl lsp-set-addresses lsp-client-router router > +check ovn-nbctl lsp-set-addresses lsp-server-router router > +check ovn-nbctl lb-add lb 42.42.42.42:80 2.2.2.10:80 udp > +check ovn-nbctl lr-lb-add lr lb > + > +# Create a hypervisor and create OVS ports corresponding to logical ports. > +net_add n1 > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=client \ > + options:tx_pcap=hv1/client-tx.pcap \ > + options:rxq_pcap=hv1/client-rx.pcap > + > +check ovs-vsctl -- add-port br-int hv1-vif2 -- \ > + set interface hv1-vif2 external-ids:iface-id=server \ > + options:tx_pcap=hv1/server-tx.pcap \ > + options:rxq_pcap=hv1/server-rx.pcap > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +tx_src_mac="02:00:00:00:00:01" > +tx_dst_mac="02:00:00:00:00:02" > +tx_src_ip=1.1.1.10 > +tx_dst_ip=42.42.42.42 > +request=$(fmt_pkt "Ether(dst='${tx_dst_mac}', src='${tx_src_mac}')/ \ > + IP(src='${tx_src_ip}', dst='${tx_dst_ip}')/ \ > + UDP(sport=20001, dport=80)") Nit: I'd indent these under Ether. > + > +check as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $request > + > +rx_src_mac="02:00:00:00:00:03" > +rx_dst_mac="02:00:00:00:00:04" > +rx_src_ip=2.2.2.1 > +rx_dst_ip=2.2.2.10 > +expected=$(fmt_pkt "Ether(dst='${rx_dst_mac}', src='${rx_src_mac}')/ \ > + IP(src='${rx_src_ip}', dst='${rx_dst_ip}', ttl=0x3F)/ \ > + UDP(sport=20001, dport=80)") Nit: I'd indent these under Ether. > + > +echo $expected > expected > +OVN_CHECK_PACKETS([hv1/server-tx.pcap], [expected]) > +AT_CLEANUP I could squash in the following incremental diff that takes care of the review comments before applying the patch to main. I can also take care of updating the commit message as you suggested in your other reply. What do you think? diff --git a/lib/ovn-util.c b/lib/ovn-util.c index c65b36bb5f..032b16ff38 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -897,7 +897,7 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address, * * This value is also used to handle some backward compatibility during * upgrading. It should never decrease or rewind. */ -#define OVN_INTERNAL_MINOR_VER 6 +#define OVN_INTERNAL_MINOR_VER 7 /* Returns the OVN version. The caller must free the returned value. */ char * diff --git a/northd/northd.c b/northd/northd.c index c94f83e328..cd35204b90 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -13086,6 +13086,7 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op, ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 105, ds_cstr(match), ds_cstr(actions), lflow_ref); } + /* op->lrp_networks.ipv6_addrs will always have LLA, so only add flow if * there is more than 1. */ if (op->lrp_networks.n_ipv6_addrs > 1) { @@ -14977,8 +14978,8 @@ build_arp_request_flows_for_lrouter( static void build_lrouter_network_id_flows( - struct ovn_datapath *od, struct lflow_table *lflows, - struct ds *match, struct ds *actions, struct lflow_ref *lflow_ref) + struct ovn_datapath *od, struct lflow_table *lflows, + struct ds *match, struct ds *actions, struct lflow_ref *lflow_ref) { const struct ovn_port *op; size_t network_id; @@ -17455,7 +17456,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od, lsi->meter_groups, NULL); build_lrouter_network_id_flows(od, lsi->lflows, &lsi->match, - &lsi->actions, NULL); + &lsi->actions, NULL); build_misc_local_traffic_drop_flows_for_lrouter(od, lsi->lflows, NULL); build_lr_nat_defrag_and_lb_default_flows(od, lsi->lflows, NULL); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6b981fd890..2e11c2922f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -16764,6 +16764,7 @@ check grep -q "Bad configuration: The peer of the switch port 'ls-lrp1' (LRP pee AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([lb_force_snat_ip=router_ip generate flags.network_id flows]) ovn_start @@ -16788,11 +16789,11 @@ check ovn-nbctl lsp-add ls-client client check ovn-nbctl lsp-add ls-server server check ovn-nbctl lsp-set-addresses client "02:00:00:00:00:01 1.1.1.10" check ovn-nbctl lsp-set-addresses server "02:00:00:00:00:04 2.2.2.10" -Andover, Massachusettscheck ovn-nbctl lsp-set-addresses lsp-client-router router +check ovn-nbctl lsp-set-addresses lsp-client-router router check ovn-nbctl lsp-set-addresses lsp-server-router router check ovn-nbctl lb-add lb 42.42.42.42:80 2.2.2.10:80 udp check ovn-nbctl lr-lb-add lr lb -check ovn-nbctl --wait=hv sync +check ovn-nbctl --wait=sb sync ovn-sbctl dump-flows lr > lrflows AT_CAPTURE_FILE([lrflows]) @@ -16844,7 +16845,9 @@ AT_CHECK([grep -E flags.network_id lrflows | ovn_strip_lflows], [0], [dnl table=??(lr_out_snat ), priority=110 , match=(flags.force_snat_for_lb == 1 && flags.network_id == 9 && ip4 && outport == "lrp-server"), action=(ct_snat(6.5.1.1);) ]) AT_CLEANUP +]) +OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([lb_force_snat_ip=router_ip one IPv6 address]) ovn_start @@ -16857,10 +16860,19 @@ check ovn-nbctl lsp-add sw0 lrp0-attachment check ovn-nbctl lsp-set-type lrp0-attachment router check ovn-nbctl lsp-set-addresses lrp0-attachment 02:00:00:00:00:01 check ovn-nbctl lsp-set-options lrp0-attachment router-port=lrp0 -check ovn-nbctl --wait=hv sync +check ovn-nbctl --wait=sb sync ovn-sbctl dump-flows lr0 > lrflows AT_CAPTURE_FILE([lrflows]) -AT_CHECK([grep -E "flags.force_snat_for_lb == 1" lrflows | grep priority=105 | ovn_strip_lflows], [0], [dnl +AT_CHECK([! ovn_strip_lflows < lrflows | grep priority=105 | grep -q "flags.force_snat_for_lb == 1"]) + +check ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 02:00:00:00:00:01 4242::4242/64 +check ovn-nbctl --wait=sb sync +ovn-sbctl dump-flows lr0 > lrflows +AT_CAPTURE_FILE([lrflows]) +AT_CHECK([ovn_strip_lflows < lrflows | grep priority=105 | grep -c "flags.force_snat_for_lb == 1"], [0], [dnl +1 ]) + AT_CLEANUP +]) diff --git a/tests/ovn.at b/tests/ovn.at index 4156cc3767..f1600a2393 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -42164,6 +42164,7 @@ wait_row_count ACL_ID 0 AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ AT_SETUP([lb_force_snat_ip=router_ip select correct network for snat]) AT_SKIP_IF([test $HAVE_SCAPY = no]) ovn_start @@ -42174,8 +42175,8 @@ check ovn-nbctl set logical_router lr options:lb_force_snat_ip=router_ip check ovn-nbctl lrp-add lr lrp-client 02:00:00:00:00:02 1.1.1.1/24 check ovn-nbctl lrp-add lr lrp-server 02:00:00:00:00:03 1.1.2.1/24 1.2.1.1/24 \ 1.1.3.1/24 2.2.2.1/24 7.7.7.1/24 8.8.8.1/24 6.7.1.1/24 6.8.1.1/24 \ - 8.8.8.1/24 1.2.1.1/24 1.2.2.1/24 3.3.3.1/24 4.4.4.1/24 5.5.5.1/24 \ - 6.6.6.1/24 6.2.1.1/24 6.3.1.1/24 6.4.1.1/24 6.5.1.1/24 6.6.1.1/24 + 1.2.2.1/24 3.3.3.1/24 4.4.4.1/24 5.5.5.1/24 6.6.6.1/24 6.2.1.1/24 \ + 6.3.1.1/24 6.4.1.1/24 6.5.1.1/24 6.6.1.1/24 check ovn-nbctl ls-add ls-client check ovn-nbctl ls-add ls-server check ovn-nbctl lsp-add ls-client lsp-client-router @@ -42213,13 +42214,17 @@ check ovs-vsctl -- add-port br-int hv1-vif2 -- \ wait_for_ports_up check ovn-nbctl --wait=hv sync +check grep -q \ + 'Logical router port "lrp-server" already has the max of 16 networks configured' \ + northd/ovn-northd.log + tx_src_mac="02:00:00:00:00:01" tx_dst_mac="02:00:00:00:00:02" tx_src_ip=1.1.1.10 tx_dst_ip=42.42.42.42 request=$(fmt_pkt "Ether(dst='${tx_dst_mac}', src='${tx_src_mac}')/ \ - IP(src='${tx_src_ip}', dst='${tx_dst_ip}')/ \ - UDP(sport=20001, dport=80)") + IP(src='${tx_src_ip}', dst='${tx_dst_ip}')/ \ + UDP(sport=20001, dport=80)") check as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $request @@ -42228,9 +42233,10 @@ rx_dst_mac="02:00:00:00:00:04" rx_src_ip=2.2.2.1 rx_dst_ip=2.2.2.10 expected=$(fmt_pkt "Ether(dst='${rx_dst_mac}', src='${rx_src_mac}')/ \ - IP(src='${rx_src_ip}', dst='${rx_dst_ip}', ttl=0x3F)/ \ - UDP(sport=20001, dport=80)") + IP(src='${rx_src_ip}', dst='${rx_dst_ip}', ttl=0x3F)/ \ + UDP(sport=20001, dport=80)") echo $expected > expected OVN_CHECK_PACKETS([hv1/server-tx.pcap], [expected]) AT_CLEANUP +]) -- Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
