-
Notifications
You must be signed in to change notification settings - Fork 573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[universal] - Issue universal config change for non-root default codespace user and installing google chrome browser reuse sandboc to run puppeteer cli in universal image #1287
base: main
Are you sure you want to change the base?
Conversation
Hi @ddoyle2017 , Tested another workaround for this instead of forcing the UID 1001 as that was increasing the size of the image considerably.
Would you kindly review the same. Please let me know in case of any concern. With Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Can we revert the puppeteer --no-sandbox
changes and rerun the Actions?
Hi @ddoyle2017 , I have tested one fix for the puppeteer cli issue. Uploaded the fix. Would you kindly review the same & let me know in case of any concern. PFB the details:
With Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a reasonable work around for the Puppeteer issues with Ubuntu 24.04! Can you add a comment above both your changes to explain what issue they're solving?
@@ -103,7 +103,8 @@ | |||
], | |||
"remoteUser": "codespace", | |||
"containerUser": "codespace", | |||
|
|||
"updateRemoteUserUID": false, | |||
"postStartCommand": "sudo chown -R 1000:1000 /workspaces/images/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I know why this need to be a postStartCommand
?
postStartCommand
re runs on every time container is restarted. We need to do this work only once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eljog ,
Indeed it would be executed during every restart. Initially I saw that as part of configurations in one of the features of universal image, git-lfs has already got postCreateCommand used in devcontainer-feature.json. So I avoided the postCreateCommand option & opted for postStartCommand.
However, I have checked now that for a devcontainer configuration if the same lifecycle hook such as postCreateCommand is used in both devcontainer-feature.json & devcontainer.json, then the commands provided in the devcontainer-feature.json would be executed first followed by the commands in devcontainer.json. So I changed that & tested the same now. Kindly let me know in case of any further concerns on this.
With Regards,
Kaniska
src/universal/test-project/test.sh
Outdated
|
||
# export CHROME_DEVEL_SANDBOX env variable | ||
export CHROME_DEVEL_SANDBOX=/usr/local/sbin/chrome-devel-sandbox | ||
cd /workspaces/images/src/universal/test-project/ | ||
yarn | ||
check "run-puppeteer" node puppeteer.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this command work on the container when the user runs it?
Or is it passing here because the additional setup done above, before running the check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eljog ,
My idea was that the main objective behind this particular test "run-puppeteer" was to demonstrate that puppeteer cli library can be used in a node app & the same cane be executed in the universal image. So I changed it in the test.sh alone considering that & also due to the fact that as part of the solution I am downloading & installing google chrome in the image which increases the image size if done as part of the build.
Now I have changed that. Following points are needed to be considered for this change.
- Made change in Dockerfile to install three more libraries which are required to install chrome.
- Made change in install.sh of the local feature setup-user to install chrome & placing the sandbox in the desired location. Please let me know if I should introduce a new local feature for this instead.
- Made change in devcontainer.json to configure "remoteEnv" tag for the CHROME_DEVEL_SANDBOX variable with the sandbox location which is used by the puppeteer library while launching the browser.
- Size of the image has now increased considerably due to the chrome installation which is still within the configured threshold but only by about 64 MB. With the previous change the gap was about 422 MB.
Kindly let me know in case of any concerns on this.
With Regards,
Kaniska Sengupta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is more about the user experience here.
If we don't install chrome on the image, will the puppeteer scenarios start breaking for them from the new version?
Are the users expected to manually install the setup each time, to get things working? Not sure if that's the expectation on using universal images.
Tagging other maintainers for their thoughts. @chrmarti @bamurtaugh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is more about the user experience here. If we don't install chrome on the image, will the puppeteer scenarios start breaking for them from the new version? Are the users expected to manually install the setup each time, to get things working? Not sure if that's the expectation on using universal images.
Tagging other maintainers for their thoughts. @chrmarti @bamurtaugh
Hi @eljog ,
Actually right now in the image built on top of ubuntu-24.04 runner image in CI, no sandbox is available to reuse for puppeteer node app which forces us to install chrome as it comes with sandbox & also due to enhanced security feature of ubuntu-24.04, its not possible to create sandbox without enabling user namespace cloning which still has some vulnerabilities.
Ref:- https://github.com/puppeteer/puppeteer/blob/88ef09c6ccc27d024972725c728a9db50f010f36/docs/troubleshooting.md#setting-up-chrome-linux-sandbox
puppeteer/puppeteer#12818 (comment)
With Regards,
Kaniska
hey @Kaniska244, so @eljog and I had a discussion about how to move forward with the release and I think we need to reorganize your changes for Puppeteer. Currently, your Chrome workaround works for our CI test, but Puppeteer will still be broken for users of this image. We should apply your updates to the image as a whole (not just in |
…al image and setting postCreateCommand instead of postStartCommand.
Hi @ddoyle2017 , I have made the changes according to the comments & all tests are fine with this. Would you kindly review the latest change.
With Regards, |
…es/images directory doesn't exist.
Ref: actions/runner-images#10936
Dev container name:
Universal
Description:
This PR change is made to force 1001 as UID for the non-root default codespace user in the universal image due changes in GitHub runner for larger runners to force 1001 as UID for the runner user.
Changelog:
Checklist: