From 7d7ce0909fb25c3bcfc2e201616c410ea9e4b406 Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Thu, 14 Jul 2022 14:45:17 -0700 Subject: [PATCH] Restart tests carry forward encryption knobs value (#7497) Previously to get around the issue that EKP is not present when restart test switching encryption from on to off and read encrypted data, EKP was made to start in simulation regardless of encryption knob. This PR revert that change, and instead force restart test not to change encryption knob, by passing previous encryption knob through restartInfo.ini file. Also since we don't allow downgrading an encrypted cluster to previous version, disable encryption in downgrade tests. Also adding an assert to allow reading encrypted mutations only if encryption knob is on. We may reconsider allowing switching encryption on/off for existing cluster, but for now we don't allow it. --- fdbserver/ClusterController.actor.cpp | 16 +++++------ fdbserver/KeyValueStoreMemory.actor.cpp | 6 ++++ fdbserver/SimulatedCluster.actor.cpp | 28 +++++++++++++++---- fdbserver/Status.actor.cpp | 2 +- fdbserver/fdbserver.actor.cpp | 4 +++ .../fdbserver/EncryptedMutationMessage.h | 2 ++ fdbserver/tester.actor.cpp | 5 +++- .../workloads/ConsistencyCheck.actor.cpp | 2 +- fdbserver/workloads/SaveAndKill.actor.cpp | 4 +++ ...onfigureStorageMigrationTestRestart-1.toml | 1 + .../to_7.1.0/CycleTestRestart-1.txt | 1 + 11 files changed, 54 insertions(+), 17 deletions(-) diff --git a/fdbserver/ClusterController.actor.cpp b/fdbserver/ClusterController.actor.cpp index f1de7a23d6..f23fd5dce4 100644 --- a/fdbserver/ClusterController.actor.cpp +++ b/fdbserver/ClusterController.actor.cpp @@ -622,7 +622,7 @@ void checkBetterSingletons(ClusterControllerData* self) { } WorkerDetails newEKPWorker; - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { newEKPWorker = findNewProcessForSingleton(self, ProcessClass::EncryptKeyProxy, id_used); } @@ -636,7 +636,7 @@ void checkBetterSingletons(ClusterControllerData* self) { } ProcessClass::Fitness bestFitnessForEKP; - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { bestFitnessForEKP = findBestFitnessForSingleton(self, newEKPWorker, ProcessClass::EncryptKeyProxy); } @@ -661,7 +661,7 @@ void checkBetterSingletons(ClusterControllerData* self) { } bool ekpHealthy = true; - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { ekpHealthy = isHealthySingleton( self, newEKPWorker, ekpSingleton, bestFitnessForEKP, self->recruitingEncryptKeyProxyID); } @@ -685,7 +685,7 @@ void checkBetterSingletons(ClusterControllerData* self) { } Optional> currEKPProcessId, newEKPProcessId; - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { currEKPProcessId = ekpSingleton.interface.get().locality.processId(); newEKPProcessId = newEKPWorker.interf.locality.processId(); } @@ -697,7 +697,7 @@ void checkBetterSingletons(ClusterControllerData* self) { newPids.emplace_back(newBMProcessId); } - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { currPids.emplace_back(currEKPProcessId); newPids.emplace_back(newEKPProcessId); } @@ -712,7 +712,7 @@ void checkBetterSingletons(ClusterControllerData* self) { } // if the knob is disabled, the EKP coloc counts should have no affect on the coloc counts check below - if (!SERVER_KNOBS->ENABLE_ENCRYPTION && !g_network->isSimulated()) { + if (!SERVER_KNOBS->ENABLE_ENCRYPTION) { ASSERT(currColocMap[currEKPProcessId] == 0); ASSERT(newColocMap[newEKPProcessId] == 0); } @@ -1271,7 +1271,7 @@ ACTOR Future registerWorker(RegisterWorkerRequest req, self, w, currSingleton, registeringSingleton, self->recruitingBlobManagerID); } - if ((SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) && req.encryptKeyProxyInterf.present()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION && req.encryptKeyProxyInterf.present()) { auto currSingleton = EncryptKeyProxySingleton(self->db.serverInfo->get().encryptKeyProxy); auto registeringSingleton = EncryptKeyProxySingleton(req.encryptKeyProxyInterf); haltRegisteringOrCurrentSingleton( @@ -2525,7 +2525,7 @@ ACTOR Future clusterControllerCore(ClusterControllerFullInterface interf, state Future> error = errorOr(actorCollection(self.addActor.getFuture())); // EncryptKeyProxy is necessary for TLog recovery, recruit it as the first process - if (SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION) { self.addActor.send(monitorEncryptKeyProxy(&self)); } self.addActor.send(clusterWatchDatabase( diff --git a/fdbserver/KeyValueStoreMemory.actor.cpp b/fdbserver/KeyValueStoreMemory.actor.cpp index c548891b85..0e31d3f85d 100644 --- a/fdbserver/KeyValueStoreMemory.actor.cpp +++ b/fdbserver/KeyValueStoreMemory.actor.cpp @@ -506,6 +506,12 @@ private: OpHeader* h, bool* isZeroFilled, int* zeroFillSize) { + // Metadata op types to be excluded from encryption. + static std::unordered_set metaOps = { OpSnapshotEnd, OpSnapshotAbort, OpCommit, OpRollback }; + if (metaOps.count((OpType)h->op) == 0) { + // It is not supported to open an encrypted store as unencrypted, or vice-versa. + ASSERT_EQ(h->op == OpEncrypted, self->enableEncryption); + } state int remainingBytes = h->len1 + h->len2 + 1; if (h->op == OpEncrypted) { // encryption header, plus the real (encrypted) op type diff --git a/fdbserver/SimulatedCluster.actor.cpp b/fdbserver/SimulatedCluster.actor.cpp index 57db74ae8e..90b5fea0fe 100644 --- a/fdbserver/SimulatedCluster.actor.cpp +++ b/fdbserver/SimulatedCluster.actor.cpp @@ -260,6 +260,9 @@ class TestConfig { if (attrib == "disableRemoteKVS") { disableRemoteKVS = strcmp(value.c_str(), "true") == 0; } + if (attrib == "disableEncryption") { + disableEncryption = strcmp(value.c_str(), "true") == 0; + } if (attrib == "restartInfoLocation") { isFirstTestInRestart = true; } @@ -297,6 +300,8 @@ public: bool disableHostname = false; // remote key value store is a child process spawned by the SS process to run the storage engine bool disableRemoteKVS = false; + // 7.2 cannot be downgraded to 7.1 or below after enabling encryption-at-rest. + bool disableEncryption = false; // Storage Engine Types: Verify match with SimulationConfig::generateNormalConfig // 0 = "ssd" // 1 = "memory" @@ -358,6 +363,7 @@ public: .add("disableTss", &disableTss) .add("disableHostname", &disableHostname) .add("disableRemoteKVS", &disableRemoteKVS) + .add("disableEncryption", &disableEncryption) .add("simpleConfig", &simpleConfig) .add("generateFearless", &generateFearless) .add("datacenters", &datacenters) @@ -1091,10 +1097,15 @@ ACTOR Future restartSimulatedSystem(std::vector>* systemActor INetworkConnections::net()->parseMockDNSFromString(mockDNSStr); } } + auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); if (testConfig.disableRemoteKVS) { - IKnobCollection::getMutableGlobalKnobCollection().setKnob("remote_kv_store", - KnobValueRef::create(bool{ false })); - TraceEvent(SevDebug, "DisaableRemoteKVS").log(); + g_knobs.setKnob("remote_kv_store", KnobValueRef::create(bool{ false })); + TraceEvent(SevDebug, "DisableRemoteKVS"); + } + if (testConfig.disableEncryption) { + g_knobs.setKnob("enable_encryption", KnobValueRef::create(bool{ false })); + g_knobs.setKnob("enable_tlog_encryption", KnobValueRef::create(bool{ false })); + TraceEvent(SevDebug, "DisableEncryption"); } *pConnString = conn; *pTesterCount = testerCount; @@ -1860,10 +1871,15 @@ void setupSimulatedSystem(std::vector>* systemActors, if (testConfig.configureLocked) { startingConfigString += " locked"; } + auto& g_knobs = IKnobCollection::getMutableGlobalKnobCollection(); if (testConfig.disableRemoteKVS) { - IKnobCollection::getMutableGlobalKnobCollection().setKnob("remote_kv_store", - KnobValueRef::create(bool{ false })); - TraceEvent(SevDebug, "DisaableRemoteKVS").log(); + g_knobs.setKnob("remote_kv_store", KnobValueRef::create(bool{ false })); + TraceEvent(SevDebug, "DisableRemoteKVS"); + } + if (testConfig.disableEncryption) { + g_knobs.setKnob("enable_encryption", KnobValueRef::create(bool{ false })); + g_knobs.setKnob("enable_tlog_encryption", KnobValueRef::create(bool{ false })); + TraceEvent(SevDebug, "DisableEncryption"); } auto configDBType = testConfig.getConfigDBType(); for (auto kv : startingConfigJSON) { diff --git a/fdbserver/Status.actor.cpp b/fdbserver/Status.actor.cpp index 7c705a4055..93f750b141 100644 --- a/fdbserver/Status.actor.cpp +++ b/fdbserver/Status.actor.cpp @@ -817,7 +817,7 @@ ACTOR static Future processStatusFetcher( roles.addRole("blob_manager", db->get().blobManager.get()); } - if ((SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) && db->get().encryptKeyProxy.present()) { + if (SERVER_KNOBS->ENABLE_ENCRYPTION && db->get().encryptKeyProxy.present()) { roles.addRole("encrypt_key_proxy", db->get().encryptKeyProxy.get()); } diff --git a/fdbserver/fdbserver.actor.cpp b/fdbserver/fdbserver.actor.cpp index 6b162d2ccb..b87a03eff8 100644 --- a/fdbserver/fdbserver.actor.cpp +++ b/fdbserver/fdbserver.actor.cpp @@ -2121,6 +2121,10 @@ int main(int argc, char* argv[]) { } } } + g_knobs.setKnob("enable_encryption", + KnobValue::create(ini.GetBoolValue("META", "enableEncryption", false))); + g_knobs.setKnob("enable_tlog_encryption", + KnobValue::create(ini.GetBoolValue("META", "enableTLogEncryption", false))); } setupAndRun(dataFolder, opts.testFile, opts.restarting, (isRestoring >= 1), opts.whitelistBinPaths); g_simulator.run(); diff --git a/fdbserver/include/fdbserver/EncryptedMutationMessage.h b/fdbserver/include/fdbserver/EncryptedMutationMessage.h index d94e7f7f1f..a07643909d 100644 --- a/fdbserver/include/fdbserver/EncryptedMutationMessage.h +++ b/fdbserver/include/fdbserver/EncryptedMutationMessage.h @@ -24,6 +24,7 @@ #pragma once #include "fdbclient/CommitTransaction.h" +#include "fdbserver/Knobs.h" #include "flow/BlobCipher.h" struct EncryptedMutationMessage { @@ -96,6 +97,7 @@ struct EncryptedMutationMessage { Arena& arena, const std::unordered_map>& cipherKeys, StringRef* buf = nullptr) { + ASSERT(SERVER_KNOBS->ENABLE_ENCRYPTION); EncryptedMutationMessage msg; ar >> msg; auto textCipherItr = cipherKeys.find(msg.header.cipherTextDetails); diff --git a/fdbserver/tester.actor.cpp b/fdbserver/tester.actor.cpp index e8cbbc3f85..869bdef545 100644 --- a/fdbserver/tester.actor.cpp +++ b/fdbserver/tester.actor.cpp @@ -1172,7 +1172,10 @@ std::map> testSpecGlobalKey { "disableTss", [](const std::string& value) { TraceEvent("TestParserTest").detail("ParsedDisableTSS", ""); } }, { "disableHostname", [](const std::string& value) { TraceEvent("TestParserTest").detail("ParsedDisableHostname", ""); } }, - { "disableRemoteKVS", [](const std::string& value) { TraceEvent("TestParserTest").detail("ParsedRemoteKVS", ""); } } + { "disableRemoteKVS", + [](const std::string& value) { TraceEvent("TestParserTest").detail("ParsedRemoteKVS", ""); } }, + { "disableEncryption", + [](const std::string& value) { TraceEvent("TestParserTest").detail("ParsedRemoteKVS", ""); } } }; std::map> testSpecTestKeys = { diff --git a/fdbserver/workloads/ConsistencyCheck.actor.cpp b/fdbserver/workloads/ConsistencyCheck.actor.cpp index baad2e549c..83479c2beb 100644 --- a/fdbserver/workloads/ConsistencyCheck.actor.cpp +++ b/fdbserver/workloads/ConsistencyCheck.actor.cpp @@ -2380,7 +2380,7 @@ struct ConsistencyCheckWorkload : TestWorkload { } // Check EncryptKeyProxy - if ((SERVER_KNOBS->ENABLE_ENCRYPTION || g_network->isSimulated()) && db.encryptKeyProxy.present() && + if (SERVER_KNOBS->ENABLE_ENCRYPTION && db.encryptKeyProxy.present() && (!nonExcludedWorkerProcessMap.count(db.encryptKeyProxy.get().address()) || nonExcludedWorkerProcessMap[db.encryptKeyProxy.get().address()].processClass.machineClassFitness( ProcessClass::EncryptKeyProxy) > fitnessLowerBound)) { diff --git a/fdbserver/workloads/SaveAndKill.actor.cpp b/fdbserver/workloads/SaveAndKill.actor.cpp index 9ef8252db7..c9187ce6ff 100644 --- a/fdbserver/workloads/SaveAndKill.actor.cpp +++ b/fdbserver/workloads/SaveAndKill.actor.cpp @@ -19,6 +19,7 @@ */ #include "fdbclient/NativeAPI.actor.h" +#include "fdbserver/Knobs.h" #include "fdbserver/TesterInterface.actor.h" #include "fdbserver/workloads/workloads.actor.h" #include "fdbrpc/simulator.h" @@ -67,6 +68,9 @@ struct SaveAndKillWorkload : TestWorkload { ini.SetValue("META", "tssMode", format("%d", g_simulator.tssMode).c_str()); ini.SetValue("META", "mockDNS", INetworkConnections::net()->convertMockDNSToString().c_str()); + ini.SetBoolValue("META", "enableEncryption", SERVER_KNOBS->ENABLE_ENCRYPTION); + ini.SetBoolValue("META", "enableTLogEncryption", SERVER_KNOBS->ENABLE_TLOG_ENCRYPTION); + std::vector processes = g_simulator.getAllProcesses(); std::map rebootingProcesses = g_simulator.currentlyRebootingProcesses; std::map allProcessesMap; diff --git a/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml b/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml index b5fdf679ab..234c1c1136 100644 --- a/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml +++ b/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml @@ -2,6 +2,7 @@ extraMachineCountDC = 2 maxTLogVersion=6 disableHostname=true +disableEncryption=true storageEngineExcludeTypes=[4] [[test]] diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-1.txt b/tests/restarting/to_7.1.0/CycleTestRestart-1.txt index 4b5c917a1d..e2aeaa4291 100644 --- a/tests/restarting/to_7.1.0/CycleTestRestart-1.txt +++ b/tests/restarting/to_7.1.0/CycleTestRestart-1.txt @@ -2,6 +2,7 @@ storageEngineExcludeTypes=-1,-2,3 maxTLogVersion=6 disableTss=true disableHostname=true +disableEncryption=true testTitle=Clogged clearAfterTest=false