On 3/10/25 9:14 PM, Rosemarie O'Riorden wrote:
>
> 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.
[...]
> diff --git a/northd/northd.c b/northd/northd.c
> index 1d3e132d4..893116204 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -12985,74 +12985,114 @@ 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.ipv4_addrs[i].addr_s,
> + op->lrp_networks.ipv4_addrs[i].plen,
> + op->lrp_networks.ipv4_addrs[0].addr_s);
This should be "op->lrp_networks.ipv6_addrs" in the three lines above.
> + } 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. */
> + 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);
This needs to be done only if the router port has at least one IPv4
address.
> +
> + 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);
Same here, for IPv6. This is why the ovn-kubernetes CI is crashing for
IPv6 jobs, e.g.:
https://github.com/dceara/ovn/actions/runs/13785224928
2025-03-11T10:23:27.327873036Z stdout F SIGSEGV detected, backtrace:
2025-03-11T10:23:27.327882193Z stdout F ovn-northd(+0x7e5e7) [0x56521f0755e7]
2025-03-11T10:23:27.32789124Z stdout F /lib64/libc.so.6(+0x19dc0)
[0x7f024358fdc0]
2025-03-11T10:23:27.327894145Z stdout F /lib64/libc.so.6(+0x149edd)
[0x7f02436bfedd]
2025-03-11T10:23:27.32789674Z stdout F /lib64/libc.so.6(+0x406d8)
[0x7f02435b66d8]
2025-03-11T10:23:27.327898884Z stdout F /lib64/libc.so.6(+0x64c33)
[0x7f02435dac33]
2025-03-11T10:23:27.327913892Z stdout F ovn-northd(+0x7fa33) [0x56521f076a33]
2025-03-11T10:23:27.327916677Z stdout F ovn-northd(+0x7fbba) [0x56521f076bba]
2025-03-11T10:23:27.327919483Z stdout F ovn-northd(+0x22fb8) [0x56521f019fb8]
--->> build_lbnat_lflows_iterate_by_lrp()
2025-03-11T10:23:27.327922017Z stdout F ovn-northd(+0x440fb) [0x56521f03b0fb]
2025-03-11T10:23:27.327924672Z stdout F ovn-northd(+0x7a26f) [0x56521f07126f]
2025-03-11T10:23:27.327927417Z stdout F ovn-northd(+0x6899e) [0x56521f05f99e]
2025-03-11T10:23:27.327929762Z stdout F ovn-northd(+0x6023) [0x56521effd023]
2025-03-11T10:23:27.328024811Z stdout F /lib64/libc.so.6(+0x3248)
[0x7f0243579248]
2025-03-11T10:23:27.328029911Z stdout F
/lib64/libc.so.6(__libc_start_main+0x8b) [0x7f024357930b]
2025-03-11T10:23:27.328032546Z stdout F ovn-northd(+0x6f85) [0x56521effdf85]
The following simplified config makes ovn-northd crash when running in an
OVN sandbox too:
ovn-nbctl \
-- ls-add sw0 \
-- lr-add lr0 \
-- set logical_router lr0 options:chassis=hv1
options:lb_force_snat_ip=router_ip \
-- lrp-add lr0 lrp0 00:00:00:00:ff:01 4242::4242/64 \
-- lsp-add sw0 lrp0-attachment \
-- lsp-set-type lrp0-attachment router \
-- lsp-set-addresses lrp0-attachment 00:00:00:00:ff:01 \
-- lsp-set-options lrp0-attachment router-port=lrp0
I'm guessing the ovn-northd.at tests don't cover the case when a LRP has
a single IPv6 address (and no IPv4). Let's add one, what do you think?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
