The affinity code was differentiating between force_snat and skip snat. However, if both parameters were set at the same time the force_snat would be preferred, which shouldn't be the case.
Make sure the skip_snat is preferred if both parameters are present. Fixes: d3926b4 ("northd: rely on new actions for lb affinity") Reported-at: https://bugzilla.redhat.com/2224260 Signed-off-by: Ales Musil <amu...@redhat.com> Signed-off-by: Dumitru Ceara <dce...@redhat.com> (cherry picked from commit 0f4df76) --- northd/northd.c | 20 +++++++++++++++----- tests/ovn-northd.at | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 00a9013dd..fd1f9c629 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -10669,6 +10669,10 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, xcalloc(lb->n_nb_lr, sizeof *distributed_router); int n_distributed_router = 0; + struct ovn_datapath **lb_aff_skip_snat_router = + xcalloc(lb->n_nb_lr, sizeof *lb_aff_skip_snat_router); + int n_lb_aff_skip_snat_router = 0; + struct ovn_datapath **lb_aff_force_snat_router = xcalloc(lb->n_nb_lr, sizeof *lb_aff_force_snat_router); int n_lb_aff_force_snat_router = 0; @@ -10695,7 +10699,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, distributed_router[n_distributed_router++] = od; } - if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || + if (lb->skip_snat) { + lb_aff_skip_snat_router[n_lb_aff_skip_snat_router++] = od; + } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || od->lb_force_snat_router_ip) { lb_aff_force_snat_router[n_lb_aff_force_snat_router++] = od; } else { @@ -10752,11 +10758,14 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, "next;", lflows, prio, meter_groups); /* LB affinity flows for datapaths where CMS has specified - * skip_snat_for_lb floag option or regular datapaths. + * skip_snat_for_lb flag option. */ - char *lb_aff_action = - lb->skip_snat ? "flags.skip_snat_for_lb = 1; " : NULL; - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, lb_aff_action, + build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, + "flags.skip_snat_for_lb = 1; ", + lb_aff_skip_snat_router, + n_lb_aff_skip_snat_router); + + build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, NULL, lb_aff_router, n_lb_aff_router); /* Distributed router logic */ @@ -10852,6 +10861,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, free(gw_router_force_snat); free(gw_router_skip_snat); free(distributed_router); + free(lb_aff_skip_snat_router); free(lb_aff_force_snat_router); free(lb_aff_router); free(gw_router); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index b50f23e19..4e8eb38b8 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8334,6 +8334,25 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_snat | sort], [0], [dnl table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1; ct_commit_nat;) ]) +AS_BOX([Test LR flows - lb_force_snat_ip="172.16.0.1" + skip_snat=true]) +check ovn-nbctl --wait=sb set logical_router R1 options:lb_force_snat_ip="172.16.0.1" +check ovn-nbctl --wait=sb set load_balancer lb0 options:skip_snat=true + +ovn-sbctl dump-flows R1 > R1flows_force_skip_snat +AT_CAPTURE_FILE([R1flows_force_skip_snat]) + +grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/' | sort +AT_CHECK([grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/' | sort], [0], [dnl + table=??(lr_in_dnat ), priority=0 , match=(1), action=(next;) + table=??(lr_in_dnat ), priority=120 , match=(ct.est && !ct.rel && ip4 && reg0 == 172.16.0.10 && tcp && reg9[[16..31]] == 80 && ct_mark.natted == 1), action=(flags.skip_snat_for_lb = 1; next;) + table=??(lr_in_dnat ), priority=120 , match=(ct.new && !ct.rel && ip4 && reg0 == 172.16.0.10 && tcp && reg9[[16..31]] == 80), action=(flags.skip_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; skip_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.skip_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; skip_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.skip_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; skip_snat);) + table=??(lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est && !ct.new), action=(ct_commit_nat;) + table=??(lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb = 1; ct_commit_nat;) + table=??(lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1; ct_commit_nat;) +]) + AT_CLEANUP ]) -- 2.41.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev