Only disable update downloads over expensive connections, not all transfers.
Previous to this CL, all network traffic was disabled from the update engine
if an update wasn't allowed. Instead, in this CL, we switch to only disable
the update application portion (downloading and applying the payload). This
is done by moving the detection logic from the mechanism (the network fetcher)
to the user of said mechanism i.e. the omaha request/response actions.
I have done a little refactoring of the unittests to make this CL easier to
test and found 1 bug in the http_fetcher_unittests where we weren't sending
either the right URL or handling a server not existing correctly.
This CL comes with one caveat:
Technically in the previous impl if a download started over wifi then got
disconnected / retried and bounced to a different connection type that
we couldn't update over, we'd stop the update. This would no longer be true
here if the http_fetcher recovered seamlessly.
BUG=chromium:379832
TEST=Unittests
Change-Id: I6b1a76e4c030d4e384e8ba0b519424a35406aafb
Reviewed-on: https://chromium-review.googlesource.com/202435
Tested-by: Chris Sosa <sosa@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: Chris Sosa <sosa@chromium.org>
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index d7713c5..3392098 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -23,7 +23,6 @@
#include "update_engine/fake_system_state.h"
#include "update_engine/http_common.h"
#include "update_engine/libcurl_http_fetcher.h"
-#include "update_engine/mock_connection_manager.h"
#include "update_engine/mock_http_fetcher.h"
#include "update_engine/multi_range_http_fetcher.h"
#include "update_engine/proxy_resolver.h"
@@ -194,10 +193,7 @@
class AnyHttpFetcherTest {
public:
- AnyHttpFetcherTest()
- : mock_connection_manager_(&fake_system_state_) {
- fake_system_state_.set_connection_manager(&mock_connection_manager_);
- }
+ AnyHttpFetcherTest() {}
virtual ~AnyHttpFetcherTest() {}
virtual HttpFetcher* NewLargeFetcher(size_t num_proxies) = 0;
@@ -224,7 +220,6 @@
protected:
DirectProxyResolver proxy_resolver_;
FakeSystemState fake_system_state_;
- MockConnectionManager mock_connection_manager_;
};
class MockHttpFetcherTest : public AnyHttpFetcherTest {
@@ -426,17 +421,6 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewSmallFetcher());
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWifi),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWifi, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWifi))
- .WillRepeatedly(Return(shill::kTypeWifi));
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
ASSERT_TRUE(server->started_);
@@ -457,17 +441,6 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewLargeFetcher());
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetEthernet),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetEthernet, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetEthernet))
- .WillRepeatedly(Return(shill::kTypeEthernet));
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
ASSERT_TRUE(server->started_);
@@ -496,17 +469,6 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewSmallFetcher());
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWimax),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWimax, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWimax))
- .WillRepeatedly(Return(shill::kTypeWimax));
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
ASSERT_TRUE(server->started_);
@@ -574,17 +536,6 @@
delegate.fetcher_ = fetcher.get();
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetCellular),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetCellular, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetCellular))
- .WillRepeatedly(Return(shill::kTypeCellular));
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
ASSERT_TRUE(server->started_);
@@ -653,17 +604,6 @@
delegate.loop_ = loop;
delegate.fetcher_->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- delegate.fetcher_->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWifi),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWifi, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWifi))
- .WillRepeatedly(Return(shill::kTypeWifi));
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
this->test_.IgnoreServerAborting(server.get());
ASSERT_TRUE(server->started_);
@@ -713,18 +653,6 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewSmallFetcher());
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWifi),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWifi, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWifi))
- .WillRepeatedly(Return(shill::kTypeWifi));
-
-
scoped_ptr<HttpServer> server(this->test_.CreateServer());
ASSERT_TRUE(server->started_);
@@ -776,7 +704,7 @@
}
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
EXPECT_FALSE(successful);
- EXPECT_EQ(0, fetcher->http_response_code());
+ EXPECT_EQ(404, fetcher->http_response_code());
g_main_loop_quit(loop_);
}
virtual void TransferTerminated(HttpFetcher* fetcher) {
@@ -798,21 +726,9 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewSmallFetcher());
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetEthernet),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetEthernet, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetEthernet))
- .WillRepeatedly(Return(shill::kTypeEthernet));
-
-
StartTransferArgs start_xfer_args = {
fetcher.get(),
- LocalServerUrlForPath(0, this->test_.SmallUrl(0))
+ this->test_.SmallUrl(0)
};
g_timeout_add(0, StartTransfer, &start_xfer_args);
@@ -833,18 +749,6 @@
scoped_ptr<HttpFetcher> fetcher(this->test_.NewSmallFetcher());
fetcher->set_delegate(&delegate);
- // Don't allow connection to server by denying access over ethernet.
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetEthernet),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetEthernet, _))
- .WillRepeatedly(Return(false));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetEthernet))
- .WillRepeatedly(Return(shill::kTypeEthernet));
-
StartTransferArgs start_xfer_args = {
fetcher.get(),
LocalServerUrlForPath(0,
@@ -905,17 +809,6 @@
scoped_ptr<HttpFetcher> fetcher(http_fetcher);
fetcher->set_delegate(&delegate);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetEthernet),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetEthernet, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetEthernet))
- .WillRepeatedly(Return(shill::kTypeEthernet));
-
StartTransferArgs start_xfer_args =
{ fetcher.get(), LocalServerUrlForPath(server->GetPort(), url) };
@@ -1025,17 +918,6 @@
delegate.loop_ = loop;
delegate.fetcher_.reset(fetcher_in);
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher_in->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWifi),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWifi, _))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWifi))
- .WillRepeatedly(Return(shill::kTypeWifi));
-
MultiRangeHttpFetcher* multi_fetcher =
dynamic_cast<MultiRangeHttpFetcher*>(fetcher_in);
ASSERT_TRUE(multi_fetcher);
@@ -1226,16 +1108,6 @@
bool is_allowed = (i != 0);
scoped_ptr<HttpFetcher> fetcher(this->test_.NewLargeFetcher());
- MockConnectionManager* mock_cm = dynamic_cast<MockConnectionManager*>(
- fetcher->GetSystemState()->connection_manager());
- EXPECT_CALL(*mock_cm, GetConnectionProperties(_,_,_))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(kNetWifi),
- SetArgumentPointee<2>(NetworkTethering::kUnknown),
- Return(true)));
- EXPECT_CALL(*mock_cm, IsUpdateAllowedOver(kNetWifi, _))
- .WillRepeatedly(Return(is_allowed));
- EXPECT_CALL(*mock_cm, StringForConnectionType(kNetWifi))
- .WillRepeatedly(Return(shill::kTypeWifi));
bool is_official_build = (i == 1);
LOG(INFO) << "is_update_allowed_over_connection: " << is_allowed;