*: fill resolv.conf from command line --dns #2040

alban · 2016-01-27T11:38:03Z

rkt now provides /etc/resolv.conf in each app's chroot. All apps in the
pod have the same /etc/resolv.conf.

Getting DNS information from the CNI or from the host's resolv.conf is
out of scope of this patch. The content of /etc/resolv.conf comes from
the command line parameters:

"--dns=8.8.8.8": DNS name server
"--dns-search=domain.com": DNS search domain
"--dns-opt=debug": DNS options

Implementation detail: systemd-nspawn can overwrite /etc/resolv.conf. It
is supposed to overwrite it only when the container uses the host
network namespace. However, the container network namespace is created
by rkt via CNI and not by systemd-nspawn, so systemd-nspawn is not aware
of the network configuration. As a consequence, we cannot use stage1's
/etc/resolv.conf. Instead, rkt write /etc/rkt-resolv.conf in stage1 and
prepare-app bind-mounts it in stage2's /etc/resolv.conf.

It works in this way:

> $ sudo rkt run --net=alban --dns=1.1.1.1 --dns=2.2.2.2 \
>                --dns-search=foo.com --dns-opt=debug \
>                sha512-a2fb8f390702 --interactive
> / # cat /etc/resolv.conf
> # Generated by rkt
>
> search foo.com
> nameserver 1.1.1.1
> nameserver 2.2.2.2
> options debug
>
> / #

Fixes #660

/cc @steveej I can add tests and --dns-search= after lunch. One difference compared to #2032 is that /etc/resolv.conf is not created if the user does not add --dns=. Also, if the ACI does not have a /etc/resolv.conf, it is created automatically.

tests
more options for domain search
changelog
documentation

steveej · 2016-01-27T12:59:57Z

One difference compared to #2032 is that /etc/resolv.conf is not created if the user does not add --dns=. Also, if the ACI does not have a /etc/resolv.conf, it is created automatically.

Please explain/restate this as it is contradicting.

iaguis · 2016-01-27T13:01:06Z

stage0/run.go

@@ -379,6 +380,26 @@ func preparedWithPrivateUsers(dir string) (string, error) {
 	return string(bytes), nil
 }

+func addResolvConf(cfg RunConfig, rootFs string) {


rootFs -> rootfs for consistency

iaguis · 2016-01-27T13:05:07Z

stage1/prepare-app/prepare-app.c

+		if (access(mnt->source, F_OK) != 0)
+			continue;
+		if (access(to, F_OK) != 0) {
+			fd = creat(to, 0644);


Should we handle errors here or you're leaving it to mount?

I don't know what's better. Any preferences?

My preference: as early as possible

Fair enough. Updated.

alban · 2016-01-27T13:08:52Z

Please explain/restate this as it is contradicting

When the user adds a non-empty --dns=, rkt will write /etc/resolv.conf as specified. If /etc/resolv.conf does not exist in the ACI, it will be created by rkt. So it works both on ACI with an empty resolv.conf (example: docker://ubuntu) and ACI without resolv.conf (example: docker://busybox).
When the user does not adds --dns=, rkt will not touch /etc/resolv.conf. If the file exists in the ACI, it will stay as it is. If the file does not exist in the ACI, it will not be created.

steveej · 2016-01-27T13:19:19Z

tests/rkt_dns_test.go

+	"github.com/coreos/rkt/tests/testutils"
+)
+
+// TestDNS is checking how rkt fill /etc/resolv.conf


s/fill/fills

steveej · 2016-01-27T13:42:05Z

stage0/run.go

@@ -379,6 +380,26 @@ func preparedWithPrivateUsers(dir string) (string, error) {
 	return string(bytes), nil
 }

+func addResolvConf(cfg RunConfig, rootfs string) {
+	if len(cfg.DNS) == 0 {


IMHO it'd be better to read if this condition was also or even only used before the invocation of addResolvConf(..)

alban · 2016-01-27T16:02:22Z

rkt/run.go

@@ -328,6 +340,22 @@ func (pl *portList) Type() string {
 	return "portList"
 }

+// flagStringList implements the flag.Value interface to contain a set of strings
+type flagStringList []string


@krnowak where is the preferred place to add this new type?

I don't think we have a good place to put new types used in several, but not all commands. But often rkt.go serves as a dumpster for this kind of stuff. ;)

Ah, this is quite generic, so rkt.go it is.

I believe I wrote a similar thing somewhere before...

It's here - https://github.com/coreos/rkt/blob/master/tools/common/util.go#L24

Maybe we should move this thing elsewhere.

However, rkt/run.go already has some flag types: envMap, portList. Should I still move it to rkt.go which does not have any flag type yet?

Just keep it here then. We can later move this stuff in to a separate go file.

achanda · 2016-01-28T02:48:20Z

stage0/run.go

+	for _, server := range cfg.DNS {
+		if server == "" {
+			continue
+		}


Might be a good idea to check if server is a valid IP address here

rkt now provides /etc/resolv.conf in each app's chroot. All apps in the pod have the same /etc/resolv.conf. Getting DNS information from the CNI or from the host's resolv.conf is out of scope of this patch. The content of /etc/resolv.conf comes from the command line parameters: - "--dns=8.8.8.8": DNS name server - "--dns-search=domain.com": DNS search domain - "--dns-opt=debug": DNS options Implementation detail: systemd-nspawn can overwrite /etc/resolv.conf. It is supposed to overwrite it only when the container uses the host network namespace. However, the container network namespace is created by rkt via CNI and not by systemd-nspawn, so systemd-nspawn is not aware of the network configuration. As a consequence, we cannot use stage1's /etc/resolv.conf. Instead, rkt write /etc/rkt-resolv.conf in stage1 and prepare-app bind-mounts it in stage2's /etc/resolv.conf. It works in this way: > $ sudo rkt run --net=alban --dns=1.1.1.1 --dns=2.2.2.2 \ > --dns-search=foo.com --dns-opt=debug \ > sha512-a2fb8f390702 --interactive > / # cat /etc/resolv.conf > # Generated by rkt > > search foo.com > nameserver 1.1.1.1 > nameserver 2.2.2.2 > options debug > > / # Fixes rkt#660

steveej · 2016-01-28T12:13:33Z

stage1/prepare-app/prepare-app.c

+			continue;
+		if (access(to, F_OK) != 0) {
+			pexit_if((fd = creat(to, 0644)) == -1,
+				"Cannot create file: \"%s\"", to);


Can we get more information in case of an error? Is there an implicit ERRNO parser here?

Yes, the p of pexit_if means it will use strerror(errno) at the end of the error message.

steveej · 2016-01-28T15:23:01Z

tests/rkt_dns_test.go

+		rktCmd := fmt.Sprintf(`%s --insecure-options=image run --set-env=FILE=/etc/resolv.conf %s %s`,
+			ctx.Cmd(), tt.paramDNS, imageFile)
+		t.Logf("%s\n", rktCmd)
+		runRktAndCheckOutput(t, rktCmd, tt.expectedLine, false)


These tests should be improved in the future to match against the whole file instead of lines. That way we can ensure that the order is deterministic which might matter for some DNS implementations.

steveej · 2016-01-28T15:25:28Z

Let's keep https://github.com/coreos/rkt/pull/2040/files#r51135829 in mind for a follow-up issue.

Nice work

steveej · 2016-01-28T15:30:00Z

@achanda thanks for you input on this PR. I'm merging this as is but you are welcome to raise any of your thoughts in an issue where we can discuss this further.

*: fill resolv.conf from command line --dns* flags

alban force-pushed the alban/dns-2 branch from aa71da2 to 764650f Compare January 27, 2016 12:08

iaguis reviewed Jan 27, 2016
View reviewed changes

alban force-pushed the alban/dns-2 branch from 764650f to e743ca0 Compare January 27, 2016 13:02

iaguis reviewed Jan 27, 2016
View reviewed changes

steveej reviewed Jan 27, 2016
View reviewed changes

alban force-pushed the alban/dns-2 branch from e743ca0 to 560e71e Compare January 27, 2016 13:29

steveej reviewed Jan 27, 2016
View reviewed changes

alban force-pushed the alban/dns-2 branch 2 times, most recently from 53d4547 to 5149e3e Compare January 27, 2016 15:47

alban added area/networking needs/review labels Jan 27, 2016

alban added this to the v1.0.0 milestone Jan 27, 2016

alban reviewed Jan 27, 2016
View reviewed changes

alban force-pushed the alban/dns-2 branch 3 times, most recently from 190a00b to 8a42cf5 Compare January 27, 2016 16:40

achanda reviewed Jan 28, 2016
View reviewed changes

alban force-pushed the alban/dns-2 branch from 8a42cf5 to 69210ca Compare January 28, 2016 09:04

iaguis added the priority/P0 label Jan 28, 2016

steveej reviewed Jan 28, 2016
View reviewed changes

alban mentioned this pull request Jan 28, 2016

Set domain name in rkt pods #2042

Open

steveej reviewed Jan 28, 2016
View reviewed changes

steveej added reviewed/lgtm and removed needs/review labels Jan 28, 2016

steveej added a commit that referenced this pull request Jan 28, 2016

Merge pull request #2040 from kinvolk/alban/dns-2

ccc7442

*: fill resolv.conf from command line --dns* flags

steveej merged commit ccc7442 into rkt:master Jan 28, 2016

jonboulle mentioned this pull request Apr 13, 2016

populate /etc/resolv.conf #2032

Closed

7 tasks

jimmycuadra mentioned this pull request May 10, 2016

Automatically inheriting /etc/resolv.conf config from host #2602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fill resolv.conf from command line --dns #2040

*: fill resolv.conf from command line --dns #2040

alban commented Jan 27, 2016

steveej commented Jan 27, 2016

iaguis Jan 27, 2016

alban Jan 27, 2016

iaguis Jan 27, 2016

alban Jan 27, 2016

steveej Jan 27, 2016

alban Jan 27, 2016

alban commented Jan 27, 2016

steveej Jan 27, 2016

alban Jan 27, 2016

steveej Jan 27, 2016

alban Jan 27, 2016

alban Jan 27, 2016

krnowak Jan 27, 2016

krnowak Jan 27, 2016

krnowak Jan 27, 2016

krnowak Jan 27, 2016

alban Jan 27, 2016

krnowak Jan 27, 2016

achanda Jan 28, 2016

alban Jan 28, 2016

steveej Jan 28, 2016

alban Jan 28, 2016

steveej Jan 28, 2016

steveej commented Jan 28, 2016

steveej commented Jan 28, 2016

*: fill resolv.conf from command line --dns #2040

*: fill resolv.conf from command line --dns #2040

Conversation

alban commented Jan 27, 2016

steveej commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej commented Jan 28, 2016

steveej commented Jan 28, 2016