launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-977752 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #975450 in MAAS: "bind all services not required by the nodes to the loopback interface or add ingress firewall rules for these services"
  https://bugs.launchpad.net/maas/+bug/975450

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-977752/+merge/101346

With thanks to Julian for showing me what I couldn't find documentation for (“pre-imp”).  This makes the provisioning server listen only on the loopback interface.  I can't easily do the same for txlongpoll, because its twistd setup is hidden away in upstream code.

After mulling it over with Gavin, I refactored the service setup code a bit so that I could unit-test the resulting smaller methods.  But I didn't manage to inject a fake into TCPServer, which apparently is a bit weird and generated on the fly.  And so I ended up deleting the test.  Better ideas welcome.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-977752/+merge/101346
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-977752 into lp:maas.

=== modified file 'etc/pserv.yaml'
--- etc/pserv.yaml	2012-04-02 07:06:37 +0000
+++ etc/pserv.yaml	2012-04-10 10:19:21 +0000
@@ -6,6 +6,12 @@
 #
 # port: 5241
 
+## Network interface to bind the service on.
+# Keep this pointed at the loopback interface unless you really know what
+# you're doing.
+#
+# interface: "127.0.0.1"
+
 ## Where to log. This log can be rotated by sending SIGUSR1 to the
 ## running server.
 #

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2012-03-15 22:43:55 +0000
+++ src/provisioningserver/plugin.py	2012-04-10 10:19:21 +0000
@@ -89,6 +89,7 @@
 
     if_key_missing = None
 
+    interface = String(if_empty=b"", if_missing=b"127.0.0.1")
     port = Int(min=1, max=65535, if_missing=5241)
     logfile = String(if_empty=b"pserv.log", if_missing=b"pserv.log")
     oops = ConfigOops
@@ -126,56 +127,74 @@
         self.tapname = name
         self.description = description
 
-    def makeService(self, options):
-        """Construct a service."""
-        services = MultiService()
-
-        config_file = options["config-file"]
-        config = Config.load(config_file)
-
-        log_service = LogService(config["logfile"])
-        log_service.setServiceParent(services)
-
-        oops_config = config["oops"]
+    def _makeLogService(self, config):
+        """Create the log service."""
+        return LogService(config["logfile"])
+
+    def _makeOopsService(self, log_service, oops_config):
+        """Create the oops service."""
         oops_dir = oops_config["directory"]
         oops_reporter = oops_config["reporter"]
-        oops_service = OOPSService(log_service, oops_dir, oops_reporter)
-        oops_service.setServiceParent(services)
-
-        broker_config = config["broker"]
+        return OOPSService(log_service, oops_dir, oops_reporter)
+
+    def _makePAPI(self, cobbler_config):
+        """Create the provisioning XMLRPC API."""
+        cobbler_session = CobblerSession(
+            cobbler_config["url"], cobbler_config["username"],
+            cobbler_config["password"])
+        return ProvisioningAPI_XMLRPC(cobbler_session)
+
+    def _makeSiteService(self, papi_xmlrpc, config):
+        """Create the site service."""
+        site_root = Resource()
+        site_root.putChild("api", papi_xmlrpc)
+        site = Site(site_root)
+        site_port = config["port"]
+        site_interface = config["interface"]
+        site_service = TCPServer(site_port, site, interface=site_interface)
+        site_service.setName("site")
+        return site_service
+
+    def _makeBroker(self, broker_config):
+        """Create the messaging broker."""
         broker_port = broker_config["port"]
         broker_host = broker_config["host"]
         broker_username = broker_config["username"]
         broker_password = broker_config["password"]
         broker_vhost = broker_config["vhost"]
 
+        cb_connected = lambda ignored: None  # TODO
+        cb_disconnected = lambda ignored: None  # TODO
+        cb_failed = lambda connector_and_reason: (
+            log.err(connector_and_reason[1], "Connection failed"))
+        client_factory = AMQFactory(
+            broker_username, broker_password, broker_vhost,
+            cb_connected, cb_disconnected, cb_failed)
+        client_service = TCPClient(
+            broker_host, broker_port, client_factory)
+        client_service.setName("amqp")
+        return client_service
+
+    def makeService(self, options):
+        """Construct a service."""
+        services = MultiService()
+        config = Config.load(options["config-file"])
+
+        log_service = self._makeLogService(config)
+        log_service.setServiceParent(services)
+
+        oops_service = self._makeOopsService(log_service, config["oops"])
+        oops_service.setServiceParent(services)
+
+        broker_config = config["broker"]
         # Connecting to RabbitMQ is not yet a required component of a running
         # MAAS installation; skip unless the password has been set explicitly.
-        if broker_password is not b"test":
-            cb_connected = lambda ignored: None  # TODO
-            cb_disconnected = lambda ignored: None  # TODO
-            cb_failed = lambda connector_and_reason: (
-                log.err(connector_and_reason[1], "Connection failed"))
-            client_factory = AMQFactory(
-                broker_username, broker_password, broker_vhost,
-                cb_connected, cb_disconnected, cb_failed)
-            client_service = TCPClient(
-                broker_host, broker_port, client_factory)
-            client_service.setName("amqp")
+        if broker_config["password"] is not b"test":
+            client_service = self._makeBroker(broker_config)
             client_service.setServiceParent(services)
 
-        cobbler_config = config["cobbler"]
-        cobbler_session = CobblerSession(
-            cobbler_config["url"], cobbler_config["username"],
-            cobbler_config["password"])
-        papi_xmlrpc = ProvisioningAPI_XMLRPC(cobbler_session)
-
-        site_root = Resource()
-        site_root.putChild("api", papi_xmlrpc)
-        site = Site(site_root)
-        site_port = config["port"]
-        site_service = TCPServer(site_port, site)
-        site_service.setName("site")
+        papi_xmlrpc = self._makePAPI(config["cobbler"])
+        site_service = self._makeSiteService(papi_xmlrpc, config)
         site_service.setServiceParent(services)
 
         return services

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2012-03-21 12:18:48 +0000
+++ src/provisioningserver/tests/test_plugin.py	2012-04-10 10:19:21 +0000
@@ -55,6 +55,7 @@
                 'reporter': '',
                 },
             'port': 5241,
+            'interface': '127.0.0.1',
             }
         observed = Config.to_python({})
         self.assertEqual(expected, observed)