1440777 - Add support for local actions and implement console-log as a local action

Status: RESOLVED FIXED

(Firefox :: Normandy Client, enhancement, P1)

Description

[Michael Cooper [:mythmon]]

To make migrating to client-implemented actions easier, we'll do it one action at a time. To make that feasible, we'll need to have a hybrid action runner. If an action exists in the client, it will be used. If it is not used, fallback to requesting the action from the server and running in the driver sandbox.

To test this out, we'll convert the basic console-log action to the new system.

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1440777 - Add support for local actions and implement console-log as a local action

* Add ActionsManager to provide a common interface to local and remote actions
* Move action handle logic from RecipeRunner to new ActionsManager
* Implement BaseAction for all local actions
* Implement ConsoleLog as a subclass of BaseAction

Review commit: https://reviewboard.mozilla.org/r/228160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228160/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review234134

::: commit-message-26936:6
(Diff revision 1)
> +Bug 1440777 - Add support for local actions and implement console-log as a local action
> +
> +* Add ActionsManager to provide a common interface to local and remote actions
> +* Move action handle logic from RecipeRunner to new ActionsManager
> +* Implement BaseAction for all local actions
> +* Implement ConsoleLog as a subclass of BaseAction

So fundamentally, AFAICT a recipe runs only 1 action. What's the point of having a recipe that only does a single console log action, only with strings it provides entirely itself? :-\

::: toolkit/components/normandy/actions/ConsoleLog.jsm:13
(Diff revision 1)
> +
> +var EXPORTED_SYMBOLS = ["ConsoleLog"];
> +
> +class ConsoleLog extends BaseAction {
> +  async _run(recipe) {
> +    this.log.info(recipe.arguments.message);

The remote actions all run in sandboxes. As far as I can tell, this doesn't. So now we're moving stuff out of sandboxes into privileged code. That increases risk.

What *guarantees* do we have about what `recipe.arguments.message` is (e.g. is it always a string? Always a JSON-serializable type? Can it have a custom `toString()` method) ? Can you document this, and why those guarantees hold?

::: toolkit/components/normandy/lib/ActionsManager.jsm:24
(Diff revision 1)
> + * implementations are fetched from the Normandy server; their
> + * lifecycles are managed by `normandy/lib/ActionSandboxManager.jsm`.
> + * Local actions have their implementations packaged in the Normandy
> + * client, and manage their lifecycles internally.
> + */
> +class ActionsManager {

Nit: It's a `RecipeRunner`, `ActionSandboxManager`, `LogManager` -- shouldn't it be `ActionManager` rather than `ActionsManager`, for consistency?

::: toolkit/components/normandy/lib/ActionsManager.jsm:100
(Diff revision 1)
> +          log.error(`Could not execute recipe ${recipe.name}:`);
> +          Cu.reportError(e);

In bug 1445814 you reduced some of these duplicate calls. This seems to have been written before that bug, can you make sure we remove the duplicate call here too, after conflict resolution/rebasing ?

::: toolkit/components/normandy/test/browser/browser_ActionsManager.js:15
(Diff revision 1)
> +  withStub(NormandyApi, "fetchImplementation"),
> +  async function(fetchActionsStub, fetchImplementationStub) {
> +    const remoteAction = {name: "remote-action"};
> +    const localAction = {name: "local-action"};
> +    fetchActionsStub.resolves([remoteAction, localAction]);
> +    fetchImplementationStub.callsFake(async (...args) => `window.calledWith = ${JSON.stringify(args)};`);

Does this pass on infra? I would expect extra properties on the window to set off mochitest's "you leaked stuff onto the global scope" complaints. Maybe I'm overly paranoid...

Can you elaborate on why this is necessary? I'm very much not a sinon expert, but I thought sinon was supposed to take care of this type of thing for you? :-)

::: toolkit/components/normandy/test/browser/browser_ActionsManager.js:171
(Diff revision 1)
> +      sandboxManagerBroken.removeHold.args,
> +      [["ActionsManager"]],
> +      "sandbox holds should still be removed after a recipe failure",
> +    );
> +
> +    Assert.deepEqual(reportActionStub.args, [["test-remote-action-broken", Uptake.ACTION_SUCCESS]]);

This seems kind of odd if the action threw when running it?

::: toolkit/components/normandy/test/browser/browser_ActionsManager.js:273
(Diff revision 1)
> +        case "migratedAction":
> +          return "// this shouldn't be requested";

Is there a bug on file for fixing this? We should include a bug number here.

::: toolkit/components/normandy/test/browser/browser_BaseAction.js:167
(Diff revision 1)
> +    );
> +
> +    Assert.deepEqual(
> +      reportActionStub.args,
> +      [[action.name, Uptake.ACTION_SUCCESS]],
> +      "Action should report report success",

Nit: report report

::: toolkit/components/normandy/test/browser/browser_BaseAction.js:189
(Diff revision 1)
> +      reportActionStub.args,
> +      [[action.name, Uptake.ACTION_POST_EXECUTION_ERROR]],

It's pretty counter-intuitive that having a failing `finalize` step will prevent `ACTION_SUCCESS` from being reported, but having a failing *run* step won't (that only gets reported through the recipe, from reading these tests). Am I misunderstanding something?

Comment 3

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review234134

> So fundamentally, AFAICT a recipe runs only 1 action. What's the point of having a recipe that only does a single console log action, only with strings it provides entirely itself? :-\

The console-log action is indeed practically useless. It also already exists in the Normandy ecosystem. It is the simplest possible thing that an action and recipe can do. We use it for manual testing. It is extremely easy to tell that it ran, and it has very simple execution.

The reason it is implemented here is because having an action to port made implemented the local action system easier. Eventually I plan to port every action to this framework. This work is all filed in bugs that depend on this one.

> The remote actions all run in sandboxes. As far as I can tell, this doesn't. So now we're moving stuff out of sandboxes into privileged code. That increases risk.
> 
> What *guarantees* do we have about what `recipe.arguments.message` is (e.g. is it always a string? Always a JSON-serializable type? Can it have a custom `toString()` method) ? Can you document this, and why those guarantees hold?

Yes, local actions do not run in sandboxes. The goal here is to eventually simplify the normandy client, removing the sandbox entirely. I argue this actually reduces risk.

The sandbox is not intended to protect against malicious recipes, but malicious actions. Recipes are fetched from the server as JSON, so we can guarantee by constructions that `recipe.arguments.message` is a JSON-serializable type. Remote actions are executable Javascript fetched from a server. A local action is trusted, since it is part of the codebase.

> Nit: It's a `RecipeRunner`, `ActionSandboxManager`, `LogManager` -- shouldn't it be `ActionManager` rather than `ActionsManager`, for consistency?

`ActionSandboxManager` manages a single action, so during a single execution of Normandy, there are many instances of `ActionSandboxManager`. `ActionsManager` is plural because one instance manages multiple actions (including multiple `ActionSandboxManagers`).

To me `LogManager` and `RecipeRunner` shouldn't be plural either. `LogManager` isn't plural because it manages a single instance of `resource://gre/modules/Log.jsm` (Though maybe `LogManager` should go away...). `RecipeManager` isn't plural because to me that would sound very strange. I wouldn't speak of a "recipes book", even if it was a book containing multiples recipes. I'd call it a "recipe book". This seems fairly weak though, and is maybe a sign that `RecipeRunner` is misnamed.

Eitherway, I think `ActionsManager` is the correct name here.

> In bug 1445814 you reduced some of these duplicate calls. This seems to have been written before that bug, can you make sure we remove the duplicate call here too, after conflict resolution/rebasing ?

Oops, yes. Nice catch.

> Does this pass on infra? I would expect extra properties on the window to set off mochitest's "you leaked stuff onto the global scope" complaints. Maybe I'm overly paranoid...
> 
> Can you elaborate on why this is necessary? I'm very much not a sinon expert, but I thought sinon was supposed to take care of this type of thing for you? :-)

This passes on infra because `window` is in a sandbox object, and will be destroyed by the end of the test. It doesn't have any external references, except the sandbox manager managed by ActionsManager, which will go out of scope with this test.

In this case the bit of code here is not neccesary though, since it isn't used in this test. I'll remove it.

> This seems kind of odd if the action threw when running it?

This is mostly keeping compatibility with the existing implementation. `ACTION_SUCCESS` isn't really obvious, but the execution model allows some recipes to fail and some to pass. Consider an add-on study: it could fail to install an add-on, but succeed with another. Since individual recipes have a separate reporting line, we treat actions as successful if their pre and post-steps succeed.

> Is there a bug on file for fixing this? We should include a bug number here.

This isn't a problem in the code. This is a value that this function could concievably be called with, but if it was the test would fail. This is the case of a action that exists both remote (on the server), and locally (in the client). It is neccesary that we test this case, since it will exist in practice.

> It's pretty counter-intuitive that having a failing `finalize` step will prevent `ACTION_SUCCESS` from being reported, but having a failing *run* step won't (that only gets reported through the recipe, from reading these tests). Am I misunderstanding something?

It is because a failing run step doesn't stop other recipes from running with the action, and recipes are expected to be independent of eachother. If one execution session includes 3 add-on studies, for example, and 1 of them has a URL that doesn't resolve correctly, but the other 2 are fine, then Normamndy will install the 2 that are well formatted. The action can work perfectly well in this situation, and so would be considered successful.

However, post-steps are only one-per action, not one per each recipe. They are used for cleanup and unerollment. If they don't work, that is a fault of the action, not the recipe.

Possibly the model should be improved to distinguish between an action failing during the run step, and a recipe being semantically invalid. However, tihs is really a problem with the pre-existing action workflow, and not the change to support local actions. A lot of this patch is moving code around, and adding a new way of running actions that still conforms to the old workflow. I don't think the details of that workflow (such as uptake reporting) are appropriate to change right now.

Comment 4

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review234910

::: commit-message-26936:6
(Diff revision 1)
> +Bug 1440777 - Add support for local actions and implement console-log as a local action
> +
> +* Add ActionsManager to provide a common interface to local and remote actions
> +* Move action handle logic from RecipeRunner to new ActionsManager
> +* Implement BaseAction for all local actions
> +* Implement ConsoleLog as a subclass of BaseAction

The console-log action is indeed practically useless. It is the simplest possible thing that an action and recipe can do. We use it for manual testing. It is extremely easy to tell that it ran, and it has very simple execution.

The reason it is implemented here is because having an action to port made the local-action sytsem easier to focus on. Eventually we plan to port every action to this framework, in separate bugs (all dependents of this bug).

Comment 5

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/1-2/

Comment 6

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/2-3/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review238612


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/normandy/test/browser/browser_action_ConsoleLog.js:32
(Diff revision 3)
> +    try {
> +      // message is required
> +      let recipe = {id: 1, arguments: {}};
> +      await action.runRecipe(recipe);
> +      Assert.deepEqual(infoStub.args, [], "no message should be logged");
> +      Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]])

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/normandy/test/browser/browser_action_ConsoleLog.js:40
(Diff revision 3)
> +
> +      // message must be a string
> +      recipe = {id: 1, arguments: {message: 1}};
> +      await action.runRecipe(recipe);
> +      Assert.deepEqual(infoStub.args, [], "no message should be logged");
> +      Assert.deepEqual(reportRecipeStub.args, [[recipe.id, Uptake.RECIPE_EXECUTION_ERROR]])

Error: Missing semicolon. [eslint: semi]

Comment 8

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/3-4/

Comment 9

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/4-5/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review238814

Comment 11

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

https://reviewboard.mozilla.org/r/228160/#review238900

::: toolkit/components/normandy/actions/schemas/package.json:1
(Diff revision 5)
> +{

This file is getting included in the build, which fails the "all files referenced" check. This needs to be fixed before landing.

Comment 12

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/5-6/

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a8c20530cc7b2a0488502be4194bdca9221a5f9e -d ddd9187ed66d: rebasing 455934:a8c20530cc7b "Bug 1440777 - Add support for local actions and implement console-log as a local action r=Gijs" (tip)
merging toolkit/components/normandy/lib/RecipeRunner.jsm
warning: conflicts while merging toolkit/components/normandy/lib/RecipeRunner.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Michael Cooper [:mythmon]]

Comment on attachment 8959355 [details]
Bug 1440777 - Add support for local actions and implement console-log as a local action

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228160/diff/6-7/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bca1d807b8ad
Add support for local actions and implement console-log as a local action r=Gijs

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bca1d807b8ad