diff mbox series

[ovs-dev,v2] northd: do not centralized traffic for unclaimed virtual ports

Message ID 44d35e75854001f0eb2ad4127c2a26b1f6a6b8f8.1624362145.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series [ovs-dev,v2] northd: do not centralized traffic for unclaimed virtual ports | expand

Commit Message

Lorenzo Bianconi June 22, 2021, 12:47 p.m. UTC
Add a rule to drop traffic from a distributed NAT if the virtual
port has not claimed yet becaused otherwise the traffic will be
centralized misconfiguring the TOR switch.

https://bugzilla.redhat.com/show_bug.cgi?id=1952961

Co-authored-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- add northd documentation
- add DDlog support (numan)
---
 northd/ovn-northd.8.xml |  7 +++++++
 northd/ovn-northd.c     | 23 ++++++++++++++++++-----
 northd/ovn_northd.dl    | 15 +++++++++++++++
 tests/ovn.at            | 26 ++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 5 deletions(-)

Comments

Dumitru Ceara July 22, 2021, 10:25 a.m. UTC | #1
On 6/22/21 2:47 PM, Lorenzo Bianconi wrote:
> Add a rule to drop traffic from a distributed NAT if the virtual
> port has not claimed yet becaused otherwise the traffic will be
> centralized misconfiguring the TOR switch.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1952961
> 
> Co-authored-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---

Hi Lorenzo,

Unfortunately this patch needs a small rebase (some rather easy to fix
conflicts in ovn-northd.c).

[...]

> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 3afa80a3b..08faf5da6 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -5555,6 +5555,10 @@ for (rp in &RouterPort(.router = &Router{._uuid = lr_uuid, .options = lr_options
>      }
>  }
>  
> +relation NATForVirtualLogicalPort(logical_port: Option<string>)
> +NATForVirtualLogicalPort(Some{logical_port}) :-
> +    lsp in &nb::Logical_Switch_Port(.name = logical_port, .__type = "virtual").
> +

A minor comment is that I would call this relation 'VirtualLogicalPort'
instead of 'NATForVirtualLogicalPort' because it's populated regardless
of NAT being configured or not and may be used in the future for other
types of rules.

With that addressed, please go ahead and add my ack to the rebased v3:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Regards,
Dumitru
diff mbox series

Patch

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 407464602..d048d487f 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3710,6 +3710,13 @@  icmp6 {
         external ip and <var>D</var> is NAT external mac.
       </li>
 
+      <li>
+        For each NAT rule in the OVN Northbound database that can
+        be handled in a distributed manner, a priority-80 logical flow
+        with drop action if the NAT logical port is a virtual port not
+        claimed by any chassis yet.
+      </li>
+
       <li>
         A priority-50 logical flow with match
         <code>outport == <var>GW</var></code> has actions
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 75484c5cd..44d1a910b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -11667,6 +11667,7 @@  lrouter_check_nat_entry(struct ovn_datapath *od, const struct nbrec_nat *nat,
 static void
 build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od,
                                 struct hmap *lflows,
+                                struct hmap *ports,
                                 struct shash *meter_groups,
                                 struct hmap *lbs,
                                 struct ds *match, struct ds *actions)
@@ -11774,10 +11775,21 @@  build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od,
             ds_clear(match);
             ds_clear(actions);
             ds_put_format(match,
-                          "ip%s.src == %s && outport == %s && "
-                          "is_chassis_resident(\"%s\")",
+                          "ip%s.src == %s && outport == %s",
                           is_v6 ? "6" : "4", nat->logical_ip,
-                          od->l3dgw_port->json_key, nat->logical_port);
+                          od->l3dgw_port->json_key);
+            /* Add a rule to drop traffic from a distributed NAT if
+             * the virtual port has not claimed yet becaused otherwise
+             * the traffic will be centralized misconfiguring the TOR switch.
+             */
+            struct ovn_port *op = ovn_port_find(ports, nat->logical_port);
+            if (op && op->nbsp && !strcmp(op->nbsp->type, "virtual")) {
+                ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
+                                        80, ds_cstr(match), "drop;",
+                                        &nat->header_);
+            }
+            ds_put_format(match, " && is_chassis_resident(\"%s\")",
+                          nat->logical_port);
             ds_put_format(actions, "eth.src = %s; %s = %s; next;",
                           nat->external_mac,
                           is_v6 ? REG_SRC_IPV6 : REG_SRC_IPV4,
@@ -11936,8 +11948,9 @@  build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od,
                                         &lsi->actions);
     build_misc_local_traffic_drop_flows_for_lrouter(od, lsi->lflows);
     build_lrouter_arp_nd_for_datapath(od, lsi->lflows);
-    build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->meter_groups,
-                                    lsi->lbs, &lsi->match, &lsi->actions);
+    build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->ports,
+                                    lsi->meter_groups, lsi->lbs, &lsi->match,
+                                    &lsi->actions);
 }
 
 /* Helper function to combine all lflow generation which is iterated by port.
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3afa80a3b..08faf5da6 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5555,6 +5555,10 @@  for (rp in &RouterPort(.router = &Router{._uuid = lr_uuid, .options = lr_options
     }
 }
 
+relation NATForVirtualLogicalPort(logical_port: Option<string>)
+NATForVirtualLogicalPort(Some{logical_port}) :-
+    lsp in &nb::Logical_Switch_Port(.name = logical_port, .__type = "virtual").
+
 /* NAT rules are only valid on Gateway routers and routers with
  * l3dgw_port (router has a port with "redirect-chassis"
  * specified). */
@@ -5900,6 +5904,17 @@  for (r in &Router(._uuid = lr_uuid,
                  .actions          = actions,
                  .external_ids     = stage_hint(nat.nat._uuid));
 
+            for (NATForVirtualLogicalPort(nat.nat.logical_port)) {
+                Some{var gwport} = l3dgw_port in
+                Flow(.logical_datapath = lr_uuid,
+                     .stage            = s_ROUTER_IN_GW_REDIRECT(),
+                    .priority         = 80,
+                    .__match          = "${ipX}.src == ${nat.nat.logical_ip} && "
+                                        "outport == ${json_string_escape(gwport.name)}",
+                    .actions          = "drop;",
+                    .external_ids     = stage_hint(nat.nat._uuid))
+            };
+
             /* Egress Loopback table: For NAT on a distributed router.
              * If packets in the egress pipeline on the distributed
              * gateway port have ip.dst matching a NAT external IP, then
diff --git a/tests/ovn.at b/tests/ovn.at
index bc494fcad..cbd2ce755 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -17172,6 +17172,16 @@  send_arp_reply() {
     as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
 }
 
+send_icmp_packet() {
+    local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ipv4_dst=$6 ip_chksum=$7 data=$8
+    shift 8
+
+    local ip_ttl=ff
+    local ip_len=001c
+    local packet=${eth_dst}${eth_src}08004500${ip_len}00004000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${data}
+    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
+}
+
 net_add n1
 
 sim_add hv1
@@ -17384,6 +17394,22 @@  logical_port=sw0-vir) = x])
 wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-vir
 
 check ovn-nbctl --wait=hv sync
+
+# verify the traffic from virtual port is discarded if the port is not claimed
+AT_CHECK([grep lr_in_gw_redirect lr0-flows2 | grep "ip4.src == 10.0.0.10"], [0], [dnl
+  table=17(lr_in_gw_redirect  ), priority=100  , match=(ip4.src == 10.0.0.10 && outport == "lr0-public" && is_chassis_resident("sw0-vir")), action=(eth.src = 10:54:00:00:00:10; reg1 = 172.168.0.50; next;)
+  table=17(lr_in_gw_redirect  ), priority=80   , match=(ip4.src == 10.0.0.10 && outport == "lr0-public"), action=(drop;)
+])
+
+eth_src=505400000003
+eth_dst=00000000ff01
+ip_src=$(ip_to_hex 10 0 0 10)
+ip_dst=$(ip_to_hex 172 168 0 101)
+send_icmp_packet 1 1 $eth_src $eth_dst $ip_src $ip_dst c4c9 0000000000000000000000
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | awk '/table=25, n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
+priority=80,ip,reg15=0x3,metadata=0x3,nw_src=10.0.0.10 actions=drop
+])
+
 # hv1 should remove the flow for the ACL with is_chassis_redirect check for sw0-vir.
 check_virtual_offlows_not_present hv1