rootless: cgroup: treat EROFS as a skippable error #1759
Conversation
| "github.com/opencontainers/runc/libcontainer/cgroups" | ||
| "github.com/opencontainers/runc/libcontainer/configs" | ||
| libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils" | ||
| "github.com/pkg/errors" |
|
thanks! I can confirm that with the combination of
that runc with rootless containers works again in my new environment (which happens to be GKE). |
In some cases, /sys/fs/cgroups is mounted read-only. In rootless containers we can consider this effectively identical to having cgroups that we don't have write permission to -- because the user isn't responsible for the read-only setup and cannot modify it. The rules are identical to when /sys/fs/cgroups is not writable by the unprivileged user. An example of this is the default configuration of Docker, where cgroups are mounted as read-only as a preventative security measure. Reported-by: Vladimir Rutsky <rutsky@google.com> Signed-off-by: Aleksa Sarai <asarai@suse.de>
Previously if oomScoreAdj was not set in config.json we would implicitly set oom_score_adj to 0. This is not allowed according to the spec: > If oomScoreAdj is not set, the runtime MUST NOT change the value of > oom_score_adj. Change this so that we do not modify oom_score_adj if oomScoreAdj is not present in the configuration. While this modifies our internal configuration types, the on-disk format is still compatible. Signed-off-by: Aleksa Sarai <asarai@suse.de>
|
Fixed the build issue. /cc @opencontainers/runc-maintainers |
| ) | ||
|
|
||
| var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist") | ||
| var errSubsystemDoesNotExist = fmt.Errorf("cgroup: subsystem does not exist") |
There was a problem hiding this comment.
We have a github.com/pkg/errors import, so we can't import errors (without renaming and that's just ugly). I could do errors.Errorf but then you'd get a stack trace in the package which isn't really useful. fmt.Errorf is otherwise essentially the same as errors.New.
There was a problem hiding this comment.
Why not use https://godoc.org/github.com/pkg/errors#New
There was a problem hiding this comment.
New also records the stack trace at the point it was called.
Not a huge deal, but I don't like adding the stack trace for the package-level var.
AkihiroSuda
left a comment
There was a problem hiding this comment.
LGTM (IANAM)
Hope this will get merged soon 🙏
|
/cc @opencontainers/runc-maintainers |
| // sense of the word). This includes EROFS (which for an unprivileged user is | ||
| // basically a permission error) and EACCES (for similar reasons) as well as | ||
| // the normal EPERM. | ||
| func isIgnorableError(err error) bool { |
There was a problem hiding this comment.
Does this only make sense for rootless container? If so, can we add rootless check in it?
There was a problem hiding this comment.
The rootless check is already done at the only caller. We could pass isRootless as a bool if you think it'll look cleaner, but we can't just do an os.Geteuid() here because (in theory) you can run as rootless in other contexts too.
But yes, the EROFS case only makes sense for rootless (this was the main change when I carried this PR).
There was a problem hiding this comment.
We could pass isRootless as a bool if you think it'll look cleaner
That'll be better, my concern is that people may use this function in other circumstances and that's actually not what they expected.
There was a problem hiding this comment.
Oh, I forgot to fix this. I'll send a follow-up.
|
Any chance to get this merged? |
|
LGTM |
|
ping @opencontainers/runc-maintainers |
|
LGTM |
Caused by: * opencontainers#1688 0e56164 * opencontainers#1759 dd67ab1 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Caused by: * opencontainers#1688 0e56164 * opencontainers#1759 dd67ab1 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
In some cases, /sys/fs/cgroups is mounted read-only. In rootless
containers we can consider this effectively identical to having cgroups
that we don't have write permission to -- because the user isn't
responsible for the read-only setup and cannot modify it. The rules are
identical to when /sys/fs/cgroups is not writable by the unprivileged
user.
An example of this is the default configuration of Docker, where cgroups
are mounted as read-only as a preventative security measure.
Closes #1657
Reported-by: Vladimir Rutsky rutsky@google.com
Signed-off-by: Aleksa Sarai asarai@suse.de
I've included a change related to
oom_score_adjhere as well, because@jonboulle mentioned that he's been having permission issues recently and I
discovered that we haven't been following the spec. So I fixed that too.
/cc @rutsky