Re: [ovs-dev] [PATCH ovn v2] northd: Add VIP port to established flows in DNAT table for Load Balancers
On 24/08/2021 16:58, Numan Siddique wrote: > On Tue, Aug 24, 2021 at 11:21 AM Mark Gray wrote: >> >> On 24/08/2021 16:15, Mark Michelson wrote: >>> Excellent, thanks for the updated patch! >>> >>> Acked-by: Mark Michelson >> >> Thanks, and thanks for the review! > > Thanks. I applied this patch to the main branch. > With the ovsrobot run for this patch, I see the ovn-k8s control plane > test has passed. Great !! Even better. It was the RH ovn-kubernetes team who sent the bug my way. > > Thanks > Numan > >> >> ___ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd: Add VIP port to established flows in DNAT table for Load Balancers
On Tue, Aug 24, 2021 at 11:21 AM Mark Gray wrote: > > On 24/08/2021 16:15, Mark Michelson wrote: > > Excellent, thanks for the updated patch! > > > > Acked-by: Mark Michelson > > Thanks, and thanks for the review! Thanks. I applied this patch to the main branch. With the ovsrobot run for this patch, I see the ovn-k8s control plane test has passed. Great !! Thanks Numan > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd: Add VIP port to established flows in DNAT table for Load Balancers
On 24/08/2021 16:15, Mark Michelson wrote: > Excellent, thanks for the updated patch! > > Acked-by: Mark Michelson Thanks, and thanks for the review! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd: Add VIP port to established flows in DNAT table for Load Balancers
Excellent, thanks for the updated patch!
Acked-by: Mark Michelson
On 8/23/21 4:37 PM, Mark Gray wrote:
When adding a load balancer to a logical router, two flows are added to
the ingress DNAT table. One flow is for established connections and one is
for new connections. They have the following form:
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
As the established flow does not specify the VIP port, if two load
balancers are added with the same VIP but different VIP ports, then
two conflicting flows will be added. For example,
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
This normally does not give an issue as both flows will have the same
action: next.
However, if the logical router specifies "force_snat_for_lb" and one
load balancer specifies "skip_snat" then both flows will have the
same match but different, conflicting actions: "flags.force_snat_for_lb = 1;
next;"
and "flags.skip_snat_for_lb = 1; next;". This can cause unintended
consequences.
This commit adds the VIP port to the DNAT flow. It also updates
the defrag table to save that port in a register (before it gets
DNATted).
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1995326
Signed-off-by: Mark Gray
---
Notes:
v2: Addressed Mark's comments
- Change registers
northd/ovn-northd.8.xml | 52 +++---
northd/ovn-northd.c | 32 ++---
northd/ovn_northd.dl| 38 ++
tests/ovn-northd.at | 150
tests/ovn.at| 6 +-
5 files changed, 154 insertions(+), 124 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9b69e4e5750e..eebf0d717999 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2855,15 +2855,18 @@ icmp6 {
If load balancing rules with virtual IP addresses and ports are
configured in OVN_Northbound database for a Gateway router,
a priority-110 flow is added for each configured virtual IP address
- VIP and protocol PROTO. For IPv4 VIPs
- the flow matches ip ip4.dst == VIP
- PROTO. For IPv6 VIPs, the flow matches
+ VIP, protocol PROTO and port PORT.
+ For IPv4 VIPs the flow matches
+ ip ip4.dst == VIP
+ PROTO PROTO.dst ==
+ PORT. For IPv6 VIPs, the flow matches
ip ip6.dst == VIP
- PROTO. The flow applies the action reg0 =
- VIP; ct_dnat; (or xxreg0 for IPv6) to send
- IP packets to the connection tracker for packet de-fragmentation and to
- dnat the destination IP for the committed connection before sending it to
- the next table.
+ PROTO PROTO.dst ==
+ PORT. The flow applies the action reg0 =
+ VIP; reg9[16..31] = PROTO.dst; ct_dnat;
+ (or xxreg0 for IPv6) to send IP packets to the connection
+ tracker for packet de-fragmentation and to dnat the destination IP for
+ the committed connection before sending it to the next table.
@@ -2913,14 +2916,14 @@ icmp6 {
includes a L4 port PORT of protocol P and IPv4
or IPv6 address VIP, a priority-120 flow that matches on
ct.new ip reg0 == VIP
- P P.dst == PORT
- (xxreg0 == VIP in the IPv6
- case) with an action of ct_lb(args),
- where args contains comma separated IPv4 or IPv6 addresses
- (and optional port numbers) to load balance to. If the router is
- configured to force SNAT any load-balanced packets, the above action
- will be replaced by flags.force_snat_for_lb = 1;
- ct_lb(args);.
+ P reg9[16..31] ==
+ PORT (xxreg0 == VIP
+ in the IPv6 case) with an action of
+ ct_lb(args), where args contains
+ comma separated IPv4 or IPv6 addresses (and optional port numbers) to
+ load balance to. If the router is configured to force SNAT any
+ load-balanced packets, the above action will be replaced by
+ flags.force_snat_for_lb = 1; ct_lb(args);.
If the load balancing rule is configured with skip_snat
set to true, the above action will be replaced by
flags.skip_snat_for_lb = 1; ct_lb(args);.
@@ -2945,14 +2948,15 @@ icmp6 {
PORT of protocol P and IPv4 or IPv6 address
VIP, a priority-120 flow that matches on
ct.est ip4 reg0 == VIP
- P P.dst == PORT
- (ip6 and xxreg0 == VIP
- in the IPv6 case) with an action of next;. If
- the router is configured to force SNAT any load-balanced packets, the
- above action will be replaced by flags.force_snat_for_lb = 1;
- next;. If the load balancing rule is configured with
- skip_snat set to true, the above action will be replaced
- by flags.skip_snat_for_lb = 1; next;.
+ P reg9[16..31] ==
+ PORT
[ovs-dev] [PATCH ovn v2] northd: Add VIP port to established flows in DNAT table for Load Balancers
When adding a load balancer to a logical router, two flows are added to
the ingress DNAT table. One flow is for established connections and one is
for new connections. They have the following form:
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
As the established flow does not specify the VIP port, if two load
balancers are added with the same VIP but different VIP ports, then
two conflicting flows will be added. For example,
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
ct.est && ip4 && reg0 == 10.0.0.10 && ct_label.natted == 1 && tcp
This normally does not give an issue as both flows will have the same
action: next.
However, if the logical router specifies "force_snat_for_lb" and one
load balancer specifies "skip_snat" then both flows will have the
same match but different, conflicting actions: "flags.force_snat_for_lb = 1;
next;"
and "flags.skip_snat_for_lb = 1; next;". This can cause unintended
consequences.
This commit adds the VIP port to the DNAT flow. It also updates
the defrag table to save that port in a register (before it gets
DNATted).
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1995326
Signed-off-by: Mark Gray
---
Notes:
v2: Addressed Mark's comments
- Change registers
northd/ovn-northd.8.xml | 52 +++---
northd/ovn-northd.c | 32 ++---
northd/ovn_northd.dl| 38 ++
tests/ovn-northd.at | 150
tests/ovn.at| 6 +-
5 files changed, 154 insertions(+), 124 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9b69e4e5750e..eebf0d717999 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2855,15 +2855,18 @@ icmp6 {
If load balancing rules with virtual IP addresses and ports are
configured in OVN_Northbound database for a Gateway router,
a priority-110 flow is added for each configured virtual IP address
- VIP and protocol PROTO. For IPv4 VIPs
- the flow matches ip ip4.dst == VIP
- PROTO. For IPv6 VIPs, the flow matches
+ VIP, protocol PROTO and port PORT.
+ For IPv4 VIPs the flow matches
+ ip ip4.dst == VIP
+ PROTO PROTO.dst ==
+ PORT. For IPv6 VIPs, the flow matches
ip ip6.dst == VIP
- PROTO. The flow applies the action reg0 =
- VIP; ct_dnat; (or xxreg0 for IPv6) to send
- IP packets to the connection tracker for packet de-fragmentation and to
- dnat the destination IP for the committed connection before sending it to
- the next table.
+ PROTO PROTO.dst ==
+ PORT. The flow applies the action reg0 =
+ VIP; reg9[16..31] = PROTO.dst; ct_dnat;
+ (or xxreg0 for IPv6) to send IP packets to the connection
+ tracker for packet de-fragmentation and to dnat the destination IP for
+ the committed connection before sending it to the next table.
@@ -2913,14 +2916,14 @@ icmp6 {
includes a L4 port PORT of protocol P and IPv4
or IPv6 address VIP, a priority-120 flow that matches on
ct.new ip reg0 == VIP
- P P.dst == PORT
- (xxreg0 == VIP in the IPv6
- case) with an action of ct_lb(args),
- where args contains comma separated IPv4 or IPv6 addresses
- (and optional port numbers) to load balance to. If the router is
- configured to force SNAT any load-balanced packets, the above action
- will be replaced by flags.force_snat_for_lb = 1;
- ct_lb(args);.
+ P reg9[16..31] ==
+ PORT (xxreg0 == VIP
+ in the IPv6 case) with an action of
+ ct_lb(args), where args contains
+ comma separated IPv4 or IPv6 addresses (and optional port numbers) to
+ load balance to. If the router is configured to force SNAT any
+ load-balanced packets, the above action will be replaced by
+ flags.force_snat_for_lb = 1; ct_lb(args);.
If the load balancing rule is configured with skip_snat
set to true, the above action will be replaced by
flags.skip_snat_for_lb = 1; ct_lb(args);.
@@ -2945,14 +2948,15 @@ icmp6 {
PORT of protocol P and IPv4 or IPv6 address
VIP, a priority-120 flow that matches on
ct.est ip4 reg0 == VIP
- P P.dst == PORT
- (ip6 and xxreg0 == VIP
- in the IPv6 case) with an action of next;. If
- the router is configured to force SNAT any load-balanced packets, the
- above action will be replaced by flags.force_snat_for_lb = 1;
- next;. If the load balancing rule is configured with
- skip_snat set to true, the above action will be replaced
- by flags.skip_snat_for_lb = 1; next;.
+ P reg9[16..31] ==
+ PORT (ip6 and
+ xxreg0 == VIP in the IPv6 case) with an
+ action of next;. If the router is configured to force
+
