Move server-related Aphlict options to a configuration file

Summary: Ref T10697. This isn't everything but starts generalizing options and moving us toward a cluster-ready state of affairs.

Test Plan: Started server in various configurations, hit most (all?) of the error cases with bad configs, sent test notifications.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10697

Differential Revision: https://secure.phabricator.com/D15701

.gitignore

@@ -16,6 +16,7 @@
 /conf/keys/device.pub
 /conf/keys/device.key
 /conf/keys/device.id
+/conf/aphlict/aphlict.custom.json
 
 # Impact Font
 /resources/font/impact.ttf

conf/aphlict/README

@@ -0,0 +1,16 @@
+To customize this configuration, you have two options: create a custom
+configuration file in this directory, or specify a path to a configuration file
+explicitly when starting Aphlict.
+
+To create a custom configuration file, copy `aphlict.default.json` in this
+directory and rename it `aphlict.custom.json`. If this file exists, it will
+be read by default.
+
+To specify a path when starting Aphlict, use the `--config` flag:
+
+  phabricator/ $ ./bin/aphlict start --config path/to/config.json
+
+Specifying a configuration file explicitly overrides default configuration.
+
+For more information about configuring notifications, see the article
+"Notifications User Guide: Setup and Configuration" in the documentation.

conf/aphlict/aphlict.default.json

@@ -0,0 +1,18 @@
+{
+  "servers": [
+    {
+      "type": "client",
+      "port": 22280,
+      "listen": "0.0.0.0",
+      "ssl.key": null,
+      "ssl.cert": null
+    },
+    {
+      "type": "admin",
+      "port": 22281,
+      "listen": "127.0.0.1",
+      "ssl.key": null,
+      "ssl.cert": null
+    }
+  ]
+}

src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php

@@ -4,8 +4,7 @@ abstract class PhabricatorAphlictManagementWorkflow
   extends PhabricatorManagementWorkflow {
 
   private $debug = false;
-  private $clientHost;
-  private $clientPort;
+  private $configPath;
 
   final protected function setDebug($debug) {
     $this->debug = $debug;
@@ -15,21 +14,167 @@ abstract class PhabricatorAphlictManagementWorkflow
   protected function getLaunchArguments() {
     return array(
       array(
-        'name'  => 'client-host',
-        'param' => 'hostname',
-        'help'  => pht('Hostname to bind to for the client server.'),
-      ),
-      array(
-        'name'  => 'client-port',
-        'param' => 'port',
-        'help'  => pht('Port to bind to for the client server.'),
+        'name' => 'config',
+        'param' => 'file',
+        'help' => pht(
+          'Use a specific configuration file instead of the default '.
+          'configuration.'),
       ),
     );
   }
 
   protected function parseLaunchArguments(PhutilArgumentParser $args) {
-    $this->clientHost = $args->getArg('client-host');
-    $this->clientPort = $args->getArg('client-port');
+    $config_file = $args->getArg('config');
+    if ($config_file) {
+      $full_path = Filesystem::resolvePath($config_file);
+      $show_path = $full_path;
+    } else {
+      $root = dirname(dirname(phutil_get_library_root('phabricator')));
+
+      $try = array(
+        'phabricator/conf/aphlict/aphlict.custom.json',
+        'phabricator/conf/aphlict/aphlict.default.json',
+      );
+
+      foreach ($try as $config) {
+        $full_path = $root.'/'.$config;
+        $show_path = $config;
+        if (Filesystem::pathExists($full_path)) {
+          break;
+        }
+      }
+    }
+
+    echo tsprintf(
+      "%s\n",
+      pht(
+        'Reading configuration from: %s',
+        $show_path));
+
+    try {
+      $data = Filesystem::readFile($full_path);
+    } catch (Exception $ex) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Failed to read configuration file. %s',
+          $ex->getMessage()));
+    }
+
+    try {
+      $data = phutil_json_decode($data);
+    } catch (Exception $ex) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Configuration file is not properly formatted JSON. %s',
+          $ex->getMessage()));
+    }
+
+    try {
+      PhutilTypeSpec::checkMap(
+        $data,
+        array(
+          'servers' => 'list<wild>',
+        ));
+    } catch (Exception $ex) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Configuration file has improper configuration keys at top '.
+          'level. %s',
+          $ex->getMessage()));
+    }
+
+    $servers = $data['servers'];
+    $has_client = false;
+    $has_admin = false;
+    $port_map = array();
+    foreach ($servers as $index => $server) {
+      PhutilTypeSpec::checkMap(
+        $server,
+        array(
+          'type' => 'string',
+          'port' => 'int',
+          'listen' => 'optional string|null',
+          'ssl.key' => 'optional string|null',
+          'ssl.cert' => 'optional string|null',
+        ));
+
+      $port = $server['port'];
+      if (!isset($port_map[$port])) {
+        $port_map[$port] = $index;
+      } else {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Two servers (at indexes "%s" and "%s") both bind to the same '.
+            'port ("%s"). Each server must bind to a unique port.',
+            $port_map[$port],
+            $index,
+            $port));
+      }
+
+      $type = $server['type'];
+      switch ($type) {
+        case 'admin':
+          $has_admin = true;
+          break;
+        case 'client':
+          $has_client = true;
+          break;
+        default:
+          throw new PhutilArgumentUsageException(
+            pht(
+              'A specified server (at index "%s", on port "%s") has an '.
+              'invalid type ("%s"). Valid types are: admin, client.',
+              $index,
+              $port,
+              $type));
+      }
+
+      $ssl_key = idx($server, 'ssl.key');
+      $ssl_cert = idx($server, 'ssl.cert');
+      if (($ssl_key && !$ssl_cert) || ($ssl_cert && !$ssl_key)) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'A specified server (at index "%s", on port "%s") specifies '.
+            'only one of "%s" and "%s". Each server must specify neither '.
+            '(to disable SSL) or specify both (to enable it).',
+            $index,
+            $port,
+            'ssl.key',
+            'ssl.cert'));
+      }
+    }
+
+    if (!$servers) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Configuration file does not specify any servers. This service '.
+          'will not be able to interact with the outside world if it does '.
+          'not listen on any ports. You must specify at least one "%s" '.
+          'server and at least one "%s" server.',
+          'admin',
+          'client'));
+    }
+
+    if (!$has_client) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Configuration file does not specify any client servers. This '.
+          'service will be unable to transmit any notifications without a '.
+          'client server. You must specify at least one server with '.
+          'type "%s".',
+          'client'));
+    }
+
+    if (!$has_admin) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Configuration file does not specify any administrative '.
+          'servers. This service will be unable to receive messages. '.
+          'You must specify at least one server with type "%s".',
+          'admin'));
+    }
+
+    $this->configPath = $full_path;
   }
 
   final public function getPIDPath() {
@@ -148,38 +293,12 @@ abstract class PhabricatorAphlictManagementWorkflow
   }
 
   private function getServerArgv() {
-    $ssl_key = PhabricatorEnv::getEnvConfig('notification.ssl-key');
-    $ssl_cert = PhabricatorEnv::getEnvConfig('notification.ssl-cert');
-
-    $server_uri = PhabricatorEnv::getEnvConfig('notification.server-uri');
-    $server_uri = new PhutilURI($server_uri);
-
-    $client_uri = PhabricatorEnv::getEnvConfig('notification.client-uri');
-    $client_uri = new PhutilURI($client_uri);
-
     $log = $this->getLogPath();
 
     $server_argv = array();
-    $server_argv[] = '--client-port='.coalesce(
-      $this->clientPort,
-      $client_uri->getPort());
-    $server_argv[] = '--admin-port='.$server_uri->getPort();
-    $server_argv[] = '--admin-host='.$server_uri->getDomain();
-
-    if ($ssl_key) {
-      $server_argv[] = '--ssl-key='.$ssl_key;
-    }
-
-    if ($ssl_cert) {
-      $server_argv[] = '--ssl-cert='.$ssl_cert;
-    }
-
+    $server_argv[] = '--config='.$this->configPath;
     $server_argv[] = '--log='.$log;
 
-    if ($this->clientHost) {
-      $server_argv[] = '--client-host='.$this->clientHost;
-    }
-
     return $server_argv;
   }
 

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -182,6 +182,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       'Garbage collectors are now configured with "%s".',
       'bin/garbage set-policy');
 
+    $aphlict_reason = pht(
+      'Configuration of the notification server has changed substantially. '.
+      'For discussion, see T10794.');
+
     $ancient_config += array(
       'phid.external-loaders' =>
         pht(
@@ -298,6 +302,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       'phd.variant-config' => pht(
         'This configuration is no longer relevant because daemons '.
         'restart automatically on configuration changes.'),
+
+      'notification.ssl-cert' => $aphlict_reason,
+      'notification.ssl-key' => $aphlict_reason,
     );
 
     return $ancient_config;

src/applications/config/option/PhabricatorNotificationConfigOptions.php

@@ -46,14 +46,6 @@ final class PhabricatorNotificationConfigOptions
         ->setDescription(pht('Location of the notification receiver server.')),
       $this->newOption('notification.log', 'string', '/var/log/aphlict.log')
         ->setDescription(pht('Location of the server log file.')),
-      $this->newOption('notification.ssl-key', 'string', null)
-        ->setLocked(true)
-        ->setDescription(
-          pht('Path to SSL key to use for secure WebSockets.')),
-      $this->newOption('notification.ssl-cert', 'string', null)
-        ->setLocked(true)
-        ->setDescription(
-          pht('Path to SSL certificate to use for secure WebSockets.')),
       $this->newOption(
         'notification.pidfile',
         'string',

src/docs/user/configuration/notifications.diviner

@@ -59,19 +59,44 @@ After installing Node.js, you can control the notification server with the
 
   phabricator/ $ bin/aphlict start
 
-The server must be able to listen on port **22280** for Aphlict to work. In
-particular, if you're running in EC2, you need to unblock this port in the
-server's security group configuration. You can change this port in the
-`notification.client-uri` config.
+By default, the server must be able to listen on port `22280`. If you're using
+a host firewall (like a security group in EC2), make sure traffic can reach the
+server.
 
-You may need to adjust these settings:
+The server configuration is controlled by a configuration file, which is
+separate from Phabricator's configuration settings. The default file can
+be found at `phabricator/conf/aphlict/aphlict.default.json`.
 
-  - `notification.ssl-cert` Point this at an SSL certificate for secure
-    WebSockets.
-  - `notification.ssl-key` Point this at an SSL keyfile for secure WebSockets.
+To make adjustments to the default configuration, either copy this file to
+create `aphlict.custom.json` in the same directory (this file will be used if
+it exists) or specify a configuration file explicitly with the `--config` flag:
 
-In particular, if your server uses HTTPS, you **must** configure these options.
-Browsers will not allow you to use non-SSL websockets from an SSL web page.
+  phabricator/ $ bin/aphlict start --config path/to/config.json
+
+The configuration file has these settings:
+
+  - `servers`: A list of servers to start.
+
+Each server in the `servers` list should be an object with these keys:
+
+  - `type`: //Required string.// The type of server to start. Options are
+    `admin` or `client`. Normally, you should run one of each.
+  - `port`: //Required int.// The port this server should listen on.
+  - `listen`: //Optional string.// Which interface to bind to. By default,
+    the `admin` server is bound to localhost (so only other services on the
+    local machine can connect to it), while the `client` server is bound
+    to `0.0.0.0` (so any client can connect.
+  - `ssl.key`: //Optional string.// If you want to use SSL on this port,
+    the path to an SSL key.
+  - `ssl.cert`: //Optional string.// If you want to use SSL on this port,
+    the path to an SSL certificate.
+
+The defaults are appropriate for simple cases, but you may need to adjust them
+if you are running a more complex configuration.
+
+
+Configuring Phabricator
+=======================
 
 You may also want to adjust these settings:
 

support/aphlict/server/aphlict_server.js

@@ -7,15 +7,10 @@ var util = require('util');
 var fs = require('fs');
 
 function parse_command_line_arguments(argv) {
-  var config = {
-    'client-port': 22280,
-    'admin-port': 22281,
-    'client-host': '0.0.0.0',
-    'admin-host': '127.0.0.1',
+  var args = {
     log: '/var/log/aphlict.log',
-    'ssl-key': null,
-    'ssl-cert': null,
-    test: false
+    test: false,
+    config: null
   };
 
   for (var ii = 2; ii < argv.length; ii++) {
@@ -24,16 +19,18 @@ function parse_command_line_arguments(argv) {
     if (!matches) {
       throw new Error('Unknown argument "' + arg + '"!');
     }
-    if (!(matches[1] in config)) {
+    if (!(matches[1] in args)) {
       throw new Error('Unknown argument "' + matches[1] + '"!');
     }
-    config[matches[1]] = matches[2];
+    args[matches[1]] = matches[2];
   }
 
-  config['client-port'] = parseInt(config['client-port'], 10);
-  config['admin-port'] = parseInt(config['admin-port'], 10);
+  return args;
+}
 
-  return config;
+function parse_config(args) {
+  var data = fs.readFileSync(args.config);
+  return JSON.parse(data);
 }
 
 require('./lib/AphlictLog');
@@ -41,7 +38,8 @@ require('./lib/AphlictLog');
 var debug = new JX.AphlictLog()
   .addConsole(console);
 
-var config = parse_command_line_arguments(process.argv);
+var args = parse_command_line_arguments(process.argv);
+var config = parse_config(args);
 
 function set_exit_code(code) {
   process.on('exit', function() {
@@ -51,7 +49,7 @@ function set_exit_code(code) {
 
 process.on('uncaughtException', function(err) {
   var context = null;
-  if (err.code == 'EACCES' && err.path == config.log) {
+  if (err.code == 'EACCES' && err.path == args.log) {
     context = util.format(
       'Unable to open logfile ("%s"). Check that permissions are set ' +
       'correctly.',
@@ -71,8 +69,8 @@ process.on('uncaughtException', function(err) {
 });
 
 // Add the logfile so we'll fail if we can't write to it.
-if (config.log) {
-  debug.addLog(config.log);
+if (args.log) {
+  debug.addLog(args.log);
 }
 
 try {
@@ -90,51 +88,37 @@ try {
 require('./lib/AphlictAdminServer');
 require('./lib/AphlictClientServer');
 
-var ssl_config = {
-  enabled: (config['ssl-key'] || config['ssl-cert'])
-};
+var ii;
+var servers = [];
+for (ii = 0; ii < config.servers.length; ii++) {
+  var spec = config.servers[ii];
 
-// Load the SSL certificates (if any were provided) now, so that runs with
-// `--test` will see any errors.
-if (ssl_config.enabled) {
-  ssl_config.key = fs.readFileSync(config['ssl-key']);
-  ssl_config.cert = fs.readFileSync(config['ssl-cert']);
-} else {
-  ssl_config.key = null;
-  ssl_config.cert = null;
+  spec.listen = spec.listen || '0.0.0.0';
+
+  if (spec['ssl.key']) {
+    spec['ssl.key'] = fs.readFileSync(spec['ssl.key']);
   }
 
-var servers = [];
+  if (spec['ssl.cert']){
+    spec['ssl.cert'] = fs.readFileSync(spec['ssl.cert']);
+  }
 
-servers.push({
-  type: 'client',
-  port: config['client-port'],
-  listen: config['client-host'],
-  'ssl.key': ssl_config.key,
-  'ssl.certificate': ssl_config.cert
-});
-
-servers.push({
-  type: 'admin',
-  port: config['admin-port'],
-  listen: config['admin-host'],
-  'ssl.key': null,
-  'ssl.cert': null
-});
+  servers.push(spec);
+}
 
 // If we're just doing a configuration test, exit here before starting any
 // servers.
-if (config.test) {
+if (args.test) {
   debug.log('Configuration test OK.');
   set_exit_code(0);
   return;
 }
 
+debug.log('Starting servers (service PID %d).', process.pid);
+
 var aphlict_servers = [];
 var aphlict_clients = [];
 var aphlict_admins = [];
-
-var ii;
 for (ii = 0; ii < servers.length; ii++) {
   var server = servers[ii];
   var is_client = (server.type == 'client');
@@ -161,6 +145,12 @@ for (ii = 0; ii < servers.length; ii++) {
   aphlict_server.setLogger(debug);
   aphlict_server.listen(server.port, server.listen);
 
+  debug.log(
+    'Started %s server (Port %d, %s).',
+    server.type,
+    server.port,
+    server['ssl.key'] ? 'With SSL' : 'No SSL');
+
   aphlict_servers.push(aphlict_server);
 
   if (is_client) {
@@ -174,5 +164,3 @@ for (ii = 0; ii < aphlict_admins.length; ii++) {
   var admin_server = aphlict_admins[ii];
   admin_server.setClientServers(aphlict_clients);
 }
-
-debug.log('Started Server (PID %d)', process.pid);