From 88f4c252d6da8d62c43451e19b836a5173b91138 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Thu, 1 Jun 2017 19:27:50 +0000 Subject: [PATCH 1/2] Add sandbox /etc/hosts when using host network Signed-off-by: Lantao Liu --- pkg/os/os.go | 19 +++++++++++++++++ pkg/os/testing/fake_os.go | 17 +++++++++++++-- pkg/server/container_start.go | 39 ++++++++++++++++++++++++++++++++--- pkg/server/helpers.go | 10 ++++++++- pkg/server/sandbox_run.go | 31 +++++++++++++++------------- 5 files changed, 96 insertions(+), 20 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index 0adfe68a2..0f695ec72 100644 --- a/pkg/os/os.go +++ b/pkg/os/os.go @@ -32,6 +32,7 @@ type OS interface { RemoveAll(path string) error OpenFifo(ctx context.Context, fn string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) Stat(name string) (os.FileInfo, error) + CopyFile(src, dest string, perm os.FileMode) error } // RealOS is used to dispatch the real system level operations. @@ -56,3 +57,21 @@ func (RealOS) OpenFifo(ctx context.Context, fn string, flag int, perm os.FileMod func (RealOS) Stat(name string) (os.FileInfo, error) { return os.Stat(name) } + +// CopyFile copys src file to dest file +func (RealOS) CopyFile(src, dest string, perm os.FileMode) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + + out, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + if err != nil { + return err + } + defer out.Close() + + _, err = io.Copy(out, in) + return err +} diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index 25aa4de4a..4a3def8f2 100644 --- a/pkg/os/testing/fake_os.go +++ b/pkg/os/testing/fake_os.go @@ -34,7 +34,8 @@ type FakeOS struct { MkdirAllFn func(string, os.FileMode) error RemoveAllFn func(string) error OpenFifoFn func(context.Context, string, int, os.FileMode) (io.ReadWriteCloser, error) - StatFn func(name string) (os.FileInfo, error) + StatFn func(string) (os.FileInfo, error) + CopyFileFn func(string, string, os.FileMode) error errors map[string]error } @@ -118,7 +119,7 @@ func (f *FakeOS) OpenFifo(ctx context.Context, fn string, flag int, perm os.File return nil, nil } -// Stat is a fake call that invokes Stat or just return nil. +// Stat is a fake call that invokes StatFn or just return nil. func (f *FakeOS) Stat(name string) (os.FileInfo, error) { if err := f.getError("Stat"); err != nil { return nil, err @@ -129,3 +130,15 @@ func (f *FakeOS) Stat(name string) (os.FileInfo, error) { } return nil, nil } + +// CopyFile is a fake call that invokes CopyFileFn or just return nil. +func (f *FakeOS) CopyFile(src, dest string, perm os.FileMode) error { + if err := f.getError("CopyFile"); err != nil { + return err + } + + if f.CopyFileFn != nil { + return f.CopyFileFn(src, dest, perm) + } + return nil +} diff --git a/pkg/server/container_start.go b/pkg/server/container_start.go index 84aff28d0..458ac87b3 100644 --- a/pkg/server/container_start.go +++ b/pkg/server/container_start.go @@ -22,6 +22,7 @@ import ( "io" "os" "path/filepath" + "strings" "time" "github.com/containerd/containerd/api/services/execution" @@ -119,7 +120,10 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me if err != nil { return fmt.Errorf("failed to get container image %q: %v", meta.ImageRef, err) } - spec, err := c.generateContainerSpec(id, sandboxPid, config, sandboxConfig, imageMeta.Config) + + mounts := c.generateContainerMounts(getSandboxRootDir(c.rootDir, sandboxID), config) + + spec, err := c.generateContainerSpec(id, sandboxPid, config, sandboxConfig, imageMeta.Config, mounts) if err != nil { return fmt.Errorf("failed to generate container %q spec: %v", id, err) } @@ -218,7 +222,7 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me } func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint32, config *runtime.ContainerConfig, - sandboxConfig *runtime.PodSandboxConfig, imageConfig *imagespec.ImageConfig) (*runtimespec.Spec, error) { + sandboxConfig *runtime.PodSandboxConfig, imageConfig *imagespec.ImageConfig, extraMounts []*runtime.Mount) (*runtimespec.Spec, error) { // Creates a spec Generator with the default spec. // TODO(random-liu): [P2] Move container runtime spec generation into a helper function. g := generate.New() @@ -246,7 +250,8 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3 g.AddProcessEnv(e.GetKey(), e.GetValue()) } - addOCIBindMounts(&g, config.GetMounts()) + // Add extra mounts first so that CRI specified mounts can override. + addOCIBindMounts(&g, append(extraMounts, config.GetMounts()...)) // TODO(random-liu): [P1] Set device mapping. // Ref https://github.com/moby/moby/blob/master/oci/devices_linux.go. @@ -294,6 +299,21 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3 return g.Spec(), nil } +// generateContainerMounts sets up necessary container mounts including /dev/shm, /etc/hosts +// and /etc/resolv.conf. +func (c *criContainerdService) generateContainerMounts(sandboxRootDir string, config *runtime.ContainerConfig) []*runtime.Mount { + var mounts []*runtime.Mount + securityContext := config.GetLinux().GetSecurityContext() + mounts = append(mounts, &runtime.Mount{ + ContainerPath: etcHosts, + HostPath: getSandboxHosts(sandboxRootDir), + Readonly: securityContext.ReadonlyRootfs, + }) + // TODO(random-liu): [P0] Mount sandbox resolv.config. + // TODO(random-liu): [P0] Mount sandbox /dev/shm. + return mounts +} + // setOCIProcessArgs sets process args. It returns error if the final arg list // is empty. func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { @@ -315,6 +335,19 @@ func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, i return nil } +// addImageEnvs adds environment variables from image config. It returns error if +// an invalid environment variable is encountered. +func addImageEnvs(g *generate.Generator, imageEnvs []string) error { + for _, e := range imageEnvs { + kv := strings.Split(e, "=") + if len(kv) != 2 { + return fmt.Errorf("invalid environment variable %q", e) + } + g.AddProcessEnv(kv[0], kv[1]) + } + return nil +} + // addOCIBindMounts adds bind mounts. func addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount) { for _, mount := range mounts { diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index ee8ae1076..b07beffa3 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -85,6 +85,8 @@ const ( utsNSFormat = "/proc/%v/ns/uts" // pidNSFormat is the format of pid namespace of a process. pidNSFormat = "/proc/%v/ns/pid" + // etcHosts is the default path of /etc/hosts file. + etcHosts = "/etc/hosts" ) // generateID generates a random unique id. @@ -133,7 +135,8 @@ func getContainerRootDir(rootDir, id string) string { return filepath.Join(rootDir, containersDir, id) } -// getStreamingPipes returns the stdin/stdout/stderr pipes path in the root. +// getStreamingPipes returns the stdin/stdout/stderr pipes path in the +// container/sandbox root. func getStreamingPipes(rootDir string) (string, string, string) { stdin := filepath.Join(rootDir, stdinNamedPipe) stdout := filepath.Join(rootDir, stdoutNamedPipe) @@ -141,6 +144,11 @@ func getStreamingPipes(rootDir string) (string, string, string) { return stdin, stdout, stderr } +// getSandboxHosts returns the hosts file path inside the sandbox root directory. +func getSandboxHosts(sandboxRootDir string) string { + return filepath.Join(sandboxRootDir, "hosts") +} + // prepareStreamingPipes prepares stream named pipe for container. returns nil // streaming handler if corresponding stream path is empty. func (c *criContainerdService) prepareStreamingPipes(ctx context.Context, stdin, stdout, stderr string) ( diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index ddbc59a17..b79eee59e 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -19,7 +19,6 @@ package server import ( "encoding/json" "fmt" - "strings" "time" "github.com/containerd/containerd/api/services/execution" @@ -133,6 +132,13 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run return nil, fmt.Errorf("failed to start sandbox stderr logger: %v", err) } + // Setup sandbox /dev/shm, /etc/hosts and /etc/resolv.conf. + if err = c.setupSandboxFiles(sandboxRootDir, config); err != nil { + return nil, fmt.Errorf("failed to setup sandbox files: %v", err) + } + // No need to cleanup on error, because the whole sandbox root directory will be removed + // on error. + // Start sandbox container. spec, err := c.generateSandboxContainerSpec(id, config, imageMeta.Config) if err != nil { @@ -239,8 +245,6 @@ 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. - // TODO(random-liu): [P0] Add NamespaceGetter and PortMappingGetter to initialize network plugin. // TODO(random-liu): [P0] Add annotation to identify the container is managed by cri-containerd. @@ -270,8 +274,6 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r g.RemoveLinuxNamespace(string(runtimespec.PIDNamespace)) // nolint: errcheck } - // TODO(random-liu): [P0] Deal with /dev/shm. Use host for HostIpc, and create and mount for - // non-HostIpc. What about mqueue? if nsOptions.GetHostIpc() { g.RemoveLinuxNamespace(string(runtimespec.IPCNamespace)) // nolint: errcheck } @@ -293,15 +295,16 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r return g.Spec(), nil } -// addImageEnvs adds environment variables from image config. It returns error if -// an invalid environment variable is encountered. -func addImageEnvs(g *generate.Generator, imageEnvs []string) error { - for _, e := range imageEnvs { - kv := strings.Split(e, "=") - if len(kv) != 2 { - return fmt.Errorf("invalid environment variable %q", e) - } - g.AddProcessEnv(kv[0], kv[1]) +// setupSandboxFiles sets up necessary sandbox files including /dev/shm, /etc/hosts +// and /etc/resolv.conf. +func (c *criContainerdService) setupSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error { + // TODO(random-liu): Consider whether we should maintain /etc/hosts and /etc/resolv.conf in kubelet. + sandboxEtcHosts := getSandboxHosts(rootDir) + if err := c.os.CopyFile(etcHosts, sandboxEtcHosts, 0666); err != nil { + return fmt.Errorf("failed to generate sandbox hosts file %q: %v", sandboxEtcHosts, err) } + // TODO(random-liu): [P0] Set DNS options. Maintain a resolv.conf for the sandbox. + // TODO(random-liu): [P0] Deal with /dev/shm. Use host for HostIpc, and create and mount for + // non-HostIpc. What about mqueue? return nil } From 4eac00fe2397751dab663bbab0b5895a1c727336 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Thu, 1 Jun 2017 19:39:16 +0000 Subject: [PATCH 2/2] Add unit test. Signed-off-by: Lantao Liu --- pkg/server/container_start_test.go | 79 ++++++++++++++++++++++++++++-- pkg/server/sandbox_run_test.go | 15 ++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/pkg/server/container_start_test.go b/pkg/server/container_start_test.go index 1f0980e19..5fce69496 100644 --- a/pkg/server/container_start_test.go +++ b/pkg/server/container_start_test.go @@ -176,7 +176,7 @@ func TestGeneralContainerSpec(t *testing.T) { testPid := uint32(1234) config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData() c := newTestCRIContainerdService() - spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig) + spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig, nil) assert.NoError(t, err) specCheck(t, testID, testPid, spec) } @@ -188,7 +188,7 @@ func TestContainerSpecTty(t *testing.T) { c := newTestCRIContainerdService() for _, tty := range []bool{true, false} { config.Tty = tty - spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig) + spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig, nil) assert.NoError(t, err) specCheck(t, testID, testPid, spec) assert.Equal(t, tty, spec.Process.Terminal) @@ -202,13 +202,46 @@ func TestContainerSpecReadonlyRootfs(t *testing.T) { c := newTestCRIContainerdService() for _, readonly := range []bool{true, false} { config.Linux.SecurityContext.ReadonlyRootfs = readonly - spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig) + spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig, nil) assert.NoError(t, err) specCheck(t, testID, testPid, spec) assert.Equal(t, readonly, spec.Root.Readonly) } } +func TestContainerSpecWithExtraMounts(t *testing.T) { + testID := "test-id" + testPid := uint32(1234) + config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData() + c := newTestCRIContainerdService() + mountInConfig := &runtime.Mount{ + ContainerPath: "test-container-path", + HostPath: "test-host-path", + Readonly: false, + } + config.Mounts = append(config.Mounts, mountInConfig) + extraMount := &runtime.Mount{ + ContainerPath: "test-container-path", + HostPath: "test-host-path-extra", + Readonly: true, + } + spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig, []*runtime.Mount{extraMount}) + assert.NoError(t, err) + specCheck(t, testID, testPid, spec) + var mounts []runtimespec.Mount + for _, m := range spec.Mounts { + if m.Destination == "test-container-path" { + mounts = append(mounts, m) + } + } + t.Logf("Extra mounts should come first") + require.Len(t, mounts, 2) + assert.Equal(t, "test-host-path-extra", mounts[0].Source) + assert.Contains(t, mounts[0].Options, "ro") + assert.Equal(t, "test-host-path", mounts[1].Source) + assert.Contains(t, mounts[1].Options, "rw") +} + func TestContainerSpecCommand(t *testing.T) { for desc, test := range map[string]struct { criEntrypoint []string @@ -270,6 +303,46 @@ func TestContainerSpecCommand(t *testing.T) { } } +func TestGenerateContainerMounts(t *testing.T) { + testSandboxRootDir := "test-sandbox-root" + for desc, test := range map[string]struct { + securityContext *runtime.LinuxContainerSecurityContext + expectedMounts []*runtime.Mount + }{ + "should setup ro /etc/hosts mount when rootfs is read-only": { + securityContext: &runtime.LinuxContainerSecurityContext{ + ReadonlyRootfs: true, + }, + expectedMounts: []*runtime.Mount{{ + ContainerPath: "/etc/hosts", + HostPath: testSandboxRootDir + "/hosts", + Readonly: true, + }}, + }, + "should setup rw /etc/hosts mount when rootfs is read-write": { + securityContext: &runtime.LinuxContainerSecurityContext{}, + expectedMounts: []*runtime.Mount{{ + ContainerPath: "/etc/hosts", + HostPath: testSandboxRootDir + "/hosts", + Readonly: false, + }}, + }, + } { + config := &runtime.ContainerConfig{ + Metadata: &runtime.ContainerMetadata{ + Name: "test-name", + Attempt: 1, + }, + Linux: &runtime.LinuxContainerConfig{ + SecurityContext: test.securityContext, + }, + } + c := newTestCRIContainerdService() + mounts := c.generateContainerMounts(testSandboxRootDir, config) + assert.Equal(t, test.expectedMounts, mounts, desc) + } +} + func TestStartContainer(t *testing.T) { testID := "test-id" testSandboxID := "test-sandbox-id" diff --git a/pkg/server/sandbox_run_test.go b/pkg/server/sandbox_run_test.go index ba3bf7221..f4f81dbbe 100644 --- a/pkg/server/sandbox_run_test.go +++ b/pkg/server/sandbox_run_test.go @@ -157,6 +157,21 @@ func TestGenerateSandboxContainerSpec(t *testing.T) { } } +func TestSetupSandboxFiles(t *testing.T) { + testRootDir := "test-sandbox-root" + expectedCopys := [][]interface{}{ + {"/etc/hosts", testRootDir + "/hosts", os.FileMode(0666)}, + } + c := newTestCRIContainerdService() + var copys [][]interface{} + c.os.(*ostesting.FakeOS).CopyFileFn = func(src string, dest string, perm os.FileMode) error { + copys = append(copys, []interface{}{src, dest, perm}) + return nil + } + c.setupSandboxFiles(testRootDir, nil) + assert.Equal(t, expectedCopys, copys, "should copy /etc/hosts for sandbox") +} + func TestRunPodSandbox(t *testing.T) { config, imageConfig, specCheck := getRunPodSandboxTestData() c := newTestCRIContainerdService()