northd: Use LB port_str in northd.

As mentioned in the structure definition in lb.h, northd should never use 'vip_port' instead it should always use 'port_str'. That's because it might not always be possible to evaluate the port value in northd, e.g., when using LB templates. This commit also updates other occurrences of 'vip_port' replacing them with 'port_str' in northd. Those were not causing issues because they were only in cases when templates are not supported (LB affinity for example) but it's better to have consistency in style. Fixes: 7b94d21 ("northd: Refactor build_lrouter_nat_flows_for_lb function") Signed-off-by: Dumitru Ceara <dceara@redhat.com> Acked-by: Ales Musil <amusil@redhat.com>
ovn-org · Mar 2, 2023 · 086744a · 086744a
1 parent e0fe7d3
commit 086744a
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 40 deletions.
diff --git a/northd/northd.c b/northd/northd.c
@@ -7179,9 +7179,9 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
     ds_put_format(&aff_action, "%s = %s; ", reg_vip, lb_vip->vip_str);
     ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \"");
 
-    if (lb_vip->vip_port) {
-        ds_put_format(&aff_action_learn, ipv6 ? "[%s]:%"PRIu16 : "%s:%"PRIu16,
-                      lb_vip->vip_str, lb_vip->vip_port);
+    if (lb_vip->port_str) {
+        ds_put_format(&aff_action_learn, ipv6 ? "[%s]:%s" : "%s:%s",
+                      lb_vip->vip_str, lb_vip->port_str);
     } else {
         ds_put_cstr(&aff_action_learn, lb_vip->vip_str);
     }
@@ -7193,12 +7193,12 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
     ds_put_cstr(&aff_action_learn, "\", backend = \"");
 
     /* Prepare common part of affinity learn match. */
-    if (lb_vip->vip_port) {
+    if (lb_vip->port_str) {
         ds_put_format(&aff_match_learn, REGBIT_KNOWN_LB_SESSION" == 0 && "
                       "ct.new && %s && %s == %s && "
-                      REG_ORIG_TP_DPORT_ROUTER" == %"PRIu16" && "
+                      REG_ORIG_TP_DPORT_ROUTER" == %s && "
                       "%s.dst == ", ip_match, reg_vip, lb_vip->vip_str,
-                      lb_vip->vip_port, ip_match);
+                      lb_vip->port_str, ip_match);
     } else {
         ds_put_format(&aff_match_learn, REGBIT_KNOWN_LB_SESSION" == 0 && "
                       "ct.new && %s && %s == %s && %s.dst == ", ip_match,
@@ -7244,7 +7244,7 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
         ds_put_cstr(&aff_action, ");");
         ds_put_char(&aff_action_learn, '"');
 
-        if (lb_vip->vip_port) {
+        if (lb_vip->port_str) {
             ds_put_format(&aff_action_learn, ", proto = %s", lb->proto);
         }
 
@@ -7334,9 +7334,9 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
                       lb_vip->vip_str);
     }
 
-    if (lb_vip->vip_port) {
-        ds_put_format(&new_lb_match, " && "REG_ORIG_TP_DPORT " == %"PRIu16,
-                      lb_vip->vip_port);
+    if (lb_vip->port_str) {
+        ds_put_format(&new_lb_match, " && "REG_ORIG_TP_DPORT " == %s",
+                      lb_vip->port_str);
     }
 
     static char *aff_check = REGBIT_KNOWN_LB_SESSION" = chk_lb_aff(); next;";
@@ -7363,11 +7363,11 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
                   reg_vip, lb_vip->vip_str);
     ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \"");
 
-    if (lb_vip->vip_port) {
-        ds_put_format(&aff_action, REG_ORIG_TP_DPORT" = %"PRIu16"; ",
-                      lb_vip->vip_port);
-        ds_put_format(&aff_action_learn, ipv6 ? "[%s]:%"PRIu16 : "%s:%"PRIu16,
-                      lb_vip->vip_str, lb_vip->vip_port);
+    if (lb_vip->port_str) {
+        ds_put_format(&aff_action, REG_ORIG_TP_DPORT" = %s; ",
+                      lb_vip->port_str);
+        ds_put_format(&aff_action_learn, ipv6 ? "[%s]:%s" : "%s:%s",
+                      lb_vip->vip_str, lb_vip->port_str);
     } else {
         ds_put_cstr(&aff_action_learn, lb_vip->vip_str);
     }
@@ -7376,12 +7376,12 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
     ds_put_cstr(&aff_action_learn, "\", backend = \"");
 
     /* Prepare common part of affinity learn match. */
-    if (lb_vip->vip_port) {
+    if (lb_vip->port_str) {
         ds_put_format(&aff_match_learn, REGBIT_KNOWN_LB_SESSION" == 0 && "
                       "ct.new && %s && %s == %s && "
-                      REG_ORIG_TP_DPORT" == %"PRIu16" && %s.dst == ",
+                      REG_ORIG_TP_DPORT" == %s && %s.dst == ",
                       ip_match, reg_vip, lb_vip->vip_str,
-                      lb_vip->vip_port, ip_match);
+                      lb_vip->port_str, ip_match);
     } else {
         ds_put_format(&aff_match_learn, REGBIT_KNOWN_LB_SESSION" == 0 && "
                       "ct.new && %s && %s == %s && %s.dst == ",
@@ -7422,7 +7422,7 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
         ds_put_cstr(&aff_action, ");");
         ds_put_char(&aff_action_learn, '"');
 
-        if (lb_vip->vip_port) {
+        if (lb_vip->port_str) {
             ds_put_format(&aff_action_learn, ", proto = %s", lb->proto);
         }
 
@@ -10622,10 +10622,10 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
      */
     ds_put_format(match, "ct.new && !ct.rel && %s && %s == %s",
                   ip_match, ip_reg, lb_vip->vip_str);
-    if (lb_vip->vip_port) {
+    if (lb_vip->port_str) {
         prio = 120;
-        ds_put_format(match, " && %s && "REG_ORIG_TP_DPORT_ROUTER" == %d",
-                      lb->proto, lb_vip->vip_port);
+        ds_put_format(match, " && %s && "REG_ORIG_TP_DPORT_ROUTER" == %s",
+                      lb->proto, lb_vip->port_str);
     }
 
     ds_put_cstr(&est_match, "ct.est");

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
@@ -9612,6 +9612,7 @@ AT_CLEANUP
 
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([load-balancer template IPv4])
+AT_SKIP_IF([test $HAVE_TCPDUMP = no])
 AT_SKIP_IF([test $HAVE_NC = no])
 AT_KEYWORDS([ovnlb templates])
 
@@ -9638,8 +9639,8 @@ start_daemon ovn-controller
 #         |
 # VM2 ----+
 #
-# A templated load balancer applied on LS1 and GW-Router with
-# VM1 as backend.  The VIP should be accessible from both VM2 and VM3.
+# Two templated load balancer applied on LS1 and GW-Router with
+# VM1 as backend.  The VIPs should be accessible from both VM2 and VM3.
 
 check ovn-nbctl                                                   \
     -- lr-add rtr                                                 \
@@ -9660,15 +9661,22 @@ check ovn-nbctl                                                   \
     -- lsp-set-options ls2-rtr router-port=rtr-ls2                \
     -- lsp-add ls2 vm3 -- lsp-set-addresses vm3 00:00:00:00:00:03
 
-# Add a template LB that eventually expands to:
+# Add a TCP template LB that eventually expands to:
 # VIP=66.66.66.66:666 backends=42.42.42.2:4242 proto=tcp
+# And a UDP template LB that eventually expands to:
+# VIP=66.66.66.66:777 backends=42.42.42.2:4343 proto=udp
 
-AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" variables="{vip=66.66.66.66,vport=666,backends=\"42.42.42.2:4242\"}"],
+AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" \
+    variables="{vip=66.66.66.66,vport1=666,backends1=\"42.42.42.2:4242\",vport2=777,backends2=\"42.42.42.2:4343\"}"],
          [0], [ignore])
 
-check ovn-nbctl --template lb-add lb-test "^vip:^vport" "^backends" tcp \
-    -- ls-lb-add ls1 lb-test                                            \
-    -- lr-lb-add rtr lb-test
+check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1" tcp \
+    -- ls-lb-add ls1 lb-test-tcp                                              \
+    -- lr-lb-add rtr lb-test-tcp
+
+check ovn-nbctl --template lb-add lb-test-udp "^vip:^vport2" "^backends2" udp \
+    -- ls-lb-add ls1 lb-test-udp                                              \
+    -- lr-lb-add rtr lb-test-udp
 
 ADD_NAMESPACES(vm1)
 ADD_VETH(vm1, vm1, br-int, "42.42.42.2/24", "00:00:00:00:00:01", "42.42.42.1")
@@ -9685,24 +9693,40 @@ check ovn-nbctl --wait=hv sync
 
 AT_CHECK([ovn-appctl -t ovn-controller debug/dump-local-template-vars | sort], [0], [dnl
 Local template vars:
-name: 'backends' value: '42.42.42.2:4242'
+name: 'backends1' value: '42.42.42.2:4242'
+name: 'backends2' value: '42.42.42.2:4343'
 name: 'vip' value: '66.66.66.66'
-name: 'vport' value: '666'
+name: 'vport1' value: '666'
+name: 'vport2' value: '777'
 ])
 
 # Start IPv4 TCP server on vm1.
 NETNS_DAEMONIZE([vm1], [nc -k -l 42.42.42.2 4242], [nc-vm1.pid])
 
+NETNS_DAEMONIZE([vm1],
+    [tcpdump -n -i vm1 -nnleX -c3 udp and dst 42.42.42.2 and dst port 4343 > vm1.pcap 2>/dev/null],
+    [tcpdump1.pid])
+
 # Make sure connecting to the VIP works (hairpin, via ls and via lr).
 NS_CHECK_EXEC([vm1], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
 NS_CHECK_EXEC([vm2], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
 NS_CHECK_EXEC([vm3], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
 
+NS_CHECK_EXEC([vm1], [echo a | nc -u 66.66.66.66 777 &], [0])
+NS_CHECK_EXEC([vm2], [echo a | nc -u 66.66.66.66 777 &], [0])
+NS_CHECK_EXEC([vm3], [echo a | nc -u 66.66.66.66 777 &], [0])
+
+OVS_WAIT_UNTIL([
+    requests=`grep "UDP" -c vm1.pcap`
+    test "${requests}" -ge "3"
+])
+
 AT_CLEANUP
 ])
 
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([load-balancer template IPv6])
+AT_SKIP_IF([test $HAVE_TCPDUMP = no])
 AT_SKIP_IF([test $HAVE_NC = no])
 AT_KEYWORDS([ovnlb templates])
 
@@ -9729,8 +9753,8 @@ start_daemon ovn-controller
 #         |
 # VM2 ----+
 #
-# A templated load balancer applied on LS1 and GW-Router with
-# VM1 as backend.  The VIP should be accessible from both VM2 and VM3.
+# Two templated load balancer applied on LS1 and GW-Router with
+# VM1 as backend.  The VIPs should be accessible from both VM2 and VM3.
 
 check ovn-nbctl                                                   \
     -- lr-add rtr                                                 \
@@ -9752,14 +9776,21 @@ check ovn-nbctl                                                   \
     -- lsp-add ls2 vm3 -- lsp-set-addresses vm3 00:00:00:00:00:03
 
 # Add a template LB that eventually expands to:
-# VIP=6666::1 backends=[4242::2]:4242 proto=tcp
+# VIP=[6666::1]:666 backends=[4242::2]:4242 proto=tcp
+# Add a template LB that eventually expands to:
+# VIP=[6666::1]:777 backends=[4242::2]:4343 proto=udp
 
-AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" variables="{vip=\"6666::1\",vport=666,backends=\"[[4242::2]]:4242\"}"],
+AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" \
+    variables="{vip=\"6666::1\",vport1=666,backends1=\"[[4242::2]]:4242\",vport2=777,backends2=\"[[4242::2]]:4343\"}"],
          [0], [ignore])
 
-check ovn-nbctl --template lb-add lb-test "^vip:^vport" "^backends" tcp ipv6 \
-    -- ls-lb-add ls1 lb-test                                                 \
-    -- lr-lb-add rtr lb-test
+check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1" tcp ipv6 \
+    -- ls-lb-add ls1 lb-test-tcp                                                   \
+    -- lr-lb-add rtr lb-test-tcp
+
+check ovn-nbctl --template lb-add lb-test-udp "^vip:^vport2" "^backends2" udp ipv6 \
+    -- ls-lb-add ls1 lb-test-udp                                                   \
+    -- lr-lb-add rtr lb-test-udp
 
 ADD_NAMESPACES(vm1)
 ADD_VETH(vm1, vm1, br-int, "4242::2/64", "00:00:00:00:00:01", "4242::1")
@@ -9779,19 +9810,34 @@ check ovn-nbctl --wait=hv sync
 
 AT_CHECK([ovn-appctl -t ovn-controller debug/dump-local-template-vars | sort], [0], [dnl
 Local template vars:
-name: 'backends' value: '[[4242::2]]:4242'
+name: 'backends1' value: '[[4242::2]]:4242'
+name: 'backends2' value: '[[4242::2]]:4343'
 name: 'vip' value: '6666::1'
-name: 'vport' value: '666'
+name: 'vport1' value: '666'
+name: 'vport2' value: '777'
 ])
 
 # Start IPv6 TCP server on vm1.
 NETNS_DAEMONIZE([vm1], [nc -k -l 4242::2 4242], [nc-vm1.pid])
 
+NETNS_DAEMONIZE([vm1],
+    [tcpdump -n -i vm1 -nnleX -c3 udp and dst 4242::2 and dst port 4343 > vm1.pcap 2>/dev/null],
+    [tcpdump1.pid])
+
 # Make sure connecting to the VIP works (hairpin, via ls and via lr).
 NS_CHECK_EXEC([vm1], [nc 6666::1 666 -z], [0], [ignore], [ignore])
 NS_CHECK_EXEC([vm2], [nc 6666::1 666 -z], [0], [ignore], [ignore])
 NS_CHECK_EXEC([vm3], [nc 6666::1 666 -z], [0], [ignore], [ignore])
 
+NS_CHECK_EXEC([vm1], [echo a | nc -u 6666::1 777 &], [0])
+NS_CHECK_EXEC([vm2], [echo a | nc -u 6666::1 777 &], [0])
+NS_CHECK_EXEC([vm3], [echo a | nc -u 6666::1 777 &], [0])
+
+OVS_WAIT_UNTIL([
+    requests=`grep "UDP" -c vm1.pcap`
+    test "${requests}" -ge "3"
+])
+
 AT_CLEANUP
 ])