diff mbox series

[ovs-dev,branch-22.12] northd: Drop packets for LBs with no backends

Message ID 20230329064606.89453-1-amusil@redhat.com
State Rejected
Headers show
Series [ovs-dev,branch-22.12] northd: Drop packets for LBs with no backends | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil March 29, 2023, 6:46 a.m. UTC 
When the LB is configured without any backend
and doesn't report event or reject the packet
just simply drop the packet.

Reported-at: https://bugzilla.redhat.com/2177173
Signed-off-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
(cherry picked from commit 75b0bcbb019a8caa2cfebffeff1ecf155fcd1fbc)
---
This version applies cleanly down to 21.12.
---
 northd/northd.c     |  4 ++++
 tests/ovn-northd.at | 50 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

Comments

0-day Robot March 29, 2023, 6:57 a.m. UTC | #1 
Bleep bloop.  Greetings Ales Musil, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Dumitru Ceara <dceara@redhat.com>
Lines checked: 105, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Dumitru Ceara March 30, 2023, 11:45 a.m. UTC | #2 
On 3/29/23 08:46, Ales Musil wrote:
> When the LB is configured without any backend
> and doesn't report event or reject the packet
> just simply drop the packet.
> 
> Reported-at: https://bugzilla.redhat.com/2177173
> Signed-off-by: Ales Musil <amusil@redhat.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> (cherry picked from commit 75b0bcbb019a8caa2cfebffeff1ecf155fcd1fbc)
> ---
> This version applies cleanly down to 21.12.
> ---

Thanks for the backport, Ales!  But, as 0-day bot also points out, this
fails CI:

https://mail.openvswitch.org/pipermail/ovs-build/2023-March/029516.html
https://github.com/ovsrobot/ovn/actions/runs/4551145516/jobs/8024882262

## ----------------------- ##
## ovn 22.12.1 test suite. ##
## ----------------------- ##
994. ovn-northd.at:5066: testing ovn -- LR NAT flows -- ovn-northd -- parallelization=yes ...
[...]
+++ /home/dceara/git-repos/ovn/tests/testsuite.dir/at-groups/994/stdout 2023-03-30 13:31:49.436472934 +0200
@@ -1,6 +1,6 @@
   table=7 (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
   table=7 (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel && ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), action=(flags.skip_snat_for_lb = 1; next;)
-  table=7 (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel && ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel && ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;);)
[...]

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 3165f4bc2..6866fb6b5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3912,6 +3912,10 @@  build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
         }
     } else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
         reject = true;
+    } else if (!lb_vip->empty_backend_rej && !lb_vip->n_backends) {
+        ds_clear(action);
+        ds_put_cstr(action, debug_drop_action());
+        skip_hash_fields = true;
     } else {
         ds_put_format(action, "%s(backends=%s);", ct_lb_action,
                       lb_vip_nb->backend_ips);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5a4c6a31f..5fea88f6f 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4253,6 +4253,15 @@  AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
   table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;)
 ])
 
+# LB with event=false and reject=false
+AT_CHECK([ovn-nbctl create load_balancer name=lb1 options:reject=false options:event=false vips:\"10.0.0.20\"=\"\" protocol=tcp], [0], [ignore])
+check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
+
+AT_CHECK([ovn-sbctl dump-flows sw0 | grep "ls_in_lb " | sort ], [0], [dnl
+  table=12(ls_in_lb           ), priority=0    , match=(1), action=(next;)
+  table=12(ls_in_lb           ), priority=110  , match=(ct.new && ip4.dst == 10.0.0.20), action=(drop;)
+])
+
 AT_CLEANUP
 ])
 
@@ -5629,6 +5638,47 @@  AT_CHECK([grep "lr_out_snat" lr0flows | sed 's/table=./table=?/' | sort], [0], [
   table=? (lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
 ])
 
+# LB with event=false and reject=false
+check ovn-nbctl lr-lb-del lr0
+check ovn-nbctl remove logical_router lr0 options lb_force_snat_ip
+AT_CHECK([ovn-nbctl create load_balancer name=lb6 options:reject=false options:event=false vips:\"172.168.10.30\"=\"\" protocol=tcp], [0], [ignore])
+check ovn-nbctl --wait=sb lr-lb-add lr0 lb6
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel && ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), action=(next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel && ip4 && reg0 == 172.168.10.30), action=(drop;)
+  table=7 (lr_in_dnat         ), priority=50   , match=(ct.rel && !ct.est && !ct.new), action=(ct_commit_nat;)
+  table=7 (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=7 (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;)
+])
+
+# LB with event=false, reject=false and skip_snat
+check ovn-nbctl --wait=sb set load_balancer lb6 options:skip_snat=true
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel && ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), action=(flags.skip_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel && ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;)
+  table=7 (lr_in_dnat         ), priority=50   , match=(ct.rel && !ct.est && !ct.new), action=(ct_commit_nat;)
+  table=7 (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=7 (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;)
+])
+
+check ovn-nbctl remove load_balancer lb6 options skip_snat
+
+# LB with event=false, reject=false and force_snat
+check ovn-nbctl --wait=sb set logical_router lr0 options:lb_force_snat_ip="router_ip"
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel && ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), action=(flags.force_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel && ip4 && reg0 == 172.168.10.30), action=(flags.force_snat_for_lb = 1; drop;)
+  table=7 (lr_in_dnat         ), priority=50   , match=(ct.rel && !ct.est && !ct.new), action=(ct_commit_nat;)
+  table=7 (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=7 (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
 ])