Skip to content

Commit 69bff48

Browse files
author
epriestley
committedMar 22, 2018
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
1 parent e010aac commit 69bff48

File tree

9 files changed

+69
-3
lines changed

9 files changed

+69
-3
lines changed
 
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
ALTER TABLE {$NAMESPACE}_repository.repository_pushevent
2+
ADD requestIdentifier VARBINARY(12);
3+
4+
ALTER TABLE {$NAMESPACE}_repository.repository_pushevent
5+
ADD UNIQUE KEY `key_request` (requestIdentifier);

‎scripts/repository/commit_hook.php

+5
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,11 @@
187187
$engine->setRemoteProtocol($remote_protocol);
188188
}
189189

190+
$request_identifier = getenv(DiffusionCommitHookEngine::ENV_REQUEST);
191+
if (strlen($request_identifier)) {
192+
$engine->setRequestIdentifier($request_identifier);
193+
}
194+
190195
try {
191196
$err = $engine->execute();
192197
} catch (DiffusionCommitHookRejectException $ex) {

‎scripts/ssh/ssh-exec.php

+7
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@
88

99
$ssh_log = PhabricatorSSHLog::getLog();
1010

11+
$request_identifier = Filesystem::readRandomCharacters(12);
12+
$ssh_log->setData(
13+
array(
14+
'Q' => $request_identifier,
15+
));
16+
1117
$args = new PhutilArgumentParser($argv);
1218
$args->setTagline(pht('execute SSH requests'));
1319
$args->setSynopsis(<<<EOSYNOPSIS
@@ -248,6 +254,7 @@
248254
$workflow->setSSHUser($user);
249255
$workflow->setOriginalArguments($original_argv);
250256
$workflow->setIsClusterRequest($is_cluster_request);
257+
$workflow->setRequestIdentifier($request_identifier);
251258

252259
$sock_stdin = fopen('php://stdin', 'r');
253260
if (!$sock_stdin) {

‎src/applications/config/option/PhabricatorAccessLogConfigOptions.php

+4
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ public function getOptions() {
4747
's' => pht('The system user.'),
4848
'S' => pht('The system sudo user.'),
4949
'k' => pht('ID of the SSH key used to authenticate the request.'),
50+
51+
// TODO: This is a reasonable thing to support in the HTTP access
52+
// log, too.
53+
'Q' => pht('A random, unique string which identifies the request.'),
5054
);
5155

5256
$http_desc = pht(

‎src/applications/diffusion/engine/DiffusionCommitHookEngine.php

+23-3
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ final class DiffusionCommitHookEngine extends Phobject {
1212

1313
const ENV_REPOSITORY = 'PHABRICATOR_REPOSITORY';
1414
const ENV_USER = 'PHABRICATOR_USER';
15+
const ENV_REQUEST = 'PHABRICATOR_REQUEST';
1516
const ENV_REMOTE_ADDRESS = 'PHABRICATOR_REMOTE_ADDRESS';
1617
const ENV_REMOTE_PROTOCOL = 'PHABRICATOR_REMOTE_PROTOCOL';
1718

@@ -25,6 +26,7 @@ final class DiffusionCommitHookEngine extends Phobject {
2526
private $subversionRepository;
2627
private $remoteAddress;
2728
private $remoteProtocol;
29+
private $requestIdentifier;
2830
private $transactionKey;
2931
private $mercurialHook;
3032
private $mercurialCommits = array();
@@ -58,6 +60,15 @@ public function getRemoteAddress() {
5860
return $this->remoteAddress;
5961
}
6062

63+
public function setRequestIdentifier($request_identifier) {
64+
$this->requestIdentifier = $request_identifier;
65+
return $this;
66+
}
67+
68+
public function getRequestIdentifier() {
69+
return $this->requestIdentifier;
70+
}
71+
6172
public function setSubversionTransactionInfo($transaction, $repository) {
6273
$this->subversionTransaction = $transaction;
6374
$this->subversionRepository = $repository;
@@ -620,6 +631,7 @@ private function applyCustomHooks(array $updates) {
620631
$env = array(
621632
self::ENV_REPOSITORY => $this->getRepository()->getPHID(),
622633
self::ENV_USER => $this->getViewer()->getUsername(),
634+
self::ENV_REQUEST => $this->getRequestIdentifier(),
623635
self::ENV_REMOTE_PROTOCOL => $this->getRemoteProtocol(),
624636
self::ENV_REMOTE_ADDRESS => $this->getRemoteAddress(),
625637
);
@@ -1081,16 +1093,24 @@ private function newPushLog() {
10811093
->setDevicePHID($device_phid)
10821094
->setRepositoryPHID($this->getRepository()->getPHID())
10831095
->attachRepository($this->getRepository())
1084-
->setEpoch(time());
1096+
->setEpoch(PhabricatorTime::getNow());
10851097
}
10861098

10871099
private function newPushEvent() {
10881100
$viewer = $this->getViewer();
1089-
return PhabricatorRepositoryPushEvent::initializeNewEvent($viewer)
1101+
1102+
$event = PhabricatorRepositoryPushEvent::initializeNewEvent($viewer)
10901103
->setRepositoryPHID($this->getRepository()->getPHID())
10911104
->setRemoteAddress($this->getRemoteAddress())
10921105
->setRemoteProtocol($this->getRemoteProtocol())
1093-
->setEpoch(time());
1106+
->setEpoch(PhabricatorTime::getNow());
1107+
1108+
$identifier = $this->getRequestIdentifier();
1109+
if (strlen($identifier)) {
1110+
$event->setRequestIdentifier($identifier);
1111+
}
1112+
1113+
return $event;
10941114
}
10951115

10961116
private function rejectEnormousChanges(array $content_updates) {

‎src/applications/diffusion/ssh/DiffusionSSHWorkflow.php

+5
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ public function getEnvironment() {
3030
DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh',
3131
);
3232

33+
$identifier = $this->getRequestIdentifier();
34+
if ($identifier !== null) {
35+
$env[DiffusionCommitHookEngine::ENV_REQUEST] = $identifier;
36+
}
37+
3338
$remote_address = $this->getSSHRemoteAddress();
3439
if ($remote_address !== null) {
3540
$env[DiffusionCommitHookEngine::ENV_REMOTE_ADDRESS] = $remote_address;

‎src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php

+4
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,9 @@ protected function newExportFields() {
101101
$fields[] = id(new PhabricatorIDExportField())
102102
->setKey('pushID')
103103
->setLabel(pht('Push ID')),
104+
$fields[] = id(new PhabricatorStringExportField())
105+
->setKey('unique')
106+
->setLabel(pht('Unique')),
104107
$fields[] = id(new PhabricatorStringExportField())
105108
->setKey('protocol')
106109
->setLabel(pht('Protocol')),
@@ -209,6 +212,7 @@ protected function newExportData(array $logs) {
209212

210213
$map = array(
211214
'pushID' => $event->getID(),
215+
'unique' => $event->getRequestIdentifier(),
212216
'protocol' => $event->getRemoteProtocol(),
213217
'repositoryPHID' => $repository_phid,
214218
'repository' => $repository_name,

‎src/applications/repository/storage/PhabricatorRepositoryPushEvent.php

+6
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ final class PhabricatorRepositoryPushEvent
1111
protected $repositoryPHID;
1212
protected $epoch;
1313
protected $pusherPHID;
14+
protected $requestIdentifier;
1415
protected $remoteAddress;
1516
protected $remoteProtocol;
1617
protected $rejectCode;
@@ -29,6 +30,7 @@ protected function getConfiguration() {
2930
self::CONFIG_AUX_PHID => true,
3031
self::CONFIG_TIMESTAMPS => false,
3132
self::CONFIG_COLUMN_SCHEMA => array(
33+
'requestIdentifier' => 'bytes12?',
3234
'remoteAddress' => 'ipaddress?',
3335
'remoteProtocol' => 'text32?',
3436
'rejectCode' => 'uint32',
@@ -38,6 +40,10 @@ protected function getConfiguration() {
3840
'key_repository' => array(
3941
'columns' => array('repositoryPHID'),
4042
),
43+
'key_request' => array(
44+
'columns' => array('requestIdentifier'),
45+
'unique' => true,
46+
),
4147
),
4248
) + parent::getConfiguration();
4349
}

‎src/infrastructure/ssh/PhabricatorSSHWorkflow.php

+10
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ abstract class PhabricatorSSHWorkflow
1313
private $errorChannel;
1414
private $isClusterRequest;
1515
private $originalArguments;
16+
private $requestIdentifier;
1617

1718
public function isExecutable() {
1819
return false;
@@ -89,6 +90,15 @@ public function getOriginalArguments() {
8990
return $this->originalArguments;
9091
}
9192

93+
public function setRequestIdentifier($request_identifier) {
94+
$this->requestIdentifier = $request_identifier;
95+
return $this;
96+
}
97+
98+
public function getRequestIdentifier() {
99+
return $this->requestIdentifier;
100+
}
101+
92102
public function getSSHRemoteAddress() {
93103
$ssh_client = getenv('SSH_CLIENT');
94104
if (!strlen($ssh_client)) {

0 commit comments

Comments
 (0)
Failed to load comments.