shill: Device: Release lease if using static IP
If we are using a statically configured IP address instead
of a leased IP address, release any acquired lease so it may
be used by others. This allows us to merge other non-leased
parameters (like DNS) when they're available from a DHCP server
and not overridden by static parameters, but at the same time
we avoid taking up a dynamic IP address the DHCP server could
assign to someone else who might actually use it.
The only downside to this approach is that we will no longer
perform lease renewals via the DHCP client, so if other parameters
of the lease (default gateway, DNS servers) change while we are
connected, they will not automatically refresh at the client.
BUG=chromium:249399
TEST=Unit tests + network_DhcpNegotiationSuccess autotest + manual:
tcpdump eth0 while connecting to a network with a static IP address.
Ensure lease is acquired, then released, and the device is using the
statically-configured address. Without a static IP configuration,
ensure that the lease is retained and the host uses the DHCP supplied
address.
Change-Id: Ifdc1a1b9f2e55bead3bc2ed6e58b0fcac91bcbcd
Reviewed-on: https://gerrit.chromium.org/gerrit/58592
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: Christopher Wiley <wiley@chromium.org>
diff --git a/device.cc b/device.cc
index 5a3d17a..04c5bc1 100644
--- a/device.cc
+++ b/device.cc
@@ -366,7 +366,7 @@
void Device::DestroyIPConfig() {
DisableIPv6();
if (ipconfig_.get()) {
- ipconfig_->ReleaseIP();
+ ipconfig_->ReleaseIP(IPConfig::kReleaseReasonDisconnect);
ipconfig_ = NULL;
}
DestroyConnection();
@@ -453,6 +453,16 @@
if (selected_service_) {
ipconfig->ApplyStaticIPParameters(
selected_service_->mutable_static_ip_parameters());
+ if (selected_service_->static_ip_parameters().ContainsAddress()) {
+ // If we are using a statically configured IP address instead
+ // of a leased IP address, release any acquired lease so it may
+ // be used by others. This allows us to merge other non-leased
+ // parameters (like DNS) when they're available from a DHCP server
+ // and not overridden by static parameters, but at the same time
+ // we avoid taking up a dynamic IP address the DHCP server could
+ // assign to someone else who might actually use it.
+ ipconfig->ReleaseIP(IPConfig::kReleaseReasonStaticIP);
+ }
}
connection_->UpdateFromIPConfig(ipconfig);
// SetConnection must occur after the UpdateFromIPConfig so the
diff --git a/dhcp_config.cc b/dhcp_config.cc
index 1774214..4742baa 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -72,6 +72,7 @@
arp_gateway_(arp_gateway),
pid_(0),
child_watch_tag_(0),
+ is_lease_active_(false),
lease_acquisition_timeout_seconds_(kDHCPTimeoutSeconds),
root_("/"),
weak_ptr_factory_(this),
@@ -117,14 +118,24 @@
return true;
}
-bool DHCPConfig::ReleaseIP() {
+bool DHCPConfig::ReleaseIP(ReleaseReason reason) {
SLOG(DHCP, 2) << __func__ << ": " << device_name();
if (!pid_) {
return true;
}
+
+ // If we are using static IP and haven't retrieved a lease yet, we should
+ // allow the DHCP process to continue until we have a lease.
+ if (!is_lease_active_ && reason == IPConfig::kReleaseReasonStaticIP) {
+ return true;
+ }
+
// If we are using gateway unicast ARP to speed up re-connect, don't
// give up our leases when we disconnect.
- if (!arp_gateway_ && proxy_.get()) {
+ bool should_keep_lease =
+ reason == IPConfig::kReleaseReasonDisconnect && arp_gateway_;
+
+ if (!should_keep_lease && proxy_.get()) {
proxy_->Release(device_name());
}
Stop(__func__);
@@ -156,6 +167,12 @@
}
IPConfig::Properties properties;
CHECK(ParseConfiguration(configuration, &properties));
+
+ // This needs to be set before calling UpdateProperties() below since
+ // those functions may indirectly call other methods like ReleaseIP that
+ // depend on or change this value.
+ is_lease_active_ = true;
+
if (reason == kReasonGatewayArp) {
// This is a non-authoritative confirmation that we or on the same
// network as the one we received a lease on previously. The DHCP
@@ -444,6 +461,7 @@
// |this| instance may be destroyed after this call.
provider_->UnbindPID(pid);
}
+ is_lease_active_ = false;
}
void DHCPConfig::StartDHCPTimeout() {
diff --git a/dhcp_config.h b/dhcp_config.h
index d21eb24..fdd71f9 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -54,7 +54,7 @@
// Inherited from IPConfig.
virtual bool RequestIP();
virtual bool RenewIP();
- virtual bool ReleaseIP();
+ virtual bool ReleaseIP(ReleaseReason reason);
// If |proxy_| is not initialized already, sets it to a new D-Bus proxy to
// |service|.
@@ -79,6 +79,8 @@
FRIEND_TEST(DHCPConfigTest, ProcessEventSignalUnknown);
FRIEND_TEST(DHCPConfigTest, ReleaseIP);
FRIEND_TEST(DHCPConfigTest, ReleaseIPArpGW);
+ FRIEND_TEST(DHCPConfigTest, ReleaseIPStaticIPWithLease);
+ FRIEND_TEST(DHCPConfigTest, ReleaseIPStaticIPWithoutLease);
FRIEND_TEST(DHCPConfigTest, RenewIP);
FRIEND_TEST(DHCPConfigTest, RequestIP);
FRIEND_TEST(DHCPConfigTest, RequestIPTimeout);
@@ -190,6 +192,9 @@
// Child exit watch callback source tag.
unsigned int child_watch_tag_;
+ // Whether a lease has been acquired from the DHCP server or gateway ARP.
+ bool is_lease_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 ec9dafd..4b45138 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -445,7 +445,7 @@
config_->arp_gateway_ = false;
EXPECT_CALL(*proxy_, Release(kDeviceName)).Times(1);
config_->proxy_.reset(proxy_.release());
- EXPECT_TRUE(config_->ReleaseIP());
+ EXPECT_TRUE(config_->ReleaseIP(IPConfig::kReleaseReasonDisconnect));
config_->pid_ = 0;
}
@@ -454,7 +454,31 @@
config_->arp_gateway_ = true;
EXPECT_CALL(*proxy_, Release(kDeviceName)).Times(0);
config_->proxy_.reset(proxy_.release());
- EXPECT_TRUE(config_->ReleaseIP());
+ EXPECT_TRUE(config_->ReleaseIP(IPConfig::kReleaseReasonDisconnect));
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, ReleaseIPStaticIPWithLease) {
+ config_->pid_ = 1 << 18; // Ensure unknown positive PID.
+ config_->arp_gateway_ = true;
+ config_->is_lease_active_ = true;
+ EXPECT_CALL(*proxy_, Release(kDeviceName));
+ config_->proxy_.reset(proxy_.release());
+ EXPECT_TRUE(config_->ReleaseIP(IPConfig::kReleaseReasonStaticIP));
+ EXPECT_EQ(NULL, config_->proxy_.get());
+ config_->pid_ = 0;
+}
+
+TEST_F(DHCPConfigTest, ReleaseIPStaticIPWithoutLease) {
+ config_->pid_ = 1 << 18; // Ensure unknown positive PID.
+ config_->arp_gateway_ = true;
+ config_->is_lease_active_ = false;
+ EXPECT_CALL(*proxy_, Release(kDeviceName)).Times(0);
+ MockDHCPProxy *proxy_pointer = proxy_.get();
+ config_->proxy_.reset(proxy_.release());
+ EXPECT_TRUE(config_->ReleaseIP(IPConfig::kReleaseReasonStaticIP));
+ // Expect that proxy has not been released.
+ EXPECT_EQ(proxy_pointer, config_->proxy_.get());
config_->pid_ = 0;
}
diff --git a/ipconfig.cc b/ipconfig.cc
index ce8c9c1..8fb2214 100644
--- a/ipconfig.cc
+++ b/ipconfig.cc
@@ -90,7 +90,7 @@
return false;
}
-bool IPConfig::ReleaseIP() {
+bool IPConfig::ReleaseIP(ReleaseReason reason) {
return false;
}
diff --git a/ipconfig.h b/ipconfig.h
index 9c76287..96d07b3 100644
--- a/ipconfig.h
+++ b/ipconfig.h
@@ -60,6 +60,11 @@
std::vector<Route> routes;
};
+ enum ReleaseReason {
+ kReleaseReasonDisconnect,
+ kReleaseReasonStaticIP
+ };
+
IPConfig(ControlInterface *control_interface, const std::string &device_name);
IPConfig(ControlInterface *control_interface,
const std::string &device_name,
@@ -93,7 +98,7 @@
// faster.
virtual bool RequestIP();
virtual bool RenewIP();
- virtual bool ReleaseIP();
+ virtual bool ReleaseIP(ReleaseReason reason);
// Refresh IP configuration. Called by the DBus Adaptor "Refresh" call.
void Refresh(Error *error);
diff --git a/ipconfig_unittest.cc b/ipconfig_unittest.cc
index 91b18bb..1c407aa 100644
--- a/ipconfig_unittest.cc
+++ b/ipconfig_unittest.cc
@@ -64,7 +64,7 @@
}
TEST_F(IPConfigTest, ReleaseIP) {
- EXPECT_FALSE(ipconfig_->ReleaseIP());
+ EXPECT_FALSE(ipconfig_->ReleaseIP(IPConfig::kReleaseReasonDisconnect));
}
TEST_F(IPConfigTest, SaveLoad) {
diff --git a/mock_ipconfig.h b/mock_ipconfig.h
index ba2f8ff..aed364f 100644
--- a/mock_ipconfig.h
+++ b/mock_ipconfig.h
@@ -23,7 +23,7 @@
MOCK_CONST_METHOD0(properties, const Properties &(void));
MOCK_METHOD0(RequestIP, bool(void));
MOCK_METHOD0(RenewIP, bool(void));
- MOCK_METHOD0(ReleaseIP, bool(void));
+ MOCK_METHOD1(ReleaseIP, bool(ReleaseReason reason));
MOCK_METHOD2(Load, bool(StoreInterface *storage,
const std::string &id_suffix));