DHCP client extends its lease by unicasting a DHCPREQUEST to its
original DHCP server when the T1 timer fires (typically at 50% of
the lease).  If that DHCPREQUEST gets no response, the T2 timer
fires (typically at 87.5% of the lease) and the client enters the
REBINDING state, where it broadcasts a DHCPREQUEST using its
currently leased IP as the source address (rather than 0.0.0.0).

Today OVN's DHCP relay only matches DHCPREQUEST packets whose source
IP is 0.0.0.0, so the broadcast DHCPREQUEST sent by a client in the
REBINDING state is not matched by any relay flow and is dropped.
The lease cannot be renewed through the relay and the client
eventually falls back to a fresh DISCOVER.

Extend the relay to also forward DHCPREQUEST packets whose source IP
lies in the relay LRP's subnet by widening the source-IP match of
the DHCP relay request flows to the set
ip4.src == {0.0.0.0, <lrp_cidr>}, where <lrp_cidr> is the CIDR of
the relay Logical_Router_Port.  The set is applied to:

- The priority-100 logical switch flow in ls_in_l2_lkup that
forwards client DHCP broadcasts to the relay LRP-attachment
port.
- The logical router lr_in_ip_input priority-110
dhcp_relay_req_chk flow.
- The lr_in_dhcp_relay_req priority-100 forward and priority-1
drop flows.

Fixes: dce4abfc68eb ("northd, tests: DHCP Relay Agent support for overlay IPv4 
subnets.")
Signed-off-by: Naveen Yerramneni <[email protected]>
Acked-by: Aditya Mehakare <[email protected]>
Acked-by: Huzaifa Calcuttawala <[email protected]>
---
V2:
  - consider dhcp_relay_handle_rebind as optional to avoid unwanted
    northd recompute.
V3:
  - Addresed review comments from Dumitru.
---
 Documentation/ref/ovn-logical-flows.7.rst | 28 ++++++++++++----------
 northd/northd.c                           | 29 +++++++++++++++--------
 tests/ovn-northd.at                       | 14 +++++------
 tests/ovn.at                              |  9 +++++--
 4 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/Documentation/ref/ovn-logical-flows.7.rst 
b/Documentation/ref/ovn-logical-flows.7.rst
index b60f8609c..7f83e5fe9 100644
--- a/Documentation/ref/ovn-logical-flows.7.rst
+++ b/Documentation/ref/ovn-logical-flows.7.rst
@@ -1382,7 +1382,8 @@ This table implements switching behavior.  It contains 
these logical flows:
 
 - A priority-100 flow that forwards all DHCP broadcast packets coming from VIFs
   to the logical router port's MAC when DHCP relay is enabled on the logical
-  switch.
+  switch.  The ``ip4.src`` match is the set ``{0.0.0.0, lrp_cidr}``, where
+  ``lrp_cidr`` is the CIDR of the relay logical router port.
 
 - A priority-100 flow that matches ``reg8[23] == 1`` and does ``output`` 
action.
   This ensures that packets that got injected back into this table from egress
@@ -2194,9 +2195,10 @@ contains the following flows to implement very basic IP 
host functionality.
 - For each logical router port configured with DHCP relay the following
   priority-110 flows are added to manage the DHCP relay traffic:
 
-  - if ``inport`` is lrp and ``ip4.src == 0.0.0.0`` and ``ip4.dst ==
-    255.255.255.255`` and ``ip4.frag == 0`` and ``udp.src == 68`` and ``udp.dst
-    == 67``, the ``dhcp_relay_req_chk`` action is executed. ::
+  - if ``inport`` is lrp and ``ip4.src == {0.0.0.0, lrp_cidr}`` and ``ip4.dst
+    == 255.255.255.255`` and ``ip4.frag == 0`` and ``udp.src == 68`` and
+    ``udp.dst == 67``, the ``dhcp_relay_req_chk`` action is executed.
+    ``lrp_cidr`` is the CIDR of the relay logical router port. ::
 
         reg9[7] = dhcp_relay_req_chk(lrp_ip, dhcp_server_ip);next
 
@@ -2494,10 +2496,11 @@ This stage process the DHCP request packets on which 
``dhcp_relay_req_chk``
 action is applied in the IP input stage.
 
 - A priority-100 logical flow is added for each logical router port configured
-  with DHCP relay that matches ``inport`` is lrp and ``ip4.src == 0.0.0.0`` and
-  ``ip4.dst == 255.255.255.255`` and ``udp.src == 68`` and ``udp.dst == 67`` 
and
-  ``reg9[7] == 1`` and applies following actions. If ``reg9[7]`` is set to 1
-  then, ``dhcp_relay_req_chk`` action was successful. ::
+  with DHCP relay that matches ``inport`` is lrp and ``ip4.src == {0.0.0.0,
+  lrp_cidr}`` and ``ip4.dst == 255.255.255.255`` and ``udp.src == 68`` and
+  ``udp.dst == 67`` and ``reg9[7] == 1`` and applies following actions. If
+  ``reg9[7]`` is set to 1 then, ``dhcp_relay_req_chk`` action was successful.
+  ``lrp_cidr`` is the CIDR of the relay logical router port. ::
 
       ip4.src=lrp ip;
       ip4.dst=dhcp server ip;
@@ -2505,10 +2508,11 @@ action is applied in the IP input stage.
       next;
 
 - A priority-1 logical flow is added for each logical router port configured
-  with DHCP relay that matches ``inport`` is lrp and ``ip4.src == 0.0.0.0`` and
-  ``ip4.dst == 255.255.255.255`` and ``udp.src == 68`` and ``udp.dst == 67`` 
and
-  ``reg9[7] == 0`` and drops the packet. If ``reg9[7]`` is set to 0 then,
-  ``dhcp_relay_req_chk`` action was unsuccessful.
+  with DHCP relay that matches ``inport`` is lrp and ``ip4.src == {0.0.0.0,
+  lrp_cidr}`` and ``ip4.dst == 255.255.255.255`` and ``udp.src == 68`` and
+  ``udp.dst == 67`` and ``reg9[7] == 0`` and drops the packet. If ``reg9[7]``
+  is set to 0 then, ``dhcp_relay_req_chk`` action was unsuccessful.
+  ``lrp_cidr`` is the CIDR of the relay logical router port.
 
 - A priority-0 flow that matches all packets to advance to the next table.
 
diff --git a/northd/northd.c b/northd/northd.c
index 0ea7c1b95..74ec487b3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10058,7 +10058,8 @@ build_lswitch_dhcp_relay_flows(struct ovn_port *op,
     }
 
     struct ovn_port *rp = sp->peer;
-    if (!rp || !rp->nbrp || !rp->nbrp->dhcp_relay || rp->peer != sp) {
+    if (!rp || !rp->nbrp || !rp->nbrp->dhcp_relay || rp->peer != sp ||
+        !rp->lrp_networks.n_ipv4_addrs) {
         return;
     }
 
@@ -10082,12 +10083,14 @@ build_lswitch_dhcp_relay_flows(struct ovn_port *op,
 
     ds_put_format(
         match, "inport == %s && eth.src == %s && "
-        "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
+        "ip4.src == {0.0.0.0, %s/%u} && ip4.dst == 255.255.255.255 && "
         "udp.src == 68 && udp.dst == 67",
-        op->json_key, op->lsp_addrs[0].ea_s);
+        op->json_key, op->lsp_addrs[0].ea_s,
+        rp->lrp_networks.ipv4_addrs[0].network_s,
+        rp->lrp_networks.ipv4_addrs[0].plen);
     ds_put_format(actions,
                   "eth.dst = %s; outport = %s; next; /* DHCP_RELAY_REQ */",
-                  rp->lrp_networks.ea_s,sp->json_key);
+                  rp->lrp_networks.ea_s, sp->json_key);
     ovn_lflow_add(lflows, op->od,
                   S_SWITCH_IN_L2_LKUP, 100,
                   ds_cstr(match),
@@ -16701,9 +16704,11 @@ build_dhcp_relay_flows_for_lrouter_port(struct 
ovn_port *op,
 
     ds_put_format(
         match, "inport == %s && "
-        "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
+        "ip4.src == {0.0.0.0, %s/%u} && ip4.dst == 255.255.255.255 && "
         "ip.frag == 0 && udp.src == 68 && udp.dst == 67",
-        op->json_key);
+        op->json_key,
+        op->lrp_networks.ipv4_addrs[0].network_s,
+        op->lrp_networks.ipv4_addrs[0].plen);
     ds_put_format(actions,
                   REGBIT_DHCP_RELAY_REQ_CHK
                   " = dhcp_relay_req_chk(%s, %s);"
@@ -16722,10 +16727,12 @@ build_dhcp_relay_flows_for_lrouter_port(struct 
ovn_port *op,
 
     ds_put_format(
         match, "inport == %s && "
-        "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
+        "ip4.src == {0.0.0.0, %s/%u} && ip4.dst == 255.255.255.255 && "
         "udp.src == 68 && udp.dst == 67 && "
         REGBIT_DHCP_RELAY_REQ_CHK,
-        op->json_key);
+        op->json_key,
+        op->lrp_networks.ipv4_addrs[0].network_s,
+        op->lrp_networks.ipv4_addrs[0].plen);
     ds_put_format(actions,
                   "ip4.src = %s; ip4.dst = %s; udp.src = 67; next; "
                   "/* DHCP_RELAY_REQ */",
@@ -16740,10 +16747,12 @@ build_dhcp_relay_flows_for_lrouter_port(struct 
ovn_port *op,
 
     ds_put_format(
         match, "inport == %s && "
-        "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
+        "ip4.src == {0.0.0.0, %s/%u} && ip4.dst == 255.255.255.255 && "
         "udp.src == 68 && udp.dst == 67 && "
         REGBIT_DHCP_RELAY_REQ_CHK" == 0",
-        op->json_key);
+        op->json_key,
+        op->lrp_networks.ipv4_addrs[0].network_s,
+        op->lrp_networks.ipv4_addrs[0].plen);
     ds_put_format(actions, "drop; /* DHCP_RELAY_REQ */");
 
     ovn_lflow_add(lflows, op->od, S_ROUTER_IN_DHCP_RELAY_REQ, 1,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 26a19bd96..9d03de4fe 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -13434,14 +13434,14 @@ ovn-sbctl lflow-list > lflows
 AT_CAPTURE_FILE([lflows])
 
 AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed 's/table=../table=??/'], [0], [dnl
-  table=??(lr_in_ip_input     ), priority=110  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && ip.frag == 0 && udp.src == 
68 && udp.dst == 67), action=(reg9[[7]] = dhcp_relay_req_chk(192.168.1.1, 
172.16.1.1);next; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_ip_input     ), priority=110  , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && ip.frag 
== 0 && udp.src == 68 && udp.dst == 67), action=(reg9[[7]] = 
dhcp_relay_req_chk(192.168.1.1, 172.16.1.1);next; /* DHCP_RELAY_REQ */)
   table=??(lr_in_ip_input     ), priority=110  , match=(ip4.src == 172.16.1.1 
&& ip4.dst == 192.168.1.1 && ip.frag == 0 && udp.src == 67 && udp.dst == 67), 
action=(next; /* DHCP_RELAY_RESP */)
-  table=??(lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]]), action=(ip4.src = 192.168.1.1; ip4.dst = 172.16.1.1; udp.src 
= 67; next; /* DHCP_RELAY_REQ */)
-  table=??(lr_in_dhcp_relay_req), priority=1    , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]] == 0), action=(drop; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && udp.src 
== 68 && udp.dst == 67 && reg9[[7]]), action=(ip4.src = 192.168.1.1; ip4.dst = 
172.16.1.1; udp.src = 67; next; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_req), priority=1    , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && udp.src 
== 68 && udp.dst == 67 && reg9[[7]] == 0), action=(drop; /* DHCP_RELAY_REQ */)
   table=??(lr_in_dhcp_relay_resp_chk), priority=100  , match=(ip4.src == 
172.16.1.1 && ip4.dst == 192.168.1.1 && udp.src == 67 && udp.dst == 67), 
action=(reg2 = ip4.dst; reg9[[8]] = dhcp_relay_resp_chk(192.168.1.1, 
172.16.1.1); next; /* DHCP_RELAY_RESP */)
   table=??(lr_in_dhcp_relay_resp), priority=100  , match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]]), action=(ip4.src = 192.168.1.1; udp.dst = 68; outport = "lrp1"; 
output; /* DHCP_RELAY_RESP */)
   table=??(lr_in_dhcp_relay_resp), priority=1    , match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]] == 0), action=(drop; /* DHCP_RELAY_RESP */)
-  table=??(ls_in_l2_lkup      ), priority=100  , match=(inport == "ls0-port1" 
&& eth.src == 02:00:00:00:00:10 && ip4.src == 0.0.0.0 && ip4.dst == 
255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(eth.dst = 
02:00:00:00:00:01; outport = "lrp1-attachment"; next; /* DHCP_RELAY_REQ */)
+  table=??(ls_in_l2_lkup      ), priority=100  , match=(inport == "ls0-port1" 
&& eth.src == 02:00:00:00:00:10 && ip4.src == {0.0.0.0, 192.168.1.0/24} && 
ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(eth.dst 
= 02:00:00:00:00:01; outport = "lrp1-attachment"; next; /* DHCP_RELAY_REQ */)
 ])
 
 # Set invalid dhcp_relay_port in ls0
@@ -13451,10 +13451,10 @@ ovn-sbctl lflow-list > lflows
 AT_CAPTURE_FILE([lflows])
 
 AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed 's/table=../table=??/'], [0], [dnl
-  table=??(lr_in_ip_input     ), priority=110  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && ip.frag == 0 && udp.src == 
68 && udp.dst == 67), action=(reg9[[7]] = dhcp_relay_req_chk(192.168.1.1, 
172.16.1.1);next; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_ip_input     ), priority=110  , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && ip.frag 
== 0 && udp.src == 68 && udp.dst == 67), action=(reg9[[7]] = 
dhcp_relay_req_chk(192.168.1.1, 172.16.1.1);next; /* DHCP_RELAY_REQ */)
   table=??(lr_in_ip_input     ), priority=110  , match=(ip4.src == 172.16.1.1 
&& ip4.dst == 192.168.1.1 && ip.frag == 0 && udp.src == 67 && udp.dst == 67), 
action=(next; /* DHCP_RELAY_RESP */)
-  table=??(lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]]), action=(ip4.src = 192.168.1.1; ip4.dst = 172.16.1.1; udp.src 
= 67; next; /* DHCP_RELAY_REQ */)
-  table=??(lr_in_dhcp_relay_req), priority=1    , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]] == 0), action=(drop; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && udp.src 
== 68 && udp.dst == 67 && reg9[[7]]), action=(ip4.src = 192.168.1.1; ip4.dst = 
172.16.1.1; udp.src = 67; next; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_req), priority=1    , match=(inport == "lrp1" && 
ip4.src == {0.0.0.0, 192.168.1.0/24} && ip4.dst == 255.255.255.255 && udp.src 
== 68 && udp.dst == 67 && reg9[[7]] == 0), action=(drop; /* DHCP_RELAY_REQ */)
   table=??(lr_in_dhcp_relay_resp_chk), priority=100  , match=(ip4.src == 
172.16.1.1 && ip4.dst == 192.168.1.1 && udp.src == 67 && udp.dst == 67), 
action=(reg2 = ip4.dst; reg9[[8]] = dhcp_relay_resp_chk(192.168.1.1, 
172.16.1.1); next; /* DHCP_RELAY_RESP */)
   table=??(lr_in_dhcp_relay_resp), priority=100  , match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]]), action=(ip4.src = 192.168.1.1; udp.dst = 68; outport = "lrp1"; 
output; /* DHCP_RELAY_RESP */)
   table=??(lr_in_dhcp_relay_resp), priority=1    , match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]] == 0), action=(drop; /* DHCP_RELAY_RESP */)
diff --git a/tests/ovn.at b/tests/ovn.at
index 0c3d4199b..ca8f197ec 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -41164,8 +41164,12 @@ sid=$src_ip
 # send packet
 send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 01 01 $yiaddr $giaddr $sid 
vif0
 
+# The default set match ip4.src == {0.0.0.0, lrp_cidr} compiles to
+# two OF flows; only the 0.0.0.0 one sees the DISCOVER packet.
+# Sort for stable ordering.
 OVS_WAIT_FOR_OUTPUT([ovs-ofctl dump-flows br-int table=$dhcp_relay_req_table | 
grep -v NXST | grep 255.255.255.255 | grep resubmit |
-cut -d ' ' -f5-5 | sed "s/,//"], [0], [dnl
+cut -d ' ' -f5-5 | sed "s/,//" | sort], [0], [dnl
+n_packets=0
 n_packets=1
 ])
 
@@ -41176,7 +41180,8 @@ giaddr=`ip_to_hex 192.168.1.1`
 send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 01 01 $yiaddr $giaddr $sid 
vif0
 
 OVS_WAIT_FOR_OUTPUT([ovs-ofctl dump-flows br-int table=$dhcp_relay_req_table | 
grep -v NXST | grep 255.255.255.255 | grep drop |
-cut -d ' ' -f5-5 | sed "s/,//"], [0], [dnl
+cut -d ' ' -f5-5 | sed "s/,//" | sort], [0], [dnl
+n_packets=0
 n_packets=1
 ])
 
-- 
2.43.5

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to