chore: Download browsers for all platforms to release builds from single machine #165

kyungeunni · 2022-02-24T15:32:57Z

Summary

fixes #150
fixes #172 (fixed by 891b9f6)

This PR adds an additional pre-build step. When running npm run release script, the pre-build script will download browsers for Mac Windows, and Linux (if the browsers are already downloaded, it will skip downloading steps. The path contains the revision number so it also prevents packaging the stale browser.). Then electron builder will include the right browser for each platform as specified in electron-builder.yml.

Implementation details

I couldn't utilize the playwright's registry library, as they are hardly coupled with the host machine's OS. Instead, I've used fetchBrowser library that is used by the registry's install function.

How to validate this change

runnpm run pack -- -mwl. It will digest builds in your local dist directory without publishing it.
try run each of the binaries in each platform.

Mac
Windows
Linux (.deb for the time being)

Next Steps

Release the binaries as described in RELEASE.md
Stretch it to include Apple Silicon build
Run the release script in CI on tags push.

apmmachine · 2022-02-24T15:49:21Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-04-06T14:13:04.220+0000
Duration: 14 min 55 sec

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

justinkambic · 2022-02-24T19:03:56Z

Thanks for taking this on, it represents a huge accomplishment.

In my process of smoke testing binaries I built with this branch, I encountered a problem running tests on my recorded steps for Linux and Windows.

This problem doesn't happen on the Mac build, so I think we are probably seeing the same issue here as we had with the browsers. Sharp isn't getting copied over for non-native builds, and thus it seems to be causing the test feature to fail.

vigneshshanmugam

Great work @kyungeunni 🎉

vigneshshanmugam · 2022-02-24T20:22:38Z

electron-builder.env

@@ -0,0 +1 @@
+BROWSER_REVISION=907428


We can automate this with ENV variable. We need to get it from

synthetics-recorder/scripts/install-pw.js

Line 29 in 13a1463

const SYNTHETICS_BROWSER_REVISIONS = require("@elastic/synthetics/node_modules/playwright-chromium/browsers.json");

vigneshshanmugam · 2022-02-24T20:23:42Z

scripts/download-browsers.js

+const { getChromeVersion } = require("./install-pw");
+
+const SUPPORTED_PLATFORMS = ["linux", "mac", "win"];
+const EXECUTABLE_PATHS = {


Need ffmpeg as well if i remember correctly for all platforms. Please double check if thats not the case.

Thanks for reminding me this!! I've updated the script to also download ffmpeg too.

vigneshshanmugam · 2022-02-24T20:25:07Z

scripts/download-browsers.js

+  }
+}
+// eslint-disable-next-line no-console
+downloadAll().then(() => console.log("Download succeeded"));


Move them inside this check if we want to execute only when running as main module and not while importing -

synthetics-recorder/scripts/install-pw.js

Line 50 in 13a1463

if (require.main === module) {

vigneshshanmugam · 2022-02-24T20:26:13Z

package.json

+    "release": "npm run pack -- -mwl --publish=always",
+    "pack": "run-s prepack electron-builder",
+    "prepack": "run-p build prepack:browsers",
+    "prepack:browsers": "node scripts/download-browsers",


As @justinkambic experienced, this would still break for native deps, We can fix it by running the preparation and package script serially for all platforms and uploading the pack one by one.

vigneshshanmugam · 2022-02-24T20:27:14Z

scripts/download-browsers.js

+  // 'mac-arm64': '%s/builds/chromium/%s/chromium-mac-arm64.zip',
+};
+
+async function download(platform, revision, directory) {


Did you try to check the Registry in Playwright if there was any other way to do this?

Yes I went through all the Registry in Playwright and it seemed to be no work way around in there, especially I couldn't mock the platform as they would check the host machine's platform in various steps. So I used one of the functions that Registry uses to download the browser which is not tied to the host machine's platform(you pass it via parameter)

kyungeunni · 2022-03-18T11:17:43Z

This PR is ready for re-review. What I've fixed since the last review:

Download ffmpeg as well as chromium before packing. Previously I've downloaded all the browsers for all the platform prior to packing, but I've updated it to download browsers per the requested platform. (e.g. when building binary for windows, download only for windows).
Fixed native module download issue, namely sharp which is used by @elastic/synthetics. electron-builder is supposed to handle this but it is incompatible with how sharp's install script works. more details are described here.

_The latest build works well on Mac and Windows. When running it on Linux(Ubuntu 18.04) it had an issue launching the playback(test) but that is out of the scope as it doesn't seem to be related to the build. (I will create another ticket regarding that shortly).

As the binary for Linux doesn't work at the moment, I excluded Linux from the build configs, we will re-enable it once we make it work on Linux._
Update: @emilioalvap's suggestion to pass over parent's process.env to the child process actually fixed the issue. so including linux build, too 👍

cc @justinkambic @vigneshshanmugam

justinkambic · 2022-03-18T13:34:24Z

Fantastic update @kyungeunni and thank you for the advice @emilioalvap ❤️

I will review as soon as possible.

kyungeunni · 2022-03-21T16:03:08Z

Hey @lucasfcosta 👋 @justinkambic mentioned that it would be great if you could test running the build script from your machine and see if it works well just to make sure it is not customed to work only on my machine. When you have a minute, could you try running npm run pack -- -mwl and see all the builds are created under /dist without an error please? 🙏 I'm not sure

justinkambic · 2022-03-21T19:03:13Z

@kyungeunni I'm still seeing an issue on Linux unfortunately 😞

vigneshshanmugam · 2022-03-21T21:30:58Z

Context - .synthetics folder gets created by Synthetics library to put the screenshots and other cached artifacts to be constructed later when the journeys are completed. We need to add permission to be able to create that folder across all OS.

kyungeunni · 2022-03-22T08:45:58Z

Thanks, @vigneshshanmugam !! I'm wondering if it was a permission issue, shouldn't the error code be EACCES: permission denied?

lucasfcosta

Thanks for this @kyungeunni!

My only comments were around env vars and small nits.

Btw, I did build the runner on my Mac and ran the built recorder. I was able to record a multi-step journey and replay it just fine :)

lucasfcosta · 2022-03-22T15:11:42Z

electron/execution.js

@@ -245,6 +245,7 @@ async function onTest(data, browserWindow) {
     */
    synthCliProcess = fork(`${SYNTHETICS_CLI}`, args, {
      env: {
+        ...process.env,


I know we're invoking our own cli here, but I'd usually be a bit concerned with passing the entire env to it because of the secrets it may contain. Can we detect which parts of process.env do we need and pass only those?

Not a big deal, but just thought I'd add the comment for us to discuss.

This is something I'm uncertain about as I'm not accustomed to the security implications of changes to CLI interaction. From speaking offline it seems like this is not a large risk, but I'd like to hear others' opinions.

As @emilioalvap mentioned in another comment, I think there will be various types of env values as well as platform specific ones, so it can be a bit tricky to track all of them. We should measure the effort vs worth by doing so (if we decide to do it).

Another note, when env option is not specified when spawning, Node assigns parent's process.env as a default

lucasfcosta · 2022-03-23T13:49:53Z

scripts/beforePack.js

+      stdio: "inherit",
+      shell: true,
+      env: {
+        ...process.env,


Here too, I'm not sure if this is a big deal, but I'd imagine that if anyone pushes a malicious update to libvips or any other library we use in the fix-sharp command, they could have an environment variable containing our secrets used for signing the app or other credentials.

Maybe I'm being paranoid or there are mechanisms to prevent this already, but I'd appreciate more thoughts from others here.

@lucasfcosta Good point. I'm not sure we would like to follow an allow list approach here, since we are taking into consideration multiple types (X, chromium, playwright, distro, etc...) and user-defined variables.
IMO, it should suffice to filter those variables that are only meant to be used in the recorder itself: user auth, tokens and such.

Yeah, that's a valid point 👍 At the same time, I also thought would we have the same security concerns if we were to run the same script in the main process. Let me know if I'm being so naive!! Anyways It is always to be precautious about it, and I really like what @emilioalvap suggested. We should consider auditing some of the sensitive env vars and filter them out before passing to the child process.

Agreed, a filter out should be enough rather than a filter in just to be sure we're not passing our own signing keys to a malicious package downwards.

Just so I can better understand here, isn't there a whole stack of dependencies we already use, given that we use an OSS toolchain? I guess my question is why are we particularly concerned here? I spoke briefly with @emilioalvap , and he pointed out that NPM is perhaps weaker than a linux distro in terms of supply chain (though probably not for many attacks).

CC @levinebw, your thoughts here?

lucasfcosta · 2022-03-23T13:50:37Z

scripts/beforePack.js

+    npmInstall.on("error", error => {
+      reject(error);
+    });


nit:

Suggested change

npmInstall.on("error", error => {

reject(error);

});

npmInstall.on("error", reject);

Thanks updated!(The less code the better 🧹)

lucasfcosta · 2022-03-23T13:57:06Z

scripts/download-browsers.js

+
+exports.downloadForPlatform = async function downloadForPlatform(platform) {
+  platform = translatePlatform(platform);
+  for (const browser of ["chromium", "ffmpeg"]) {


Nit: should we change browser to software here and in the references within the function like getBrowserVersion given we're not only dealing with browsers here?

That's a good point, will amend 👍

vigneshshanmugam · 2022-03-23T15:41:34Z

Thanks, @vigneshshanmugam !! I'm wondering if it was a permission issue, shouldn't the error code be EACCES: permission denied?

It seems like we are trying to access it for the screenshots, In that case, can you try with screenshots: off when running Synthetics test command. That should make the issue go away.

lucasfcosta · 2022-03-23T16:50:02Z

Thanks, @vigneshshanmugam !! I'm wondering if it was a permission issue, shouldn't the error code be EACCES: permission denied?

It seems like we are trying to access it for the screenshots, In that case, can you try with screenshots: off when running Synthetics test command. That should make the issue go away.

@vigneshshanmugam I think even if we turn it off, the runner would still try to create a folder as specified here though, wouldn't it? Or am I missing something?

vigneshshanmugam · 2022-03-24T20:34:47Z

@vigneshshanmugam I think even if we turn it off, the runner would still try to create a folder as specified here though, wouldn't it? Or am I missing something?

++ As you mentioned there is no way to turn it off, turning off screenshots wont just look for it.

justinkambic

I'm going to comment here and leave a passive LGTM - we should make sure that we don't leave these threads questioning security topics open before merging. I'll reach out to the other reviewers to see if they have any further thoughts on revisions we can make here, or if they're alright to merge as-is.

justinkambic · 2022-03-28T20:21:23Z

electron/execution.js

@@ -245,6 +245,7 @@ async function onTest(data, browserWindow) {
     */
    synthCliProcess = fork(`${SYNTHETICS_CLI}`, args, {
      env: {
+        ...process.env,


This is something I'm uncertain about as I'm not accustomed to the security implications of changes to CLI interaction. From speaking offline it seems like this is not a large risk, but I'd like to hear others' opinions.

justinkambic · 2022-03-28T20:22:54Z

scripts/beforePack.js

+  return Promise.all([downloadForPlatform(platform), fixSharp(arch, platform)]);
+};
+
+const { Arch } = require("electron-builder");


Is there a functional reason that Arch needs to be required after beforePack is initialized?

Oh good catch @justinkambic, I will move it to the top of the file as it should be 👍

lucasfcosta

I'll follow @justinkambic's on this and leave this as a passive LGTM considering that filtering out the environment variables with our keys should be enough to prevent it from being passed downwards to other packages, although I do think we should just consult with security to be sure before merging.

As a note, I've also tested E2E the build for my MacOS and it worked fine for recording, replaying, and generating code for a journey.

andrewvc · 2022-03-29T13:08:32Z

Works fine on windows!

kyungeunni · 2022-03-29T15:37:00Z

Currently, we do not store any secrets in env variables. When we add then, we should remember and filter those secrets out when spawning the child process. We probably need to consider naming convention for it so we don't need to manually add what to filter. For example, we can name sensitive env variables with SECRET_* prefix and we can filter them whenever needed.

vigneshshanmugam

LGTM, Great work @kyungeunni

vigneshshanmugam · 2022-03-29T18:45:50Z

scripts/afterPack.js

+  const platform = ctx.electronPlatformName;
+  if (platform === "linux") {
+    const resourcesPath = path.join(ctx.appOutDir, "resources");
+    changePermission(resourcesPath);


Did you miss the return statement here?

vigneshshanmugam · 2022-03-31T00:21:48Z

scripts/beforePack.js

+exports.default = function beforePack(ctx) {
+  const arch = Arch[ctx.arch];
+  const platform = ctx.electronPlatformName;
+  return Promise.all([downloadForPlatform(platform), fixSharp(arch, platform)]);


I just noted this today, which function would be responsible for handling the error when one of the promises break? Does electron builder takes care of this?

The behavior I observed is that when one of the promises breaks, the electron-builder would throw an error and abort the building process, which is what I expect it to do so. (downloading a browser and sharp lib for each platform is a prerequisite prior to bundling)

If the error handling is taken care of the electron-builder, then all good 👍🏽 I just wanted us to avoid UnhandledPromiseRejection errors.

yeah it's handled by electron-builder and looks like this 👍 thanks for double checking it with me!!

…into 150-poc-browser-per-platform

lucasfcosta · 2022-04-06T13:10:36Z

Just adding an approval here considering the conversation we've just had with someone representing the security team (meaning we won't do any signing at this stage and thus not pass credentials down through env).

…-in-the-ci * upstream/main: Disable record journey button when URL input is empty. (elastic#218) Bump EUI version. (elastic#219) fix: use action.url instead of frameUrl in header (elastic#212) fix: Reset default values when action/assertion edit view closes (elastic#208) Fix Header layout when paused. (elastic#206) Make Test button disabled during recording/paused states. (elastic#207) chore: Add update to `README.md` to explain undefined reference error (elastic#198) chore: update .prettierrc to inherit Kibana's config (elastic#197) 0.0.1-alpha.3 Revert release version in `package.json` to reflect current version. feat: Step divider UI (elastic#175) chore: Remove unused exports (elastic#189) chore: Download browsers for all platforms to release builds from single machine (elastic#165) fix: wrap control button with tooltip when necessary only (elastic#184) fix: URL input expands for longer value (elastic#178) ci: release automation for unsigned artifacts (stage 1) (elastic#183) feat: switch `Steps` data structure from nested array to map (elastic#171) fix: reference the correct pw fork (elastic#186)

kyungeunni requested review from justinkambic and vigneshshanmugam February 24, 2022 15:33

kyungeunni changed the title ~~chore: Download browsers for all platforms to release from single machine~~ chore: Download browsers for all platforms to release builds from single machine Feb 24, 2022

vigneshshanmugam reviewed Feb 24, 2022

View reviewed changes

kyungeunni force-pushed the 150-poc-browser-per-platform branch 3 times, most recently from c33b1d4 to 2eed943 Compare March 18, 2022 11:00

kyungeunni force-pushed the 150-poc-browser-per-platform branch from 0ed182f to 163c887 Compare March 18, 2022 14:06

kyungeunni requested a review from lucasfcosta March 21, 2022 15:57

lucasfcosta reviewed Mar 23, 2022

View reviewed changes

justinkambic reviewed Mar 28, 2022

View reviewed changes

kyungeunni force-pushed the 150-poc-browser-per-platform branch from abee6f0 to 77c005d Compare March 29, 2022 07:26

lucasfcosta approved these changes Mar 29, 2022

View reviewed changes

vigneshshanmugam approved these changes Mar 29, 2022

View reviewed changes

kyungeunni added 3 commits March 30, 2022 18:32

chore: move electron-builder config to yml file

4ed63a0

poc: configure electron-builder to include specific browser

6b368d5

feat: add script downloading all browsers

a8a1be1

kyungeunni added 13 commits March 30, 2022 18:32

fix: bring back build script

ad5632e

fix: add additional config for linux

9e90618

fix: extra install steps for sharp(native dep)

5b43fad

fix: translate platform for downloading browser

c058e3b

fix: download ffmpeg browsers along with chromium

4a574b2

chore: refactor scripts

5098cf0

fix: assign parent process.env when spawning

0b3a3ac

chore: add option for linux build when publishing

a47e0d2

0.0.1-alpha.4

e28a41b

fix: chmod resources dir when building for linux

797c9c2

chore: address feedback

bfda76c

chore: remove Appimage from linux target

c4de212

fix: move require statement to the top

63c4ae9

kyungeunni force-pushed the 150-poc-browser-per-platform branch from 77c005d to 63c4ae9 Compare March 30, 2022 16:32

vigneshshanmugam reviewed Mar 31, 2022

View reviewed changes

kyungeunni and others added 3 commits March 31, 2022 18:15

chore: remove ffmpeg installer

37ee83a

fix: specify correct directory for browser download

a3c07fc

Merge branch 'main' of https://github.com/elastic/synthetics-recorder …

2deebb9

…into 150-poc-browser-per-platform

chore: remove npm-run-all dep

a6187ff

kyungeunni merged commit 27cf388 into elastic:main Apr 6, 2022

kyungeunni deleted the 150-poc-browser-per-platform branch April 6, 2022 14:28

kyungeunni mentioned this pull request May 16, 2022

ci: move release process to the internal system (sign linux and macos) #238

Merged

2 tasks

shahzad31 mentioned this pull request May 30, 2022

fix protocol build config #260

Closed

		@@ -0,0 +1 @@
		BROWSER_REVISION=907428

chore: Download browsers for all platforms to release builds from single machine #165

chore: Download browsers for all platforms to release builds from single machine #165

Conversation

kyungeunni commented Feb 24, 2022 • edited Loading

Summary

Implementation details

How to validate this change

Next Steps

apmmachine commented Feb 24, 2022 • edited Loading

💚 Build Succeeded

Build stats

🤖 GitHub comments

justinkambic commented Feb 24, 2022 • edited Loading

vigneshshanmugam 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

kyungeunni commented Mar 18, 2022 • edited Loading

justinkambic commented Mar 18, 2022

kyungeunni commented Mar 21, 2022

justinkambic commented Mar 21, 2022

vigneshshanmugam commented Mar 21, 2022

kyungeunni commented Mar 22, 2022

lucasfcosta 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

kyungeunni Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigneshshanmugam commented Mar 23, 2022

lucasfcosta commented Mar 23, 2022

vigneshshanmugam commented Mar 24, 2022

justinkambic 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

lucasfcosta left a comment

Choose a reason for hiding this comment

andrewvc commented Mar 29, 2022

kyungeunni commented Mar 29, 2022

vigneshshanmugam 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

lucasfcosta commented Apr 6, 2022

kyungeunni commented Feb 24, 2022 •

edited

Loading

apmmachine commented Feb 24, 2022 •

edited

Loading

justinkambic commented Feb 24, 2022 •

edited

Loading

kyungeunni commented Mar 18, 2022 •

edited

Loading

kyungeunni Mar 24, 2022 •

edited

Loading