diff mbox series

[ovs-dev,v2] northd: match only on supported protocols to handle_svc_check

Message ID 20230530124157.1223591-1-odivlad@gmail.com
State Accepted
Headers show
Series [ovs-dev,v2] northd: match only on supported protocols to handle_svc_check | expand

Checks

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

Commit Message

Vladislav Odintsov May 30, 2023, 12:41 p.m. UTC 
Depending on the udp service, it can reply with some udp data.
In that case ovn-controller will warn with next message:

pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol - [11]

This is not something abnormal, so it needs to be fixed.
With this patch ovn-northd changes match of appropriate lflow, which sends
traffic to ovn-controller's pinctrl thread to handle_svc_check action.
Now only supported protocols allowed to reach ovn-controller when destined
to $svc_monitor_mac (tcp, icmp, icmpv6).

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
---
v1 -> v2:
  - Addressed Dumitru's suggestion to match on supported protocols
    instead of validating protocol inside pinctrl thread.
---
 northd/northd.c     |  2 +-
 tests/ovn-northd.at | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 16 deletions(-)

Comments

Dumitru Ceara June 8, 2023, 1:31 p.m. UTC | #1 
On 5/30/23 14:41, Vladislav Odintsov wrote:
> Depending on the udp service, it can reply with some udp data.
> In that case ovn-controller will warn with next message:
> 
> pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol - [11]
> 
> This is not something abnormal, so it needs to be fixed.
> With this patch ovn-northd changes match of appropriate lflow, which sends
> traffic to ovn-controller's pinctrl thread to handle_svc_check action.
> Now only supported protocols allowed to reach ovn-controller when destined
> to $svc_monitor_mac (tcp, icmp, icmpv6).
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
> ---
> v1 -> v2:
>   - Addressed Dumitru's suggestion to match on supported protocols
>     instead of validating protocol inside pinctrl thread.
> ---

Applied to main and backported to all stable branches down to 22.03, thanks!
Vladislav Odintsov June 8, 2023, 2:15 p.m. UTC | #2 
Thanks, Dumitru!

> On 8 Jun 2023, at 16:31, Dumitru Ceara <dceara@redhat.com> wrote:
> 
> On 5/30/23 14:41, Vladislav Odintsov wrote:
>> Depending on the udp service, it can reply with some udp data.
>> In that case ovn-controller will warn with next message:
>> 
>> pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol - [11]
>> 
>> This is not something abnormal, so it needs to be fixed.
>> With this patch ovn-northd changes match of appropriate lflow, which sends
>> traffic to ovn-controller's pinctrl thread to handle_svc_check action.
>> Now only supported protocols allowed to reach ovn-controller when destined
>> to $svc_monitor_mac (tcp, icmp, icmpv6).
>> 
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
>> ---
>> v1 -> v2:
>>  - Addressed Dumitru's suggestion to match on supported protocols
>>    instead of validating protocol inside pinctrl thread.
>> ---
> 
> Applied to main and backported to all stable branches down to 22.03, thanks!
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org <mailto:dev@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Regards,
Vladislav Odintsov
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index a6eca916b..c459887b7 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9124,7 +9124,7 @@  build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
     ovs_assert(od->nbs);
 
     ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
-                  "eth.dst == $svc_monitor_mac",
+                  "eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)",
                   "handle_svc_check(inport);");
 
     struct mcast_switch_info *mcast_sw_info = &od->mcast_info.sw;
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index e3669bdf5..8eadad8bf 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4974,7 +4974,7 @@  check ovn-nbctl lsp-set-options ls2-ro2 router-port=ro2-ls2
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -4986,7 +4986,7 @@  AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort],
 ovn-sbctl lflow-list ls2 > ls2_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:01), action=(outport = "ls2-ro2"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:02), action=(outport = "vm2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5006,7 +5006,7 @@  check ovn-nbctl --wait=sb lr-nat-add ro2 snat 20.0.0.200 192.168.2.200/30
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5019,7 +5019,7 @@  AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort],
 ovn-sbctl lflow-list ls2 > ls2_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:01), action=(outport = "ls2-ro2"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:02), action=(outport = "vm2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5040,7 +5040,7 @@  check ovn-nbctl --wait=sb lr-nat-add ro2 snat 40.0.0.200 192.168.2.148/30
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5054,7 +5054,7 @@  AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort],
 ovn-sbctl lflow-list ls2 > ls2_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:01), action=(outport = "ls2-ro2"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:02:02), action=(outport = "vm2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5073,7 +5073,7 @@  ovn-nbctl --wait=sb lr-lb-add ro1 lb1
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5091,7 +5091,7 @@  ovn-nbctl --wait=sb lb-add lb1 192.168.4.100:80 10.0.0.10:80
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -5115,7 +5115,7 @@  ovn-nbctl --wait=sb lrp-set-gateway-chassis ro1-ls1 chassis-1 30
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -8195,7 +8195,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=0    , match=(1), action=(output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
   table=??(ls_in_l2_unknown   ), priority=0    , match=(1), action=(output;)
   table=??(ls_in_l2_unknown   ), priority=50   , match=(outport == "none"), action=(drop;)
@@ -8220,7 +8220,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=0    , match=(1), action=(output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:01), action=(outport = "sw0p1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:02), action=(outport = "sw0p2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -8246,7 +8246,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=0    , match=(1), action=(output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:01), action=(outport = "sw0p1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:02), action=(outport = "sw0p2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -8273,7 +8273,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=0    , match=(1), action=(output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:01), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:02), action=(outport = "sw0p2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -8301,7 +8301,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=110  , match=(outport == "localnetport" && inport == "sw0p2"), action=(set_queue(10); output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:01), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:02), action=(outport = "sw0p2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)
@@ -8333,7 +8333,7 @@  sort | sed 's/table=../table=??/' ], [0], [dnl
   table=??(ls_out_apply_port_sec), priority=110  , match=(outport == "localnetport" && inport == "sw0p2"), action=(set_queue(10); output;)
   table=??(ls_out_apply_port_sec), priority=50   , match=(reg0[[15]] == 1), action=(drop;)
   table=??(ls_in_l2_lkup      ), priority=0    , match=(1), action=(outport = get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup      ), priority=110  , match=(eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:01), action=(outport = "sw0p1"; output;)
   table=??(ls_in_l2_lkup      ), priority=50   , match=(eth.dst == 00:00:00:00:00:02), action=(outport = "sw0p2"; output;)
   table=??(ls_in_l2_lkup      ), priority=70   , match=(eth.mcast), action=(outport = "_MC_flood"; output;)