[M118] Retry notarization failures in the internal invoker

This cherry-picks the following commits:

eb9f3eb Roll src/chrome/installer/mac/internal/ 3cb1c9bad..ed0d43308 (2 commits)
e06e58c macOS Signing Scripts: Make the retry logic part of the notarize module API
1bf5c0c macOS Signing Scripts: Remove legacy notarization arguments
e096178 Roll src/chrome/installer/mac/internal/ d7fb448e1..3cb1c9bad (1 commit)

Bug: 1496421, b/281728988, 1442256
Change-Id: If0e4d435b2f3aaf1cd6d0064081c5dbc750d47b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4974536
Auto-Submit: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/branch-heads/5993@{#1509}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}

DEPS

@@ -4109,7 +4109,7 @@ deps = {
   # grepping.
   'src/chrome/installer/mac/internal': {
       'url': Var('chrome_git') + '/chrome/installer/mac/internal.git' + '@' +
-        'd7fb448e1fc84c05c40719d83d785fe2a131e616',
+        'ed0d43308e9fa81b929468a3e651cb60744d9415',
       'condition': 'checkout_src_internal',
   },
 

chrome/installer/mac/signing/driver_test.py

@@ -212,51 +212,3 @@ def test_notarize_notarytool(self, sign_all, **kwargs):
         config = sign_all.call_args.args[1]
         self.assertEquals(model.NotarizeAndStapleLevel.STAPLE, config.notarize)
         self.assertEquals([], config.invoker.notarizer.notary_args)
-
-    def test_notarize_legacy_args_user(self, sign_all, **kwargs):
-        driver.main([
-            '--input',
-            '/input',
-            '--output',
-            '/output',
-            '--identity',
-            'G',
-            '--notarize',
-            '--notary-user',
-            'Notary-User',
-        ])
-        self.assertEquals(1, sign_all.call_count)
-        config = sign_all.call_args.args[1]
-        self.assertEquals([], config.invoker.notarizer.notary_args)
-
-    def test_notarize_legacy_args_password(self, sign_all, **kwargs):
-        driver.main([
-            '--input',
-            '/input',
-            '--output',
-            '/output',
-            '--identity',
-            'G',
-            '--notarize',
-            '--notary-password',
-            '@env:PASSWORD',
-        ])
-        self.assertEquals(1, sign_all.call_count)
-        config = sign_all.call_args.args[1]
-        self.assertEquals([], config.invoker.notarizer.notary_args)
-
-    def test_notarize_legacy_args_team_id(self, sign_all, **kwargs):
-        driver.main([
-            '--input',
-            '/input',
-            '--output',
-            '/output',
-            '--identity',
-            'G',
-            '--notarize',
-            '--notary-team-id',
-            'TeamId',
-        ])
-        self.assertEquals(1, sign_all.call_count)
-        config = sign_all.call_args.args[1]
-        self.assertEquals([], config.invoker.notarizer.notary_args)

chrome/installer/mac/signing/notarize.py

@@ -38,24 +38,9 @@ def register_arguments(parser):
             'notarization tool and are intended to specify authentication '
             'parameters.')
 
-        # Legacy arguments that will be removed in the future.
-        legacy_help = ('This argument is deprecated. '
-                       'Please use --notary-arg instead.')
-        parser.add_argument('--notary-user', help=legacy_help)
-        parser.add_argument('--notary-password', help=legacy_help)
-        parser.add_argument('--notary-team-id', help=legacy_help)
-
     def __init__(self, args, config):
         self._notary_args = args.notary_arg
 
-        if any([
-                getattr(args, arg, None)
-                for arg in ('notary_user', 'notary_password', 'notary_team_id')
-        ]):
-            logger.warning(
-                'Explicit notarization authentication arguments are deprecated. Use --notary-arg instead.'
-            )
-
     @property
     def notary_args(self):
         return self._notary_args
@@ -71,9 +56,6 @@ def submit(self, path, config):
             'plist',
         ] + self.notary_args
 
-        # TODO(rsesek): As the reliability of notarytool is determined,
-        # potentially run the command through _notary_service_retry().
-
         output = commands.run_command_output(command)
         try:
             plist = plistlib.loads(output)
@@ -233,48 +215,76 @@ def staple(path):
             notarization and is now ready for stapling.
     """
 
-    def staple_command():
-        commands.run_command(['xcrun', 'stapler', 'staple', '--verbose', path])
-
-    # Known bad codes:
-    # 65 - CloudKit query failed due to "(null)"
-    # 68 - A server with the specified hostname could not be found.
-    _notary_service_retry(
-        staple_command, (65, 68), 'staple', sleep_before_retry=True)
-
+    retry = Retry('staple', sleep_before_retry=True)
+    while retry.keep_going():
+        try:
+            commands.run_command(
+                ['xcrun', 'stapler', 'staple', '--verbose', path])
+            return
+        except subprocess.CalledProcessError as e:
+            # Known bad codes:
+            bad_codes = (
+                65,  # CloudKit query failed due to "(null)"
+                68,  # A server with the specified hostname could not be found.
+            )
+            if e.returncode in bad_codes and retry.failed_should_retry(
+                    f'Output: {e.output}'):
+                continue
+            raise e
 
-def _notary_service_retry(func,
-                          known_bad_returncodes,
-                          short_command_name,
-                          sleep_before_retry=False):
-    """Calls the function |func| that runs a subprocess command, retrying it if
-    the command exits uncleanly and the returncode is known to be bad (e.g.
-    flaky).
 
-    Args:
-        func: The function to call within try block that wil catch
-            CalledProcessError.
-        known_bad_returncodes: An iterable of the returncodes that should be
-            ignored and |func| retried.
-        short_command_name: A short descriptive string of |func| that will be
-            logged when |func| is retried.
-        sleep_before_retry: If True, will wait before re-trying when a known
-            failure occurs.
+class Retry(object):
+    """Retry is a helper class that manages retrying notarization operations
+    that may fail due to transient issues. Usage:
 
-    Returns:
-        The result of |func|.
-    """
-    attempt = 0
-    while True:
-        try:
-            return func()
-        except subprocess.CalledProcessError as e:
-            attempt += 1
-            if (attempt < _NOTARY_SERVICE_MAX_RETRIES and
-                    e.returncode in known_bad_returncodes):
-                logger.warning('Retrying %s, exited %d, output: %s',
-                               short_command_name, e.returncode, e.output)
-                if sleep_before_retry:
-                    time.sleep(30)
-            else:
+        retry = Retry('staple')
+        while retry.keep_going():
+            try:
+                return operation()
+            except Exception as e:
+                if is_transient(e) and retry.failed_should_retry():
+                    continue
                 raise e
+    """
+
+    def __init__(self, desc, sleep_before_retry=False):
+        """Creates a retry state object.
+
+        Args:
+            desc: A short description of the operation to retry.
+            sleep_before_retry: If True, will sleep before proceeding with
+                a retry.
+        """
+        self._attempt = 0
+        self._desc = desc
+        self._sleep_before_retry = sleep_before_retry
+
+    def keep_going(self):
+        """Used as the condition for a retry loop."""
+        if self._attempt < _NOTARY_SERVICE_MAX_RETRIES:
+            return True
+        raise RuntimeError(
+            'Loop should have terminated at failed_should_retry()')
+
+    def failed_should_retry(self, msg=''):
+        """If the operation failed and the caller wants to retry it, this
+        method increments the attempt count and determines if another
+        attempt should be made.
+
+        Args:
+            msg: An optional message to describe the failure.
+
+        Returns:
+            True if the retry loop should continue, and False if the loop
+            should terminate with an error.
+        """
+        self._attempt += 1
+        if self._attempt < _NOTARY_SERVICE_MAX_RETRIES:
+            retry_when_message = ('after 30 seconds' if self._sleep_before_retry
+                                  else 'immediately')
+            logger.warning(f'Error during notarization command {self._desc}. ' +
+                           f'Retrying {retry_when_message}. {msg}')
+            if self._sleep_before_retry:
+                time.sleep(30)
+            return True
+        return False

chrome/installer/mac/signing/notarize_test.py

@@ -334,3 +334,22 @@ def test_fail_three_times(self, run_command, **kwargs):
                 ['xcrun', 'stapler', 'staple', '--verbose', '/tmp/file.dmg'])
         ])
         self.assertEqual(2, kwargs['sleep'].call_count)
+
+
+class TestRetry(unittest.TestCase):
+
+    def test_retry(self):
+        retry = notarize.Retry('test')
+        self.assertTrue(retry.failed_should_retry())
+        self.assertTrue(retry.keep_going())
+        self.assertTrue(retry.failed_should_retry())
+        self.assertTrue(retry.keep_going())
+        self.assertFalse(retry.failed_should_retry())
+        with self.assertRaises(RuntimeError):
+            retry.keep_going()
+
+    @mock.patch('time.sleep')
+    def test_with_sleep(self, sleep):
+        retry = notarize.Retry('test', sleep_before_retry=True)
+        self.assertTrue(retry.failed_should_retry())
+        self.assertEqual(1, sleep.call_count)