Bug 1391476 - Automatically set cache/volume permissions in run-task; r?dustin draft
authorGregory Szorc <gps@mozilla.com>
Wed, 23 Aug 2017 12:07:18 -0700
changeset 651441 6a4982908949b685c12c53874ea43f5caf3d2552
parent 651410 478bf611305a514ffa1698f6f0fac44ef3b34e8c
child 651442 8cdcd2f31b3764ab5380cea1db59ec3285074ef5
push id75735
push usergszorc@mozilla.com
push dateWed, 23 Aug 2017 19:08:05 +0000
reviewersdustin
bugs1391476
milestone57.0a1
Bug 1391476 - Automatically set cache/volume permissions in run-task; r?dustin run-task's --chown and --chown-recursive are only used on volumes and caches - the only locations that aren't controlled by the Docker image itself and thus whose permissions could be "undefined." Previous commits have taught run-task about the locations of all caches and volumes. Therefore, we no longer need to manually define paths to chown. Instead, we can chown as a side-effect of the path being a cache or a volume. So, this commit changes run-task to chown caches and volumes automatically. Since we no longer have a use for --chown and --chown-recursive, those arguments are removed. There /could/ be some paths that are caches or volumes but aren't getting defined as such in Taskgraph. I consider this a bug in Taskgraph and the recourse is to properly define a path as a cache or a volume there. MozReview-Commit-ID: 1yqrhjil6gy
taskcluster/docker/image_builder/build-image.sh
taskcluster/docker/recipes/run-task
taskcluster/taskgraph/transforms/job/hazard.py
taskcluster/taskgraph/transforms/job/mozharness.py
taskcluster/taskgraph/transforms/job/mozharness_test.py
taskcluster/taskgraph/transforms/job/spidermonkey.py
taskcluster/taskgraph/transforms/job/toolchain.py
--- a/taskcluster/docker/image_builder/build-image.sh
+++ b/taskcluster/docker/image_builder/build-image.sh
@@ -21,17 +21,16 @@ test -n "$IMAGE_NAME" || raise_error "IM
 # Create artifact folder
 mkdir -p /home/worker/workspace/artifacts
 
 # Construct a CONTEXT_FILE
 CONTEXT_FILE=/home/worker/workspace/context.tar
 
 # Run ./mach taskcluster-build-image with --context-only to build context
 run-task \
-  --chown-recursive "/home/worker/workspace" \
   --vcs-checkout "/home/worker/checkouts/gecko" \
   -- \
   /home/worker/checkouts/gecko/mach taskcluster-build-image \
   --context-only "$CONTEXT_FILE" \
   "$IMAGE_NAME"
 test -f "$CONTEXT_FILE" || raise_error "Context file wasn't created"
 
 # Post context tar-ball to docker daemon
--- a/taskcluster/docker/recipes/run-task
+++ b/taskcluster/docker/recipes/run-task
@@ -212,23 +212,16 @@ def main(args):
         task_args = args[i + 1:]
     except ValueError:
         our_args = args
         task_args = []
 
     parser = argparse.ArgumentParser()
     parser.add_argument('--user', default='worker', help='user to run as')
     parser.add_argument('--group', default='worker', help='group to run as')
-    # We allow paths to be chowned by the --user:--group before permissions are
-    # dropped. This is often necessary for caches/volumes, since they default
-    # to root:root ownership.
-    parser.add_argument('--chown', action='append',
-                        help='Directory to chown to --user:--group')
-    parser.add_argument('--chown-recursive', action='append',
-                        help='Directory to recursively chown to --user:--group')
     parser.add_argument('--vcs-checkout',
                         help='Directory where Gecko checkout should be created')
     parser.add_argument('--comm-checkout',
                         help='Directory where Comm checkout should be created')
     parser.add_argument('--tools-checkout',
                         help='Directory where build/tools checkout should be created')
     parser.add_argument('--fetch-hgfingerprint', action='store_true',
                         help='Fetch the latest hgfingerprint from the secrets store, '
@@ -236,20 +229,16 @@ def main(args):
 
     args = parser.parse_args(our_args)
 
     # expand ~ in some paths
     if args.vcs_checkout:
         args.vcs_checkout = os.path.expanduser(args.vcs_checkout)
     if args.tools_checkout:
         args.tools_checkout = os.path.expanduser(args.tools_checkout)
-    if args.chown:
-        args.chown = [os.path.expanduser(p) for p in args.chown]
-    if args.chown_recursive:
-        args.chown_recursive = [os.path.expanduser(p) for p in args.chown_recursive]
     if 'HG_STORE_PATH' in os.environ:
         os.environ['HG_STORE_PATH'] = os.path.expanduser(os.environ['HG_STORE_PATH'])
 
     if running_as_root:
         try:
             user = pwd.getpwnam(args.user)
         except KeyError:
             print('could not find user %s; specify --user to a known user' %
@@ -315,16 +304,18 @@ def main(args):
                        b'%s\n' % (cache, b' '.join(sorted(our_requirements))))
 
             with open(requires_path, 'wb') as fh:
                 fh.write(b'\n'.join(sorted(our_requirements)))
 
             # And make it read-only as a precaution against deletion.
             os.chmod(requires_path, stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH)
 
+            set_dir_permissions(cache, uid, gid)
+
         # The cache has content and we have a requirements file. Validate
         # requirements alignment.
         elif os.path.exists(requires_path):
             with open(requires_path, 'rb') as fh:
                 wanted_requirements = set(fh.read().splitlines())
 
             print_line(b'cache', b'cache %s exists; requirements: %s\n' % (
                        cache, b' '.join(sorted(wanted_requirements))))
@@ -334,16 +325,18 @@ def main(args):
                 print('requirements for populated cache %s differ from '
                       'this task' % cache)
                 print('cache requirements: %s' % ' '.join(sorted(
                     wanted_requirements)))
                 print('our requirements:   %s' % ' '.join(sorted(
                     our_requirements)))
                 return 1
 
+            chown_recursive(cache, user.pw_name, group.gr_name, uid, gid)
+
         # The cache has content and no requirements file. This shouldn't
         # happen because run-task should be the first thing that touches a
         # cache.
         else:
             print('cache %s is not empty and is missing a .cacherequires '
                   'file; the cache names for this task are likely '
                   'mis-configured')
             return 1
@@ -379,35 +372,16 @@ def main(args):
         # The volume is almost certainly owned by root:root. Chown it so it
         # is writable.
 
         if running_as_root:
             print_line(b'volume', b'changing ownership of volume %s '
                                   b'to %d:%d\n' % (volume, uid, gid))
             set_dir_permissions(volume, uid, gid)
 
-    # Change ownership of requested paths.
-    # FUTURE: parse argument values for user/group if we don't want to
-    # use --user/--group.
-    for path in args.chown or []:
-        if not running_as_root:
-            print_line(b'set_dir_permissions', b'--chown not allowed when not running as root')
-            return 1
-
-        print_line(b'chown', b'changing ownership of %s to %s:%s\n' % (
-                   path, user.pw_name, group.gr_name))
-        set_dir_permissions(path, uid, gid)
-
-    for path in args.chown_recursive or []:
-        if not running_as_root:
-            print_line(b'set_dir_permissions', b'--chown-recursive not allowed when not running as root')
-            return 1
-
-        chown_recursive(path, user.pw_name, group.gr_name, uid, gid)
-
     def prepare_checkout_dir(checkout):
         if not checkout:
             return
 
         # Ensure the directory for the source checkout exists.
         try:
             os.makedirs(os.path.dirname(checkout))
         except OSError as e:
--- a/taskcluster/taskgraph/transforms/job/hazard.py
+++ b/taskcluster/taskgraph/transforms/job/hazard.py
@@ -61,14 +61,12 @@ def docker_worker_hazard(config, job, ta
         env['MOZCONFIG'] = run['mozconfig']
 
     # build-haz-linux.sh needs this otherwise it assumes the checkout is in
     # the workspace.
     env['GECKO_DIR'] = '/home/worker/checkouts/gecko'
 
     worker['command'] = [
         '/home/worker/bin/run-task',
-        '--chown-recursive', '/home/worker/tooltool-cache',
-        '--chown-recursive', '/home/worker/workspace',
         '--vcs-checkout', '/home/worker/checkouts/gecko',
         '--',
         '/bin/bash', '-c', run['command']
     ]
--- a/taskcluster/taskgraph/transforms/job/mozharness.py
+++ b/taskcluster/taskgraph/transforms/job/mozharness.py
@@ -164,19 +164,16 @@ def mozharness_on_docker_worker_setup(co
 
     # Retry if mozharness returns TBPL_RETRY
     worker['retry-exit-status'] = 4
 
     docker_worker_setup_secrets(config, job, taskdesc)
 
     command = [
         '/home/worker/bin/run-task',
-        # Various caches/volumes are default owned by root:root.
-        '--chown-recursive', '/home/worker/workspace',
-        '--chown-recursive', '/home/worker/tooltool-cache',
         '--vcs-checkout', '/home/worker/workspace/build/src',
         '--tools-checkout', '/home/worker/workspace/build/tools',
         '--',
         '/home/worker/workspace/build/src/{}'.format(
             run.get('job-script', 'taskcluster/scripts/builder/build-linux.sh')
         ),
     ]
 
--- a/taskcluster/taskgraph/transforms/job/mozharness_test.py
+++ b/taskcluster/taskgraph/transforms/job/mozharness_test.py
@@ -152,18 +152,16 @@ def mozharness_test_on_docker(config, jo
         docker_worker_add_tooltool(config, job, taskdesc, internal=True)
 
     if test['reboot']:
         raise Exception('reboot: {} not supported on generic-worker'.format(test['reboot']))
 
     # assemble the command line
     command = [
         '/home/worker/bin/run-task',
-        # The workspace cache/volume is default owned by root:root.
-        '--chown', '/home/worker/workspace',
     ]
 
     # Support vcs checkouts regardless of whether the task runs from
     # source or not in case it is needed on an interactive loaner.
     support_vcs_checkout(config, job, taskdesc)
 
     # If we have a source checkout, run mozharness from it instead of
     # downloading a zip file with the same content.
--- a/taskcluster/taskgraph/transforms/job/spidermonkey.py
+++ b/taskcluster/taskgraph/transforms/job/spidermonkey.py
@@ -60,18 +60,16 @@ def docker_worker_spidermonkey(config, j
     script = "build-sm.sh"
     if run['using'] == 'spidermonkey-package':
         script = "build-sm-package.sh"
     elif run['using'] == 'spidermonkey-mozjs-crate':
         script = "build-sm-mozjs-crate.sh"
 
     worker['command'] = [
         '/home/worker/bin/run-task',
-        '--chown-recursive', '/home/worker/workspace',
-        '--chown-recursive', '/home/worker/tooltool-cache',
         '--vcs-checkout', '/home/worker/workspace/build/src',
         '--',
         '/bin/bash',
         '-c',
         'cd /home/worker && workspace/build/src/taskcluster/scripts/builder/%s' % script
     ]
 
 
--- a/taskcluster/taskgraph/transforms/job/toolchain.py
+++ b/taskcluster/taskgraph/transforms/job/toolchain.py
@@ -113,19 +113,16 @@ def docker_worker_toolchain(config, job,
     })
 
     if run['tooltool-downloads']:
         internal = run['tooltool-downloads'] == 'internal'
         docker_worker_add_tooltool(config, job, taskdesc, internal=internal)
 
     worker['command'] = [
         '/home/worker/bin/run-task',
-        # Various caches/volumes are default owned by root:root.
-        '--chown-recursive', '/home/worker/workspace',
-        '--chown-recursive', '/home/worker/tooltool-cache',
         '--vcs-checkout=/home/worker/workspace/build/src',
         '--',
         'bash',
         '-c',
         'cd /home/worker && '
         './workspace/build/src/taskcluster/scripts/misc/{}'.format(
             run['script'])
     ]