add support for podman create/run --domainname #15200

cdoern · 2022-08-04T20:06:50Z

domainname functions similarly to --hostname, it sets the value in /etc/hosts
to whatever you speficy in conjunction with --hostname. so if you pass
--hostname=foo --domainmame=baz.net, /etc/hosts will get the combined entry and /proc/sys/kernel/domainname will get baz.net via a sysctl.

resolves #15102

Signed-off-by: Charlie Doern [email protected]

Does this PR introduce a user-facing change?

introduce --domanname to podman create/run which allows users to set a domainname along with a hostname in their host files

Luap99

I took a quick look at docker and this doe snot match it.
The domain name is added in /etc/hosts and setdomainname()? Looks like they jusy use a sysctl for that: https://github.com/moby/moby/blob/master/daemon/oci_utils.go#L8

Also you are ignoring the compat API value and it must be added to podman container inspect.

cmd/podman/containers/create.go

pkg/specgen/generate/namespaces.go

giuseppe · 2022-08-05T21:29:29Z

I think the problem is with the /proc/sys/kernel/domainname file that is always owned by root in the initial user namespace, so writing to it always fails from a rootless user namespace, even if the value is namespaced.

I am not sure how the patch to crun can address it, have you verified it really writes to /proc/sys/kernel/domainname and that you can read the value back from /proc/sys/kernel/domainname within the container?

The correct fix IMO is to address it in the OCI specs and make domainname explicit as the hostname is.

The only possible workaround I see is to convert a write to kernel.domainname to a setdomainname(2), something like:

diff --git a/src/libcrun/linux.c b/src/libcrun/linux.c
index 5a7531e..4cd39e2 100644
--- a/src/libcrun/linux.c
+++ b/src/libcrun/linux.c
@@ -3255,6 +3255,14 @@ libcrun_set_sysctl (libcrun_container_t *container, libcrun_error_t *err)
       if (UNLIKELY (ret < 0))
         return ret;
 
+      if (strcmp (name, "kernel/domainname") == 0)
+        {
+          ret = setdomainname (def->linux->sysctl->values[i], strlen (def->linux->sysctl->values[i]));
+          if (UNLIKELY (ret < 0))
+            return crun_make_error (err, errno, "setdomainname");
+          continue;
+        }
+

I am fine to such a workaround if it unblocks you, but I still suggest to 1) verify your patch really does what it is supposed to do 2) propose a fix for the OCI runtime specs.

Also no need to block with rootless if it is not currently supported. Just skip the test.

cdoern · 2022-08-05T21:37:39Z

I agree with the OCI spec approach. I've been messing around with the crun hack all day and it doesn't seem to want to cooperate. I'll make a PR for the runtime spec on Monday.

rhatdan · 2022-08-06T11:26:08Z

Does this work with Docker? Perhaps only because docker daemon runs as root and running docker in rootless mode this would fail also?

mheon · 2022-08-06T16:48:31Z

Do we want to add the flag as root-only for now, and enable rootless support once the OCI spec PR merges and support actually lands in crun/runc?

cdoern · 2022-08-06T16:55:01Z

Do we want to add the flag as root-only for now, and enable rootless support once the OCI spec PR merges and support actually lands in crun/runc?

That sgtm, I haven't pushed the new version with the sysctl. I'll finish that up on Monday then take care of the spec PR

docs/source/markdown/options/domainname.md

pkg/specgen/generate/namespaces.go

docs/source/markdown/podman-create.1.md.in

cdoern · 2022-09-09T18:32:23Z

/hold

cdoern · 2022-10-11T17:17:02Z

@giuseppe @Luap99 @rhatdan PTAL, once my PR in runtime-tools gets merged in I think this is the proper structure.

cdoern · 2022-12-19T14:30:48Z

@giuseppe @rhatdan PTAL

docs/source/markdown/options/domainname.md

giuseppe · 2022-12-20T08:41:07Z

test/e2e/run_dns_test.go

@@ -99,6 +99,19 @@ var _ = Describe("Podman run dns", func() {
 		Expect(session.OutputToString()).To(ContainSubstring("foobar"))
 	})

+	It("podman run domainname sets /etc/hosts and /proc/sys/kernel/domainname", func() {
+		Skip("Waiting for crun release > 1.6")


crun 1.6 was released some time ago. Where is the test failing?

ah, let me un-comment that and see if this all works...

cdoern · 2022-12-20T18:07:33Z

@giuseppe it seems /proc/sys/kernel/domainname does not get set but /etc/hosts does. Does this mean the domainname is working or not? not sure if the /proc file is meant to be set

giuseppe · 2022-12-21T18:38:35Z

@giuseppe it seems /proc/sys/kernel/domainname does not get set but /etc/hosts does. Does this mean the domainname is working or not? not sure if the /proc file is meant to be set

it sounds like working from Podman but it doesn't look like working from the OCI runtime. To what test are you referring?

cdoern · 2022-12-21T20:00:07Z

@giuseppe it seems /proc/sys/kernel/domainname does not get set but /etc/hosts does. Does this mean the domainname is working or not? not sure if the /proc file is meant to be set

it sounds like working from Podman but it doesn't look like working from the OCI runtime. To what test are you referring?

@giuseppe I was checking /proc/sys/kernel/domainname in a test but I am not sure if that file is meant to contain the --domainname. running commands like hostname -A work appropriately though... so i am not sure if crun is doing it right or not.

rhatdan · 2022-12-21T20:46:47Z

# domainname dan
sh-5.2# domainname
dan
sh-5.2# cat /proc/sys/kernel/domainname 
dan

If I run a container I will also see domainname dan within it, which seems wrong, but I can change it

sh-5.2# podman run -ti --privileged fedora sh
# dnf -y install /usr/bin/domainname
...
Installed:
  hostname-3.23-7.fc37.x86_64                                                                                                                                                        

Complete!
# domainname
dan
# domainname walsh
# domainname
walsh
# exit
sh-5.2# domainname
dan

rhatdan · 2022-12-21T20:47:28Z

I would expect the /proc/sys/kernel/domainname to show the containers domainname, if this is set.

cdoern · 2022-12-22T02:11:33Z

I would expect the /proc/sys/kernel/domainname to show the containers domainname, if this is set.

ok, then I think this is an issue with crun. Since /etc/hosts seems to be updated and the spec does indeed have the proper domainname entity set, something must be wrong..

though, in crun all of the domainname tests pass. @giuseppe WDYT?

giuseppe · 2022-12-22T08:44:47Z

is the correct crun installed?

If I pull locally your changes (testing commit c91508b9c18a3f193c7ab28beaaad85573747e3d), I see it works:

$ sudo bin/podman run --rm --domainname as-root fedora cat /proc/sys/kernel/domainname
as-root
$ bin/podman run --rm --domainname and-also-rootless fedora cat /proc/sys/kernel/domainname
and-also-rootless

Instead of installing an additional package, let's change the test to print directly /proc/sys/kernel/domainname

cdoern · 2022-12-22T18:29:04Z

@giuseppe could ci have an old crun version?

giuseppe · 2022-12-22T18:48:12Z

yes I think that is the issue. It is using crun-1.6-2.fc36-x86_64 while the support was added in crun 1.7.

Let's not block on the CI, just skip the test for now

domainname functions similarly to --hostname, it sets the value in /etc/hosts to whatever you speficy in conjunction with --hostname. so if you pass --hostname=foo --domainmame=baz.net, /etc/hosts will get the combined entry and /proc/sys/kernel/domainname will get baz.net resolves containers#15102 Signed-off-by: Charlie Doern <[email protected]>

vrothberg · 2023-01-17T10:08:49Z

@cdoern I think rebasing and unskipping the test is possible now. Do you have time to tackle it?

Luap99 · 2023-01-17T13:25:32Z

@cdoern I think rebasing and unskipping the test is possible now. Do you have time to tackle it?

The VM image update is still blocked, so there were no updates since November so we are still on crun 1.6.2 at the moment AFAIK.

openshift-merge-robot · 2023-01-22T22:42:54Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrothberg · 2023-03-28T13:28:34Z

Friendly ping. @cdoern I think we are good to give it another try.

openshift-ci bot added the release-note label Aug 4, 2022

Luap99 requested changes Aug 5, 2022

View reviewed changes

cmd/podman/containers/create.go Outdated Show resolved Hide resolved

pkg/specgen/generate/namespaces.go Outdated Show resolved Hide resolved

cdoern mentioned this pull request Aug 5, 2022

defer creation of sysctl until after all namespaces have been created containers/crun#983

Closed

cdoern force-pushed the domainName branch 4 times, most recently from 3b6d21a to cb3104a Compare August 8, 2022 15:41

rhatdan reviewed Aug 9, 2022

View reviewed changes

docs/source/markdown/options/domainname.md Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2022

cdoern force-pushed the domainName branch from cb3104a to b22404b Compare August 11, 2022 18:35

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2022

cdoern force-pushed the domainName branch from b22404b to eef189e Compare August 11, 2022 19:03

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2022

cdoern force-pushed the domainName branch from eef189e to a61429a Compare August 31, 2022 13:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

giuseppe reviewed Aug 31, 2022

View reviewed changes

pkg/specgen/generate/namespaces.go Outdated Show resolved Hide resolved

docs/source/markdown/podman-create.1.md.in Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2022

cdoern force-pushed the domainName branch from a61429a to e698a27 Compare September 9, 2022 18:33

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 9, 2022

cdoern force-pushed the domainName branch from e698a27 to 1282d38 Compare October 11, 2022 17:13

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2022

cdoern force-pushed the domainName branch from a5b76bf to cd7ff2d Compare December 19, 2022 14:30

giuseppe reviewed Dec 20, 2022

View reviewed changes

cdoern force-pushed the domainName branch from cd7ff2d to 0a760b6 Compare December 20, 2022 17:11

cdoern force-pushed the domainName branch 5 times, most recently from 51427a9 to bac6343 Compare December 21, 2022 16:10

cdoern force-pushed the domainName branch from bac6343 to c91508b Compare December 21, 2022 19:58

cdoern force-pushed the domainName branch from c91508b to 4076d25 Compare December 22, 2022 16:07

cdoern force-pushed the domainName branch from 4076d25 to 93f5b13 Compare December 22, 2022 21:20

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2023

vrothberg closed this May 3, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for podman create/run --domainname #15200

add support for podman create/run --domainname #15200

cdoern commented Aug 4, 2022 •

edited

Loading

Luap99 left a comment

giuseppe commented Aug 5, 2022

cdoern commented Aug 5, 2022

rhatdan commented Aug 6, 2022

mheon commented Aug 6, 2022

cdoern commented Aug 6, 2022

cdoern commented Sep 9, 2022

cdoern commented Oct 11, 2022

cdoern commented Dec 19, 2022

giuseppe Dec 20, 2022

cdoern Dec 20, 2022

cdoern commented Dec 20, 2022

giuseppe commented Dec 21, 2022

cdoern commented Dec 21, 2022

rhatdan commented Dec 21, 2022

rhatdan commented Dec 21, 2022

cdoern commented Dec 22, 2022

giuseppe commented Dec 22, 2022

cdoern commented Dec 22, 2022

giuseppe commented Dec 22, 2022

vrothberg commented Jan 17, 2023

Luap99 commented Jan 17, 2023

openshift-merge-robot commented Jan 22, 2023

vrothberg commented Mar 28, 2023

add support for podman create/run --domainname #15200

add support for podman create/run --domainname #15200

Conversation

cdoern commented Aug 4, 2022 • edited Loading

Does this PR introduce a user-facing change?

Luap99 left a comment

Choose a reason for hiding this comment

giuseppe commented Aug 5, 2022

cdoern commented Aug 5, 2022

rhatdan commented Aug 6, 2022

mheon commented Aug 6, 2022

cdoern commented Aug 6, 2022

cdoern commented Sep 9, 2022

cdoern commented Oct 11, 2022

cdoern commented Dec 19, 2022

giuseppe Dec 20, 2022

Choose a reason for hiding this comment

cdoern Dec 20, 2022

Choose a reason for hiding this comment

cdoern commented Dec 20, 2022

giuseppe commented Dec 21, 2022

cdoern commented Dec 21, 2022

rhatdan commented Dec 21, 2022

rhatdan commented Dec 21, 2022

cdoern commented Dec 22, 2022

giuseppe commented Dec 22, 2022

cdoern commented Dec 22, 2022

giuseppe commented Dec 22, 2022

vrothberg commented Jan 17, 2023

Luap99 commented Jan 17, 2023

openshift-merge-robot commented Jan 22, 2023

vrothberg commented Mar 28, 2023

cdoern commented Aug 4, 2022 •

edited

Loading