Skip to content

Commit d8550c1

Browse files
author
epriestley
committed
Promote instance identity to the upstream and pass it to commit hooks
Summary: Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance. For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook. For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN. Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small. I poked at trying to do this in a more general way, but: - We MUST know this BEFORE we run code, so the normal subclassing stuff is useless. - I couldn't come up with any other parameter which might ever be useful to pass in. Test Plan: Used `git push` to push code through proxied HTTP, got a clean push. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7019 Differential Revision: https://secure.phabricator.com/D11495
1 parent fb5e50e commit d8550c1

File tree

4 files changed

+57
-4
lines changed

4 files changed

+57
-4
lines changed

scripts/repository/commit_hook.php

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,25 @@
11
#!/usr/bin/env php
22
<?php
33

4+
// Commit hooks execute in an unusual context where the environment may be
5+
// unavailable, particularly in SVN. The first parameter to this script is
6+
// either a bare repository identifier ("X"), or a repository identifier
7+
// followed by an instance identifier ("X:instance"). If we have an instance
8+
// identifier, unpack it into the environment before we start up. This allows
9+
// subclasses of PhabricatorConfigSiteSource to read it and build an instance
10+
// environment.
11+
12+
if ($argc > 1) {
13+
$context = $argv[1];
14+
$context = explode(':', $context, 2);
15+
$argv[1] = $context[0];
16+
17+
if (count($context) > 1) {
18+
$_ENV['PHABRICATOR_INSTANCE'] = $context[1];
19+
putenv('PHABRICATOR_INSTANCE='.$context[1]);
20+
}
21+
}
22+
423
$root = dirname(dirname(dirname(__FILE__)));
524
require_once $root.'/scripts/__init_script__.php';
625

src/applications/config/option/PhabricatorClusterConfigOptions.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,18 @@ public function getOptions() {
5252
'0.0.0.0/0',
5353
),
5454
pht('Allow Any Host (Insecure!)')),
55+
$this->newOption('cluster.instance', 'string', null)
56+
->setLocked(true)
57+
->setSummary(pht('Instance identifier for multi-tenant clusters.'))
58+
->setDescription(
59+
pht(
60+
'WARNING: This is a very advanced option, and only useful for '.
61+
'hosting providers running multi-tenant clusters.'.
62+
"\n\n".
63+
'If you provide an instance identifier here (normally by '.
64+
'injecting it with a `PhabricatorConfigSiteSource`), Phabricator '.
65+
'will pass it to subprocesses and commit hooks in the '.
66+
'`PHABRICATOR_INSTANCE` environmental variable.')),
5567
);
5668
}
5769

src/applications/repository/engine/PhabricatorRepositoryPullEngine.php

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ private function installHook($path) {
161161
$this->log('%s', pht('Installing commit hook to "%s"...', $path));
162162

163163
$repository = $this->getRepository();
164-
$callsign = $repository->getCallsign();
164+
$identifier = $this->getHookContextIdentifier($repository);
165165

166166
$root = dirname(phutil_get_library_root('phabricator'));
167167
$bin = $root.'/bin/commit-hook';
@@ -171,7 +171,7 @@ private function installHook($path) {
171171
'exec %s -f %s -- %s "$@"',
172172
$full_php_path,
173173
$bin,
174-
$callsign);
174+
$identifier);
175175

176176
$hook = "#!/bin/sh\nexport TERM=dumb\n{$cmd}\n";
177177

@@ -190,6 +190,17 @@ private function installHookDirectory($path) {
190190
Filesystem::writeFile($path.'/README', $readme);
191191
}
192192

193+
private function getHookContextIdentifier(PhabricatorRepository $repository) {
194+
$identifier = $repository->getCallsign();
195+
196+
$instance = PhabricatorEnv::getEnvConfig('cluster.instance');
197+
if (strlen($instance)) {
198+
$identifier = "{$identifier}:{$instance}";
199+
}
200+
201+
return $identifier;
202+
}
203+
193204

194205
/* -( Pulling Git Working Copies )----------------------------------------- */
195206

@@ -412,6 +423,8 @@ private function installMercurialHook() {
412423
$repository = $this->getRepository();
413424
$path = $repository->getLocalPath().'/.hg/hgrc';
414425

426+
$identifier = $this->getHookContextIdentifier($repository);
427+
415428
$root = dirname(phutil_get_library_root('phabricator'));
416429
$bin = $root.'/bin/commit-hook';
417430

@@ -422,14 +435,14 @@ private function installMercurialHook() {
422435
$data[] = csprintf(
423436
'pretxnchangegroup.phabricator = %s %s %s',
424437
$bin,
425-
$repository->getCallsign(),
438+
$identifier,
426439
'pretxnchangegroup');
427440

428441
// This one handles creating bookmarks.
429442
$data[] = csprintf(
430443
'prepushkey.phabricator = %s %s %s',
431444
$bin,
432-
$repository->getCallsign(),
445+
$identifier,
433446
'prepushkey');
434447

435448
$data[] = null;

src/infrastructure/env/PhabricatorEnv.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,15 @@ private static function initializeCommonEnvironment() {
112112
// subprocess environments.
113113
$_ENV['PATH'] = $env_path;
114114

115+
116+
// If an instance identifier is defined, write it into the environment so
117+
// it's available to subprocesses.
118+
$instance = PhabricatorEnv::getEnvConfig('cluster.instance');
119+
if (strlen($instance)) {
120+
putenv('PHABRICATOR_INSTANCE='.$instance);
121+
$_ENV['PHABRICATOR_INSTANCE'] = $instance;
122+
}
123+
115124
PhabricatorEventEngine::initialize();
116125

117126
$translation = PhabricatorEnv::newObjectFromConfig('translation.provider');

0 commit comments

Comments
 (0)