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: d3926b433e44 ("northd: rely on new actions for lb affinity") Reported-at: https://bugzilla.redhat.com/2224260 Signed-off-by: Ales Musil <amu...@redhat.com> --- northd/northd.c | 53 ++++++++++++++++----------------------------- tests/ovn-northd.at | 20 +++++++++++++++++ 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index b9605862e..6fac13d00 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11478,6 +11478,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, { bool ipv4 = lb_vip->address_family == AF_INET; const char *ip_match = ipv4 ? "ip4" : "ip6"; + char *aff_action[LROUTER_NAT_LB_FLOW_MAX] = { + NULL, + "flags.skip_snat_for_lb = 1; ", + "flags.force_snat_for_lb = 1; " + }; int prio = 110; @@ -11552,15 +11557,13 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, ctx.new_action[LROUTER_NAT_LB_FLOW_SKIP_SNAT] = ds_cstr(&skip_snat_act); ctx.new_action[LROUTER_NAT_LB_FLOW_FORCE_SNAT] = ds_cstr(&force_snat_act); - enum { - LROUTER_NAT_LB_AFF = LROUTER_NAT_LB_FLOW_MAX, - LROUTER_NAT_LB_AFF_FORCE_SNAT = LROUTER_NAT_LB_FLOW_MAX + 1, - }; - unsigned long *dp_bitmap[LROUTER_NAT_LB_FLOW_MAX + 2]; + unsigned long *gw_dp_bitmap[LROUTER_NAT_LB_FLOW_MAX]; + unsigned long *aff_dp_bitmap[LROUTER_NAT_LB_FLOW_MAX]; size_t bitmap_len = ods_size(lr_datapaths); - for (size_t i = 0; i < LROUTER_NAT_LB_FLOW_MAX + 2; i++) { - dp_bitmap[i] = bitmap_allocate(bitmap_len); + for (size_t i = 0; i < LROUTER_NAT_LB_FLOW_MAX; i++) { + gw_dp_bitmap[i] = bitmap_allocate(bitmap_len); + aff_dp_bitmap[i] = bitmap_allocate(bitmap_len); } /* Group gw router since we do not have datapath dependency in @@ -11581,18 +11584,13 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, } if (!od->n_l3dgw_ports) { - bitmap_set1(dp_bitmap[type], index); + bitmap_set1(gw_dp_bitmap[type], index); } else { build_distr_lrouter_nat_flows_for_lb(&ctx, type, od); } if (lb->affinity_timeout) { - if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || - od->lb_force_snat_router_ip) { - bitmap_set1(dp_bitmap[LROUTER_NAT_LB_AFF_FORCE_SNAT], index); - } else { - bitmap_set1(dp_bitmap[LROUTER_NAT_LB_AFF], index); - } + bitmap_set1(aff_dp_bitmap[type], index); } if (sset_contains(&od->external_ips, lb_vip->vip_str)) { @@ -11615,34 +11613,21 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, for (size_t type = 0; type < LROUTER_NAT_LB_FLOW_MAX; type++) { build_gw_lrouter_nat_flows_for_lb(&ctx, type, lr_datapaths, - dp_bitmap[type]); + gw_dp_bitmap[type]); + build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match), + aff_action[type], aff_dp_bitmap[type], + lr_datapaths); } - /* LB affinity flows for datapaths where CMS has specified - * force_snat_for_lb floag option. - */ - build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match), - "flags.force_snat_for_lb = 1; ", - dp_bitmap[LROUTER_NAT_LB_AFF_FORCE_SNAT], - lr_datapaths); - - /* LB affinity flows for datapaths where CMS has specified - * skip_snat_for_lb floag option or regular datapaths. - */ - char *lb_aff_action = - lb->skip_snat ? "flags.skip_snat_for_lb = 1; " : NULL; - build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match), - lb_aff_action, dp_bitmap[LROUTER_NAT_LB_AFF], - lr_datapaths); - ds_destroy(&unsnat_match); ds_destroy(&undnat_match); ds_destroy(&skip_snat_act); ds_destroy(&force_snat_act); ds_destroy(&gw_redir_action); - for (size_t i = 0; i < LROUTER_NAT_LB_FLOW_MAX + 2; i++) { - bitmap_free(dp_bitmap[i]); + for (size_t i = 0; i < LROUTER_NAT_LB_FLOW_MAX; i++) { + bitmap_free(gw_dp_bitmap[i]); + bitmap_free(aff_dp_bitmap[i]); } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3e06f14c9..9af4605f6 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8828,6 +8828,26 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_snat | sed 's/table=../table=??/' | s 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;) ]) +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]) + +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.new && !ct.rel && ip4 && ip4.dst == 172.16.0.10 && tcp && tcp.dst == 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.est && !ct.rel && !ct.new && ct_mark.natted), action=(next;) + 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.est && !ct.rel && !ct.new && ct_mark.natted && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb = 1; next;) + table=??(lr_in_dnat ), priority=70 , match=(ct.est && !ct.rel && !ct.new && ct_mark.natted && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1; next;) + 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