Merge pull request #1644 from Expensify/main

Update expensify_prod branch

BedrockCommand.cpp

@@ -1,9 +1,12 @@
 #include <libstuff/libstuff.h>
+#include <libstuff/SHTTPSManager.h>
 #include "BedrockCommand.h"
 #include "BedrockPlugin.h"
 
 atomic<size_t> BedrockCommand::_commandCount(0);
 
+SStandaloneHTTPSManager BedrockCommand::_noopHTTPSManager;
+
 const string BedrockCommand::defaultPluginName("NO_PLUGIN");
 
 BedrockCommand::BedrockCommand(SQLiteCommand&& baseCommand, BedrockPlugin* plugin, bool escalateImmediately_) :
@@ -303,3 +306,59 @@ void BedrockCommand::setTimeout(uint64_t timeoutDurationMS) {
 bool BedrockCommand::shouldCommitEmptyTransactions() const {
     return _commitEmptyTransactions;
 }
+
+void BedrockCommand::deserializeHTTPSRequests(const string& serializedHTTPSRequests) {
+    if (serializedHTTPSRequests.empty()) {
+        return;
+    }
+
+    list<string> requests = SParseJSONArray(serializedHTTPSRequests);
+    for (const string& requestStr : requests) {
+        STable requestMap = SParseJSONObject(requestStr);
+
+        SHTTPSManager::Transaction* httpsRequest = new SHTTPSManager::Transaction(_noopHTTPSManager, request["requestID"]);
+        httpsRequest->s = nullptr;
+        httpsRequest->created = SToUInt64(requestMap["created"]);
+        httpsRequest->finished = SToUInt64(requestMap["finished"]);
+        httpsRequest->timeoutAt = SToUInt64(requestMap["timeoutAt"]);
+        httpsRequest->sentTime = SToUInt64(requestMap["sentTime"]);
+        httpsRequest->response = SToInt(requestMap["response"]);
+        httpsRequest->fullRequest.deserialize(SDecodeBase64(requestMap["fullRequest"]));
+        httpsRequest->fullResponse.deserialize(SDecodeBase64(requestMap["fullResponse"]));
+
+        httpsRequests.push_back(httpsRequest);
+
+        // These should never be incomplete when passed with a serialized command.
+        if (!httpsRequest->response) {
+            SWARN("Received incomplete HTTPS request.");
+        }
+    }
+}
+
+string BedrockCommand::serializeHTTPSRequests() {
+    if (!httpsRequests.size()) {
+        return "";
+    }
+
+    list<string> requests;
+    for (const auto& httpsRequest : httpsRequests) {
+        STable data;
+        data["created"] = to_string(httpsRequest->created);
+        data["finished"] = to_string(httpsRequest->finished);
+        data["timeoutAt"] = to_string(httpsRequest->timeoutAt);
+        data["sentTime"] = to_string(httpsRequest->sentTime);
+        data["response"] = to_string(httpsRequest->response);
+        data["fullRequest"] = SEncodeBase64(httpsRequest->fullRequest.serialize());
+        data["fullResponse"] = SEncodeBase64(httpsRequest->fullResponse.serialize());
+        requests.push_back(SComposeJSONObject(data));
+    }
+
+    return SComposeJSONArray(requests);
+}
+
+string BedrockCommand::serializeData() const {
+    return "";
+}
+
+void BedrockCommand::deserializeData(const string& data) {
+}

BedrockCommand.h

@@ -80,6 +80,12 @@ class BedrockCommand : public SQLiteCommand {
     // Return the name of the plugin for this command.
     const string& getName() const;
 
+    // Take all of the HTTPS requests attached to this object, and serialize them to a string.
+    string serializeHTTPSRequests();
+
+    // Take a serialized list of HTTPS requests as from `serializeHTTPSRequests` and deserialize them into the `httpsRequests` object.
+    void deserializeHTTPSRequests(const string& serializedHTTPSRequests);
+
     // Bedrock will call this before each `processCommand` (note: not `peekCommand`) for each plugin to allow it to
     // enable query rewriting. If a plugin would like to enable query rewriting, this should return true, and it should
     // set the rewriteHandler it would like to use.
@@ -185,6 +191,9 @@ class BedrockCommand : public SQLiteCommand {
     // Return the number of commands in existence.
     static size_t getCommandCount() { return _commandCount.load(); }
 
+    virtual string serializeData() const;
+    virtual void deserializeData(const string& data);
+
     // True if this command should be escalated immediately. This can be true for any command that does all of its work
     // in `process` instead of peek, as it will always be escalated to leader
     const bool escalateImmediately;
@@ -231,5 +240,7 @@ class BedrockCommand : public SQLiteCommand {
 
     static atomic<size_t> _commandCount;
 
+    static SStandaloneHTTPSManager _noopHTTPSManager;
+
     static const string defaultPluginName;
 };

BedrockPlugin.cpp

@@ -8,9 +8,6 @@ BedrockPlugin::BedrockPlugin(BedrockServer& s) : server(s) {
 }
 
 BedrockPlugin::~BedrockPlugin() {
-    for (auto httpsManager : httpsManagers) {
-        delete httpsManager;
-    }
 }
 
 bool BedrockPlugin::isValidDate(const string& date)

BedrockPlugin.h

@@ -38,10 +38,6 @@ class BedrockPlugin {
     // Called at some point during initiation to allow the plugin to verify/change the database schema.
     virtual void upgradeDatabase(SQLite& db);
 
-    // A list of SHTTPSManagers that the plugin would like the server to watch for activity. It is only guaranteed to
-    // be safe to modify this list during `initialize`.
-    list<SHTTPSManager*> httpsManagers;
-
     // The plugin can register any number of timers it wants. When any of them `ding`, then the `timerFired`
     // function will be called, and passed the timer that is dinging.
     set<SStopwatch*> timers;

BedrockServer.cpp

@@ -944,19 +944,6 @@ void BedrockServer::runCommand(unique_ptr<BedrockCommand>&& _command, bool isBlo
                 // write it. We'll flag that here, to keep the node from falling out of LEADING/STANDINGDOWN
                 // until we're finished with this command.
                 if (command->httpsRequests.size()) {
-                    // This *should* be impossible, but previous bugs have existed where it's feasible that we call
-                    // `peekCommand` while leading, and by the time we're done, we're FOLLOWING, so we check just
-                    // in case we ever introduce another similar bug.
-                    if (state != SQLiteNodeState::LEADING && state != SQLiteNodeState::STANDINGDOWN) {
-                        SALERT("Not leading or standing down (" << SQLiteNode::stateName(state)
-                               << ") but have outstanding HTTPS command: " << command->request.methodLine
-                               << ", returning 500.");
-                        command->response.methodLine = "500 STANDDOWN TIMEOUT";
-                        _reply(command);
-                        core.rollback();
-                        break;
-                    }
-
                     if (command->repeek || !command->areHttpsRequestsComplete()) {
                         // Roll back the existing transaction, but only if we are inside an transaction
                         if (calledPeek) {
@@ -2170,8 +2157,25 @@ unique_ptr<BedrockCommand> BedrockServer::buildCommandFromRequest(SData&& reques
         request["_source"] = ip;
     }
 
+    // Pull any serialized https requests off the requests object to apply to the command.
+    string serializedHTTPSRequests = request["httpsRequests"];
+    string serializedData = request["serializedData"];
+    request.erase("httpsRequests");
+    request.erase("serializedData");
+
     // Create a command.
     unique_ptr<BedrockCommand> command = getCommandFromPlugins(move(request));
+
+    // Apply HTTPS requests. If we had any, pretend we peeked this command in our current (likely LEADING) state.
+    if (serializedHTTPSRequests.size()) {
+        command->deserializeHTTPSRequests(serializedHTTPSRequests);
+        command->lastPeekedOrProcessedInState = _syncNode->getState();
+        SINFO("Deserialized " << command->httpsRequests.size() << " HTTPS requests for command " << command->request.methodLine << ".");
+    }
+    if (serializedData.size()) {
+        command->deserializeData(serializedData);
+    }
+
     SDEBUG("Deserialized command " << command->request.methodLine);
     command->socket = fireAndForget ? nullptr : &socket;
 

libstuff/SHTTPSManager.cpp

@@ -8,13 +8,11 @@
 
 SHTTPSManager::SHTTPSManager(BedrockPlugin& plugin_) : plugin(plugin_)
 {
-    plugin.httpsManagers.push_back(this);
 }
 
 SHTTPSManager::SHTTPSManager(BedrockPlugin& plugin_, const string& pem, const string& srvCrt, const string& caCrt)
   : SStandaloneHTTPSManager(pem, srvCrt, caCrt), plugin(plugin_)
 {
-    plugin.httpsManagers.push_back(this);
 }
 
 void SHTTPSManager::validate() {
@@ -102,7 +100,7 @@ void SStandaloneHTTPSManager::postPoll(fd_map& fdm, SStandaloneHTTPSManager::Tra
         // content. Why this is the what constitutes a valid response is lost to time. Any well-formed response should
         // be valid here, and this should get cleaned up. However, this requires testing anything that might rely on
         // the existing behavior, which is an exercise for later.
-        if (handleAllResponses() || SContains(transaction.fullResponse.methodLine, " 200 ") || transaction.fullResponse.content.size()) {
+        if (SContains(transaction.fullResponse.methodLine, " 200 ") || transaction.fullResponse.content.size()) {
             // Pass the transaction down to the subclass.
             _onRecv(&transaction);
         } else {
@@ -133,16 +131,17 @@ void SStandaloneHTTPSManager::postPoll(fd_map& fdm, SStandaloneHTTPSManager::Tra
     }
 }
 
-SStandaloneHTTPSManager::Transaction::Transaction(SStandaloneHTTPSManager& manager_) :
+SStandaloneHTTPSManager::Transaction::Transaction(SStandaloneHTTPSManager& manager_, const string& requestID) :
     s(nullptr),
     created(STimeNow()),
     finished(0),
     timeoutAt(0),
     response(0),
     manager(manager_),
     sentTime(0),
-    requestID(SThreadLogPrefix)
+    requestID(requestID.empty() ? SThreadLogPrefix : requestID)
 {
+    // TODO: Remove this block to to enable HTTPS on followers. Also the `validate` method can be removed entirely.
     manager.validate();
 }
 

libstuff/SHTTPSManager.h

@@ -9,7 +9,7 @@ class SStandaloneHTTPSManager : public STCPManager {
   public:
     struct Transaction {
         // Constructor/Destructor
-        Transaction(SStandaloneHTTPSManager& manager_);
+        Transaction(SStandaloneHTTPSManager& manager_, const string& requestID = "");
         ~Transaction();
 
         // Attributes
@@ -60,16 +60,14 @@ class SStandaloneHTTPSManager : public STCPManager {
     Transaction* _httpsSend(const string& url, const SData& request);
     Transaction* _createErrorTransaction();
     virtual bool _onRecv(Transaction* transaction);
-
-    // Historically we only call _onRecv for `200 OK` responses. This allows manangers to handle all responses.
-    virtual bool handleAllResponses() { return false; }
 };
 
 class SHTTPSManager : public SStandaloneHTTPSManager {
     public:
     SHTTPSManager(BedrockPlugin& plugin_);
     SHTTPSManager(BedrockPlugin& plugin_, const string& pem, const string& srvCrt, const string& caCrt);
 
+    // TODO: Remove this once Auth no longer checks for it.
     class NotLeading : public exception {
         const char* what() {
             return "Can't create SHTTPSManager::Transaction when not leading";

libstuff/libstuff.cpp

@@ -1460,7 +1460,6 @@ const char* _SParseJSONObject(const char* ptr, const char* end, STable& out, boo
         _JSONASSERTPTR(); // Make sure no parse error.
         if (populateOut) {
             // Got one more
-            SDEBUG("Parsed: '" << name << "':'" << value << "'");
             out[name] = value;
         }
         _JSONLOG();

sqlitecluster/SQLiteClusterMessenger.cpp

@@ -137,6 +137,17 @@ bool SQLiteClusterMessenger::_sendCommandOnSocket(SHTTPSManager::Socket& socket,
 
     // This is what we need to send.
     SData request = command.request;
+
+    // If the command has https requests, we serialize them to escalate. In this case we also check if the command has data
+    // that needs serialization, and if so, we serialize that as well.
+    if (command.httpsRequests.size()) {
+        request["httpsRequests"] = command.serializeHTTPSRequests();
+        string serializedData = command.serializeData();
+        if (serializedData.size()) {
+            request["serializedData"] = move(serializedData);
+        }
+    }
+
     request.nameValueMap["ID"] = command.id;
     SFastBuffer buf(request.serialize());
 

test/clustertest/testplugin/TestPlugin.cpp

@@ -223,10 +223,6 @@ bool TestPluginCommand::peek(SQLite& db) {
         response["stored_not_special"] = plugin().arbitraryData["not_special"];
         return true;
     } else if (SStartsWith(request.methodLine, "sendrequest")) {
-        if (plugin().server.getState() != SQLiteNodeState::LEADING && plugin().server.getState() != SQLiteNodeState::STANDINGDOWN) {
-            // Only start HTTPS requests on leader, otherwise, we'll escalate.
-            return false;
-        }
         int requestCount = 1;
         if (request.isSet("httpsRequestCount")) {
             requestCount = max(request.calc("httpsRequestCount"), 1);

test/clustertest/tests/HTTPSTest.cpp

@@ -35,7 +35,7 @@ struct HTTPSTest : tpunit::TestFixture {
     }
 
     void testMultipleRequests() {
-        BedrockTester& brtester = tester->getTester(0);
+        BedrockTester& brtester = tester->getTester(1);
         SData request("sendrequest");
         request["httpsRequestCount"] = to_string(3);
         auto result = brtester.executeWaitVerifyContent(request);
@@ -45,7 +45,7 @@ struct HTTPSTest : tpunit::TestFixture {
 
     void test() {
         // Send one request to verify that it works.
-        BedrockTester& brtester = tester->getTester(0);
+        BedrockTester& brtester = tester->getTester(1);
 
         SData request("sendrequest a");
         auto result = brtester.executeWaitVerifyContent(request);
@@ -56,7 +56,7 @@ struct HTTPSTest : tpunit::TestFixture {
         // to cause a conflict.
         mutex m;
 
-        // Every 10th request on leader is an HTTP request.
+        // Every 10th request is an HTTP request.
         int nthHasRequest = 10;
 
         // Let's spin up three threads, each spamming commands at one of our nodes.
@@ -66,7 +66,7 @@ struct HTTPSTest : tpunit::TestFixture {
             threads.emplace_back([this, i, nthHasRequest, &responses, &m](){
                 vector<SData> requests;
                 for (int j = 0; j < 200; j++) {
-                    if (i == 0 && (j % nthHasRequest == 0)) {
+                    if (j % nthHasRequest == 0) {
                         // They should throw all sorts of errors if they repeat HTTPS requests.
                         SData request("sendrequest b");
                         request["writeConsistency"] = "ASYNC";