in tree mh script not per platform add task line bouncer soft blocked by beet mover

talked to jlund today integrate av check action into beetmover add new action action - scan with clamd before uploading to s3 when push to releases bucket, copy to releases dir new functionality: automation to continuously run and scan files on s3

So in previous discussions, we thought it would be good to have this as a separate action but since each file is moved separately I just scanned them separately before they are uploaded in upload_bit. Not sure if this should be changed - I could copy them all to a tmp dir when they are uploaded and then scan afterwards but not sure that if would be more efficient given that most files are not scanned according to the exclusion list. I also used clamscan instead of clamdscan (daemon) because I couldn't get clamdscan to work in my docker container.

Comment on attachment 8716394 [details] [diff] [review] bug1210538.patch Review of attachment 8716394 [details] [diff] [review]: ----------------------------------------------------------------- so I think if we are to scan all beets at once, we should probably upload all the beets at the end after we scan. That way we don't upload potentially bad files and have to work backwards after. patch looks great but I think as is, it accommodates a 'cache' idea of scanning multiple files. imo - we should either: 1) have three actions: --download-bits --scan-bits --upload-bits 2) do what we are doing now but get rid of the 'rmtree', 'mkdir', 'copyfile', cache stuff and just 'download, scan, upload, and remove' locally in the same path as one action. what do you think? out of those two options, I am leaning towards (1). ::: testing/mozharness/scripts/release/beet_mover.py @@ +270,5 @@ > + #todo more efficient to scan all at once? > + if self._matches_exclude(file_name): > + self.info("Excluding {} from virus scan".format(file_name)) > + else: > + if os.path.exists(self.dest_dir): typically in mozharness all artifacts and deps per each script are self contained in a work_dir. could we put the cache dir in there?: self.dest_dir = os.path.join(self.query_abs_dirs()['abs_work_dir'], 'cache') @@ +272,5 @@ > + self.info("Excluding {} from virus scan".format(file_name)) > + else: > + if os.path.exists(self.dest_dir): > + self.info('Emptying {}'.format(self.dest_dir)) > + shutil.rmtree(self.dest_dir) can we use self.rmtree() here? adds some mh logging and retry https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#162 @@ +273,5 @@ > + else: > + if os.path.exists(self.dest_dir): > + self.info('Emptying {}'.format(self.dest_dir)) > + shutil.rmtree(self.dest_dir) > + os.makedirs(self.dest_dir) self.mkdir() https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#137 @@ +275,5 @@ > + self.info('Emptying {}'.format(self.dest_dir)) > + shutil.rmtree(self.dest_dir) > + os.makedirs(self.dest_dir) > + self.info('Copying {} to {}'.format(file_name,self.dest_dir)) > + shutil.copy(file_name, self.dest_dir) self.copyfile uses shutil.copy too but with some logging. https://dxr.mozilla.org/build-central/source/mozharness/mozharness/base/script.py#517

Yes, I think 1) is the better option, I'll refactor toward that

updated patch to address feedback comments

Comment on attachment 8717525 [details] [diff] [review] bug1210538-2.patch asking rail for feedback since jlund is away not sure how we parse antivirus failures currently or if this is covered by parsing the mozharness output

Comment on attachment 8717525 [details] [diff] [review] bug1210538-2.patch Review of attachment 8717525 [details] [diff] [review]: ----------------------------------------------------------------- Just some cosmetic changes. ::: testing/mozharness/scripts/release/beet_mover.py @@ +171,5 @@ > + # if excludes is set from command line, use it otherwise use defaults > + if 'excludes' in self.config: > + self.excludes = self.config['excludes'] > + else: > + self.excludes = DEFAULT_EXCLUDES or in one line: self.excludes = self.config.get('excludes', DEFAULT_EXCLUDES) @@ +173,5 @@ > + self.excludes = self.config['excludes'] > + else: > + self.excludes = DEFAULT_EXCLUDES > + dirs = self.query_abs_dirs() > + self.dest_dir = os.path.join(dirs['abs_work_dir'],CACHE_DIR) a nit: add a space after coma. @@ +245,5 @@ > + conn = boto.connect_s3(self.aws_key_id, self.aws_secret_key) > + > + # TODO - do we want to mirror/upload to more than one region? > + dirs = self.query_abs_dirs() > + boto = self.virtualenv_imports['boto'] Looks like "boto = self..." is used twice here. Also, it doesn't look that it's used in this method. You can kill 3 lines of code and 1 comment line here! :) @@ +277,5 @@ > for deliverable in self.manifest['mapping'][locale]: > self.log("uploading '{}' deliverable for '{}' locale".format(deliverable, locale)) > + #we have already downloaded the files locally so we can use that version > + source = self.manifest['mapping'][locale][deliverable]['artifact'] > + downloaded_file = os.path.join(dirs['abs_work_dir'],source.split(os.sep)[-1]) os.sep won't work for URLs on windows (who cares!), but it's be better to either explicitly split by "/". I think, it'd be better to either pass the same file name to self.download_file() and use it here or use something like downloaded_file = os.path.join(dirs['abs_work_dir'], self.get_filename_from_url(source)) @@ +320,5 @@ > + """Gets a copy of extract_and_run_command.py from tools, and the supporting mar.py, > + so that we can unpack various files for clam to scan them.""" > + remote_file = "{}/raw-file/{}/stage/extract_and_run_command.py".format(self.config["tools_repo"], > + self.config["tools_revision"]) > + self.download_file(remote_file, file_name="extract_and_run_command.py") I don't think we use extract_and_run_command.py anywhere except AV scan. Maybe we can just drop a copy of it into "external_tools" to reduce network operations? @@ +325,3 @@ > > > + def scan_bits(self): shouldn't it be "scan_beets"? :D Just kidding. @@ +329,5 @@ > + dirs = self.query_abs_dirs() > + self._get_extract_script() > + > + file_names = [] > + from os.path import isfile, join can you move this import to the top? @@ +330,5 @@ > + self._get_extract_script() > + > + file_names = [] > + from os.path import isfile, join > + filenames = [f for f in listdir(dirs['abs_work_dir']) if isfile(join(dirs['abs_work_dir'], f))] Assuming this is a flat directory? os.listdir() won't walk sub directories. @@ +332,5 @@ > + file_names = [] > + from os.path import isfile, join > + filenames = [f for f in listdir(dirs['abs_work_dir']) if isfile(join(dirs['abs_work_dir'], f))] > + if not os.path.exists(self.dest_dir): > + self.mkdir_p(self.dest_dir) I'd just self.mkdir_p without if - it won't complain if that directory exists. @@ +337,5 @@ > + for file_name in filenames: > + if self._matches_exclude(file_name): > + self.info("Excluding {} from virus scan".format(file_name)) > + else: > + self.info('Copying {} to {}'.format(file_name,self.dest_dir)) a nit: space after coma. @@ +354,5 @@ > + def _matches_exclude(self, keyname): > + for exclude in self.excludes: > + if re.search(exclude, keyname): > + return True > + return False you can make it shorter! :) return any(re.search(exclude, keyname) for exclude in self.excludes)

updated patch with latest suggestions

Comment on attachment 8718080 [details] [diff] [review] bug1210538-3.patch Review of attachment 8718080 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! Just one nit: ::: testing/mozharness/scripts/release/beet_mover.py @@ +232,5 @@ > + self.log('downloading and uploading artifacts to self_dest_dir...') > + > + # TODO - do we want to mirror/upload to more than one region? > + dirs = self.query_abs_dirs() > + boto = self.virtualenv_imports['boto'] boto is still here and unused. you can kill it.

Comment on attachment 8718307 [details] [diff] [review] bug1210538-4.patch Review of attachment 8718307 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozharness/scripts/release/beet_mover.py @@ +323,5 @@ > + > + def _scan_files(self): > + """Scan the files we've collected. We do the download and scan concurrently to make > + it easier to have a coherent log afterwards. Uses the venv python.""" > + self.run_command([self.query_python_path(), 'extract_and_run_command.py', Sorry, I missed this in my previous review. I think mozharness won't be able to find this file in PATH, so need to explicitly specify the full path to it You can use a trick similar to https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/purge.py#13-17 (at the top) import mozharness external_tools_path = os.path.join( os.path.abspath(os.path.dirname(os.path.dirname(mozharness.__file__))), 'external_tools', ) (and here) self.run_command([self.query_python_path(), os.path.join(external_tools_path, 'extract_and_run_command.py'), ...

Comment on attachment 8719863 [details] [diff] [review] bug1210538-5.patch Review of attachment 8719863 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!

Comment on attachment 8719863 [details] [diff] [review] bug1210538-5.patch Review of attachment 8719863 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozharness/scripts/release/beet_mover.py @@ -208,5 @@ > dirs = self.query_abs_dirs() > boto = self.virtualenv_imports['boto'] > > - # download locally > - file_name = self.retry(self.download_file, I don't think we can delete the var file_name as it is used in this method. maybe we should change `file_name` refs in upload_bit() body to `source`?

apologies I missed this in the earlier patch, I had to comment out the upload part during testing because my aws credentials don't work. Found an unused variable too

Comment on attachment 8720395 [details] [diff] [review] bug1210538-6.patch Review of attachment 8720395 [details] [diff] [review]: ----------------------------------------------------------------- lgtm :)

Looking at this bug, it appears that the virus scanning worked for en-US https://tools.taskcluster.net/task-inspector/#3BcGngNCTzKFAMf77SXabQ/0 https://public-artifacts.taskcluster.net/3BcGngNCTzKFAMf77SXabQ/0/public/logs/live_backing.log There is the message "no key specified to validate 1 signature" but this appears to be harmless according to this bug https://bugzilla.mozilla.org/show_bug.cgi?id=755114 I'll land the patches on m-i and then I can close this bug

patches to upload to m-i. I also included my Dockerfile from bug 1247428