From 69bff489d4ec80db3f8ba843b7d0f7ba6ff8656b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Mar 2018 12:42:08 -0700 Subject: [PATCH] Generate a random unique "Request ID" for SSH requests so processes can coordinate better Summary: Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree. The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users. Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export. Maniphest Tasks: T13109 Differential Revision: https://secure.phabricator.com/D19249 --- .../20180322.lock.01.identifier.sql | 5 ++++ scripts/repository/commit_hook.php | 5 ++++ scripts/ssh/ssh-exec.php | 7 +++++ .../PhabricatorAccessLogConfigOptions.php | 4 +++ .../engine/DiffusionCommitHookEngine.php | 26 ++++++++++++++++--- .../diffusion/ssh/DiffusionSSHWorkflow.php | 5 ++++ ...abricatorRepositoryPushLogSearchEngine.php | 4 +++ .../PhabricatorRepositoryPushEvent.php | 6 +++++ .../ssh/PhabricatorSSHWorkflow.php | 10 +++++++ 9 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 resources/sql/autopatches/20180322.lock.01.identifier.sql diff --git a/resources/sql/autopatches/20180322.lock.01.identifier.sql b/resources/sql/autopatches/20180322.lock.01.identifier.sql new file mode 100644 index 0000000000..b115a691fa --- /dev/null +++ b/resources/sql/autopatches/20180322.lock.01.identifier.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD requestIdentifier VARBINARY(12); + +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD UNIQUE KEY `key_request` (requestIdentifier); diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php index 51abcb6c89..07d6d7cfa2 100755 --- a/scripts/repository/commit_hook.php +++ b/scripts/repository/commit_hook.php @@ -187,6 +187,11 @@ if (strlen($remote_protocol)) { $engine->setRemoteProtocol($remote_protocol); } +$request_identifier = getenv(DiffusionCommitHookEngine::ENV_REQUEST); +if (strlen($request_identifier)) { + $engine->setRequestIdentifier($request_identifier); +} + try { $err = $engine->execute(); } catch (DiffusionCommitHookRejectException $ex) { diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index 2ff1bdc198..0f2275cda8 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -8,6 +8,12 @@ require_once $root.'/scripts/__init_script__.php'; $ssh_log = PhabricatorSSHLog::getLog(); +$request_identifier = Filesystem::readRandomCharacters(12); +$ssh_log->setData( + array( + 'Q' => $request_identifier, + )); + $args = new PhutilArgumentParser($argv); $args->setTagline(pht('execute SSH requests')); $args->setSynopsis(<<setSSHUser($user); $workflow->setOriginalArguments($original_argv); $workflow->setIsClusterRequest($is_cluster_request); + $workflow->setRequestIdentifier($request_identifier); $sock_stdin = fopen('php://stdin', 'r'); if (!$sock_stdin) { diff --git a/src/applications/config/option/PhabricatorAccessLogConfigOptions.php b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php index 9ae60825ea..2b9c4bbc75 100644 --- a/src/applications/config/option/PhabricatorAccessLogConfigOptions.php +++ b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php @@ -47,6 +47,10 @@ final class PhabricatorAccessLogConfigOptions 's' => pht('The system user.'), 'S' => pht('The system sudo user.'), 'k' => pht('ID of the SSH key used to authenticate the request.'), + + // TODO: This is a reasonable thing to support in the HTTP access + // log, too. + 'Q' => pht('A random, unique string which identifies the request.'), ); $http_desc = pht( diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index a0769a51f0..cc4526dbdc 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -12,6 +12,7 @@ final class DiffusionCommitHookEngine extends Phobject { const ENV_REPOSITORY = 'PHABRICATOR_REPOSITORY'; const ENV_USER = 'PHABRICATOR_USER'; + const ENV_REQUEST = 'PHABRICATOR_REQUEST'; const ENV_REMOTE_ADDRESS = 'PHABRICATOR_REMOTE_ADDRESS'; const ENV_REMOTE_PROTOCOL = 'PHABRICATOR_REMOTE_PROTOCOL'; @@ -25,6 +26,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $subversionRepository; private $remoteAddress; private $remoteProtocol; + private $requestIdentifier; private $transactionKey; private $mercurialHook; private $mercurialCommits = array(); @@ -58,6 +60,15 @@ final class DiffusionCommitHookEngine extends Phobject { return $this->remoteAddress; } + public function setRequestIdentifier($request_identifier) { + $this->requestIdentifier = $request_identifier; + return $this; + } + + public function getRequestIdentifier() { + return $this->requestIdentifier; + } + public function setSubversionTransactionInfo($transaction, $repository) { $this->subversionTransaction = $transaction; $this->subversionRepository = $repository; @@ -620,6 +631,7 @@ final class DiffusionCommitHookEngine extends Phobject { $env = array( self::ENV_REPOSITORY => $this->getRepository()->getPHID(), self::ENV_USER => $this->getViewer()->getUsername(), + self::ENV_REQUEST => $this->getRequestIdentifier(), self::ENV_REMOTE_PROTOCOL => $this->getRemoteProtocol(), self::ENV_REMOTE_ADDRESS => $this->getRemoteAddress(), ); @@ -1081,16 +1093,24 @@ final class DiffusionCommitHookEngine extends Phobject { ->setDevicePHID($device_phid) ->setRepositoryPHID($this->getRepository()->getPHID()) ->attachRepository($this->getRepository()) - ->setEpoch(time()); + ->setEpoch(PhabricatorTime::getNow()); } private function newPushEvent() { $viewer = $this->getViewer(); - return PhabricatorRepositoryPushEvent::initializeNewEvent($viewer) + + $event = PhabricatorRepositoryPushEvent::initializeNewEvent($viewer) ->setRepositoryPHID($this->getRepository()->getPHID()) ->setRemoteAddress($this->getRemoteAddress()) ->setRemoteProtocol($this->getRemoteProtocol()) - ->setEpoch(time()); + ->setEpoch(PhabricatorTime::getNow()); + + $identifier = $this->getRequestIdentifier(); + if (strlen($identifier)) { + $event->setRequestIdentifier($identifier); + } + + return $event; } private function rejectEnormousChanges(array $content_updates) { diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index e40d8e1f51..baf1749252 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -30,6 +30,11 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh', ); + $identifier = $this->getRequestIdentifier(); + if ($identifier !== null) { + $env[DiffusionCommitHookEngine::ENV_REQUEST] = $identifier; + } + $remote_address = $this->getSSHRemoteAddress(); if ($remote_address !== null) { $env[DiffusionCommitHookEngine::ENV_REMOTE_ADDRESS] = $remote_address; diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php index 8ad76987f9..5bf2c84aeb 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php @@ -101,6 +101,9 @@ final class PhabricatorRepositoryPushLogSearchEngine $fields[] = id(new PhabricatorIDExportField()) ->setKey('pushID') ->setLabel(pht('Push ID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('unique') + ->setLabel(pht('Unique')), $fields[] = id(new PhabricatorStringExportField()) ->setKey('protocol') ->setLabel(pht('Protocol')), @@ -209,6 +212,7 @@ final class PhabricatorRepositoryPushLogSearchEngine $map = array( 'pushID' => $event->getID(), + 'unique' => $event->getRequestIdentifier(), 'protocol' => $event->getRemoteProtocol(), 'repositoryPHID' => $repository_phid, 'repository' => $repository_name, diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php index 2bc751ffca..369af15635 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php @@ -11,6 +11,7 @@ final class PhabricatorRepositoryPushEvent protected $repositoryPHID; protected $epoch; protected $pusherPHID; + protected $requestIdentifier; protected $remoteAddress; protected $remoteProtocol; protected $rejectCode; @@ -29,6 +30,7 @@ final class PhabricatorRepositoryPushEvent self::CONFIG_AUX_PHID => true, self::CONFIG_TIMESTAMPS => false, self::CONFIG_COLUMN_SCHEMA => array( + 'requestIdentifier' => 'bytes12?', 'remoteAddress' => 'ipaddress?', 'remoteProtocol' => 'text32?', 'rejectCode' => 'uint32', @@ -38,6 +40,10 @@ final class PhabricatorRepositoryPushEvent 'key_repository' => array( 'columns' => array('repositoryPHID'), ), + 'key_request' => array( + 'columns' => array('requestIdentifier'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } diff --git a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php index 6b6222304c..f7739f1332 100644 --- a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php +++ b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php @@ -13,6 +13,7 @@ abstract class PhabricatorSSHWorkflow private $errorChannel; private $isClusterRequest; private $originalArguments; + private $requestIdentifier; public function isExecutable() { return false; @@ -89,6 +90,15 @@ abstract class PhabricatorSSHWorkflow return $this->originalArguments; } + public function setRequestIdentifier($request_identifier) { + $this->requestIdentifier = $request_identifier; + return $this; + } + + public function getRequestIdentifier() { + return $this->requestIdentifier; + } + public function getSSHRemoteAddress() { $ssh_client = getenv('SSH_CLIENT'); if (!strlen($ssh_client)) {