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

[Jeroen T. Vermeulen]

When running a dev environment, move hard-coded "absolute" paths into the run/ directory.

This branch introduces a new way to compose such paths, which supports injection of a faux root directory through the MAAS_ROOT environment variable. It unbreaks startup of the dev server. To avoid import side effects, the setting is now wrapped in a getter function. We would like to do the migration of more of the hard-coded paths from celery configs in this way.

Pre-implementation call with Julian.

Also, the branch makes the upgrade/startup code skip the chmod on the GNUPGHOME if not running as root. This is not great; it might be better to try it and ignore errors if errno indicates that it's a matter of privileges. At any rate I did not want to introduce yet another way of indicating "this is just a development system."

Jeroen

[Raphaël Badin (rvb)]

Looks good. It's a bit annoying that the mechanism for overriding variables in a dev environment in pserv is so different from what we use in maasserver but I guess we will live. See my suggestions inline.

review: Approve

=== modified file 'Makefile'
--- Makefile	2014-08-21 05:12:29 +0000
+++ Makefile	2014-09-02 08:26:21 +0000
@@ -265,17 +265,17 @@
 
 # The `run` targets do not fit into the mould of the others.
 run-region:
-	@services/run $(service_names_region)
+	@env MAAS_ROOT=./run services/run $(service_names_region)
 run-cluster:
-	@services/run $(service_names_cluster)
+	@env MAAS_ROOT=./run services/run $(service_names_cluster)
 run:
-	@services/run $(service_names_all)
+	@env MAAS_ROOT=./run services/run $(service_names_all)
 
 phony_services_targets += run-region run-cluster run
 
 # This one's for the rapper, yo.
 run+webapp:
-	@services/run $(service_names_region) +webapp
+	@env MAAS_ROOT=./run services/run $(service_names_region) +webapp
 
 phony_services_targets += run+webapp
 

=== modified file 'src/maasserver/bootresources.py'
--- src/maasserver/bootresources.py	2014-09-02 07:29:01 +0000
+++ src/maasserver/bootresources.py	2014-09-02 08:26:21 +0000
@@ -49,7 +49,7 @@
 from maasserver.utils import absolute_reverse
 from maasserver.utils.async import transactional
 from maasserver.utils.orm import get_one
-from provisioningserver.auth import MAAS_USER_GPGHOME
+from provisioningserver.auth import get_maas_user_gpghome
 from provisioningserver.import_images.download_descriptions import (
     download_all_image_descriptions,
     )
@@ -878,7 +878,7 @@
 
     try:
         variables = {
-            'GNUPGHOME': MAAS_USER_GPGHOME,
+            'GNUPGHOME': get_maas_user_gpghome(),
             }
         with environment_variables(variables):
             maaslog.info("Started importing of boot resources.")

=== modified file 'src/maasserver/tests/test_bootresources.py'
--- src/maasserver/tests/test_bootresources.py	2014-09-01 10:11:15 +0000
+++ src/maasserver/tests/test_bootresources.py	2014-09-02 08:26:21 +0000
@@ -1095,7 +1095,7 @@
 
         bootresources._import_resources(force=True)
         self.assertEqual(
-            bootresources.MAAS_USER_GPGHOME,
+            bootresources.get_maas_user_gpghome(),
             fake_download.env['GNUPGHOME'])
 
     def test__import_resources_calls_import_boot_images_on_clusters(self):

=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py	2014-04-25 18:03:38 +0000
+++ src/provisioningserver/auth.py	2014-09-02 08:26:21 +0000
@@ -13,22 +13,29 @@
 
 __metaclass__ = type
 __all__ = [
+    'get_maas_user_gpghome',
     'get_recorded_api_credentials',
     'get_recorded_nodegroup_uuid',
-    'MAAS_USER_GPGHOME',
     'record_api_credentials',
     'record_nodegroup_uuid',
     ]
 
 from apiclient.creds import convert_string_to_tuple
 from provisioningserver import cache
+from provisioningserver.path import get_path
+
+
+def get_maas_user_gpghome():
+    """Return the GPG directory for the `maas` user.
+
+    Set $GPGHOME to this value ad-hoc when needed.
+    """
+    return get_path('/var/lib/maas/gnupg')
+
 
 # Cache key for the API credentials as last sent by the server.
 API_CREDENTIALS_CACHE_KEY = 'api_credentials'
 
-# GPG directory for the "maas" user.  Set $GPGHOME to this value ad-hoc when
-# needed.
-MAAS_USER_GPGHOME = '/var/lib/maas/gnupg'
 
 # Cache key for the uuid of the nodegroup that this worker manages.
 NODEGROUP_UUID_CACHE_KEY = 'nodegroup_uuid'

=== added file 'src/provisioningserver/path.py'
--- src/provisioningserver/path.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/path.py	2014-09-02 08:26:21 +0000
@@ -0,0 +1,45 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Compute paths relative to root."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'get_path',
+    ]
+
+from os import getenv
+import os.path
+
+
+def get_path(*path_elements):
+    """Return an absolute path based on the `MAAS_ROOT` environment variable.
+
+    Use this to compute paths like `/var/lib/maas/gnupg`, so that development
+    environments can redirect them to a playground location.  For example, if
+    `MAAS_ROOT` is set to `/tmp/maasroot`, then `get_path()` will return
+    `/tmp/maasroot` and `get_path('/var/lib/maas')` returns
+    `/tmp/maasroot/var/lib/maas`.  If `MAAS_ROOT` is not set, you just get (a
+    normalised version of) the location you passed in; just `get_path()` will
+    always return the root directory.
+
+    This call may have minor side effects: it reads environment variables and
+    the current working directory.  Side effects during imports are bad, so
+    avoid using this in global variables.  Instead of exporting a variable
+    that holds your path, export a getter function that returns your path.
+    Add caching if it becomes a performance problem.
+    """
+    maas_root = getenv('MAAS_ROOT', '/')
+    # Strip off a leading slash, if any.  If left in, it would override any
+    # preceding path elements.  The MAAS_ROOT would be ignored.
+    # The dot is there to make the call work even with zero path elements.
+    path = os.path.join('.', *path_elements).lstrip('/')
+    return os.path.abspath(os.path.join(maas_root, path))

=== modified file 'src/provisioningserver/rpc/boot_images.py'
--- src/provisioningserver/rpc/boot_images.py	2014-08-27 03:38:28 +0000
+++ src/provisioningserver/rpc/boot_images.py	2014-09-02 08:26:21 +0000
@@ -18,7 +18,7 @@
     ]
 
 
-from provisioningserver.auth import MAAS_USER_GPGHOME
+from provisioningserver.auth import get_maas_user_gpghome
 from provisioningserver.boot import tftppath
 from provisioningserver.config import Config
 from provisioningserver.import_images import boot_resources
@@ -41,7 +41,7 @@
     This is function is synchronous so it must be called with deferToThread.
     """
     variables = {
-        'GNUPGHOME': MAAS_USER_GPGHOME,
+        'GNUPGHOME': get_maas_user_gpghome(),
         }
     with environment_variables(variables):
         boot_resources.import_images(sources)

=== modified file 'src/provisioningserver/rpc/tests/test_boot_images.py'
--- src/provisioningserver/rpc/tests/test_boot_images.py	2014-09-01 08:40:13 +0000
+++ src/provisioningserver/rpc/tests/test_boot_images.py	2014-09-02 08:26:21 +0000
@@ -87,7 +87,7 @@
 
     def test__run_import_sets_GPGHOME(self):
         home = factory.make_name('home')
-        self.patch(boot_images, 'MAAS_USER_GPGHOME', home)
+        self.patch(boot_images, 'get_maas_user_gpghome').return_value = home
         fake = self.patch_boot_resources_function()
         _run_import(sources=[])
         self.assertEqual(home, fake.env['GNUPGHOME'])

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-08-28 10:53:00 +0000
+++ src/provisioningserver/tasks.py	2014-09-02 08:26:21 +0000
@@ -39,7 +39,7 @@
     tags,
     )
 from provisioningserver.auth import (
-    MAAS_USER_GPGHOME,
+    get_maas_user_gpghome,
     record_api_credentials,
     record_nodegroup_uuid,
     )
@@ -468,7 +468,7 @@
         if data is not None:
             source["keyring_data"] = b64decode(data)
     variables = {
-        'GNUPGHOME': MAAS_USER_GPGHOME,
+        'GNUPGHOME': get_maas_user_gpghome(),
         }
     if http_proxy is not None:
         variables['http_proxy'] = http_proxy

=== added file 'src/provisioningserver/tests/test_path.py'
--- src/provisioningserver/tests/test_path.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_path.py	2014-09-02 08:26:21 +0000
@@ -0,0 +1,66 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for filesystem paths."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from os import getcwdu
+import os.path
+
+from fixtures import EnvironmentVariableFixture
+from maastesting.factory import factory
+from maastesting.testcase import MAASTestCase
+from provisioningserver.path import get_path
+from testtools.matchers import StartsWith
+
+
+class TestGetPath(MAASTestCase):
+
+    def set_root(self, root_path=None):
+        """For the duration of this test, set the `MAAS_ROOT` variable`."""
+        self.useFixture(EnvironmentVariableFixture('MAAS_ROOT', root_path))
+
+    def test__defaults_to_root(self):
+        self.set_root()
+        self.assertEqual('/', get_path())
+
+    def test__appends_path_elements(self):
+        self.set_root()
+        part1 = factory.make_name('dir')
+        part2 = factory.make_name('file')
+        self.assertEqual(
+            os.path.join('/', part1, part2),
+            get_path(part1, part2))
+
+    def test__obeys_MAAS_ROOT_variable(self):
+        root = factory.make_name('/root')
+        self.set_root(root)
+        path = factory.make_name('path')
+        self.assertEqual(os.path.join(root, path), get_path(path))
+
+    def test__returns_absolute_path(self):
+        self.set_root('.')
+        self.assertThat(get_path(), StartsWith('/'))
+        self.assertEqual(getcwdu(), get_path())
+
+    def test__concatenates_despite_leading_slash(self):
+        root = self.make_dir()
+        self.set_root(root)
+        filename = factory.make_name('file')
+        self.assertEqual(
+            os.path.join(root, filename),
+            get_path('/' + filename))
+
+    def test__normalises(self):
+        self.set_root()
+        self.assertEqual('/foo/bar', get_path('foo///szut//..///bar//'))

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-08-28 05:47:12 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-09-02 08:26:21 +0000
@@ -598,8 +598,8 @@
         self.assertIsInstance(import_boot_images, Task)
 
     def test_import_boot_images_sets_GPGHOME(self):
-        home = factory.make_name('home')
-        self.patch(tasks, 'MAAS_USER_GPGHOME', home)
+        home = self.make_dir()
+        self.patch(tasks, 'get_maas_user_gpghome').return_value = home
         fake = self.patch_boot_resources_function()
         import_boot_images(sources=[])
         self.assertEqual(home, fake.env['GNUPGHOME'])

=== modified file 'src/provisioningserver/tests/test_upgrade_cluster.py'
--- src/provisioningserver/tests/test_upgrade_cluster.py	2014-08-13 21:49:35 +0000
+++ src/provisioningserver/tests/test_upgrade_cluster.py	2014-09-02 08:26:21 +0000
@@ -16,7 +16,7 @@
 
 from argparse import ArgumentParser
 from itertools import product
-from os import listdir
+import os
 import os.path
 
 from maastesting.factory import factory
@@ -115,7 +115,8 @@
         return os.path.join(parent_dir, factory.make_name('gpghome'))
 
     def patch_gnupg_home(self, gpghome):
-        self.patch(upgrade_cluster, 'MAAS_USER_GPGHOME', gpghome)
+        self.patch(upgrade_cluster, 'get_maas_user_gpghome').return_value = (
+            gpghome)
 
     def patch_call(self):
         return self.patch(upgrade_cluster, 'check_call')
@@ -125,7 +126,7 @@
         self.patch_gnupg_home(existing_home)
         self.patch_call()
         upgrade_cluster.create_gnupg_home()
-        self.assertEqual([], listdir(existing_home))
+        self.assertEqual([], os.listdir(existing_home))
 
     def test__creates_directory(self):
         parent = self.make_dir()
@@ -135,15 +136,25 @@
         upgrade_cluster.create_gnupg_home()
         self.assertThat(new_home, DirExists())
 
-    def test__sets_ownership_to_maas(self):
+    def test__sets_ownership_to_maas_if_running_as_root(self):
         parent = self.make_dir()
         new_home = self.make_nonexistent_path(parent)
         self.patch_gnupg_home(new_home)
         call = self.patch_call()
+        self.patch(os, 'geteuid').return_value = 0
         upgrade_cluster.create_gnupg_home()
         self.assertThat(
             call, MockCalledOnceWith(['chown', 'maas:maas', new_home]))
 
+    def test__does_not_set_ownership_if_not_running_as_root(self):
+        parent = self.make_dir()
+        new_home = self.make_nonexistent_path(parent)
+        self.patch_gnupg_home(new_home)
+        call = self.patch_call()
+        self.patch(os, 'geteuid').return_value = 101
+        upgrade_cluster.create_gnupg_home()
+        self.assertThat(call, MockNotCalled())
+
 
 class TestRetireBootResourcesYAML(MAASTestCase):
     """Tests for `retire_bootresources_yaml`."""

=== modified file 'src/provisioningserver/upgrade_cluster.py'
--- src/provisioningserver/upgrade_cluster.py	2014-08-13 21:49:35 +0000
+++ src/provisioningserver/upgrade_cluster.py	2014-09-02 08:26:21 +0000
@@ -39,7 +39,7 @@
 from textwrap import dedent
 
 from provisioningserver import config
-from provisioningserver.auth import MAAS_USER_GPGHOME
+from provisioningserver.auth import get_maas_user_gpghome
 from provisioningserver.boot.tftppath import (
     drill_down,
     list_subdirs,
@@ -63,9 +63,14 @@
 
 def create_gnupg_home():
     """Upgrade hook: create maas user's GNUPG home directory."""
-    if not os.path.isdir(MAAS_USER_GPGHOME):
-        makedirs(MAAS_USER_GPGHOME)
-        check_call(['chown', 'maas:maas', MAAS_USER_GPGHOME])
+    gpghome = get_maas_user_gpghome()
+    if not os.path.isdir(gpghome):
+        makedirs(gpghome)
+        if os.geteuid() == 0:
+            # Make the maas user the owner of its GPG home.  Do this only if
+            # running as root; otherwise it would probably fail.  We want to
+            # be able to start a development instance without triggering that.
+            check_call(['chown', 'maas:maas', gpghome])
 
 
 # Path to obsolete boot-resources configuration.