Bug 1889223 - Submit all files to notarization before polling #957

hneiva · 2024-04-02T20:16:19Z

This is the first behavior that consumes all files at once, so it's a bit odd how it's being handled in script.py

bhearsum

This overall logic here is fine, but I'd really prefer to see some cleanup happen that avoids the need for having completely separate paths in script.py for the stacked notarization and other formats. Doing that would make some of the other comments irrelevant.

I'd also like to see if we can upload a single zip instead of a bunch of smaller requests (see comment further down).

bhearsum · 2024-04-10T14:16:22Z

signingscript/src/signingscript/script.py

        filelist_dict = build_filelist_dict(context)
        for path, path_dict in filelist_dict.items():
+            if path_dict["formats"] == ["apple_notarization_stacked"]:


Will this actually function correctly if some files are not apple_notarization_stacked? Eg: will we sign (or notarize) them correctly, and then move on and do the stacked ones correctly? If not, we should disallow apple_notarization_stacked for only some files, and bail at a higher level here.

Good point. I'll disallow mixed formats when stacked is present

Did disallowing mixed formats get implemented?

bhearsum · 2024-04-10T14:17:51Z

signingscript/src/signingscript/script.py

@@ -54,6 +57,15 @@ async def async_main(context):
                copy_to_dir(os.path.join(work_dir, source), context.config["artifact_dir"], target=source)
            if "gpg" in path_dict["formats"] or "autograph_gpg" in path_dict["formats"]:
                copy_to_dir(context.config["gpg_pubkey"], context.config["artifact_dir"], target="public/build/KEY")
+
+        # notarization_stacked is a special format that takes in all files at once instead of sequentially like other formats
+        notarization_dict = {path: path_dict for path, path_dict in filelist_dict.items() if "apple_notarization_stacked" in path_dict["formats"]}


Is using a new format for this just to make it safer to roll out? If so, what are the plans for apple_notarization and apple_notarization_geckodriver after we switch to stacked? (Ideally, it seems like we ought to move everything to stacked.)

Yes, I initially replaced apple_notarization but decided against it to allow for the feature to ride the train.
I'll add deprecation notices on the old functions.

I'll leave geckodriver alone for now, and refactor that as we move things around later.

Let's get an issue on file to remove this later, too.

bhearsum · 2024-04-10T14:21:55Z

signingscript/src/signingscript/sign.py

+        if extension == ".pkg":
+            path = os.path.join(notarization_workdir, relpath)
+            utils.copy_to_dir(path_dict["full_path"], notarization_workdir, target=relpath)
+            submissions_map[path] = await retry_async(


How many files will we be notarization in parallel? We may want to limit how many we do at once. Especially spread over all of the iscript machines, this could end up doing a lot at once, and potentially hitting a rate limit or other problems.

(Note: if we end up making a single request with a big zip file, this is not an issue.)

That's why I called it stacked and not parallel.
It's still submitting each file sequentially, but it doesn't start polling for results until all files have been submitted.
That way the max parallel submissions are controlled by our chunking still.
Hopefully this won't hit a limit on apple's end.

Correct me if I'm wrong, but we're still going to have more files in the process of being notarized at the same time, no?

Just want to highlight this one last time. We're submitted the files synchronously, but we're not waiting on one to finish notarizating before we submit the next one - which means more will be in Apple's system at any given time than before. I have no issue with this, but please keep an eye on notarization tasks when this is activated to see if we start to hit more timeouts or intermittents. If we do, we should add some limits.

Ultimately I think this gets us closer to the previous behaviour where one task was submitting files to apple, and then a separate task was polling the service to get the notarized artifact back and staple it?

bhearsum · 2024-04-10T14:24:07Z

signingscript/src/signingscript/sign.py

+    relpath_index_map = {}
+    task_index = 0
+    # Submit to notarization one by one
+    for relpath, path_dict in filelist_dict.items():


Correct me if I'm wrong, but I seem to recall that we can upload a zipfile with multiple files we want notarized inside. Assuming that's true, I suggest we construct a new zipfile and make a single request rather than making a bunch of smaller ones. (This would avoid potential issues with rate limiting or similar problems.)

For what I've gathered it's supported - but on the apple side it will check the apps sequentially, making things slower.
With the current setup we're making the exact same amount of parallel requests, so not much of a risk to be rate-limited - unless they could by requests per ~minute(?), which would be weird.

bhearsum · 2024-04-10T14:26:16Z

signingscript/src/signingscript/sign.py

+
+
+@time_async_function
+async def apple_notarize_stacked(context, filelist_dict):


There's a lot of overlap between this function and _notarize_all. If we're planning to keep the latter, some refactoring to avoid all of the duplication would be nice.

bhearsum · 2024-04-10T14:28:30Z

signingscript/src/signingscript/sign.py

+            workdir_files = os.listdir(notarization_workdir)
+            supported_files = [filename for filename in workdir_files if _can_notarize(filename, (".app", ".pkg"))]
+            if not supported_files:
+                raise SigningScriptError("No supported files found")


It would be preferable to do this sort of sanity checking prior to submitting anything - otherwise we're making notarization requests that will never be used.

We're mostly handling multiple l10n versions of the same app, this should only only happen if something is broken upstream, and likely would be missing for all packages. It's a rare enough case that there's no point in optimizing for.

I don't think this is about optimization, it's about being defensive, not trusting your inputs, and failing early.

signingscript/src/signingscript/sign.py

signingscript/src/signingscript/task.py

bhearsum · 2024-04-10T14:45:37Z

signingscript/src/signingscript/sign.py

@@ -1633,8 +1633,7 @@ async def apple_notarize(context, path, *args, **kwargs):
    """
    # Setup workdir
    notarization_workdir = os.path.join(context.config["work_dir"], "apple_notarize")
-    shutil.rmtree(notarization_workdir, ignore_errors=True)
-    utils.mkdir(notarization_workdir)
+    utils.mkdir(notarization_workdir, delete_before_create=True)


This is delete_before_create necessary now? Especially for the existing notarization functions?

it's more of a safeguard. I don't see why we wouldn't want that..?

I guess what I'm wondering is: why is it getting added? Did you run into something? (I don't really have a problem with it...)

To be fair, this is more relevant for local testing to ensure we don't have anything in the workdir. AIUI In CI scriptworker takes care of deleting everything after each task is complete.

I don't think something called mkdir should be doing rmtree. Since this seems to be a no-op (we were calling rmtree + mkdir, now we're calling mkdir which itself calls rmtree before re-creating the dir), I'd rather this was reverted.

bhearsum

Leaving unapproved for the moment for the question about disallowing mixed formats - I'm OK with what's here though.

bhearsum · 2024-04-23T14:06:39Z

signingscript/src/signingscript/script.py

            if not context.config.get("apple_notarization_configs", False):
                raise Exception("Apple notarization is enabled but apple_notarization_configs is not defined")
            setup_apple_notarization_credentials(context)

        context.session = session
        context.autograph_configs = load_autograph_configs(context.config["autograph_configs"])

-        work_dir = context.config["work_dir"]
+        # TODO: Make task.sign take in the whole filelist_dict and return a dict of output files.


Let's get an issue on file for this, please.

bhearsum · 2024-04-23T14:07:13Z

signingscript/src/signingscript/script.py

        filelist_dict = build_filelist_dict(context)
        for path, path_dict in filelist_dict.items():
+            if path_dict["formats"] == ["apple_notarization_stacked"]:


Did disallowing mixed formats get implemented?

bhearsum · 2024-04-23T14:07:51Z

signingscript/src/signingscript/script.py

@@ -54,6 +57,15 @@ async def async_main(context):
                copy_to_dir(os.path.join(work_dir, source), context.config["artifact_dir"], target=source)
            if "gpg" in path_dict["formats"] or "autograph_gpg" in path_dict["formats"]:
                copy_to_dir(context.config["gpg_pubkey"], context.config["artifact_dir"], target="public/build/KEY")
+
+        # notarization_stacked is a special format that takes in all files at once instead of sequentially like other formats
+        notarization_dict = {path: path_dict for path, path_dict in filelist_dict.items() if "apple_notarization_stacked" in path_dict["formats"]}


Let's get an issue on file to remove this later, too.

bhearsum · 2024-04-23T14:12:31Z

signingscript/src/signingscript/sign.py

+        if extension == ".pkg":
+            path = os.path.join(notarization_workdir, relpath)
+            utils.copy_to_dir(path_dict["full_path"], notarization_workdir, target=relpath)
+            submissions_map[path] = await retry_async(


Just want to highlight this one last time. We're submitted the files synchronously, but we're not waiting on one to finish notarizating before we submit the next one - which means more will be in Apple's system at any given time than before. I have no issue with this, but please keep an eye on notarization tasks when this is activated to see if we start to hit more timeouts or intermittents. If we do, we should add some limits.

bhearsum · 2024-04-23T14:14:28Z

signingscript/src/signingscript/task.py

@@ -56,6 +58,8 @@
        "autograph_rsa": sign_file_detached,
        "apple_notarization": apple_notarize,
        "apple_notarization_geckodriver": apple_notarize_geckodriver,
+        # This format is handled in script.py
+        # "apple_notarization_stacked": apple_notarize_stacked,


We should be able to map this format to a function once the first todo above (about sign) is addressed. When you file that issue, please reference it here as well.

bhearsum

Thanks for filing #980!

Please add a comment near the relevant code before merging this.

jcristau

Requesting changes for the mkdir bits, I'm fine with the rest of this, thanks.

jcristau · 2024-04-24T09:26:21Z

signingscript/src/signingscript/sign.py

@@ -1633,8 +1633,7 @@ async def apple_notarize(context, path, *args, **kwargs):
    """
    # Setup workdir
    notarization_workdir = os.path.join(context.config["work_dir"], "apple_notarize")
-    shutil.rmtree(notarization_workdir, ignore_errors=True)
-    utils.mkdir(notarization_workdir)
+    utils.mkdir(notarization_workdir, delete_before_create=True)


I don't think something called mkdir should be doing rmtree. Since this seems to be a no-op (we were calling rmtree + mkdir, now we're calling mkdir which itself calls rmtree before re-creating the dir), I'd rather this was reverted.

jcristau · 2024-04-24T09:28:32Z

signingscript/src/signingscript/sign.py

+        if extension == ".pkg":
+            path = os.path.join(notarization_workdir, relpath)
+            utils.copy_to_dir(path_dict["full_path"], notarization_workdir, target=relpath)
+            submissions_map[path] = await retry_async(


Ultimately I think this gets us closer to the previous behaviour where one task was submitting files to apple, and then a separate task was polling the service to get the notarized artifact back and staple it?

jcristau · 2024-04-24T09:30:01Z

signingscript/src/signingscript/sign.py

+            workdir_files = os.listdir(notarization_workdir)
+            supported_files = [filename for filename in workdir_files if _can_notarize(filename, (".app", ".pkg"))]
+            if not supported_files:
+                raise SigningScriptError("No supported files found")


I don't think this is about optimization, it's about being defensive, not trusting your inputs, and failing early.

This is the first behavior that consumes all files at once, so it's a bit odd how it's being handled in script.py

Removed noop sign sign in tasks.py since we don't allow for mixed formats anymore

…creating

hneiva · 2024-04-25T19:00:25Z

Removed delete_before_create

Ultimately I think this gets us closer to the previous behaviour where one task was submitting files to apple, and then a separate task was polling the service to get the notarized artifact back and staple it?

At a high level, yes!

I don't think this is about optimization, it's about being defensive, not trusting your inputs, and failing early.

This feels like we're over-engineering for a red herring case, but I've updated the code to check everything before submitting.

hneiva marked this pull request as draft April 2, 2024 20:19

hneiva force-pushed the hneiva/async-notarize branch 3 times, most recently from d7c01e7 to b3dc1be Compare April 4, 2024 22:33

hneiva marked this pull request as ready for review April 9, 2024 22:07

hneiva requested a review from a team April 9, 2024 22:07

bhearsum requested changes Apr 10, 2024

View reviewed changes

hneiva force-pushed the hneiva/async-notarize branch from 025be75 to d9a2d13 Compare April 11, 2024 19:27

hneiva requested review from bhearsum and jcristau April 22, 2024 21:15

bhearsum reviewed Apr 23, 2024

View reviewed changes

hneiva mentioned this pull request Apr 23, 2024

[signingscript] Refactor behaviors and allow taking in all files at once #980

Open

hneiva force-pushed the hneiva/async-notarize branch from 58686e6 to 8f86c0d Compare April 23, 2024 18:26

bhearsum approved these changes Apr 23, 2024

View reviewed changes

hneiva force-pushed the hneiva/async-notarize branch from fee69f8 to 3babb54 Compare April 23, 2024 18:35

jcristau added the signingscript label Apr 24, 2024

jcristau requested changes Apr 24, 2024

View reviewed changes

hneiva added 6 commits April 25, 2024 10:13

Bug 1889223 - Submit all files to notarization before polling

ef6d833

This is the first behavior that consumes all files at once, so it's a bit odd how it's being handled in script.py

ci(signingscript): PR changes + Prevent mixed formats

f69f61e

Removed noop sign sign in tasks.py since we don't allow for mixed formats anymore

nit(signingscript): Add reference issue comments for stacked behavior

a1e0009

nit(signingscript): Lint fix

c26bafa

nit(signingscript): Revert utils.mkdir parameter for deleting before …

df02a5c

…creating

fix(signingscript): Check all files before submitting for notarization

7925b96

hneiva force-pushed the hneiva/async-notarize branch from 3babb54 to 7925b96 Compare April 25, 2024 17:53

chore(signingscript): Add tests for apple_notarize_stacked

12d3b77

jcristau approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'master' into hneiva/async-notarize

9eac5fd

hneiva enabled auto-merge (squash) April 26, 2024 17:19

hneiva merged commit 06e2958 into master Apr 26, 2024
9 checks passed

hneiva deleted the hneiva/async-notarize branch April 26, 2024 17:24



		@time_async_function
		async def apple_notarize_stacked(context, filelist_dict):

Bug 1889223 - Submit all files to notarization before polling #957

Bug 1889223 - Submit all files to notarization before polling #957

Conversation

hneiva commented Apr 2, 2024

bhearsum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhearsum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhearsum left a comment

Choose a reason for hiding this comment

jcristau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hneiva commented Apr 25, 2024