launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

approve
On Apr 10, 2012 12:18 PM, "Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx> 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, 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)
>
>
>

-- 
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.