fix: changing owner while creating container for download support

[VietND96]

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

fix: changing owner while creating container for download support

Motivation and Context

• Running with non-root, container could not started with reason

PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'

Command reproduce docker run --user $(id -u):$(id -g) -d selenium/hub:4.16.1-20231212

• Add back the workaround create a host directory and set perm before mounting volumes to README.
• Take note in README that the feature would support in the table matrix.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[diemol]

I know the changes are well intended because we want to enable users to just run containers without the need for workarounds.

However, now we have 3 new scripts and several changes in many Dockerfiles, which increases the maintenance of the images. While these scripts juggle and find the ideal values, the whole thing gets harder to maintain.

I am really happy for all the good work that you are doing, but in this case, after seeing all the overhead to cover this use case, I feel we should go back to just mentioning the workaround in the README.

What do you think?

[VietND96]

@diemol, I understand your concern, always the situation is easy to add but difficult to keep it up-to-date :)
So, I have reverted the last point on startup hooks, just based on previous changes and had some re-organization.
Can you take a look and feedback?

[therealdjryan]

@diemol,
I think it is useful for me to add a comment to this as I have tested the changes in my environment running podman. In my case this fix did not work. Here are some of my comments from a discussion in a thread on the #selenium slack channel

I see your changes and I think I know the problem. The way I solved the permissions issue is not by changing permissions but by running compose with
--userns=keep-id --user=$(id -u):$(id -g)
so by default seluser is uid:1200 gid: 1201, but with those args the uid is not 1200 but the uid of the user running the container. Locally I can reproduce the problem with this:
docker run --user $(id -u):$(id -g) -d selenium/hub:4.16.1-20231212
When using the bash test $(id -u ${SEL_USER}) could you instead, or in addition to use $(id -u -n ${SEL_USER}) which will return ‘seluser’ instead of 1200
OR could you try running pre 4.16.1-20231212 images with the user options I have and see if it solves the problem?

There are more comments. Here is a link to the thread: https://seleniumhq.slack.com/archives/C0ABCS03F/p1702412090176059

[VietND96]

In our CI there are tests to guarantee arbitrary --user {{ randomUID }}
However, when considering the removing 777 in last changes, there is another issue if specify both host uid and gid --user $(id -u):$(id -g) (e.g --user 1003:1004) it breaks

[VietND96]

After trying something, when docker run --user $(id -u):$(id -g) always need to give write access for others (777) for few directories, where the app needs to write data (e.g supervisor logs, $HOME, /opt/selenium/).
So added back 777 to grant permission for those dir. There is an improvement here is it not present static in Dockerfile anymore, it now in fix-permissions only, where the user is able to customize or rewrite via volume mount, ConfigMap. I guess it would not be audited as a security issue.
Updated the sanity test to cover docker run --user $(id -u):$(id -g) to ensure without regression issue, also it is reasonable why 777 is retained. If anyone wants to change or improve, then only make those tests pass on CI.
Revert unnecessary changes, and keep the minimal implementation.
@diemol, @therealdjryan, can you review and feedback once.

[therealdjryan]

@diemol In my opinion this feature is unnecessary.
Permissions are not an issue if the container is started in the right way even without the chmod 777 (I have tested this).

using an alternative download directory is a reasonable feature, but selenium has the ability to mount remote directories so I don't understand the need for creating directories. If the problem is the inability to mount directories on video and browser nodes in dynamic grid environments, the correct fix in my opinion is to add the ability to define remote directories in the config.toml.

I believe that if this feature is to remain then there should be a flag or var to enable it, but it should be disabled by default.

[therealdjryan]

I just tested ndviet/node-docker:latest and ndviet/hub:latest

b574251b9ef7 There is no entry in /etc/passwd for our UID=222. Attempting to fix...
7613ab3ab35e Traceback (most recent call last):
b574251b9ef7 Traceback (most recent call last):
b574251b9ef7   File "/usr/bin/supervisord", line 33, in <module>
7613ab3ab35e   File "/usr/bin/supervisord", line 33, in <module>
b574251b9ef7     sys.exit(load_entry_point('supervisor==4.2.1', 'console_scripts', 'supervisord')())
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 361, in main
7613ab3ab35e     sys.exit(load_entry_point('supervisor==4.2.1', 'console_scripts', 'supervisord')())
b574251b9ef7     go(options)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 361, in main
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 371, in go
7613ab3ab35e     go(options)
b574251b9ef7     d.main()
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 371, in go
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 72, in main
7613ab3ab35e     d.main()
b574251b9ef7     self.options.make_logger()
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 72, in main
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/options.py", line 1470, in make_logger
7613ab3ab35e     self.options.make_logger()
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/options.py", line 1470, in make_logger
b574251b9ef7     loggers.handle_file(
7613ab3ab35e     loggers.handle_file(
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 417, in handle_file
7613ab3ab35e     handler = RotatingFileHandler(filename, 'a', maxbytes, backups)
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 417, in handle_file
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 213, in __init__
7613ab3ab35e     FileHandler.__init__(self, filename, mode)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 160, in __init__
7613ab3ab35e     self.stream = open(filename, mode)
b574251b9ef7     handler = RotatingFileHandler(filename, 'a', maxbytes, backups)
7613ab3ab35e PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'
7613ab3ab35e There is no entry in /etc/passwd for our UID=222. Attempting to fix...
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 213, in __init__
7613ab3ab35e Traceback (most recent call last):
7613ab3ab35e   File "/usr/bin/supervisord", line 33, in <module>
7613ab3ab35e     sys.exit(load_entry_point('supervisor==4.2.1', 'console_scripts', 'supervisord')())
b574251b9ef7     FileHandler.__init__(self, filename, mode)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 361, in main
7613ab3ab35e     go(options)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 371, in go
b574251b9ef7   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 160, in __init__
7613ab3ab35e     d.main()
b574251b9ef7     self.stream = open(filename, mode)
b574251b9ef7 PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 72, in main
7613ab3ab35e     self.options.make_logger()
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/options.py", line 1470, in make_logger
7613ab3ab35e     loggers.handle_file(
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 417, in handle_file
7613ab3ab35e     handler = RotatingFileHandler(filename, 'a', maxbytes, backups)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 213, in __init__
7613ab3ab35e     FileHandler.__init__(self, filename, mode)
7613ab3ab35e   File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 160, in __init__
7613ab3ab35e     self.stream = open(filename, mode)
7613ab3ab35e PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'

[diemol]

That is good feedback, @therealdjryan, thank you.

My thought was to release the feature and get feedback and then decide if we wanted to keep it or not. Probably it is not being as helpful as we thought?

What do you think, @VietND96? Should we leave it as it was before all these changes?

[VietND96]

Hi @therealdjryan, thank you for your feedback. As of now it would be disabled and not touch main process if SE_DOWNLOAD_DIR is not set by user.
Regarding the image tests, I didn't push the new one that contains this PR today. I just pushed a new one, can you retry? Ideally, CI tests reflect the change should work.
@diemol, sure, let me try to isolate the change by a flag, and leave others as it was before.

[therealdjryan]

@VietND96, I'm going on vacation in a couple hours so I may not have a chance to look until the new year.

[VietND96]

@therealdjryan, sure, have a nice vacation. I'm also going to isolate and rollback this feat in next stable release, and will rework it in some time

[therealdjryan]

@VietND96, @diemol
Finally trying 4.17 and it is failing when mounting Downloads. If running with --enable-managed-downloads true why is it necessary to mount this directory?
time="2024-02-02T14:38:20-05:00" level=error msg="runc create failed: unable to start container process: error during container init: error mounting \"/tmp/containers-user-222/storage/volumes/16fefd4c7220939fb76bd6602fe65e3f9f13249bc402203c4f92d93dae662e9e/_data\" to rootfs at \"/home/seluser/Downloads\": mount /proc/self/fd/90:/home/seluser/Downloads (via /proc/self/fd/118), flags: 0x5026: operation not permitted": OCI permission denied

[VietND96]

Hi @therealdjryan,

docker-selenium/Base/Dockerfile

Line 131 in 7b7faff

VOLUME ${SEL_DOWNLOAD_DIR}

The issue looks like due to this VOLUME instruction in Dockerfile. I had misunderstood usage of this instruction, it easier to start containers without remembering the -v flags to apply. That means it always mounts a temporary folder in the host even the user without specifying -v when starting the container.
I am going to remove this VOLUME instruction, and bump nightly image tag for the preview.