linux: reject sysctl kernel.domainname when OCI knob domainname is set
#1017
Conversation
|
Previous discussion here: opencontainers/runtime-spec#1156 (comment) |
|
@giuseppe @AkihiroSuda PTAL, I think we should block |
|
@kolyshkin WDYT? |
|
It is still a valid sysctl value and not rejected in the Runtime spec, so no need to reject it in crun/runc IMHO |
|
@AkihiroSuda Ah while writing/running tests it looked like, But if we decide to allow this, should we specify which one will take priority is it sysctl or the field Edit: there is also this line in validator https://github.com/opencontainers/runc/blob/main/libcontainer/configs/validate/validator.go#L203 |
|
I think we should reject the sysctl only when the Otherwise, we risk breaking older users that were using the sysctl directly. |
flouthoc
force-pushed
the
reject-sysctl-domainname
branch
from
6894b64 to
fc46676
Compare
flouthoc
changed the title
kernel.domainname in favor of OCI knob domainnamekernel.domainname when OCI knob domainname is set
flouthoc
force-pushed
the
reject-sysctl-domainname
branch
from
fc46676 to
ab895b8
Compare
src/libcrun/linux.c
Outdated
| // conflict with already set field `domainname` in | ||
| // OCI spec, in such scenario crun will fail to prevent | ||
| // unexpected behaviour for end user. | ||
| return crun_make_error (err, 0, "the sysctl `%s` conflicts with OCI field `domainname`", original_value); |
There was a problem hiding this comment.
Probably no need to raise an error if strcmp(name, def->domainname) == 0
There was a problem hiding this comment.
@AkihiroSuda I think you meant strcmp(original_value, def->domainname) , since name is the key of sysctl which is kernel.domainname.
I have added this condition and also a test for this.
flouthoc
force-pushed
the
reject-sysctl-domainname
branch
2 times, most recently
from
ee34c11 to
2c73b88
Compare
Setting sysctl `kernel.domainname` directly by user is not environment agnostic, it shows either incorrect ( on non-working ) behaviour in `rootless` environment. It was decided to make this part of `runtime-spec` so the OCI runtime can itself handle this behaviour correctly. As a result a new field `domainname` was added to `runtime-spec`. Since crun already implementes this field therefore `sysctl` configured by user conflicts with the behaviour expected by the OCI runtime. Runtime-spec PR: opencontainers/runtime-spec#1156 Furthermore a similar `sysctl` `kernal.hostname` is blocked by crun explicitly to prevent this conflicting behaviour. https://github.com/containers/crun/blob/main/src/libcrun/linux.c#L3203 Following commit ensures that crun rejects sysctl `kernel.domainname` when OCI field `domainname` is already set. Signed-off-by: Aditya R <arajan@redhat.com>
flouthoc
force-pushed
the
reject-sysctl-domainname
branch
from
2c73b88 to
391df45
Compare
flouthoc
requested review from
AkihiroSuda and
giuseppe
and removed request for
AkihiroSuda
|
Runc issue for similar PR: opencontainers/runc#3629 |
Setting sysctl
kernel.domainnamedirectly by user is not environment agnostic, it shows either incorrect ( on non-working ) behaviour inrootlessenvironment.It was decided to make this part of
runtime-specso the OCI runtime can itself handle this behaviour correctly. As a result a new fielddomainnamewas added toruntime-spec. Since crun already implementes this field thereforesysctlconfigured by user conflicts with the behaviour expected by the OCI runtime.Runtime-spec PR: opencontainers/runtime-spec#1156
Furthermore a similar
sysctlkernal.hostnameis blocked by crun explicitly to prevent this conflicting behaviour. https://github.com/containers/crun/blob/main/src/libcrun/linux.c#L3203