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 domainname spec entity #1156

Merged
merged 1 commit into from Aug 25, 2022
Merged

add domainname spec entity #1156

merged 1 commit into from Aug 25, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Aug 7, 2022

add the domainname entity so that container runtimes can add special handling similar to hostname. The current workaround of adding a sysctl for kernel.domainname only works with rootful execution in most cases. This will allow for rootless execution.

container runtimes will be able to add special handling as they do for hostname, using setdomainname to add the entry to /proc/sys/kernel/domainname.

Signed-off-by: Charlie Doern cdoern@redhat.com

@cdoern
Copy link
Contributor Author

cdoern commented Aug 7, 2022

@giuseppe PTAL

Copy link
Contributor

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

I think this should also update the config schema

diff --git a/schema/config-schema.json b/schema/config-schema.json
index a4d1274..cf66c65 100644
--- a/schema/config-schema.json
+++ b/schema/config-schema.json
@@ -35,6 +35,9 @@
         "hostname": {
             "type": "string"
         },
+        "domainname": {
+            "type": "string"
+        },
         "mounts": {
             "type": "array",
             "items": {

@flouthoc
Copy link
Contributor

flouthoc commented Aug 8, 2022

Nit:

This will allow for rootless execution

I think in commit message following must be extended to specify how a container-runtime should utilize this field, two approach i could think are runtime invokes setdomainname( or the second one is write /proc/sys/kernel/domainname before setting up UTS namespace but is it possible in rootless session ? (Requesting maintainers to confirm)

@giuseppe
Copy link
Member

giuseppe commented Aug 8, 2022

as @flouthoc pointed out, please update the config schema as well

@cdoern
Copy link
Contributor Author

cdoern commented Aug 8, 2022

Nit:

This will allow for rootless execution

I think in commit message following must be extended to specify how a container-runtime should utilize this field, two approach i could think are runtime invokes setdomainname( or the second one is write /proc/sys/kernel/domainname before setting up UTS namespace but is it possible in rootless session ? (Requesting maintainers to confirm)

I think using setdomainname makes the most sense. Currently crun uses sethostname for the hostname.

add the domainname entity so that container runtimes can add special handling similar to hostname. The current workaround of adding a sysctl for kernel.domainname only works with rootful execution in most cases. This will allow for rootless execution.

container runtimes will be able to add special handling as they do for hostname, using setdomainname to add the entry to /proc/sys/kernel/domainname.

Signed-off-by: Charlie Doern <cdoern@redhat.com>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda
Copy link
Member

What should happen if the sysctl is also specified but with a different value? Should it fail?

@flouthoc
Copy link
Contributor

flouthoc commented Aug 10, 2022

@AkihiroSuda Afaik In crun i think sysctl is not allowed at all if OCI spec already has a knob to configure it, for instance i can see for kernel/hostname : https://github.com/containers/crun/blob/main/src/libcrun/linux.c#L3198

@AkihiroSuda
Copy link
Member

@opencontainers/runtime-spec-maintainers Can we merge this?

@vbatts vbatts merged commit 86290f6 into opencontainers:main Aug 25, 2022
utam0k added a commit to utam0k/oci-spec-rs that referenced this pull request Aug 28, 2022
utam0k added a commit to utam0k/oci-spec-rs that referenced this pull request Aug 28, 2022
utam0k added a commit to utam0k/oci-spec-rs that referenced this pull request Aug 28, 2022
utam0k added a commit to utam0k/kata-containers that referenced this pull request Sep 5, 2022
opencontainers/runtime-spec#1156
Signed-off-by: utam0k <k0ma@utam0k.jp>
utam0k added a commit to utam0k/runc that referenced this pull request Sep 11, 2022
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
utam0k added a commit to utam0k/kata-containers that referenced this pull request Sep 12, 2022
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
utam0k added a commit to utam0k/runc that referenced this pull request Sep 23, 2022
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>

Implement to set a domain name

Signed-off-by: utam0k <k0ma@utam0k.jp>
@utam0k utam0k mentioned this pull request Sep 25, 2022
2 tasks
flouthoc added a commit to flouthoc/crun that referenced this pull request Oct 1, 2022
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

Signed-off-by: Aditya R <arajan@redhat.com>
flouthoc added a commit to flouthoc/crun that referenced this pull request Oct 3, 2022
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 added a commit to flouthoc/crun that referenced this pull request Oct 3, 2022
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 added a commit to flouthoc/crun that referenced this pull request Oct 3, 2022
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 added a commit to flouthoc/crun that referenced this pull request Oct 3, 2022
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 added a commit to flouthoc/crun that referenced this pull request Oct 3, 2022
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>
utam0k added a commit to utam0k/runc that referenced this pull request Oct 4, 2022
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
This was referenced Jan 24, 2023
@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023
utam0k added a commit to utam0k/runc that referenced this pull request Feb 1, 2023
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
utam0k added a commit to utam0k/runc that referenced this pull request Feb 14, 2023
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
utam0k added a commit to utam0k/runc that referenced this pull request Apr 12, 2023
opencontainers/runtime-spec#1156

Signed-off-by: utam0k <k0ma@utam0k.jp>
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 tasks
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.

None yet

5 participants