From e36c9175f49c0fac4f0511d4cae5ab991fd9cb7d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 25 Mar 2022 04:40:49 +0000 Subject: [PATCH] Add protocol versions to `{Worker,Serve}Proto::*Conn` This will allow us to factor out logic, which is currently scattered inline, into several reusable instances The tests are also updated to support versioning. Currently all Worker and Serve protocol tests are using the minimum version, since no version-specific serialisers have been created yet. But in subsequent commits when that changes, we will test individual versions to ensure complete coverage. --- src/libstore/daemon.cc | 15 ++++- src/libstore/legacy-ssh-store.cc | 2 + src/libstore/path-info.cc | 10 +++- src/libstore/remote-store-connection.hh | 2 + src/libstore/serve-protocol.hh | 8 +-- src/libstore/tests/common-protocol.cc | 73 +++++++++++++++++++++---- src/libstore/tests/protocol.hh | 45 ++++++++------- src/libstore/tests/serve-protocol.cc | 38 +++++++++---- src/libstore/tests/worker-protocol.cc | 49 ++++++++++++----- src/libstore/worker-protocol.hh | 8 +-- src/nix-store/nix-store.cc | 10 +++- 11 files changed, 185 insertions(+), 75 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index fbff64e89..d61e97a64 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -277,8 +277,14 @@ static void performOp(TunnelLogger * logger, ref store, TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion, Source & from, BufferedSink & to, WorkerProto::Op op) { - WorkerProto::ReadConn rconn { .from = from }; - WorkerProto::WriteConn wconn { .to = to }; + WorkerProto::ReadConn rconn { + .from = from, + .version = clientVersion, + }; + WorkerProto::WriteConn wconn { + .to = to, + .version = clientVersion, + }; switch (op) { @@ -1052,7 +1058,10 @@ void processConnection( auto temp = trusted ? store->isTrustedClient() : std::optional { NotTrusted }; - WorkerProto::WriteConn wconn { .to = to }; + WorkerProto::WriteConn wconn { + .to = to, + .version = clientVersion, + }; WorkerProto::write(*store, wconn, temp); } diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index efced20d0..9143be72e 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -60,6 +60,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor { return ServeProto::ReadConn { .from = from, + .version = remoteVersion, }; } @@ -75,6 +76,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor { return ServeProto::WriteConn { .to = to, + .version = remoteVersion, }; } }; diff --git a/src/libstore/path-info.cc b/src/libstore/path-info.cc index ccb57104f..a9231ce8d 100644 --- a/src/libstore/path-info.cc +++ b/src/libstore/path-info.cc @@ -141,7 +141,10 @@ ValidPathInfo ValidPathInfo::read(Source & source, const Store & store, unsigned ValidPathInfo info(path, narHash); if (deriver != "") info.deriver = store.parseStorePath(deriver); info.references = WorkerProto::Serialise::read(store, - WorkerProto::ReadConn { .from = source }); + WorkerProto::ReadConn { + .from = source, + .version = format, + }); source >> info.registrationTime >> info.narSize; if (format >= 16) { source >> info.ultimate; @@ -163,7 +166,10 @@ void ValidPathInfo::write( sink << (deriver ? store.printStorePath(*deriver) : "") << narHash.to_string(Base16, false); WorkerProto::write(store, - WorkerProto::WriteConn { .to = sink }, + WorkerProto::WriteConn { + .to = sink, + .version = format, + }, references); sink << registrationTime << narSize; if (format >= 16) { diff --git a/src/libstore/remote-store-connection.hh b/src/libstore/remote-store-connection.hh index 8738e4d95..e4a9cacb9 100644 --- a/src/libstore/remote-store-connection.hh +++ b/src/libstore/remote-store-connection.hh @@ -70,6 +70,7 @@ struct RemoteStore::Connection { return WorkerProto::ReadConn { .from = from, + .version = daemonVersion, }; } @@ -85,6 +86,7 @@ struct RemoteStore::Connection { return WorkerProto::WriteConn { .to = to, + .version = daemonVersion, }; } diff --git a/src/libstore/serve-protocol.hh b/src/libstore/serve-protocol.hh index dddb7c54d..a627c6ad6 100644 --- a/src/libstore/serve-protocol.hh +++ b/src/libstore/serve-protocol.hh @@ -40,23 +40,19 @@ struct ServeProto /** * A unidirectional read connection, to be used by the read half of the * canonical serializers below. - * - * This currently is just a `Source &`, but more fields will be added - * later. */ struct ReadConn { Source & from; + Version version; }; /** * A unidirectional write connection, to be used by the write half of the * canonical serializers below. - * - * This currently is just a `Sink &`, but more fields will be added - * later. */ struct WriteConn { Sink & to; + Version version; }; /** diff --git a/src/libstore/tests/common-protocol.cc b/src/libstore/tests/common-protocol.cc index de57211f0..61c2cb70c 100644 --- a/src/libstore/tests/common-protocol.cc +++ b/src/libstore/tests/common-protocol.cc @@ -13,10 +13,71 @@ namespace nix { const char commonProtoDir[] = "common-protocol"; -using CommonProtoTest = ProtoTest; +class CommonProtoTest : public ProtoTest +{ +public: + /** + * Golden test for `T` reading + */ + template + void readTest(PathView testStem, T value) + { + if (testAccept()) + { + GTEST_SKIP() << "Cannot read golden master because another test is also updating it"; + } + else + { + auto expected = readFile(goldenMaster(testStem)); + + T got = ({ + StringSource from { expected }; + CommonProto::Serialise::read( + *store, + CommonProto::ReadConn { .from = from }); + }); + + ASSERT_EQ(got, value); + } + } + + /** + * Golden test for `T` write + */ + template + void writeTest(PathView testStem, const T & value) + { + auto file = goldenMaster(testStem); + + StringSink to; + CommonProto::write( + *store, + CommonProto::WriteConn { .to = to }, + value); + + if (testAccept()) + { + createDirs(dirOf(file)); + writeFile(file, to.s); + GTEST_SKIP() << "Updating golden master"; + } + else + { + auto expected = readFile(file); + ASSERT_EQ(to.s, expected); + } + } +}; + +#define CHARACTERIZATION_TEST(NAME, STEM, VALUE) \ + TEST_F(CommonProtoTest, NAME ## _read) { \ + readTest(STEM, VALUE); \ + } \ + TEST_F(CommonProtoTest, NAME ## _write) { \ + writeTest(STEM, VALUE); \ + } CHARACTERIZATION_TEST( - CommonProtoTest, string, "string", (std::tuple { @@ -28,7 +89,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, storePath, "store-path", (std::tuple { @@ -37,7 +97,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, contentAddress, "content-address", (std::tuple { @@ -56,7 +115,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, drvOutput, "drv-output", (std::tuple { @@ -71,7 +129,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, realisation, "realisation", (std::tuple { @@ -103,7 +160,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, vector, "vector", (std::tuple, std::vector, std::vector, std::vector>> { @@ -114,7 +170,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, set, "set", (std::tuple, std::set, std::set, std::set>> { @@ -125,7 +180,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, optionalStorePath, "optional-store-path", (std::tuple, std::optional> { @@ -136,7 +190,6 @@ CHARACTERIZATION_TEST( })) CHARACTERIZATION_TEST( - CommonProtoTest, optionalContentAddress, "optional-content-address", (std::tuple, std::optional> { diff --git a/src/libstore/tests/protocol.hh b/src/libstore/tests/protocol.hh index 86a900757..496915745 100644 --- a/src/libstore/tests/protocol.hh +++ b/src/libstore/tests/protocol.hh @@ -9,26 +9,23 @@ namespace nix { template class ProtoTest : public LibStoreTest { - /** - * Read this as simply `using S = Inner::Serialise;`. - * - * See `LengthPrefixedProtoHelper::S` for the same trick, and its - * rationale. - */ - template using S = typename Proto::template Serialise; - -public: +protected: Path unitTestData = getUnitTestData() + "/libstore/" + protocolDir; Path goldenMaster(std::string_view testStem) { return unitTestData + "/" + testStem + ".bin"; } +}; +template +class VersionedProtoTest : public ProtoTest +{ +public: /** * Golden test for `T` reading */ template - void readTest(PathView testStem, T value) + void readTest(PathView testStem, typename Proto::Version version, T value) { if (testAccept()) { @@ -36,13 +33,16 @@ public: } else { - auto expected = readFile(goldenMaster(testStem)); + auto expected = readFile(ProtoTest::goldenMaster(testStem)); T got = ({ StringSource from { expected }; - S::read( - *store, - typename Proto::ReadConn { .from = from }); + Proto::template Serialise::read( + *LibStoreTest::store, + typename Proto::ReadConn { + .from = from, + .version = version, + }); }); ASSERT_EQ(got, value); @@ -53,14 +53,17 @@ public: * Golden test for `T` write */ template - void writeTest(PathView testStem, const T & value) + void writeTest(PathView testStem, typename Proto::Version version, const T & value) { - auto file = goldenMaster(testStem); + auto file = ProtoTest::goldenMaster(testStem); StringSink to; Proto::write( - *store, - typename Proto::WriteConn { .to = to }, + *LibStoreTest::store, + typename Proto::WriteConn { + .to = to, + .version = version, + }, value); if (testAccept()) @@ -77,12 +80,12 @@ public: } }; -#define CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VALUE) \ +#define VERSIONED_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \ TEST_F(FIXTURE, NAME ## _read) { \ - readTest(STEM, VALUE); \ + readTest(STEM, VERSION, VALUE); \ } \ TEST_F(FIXTURE, NAME ## _write) { \ - writeTest(STEM, VALUE); \ + writeTest(STEM, VERSION, VALUE); \ } } diff --git a/src/libstore/tests/serve-protocol.cc b/src/libstore/tests/serve-protocol.cc index 5d7df56cf..ba798ab1c 100644 --- a/src/libstore/tests/serve-protocol.cc +++ b/src/libstore/tests/serve-protocol.cc @@ -11,14 +11,22 @@ namespace nix { -const char commonProtoDir[] = "serve-protocol"; +const char serveProtoDir[] = "serve-protocol"; -using ServeProtoTest = ProtoTest; +struct ServeProtoTest : VersionedProtoTest +{ + /** + * For serializers that don't care about the minimum version, we + * used the oldest one: 1.0. + */ + ServeProto::Version defaultVersion = 1 << 8 | 0; +}; -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, string, "string", + defaultVersion, (std::tuple { "", "hi", @@ -27,19 +35,21 @@ CHARACTERIZATION_TEST( "oh no \0\0\0 what was that!", })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, storePath, "store-path", + defaultVersion, (std::tuple { StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" }, StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo-bar" }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, contentAddress, "content-address", + defaultVersion, (std::tuple { ContentAddress { .method = TextIngestionMethod {}, @@ -55,10 +65,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, drvOutput, "drv-output", + defaultVersion, (std::tuple { { .drvHash = Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="), @@ -70,10 +81,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, realisation, "realisation", + defaultVersion, (std::tuple { Realisation { .id = DrvOutput { @@ -102,10 +114,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, vector, "vector", + defaultVersion, (std::tuple, std::vector, std::vector, std::vector>> { { }, { "" }, @@ -113,10 +126,11 @@ CHARACTERIZATION_TEST( { {}, { "" }, { "", "1", "2" } }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, set, "set", + defaultVersion, (std::tuple, std::set, std::set, std::set>> { { }, { "" }, @@ -124,10 +138,11 @@ CHARACTERIZATION_TEST( { {}, { "" }, { "", "1", "2" } }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, optionalStorePath, "optional-store-path", + defaultVersion, (std::tuple, std::optional> { std::nullopt, std::optional { @@ -135,10 +150,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, optionalContentAddress, "optional-content-address", + defaultVersion, (std::tuple, std::optional> { std::nullopt, std::optional { diff --git a/src/libstore/tests/worker-protocol.cc b/src/libstore/tests/worker-protocol.cc index 59d7ff96d..63fecce96 100644 --- a/src/libstore/tests/worker-protocol.cc +++ b/src/libstore/tests/worker-protocol.cc @@ -14,12 +14,21 @@ namespace nix { const char workerProtoDir[] = "worker-protocol"; -using WorkerProtoTest = ProtoTest; +struct WorkerProtoTest : VersionedProtoTest +{ + /** + * For serializers that don't care about the minimum version, we + * used the oldest one: 1.0. + */ + WorkerProto::Version defaultVersion = 1 << 8 | 0; +}; -CHARACTERIZATION_TEST( + +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, string, "string", + defaultVersion, (std::tuple { "", "hi", @@ -28,19 +37,21 @@ CHARACTERIZATION_TEST( "oh no \0\0\0 what was that!", })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, storePath, "store-path", + defaultVersion, (std::tuple { StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" }, StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo-bar" }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, contentAddress, "content-address", + defaultVersion, (std::tuple { ContentAddress { .method = TextIngestionMethod {}, @@ -56,10 +67,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, derivedPath, "derived-path", + defaultVersion, (std::tuple { DerivedPath::Opaque { .path = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" }, @@ -72,10 +84,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, drvOutput, "drv-output", + defaultVersion, (std::tuple { { .drvHash = Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="), @@ -87,10 +100,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, realisation, "realisation", + defaultVersion, (std::tuple { Realisation { .id = DrvOutput { @@ -119,10 +133,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult, "build-result", + defaultVersion, ({ using namespace std::literals::chrono_literals; std::tuple t { @@ -177,10 +192,11 @@ CHARACTERIZATION_TEST( t; })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, keyedBuildResult, "keyed-build-result", + defaultVersion, ({ using namespace std::literals::chrono_literals; std::tuple t { @@ -213,20 +229,22 @@ CHARACTERIZATION_TEST( t; })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, optionalTrustedFlag, "optional-trusted-flag", + defaultVersion, (std::tuple, std::optional, std::optional> { std::nullopt, std::optional { Trusted }, std::optional { NotTrusted }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, vector, "vector", + defaultVersion, (std::tuple, std::vector, std::vector, std::vector>> { { }, { "" }, @@ -234,10 +252,11 @@ CHARACTERIZATION_TEST( { {}, { "" }, { "", "1", "2" } }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, set, "set", + defaultVersion, (std::tuple, std::set, std::set, std::set>> { { }, { "" }, @@ -245,10 +264,11 @@ CHARACTERIZATION_TEST( { {}, { "" }, { "", "1", "2" } }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, optionalStorePath, "optional-store-path", + defaultVersion, (std::tuple, std::optional> { std::nullopt, std::optional { @@ -256,10 +276,11 @@ CHARACTERIZATION_TEST( }, })) -CHARACTERIZATION_TEST( +VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, optionalContentAddress, "optional-content-address", + defaultVersion, (std::tuple, std::optional> { std::nullopt, std::optional { diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index e9fc8d957..2d55d926a 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -57,23 +57,19 @@ struct WorkerProto /** * A unidirectional read connection, to be used by the read half of the * canonical serializers below. - * - * This currently is just a `Source &`, but more fields will be added - * later. */ struct ReadConn { Source & from; + Version version; }; /** * A unidirectional write connection, to be used by the write half of the * canonical serializers below. - * - * This currently is just a `Sink &`, but more fields will be added - * later. */ struct WriteConn { Sink & to; + Version version; }; /** diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index ea53e49b0..11d2e86e3 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -820,8 +820,14 @@ static void opServe(Strings opFlags, Strings opArgs) out.flush(); ServeProto::Version clientVersion = readInt(in); - ServeProto::ReadConn rconn { .from = in }; - ServeProto::WriteConn wconn { .to = out }; + ServeProto::ReadConn rconn { + .from = in, + .version = clientVersion, + }; + ServeProto::WriteConn wconn { + .to = out, + .version = clientVersion, + }; auto getBuildSettings = [&]() { // FIXME: changing options here doesn't work if we're