Re: [ovs-dev] [PATCH ovn] northd: Add VIP port to established flows in DNAT table for Load Balancers

Mark Michelson Fri, 20 Aug 2021 12:48:40 -0700

Hi Mark,

One thing that is not addressed in the commit message is the change fromusing reg0 to reg1 in some of the flows. For logical routers,reg0/xxreg0 is the next hop address, and reg1/xxreg1 is the source IP.The fact you're setting reg1 to the destination IP may cause somestrangeness in places that are assuming reg1 is the source IP. And sincethere are places where you are not setting reg0 to the next hop IP, itcould cause routing issues. In ovn-northd.c, you can find the following:


  /* Registers used for routing. */
  #define REG_NEXT_HOP_IPV4 "reg0"
  #define REG_NEXT_HOP_IPV6 "xxreg0"
  #define REG_SRC_IPV4 "reg1"
  #define REG_SRC_IPV6 "xxreg1"

As an aside, it's annoying that the code sometimes eschews these niceconstants in favor of using "reg0" or "reg1" directly.

I think the switch to using reg1/xxreg1 is not correct here. I thinkthat the original registers need to be used.

I may not understand the original problem well enough, but I also don'treally understand the need to save the destination port before DNATting.But if it is necessary, and you restore the original register usage,then you will need to use bits in reg9 for the destination port.Everywhere else is already taken up for logical routers.


On 8/19/21 4:04 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 <[email protected]>
---
  northd/ovn-northd.8.xml |  76 +++++++++--------
  northd/ovn-northd.c     |  31 ++++---
  northd/ovn_northd.dl    |  55 ++++++++----
  tests/ovn-northd.at     | 180 ++++++++++++++++++++--------------------
  tests/ovn.at            |   6 +-
  5 files changed, 190 insertions(+), 158 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9b69e4e5750e..2b301fe7b6b8 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2844,8 +2844,8 @@ icmp6 {
        <var>VIP</var>. For IPv4 <var>VIPs</var> the flow matches
        <code>ip &amp;&amp; ip4.dst == <var>VIP</var></code>.  For IPv6
        <var>VIPs</var>, the flow matches <code>ip &amp;&amp; ip6.dst ==
-      <var>VIP</var></code>. The flow applies the action <code>reg0 =
-      <var>VIP</var>; ct_dnat;</code>  (or <code>xxreg0</code> for IPv6) to
+      <var>VIP</var></code>. The flow applies the action <code>reg1 =
+      <var>VIP</var>; ct_dnat;</code>  (or <code>xxreg1</code> 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.
@@ -2855,15 +2855,18 @@ icmp6 {
        If load balancing rules with virtual IP addresses and ports are
        configured in <code>OVN_Northbound</code> database for a Gateway router,
        a priority-110 flow is added for each configured virtual IP address
-      <var>VIP</var> and protocol <var>PROTO</var>. For IPv4 <var>VIPs</var>
-      the flow matches <code>ip &amp;&amp; ip4.dst == <var>VIP</var> &amp;&amp;
-      <var>PROTO</var></code>. For IPv6 <var>VIPs</var>, the flow matches
+      <var>VIP</var>, protocol <var>PROTO</var> and port <var>PORT</var>.
+      For IPv4 <var>VIPs</var> the flow matches
+      <code>ip &amp;&amp; ip4.dst == <var>VIP</var> &amp;&amp;
+      <var>PROTO</var> &amp;&amp; <var>PROTO</var>.dst ==
+      <var>PORT</var></code>. For IPv6 <var>VIPs</var>, the flow matches
        <code>ip &amp;&amp; ip6.dst == <var>VIP</var> &amp;&amp;
-      <var>PROTO</var></code>. The flow applies the action <code>reg0 =
-      <var>VIP</var>; ct_dnat;</code> (or <code>xxreg0</code> 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.
+      <var>PROTO</var> &amp;&amp; <var>PROTO</var>.dst ==
+      <var>PORT</var></code>. The flow applies the action <code>reg1 =
+      <var>VIP</var>; reg2[0..15] = <var>PROTO</var>.dst; ct_dnat;</code>
+      (or <code>xxreg1</code> 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.
      </p>

@@ -2912,15 +2915,15 @@ icmp6 {
            Router with gateway port in <code>OVN_Northbound</code> database 
that
            includes a L4 port <var>PORT</var> of protocol <var>P</var> and IPv4
            or IPv6 address <var>VIP</var>, a priority-120 flow that matches on
-          <code>ct.new &amp;&amp; ip &amp;&amp; reg0 == <var>VIP</var>
-          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
-          </var></code> (<code>xxreg0 == <var>VIP</var></code> in the IPv6
-          case) with an action of <code>ct_lb(<var>args</var>)</code>,
-          where <var>args</var> 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 <code>flags.force_snat_for_lb = 1;
-          ct_lb(<var>args</var>);</code>.
+          <code>ct.new &amp;&amp; ip &amp;&amp; reg1 == <var>VIP</var>
+          &amp;&amp; <var>P</var> &amp;&amp; reg2[0..15] == </code>
+          <code><var>PORT</var></code> (<code>xxreg1 == <var>VIP</var></code>
+          in the IPv6 case) with an action of
+          <code>ct_lb(<var>args</var>)</code>, where <var>args</var> 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
+          <code>flags.force_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
            If the load balancing rule is configured with <code>skip_snat</code>
            set to true, the above action will be replaced by
            <code>flags.skip_snat_for_lb = 1; ct_lb(<var>args</var>);</code>.
@@ -2932,7 +2935,7 @@ icmp6 {

            The previous table <code>lr_in_defrag</code> sets the register
-          <code>reg0</code> (or <code>xxreg0</code> for IPv6) and does
+          <code>reg1</code> (or <code>xxreg1</code> for IPv6) and does
            <code>ct_dnat</code>.  Hence for established traffic, this
            table just advances the packet to the next stage.
          </p>
@@ -2944,20 +2947,21 @@ icmp6 {
            <code>OVN_Northbound</code> database that includes a L4 port
            <var>PORT</var> of protocol <var>P</var> and IPv4 or IPv6 address
            <var>VIP</var>, a priority-120 flow that matches on
-          <code>ct.est &amp;&amp; ip4 &amp;&amp; reg0 == <var>VIP</var>
-          &amp;&amp; <var>P</var> &amp;&amp; <var>P</var>.dst == <var>PORT
-          </var></code> (<code>ip6</code> and <code>xxreg0 == <var>VIP</var>
-          </code> in the IPv6 case) with an action of <code>next;</code>. If
-          the router is configured to force SNAT any load-balanced packets, the
-          above action will be replaced by <code>flags.force_snat_for_lb = 1;
-          next;</code>. If the load balancing rule is configured with
-          <code>skip_snat</code> set to true, the above action will be replaced
-          by <code>flags.skip_snat_for_lb = 1; next;</code>.
+          <code>ct.est &amp;&amp; ip4 &amp;&amp; reg1 == <var>VIP</var>
+          &amp;&amp; <var>P</var> &amp;&amp; reg2[0..15] == </code>
+          <code><var>PORT</var></code> (<code>ip6</code> and
+          <code>xxreg1 == <var>VIP</var></code> in the IPv6 case) with an
+          action of <code>next;</code>. If the router is configured to force
+          SNAT any load-balanced packets, the above action will be replaced by
+          <code>flags.force_snat_for_lb = 1; next;</code>. If the load
+          balancing rule is configured with <code>skip_snat</code> set to true,
+          the above action will be replaced by
+          <code>flags.skip_snat_for_lb = 1; next;</code>.
          </p>

            The previous table <code>lr_in_defrag</code> sets the register
-          <code>reg0</code> (or <code>xxreg0</code> for IPv6) and does
+          <code>reg1</code> (or <code>xxreg1</code> for IPv6) and does
            <code>ct_dnat</code>.  Hence for established traffic, this
            table just advances the packet to the next stage.
          </p>
@@ -2968,8 +2972,8 @@ icmp6 {
            For all the configured load balancing rules for a router in
            <code>OVN_Northbound</code> database that includes just an IP 
address
            <var>VIP</var> to match on, a priority-110 flow that matches on
-          <code>ct.new &amp;&amp; ip4 &amp;&amp; reg0 ==
-          <var>VIP</var></code> (<code>ip6</code> and <code>xxreg0 ==
+          <code>ct.new &amp;&amp; ip4 &amp;&amp; reg1 ==
+          <var>VIP</var></code> (<code>ip6</code> and <code>xxreg1 ==
            <var>VIP</var></code> in the IPv6 case) with an action of
            <code>ct_lb(<var>args</var>)</code>, where <var>args</var> contains
            comma separated IPv4 or IPv6 addresses.  If the router is configured
@@ -2983,7 +2987,7 @@ icmp6 {

            The previous table <code>lr_in_defrag</code> sets the register
-          <code>reg0</code> (or <code>xxreg0</code> for IPv6) and does
+          <code>reg1</code> (or <code>xxreg1</code> for IPv6) and does
            <code>ct_dnat</code>.  Hence for established traffic, this
            table just advances the packet to the next stage.
          </p>
@@ -2995,9 +2999,9 @@ icmp6 {
            For all the configured load balancing rules for a router in
            <code>OVN_Northbound</code> database that includes just an IP 
address
            <var>VIP</var> to match on, a priority-110 flow that matches on
-          <code>ct.est &amp;&amp; ip4 &amp;&amp; reg0 ==
+          <code>ct.est &amp;&amp; ip4 &amp;&amp; reg1 ==
            <var>VIP</var></code> (or <code>ip6</code> and
-          <code>xxreg0 == <var>VIP</var></code>) with an action of
+          <code>xxreg1 == <var>VIP</var></code>) with an action of
            <code>next;</code>. If the router is configured to force SNAT any
            load-balanced packets, the above action will be replaced by
            <code>flags.force_snat_for_lb = 1; next;</code>.
@@ -3008,7 +3012,7 @@ icmp6 {

            The previous table <code>lr_in_defrag</code> sets the register
-          <code>reg0</code> (or <code>xxreg0</code> for IPv6) and does
+          <code>reg1</code> (or <code>xxreg1</code> for IPv6) and does
            <code>ct_dnat</code>.  Hence for established traffic, this
            table just advances the packet to the next stage.
          </p>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3d8e21a4fe8a..de046751788b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -243,12 +243,12 @@ enum ovn_stage {
  #define REGBIT_HAIRPIN_REPLY      "reg0[12]"
  #define REGBIT_ACL_LABEL          "reg0[13]"

+

+/* Register definitions for switches and routers. */
  #define REG_ORIG_DIP_IPV4         "reg1"
  #define REG_ORIG_DIP_IPV6         "xxreg1"
  #define REG_ORIG_TP_DPORT         "reg2[0..15]"

-/* Register definitions for switches and routers. */

-
  /* Indicate that this packet has been recirculated using egress
   * loopback.  This allows certain checks to be bypassed, such as a
   * logical router dropping packets with source IP address equals
@@ -9147,9 +9147,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
       * an action of "next;".
       */
      if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
-        ds_put_format(match, "ip4 && reg0 == %s", lb_vip->vip_str);
+        ds_put_format(match, "ip4 && "REG_ORIG_DIP_IPV4" == %s",
+                      lb_vip->vip_str);

This is an example where I think the original code should be expressedas REG_NEXT_HOP_IPV4 instead of just "reg0". There are more throughoutyour change, but I won't point them all out.

      } else {
-        ds_put_format(match, "ip6 && xxreg0 == %s", lb_vip->vip_str);
+        ds_put_format(match, "ip6 && "REG_ORIG_DIP_IPV6" == %s",
+                      lb_vip->vip_str);
      }

enum lb_snat_type snat_type = NO_FORCE_SNAT;

@@ -9164,11 +9166,13 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
      int prio = 110;
      if (lb_vip->vip_port) {
          prio = 120;
-        new_match = xasprintf("ct.new && %s && %s && %s.dst == %d",
-                              ds_cstr(match), lb->proto, lb->proto,
-                              lb_vip->vip_port);
-        est_match = xasprintf("ct.est && %s && ct_label.natted == 1 && %s",
-                              ds_cstr(match), lb->proto);
+        new_match = xasprintf("ct.new && %s && %s && "
+                              REG_ORIG_TP_DPORT" == %d",
+                              ds_cstr(match), lb->proto, lb_vip->vip_port);
+        est_match = xasprintf("ct.est && %s && %s && "
+                              REG_ORIG_TP_DPORT" == %d && "
+                              "ct_label.natted == 1",
+                              ds_cstr(match), lb->proto, lb_vip->vip_port);
      } else {
          new_match = xasprintf("ct.new && %s", ds_cstr(match));
          est_match = xasprintf("ct.est && %s && ct_label.natted == 1",
@@ -9399,19 +9403,24 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb 
*lb,

if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {

              ds_put_format(match, "ip && ip4.dst == %s", lb_vip->vip_str);
-            ds_put_format(&defrag_actions, "reg0 = %s; ct_dnat;",
+            ds_put_format(&defrag_actions, REG_ORIG_DIP_IPV4" = %s; ",
                            lb_vip->vip_str);
          } else {
              ds_put_format(match, "ip && ip6.dst == %s", lb_vip->vip_str);
-            ds_put_format(&defrag_actions, "xxreg0 = %s; ct_dnat;",
+            ds_put_format(&defrag_actions, REG_ORIG_DIP_IPV6" = %s; ",
                            lb_vip->vip_str);
          }

if (lb_vip->vip_port) {

              ds_put_format(match, " && %s", lb->proto);
              prio = 110;
+
+            ds_put_format(&defrag_actions, REG_ORIG_TP_DPORT
+                          " = %s.dst; ", lb->proto);
          }

+ ds_put_format(&defrag_actions, "ct_dnat;");

+
          for (size_t j = 0; j < lb->n_nb_lr; j++) {
              ovn_lflow_add_with_hint(lflows, lb->nb_lr[j], S_ROUTER_IN_DEFRAG,
                                      prio, ds_cstr(match),
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 9cf4c373bbb3..ebd9b59e5be7 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -1620,11 +1620,11 @@ function rEGBIT_LKUP_FDB()         : string = "reg0[11]"
  function rEGBIT_HAIRPIN_REPLY()    : string = "reg0[12]"
  function rEGBIT_ACL_LABEL()        : string = "reg0[13]"

-function rEG_ORIG_DIP_IPV4() : string = "reg1"

-function rEG_ORIG_DIP_IPV6()       : string = "xxreg1"
-function rEG_ORIG_TP_DPORT()       : string = "reg2[0..15]"

/* Register definitions for switches and routers. */

+function rEG_ORIG_TP_DPORT()       : string = "reg2[0..15]"
+function rEG_ORIG_DIP_IPV4()       : string = "reg1"
+function rEG_ORIG_DIP_IPV6()       : string = "xxreg1"

/* Indicate that this packet has been recirculated using egress

   * loopback.  This allows certain checks to be bypassed, such as a
@@ -3147,7 +3147,9 @@ function get_match_for_lb_key(ip_address: v46_ip,
                                port: bit<16>,
                                protocol: Option<string>,
                                redundancy: bool,
-                              use_nexthop_reg: bool): string = {
+                              use_nexthop_reg: bool,
+                              use_dest_tp_reg: bool,
+                              use_orig_dip_reg: bool): string = {
      var port_match = if (port != 0) {
          var proto = if (protocol == Some{"udp"}) {
              "udp"
@@ -3155,7 +3157,11 @@ function get_match_for_lb_key(ip_address: v46_ip,
              "tcp"
          };
          if (redundancy) { " && ${proto}" } else { "" } ++
-        " && ${proto}.dst == ${port}"
+        if (use_dest_tp_reg) {
+            " && ${rEG_ORIG_TP_DPORT()} == ${port}"
+        } else {
+            " && ${proto}.dst == ${port}"
+        }
      } else {
          ""
      };
@@ -3164,12 +3170,16 @@ function get_match_for_lb_key(ip_address: v46_ip,
          IPv4{ipv4} ->
              if (use_nexthop_reg) {
                  "${rEG_NEXT_HOP()} == ${ipv4}"
+            } else if (use_orig_dip_reg) {
+                "${rEG_ORIG_DIP_IPV4()} == ${ipv4}"
              } else {
                  "ip4.dst == ${ipv4}"
              },
          IPv6{ipv6} ->
              if (use_nexthop_reg) {
                  "xx${rEG_NEXT_HOP()} == ${ipv6}"
+            } else if (use_orig_dip_reg) {
+                "${rEG_ORIG_DIP_IPV6()} == ${ipv6}"
              } else {
                  "ip6.dst == ${ipv6}"
              }
@@ -3268,7 +3278,7 @@ Flow(.logical_datapath = sw._uuid,
      } else {
          None
      },
-    var __match = "ct.new && " ++ get_match_for_lb_key(lbvip.vip_addr, 
lbvip.vip_port, lb.protocol, false, false).
+    var __match = "ct.new && " ++ get_match_for_lb_key(lbvip.vip_addr, 
lbvip.vip_port, lb.protocol, false, false, false, false).

/* Ingress Pre-Hairpin/Nat-Hairpin/Hairpin tabled (Priority 0).

   * Packets that don't need hairpinning should continue processing.
@@ -6730,8 +6740,18 @@ for (RouterLBVIP(
                  (100, "")
              } in
          var __match = match1 ++ match2 in
-        var xx = ip_address.xxreg() in
-        var __actions = i"${xx}${rEG_NEXT_HOP()} = ${ip_address}; ct_dnat;" in
+        var actions1 =
+            match (ip_address) {
+                IPv4{_} -> "${rEG_ORIG_DIP_IPV4()} = ${ip_address}; ",
+                IPv6{_} -> "${rEG_ORIG_DIP_IPV6()} = ${ip_address}; "
+            } in
+        var actions2 =
+            if (port != 0) {
+                "${rEG_ORIG_TP_DPORT()} = ${proto}.dst; ct_dnat;"
+            } else {
+                "ct_dnat;"
+            } in
+        var __actions = actions1 ++ actions2 in
          /* One of these flows must be created for each unique LB VIP address.
           * We create one for each VIP:port pair; flows with the same IP and
           * different port numbers will produce identical flows that will
@@ -6740,7 +6760,7 @@ for (RouterLBVIP(
               .stage            = s_ROUTER_IN_DEFRAG(),
               .priority         = prio,
               .__match          = __match.intern(),
-             .actions          = __actions,
+             .actions          = __actions.intern(),
               .stage_hint       = 0,
               .io_port          = None,
               .controller_meter = None);
@@ -6751,10 +6771,14 @@ for (RouterLBVIP(
           * on ct.new with an action of "ct_lb($targets);".  The other
           * flow is for ct.est with an action of "ct_dnat;". */
          var xx = ip_address.xxreg() in
-        var match1 = "${ipX} && ${xx}${rEG_NEXT_HOP()} == ${ip_address}" in
+        var match1 =
+            match (ip_address) {
+                IPv4{_} -> "${ipX} && ${rEG_ORIG_DIP_IPV4()} == ${ip_address}",
+                IPv6{_} -> "${ipX} && ${rEG_ORIG_DIP_IPV6()} == ${ip_address}"
+            } in
          (var prio, var match2) =
              if (port != 0) {
-                (120, " && ${proto} && ${proto}.dst == ${port}")
+                (120, " && ${proto} && ${rEG_ORIG_TP_DPORT()} == ${port}")
              } else {
                  (110, "")
              } in
@@ -6766,12 +6790,7 @@ for (RouterLBVIP(
          var snat_for_lb = snat_for_lb(r.options, lb) in
          {
              /* A match and actions for established connections. */
-            var est_match = "ct.est && " ++ match1 ++ " && ct_label.natted == 
1" ++
-                if (port != 0) {
-                    " && ${proto}"
-                } else {
-                    ""
-                } ++
+            var est_match = "ct.est && " ++ match1 ++ match2 ++ " && 
ct_label.natted == 1" ++
                  match ((l3dgw_ports.nth(0), backends != "" or 
lb.options.get_bool_def("reject", false))) {
                      (Some {var gw_port}, true) -> " && 
is_chassis_resident(${json_string_escape(chassis_redirect_name(gw_port.name))})",
                      _ -> ""
@@ -6879,7 +6898,7 @@ Flow(.logical_datapath = r._uuid,
      r.load_balancer.contains(lb._uuid),
      var __match
          = "ct.new && " ++
-          get_match_for_lb_key(lbvip.vip_addr, lbvip.vip_port, lb.protocol, 
true, true) ++
+          get_match_for_lb_key(lbvip.vip_addr, lbvip.vip_port, lb.protocol, 
true, false, true, true) ++
            match (r.l3dgw_ports.nth(0)) {
                Some{gw_port} -> " && 
is_chassis_resident(${json_string_escape(chassis_redirect_name(gw_port.name))})",
                _ -> ""
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 93bb0e1da2f0..f845d643e85d 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3269,13 +3269,13 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], 
[dnl