diff --git a/src/libstore-test-support/https-store.cc b/src/libstore-test-support/https-store.cc index e5a34eba14f..3d4ba8d0a74 100644 --- a/src/libstore-test-support/https-store.cc +++ b/src/libstore-test-support/https-store.cc @@ -9,11 +9,12 @@ void TestHttpBinaryCacheStore::init() BinaryCacheStore::init(); } -ref TestHttpBinaryCacheStoreConfig::openTestStore() const +ref TestHttpBinaryCacheStoreConfig::openTestStore(ref fileTransfer) const { auto store = make_ref( ref{// FIXME we shouldn't actually need a mutable config - std::const_pointer_cast(shared_from_this())}); + std::const_pointer_cast(shared_from_this())}, + fileTransfer); store->init(); return store; } @@ -79,16 +80,18 @@ void HttpsBinaryCacheStoreTest::SetUp() for the port explicitly - this is enough. */ std::this_thread::sleep_for(std::chrono::milliseconds(50)); - /* FIXME: Don't use global settings. Tests are not run concurrently, so this is fine for now. */ - oldCaCert = fileTransferSettings.caFile; - fileTransferSettings.caFile = caCert.string(); + /* Create custom FileTransferSettings with our test CA certificate. + This avoids mutating global settings. */ + testFileTransferSettings = std::make_unique(); + testFileTransferSettings->caFile = caCert; + testFileTransfer = makeFileTransfer(*testFileTransferSettings); } void HttpsBinaryCacheStoreTest::TearDown() { - fileTransferSettings.caFile = oldCaCert; serverPid.kill(); delTmpDir.reset(); + testFileTransferSettings.reset(); } std::vector HttpsBinaryCacheStoreTest::serverArgs() @@ -115,11 +118,17 @@ std::vector HttpsBinaryCacheStoreMtlsTest::serverArgs() return args; } -ref HttpsBinaryCacheStoreTest::makeConfig(BinaryCacheStoreConfig::Params params) +ref HttpsBinaryCacheStoreTest::makeConfig() { - auto res = make_ref("https", fmt("localhost:%d", port), std::move(params)); + auto res = make_ref( + "https", fmt("localhost:%d", port), TestHttpBinaryCacheStoreConfig::Params{}); res->pathInfoCacheSize = 0; /* We don't want any caching in tests. */ return res; } +ref HttpsBinaryCacheStoreTest::openStore(ref config) +{ + return config->openTestStore(ref{testFileTransfer}); +} + } // namespace nix::testing diff --git a/src/libstore-test-support/include/nix/store/tests/https-store.hh b/src/libstore-test-support/include/nix/store/tests/https-store.hh index e077382d100..d0265eb90ee 100644 --- a/src/libstore-test-support/include/nix/store/tests/https-store.hh +++ b/src/libstore-test-support/include/nix/store/tests/https-store.hh @@ -28,10 +28,10 @@ public: TestHttpBinaryCacheStore & operator=(const TestHttpBinaryCacheStore &) = delete; TestHttpBinaryCacheStore & operator=(TestHttpBinaryCacheStore &&) = delete; - TestHttpBinaryCacheStore(ref config) + TestHttpBinaryCacheStore(ref config, ref fileTransfer) : Store{*config} , BinaryCacheStore{*config} - , HttpBinaryCacheStore(config) + , HttpBinaryCacheStore(config, fileTransfer) { diskCache = nullptr; /* Disable caching, we'll be creating a new binary cache for each test. */ } @@ -49,12 +49,7 @@ public: { } - ref openTestStore() const; - - ref openStore() const override - { - return openTestStore(); - } + ref openTestStore(ref fileTransfer) const; }; class HttpsBinaryCacheStoreTest : public virtual LibStoreNetworkTest @@ -71,17 +66,29 @@ protected: std::filesystem::path tmpDir, cacheDir; std::filesystem::path caCert, caKey, serverCert, serverKey; std::filesystem::path clientCert, clientKey; - std::optional oldCaCert; Pid serverPid; uint16_t port = 8443; std::shared_ptr localCacheStore; + /** + * Custom FileTransferSettings with the test CA certificate. + * This is used instead of modifying global settings. + */ + std::unique_ptr testFileTransferSettings; + + /** + * FileTransfer instance using our test settings. + * Initialized in SetUp(). + */ + std::shared_ptr testFileTransfer; + static void openssl(Strings args); void SetUp() override; void TearDown() override; virtual std::vector serverArgs(); - ref makeConfig(BinaryCacheStoreConfig::Params params); + ref makeConfig(); + ref openStore(ref config); }; class HttpsBinaryCacheStoreMtlsTest : public HttpsBinaryCacheStoreTest diff --git a/src/libstore-tests/http-binary-cache-store.cc b/src/libstore-tests/http-binary-cache-store.cc index bee23216f57..33369cd9b76 100644 --- a/src/libstore-tests/http-binary-cache-store.cc +++ b/src/libstore-tests/http-binary-cache-store.cc @@ -45,28 +45,18 @@ using namespace std::string_literals; TEST_F(HttpsBinaryCacheStoreTest, queryPathInfo) { - auto config = makeConfig({}); - auto store = config->openStore(); + auto store = openStore(makeConfig()); StringSource dump{"test"sv}; auto path = localCacheStore->addToStoreFromDump(dump, "test-name", FileSerialisationMethod::Flat); EXPECT_NO_THROW(store->queryPathInfo(path)); } -auto withNoRetries() -{ - auto oldTries = fileTransferSettings.tries.get(); - Finally restoreTries = [=]() { fileTransferSettings.tries = oldTries; }; - fileTransferSettings.tries = 1; /* FIXME: Don't use global settings. */ - return restoreTries; -} - TEST_F(HttpsBinaryCacheStoreMtlsTest, queryPathInfo) { - auto config = makeConfig({ - {"tls-certificate"s, clientCert.string()}, - {"tls-private-key"s, clientKey.string()}, - }); - auto store = config->openStore(); + auto config = makeConfig(); + config->tlsCert = clientCert; + config->tlsKey = clientKey; + auto store = openStore(config); StringSource dump{"test"sv}; auto path = localCacheStore->addToStoreFromDump(dump, "test-name", FileSerialisationMethod::Flat); EXPECT_NO_THROW(store->queryPathInfo(path)); @@ -74,9 +64,8 @@ TEST_F(HttpsBinaryCacheStoreMtlsTest, queryPathInfo) TEST_F(HttpsBinaryCacheStoreMtlsTest, rejectsWithoutClientCert) { - auto restoreTries = withNoRetries(); - auto config = makeConfig({}); - EXPECT_THROW(config->openStore(), Error); + testFileTransferSettings->tries = 1; + EXPECT_THROW(openStore(makeConfig()), Error); } TEST_F(HttpsBinaryCacheStoreMtlsTest, rejectsWrongClientCert) @@ -89,12 +78,11 @@ TEST_F(HttpsBinaryCacheStoreMtlsTest, rejectsWrongClientCert) openssl({"req", "-new", "-x509", "-days", "1", "-key", wrongKey.string(), "-out", wrongCert.string(), "-subj", "/CN=WrongClient"}); // clang-format on - auto config = makeConfig({ - {"tls-certificate"s, wrongCert.string()}, - {"tls-private-key"s, wrongKey.string()}, - }); - auto restoreTries = withNoRetries(); - EXPECT_THROW(config->openStore(), Error); + auto config = makeConfig(); + config->tlsCert = wrongCert; + config->tlsKey = wrongKey; + testFileTransferSettings->tries = 1; + EXPECT_THROW(openStore(config), Error); } TEST_F(HttpsBinaryCacheStoreMtlsTest, doesNotSendCertOnRedirectToDifferentAuthority) @@ -109,13 +97,11 @@ TEST_F(HttpsBinaryCacheStoreMtlsTest, doesNotSendCertOnRedirectToDifferentAuthor writeFile(entry.path(), content); } - auto config = makeConfig({ - {"tls-certificate"s, clientCert.string()}, - {"tls-private-key"s, clientKey.string()}, - }); - auto store = config->openStore(); - - auto restoreTries = withNoRetries(); + auto config = makeConfig(); + config->tlsCert = clientCert; + config->tlsKey = clientKey; + testFileTransferSettings->tries = 1; + auto store = openStore(config); auto info = store->queryPathInfo(path); NullSink null; EXPECT_THROW(store->narFromPath(path, null), Error); diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 20433d642de..56811a9e3fe 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -75,6 +75,8 @@ struct curlMultiError : Error struct curlFileTransfer : public FileTransfer { + const FileTransferSettings & settings; + curlMulti curlm; std::random_device rd; @@ -480,10 +482,11 @@ struct curlFileTransfer : public FileTransfer req, CURLOPT_USERAGENT, ("curl/" LIBCURL_VERSION " Nix/" + nixVersion - + (fileTransferSettings.userAgentSuffix != "" ? " " + fileTransferSettings.userAgentSuffix.get() : "")) + + (fileTransfer.settings.userAgentSuffix != "" ? " " + fileTransfer.settings.userAgentSuffix.get() + : "")) .c_str()); curl_easy_setopt(req, CURLOPT_PIPEWAIT, 1); - if (fileTransferSettings.enableHttp2) + if (fileTransfer.settings.enableHttp2) curl_easy_setopt(req, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2TLS); else curl_easy_setopt(req, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); @@ -498,9 +501,9 @@ struct curlFileTransfer : public FileTransfer curl_easy_setopt(req, CURLOPT_HTTPHEADER, requestHeaders.get()); - if (fileTransferSettings.downloadSpeed.get() > 0) + if (fileTransfer.settings.downloadSpeed.get() > 0) curl_easy_setopt( - req, CURLOPT_MAX_RECV_SPEED_LARGE, (curl_off_t) (fileTransferSettings.downloadSpeed.get() * 1024)); + req, CURLOPT_MAX_RECV_SPEED_LARGE, (curl_off_t) (fileTransfer.settings.downloadSpeed.get() * 1024)); if (request.method == HttpMethod::Head) curl_easy_setopt(req, CURLOPT_NOBODY, 1); @@ -529,20 +532,20 @@ struct curlFileTransfer : public FileTransfer curl_easy_setopt(req, CURLOPT_SEEKDATA, this); } - if (auto & caFile = fileTransferSettings.caFile.get()) + if (auto & caFile = fileTransfer.settings.caFile.get()) curl_easy_setopt(req, CURLOPT_CAINFO, caFile->c_str()); #if !defined(_WIN32) curl_easy_setopt(req, CURLOPT_SOCKOPTFUNCTION, cloexec_callback); #endif - curl_easy_setopt(req, CURLOPT_CONNECTTIMEOUT, fileTransferSettings.connectTimeout.get()); + curl_easy_setopt(req, CURLOPT_CONNECTTIMEOUT, fileTransfer.settings.connectTimeout.get()); curl_easy_setopt(req, CURLOPT_LOW_SPEED_LIMIT, 1L); - curl_easy_setopt(req, CURLOPT_LOW_SPEED_TIME, fileTransferSettings.stalledDownloadTimeout.get()); + curl_easy_setopt(req, CURLOPT_LOW_SPEED_TIME, fileTransfer.settings.stalledDownloadTimeout.get()); /* If no file exist in the specified path, curl continues to work anyway as if netrc support was disabled. */ - curl_easy_setopt(req, CURLOPT_NETRC_FILE, fileTransferSettings.netrcFile.get().c_str()); + curl_easy_setopt(req, CURLOPT_NETRC_FILE, fileTransfer.settings.netrcFile.get().c_str()); curl_easy_setopt(req, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); if (writtenToSink) @@ -788,10 +791,12 @@ struct curlFileTransfer : public FileTransfer std::thread workerThread; - const size_t maxQueueSize = fileTransferSettings.httpConnections.get() * 5; + const size_t maxQueueSize; - curlFileTransfer() - : mt19937(rd()) + curlFileTransfer(const FileTransferSettings & settings) + : settings(settings) + , mt19937(rd()) + , maxQueueSize(settings.httpConnections.get() * 5) { static std::once_flag globalInit; std::call_once(globalInit, curl_global_init, CURL_GLOBAL_ALL); @@ -799,7 +804,7 @@ struct curlFileTransfer : public FileTransfer curlm = curlMulti(curl_multi_init()); curl_multi_setopt(curlm.get(), CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX); - curl_multi_setopt(curlm.get(), CURLMOPT_MAX_TOTAL_CONNECTIONS, fileTransferSettings.httpConnections.get()); + curl_multi_setopt(curlm.get(), CURLMOPT_MAX_TOTAL_CONNECTIONS, settings.httpConnections.get()); workerThread = std::thread([&]() { workerThreadEntry(); }); } @@ -1000,9 +1005,9 @@ struct curlFileTransfer : public FileTransfer } }; -ref makeCurlFileTransfer() +ref makeCurlFileTransfer(const FileTransferSettings & settings = fileTransferSettings) { - return make_ref(); + return make_ref(settings); } ref getFileTransfer() @@ -1015,9 +1020,9 @@ ref getFileTransfer() return fileTransfer; } -ref makeFileTransfer() +ref makeFileTransfer(const FileTransferSettings & settings) { - return makeCurlFileTransfer(); + return makeCurlFileTransfer(settings); } void FileTransferRequest::setupForS3() diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 943f1ed94d6..e0c12fb4402 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -50,9 +50,10 @@ std::string HttpBinaryCacheStoreConfig::doc() ; } -HttpBinaryCacheStore::HttpBinaryCacheStore(ref config) +HttpBinaryCacheStore::HttpBinaryCacheStore(ref config, ref fileTransfer) : Store{*config} // TODO it will actually mutate the configuration , BinaryCacheStore{*config} + , fileTransfer{fileTransfer} , config{config} { diskCache = getNarInfoDiskCache(); @@ -121,7 +122,7 @@ bool HttpBinaryCacheStore::fileExists(const std::string & path) try { FileTransferRequest request(makeRequest(path)); request.method = HttpMethod::Head; - getFileTransfer()->download(request); + fileTransfer->download(request); return true; } catch (FileTransferError & e) { /* S3 buckets return 403 if a file doesn't exist and the @@ -151,7 +152,7 @@ void HttpBinaryCacheStore::upload( req.data = {sizeHint, source}; req.mimeType = mimeType; - getFileTransfer()->upload(req); + fileTransfer->upload(req); } void HttpBinaryCacheStore::upsertFile( @@ -219,7 +220,7 @@ void HttpBinaryCacheStore::getFile(const std::string & path, Sink & sink) checkEnabled(); auto request(makeRequest(path)); try { - getFileTransfer()->download(std::move(request), sink); + fileTransfer->download(std::move(request), sink); } catch (FileTransferError & e) { if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden) throw NoSuchBinaryCacheFile( @@ -238,19 +239,19 @@ void HttpBinaryCacheStore::getFile(const std::string & path, CallbackenqueueFileTransfer(request, {[callbackPtr, this](std::future result) { - try { - (*callbackPtr)(std::move(result.get().data)); - } catch (FileTransferError & e) { - if (e.error == FileTransfer::NotFound - || e.error == FileTransfer::Forbidden) - return (*callbackPtr)({}); - maybeDisable(); - callbackPtr->rethrow(); - } catch (...) { - callbackPtr->rethrow(); - } - }}); + fileTransfer->enqueueFileTransfer(request, {[callbackPtr, this](std::future result) { + try { + (*callbackPtr)(std::move(result.get().data)); + } catch (FileTransferError & e) { + if (e.error == FileTransfer::NotFound + || e.error == FileTransfer::Forbidden) + return (*callbackPtr)({}); + maybeDisable(); + callbackPtr->rethrow(); + } catch (...) { + callbackPtr->rethrow(); + } + }}); } catch (...) { callbackPtr->rethrow(); @@ -261,7 +262,7 @@ void HttpBinaryCacheStore::getFile(const std::string & path, Callback HttpBinaryCacheStore::getNixCacheInfo() { try { - auto result = getFileTransfer()->download(makeRequest(cacheInfoFile)); + auto result = fileTransfer->download(makeRequest(cacheInfoFile)); return result.data; } catch (FileTransferError & e) { if (e.error == FileTransfer::NotFound) @@ -284,11 +285,17 @@ std::optional HttpBinaryCacheStore::isTrustedClient() return std::nullopt; } -ref HttpBinaryCacheStore::Config::openStore() const +ref HttpBinaryCacheStore::Config::openStore(ref fileTransfer) const { return make_ref( ref{// FIXME we shouldn't actually need a mutable config - std::const_pointer_cast(shared_from_this())}); + std::const_pointer_cast(shared_from_this())}, + fileTransfer); +} + +ref HttpBinaryCacheStoreConfig::openStore() const +{ + return openStore(getFileTransfer()); } static RegisterStoreImplementation regHttpBinaryCacheStore; diff --git a/src/libstore/include/nix/store/filetransfer.hh b/src/libstore/include/nix/store/filetransfer.hh index 9a509bb8658..1bab158e8fa 100644 --- a/src/libstore/include/nix/store/filetransfer.hh +++ b/src/libstore/include/nix/store/filetransfer.hh @@ -12,7 +12,6 @@ #include "nix/util/url.hh" #include "nix/store/config.hh" -#include "nix/store/globals.hh" #if NIX_WITH_AWS_AUTH # include "nix/store/aws-creds.hh" #endif @@ -20,6 +19,8 @@ namespace nix { +const std::filesystem::path & nixConfDir(); + struct FileTransferSettings : Config { private: @@ -401,7 +402,7 @@ ref getFileTransfer(); * * Prefer getFileTransfer() to this; see its docs for why. */ -ref makeFileTransfer(); +ref makeFileTransfer(const FileTransferSettings & settings = fileTransferSettings); class FileTransferError : public Error { diff --git a/src/libstore/include/nix/store/http-binary-cache-store.hh b/src/libstore/include/nix/store/http-binary-cache-store.hh index e32362efb59..9c251fc680a 100644 --- a/src/libstore/include/nix/store/http-binary-cache-store.hh +++ b/src/libstore/include/nix/store/http-binary-cache-store.hh @@ -52,6 +52,8 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this openStore(ref fileTransfer) const; + ref openStore() const override; StoreReference getReference() const override; @@ -67,13 +69,17 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore Sync _state; +protected: + + ref fileTransfer; + public: using Config = HttpBinaryCacheStoreConfig; ref config; - HttpBinaryCacheStore(ref config); + HttpBinaryCacheStore(ref config, ref fileTransfer = getFileTransfer()); void init() override;