Differential D11036 Diff 26502 src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
Changeset View
Changeset View
Standalone View
Standalone View
src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
| <?php | <?php | ||||
| abstract class PhabricatorDaemonManagementWorkflow | abstract class PhabricatorDaemonManagementWorkflow | ||||
| extends PhabricatorManagementWorkflow { | extends PhabricatorManagementWorkflow { | ||||
| private $runDaemonsAsUser = null; | |||||
| protected final function loadAvailableDaemonClasses() { | protected final function loadAvailableDaemonClasses() { | ||||
| $loader = new PhutilSymbolLoader(); | $loader = new PhutilSymbolLoader(); | ||||
| return $loader | return $loader | ||||
| ->setAncestorClass('PhutilDaemon') | ->setAncestorClass('PhutilDaemon') | ||||
| ->setConcreteOnly(true) | ->setConcreteOnly(true) | ||||
| ->selectSymbolsWithoutLoading(); | ->selectSymbolsWithoutLoading(); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 84 Lines • ▼ Show 20 Lines | if (count($match) == 0) { | ||||
| "Specify a daemon unambiguously. Multiple daemons match '%s': %s.", | "Specify a daemon unambiguously. Multiple daemons match '%s': %s.", | ||||
| $substring, | $substring, | ||||
| implode(', ', $match))); | implode(', ', $match))); | ||||
| } | } | ||||
| return head($match); | return head($match); | ||||
| } | } | ||||
| protected final function launchDaemon($class, array $argv, $debug) { | protected final function launchDaemon( | ||||
| $class, | |||||
| array $argv, | |||||
| $debug, | |||||
| $run_as_current_user = false) { | |||||
| $daemon = $this->findDaemonClass($class); | $daemon = $this->findDaemonClass($class); | ||||
| $console = PhutilConsole::getConsole(); | $console = PhutilConsole::getConsole(); | ||||
| if (!$run_as_current_user) { | |||||
| // Check if the script is started as the correct user | |||||
| $phd_user = PhabricatorEnv::getEnvConfig('phd.user'); | |||||
| $current_user = posix_getpwuid(posix_geteuid()); | |||||
| $current_user = $current_user['name']; | |||||
| if ($phd_user && $phd_user != $current_user) { | |||||
| if ($debug) { | |||||
| throw new PhutilArgumentUsageException(pht( | |||||
| 'You are trying to run a daemon as a nonstandard user, '. | |||||
| 'and `phd` was not able to `sudo` to the correct user. '."\n". | |||||
| 'Phabricator is configured to run daemons as "%s", '. | |||||
| 'but the current user is "%s". '."\n". | |||||
| 'Use `sudo` to run as a different user, pass `--as-current-user` '. | |||||
| 'to ignore this warning, or edit `phd.user` '. | |||||
| 'to change the configuration.', $phd_user, $current_user)); | |||||
| } else { | |||||
| $this->runDaemonsAsUser = $phd_user; | |||||
| $console->writeOut(pht('Starting daemons as %s', $phd_user)."\n"); | |||||
| } | |||||
| } | |||||
| } | |||||
| if ($debug) { | if ($debug) { | ||||
| if ($argv) { | if ($argv) { | ||||
| $console->writeOut( | $console->writeOut( | ||||
| pht( | pht( | ||||
| "Launching daemon \"%s\" in debug mode (not daemonized) ". | "Launching daemon \"%s\" in debug mode (not daemonized) ". | ||||
| "with arguments %s.\n", | "with arguments %s.\n", | ||||
| $daemon, | $daemon, | ||||
| csprintf('%LR', $argv))); | csprintf('%LR', $argv))); | ||||
| ▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Lines | protected final function launchDaemon( | ||||
| $command = csprintf( | $command = csprintf( | ||||
| './phd-daemon %s %C %C', | './phd-daemon %s %C %C', | ||||
| $daemon, | $daemon, | ||||
| implode(' ', $flags), | implode(' ', $flags), | ||||
| implode(' ', $argv)); | implode(' ', $argv)); | ||||
| $phabricator_root = dirname(phutil_get_library_root('phabricator')); | $phabricator_root = dirname(phutil_get_library_root('phabricator')); | ||||
| $daemon_script_dir = $phabricator_root.'/scripts/daemon/'; | $daemon_script_dir = $phabricator_root.'/scripts/daemon/'; | ||||
epriestley: Use `%s` instead of `"%C"` -- `%s` will automatically apply proper escaping, but `"%C"` will… | |||||
| if ($debug) { | if ($debug) { | ||||
| // Don't terminate when the user sends ^C; it will be sent to the | // Don't terminate when the user sends ^C; it will be sent to the | ||||
| // subprocess which will terminate normally. | // subprocess which will terminate normally. | ||||
| pcntl_signal( | pcntl_signal( | ||||
| SIGINT, | SIGINT, | ||||
| array(__CLASS__, 'ignoreSignal')); | array(__CLASS__, 'ignoreSignal')); | ||||
| echo "\n phabricator/scripts/daemon/ \$ {$command}\n\n"; | echo "\n phabricator/scripts/daemon/ \$ {$command}\n\n"; | ||||
| phutil_passthru('(cd %s && exec %C)', $daemon_script_dir, $command); | phutil_passthru('(cd %s && exec %C)', $daemon_script_dir, $command); | ||||
| } else { | } else { | ||||
| try { | |||||
| $this->executeDaemonLaunchCommand( | |||||
| $command, | |||||
| $daemon_script_dir, | |||||
| $this->runDaemonsAsUser); | |||||
| } catch (CommandException $e) { | |||||
| // Retry without sudo | |||||
| $console->writeOut(pht( | |||||
| "sudo command failed. Starting daemon as current user\n")); | |||||
| $this->executeDaemonLaunchCommand( | |||||
| $command, | |||||
| $daemon_script_dir); | |||||
| } | |||||
| } | |||||
| } | |||||
| private function executeDaemonLaunchCommand( | |||||
| $command, | |||||
| $daemon_script_dir, | |||||
| $run_as_user = null) { | |||||
| if ($run_as_user) { | |||||
| // If anything else besides sudo should be | |||||
| // supported then insert it here (runuser, su, ...) | |||||
| $command = csprintf( | |||||
| 'sudo -En -u %s -- %C', | |||||
| $run_as_user, | |||||
| $command); | |||||
| } | |||||
| $future = new ExecFuture('exec %C', $command); | $future = new ExecFuture('exec %C', $command); | ||||
Not Done Inline ActionsI'd expect cd %s && ... to be unnecessary given that setCWD() is called below. Is this just a copy/paste thing or is something more subtle going on? (phutil_passthru() did not support setCWD() when this code was written, which is why cd %s && ... is used above, although it's technically possible to rewrite the phutil_passthru() with PhutilExcePassthru now, which does support setCWD().) epriestley: I'd expect `cd %s && ...` to be unnecessary given that `setCWD()` is called below. Is this just… | |||||
| // Play games to keep 'ps' looking reasonable. | // Play games to keep 'ps' looking reasonable. | ||||
| $future->setCWD($daemon_script_dir); | $future->setCWD($daemon_script_dir); | ||||
| $future->resolvex(); | $future->resolvex(); | ||||
| } | } | ||||
| } | |||||
| public static function ignoreSignal($signo) { | public static function ignoreSignal($signo) { | ||||
| return; | return; | ||||
| } | } | ||||
| public static function requireExtensions() { | public static function requireExtensions() { | ||||
| self::mustHaveExtension('pcntl'); | self::mustHaveExtension('pcntl'); | ||||
| self::mustHaveExtension('posix'); | self::mustHaveExtension('posix'); | ||||
| ▲ Show 20 Lines • Show All 70 Lines • ▼ Show 20 Lines | protected final function executeStartCommand($keep_leases = false) { | ||||
| for ($ii = 0; $ii < $taskmasters; $ii++) { | for ($ii = 0; $ii < $taskmasters; $ii++) { | ||||
| $daemons[] = array('PhabricatorTaskmasterDaemon', array()); | $daemons[] = array('PhabricatorTaskmasterDaemon', array()); | ||||
| } | } | ||||
| $this->willLaunchDaemons(); | $this->willLaunchDaemons(); | ||||
| foreach ($daemons as $spec) { | foreach ($daemons as $spec) { | ||||
| list($name, $argv) = $spec; | list($name, $argv) = $spec; | ||||
| $this->launchDaemon($name, $argv, $is_debug = false); | $this->launchDaemon($name, $argv, $is_debug = false); | ||||
| } | } | ||||
| $console->writeErr(pht('Done.')."\n"); | $console->writeErr(pht('Done.')."\n"); | ||||
Not Done Inline ActionsPrefer to throw new PhutilArgumentUsageException(...) to indicate that a user has misused or misconfigured a command. This will give the program more flexibility in shutting down than exit(1). epriestley: Prefer to `throw new PhutilArgumentUsageException(...)` to indicate that a user has misused or… | |||||
| return 0; | return 0; | ||||
| } | } | ||||
| protected final function executeStopCommand( | protected final function executeStopCommand( | ||||
| array $pids, | array $pids, | ||||
| $grace_period, | $grace_period, | ||||
| $force) { | $force) { | ||||
| ▲ Show 20 Lines • Show All 186 Lines • Show Last 20 Lines | |||||
Use %s instead of "%C" -- %s will automatically apply proper escaping, but "%C" will produce the wrong result if $command already contains a quote (or various other special characters).
You should also use %s for the "run as user", since it's possible that phd.user might be set to `rm -rf /` or similar (with backticks, so it executes a command if used unquoted).
This would be a bit cleaner as sudo -En -u %s -- %s, which makes sure the environment is preserved and allows administrators to configure this to work through sudoers instead of needing to start the daemons as root (it looks like su also has -m to preserve the environment, but it's less configurable).