Skip to content
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

Add no-sandbox arg to browser enabled Docker image #3340

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

ka3de
Copy link
Contributor

@ka3de ka3de commented Sep 19, 2023

What?

Sets the no-sandbox chrome argument by default through the K6_BROWSER_ARGS ENV variable in the Dockerfile definition for the browser enabled docker image.

Why?

In order to allow chrome to execute in the browser enabled image, either SYS_ADMIN Docker capability or no-sandbox browser argument had to be set. This sets the no-sandbox option in the Dockerfile definition itself so this action is no longer necessary to run the Docker container. This is arguably also the better option, as we are running the chrome browser inside a container, versus using the Docker capability.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

In order to allow chrome to execute in the browser enabled image, either
SYS_ADMIN Docker capability or no-sandbox browser argument had to be
set. This commit sets the 'no-sandbox' option in the Dockerfile
definition itself so this action is no longer necessary to run the
Docker container. This is arguably also the better option, as we are
running the chrome browser inside a container, versus using the Docker
capability.
@ka3de ka3de self-assigned this Sep 19, 2023
@codebien codebien requested review from a team and olegbespalov and removed request for a team September 19, 2023 14:09
Copy link
Contributor

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Makes things simpler for users. LGTM

Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍 Documentation updates await 😅 I can help.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with this arg, but the change looks good 👍

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (81005d8) 73.17% compared to head (2357c3c) 73.17%.
Report is 1 commits behind head on master.

❗ Current head 2357c3c differs from pull request most recent head 55eba3a. Consider uploading reports for the commit 55eba3a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3340   +/-   ##
=======================================
  Coverage   73.17%   73.17%           
=======================================
  Files         258      258           
  Lines       19887    19886    -1     
=======================================
  Hits        14552    14552           
+ Misses       4414     4413    -1     
  Partials      921      921           
Flag Coverage Δ
ubuntu 73.12% <100.00%> (+<0.01%) ⬆️
windows 73.01% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
converter/har/converter.go 61.02% <100.00%> (-0.16%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codebien
Copy link
Contributor

It seems to be not really safe and we should research for alternatives. https://www.google.com/googlebooks/chrome/med_26.html

We should open a dedicated issue for researching alternatives. Here is an example: https://github.com/Zenika/alpine-chrome#-the-best-with-seccomp

I'm fine with it if we make it temporary, we document it and we are clear on the risks.

@ka3de
Copy link
Contributor Author

ka3de commented Sep 19, 2023

It seems to be not really safe and we should research for alternatives. https://www.google.com/googlebooks/chrome/med_26.html

We should open a dedicated issue for researching alternatives. Here is an example: https://github.com/Zenika/alpine-chrome#-the-best-with-seccomp

We are aware of this, but considering that in our case the browser runs in a container, the no-sandbox usage was more reasonable, even if it's not as running it in a virtual machine. We use a SECCOMP in the cloud, but that is not much user friendly either.

But I agree, if we stick to the no-sandbox option, we should document it as a note/warning in the docs so users are aware that it's set by default.

@codebien codebien merged commit 22fd22a into master Sep 19, 2023
22 checks passed
@codebien codebien deleted the update/dockerfile-with-browser branch September 19, 2023 15:13
@codebien codebien added this to the v0.47.0 milestone Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants