Merge lp:~jtv/maas/bug-977752 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Don't just expose pserv on the network.

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

[Gavin Panella (allenap)]

approve
On Apr 10, 2012 12:18 PM, "Jeroen T. Vermeulen" <email address hidden> wrote:

> 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
> You are subscribed to branch 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, ...

[Gavin Panella (allenap)]

review: Approve

=== 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)