shill: DhcpConfig: Save Gateway-ARP lease on timeout
If Gateway ARP succeeds, we should not consider a timeout in DHCP
renewal to be a failure. This is because in the absence of any
other information, it is reasonable to expect that our lease is
still valid during its scheduled period.
BUG=chromium:287896
TEST=Unit tests
Change-Id: I3597f7f0cb77af2665157b897de69ff33f94bfea
Reviewed-on: https://chromium-review.googlesource.com/168849
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/dhcp_config.cc b/dhcp_config.cc
index bd04c1e..e14b878 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -51,6 +51,7 @@
const char DHCPConfig::kReasonBound[] = "BOUND";
const char DHCPConfig::kReasonFail[] = "FAIL";
const char DHCPConfig::kReasonGatewayArp[] = "GATEWAY-ARP";
+const char DHCPConfig::kReasonNak[] = "NAK";
const char DHCPConfig::kReasonRebind[] = "REBIND";
const char DHCPConfig::kReasonReboot[] = "REBOOT";
const char DHCPConfig::kReasonRenew[] = "RENEW";
@@ -75,6 +76,7 @@
pid_(0),
child_watch_tag_(0),
is_lease_active_(false),
+ is_gateway_arp_active_(false),
lease_acquisition_timeout_seconds_(kDHCPTimeoutSeconds),
root_("/"),
weak_ptr_factory_(this),
@@ -158,8 +160,14 @@
LOG(ERROR) << "Received failure event from DHCP client.";
UpdateProperties(IPConfig::Properties(), false);
return;
- }
- if (reason != kReasonBound &&
+ } else if (reason == kReasonNak) {
+ // If we got a NAK, this means the DHCP server is active, and any
+ // Gateway ARP state we have is no longer sufficient.
+ LOG_IF(ERROR, is_gateway_arp_active_)
+ << "Received NAK event for our gateway-ARP lease.";
+ is_gateway_arp_active_ = false;
+ return;
+ } else if (reason != kReasonBound &&
reason != kReasonRebind &&
reason != kReasonReboot &&
reason != kReasonRenew &&
@@ -182,8 +190,10 @@
// until that completes. In the meantime, however, we can tentatively
// configure our network in anticipation of successful completion.
IPConfig::UpdateProperties(properties, true);
+ is_gateway_arp_active_ = true;
} else {
UpdateProperties(properties, true);
+ is_gateway_arp_active_ = false;
}
}
@@ -483,7 +493,11 @@
void DHCPConfig::ProcessDHCPTimeout() {
LOG(ERROR) << "Timed out waiting for DHCP lease on " << device_name() << " "
<< "(after " << lease_acquisition_timeout_seconds_ << " seconds).";
- UpdateProperties(IPConfig::Properties(), false);
+ if (is_gateway_arp_active_) {
+ LOG(INFO) << "Continuing to use our previous lease, due to gateway-ARP.";
+ } else {
+ UpdateProperties(IPConfig::Properties(), false);
+ }
}
} // namespace shill
diff --git a/dhcp_config.h b/dhcp_config.h
index 60e9a03..354dfa2 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -75,6 +75,8 @@
FRIEND_TEST(DHCPConfigTest, ParseClasslessStaticRoutes);
FRIEND_TEST(DHCPConfigTest, ParseConfiguration);
FRIEND_TEST(DHCPConfigTest, ProcessEventSignalFail);
+ FRIEND_TEST(DHCPConfigTest, ProcessEventSignalGatewayArp);
+ FRIEND_TEST(DHCPConfigTest, ProcessEventSignalGatewayArpNak);
FRIEND_TEST(DHCPConfigTest, ProcessEventSignalSuccess);
FRIEND_TEST(DHCPConfigTest, ProcessEventSignalUnknown);
FRIEND_TEST(DHCPConfigTest, ReleaseIP);
@@ -115,8 +117,9 @@
static const char kDHCPCDUser[];
static const char kReasonBound[];
- static const char kReasonGatewayArp[];
static const char kReasonFail[];
+ static const char kReasonGatewayArp[];
+ static const char kReasonNak[];
static const char kReasonRebind[];
static const char kReasonReboot[];
static const char kReasonRenew[];
@@ -196,6 +199,9 @@
// Whether a lease has been acquired from the DHCP server or gateway ARP.
bool is_lease_active_;
+ // Whether it is valid to retain the lease acquired via gateway ARP.
+ bool is_gateway_arp_active_;
+
// The proxy for communicating with the DHCP client.
scoped_ptr<DHCPProxyInterface> proxy_;
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index 4b45138..8116a65 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -439,6 +439,70 @@
EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
+TEST_F(DHCPConfigTest, ProcessEventSignalGatewayArp) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ UpdateCallbackTest success_callback_test(DHCPConfig::kReasonGatewayArp,
+ config_, true);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&success_callback_test)));
+ EXPECT_CALL(*minijail_, RunAndDestroy(_, _, _)).WillOnce(Return(true));
+ config_->Start();
+ config_->ProcessEventSignal(DHCPConfig::kReasonGatewayArp, conf);
+ EXPECT_TRUE(success_callback_test.called());
+ EXPECT_EQ("4.3.2.1", config_->properties().address);
+ EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+ EXPECT_TRUE(config_->is_gateway_arp_active_);
+
+ // If the timeout gets called, we shouldn't lose the lease since GatewayArp
+ // is active.
+ ScopedMockLog log;
+ EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
+ EXPECT_CALL(log, Log(_, _, ::testing::EndsWith(
+ "Continuing to use our previous lease, due to gateway-ARP.")));
+ UpdateCallbackTest failure_callback_test("Timeout", config_, true);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&failure_callback_test)));
+ config_->lease_acquisition_timeout_callback_.callback().Run();
+ EXPECT_FALSE(failure_callback_test.called());
+ EXPECT_TRUE(config_->is_gateway_arp_active_);
+
+ // An official reply from a DHCP server should reset our GatewayArp state.
+ UpdateCallbackTest renew_callback_test(DHCPConfig::kReasonRenew,
+ config_, true);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&renew_callback_test)));
+ config_->ProcessEventSignal(DHCPConfig::kReasonRenew, conf);
+ EXPECT_TRUE(renew_callback_test.called());
+ EXPECT_FALSE(config_->is_gateway_arp_active_);
+}
+
+TEST_F(DHCPConfigTest, ProcessEventSignalGatewayArpNak) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ UpdateCallbackTest success_callback_test(DHCPConfig::kReasonGatewayArp,
+ config_, true);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&success_callback_test)));
+ EXPECT_CALL(*minijail_, RunAndDestroy(_, _, _)).WillOnce(Return(true));
+ config_->Start();
+ config_->ProcessEventSignal(DHCPConfig::kReasonGatewayArp, conf);
+ EXPECT_TRUE(config_->is_gateway_arp_active_);
+
+ // Sending a NAK should clear is_gateway_arp_active_.
+ config_->ProcessEventSignal(DHCPConfig::kReasonNak, conf);
+ EXPECT_FALSE(config_->is_gateway_arp_active_);
+
+ // If the timeout gets called, we should lose the lease since GatewayArp
+ // is not active any more.
+ UpdateCallbackTest failure_callback_test("Timeout", config_, false);
+ config_->RegisterUpdateCallback(
+ Bind(&UpdateCallbackTest::Callback, Unretained(&failure_callback_test)));
+ config_->lease_acquisition_timeout_callback_.callback().Run();
+ EXPECT_TRUE(failure_callback_test.called());
+}
TEST_F(DHCPConfigTest, ReleaseIP) {
config_->pid_ = 1 << 18; // Ensure unknown positive PID.