diff mbox

[ovs-dev] ovn-northd: logical router icmp response should not care about inport

Message ID 1463784792-27011-1-git-send-email-flavio@flaviof.com
State Superseded
Headers show

Commit Message

Flaviof May 20, 2016, 10:53 p.m. UTC
When responding to icmp echo requests (aka ping) packets, the logical
router should not restrict responses based on the inport.

Example diagram:

vm: IP1.1 (subnet1)
logical_router: IP1.2 (subnet1) and IP2.2 (subnet2)

   vm -------[subnet1]------- logical_router -------[subnet2]
   <IP1.1>                <IP1.2>        <IP2.2>

vm should be able to ping <IP1.2>, even though it is an address
of a subnet that can only be reached through L3 routing.

Reference to the mailing list thread:
http://openvswitch.org/pipermail/discuss/2016-May/021172.html

Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
---

TO BE IMPLEMENTED:

Unit tests. To be done as a separate patch.

TO BE DISCUSSED:

One caveat here is that ttl should be decremented before vm
reaches <IP2.2> and that logic is not invoked until later
in the pipeline. Further work may be necessary in order
to make ip.ttl part of the match in the logical table, so
pings from the non-local subnet are only responded if ttl > 1.
As far as I can tell, the match on logical flows currently does
not handle ip.ttl.

In short, instead of simply removing the inport match in the icmp rule,
we may actually add a second rule that would do something like:

  "inport == %s && (ip4.dst == "IP_FMT" || ip4.dst == "IP_FMT") && ... <= priority 90
  "ip.ttl > 1 && (ip4.dst == "IP_FMT" || ip4.dst == "IP_FMT") && ... <= priority 90-X



 ovn/northd/ovn-northd.c |  8 +++++---
 tests/ovn.at            | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)
diff mbox

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 44e9430..170cad8 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1892,11 +1892,13 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
         free(match);
 
         /* ICMP echo reply.  These flows reply to ICMP echo requests
-         * received for the router's IP address. */
+         * received for the router's IP address. Since packets only
+         * get here as part of the logical router datapath, the inport
+         * should not matter. */
         match = xasprintf(
-            "inport == %s && (ip4.dst == "IP_FMT" || ip4.dst == "IP_FMT") && "
+            "(ip4.dst == "IP_FMT" || ip4.dst == "IP_FMT") && "
             "icmp4.type == 8 && icmp4.code == 0",
-            op->json_key, IP_ARGS(op->ip), IP_ARGS(op->bcast));
+            IP_ARGS(op->ip), IP_ARGS(op->bcast));
         char *actions = xasprintf(
             "ip4.dst = ip4.src; "
             "ip4.src = "IP_FMT"; "
diff --git a/tests/ovn.at b/tests/ovn.at
index e6ac1d7..2addb45 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -657,6 +657,25 @@  test_arp() {
     fi
 }
 
+# test_ipv4_icmp_request INPORT SHA SPA TPA IPV4_SRC IPV4_DST [ICMPV4_CODE]
+#
+# Causes a packet to be received on INPORT.  The packet is an ICMP
+# query with SHA, SPA, TPA, IPV4_SRC and IPV4_DST as specified.  If ICMPV4_CODE
+# is provided, then it should be the icmp code of the packet responded;
+# otherwise no reply is expected.
+#
+# INPORT is an lport number, e.g. 11 for vif11.
+# SHA and REPLY_HA are each 12 hex digits.
+# SPA and TPA are each 8 hex digits.
+# IPV4_SRC and IPV4_DST are each 8 hex digits.
+# ICMPV4_CODE is 2 hex digits.
+test_ipv4_icmp_request() {
+    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
+
+    # TODO (FF): Implement this!
+
+}
+
 ip_to_hex() {
     printf "%02x%02x%02x%02x" "$@"
 }
@@ -758,6 +777,10 @@  for is in 1 2 3; do
         test_packet $s 010000000000 f000000000$s 1236 $bacl3         #8, acl3
     done
 done
+#
+# 11. icmp echo packets to logical router interfaces.
+#
+
 
 # set address for lp13 with invalid characters.
 # lp13 should be configured with only 192.168.0.13.