| Message ID | 20240123150036.52490-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Make sure that affinity flows match on VIP. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Ales, Acked-by: Mark Michelson <mmichels@redhat.com> I was ready to point out this doesn't have an accompanying change to ovn-northd.8.xml . However, looking at the existing documentation, the match on the LB VIP is already documented. Apparently this was planned from the beginning but didn't actually make it into the code. On 1/23/24 10:00, Ales Musil wrote: > Consider the two following LBs: > 1) 192.168.0.100:8080 -> 192.168.0.10:80 > affinity_timeout=120, skip_snat=true > 2) 172.10.0.100:8080 -> 192.168.0.10:80 > affinity_timeout=120, skip_snat=false > > Connected to the same LR with "lb_force_snat_ip" option set. > This combination would produce two flows with the same match > but different action: > 1) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) > action=(reg0 = 192.168.0.100; flags.skip_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; skip_snat);) > > 2) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) > action=(reg0 = 172.10.0.100; flags.force_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; force_snat);) > > This causes issues because it's not defined what flow will take > priority because they have the same match. So it might happen that > it the traffic undergoes SNAT when it shouldn't or vice versa. > > To make sure that correct action is taken, differentiate the match by > adding VIP match (ip.dst == VIP). > > Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity") > Reported-at: https://issues.redhat.com/browse/FDP-290 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > northd/northd.c | 14 +++++++----- > tests/ofproto-macros.at | 4 ++-- > tests/ovn-northd.at | 48 +++++++++++++++++++++++++++++++--------- > tests/system-ovn-kmod.at | 8 +++---- > 4 files changed, 52 insertions(+), 22 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index de15ca101..5763a1b8b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -8404,12 +8404,12 @@ build_lb_rules_pre_stateful(struct hmap *lflows, > * > * - load balancing: > * table=lr_in_dnat, priority=150 > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) > * action=(REG_NEXT_HOP_IPV4 = V; lb_action; > * ct_lb_mark(backends=B1:BP1; ct_flag);) > * table=lr_in_dnat, priority=150 > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) > * action=(REG_NEXT_HOP_IPV4 = V; lb_action; > * ct_lb_mark(backends=B2:BP2; ct_flag);) > @@ -8508,7 +8508,8 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, > > /* Prepare common part of affinity match. */ > ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " > - "ct.new && %s && %s == ", ip_match, reg_backend); > + "ct.new && %s.dst == %s && %s == ", ip_match, > + lb_vip->vip_str, reg_backend); > > /* Store the common part length. */ > size_t aff_action_len = aff_action.length; > @@ -8587,13 +8588,13 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, > * > * - load balancing: > * table=ls_in_lb, priority=150 > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) > * action=(REGBIT_CONNTRACK_COMMIT = 0; > * REG_ORIG_DIP_IPV4 = V; REG_ORIG_TP_DPORT = VP; > * ct_lb_mark(backends=B1:BP1);) > * table=ls_in_lb, priority=150 > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) > * action=(REGBIT_CONNTRACK_COMMIT = 0; > * REG_ORIG_DIP_IPV4 = V; > @@ -8696,7 +8697,8 @@ build_lb_affinity_ls_flows(struct hmap *lflows, > > /* Prepare common part of affinity match. */ > ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " > - "ct.new && %s && %s == ", ip_match, reg_backend); > + "ct.new && %s.dst == %s && %s == ", ip_match, > + lb_vip->vip_str, reg_backend); > > /* Store the common part length. */ > size_t aff_action_len = aff_action.length; > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 07ef1d092..bccedbaf7 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -200,14 +200,14 @@ m4_define([_OVS_VSWITCHD_START], > AT_CHECK([[sed < stderr ' > /vlog|INFO|opened log file/d > /ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']]) > - AT_CAPTURE_FILE([ovsdb-server.log]) > + # AT_CAPTURE_FILE([ovsdb-server.log]) > > dnl Initialize database. > AT_CHECK([ovs-vsctl --no-wait init $2]) > > dnl Start ovs-vswitchd. > AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) > - AT_CAPTURE_FILE([ovs-vswitchd.log]) > + #AT_CAPTURE_FILE([ovs-vswitchd.log]) > on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" > AT_CHECK([[sed < stderr ' > /ovs_numa|INFO|Discovered /d > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 9a0d418e4..45faa5f47 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -8862,8 +8862,8 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | sed 's/table=../table=??/' | sort] > AT_CHECK([grep "ls_in_lb " S0flows | sed 's/table=../table=??/' | sort], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > ]) > AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | sed 's/table=../table=??/' | sort], [0], [dnl > table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) > @@ -8882,8 +8882,8 @@ AT_CHECK([grep "lr_in_lb_aff_check" R1flows | sed 's/table=../table=??/' | sort] > AT_CHECK([grep "lr_in_dnat " R1flows | 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=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > - 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; ct_lb_mark(backends=10.0.0.2:80);) > - 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; ct_lb_mark(backends=20.0.0.2:80);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=10.0.0.2:80);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=20.0.0.2:80);) > 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;) > @@ -8906,8 +8906,8 @@ AT_CAPTURE_FILE([R1flows_skip_snat]) > AT_CHECK([grep "lr_in_dnat " R1flows_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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) > @@ -8927,8 +8927,8 @@ AT_CAPTURE_FILE([R1flows_force_snat]) > AT_CHECK([grep "lr_in_dnat " R1flows_force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_snat);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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;) > @@ -8947,8 +8947,8 @@ 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) > @@ -8957,6 +8957,34 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/ > 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 - 2 LBs, lb0 skip_snat=true, lb1 lb_force_snat_ip="172.16.0.1"]) > +check ovn-nbctl set logical_router R1 options:lb_force_snat_ip="172.16.0.1" > +check ovn-nbctl set load_balancer lb0 options:skip_snat=true > +check ovn-nbctl lb-add lb1 172.16.0.20:80 10.0.0.2:80,20.0.0.2:80 tcp > +check ovn-nbctl set load_balancer lb1 options:affinity_timeout=60 > +check ovn-nbctl lr-lb-add R1 lb1 > +check ovn-nbctl --wait=sb sync > + > +ovn-sbctl dump-flows R1 > R1flows_2lbs > +AT_CAPTURE_FILE([R1flows_2lbs]) > + > +AT_CHECK([grep "lr_in_dnat " R1flows_2lbs | 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=120 , match=(ct.new && !ct.rel && ip4 && ip4.dst == 172.16.0.20 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_snat);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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 > ]) > > diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at > index a08116019..88c72edcd 100644 > --- a/tests/system-ovn-kmod.at > +++ b/tests/system-ovn-kmod.at > @@ -151,8 +151,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki > ]) > > check_affinity_flows () { > -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102/{print substr($4,11,length($4)-11)}') > -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202/{print substr($4,11,length($4)-11)}') > +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') > +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') > [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] > echo $? > } > @@ -448,8 +448,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki > ]) > > check_affinity_flows () { > -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000/{print substr($4,11,length($4)-11)}') > -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000/{print substr($4,11,length($4)-11)}') > +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') > +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') > [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] > echo $? > }
On Wed, Jan 24, 2024 at 4:52 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Ales, > > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks. I applied this patch to main and backported to branch-23.09 and branch-23.06. It doesn't apply cleanly to older branches. Please submit backport patches if it needs to be backported further down. Numan > I was ready to point out this doesn't have an accompanying change to > ovn-northd.8.xml . However, looking at the existing documentation, the > match on the LB VIP is already documented. Apparently this was planned > from the beginning but didn't actually make it into the code. > > On 1/23/24 10:00, Ales Musil wrote: > > Consider the two following LBs: > > 1) 192.168.0.100:8080 -> 192.168.0.10:80 > > affinity_timeout=120, skip_snat=true > > 2) 172.10.0.100:8080 -> 192.168.0.10:80 > > affinity_timeout=120, skip_snat=false > > > > Connected to the same LR with "lb_force_snat_ip" option set. > > This combination would produce two flows with the same match > > but different action: > > 1) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) > > action=(reg0 = 192.168.0.100; flags.skip_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; skip_snat);) > > > > 2) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) > > action=(reg0 = 172.10.0.100; flags.force_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; force_snat);) > > > > This causes issues because it's not defined what flow will take > > priority because they have the same match. So it might happen that > > it the traffic undergoes SNAT when it shouldn't or vice versa. > > > > To make sure that correct action is taken, differentiate the match by > > adding VIP match (ip.dst == VIP). > > > > Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity") > > Reported-at: https://issues.redhat.com/browse/FDP-290 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > northd/northd.c | 14 +++++++----- > > tests/ofproto-macros.at | 4 ++-- > > tests/ovn-northd.at | 48 +++++++++++++++++++++++++++++++--------- > > tests/system-ovn-kmod.at | 8 +++---- > > 4 files changed, 52 insertions(+), 22 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index de15ca101..5763a1b8b 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -8404,12 +8404,12 @@ build_lb_rules_pre_stateful(struct hmap *lflows, > > * > > * - load balancing: > > * table=lr_in_dnat, priority=150 > > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > > * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) > > * action=(REG_NEXT_HOP_IPV4 = V; lb_action; > > * ct_lb_mark(backends=B1:BP1; ct_flag);) > > * table=lr_in_dnat, priority=150 > > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > > * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) > > * action=(REG_NEXT_HOP_IPV4 = V; lb_action; > > * ct_lb_mark(backends=B2:BP2; ct_flag);) > > @@ -8508,7 +8508,8 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, > > > > /* Prepare common part of affinity match. */ > > ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " > > - "ct.new && %s && %s == ", ip_match, reg_backend); > > + "ct.new && %s.dst == %s && %s == ", ip_match, > > + lb_vip->vip_str, reg_backend); > > > > /* Store the common part length. */ > > size_t aff_action_len = aff_action.length; > > @@ -8587,13 +8588,13 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, > > * > > * - load balancing: > > * table=ls_in_lb, priority=150 > > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > > * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) > > * action=(REGBIT_CONNTRACK_COMMIT = 0; > > * REG_ORIG_DIP_IPV4 = V; REG_ORIG_TP_DPORT = VP; > > * ct_lb_mark(backends=B1:BP1);) > > * table=ls_in_lb, priority=150 > > - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 > > + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V > > * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) > > * action=(REGBIT_CONNTRACK_COMMIT = 0; > > * REG_ORIG_DIP_IPV4 = V; > > @@ -8696,7 +8697,8 @@ build_lb_affinity_ls_flows(struct hmap *lflows, > > > > /* Prepare common part of affinity match. */ > > ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " > > - "ct.new && %s && %s == ", ip_match, reg_backend); > > + "ct.new && %s.dst == %s && %s == ", ip_match, > > + lb_vip->vip_str, reg_backend); > > > > /* Store the common part length. */ > > size_t aff_action_len = aff_action.length; > > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > > index 07ef1d092..bccedbaf7 100644 > > --- a/tests/ofproto-macros.at > > +++ b/tests/ofproto-macros.at > > @@ -200,14 +200,14 @@ m4_define([_OVS_VSWITCHD_START], > > AT_CHECK([[sed < stderr ' > > /vlog|INFO|opened log file/d > > /ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']]) > > - AT_CAPTURE_FILE([ovsdb-server.log]) > > + # AT_CAPTURE_FILE([ovsdb-server.log]) > > > > dnl Initialize database. > > AT_CHECK([ovs-vsctl --no-wait init $2]) > > > > dnl Start ovs-vswitchd. > > AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) > > - AT_CAPTURE_FILE([ovs-vswitchd.log]) > > + #AT_CAPTURE_FILE([ovs-vswitchd.log]) > > on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" > > AT_CHECK([[sed < stderr ' > > /ovs_numa|INFO|Discovered /d > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 9a0d418e4..45faa5f47 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -8862,8 +8862,8 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | sed 's/table=../table=??/' | sort] > > AT_CHECK([grep "ls_in_lb " S0flows | sed 's/table=../table=??/' | sort], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > > ]) > > AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | sed 's/table=../table=??/' | sort], [0], [dnl > > table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) > > @@ -8882,8 +8882,8 @@ AT_CHECK([grep "lr_in_lb_aff_check" R1flows | sed 's/table=../table=??/' | sort] > > AT_CHECK([grep "lr_in_dnat " R1flows | 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=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > > - 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; ct_lb_mark(backends=10.0.0.2:80);) > > - 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; ct_lb_mark(backends=20.0.0.2:80);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=10.0.0.2:80);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=20.0.0.2:80);) > > 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;) > > @@ -8906,8 +8906,8 @@ AT_CAPTURE_FILE([R1flows_skip_snat]) > > AT_CHECK([grep "lr_in_dnat " R1flows_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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) > > @@ -8927,8 +8927,8 @@ AT_CAPTURE_FILE([R1flows_force_snat]) > > AT_CHECK([grep "lr_in_dnat " R1flows_force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_snat);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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;) > > @@ -8947,8 +8947,8 @@ 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) > > @@ -8957,6 +8957,34 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/ > > 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 - 2 LBs, lb0 skip_snat=true, lb1 lb_force_snat_ip="172.16.0.1"]) > > +check ovn-nbctl set logical_router R1 options:lb_force_snat_ip="172.16.0.1" > > +check ovn-nbctl set load_balancer lb0 options:skip_snat=true > > +check ovn-nbctl lb-add lb1 172.16.0.20:80 10.0.0.2:80,20.0.0.2:80 tcp > > +check ovn-nbctl set load_balancer lb1 options:affinity_timeout=60 > > +check ovn-nbctl lr-lb-add R1 lb1 > > +check ovn-nbctl --wait=sb sync > > + > > +ovn-sbctl dump-flows R1 > R1flows_2lbs > > +AT_CAPTURE_FILE([R1flows_2lbs]) > > + > > +AT_CHECK([grep "lr_in_dnat " R1flows_2lbs | 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=120 , match=(ct.new && !ct.rel && ip4 && ip4.dst == 172.16.0.20 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_snat);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) > > + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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 > > ]) > > > > diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at > > index a08116019..88c72edcd 100644 > > --- a/tests/system-ovn-kmod.at > > +++ b/tests/system-ovn-kmod.at > > @@ -151,8 +151,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki > > ]) > > > > check_affinity_flows () { > > -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102/{print substr($4,11,length($4)-11)}') > > -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202/{print substr($4,11,length($4)-11)}') > > +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') > > +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') > > [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] > > echo $? > > } > > @@ -448,8 +448,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki > > ]) > > > > check_affinity_flows () { > > -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000/{print substr($4,11,length($4)-11)}') > > -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000/{print substr($4,11,length($4)-11)}') > > +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') > > +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') > > [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] > > echo $? > > } > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/northd.c b/northd/northd.c index de15ca101..5763a1b8b 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -8404,12 +8404,12 @@ build_lb_rules_pre_stateful(struct hmap *lflows, * * - load balancing: * table=lr_in_dnat, priority=150 - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) * action=(REG_NEXT_HOP_IPV4 = V; lb_action; * ct_lb_mark(backends=B1:BP1; ct_flag);) * table=lr_in_dnat, priority=150 - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) * action=(REG_NEXT_HOP_IPV4 = V; lb_action; * ct_lb_mark(backends=B2:BP2; ct_flag);) @@ -8508,7 +8508,8 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, /* Prepare common part of affinity match. */ ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " - "ct.new && %s && %s == ", ip_match, reg_backend); + "ct.new && %s.dst == %s && %s == ", ip_match, + lb_vip->vip_str, reg_backend); /* Store the common part length. */ size_t aff_action_len = aff_action.length; @@ -8587,13 +8588,13 @@ build_lb_affinity_lr_flows(struct hmap *lflows, const struct ovn_northd_lb *lb, * * - load balancing: * table=ls_in_lb, priority=150 - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1) * action=(REGBIT_CONNTRACK_COMMIT = 0; * REG_ORIG_DIP_IPV4 = V; REG_ORIG_TP_DPORT = VP; * ct_lb_mark(backends=B1:BP1);) * table=ls_in_lb, priority=150 - * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4 + * match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4.dst == V * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2) * action=(REGBIT_CONNTRACK_COMMIT = 0; * REG_ORIG_DIP_IPV4 = V; @@ -8696,7 +8697,8 @@ build_lb_affinity_ls_flows(struct hmap *lflows, /* Prepare common part of affinity match. */ ds_put_format(&aff_match, REGBIT_KNOWN_LB_SESSION" == 1 && " - "ct.new && %s && %s == ", ip_match, reg_backend); + "ct.new && %s.dst == %s && %s == ", ip_match, + lb_vip->vip_str, reg_backend); /* Store the common part length. */ size_t aff_action_len = aff_action.length; diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 07ef1d092..bccedbaf7 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -200,14 +200,14 @@ m4_define([_OVS_VSWITCHD_START], AT_CHECK([[sed < stderr ' /vlog|INFO|opened log file/d /ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']]) - AT_CAPTURE_FILE([ovsdb-server.log]) + # AT_CAPTURE_FILE([ovsdb-server.log]) dnl Initialize database. AT_CHECK([ovs-vsctl --no-wait init $2]) dnl Start ovs-vswitchd. AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) - AT_CAPTURE_FILE([ovs-vswitchd.log]) + #AT_CAPTURE_FILE([ovs-vswitchd.log]) on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`" AT_CHECK([[sed < stderr ' /ovs_numa|INFO|Discovered /d diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 9a0d418e4..45faa5f47 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8862,8 +8862,8 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | sed 's/table=../table=??/' | sort] AT_CHECK([grep "ls_in_lb " S0flows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) ]) AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) @@ -8882,8 +8882,8 @@ AT_CHECK([grep "lr_in_lb_aff_check" R1flows | sed 's/table=../table=??/' | sort] AT_CHECK([grep "lr_in_dnat " R1flows | 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=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) - 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; ct_lb_mark(backends=10.0.0.2:80);) - 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; ct_lb_mark(backends=20.0.0.2:80);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=10.0.0.2:80);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; ct_lb_mark(backends=20.0.0.2:80);) 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;) @@ -8906,8 +8906,8 @@ AT_CAPTURE_FILE([R1flows_skip_snat]) AT_CHECK([grep "lr_in_dnat " R1flows_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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) @@ -8927,8 +8927,8 @@ AT_CAPTURE_FILE([R1flows_force_snat]) AT_CHECK([grep "lr_in_dnat " R1flows_force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_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.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.10; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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;) @@ -8947,8 +8947,8 @@ 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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;) @@ -8957,6 +8957,34 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/ 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 - 2 LBs, lb0 skip_snat=true, lb1 lb_force_snat_ip="172.16.0.1"]) +check ovn-nbctl set logical_router R1 options:lb_force_snat_ip="172.16.0.1" +check ovn-nbctl set load_balancer lb0 options:skip_snat=true +check ovn-nbctl lb-add lb1 172.16.0.20:80 10.0.0.2:80,20.0.0.2:80 tcp +check ovn-nbctl set load_balancer lb1 options:affinity_timeout=60 +check ovn-nbctl lr-lb-add R1 lb1 +check ovn-nbctl --wait=sb sync + +ovn-sbctl dump-flows R1 > R1flows_2lbs +AT_CAPTURE_FILE([R1flows_2lbs]) + +AT_CHECK([grep "lr_in_dnat " R1flows_2lbs | 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=120 , match=(ct.new && !ct.rel && ip4 && ip4.dst == 172.16.0.20 && tcp && tcp.dst == 80), action=(flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80; force_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && 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.dst == 172.16.0.10 && 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=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=10.0.0.2:80; force_snat);) + table=??(lr_in_dnat ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.20 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0 = 172.16.0.20; flags.force_snat_for_lb = 1; ct_lb_mark(backends=20.0.0.2:80; force_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 ]) diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at index a08116019..88c72edcd 100644 --- a/tests/system-ovn-kmod.at +++ b/tests/system-ovn-kmod.at @@ -151,8 +151,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki ]) check_affinity_flows () { -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102/{print substr($4,11,length($4)-11)}') -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202/{print substr($4,11,length($4)-11)}') +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80102,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ip,reg4=0xc0a80202,.*nw_dst=172.16.1.100/{print substr($4,11,length($4)-11)}') [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] echo $? } @@ -448,8 +448,8 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=78 --no-stats | strip_cooki ]) check_affinity_flows () { -n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000/{print substr($4,11,length($4)-11)}') -n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000/{print substr($4,11,length($4)-11)}') +n1=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd110000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') +n2=$(ovs-ofctl dump-flows br-int table=15 |awk '/priority=150,ct_state=\+new\+trk,ipv6,reg4=0xfd120000,.*ipv6_dst=fd30::1\s/{print substr($4,11,length($4)-11)}') [[ $n1 -gt 0 -a $n2 -eq 0 ]] || [[ $n1 -eq 0 -a $n2 -gt 0 ]] echo $? }
Consider the two following LBs: 1) 192.168.0.100:8080 -> 192.168.0.10:80 affinity_timeout=120, skip_snat=true 2) 172.10.0.100:8080 -> 192.168.0.10:80 affinity_timeout=120, skip_snat=false Connected to the same LR with "lb_force_snat_ip" option set. This combination would produce two flows with the same match but different action: 1) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) action=(reg0 = 192.168.0.100; flags.skip_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; skip_snat);) 2) priority=150, match=(reg9[6] == 1 && ct.new && ip4 && reg4 == 192.168.0.10 && reg8[0..15] == 80) action=(reg0 = 172.10.0.100; flags.force_snat_for_lb = 1; ct_lb_mark(backends=192.168.0.10:80; force_snat);) This causes issues because it's not defined what flow will take priority because they have the same match. So it might happen that it the traffic undergoes SNAT when it shouldn't or vice versa. To make sure that correct action is taken, differentiate the match by adding VIP match (ip.dst == VIP). Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity") Reported-at: https://issues.redhat.com/browse/FDP-290 Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/northd.c | 14 +++++++----- tests/ofproto-macros.at | 4 ++-- tests/ovn-northd.at | 48 +++++++++++++++++++++++++++++++--------- tests/system-ovn-kmod.at | 8 +++---- 4 files changed, 52 insertions(+), 22 deletions(-)