From 441c000b5c3d8a4dfe67616d2a281c58184ef3ea Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 25 May 2021 18:09:29 -0400 Subject: [PATCH] Factor remote process exec out of protocolRemoteGet(). This allows protocolRemoteExec() to be shimmed, which means the remote can be run as a child of the test process, simplifying coverage testing. The shim does not need SSH parameters, so also split those out into a separate function and update the tests to match. --- doc/xml/release.xml | 2 + src/protocol/helper.c | 132 ++++++++++++++++-------- test/src/module/protocol/protocolTest.c | 14 +-- 3 files changed, 98 insertions(+), 50 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 5ac835d8c..167495b69 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -211,6 +211,8 @@ + +

Add local processs shim.

diff --git a/src/protocol/helper.c b/src/protocol/helper.c index 62b3034e7..445578ee2 100644 --- a/src/protocol/helper.c +++ b/src/protocol/helper.c @@ -316,31 +316,6 @@ protocolRemoteParam(ProtocolStorageType protocolStorageType, unsigned int hostId // Is this a repo remote? bool isRepo = protocolStorageType == protocolStorageTypeRepo; - // Fixed parameters for ssh command - StringList *result = strLstNew(); - strLstAddZ(result, "-o"); - strLstAddZ(result, "LogLevel=error"); - strLstAddZ(result, "-o"); - strLstAddZ(result, "Compression=no"); - strLstAddZ(result, "-o"); - strLstAddZ(result, "PasswordAuthentication=no"); - - // Append port if specified - ConfigOption optHostPort = isRepo ? cfgOptRepoHostPort : cfgOptPgHostPort; - - if (cfgOptionIdxTest(optHostPort, hostIdx)) - { - strLstAddZ(result, "-p"); - strLstAdd(result, strNewFmt("%u", cfgOptionIdxUInt(optHostPort, hostIdx))); - } - - // Append user/host - strLstAdd( - result, - strNewFmt( - "%s@%s", strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHostUser : cfgOptPgHostUser, hostIdx)), - strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHost : cfgOptPgHost, hostIdx)))); - // Option replacements KeyValue *optionReplace = kvNew(); @@ -444,14 +419,99 @@ protocolRemoteParam(ProtocolStorageType protocolStorageType, unsigned int hostId // Add the remote type kvPut(optionReplace, VARSTRDEF(CFGOPT_REMOTE_TYPE), VARSTR(strIdToStr(protocolStorageType))); - StringList *commandExec = cfgExecParam(cfgCommand(), cfgCmdRoleRemote, optionReplace, false, true); - strLstInsert(commandExec, 0, cfgOptionIdxStr(isRepo ? cfgOptRepoHostCmd : cfgOptPgHostCmd, hostIdx)); - strLstAdd(result, strLstJoin(commandExec, " ")); + FUNCTION_LOG_RETURN(STRING_LIST, cfgExecParam(cfgCommand(), cfgCmdRoleRemote, optionReplace, false, true)); +} + +// Helper to add SSH parameters when executing the remote via SSH +static StringList * +protocolRemoteParamSsh(const ProtocolStorageType protocolStorageType, const unsigned int hostIdx) +{ + FUNCTION_LOG_BEGIN(logLevelDebug); + FUNCTION_LOG_PARAM(STRING_ID, protocolStorageType); + FUNCTION_LOG_PARAM(UINT, hostIdx); + FUNCTION_LOG_END(); + + StringList *result = NULL; + + MEM_CONTEXT_TEMP_BEGIN() + { + // Is this a repo remote? + bool isRepo = protocolStorageType == protocolStorageTypeRepo; + + // Fixed parameters for ssh command + result = strLstNew(); + strLstAddZ(result, "-o"); + strLstAddZ(result, "LogLevel=error"); + strLstAddZ(result, "-o"); + strLstAddZ(result, "Compression=no"); + strLstAddZ(result, "-o"); + strLstAddZ(result, "PasswordAuthentication=no"); + + // Append port if specified + ConfigOption optHostPort = isRepo ? cfgOptRepoHostPort : cfgOptPgHostPort; + + if (cfgOptionIdxTest(optHostPort, hostIdx)) + { + strLstAddZ(result, "-p"); + strLstAdd(result, strNewFmt("%u", cfgOptionIdxUInt(optHostPort, hostIdx))); + } + + // Append user/host + strLstAdd( + result, + strNewFmt( + "%s@%s", strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHostUser : cfgOptPgHostUser, hostIdx)), + strZ(cfgOptionIdxStr(isRepo ? cfgOptRepoHost : cfgOptPgHost, hostIdx)))); + + // Add remote command and parameters + StringList *paramList = protocolRemoteParam(protocolStorageType, hostIdx); + + strLstInsert(paramList, 0, cfgOptionIdxStr(isRepo ? cfgOptRepoHostCmd : cfgOptPgHostCmd, hostIdx)); + strLstAdd(result, strLstJoin(paramList, " ")); + + // Move to prior context + strLstMove(result, memContextPrior()); + } + MEM_CONTEXT_TEMP_END(); FUNCTION_LOG_RETURN(STRING_LIST, result); } /**********************************************************************************************************************************/ +// Helper to execute the local process. This is a separate function solely so that it can be shimmed during testing. +static void +protocolRemoteExec( + ProtocolHelperClient *const helper, const ProtocolStorageType protocolStorageType, const unsigned int hostIdx, + const unsigned int processId) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM_P(VOID, helper); + FUNCTION_TEST_PARAM(ENUM, protocolStorageType); + FUNCTION_TEST_PARAM(UINT, hostIdx); + FUNCTION_TEST_PARAM(UINT, processId); + FUNCTION_TEST_END(); + + ASSERT(helper != NULL); + + // Execute the protocol command + const char *const host = + strZ(cfgOptionIdxStr(protocolStorageType == protocolStorageTypeRepo ? cfgOptRepoHost : cfgOptPgHost, hostIdx)); + + helper->exec = execNew( + cfgOptionStr(cfgOptCmdSsh), protocolRemoteParamSsh(protocolStorageType, hostIdx), + strNewFmt(PROTOCOL_SERVICE_REMOTE "-%u process on '%s'", processId, host), cfgOptionUInt64(cfgOptProtocolTimeout)); + execOpen(helper->exec); + + // Create protocol object + helper->client = protocolClientNew( + strNewFmt(PROTOCOL_SERVICE_REMOTE "-%u protocol on '%s'", processId, host), PROTOCOL_SERVICE_REMOTE_STR, + execIoRead(helper->exec), execIoWrite(helper->exec)); + + protocolClientMove(helper->client, execMemContext(helper->exec)); + + FUNCTION_TEST_RETURN_VOID(); +} + ProtocolClient * protocolRemoteGet(ProtocolStorageType protocolStorageType, unsigned int hostIdx) { @@ -496,19 +556,7 @@ protocolRemoteGet(ProtocolStorageType protocolStorageType, unsigned int hostIdx) { MEM_CONTEXT_BEGIN(protocolHelper.memContext) { - unsigned int optHost = isRepo ? cfgOptRepoHost : cfgOptPgHost; - - // Execute the protocol command - protocolHelperClient->exec = execNew( - cfgOptionStr(cfgOptCmdSsh), protocolRemoteParam(protocolStorageType, hostIdx), - strNewFmt(PROTOCOL_SERVICE_REMOTE "-%u process on '%s'", processId, strZ(cfgOptionIdxStr(optHost, hostIdx))), - cfgOptionUInt64(cfgOptProtocolTimeout)); - execOpen(protocolHelperClient->exec); - - // Create protocol object - protocolHelperClient->client = protocolClientNew( - strNewFmt(PROTOCOL_SERVICE_REMOTE "-%u protocol on '%s'", processId, strZ(cfgOptionIdxStr(optHost, hostIdx))), - PROTOCOL_SERVICE_REMOTE_STR, execIoRead(protocolHelperClient->exec), execIoWrite(protocolHelperClient->exec)); + protocolRemoteExec(protocolHelperClient, protocolStorageType, hostIdx, processId); // Get cipher options from the remote if none are locally configured if (isRepo && cfgOptionIdxStrId(cfgOptRepoCipherType, hostIdx) == cipherTypeNone) @@ -526,8 +574,6 @@ protocolRemoteGet(ProtocolStorageType protocolStorageType, unsigned int hostIdx) cfgOptionIdxSet(cfgOptRepoCipherPass, hostIdx, cfgSourceConfig, varLstGet(optionList, 1)); } } - - protocolClientMove(protocolHelperClient->client, execMemContext(protocolHelperClient->exec)); } MEM_CONTEXT_END(); } diff --git a/test/src/module/protocol/protocolTest.c b/test/src/module/protocol/protocolTest.c index afcaec397..6ac5bedef 100644 --- a/test/src/module/protocol/protocolTest.c +++ b/test/src/module/protocol/protocolTest.c @@ -285,7 +285,7 @@ testRun(void) } // ***************************************************************************************************************************** - if (testBegin("protocolRemoteParam()")) + if (testBegin("protocolRemoteParam() and protocolRemoteParamSsh()")) { storagePutP(storageNewWriteP(storageTest, STRDEF("pgbackrest.conf")), bufNew(0)); @@ -303,7 +303,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypeRepo, 0), + protocolRemoteParamSsh(protocolStorageTypeRepo, 0), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\nrepo-host-user@repo-host\n" "pgbackrest --exec-id=1-test --log-level-console=off --log-level-file=off --log-level-stderr=error" " --pg1-path=/path/to/pg --process=0 --remote-type=repo --repo=1 --stanza=test1 archive-get:remote\n", @@ -326,7 +326,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypeRepo, 0), + protocolRemoteParamSsh(protocolStorageTypeRepo, 0), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\n-p\n444\nrepo-host-user@repo-host\n" "pgbackrest --config=/path/pgbackrest.conf --config-include-path=/path/include --config-path=/path/config" " --exec-id=1-test --log-level-console=off --log-level-file=info --log-level-stderr=error --log-subprocess" @@ -346,7 +346,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypeRepo, 0), + protocolRemoteParamSsh(protocolStorageTypeRepo, 0), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\npgbackrest@repo-host\n" "pgbackrest --exec-id=1-test --log-level-console=off --log-level-file=off --log-level-stderr=error" " --pg1-path=/path/to/pg --process=3 --remote-type=repo --repo=1 --stanza=test1 archive-get:remote\n", @@ -363,7 +363,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypePg, 0), + protocolRemoteParamSsh(protocolStorageTypePg, 0), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\npostgres@pg1-host\n" "pgbackrest --exec-id=1-test --log-level-console=off --log-level-file=off --log-level-stderr=error" " --pg1-path=/path/to/1 --process=0 --remote-type=pg --stanza=test1 backup:remote\n", @@ -385,7 +385,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypePg, 1), + protocolRemoteParamSsh(protocolStorageTypePg, 1), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\npostgres@pg2-host\n" "pgbackrest --exec-id=1-test --log-level-console=off --log-level-file=off --log-level-stderr=error" " --pg1-path=/path/to/2 --process=4 --remote-type=pg --stanza=test1 backup:remote\n", @@ -407,7 +407,7 @@ testRun(void) harnessCfgLoadRaw(strLstSize(argList), strLstPtr(argList)); TEST_RESULT_STRLST_Z( - protocolRemoteParam(protocolStorageTypePg, 1), + protocolRemoteParamSsh(protocolStorageTypePg, 1), "-o\nLogLevel=error\n-o\nCompression=no\n-o\nPasswordAuthentication=no\npostgres@pg3-host\n" "pgbackrest --exec-id=1-test --log-level-console=off --log-level-file=off --log-level-stderr=error" " --pg1-path=/path/to/3 --pg1-port=3333 --pg1-socket-path=/socket3 --process=4 --remote-type=pg --stanza=test1"