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 <[email protected]>
---
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
