Add garden to root execution allow list for Concourse CI containers
#6849
Conversation
- Concourse CI uses `garden` under the hood for its containers. - Running the Homebrew Docker container for a Concourse job was failing with the "can't run as root" error. In the same way as bc320ad, this adds `garden` as one of the allowed providers.
| # Allow Azure Pipelines/Docker/Kubernetes to do everything as root (as it's normal there) | ||
| [[ -f /proc/1/cgroup ]] && grep -E "azpl_job|docker|kubepods" -q /proc/1/cgroup && return | ||
| # Allow Azure Pipelines/Docker/Concourse/Kubernetes to do everything as root (as it's normal there) | ||
| [[ -f /proc/1/cgroup ]] && grep -E "azpl_job|docker|garden|kubepods" -q /proc/1/cgroup && return |
There was a problem hiding this comment.
Maybe it's worth just removing the grep entirely assuming the /proc/1/cgroup doesn't exist on a non-container machine?
There was a problem hiding this comment.
(also: what is "Concourse"?)
There was a problem hiding this comment.
It's an "open-source continuous thing-doer": https://concourse-ci.org/
There was a problem hiding this comment.
Maybe. That'd be better than adding every CI provider people ever want to use. But was there a reason for adding the grep in the first place with only a few values?
There was a problem hiding this comment.
I don't know what's in /proc/1/cgroup on a typical, non-container Linux machine. Can you fill me in? The initial intent was just to stop this being a "all Linux machines" whitelist but we would like it to be an "all containers" whitelist.
There was a problem hiding this comment.
Looks like we do still need the grep, so this change will have to stay:
issyl0@vulpecula:~$ cat /proc/1/cgroup
12:cpuset:/
11:cpu,cpuacct:/
10:hugetlb:/
9:perf_event:/
8:pids:/
7:devices:/
6:net_cls,net_prio:/
5:rdma:/
4:memory:/
3:blkio:/
2:freezer:/
1:name=systemd:/init.scope
0::/init.scope
There was a problem hiding this comment.
I wonder if there's any better more generic version of detecting containers than this? @Homebrew/linux: thoughts?
There was a problem hiding this comment.
Wouldn't simple check for /.dockerenv file existence be sufficient? It seems to be present in every Docker container.
There was a problem hiding this comment.
I don't know myself of any easier way to detect if we're running in any container than simply extending this white list. There's also so many container environments, so I'm personally okay with continuing to extend this white list.
There was a problem hiding this comment.
Thanks. All my Googling turned up the same as we're already doing, too. Let's carry on like this.
|
Thanks again @issyl0! |
Have you written new tests for your changes? Here's an example.Have you successfully runbrew stylewith your changes locally?Have you successfully runbrew testswith your changes locally?gardenunder the hood for its containers.gardenas one of the allowed providers.