Generate and maintain resolv.conf for sandbox #50

Crazykev · 2017-05-25T02:00:41Z

This is implement part of #28, I'll continue to write some related unit test for this.

And we may need to maintain it for hostNetwork pod, let me double check it and rely later.

@Random-Liu I copy a host resolv.conf for each pod with hostNetwork, since user may change this file in container. Let me know if you change your mind.

Signed-off-by: Crazykev crazykev@zju.edu.cn

Random-Liu · 2017-05-25T06:20:31Z

Will take a look tomorrow, and let's try to get it merged this week.

Random-Liu · 2017-05-31T21:45:13Z

pkg/server/sandbox_run.go

-	// TODO(random-liu): [P0] Set DNS options. Maintain a resolv.conf for the sandbox.
+	// Set DNS options.
+	if dnsConfig := config.GetDnsConfig(); dnsConfig != nil {
+		// resolvPath is not "" guaranteed by 'dnsConfig != nil'


We should generate resolve file in RunPodSandbox.
And only AddBindMount in generateSandboxContainerSpec.

I want to make generateSandboxContainerSpec only simply convert CRI config into OCI spec, so that we could reuse it or consolidate it with cri-o in the future.

Random-Liu · 2017-05-31T22:06:08Z

pkg/server/sandbox_run.go

+	if dnsConfig := config.GetDnsConfig(); dnsConfig != nil {
+		// resolvPath is not "" guaranteed by 'dnsConfig != nil'
+		resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, id), config)
+		err := parseDNSOptions(dnsConfig.Servers, dnsConfig.Searches, dnsConfig.Options, resolvPath)


Separate parse logic and write logic, which makes it easier to write unit test, and also makes the code more clear.

Random-Liu · 2017-05-31T22:08:23Z

pkg/server/sandbox_run.go

+		resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, id), config)
+		err := parseDNSOptions(dnsConfig.Servers, dnsConfig.Searches, dnsConfig.Options, resolvPath)
+		if err != nil {
+			err1 := removeFile(resolvPath)


Don't need to add a new function, use RemoveAll which doesn't report error when file doesn't exist.

Random-Liu · 2017-05-31T22:09:03Z

pkg/server/sandbox_run.go

+		resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, id), config)
+		err := parseDNSOptions(dnsConfig.Servers, dnsConfig.Searches, dnsConfig.Options, resolvPath)
+		if err != nil {
+			err1 := removeFile(resolvPath)


Use defer to cleanup file, or else if there is error returned later in this function, you will not cleanup the file.
And another reason we should create the file in RunPodSandbox is that we should cleanup the file whenever there is an error in RunPodSandbox.
In your current implementation, if there is an error create the container, RunPodSandbox will return an error, but the resolve.conf file will not be cleaned up, which causes unnecessary file leakage.

I found that we've already had code cleaning up the sandbox root. https://github.com/kubernetes-incubator/cri-containerd/blob/master/pkg/server/sandbox_run.go#L112

We can rely on that to cleanup.

Random-Liu · 2017-05-31T22:09:27Z

pkg/server/sandbox_run.go

+		resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, id), config)
+		err := parseDNSOptions(dnsConfig.Servers, dnsConfig.Searches, dnsConfig.Options, resolvPath)
+		if err != nil {
+			err1 := removeFile(resolvPath)


Log the error in defer.

Random-Liu · 2017-05-31T22:38:46Z

pkg/server/container_start_test.go

 	testPid := uint32(1234)
 	config, sandboxConfig, specCheck := getStartContainerTestData()
 	c := newTestCRIContainerdService()
-	spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig)
+	spec, err := c.generateContainerSpec(testID, testPodID, testPid, config, sandboxConfig)
 	assert.NoError(t, err)
 	specCheck(t, testID, testPid, spec)


Pass testSandboxID in, and check whether resolv.conf is mounted.

Random-Liu · 2017-05-31T22:50:45Z

pkg/server/sandbox_run.go

@@ -225,7 +228,20 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 	// Set hostname.
 	g.SetHostname(config.GetHostname())

-	// TODO(random-liu): [P0] Set DNS options. Maintain a resolv.conf for the sandbox.
+	// Set DNS options.
+	if dnsConfig := config.GetDnsConfig(); dnsConfig != nil {


Even when dnsConfig == nil, we should use the copy of resolve.conf on the host.
That should be the default behavior.

Yep, this is my original thought, not sure why cri-o guys do this assumption...

Random-Liu · 2017-05-31T22:53:11Z

pkg/server/sandbox_run.go

@@ -225,7 +228,20 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 	// Set hostname.
 	g.SetHostname(config.GetHostname())

-	// TODO(random-liu): [P0] Set DNS options. Maintain a resolv.conf for the sandbox.


We should remove resolve.conf in RemovePodSandbox.

NVM, the whole sandbox root directory will be removed.

Random-Liu · 2017-06-01T01:21:36Z

pkg/server/container_start.go

-	// TODO(random-liu): [P0] Bind mount sandbox resolv.conf.
+	// Bind mount sandbox resolv.conf.
+	if resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, sandboxID), sandboxConfig); resolvPath != "" {
+		g.AddBindMount(resolvPath, resolvConfPath, []string{"ro"})


Let's mount it as "rw"/"ro" based on securityContext.GetReadonlyRootfs() for now (Docker's behavior today), and add TODO to figure out whether we should always mount it as read-only.

Let's avoid regression for now, there may be user relying on this behavior, although it's not encouraged.

Random-Liu · 2017-06-01T01:37:06Z

pkg/server/sandbox_run_test.go

@@ -113,7 +113,8 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 		if test.configChange != nil {
 			test.configChange(config)
 		}
-		spec := c.generateSandboxContainerSpec(testID, config)
+		spec, err := c.generateSandboxContainerSpec(testID, config)


Test resolve.conf bind mount.

Random-Liu · 2017-06-01T01:37:34Z

@Crazykev Please rebase your PR and address comments

Crazykev · 2017-06-06T11:44:05Z

@Random-Liu Addressed comments and added related unit test. PTAL.

Crazykev · 2017-06-06T11:45:43Z

pkg/server/sandbox_run.go

+	resolvPath := getResolvPath(sandboxRootDir)
+	if len(resolvContent) == 0 && hostNetwork {
+		// copy host's resolv.conf to resolvPath
+		// err = c.os.CopyFile(resolvConfPath, resolvPath)


will uncomment this after #60 merged.

Crazykev · 2017-06-07T04:58:48Z

@Random-Liu Rebased.

Random-Liu · 2017-06-07T06:24:40Z

pkg/os/os.go

@@ -75,3 +77,9 @@ func (RealOS) CopyFile(src, dest string, perm os.FileMode) error {
 	_, err = io.Copy(out, in)
 	return err
 }
+
+// WriteFile writes data to a file named by filename. If the file does not exist,


nit: WriteFile will call ioutil.WriteFile to write data into a file.

The details comment of the function is already covered by golang doc.

Random-Liu · 2017-06-07T06:25:26Z

pkg/os/testing/fake_os.go

@@ -142,3 +143,15 @@ func (f *FakeOS) CopyFile(src, dest string, perm os.FileMode) error {
 	}
 	return nil
 }
+
+// WriteFile is a fake call that invokes WriteFile or just return nil.


s/WriteFile/WrteFileFn.

Random-Liu · 2017-06-07T06:28:12Z

pkg/server/container_start.go


 	// TODO(random-liu): [P2] Add apparmor and seccomp.

 	// TODO(random-liu): [P1] Bind mount sandbox /dev/shm.

-	// TODO(random-liu): [P0] Bind mount sandbox resolv.conf.
+	// Bind mount sandbox resolv.conf.


Could you add this logic into generateContainerMounts?
And remove the corresponding TODO in generateContainerMounts.

Random-Liu · 2017-06-07T06:28:59Z

pkg/server/container_start.go

-	// TODO(random-liu): [P0] Bind mount sandbox resolv.conf.
+	// Bind mount sandbox resolv.conf.
+	resolvPath := getResolvPath(getSandboxRootDir(c.rootDir, sandboxID))
+	// TODO Need to figure out whether we should always mount it as read-only


s/TODO/TODO:

Random-Liu · 2017-06-07T06:31:44Z

pkg/server/helpers.go

@@ -393,3 +398,8 @@ func (c *criContainerdService) ensureImageExists(ctx context.Context, ref string
 	}
 	return meta, nil
 }
+
+// getResolvPath returns resolv.conf filepath for specified sandbox.
+func getResolvPath(sandboxRoot string) string {


nit: Move this up to around getSandboxHosts.

Random-Liu · 2017-06-07T07:13:00Z

pkg/server/sandbox_run.go

+		// copy host's resolv.conf to resolvPath
+		err = c.os.CopyFile(resolvConfPath, resolvPath, 0755)
+	} else {
+		err = c.os.WriteFile(resolvPath, resolvContent, 0755)


Same, should be 0644.

Random-Liu · 2017-06-07T07:15:19Z

pkg/server/sandbox_run.go

+		err = c.os.WriteFile(resolvPath, resolvContent, 0755)
+	}
+	if err != nil {
+		return fmt.Errorf("failed to copy/write file to %q: %v", resolvPath, err)


Let's return err separately.

Random-Liu · 2017-06-07T07:16:05Z

pkg/server/sandbox_run.go

+	if dnsConfig := config.GetDnsConfig(); dnsConfig != nil {
+		resolvContent, err = parseDNSOptions(dnsConfig.Servers, dnsConfig.Searches, dnsConfig.Options)
+		if err != nil {
+			return fmt.Errorf("failed to parse sandbox dnsConfig: %v", err)


fmt.Errorf("failed to parse sandbox DNSConfig %+v: %v", DNSConfig, err)

Random-Liu · 2017-06-07T07:20:29Z

pkg/server/sandbox_run_test.go

+			servers:  []string{"8.8.8.8", "server.google.com"},
+			searches: []string{"114.114.114.114"},
+			options:  []string{"timeout:1"},
+		},


Let's add test case to check whether the function returns error when searches is more than 6

Let's add expectedOutput into the test case, and directly check assert.Equal(t, expectedOutput, resolvContent), e.g.:

map[string]struct{ dnsConfig *runtime.DNSConfig expectedContent string expectErr bool } { "empty dns options should return empty content": { dnsConfig: &runtime.DNSConfig{}, expectedOutput: "", }, "non-empty dns options should return correct content": { dnsConfig: &runtime.DNSConfig{/*...*/}, expectedOutput: `search 114.114.114.114 nameserver 8.8.8.8 nameserver server.google.com ... ` }, "should return error if dns search exceeds limit": { /*...*/ }, }

Random-Liu · 2017-06-07T07:28:32Z

pkg/server/sandbox_run.go

+
+// parseDNSOptions parse DNS options into resolv.conf format content,
+// if none option is specified, will return empty with no error.
+func parseDNSOptions(servers, searches, options []string) ([]byte, error) {


nit: Can we use string in this function, and use resolvContent += fmt.Sprintf("search %s\n", strings.Join(searches, " ")), which is much more cleaner.

Random-Liu · 2017-06-07T07:30:43Z

@Crazykev Reviewed. Nits are nice to address. Others please take a look.

Crazykev · 2017-06-08T03:54:53Z

@Random-Liu Updated, PTAL

Random-Liu

One change request and several nits, otherwise LGTM.

Random-Liu · 2017-06-08T06:19:17Z

pkg/server/container_start.go

+	// TODO: Need to figure out whether we should always mount it as read-only
+	mounts = append(mounts, &runtime.Mount{
+		ContainerPath: resolvConfPath,
+		HostPath:      resolvPath,


nit: getResolvPath(sandboxRootDir) to save one line.

Random-Liu · 2017-06-08T06:20:28Z

pkg/server/container_start_test.go

+				},
+				{
+					ContainerPath: resolvConfPath,
+					HostPath:      getResolvPath(testSandboxRootDir),


nit: Use testSandboxRootDir + "/resolv.conf", because we don't know whether getResolvPath works or not, which is also what we want to test.

Random-Liu · 2017-06-08T06:27:48Z

pkg/server/sandbox_run.go

+	}
+	resolvPath := getResolvPath(rootDir)
+	hostNetwork := config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork()
+	if resolvContent == "" && hostNetwork {


Based on https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_sandbox.go#L98-L103 and https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_sandbox.go#L616-L651, we should overwrite resolv.conf no matter it's using hostNetwork or not as long as resolvContent != "".

Should be:

if resolvContent == "" { // CopyFile } else { // WriteFile }

@Random-Liu

we should overwrite resolv.conf no matter it's using hostNetwork or not as long as resolvContent != "".

Yep, we should. While if it's not hostNetwork and resolvContent is empty(DnsConfig is nil or content is empty), should we just generate a empty resolv.conf(maybe one with default setting?) for this pod? In your logic it like we should pass host's resolv.conf to pod in this scene.

@Crazykev Based on current dockershim implementation, we only rewrite when resolvContent is not empty. Let's keep the same behavior now.

Random-Liu · 2017-06-08T06:29:15Z

pkg/server/sandbox_run.go

+	}
+	resolvPath := getResolvPath(rootDir)
+	hostNetwork := config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork()
+	if resolvContent == "" && hostNetwork {


Actually we also need unit test for this function. However, I'm fine with leave it for now. I'll add it in #67.

Random-Liu · 2017-06-08T06:30:00Z

pkg/server/sandbox_run.go

+	nSearches := len(searches)
+	nOptions := len(options)
+	resolvContent := ""
+	if nServers == 0 && nSearches == 0 && nOptions == 0 {


nit: This seems unnecessary, the function will return empty string anyway if all are empty.

And if we get rid of this block, we could directly use len in the following if clause, because we only need the result once.

Crazykev · 2017-06-09T10:29:50Z

@Random-Liu Addressed comments except adding a new test case, left it to you ;-)

Signed-off-by: Crazykev <crazykev@zju.edu.cn>

Random-Liu

LGTM with one nit. Will just apply the label and merge.
@Crazykev Thanks a lot!

Random-Liu · 2017-06-09T16:40:51Z

pkg/server/container_start_test.go

+				},
+				{
+					ContainerPath: resolvConfPath,
+					HostPath:      getResolvPath(testSandboxRootDir),


nit: This also needs to be updated, but I could do it in my PR.

Generate and maintain resolv.conf for sandbox

k8s-ci-robot added the cncf-cla: yes label May 25, 2017

Crazykev force-pushed the resolv branch from 0849c3f to 3f414ee Compare May 25, 2017 06:22

Random-Liu added this to the v0.1.0-rc2 milestone May 30, 2017

Random-Liu self-assigned this May 31, 2017

Random-Liu reviewed Jun 1, 2017

View reviewed changes

Random-Liu mentioned this pull request Jun 2, 2017

CRI-Containerd Missing Pieces #62

Closed

42 tasks

Crazykev force-pushed the resolv branch from 3f414ee to e487f77 Compare June 6, 2017 11:39

Crazykev commented Jun 6, 2017

View reviewed changes

Random-Liu mentioned this pull request Jun 7, 2017

Add sandbox dev shm #67

Merged

Crazykev force-pushed the resolv branch 2 times, most recently from 7c6bccb to 5668536 Compare June 7, 2017 04:44

Random-Liu reviewed Jun 7, 2017

View reviewed changes

Crazykev force-pushed the resolv branch 2 times, most recently from e1f793c to 0ab02d0 Compare June 8, 2017 03:28

Random-Liu reviewed Jun 8, 2017

View reviewed changes

Crazykev force-pushed the resolv branch from 0ab02d0 to 54bd2fa Compare June 9, 2017 10:21

Crazykev added 2 commits June 9, 2017 19:36

generate and maintain resolv.conf for sandbox

9bf7ffd

Signed-off-by: Crazykev <crazykev@zju.edu.cn>

add unit test

62d1e5d

Signed-off-by: Crazykev <crazykev@zju.edu.cn>

Crazykev force-pushed the resolv branch from 54bd2fa to 62d1e5d Compare June 9, 2017 11:37

Random-Liu reviewed Jun 9, 2017

View reviewed changes

Random-Liu added the lgtm label Jun 9, 2017

Random-Liu merged commit 227dbe9 into containerd:master Jun 9, 2017

Crazykev deleted the resolv branch June 10, 2017 11:45

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#50 from Crazykev/resolv

976ca4b

Generate and maintain resolv.conf for sandbox

Generate and maintain resolv.conf for sandbox #50

Generate and maintain resolv.conf for sandbox #50

Conversation

Crazykev commented May 25, 2017

Random-Liu commented May 25, 2017

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

Random-Liu Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Jun 1, 2017

Crazykev commented Jun 6, 2017

Choose a reason for hiding this comment

Crazykev commented Jun 7, 2017

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

Random-Liu commented Jun 7, 2017 • edited

Crazykev commented Jun 8, 2017

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Jun 8, 2017 • edited

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

Crazykev commented Jun 9, 2017

Random-Liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Jun 1, 2017 •

edited

Random-Liu commented Jun 7, 2017 •

edited

Random-Liu Jun 8, 2017 •

edited