VPP-1294: add missing feature arc constraint

the ip4-dhcp-client-detect feature MUST run prior to nat44-out2in, or
inbound dhcp broadcast packets will be dropped. Certain dhcp servers
answer lease renewal dhcp-request packets with broadcast dhcp-acks, leading
to unrecoverable lease loss.

In detail, this constraint:

VNET_FEATURE_INIT (ip4_snat_out2in, static) = {
  .arc_name = "ip4-unicast",
  .node_name = "nat44-out2in",
  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa"),
};

doesn't get the job done:

ip4-unicast:
  [17] nat44-out2in
  [23] ip4-dhcp-client-detect
  [26] ip4-not-enabled

Add a proper constraint:

VNET_FEATURE_INIT (ip4_snat_out2in, static) = {
  .arc_name = "ip4-unicast",
  .node_name = "nat44-out2in",
  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa",
                               "ip4-dhcp-client-detect"),
};

and the interface feature order is OK, at least in this regard:

ip4-unicast:
  [17] ip4-dhcp-client-detect
  [18] nat44-out2in
  [26] ip4-not-enabled

We need to carefully audit (especially) the ip4-unicast feature arc,
which has [gasp] 37 features on it!

Change-Id: I5e749ead7ab2a25d80839a331de6261e112977ad
Signed-off-by: Dave Barach <dave@barachs.net>

1 files changed, 20 insertions, 4 deletions

diff --git a/src/vnet/dhcp/client.c b/src/vnet/dhcp/client.c
index dbcb2a53dbc..11e47f9ad7a 100644
--- a/src/vnet/dhcp/client.c
+++ b/src/vnet/dhcp/client.c
@@ -106,6 +106,7 @@ dhcp_client_addr_callback (dhcp_client_t * c)
   vnet_feature_enable_disable ("ip4-unicast",
 			       "ip4-dhcp-client-detect",
 			       c->sw_if_index, 0 /* disable */ , 0, 0);
+  c->client_detect_feature_enabled = 0;
 
   /* if renewing the lease, the address and route have already been added */
   if (c->state == DHCP_BOUND)
@@ -220,6 +221,7 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 	  {
 	    u32 lease_time_in_seconds =
 	      clib_host_to_net_u32 (o->data_as_u32[0]);
+	    // for debug: lease_time_in_seconds = 20; /*$$$$*/
 	    c->lease_expires = now + (f64) lease_time_in_seconds;
 	    c->lease_lifetime = lease_time_in_seconds;
 	    /* Set a sensible default, in case we don't get opt 58 */
@@ -307,6 +309,7 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
 					   "ip4-dhcp-client-detect",
 					   c->sw_if_index, 1 /* enable */ ,
 					   0, 0);
+	      c->client_detect_feature_enabled = 1;
 	    }
 	  /* Wipe out any memory of the address we had... */
 	  c->state = DHCP_DISCOVER;
@@ -577,6 +580,15 @@ dhcp_discover_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * State machine "DISCOVER" state. Send a dhcp discover packet,
    * eventually back off the retry rate.
    */
+
+  if (c->client_detect_feature_enabled == 0)
+    {
+      vnet_feature_enable_disable ("ip4-unicast",
+				   "ip4-dhcp-client-detect",
+				   c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
+    }
+
   send_dhcp_pkt (dcm, c, DHCP_PACKET_DISCOVER, 1 /* is_broadcast */ );
 
   c->retry_count++;
@@ -623,10 +635,13 @@ dhcp_bound_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * DHCP address. Turn it back on again on first renew attempt.
    * Otherwise, if the DHCP server replies we'll never see it.
    */
-  if (!c->retry_count)
-    vnet_feature_enable_disable ("ip4-unicast",
-				 "ip4-dhcp-client-detect",
-				 c->sw_if_index, 1 /* enable */ , 0, 0);
+  if (c->client_detect_feature_enabled == 0)
+    {
+      vnet_feature_enable_disable ("ip4-unicast",
+				   "ip4-dhcp-client-detect",
+				   c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
+    }
 
   send_dhcp_pkt (dcm, c, DHCP_PACKET_REQUEST, 0 /* is_broadcast */ );
 
@@ -928,6 +943,7 @@ dhcp_client_add_del (dhcp_client_add_del_args_t * a)
       vnet_feature_enable_disable ("ip4-unicast",
 				   "ip4-dhcp-client-detect",
 				   c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
 
       vlib_process_signal_event (vm, dhcp_client_process_node.index,
 				 EVENT_DHCP_CLIENT_WAKEUP, c - dcm->clients);