Re: [ovs-dev] [PATCH ovn] northd: Make sure that affinity flows match on VIP.
On Wed, Jan 24, 2024 at 4:52 PM Mark Michelson wrote:
>
> Hi Ales,
>
> Acked-by: Mark Michelson
>
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
> > ---
> > 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(_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(_match, REGBIT_KNOWN_LB_SESSION" == 1 && "
> > -
Re: [ovs-dev] [PATCH ovn] northd: Make sure that affinity flows match on VIP.
Hi Ales,
Acked-by: Mark Michelson
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
---
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(_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(_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']])
-
[ovs-dev] [PATCH ovn] northd: Make sure that affinity flows match on VIP.
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
---
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(_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(_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
