From c5c1422ad4fa4dbdfc950bbff897d4c472e2a96c Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Wed, 21 Mar 2018 15:34:29 +0100 Subject: [PATCH 1/7] validation: add test for NSProcInPath I initially tried to add the checks in the container process 'runtimetest' by adding annotations prefixed with "runtimetest/". But that proved impractical with TAP outputs because I wanted to have several tests for each namespace. This patch now validates the namespaces outside the container with util.RuntimeOutsideValidate(). Signed-off-by: Alban Crequy --- validation/linux_ns_path.go | 126 ++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 validation/linux_ns_path.go diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go new file mode 100644 index 00000000..711a4eaa --- /dev/null +++ b/validation/linux_ns_path.go @@ -0,0 +1,126 @@ +package main + +import ( + "fmt" + "os" + "os/exec" + "runtime" + "time" + + "github.com/mndrix/tap-go" + rspec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/runtime-tools/validation/util" +) + +func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { + // Calling 'unshare' (part of util-linux) is easier than doing it from + // Golang: mnt namespaces cannot be unshared from multithreaded + // programs. + cmd := exec.Command("unshare", unshareOpt, "--fork", "sleep", "10000") + err := cmd.Start() + if err != nil { + return fmt.Errorf("cannot run unshare: %s", err) + } + defer func() { + if cmd.Process != nil { + cmd.Process.Kill() + } + cmd.Wait() + }() + if cmd.Process == nil { + return fmt.Errorf("process failed to start") + } + unsharePid := cmd.Process.Pid + + // Wait until 'unshare' switched its namespace + // TODO: avoid synchronisation with sleeps. + time.Sleep(time.Second) + + specialChildren := "" + if ns == "pid" { + // Unsharing pidns does not move the process into the new + // pidns but the next forked process. 'unshare' is called with + // '--fork' so the pidns will be fully created and populated + // with a pid 1. + // + // However, finding out the pid of the child process is not + // trivial: it would require to parse + // /proc/$pid/task/$tid/children but that only works on kernels + // with CONFIG_PROC_CHILDREN (not all distros have that). + // + // It is easier to look at /proc/$pid/ns/pid_for_children on + // the parent process. Available since Linux 4.12. + specialChildren = "_for_children" + } + unshareNsPath := fmt.Sprintf("/proc/%d/ns/%s", unsharePid, ns+specialChildren) + unshareNsInode, err := os.Readlink(unshareNsPath) + if err != nil { + return fmt.Errorf("cannot read namespace link for the unshare process: %s", err) + } + + g, err := util.GetDefaultGenerator() + if err != nil { + return fmt.Errorf("cannot get the default generator: %v", err) + } + + rtns := getRuntimeToolsNamespace(ns) + g.AddOrReplaceLinuxNamespace(rtns, unshareNsPath) + + // The spec is not clear about userns mappings when reusing an + // existing userns. + // See https://github.com/opencontainers/runtime-spec/issues/961 + //if ns == "user" { + // g.AddLinuxUIDMapping(uint32(1000), uint32(0), uint32(1000)) + // g.AddLinuxGIDMapping(uint32(1000), uint32(0), uint32(1000)) + //} + + err = util.RuntimeOutsideValidate(g, func(config *rspec.Spec, state *rspec.State) error { + containerNsPath := fmt.Sprintf("/proc/%d/ns/%s", state.Pid, ns) + containerNsInode, err := os.Readlink(containerNsPath) + if err != nil { + out, err2 := exec.Command("sh", "-c", fmt.Sprintf("ls -la /proc/%d/ns/", state.Pid)).CombinedOutput() + return fmt.Errorf("cannot read namespace link for the container process: %s\n%v\n%v", err, err2, out) + } + if containerNsInode != unshareNsInode { + return fmt.Errorf("expected: %q, found: %q", unshareNsInode, containerNsInode) + } + return nil + }) + + return err +} + +func main() { + t := tap.New() + t.Header(0) + + cases := []struct { + name string + unshareOpt string + }{ + {"cgroup", "--cgroup"}, + {"ipc", "--ipc"}, + {"mnt", "--mount"}, + {"net", "--net"}, + {"pid", "--pid"}, + {"user", "--user"}, + {"uts", "--uts"}, + } + + for _, c := range cases { + if "linux" != runtime.GOOS { + t.Skip(1, fmt.Sprintf("linux-specific namespace test: %s", c)) + } + + err := testNamespacePath(t, c.name, c.unshareOpt) + t.Ok(err == nil, fmt.Sprintf("set %s namespace by path", c.name)) + if err != nil { + diagnostic := map[string]string{ + "error": err.Error(), + } + t.YAML(diagnostic) + } + } + + t.AutoPlan() +} From 73358a3df6a602ea624fecd751f8b5cd2f6c600a Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Tue, 10 Apr 2018 17:45:14 +0200 Subject: [PATCH 2/7] validation: fix a bug when passing in namespace strings We need to deal with additional namespace strings, in case of mount & network namespaces, because `MapStrToNamespace()` does not recognize input strings like `mnt` or `net`. Found by @alban. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index 711a4eaa..1bc785b9 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -12,6 +12,21 @@ import ( "github.com/opencontainers/runtime-tools/validation/util" ) +func getRuntimeToolsNamespace(ns string) string { + // Deal with exceptional cases of "net" and "mnt", because those strings + // cannot be recognized by mapStrToNamespace(), which actually expects + // "network" and "mount" respectively. + switch ns { + case "net": + return "network" + case "mnt": + return "mount" + } + + // In other cases, return just the original string + return ns +} + func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { // Calling 'unshare' (part of util-linux) is easier than doing it from // Golang: mnt namespaces cannot be unshared from multithreaded From e132d370c703dc1abaeaf0eb7baad462e4ca537b Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Tue, 17 Apr 2018 17:01:16 +0200 Subject: [PATCH 3/7] validation: kill child processes by setting process groups `unshare --fork` spawn child processes, which remains even after the test program finished. To be able to kill these processes at once, we should set a process group for the child processes. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index 1bc785b9..0bda2b89 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "runtime" + "syscall" "time" "github.com/mndrix/tap-go" @@ -32,6 +33,9 @@ func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { // Golang: mnt namespaces cannot be unshared from multithreaded // programs. cmd := exec.Command("unshare", unshareOpt, "--fork", "sleep", "10000") + // We shoud set Setpgid to true, to be able to allow the unshare process + // as well as its child processes to be killed by a single kill command. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} err := cmd.Start() if err != nil { return fmt.Errorf("cannot run unshare: %s", err) @@ -41,6 +45,7 @@ func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { cmd.Process.Kill() } cmd.Wait() + syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) }() if cmd.Process == nil { return fmt.Errorf("process failed to start") From 23c9a517ea327f89d45522267da127698e2365dc Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 20 Apr 2018 14:48:36 +0200 Subject: [PATCH 4/7] validation: sync with unshare by using select & time ticker Since it takes some time until unshare can switch to a new namespace, we need a sync mechanism for the NSProcInPath. Let's use a generic sync mechanism by using select & time ticker instead of pure sleep. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 140 +++++++++++++++++++++++------------- 1 file changed, 89 insertions(+), 51 deletions(-) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index 0bda2b89..3cdb7b8c 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -28,54 +28,67 @@ func getRuntimeToolsNamespace(ns string) string { return ns } -func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { - // Calling 'unshare' (part of util-linux) is easier than doing it from - // Golang: mnt namespaces cannot be unshared from multithreaded - // programs. - cmd := exec.Command("unshare", unshareOpt, "--fork", "sleep", "10000") - // We shoud set Setpgid to true, to be able to allow the unshare process - // as well as its child processes to be killed by a single kill command. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - err := cmd.Start() +func waitForState(stateCheckFunc func() error) error { + timeout := 3 * time.Second + alarm := time.After(timeout) + ticker := time.Tick(200 * time.Millisecond) + for { + select { + case <-alarm: + return fmt.Errorf("failed to reach expected state within %v", timeout) + case <-ticker: + if err := stateCheckFunc(); err == nil { + return nil + } + } + } +} + +func checkNamespacePath(unsharePid int, ns string) error { + testNsPath := fmt.Sprintf("/proc/%d/ns/%s", os.Getpid(), ns) + testNsInode, err := os.Readlink(testNsPath) if err != nil { - return fmt.Errorf("cannot run unshare: %s", err) + out, err2 := exec.Command("sh", "-c", fmt.Sprintf("ls -la /proc/%d/ns/", os.Getpid())).CombinedOutput() + return fmt.Errorf("cannot read namespace link for the test process: %s\n%v\n%v", err, err2, string(out)) } - defer func() { - if cmd.Process != nil { - cmd.Process.Kill() + + unshareNsPath := "" + unshareNsInode := "" + + doCheckNamespacePath := func() error { + specialChildren := "" + if ns == "pid" { + // Unsharing pidns does not move the process into the new + // pidns but the next forked process. 'unshare' is called with + // '--fork' so the pidns will be fully created and populated + // with a pid 1. + // + // However, finding out the pid of the child process is not + // trivial: it would require to parse + // /proc/$pid/task/$tid/children but that only works on kernels + // with CONFIG_PROC_CHILDREN (not all distros have that). + // + // It is easier to look at /proc/$pid/ns/pid_for_children on + // the parent process. Available since Linux 4.12. + specialChildren = "_for_children" } - cmd.Wait() - syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - }() - if cmd.Process == nil { - return fmt.Errorf("process failed to start") - } - unsharePid := cmd.Process.Pid - - // Wait until 'unshare' switched its namespace - // TODO: avoid synchronisation with sleeps. - time.Sleep(time.Second) - - specialChildren := "" - if ns == "pid" { - // Unsharing pidns does not move the process into the new - // pidns but the next forked process. 'unshare' is called with - // '--fork' so the pidns will be fully created and populated - // with a pid 1. - // - // However, finding out the pid of the child process is not - // trivial: it would require to parse - // /proc/$pid/task/$tid/children but that only works on kernels - // with CONFIG_PROC_CHILDREN (not all distros have that). - // - // It is easier to look at /proc/$pid/ns/pid_for_children on - // the parent process. Available since Linux 4.12. - specialChildren = "_for_children" + unshareNsPath = fmt.Sprintf("/proc/%d/ns/%s", unsharePid, ns+specialChildren) + unshareNsInode, err = os.Readlink(unshareNsPath) + if err != nil { + return fmt.Errorf("cannot read namespace link for the unshare process: %s", err) + } + + if testNsInode == unshareNsInode { + return fmt.Errorf("expected: %q, found: %q", testNsInode, unshareNsInode) + } + + return nil } - unshareNsPath := fmt.Sprintf("/proc/%d/ns/%s", unsharePid, ns+specialChildren) - unshareNsInode, err := os.Readlink(unshareNsPath) - if err != nil { - return fmt.Errorf("cannot read namespace link for the unshare process: %s", err) + + // Since it takes some time until unshare switched to the new namespace, + // we should make a loop to check for the result up to 3 seconds. + if err := waitForState(doCheckNamespacePath); err != nil { + return err } g, err := util.GetDefaultGenerator() @@ -87,14 +100,15 @@ func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { g.AddOrReplaceLinuxNamespace(rtns, unshareNsPath) // The spec is not clear about userns mappings when reusing an - // existing userns. + // existing userns. Anyway in reality, we should set up uid/gid + // mappings, to make userns work in most runtimes. // See https://github.com/opencontainers/runtime-spec/issues/961 - //if ns == "user" { - // g.AddLinuxUIDMapping(uint32(1000), uint32(0), uint32(1000)) - // g.AddLinuxGIDMapping(uint32(1000), uint32(0), uint32(1000)) - //} + // if ns == "user" { + // g.AddLinuxUIDMapping(uint32(1000), uint32(0), uint32(1000)) + // g.AddLinuxGIDMapping(uint32(1000), uint32(0), uint32(1000)) + // } - err = util.RuntimeOutsideValidate(g, func(config *rspec.Spec, state *rspec.State) error { + return util.RuntimeOutsideValidate(g, func(config *rspec.Spec, state *rspec.State) error { containerNsPath := fmt.Sprintf("/proc/%d/ns/%s", state.Pid, ns) containerNsInode, err := os.Readlink(containerNsPath) if err != nil { @@ -106,8 +120,32 @@ func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { } return nil }) +} + +func testNamespacePath(t *tap.T, ns string, unshareOpt string) error { + // Calling 'unshare' (part of util-linux) is easier than doing it from + // Golang: mnt namespaces cannot be unshared from multithreaded + // programs. + cmd := exec.Command("unshare", unshareOpt, "--fork", "sleep", "10000") + // We shoud set Setpgid to true, to be able to allow the unshare process + // as well as its child processes to be killed by a single kill command. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + err := cmd.Start() + if err != nil { + return fmt.Errorf("cannot run unshare: %s", err) + } + defer func() { + if cmd.Process != nil { + cmd.Process.Kill() + } + cmd.Wait() + syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + }() + if cmd.Process == nil { + return fmt.Errorf("process failed to start") + } - return err + return checkNamespacePath(cmd.Process.Pid, ns) } func main() { From a90cd2ba3c0b47bfb7f9667daf072c7f89bcaef0 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Mon, 23 Apr 2018 16:12:27 +0200 Subject: [PATCH 5/7] validation: print out correct diagnostics based on specError Like other tests, `testNamespacePath` should also print out details about the failed tests, based on specError. Also make `waitForState(doCheckNamespacePath)` return a correct error coming from `checkNamespacePath`, not from `waitForState`. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index 3cdb7b8c..eee92b89 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -10,6 +10,7 @@ import ( "github.com/mndrix/tap-go" rspec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/runtime-tools/specerror" "github.com/opencontainers/runtime-tools/validation/util" ) @@ -52,6 +53,8 @@ func checkNamespacePath(unsharePid int, ns string) error { return fmt.Errorf("cannot read namespace link for the test process: %s\n%v\n%v", err, err2, string(out)) } + var errNsPath error + unshareNsPath := "" unshareNsInode := "" @@ -75,11 +78,13 @@ func checkNamespacePath(unsharePid int, ns string) error { unshareNsPath = fmt.Sprintf("/proc/%d/ns/%s", unsharePid, ns+specialChildren) unshareNsInode, err = os.Readlink(unshareNsPath) if err != nil { - return fmt.Errorf("cannot read namespace link for the unshare process: %s", err) + errNsPath = fmt.Errorf("cannot read namespace link for the unshare process: %s", err) + return errNsPath } if testNsInode == unshareNsInode { - return fmt.Errorf("expected: %q, found: %q", testNsInode, unshareNsInode) + errNsPath = fmt.Errorf("expected: %q, found: %q", testNsInode, unshareNsInode) + return errNsPath } return nil @@ -88,7 +93,10 @@ func checkNamespacePath(unsharePid int, ns string) error { // Since it takes some time until unshare switched to the new namespace, // we should make a loop to check for the result up to 3 seconds. if err := waitForState(doCheckNamespacePath); err != nil { - return err + // we should return errNsPath instead of err, because errNsPath is what + // returned from the actual test function doCheckNamespacePath(), not + // waitForState(). + return errNsPath } g, err := util.GetDefaultGenerator() @@ -173,8 +181,13 @@ func main() { err := testNamespacePath(t, c.name, c.unshareOpt) t.Ok(err == nil, fmt.Sprintf("set %s namespace by path", c.name)) if err != nil { + specErr := specerror.NewError(specerror.NSProcInPath, err, rspec.Version) diagnostic := map[string]string{ - "error": err.Error(), + "actual": fmt.Sprintf("err == %v", err), + "expected": "err == nil", + "namespace type": c.name, + "level": specErr.(*specerror.Error).Err.Level.String(), + "reference": specErr.(*specerror.Error).Err.Reference, } t.YAML(diagnostic) } From 1ceca9e1648b206ed33caad30c5dd73cc5ec4f7a Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Tue, 24 Apr 2018 10:24:35 +0200 Subject: [PATCH 6/7] validation: use rfcError instead of specerror Now that a new helper `NewRFCError()` is available, we should make use of the helper instead of `specerror.NewError()`. This way, we can avoid doing multiple casts to be able to get rfcError. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index eee92b89..bd4b153f 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -181,13 +181,16 @@ func main() { err := testNamespacePath(t, c.name, c.unshareOpt) t.Ok(err == nil, fmt.Sprintf("set %s namespace by path", c.name)) if err != nil { - specErr := specerror.NewError(specerror.NSProcInPath, err, rspec.Version) + rfcError, errRfc := specerror.NewRFCError(specerror.NSProcInPath, err, rspec.Version) + if errRfc != nil { + continue + } diagnostic := map[string]string{ "actual": fmt.Sprintf("err == %v", err), "expected": "err == nil", "namespace type": c.name, - "level": specErr.(*specerror.Error).Err.Level.String(), - "reference": specErr.(*specerror.Error).Err.Reference, + "level": rfcError.Level.String(), + "reference": rfcError.Reference, } t.YAML(diagnostic) } From ad0e97efdb2fc32d98a259ad2647dec02cd3de89 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 25 May 2018 13:15:45 +0200 Subject: [PATCH 7/7] validation: exclude user namespaces and cgroup namespaces Cgroup namespaces test fails because runc does not support it yet. User namespaces test fails because of many unexpected issues when running unshare with runc, etc. We are going to revisit these tests later, to figure out how to deal with them. Let's exclude these two types of namespaces for now. Signed-off-by: Dongsu Park --- validation/linux_ns_path.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/validation/linux_ns_path.go b/validation/linux_ns_path.go index bd4b153f..7f0d90a3 100644 --- a/validation/linux_ns_path.go +++ b/validation/linux_ns_path.go @@ -107,15 +107,6 @@ func checkNamespacePath(unsharePid int, ns string) error { rtns := getRuntimeToolsNamespace(ns) g.AddOrReplaceLinuxNamespace(rtns, unshareNsPath) - // The spec is not clear about userns mappings when reusing an - // existing userns. Anyway in reality, we should set up uid/gid - // mappings, to make userns work in most runtimes. - // See https://github.com/opencontainers/runtime-spec/issues/961 - // if ns == "user" { - // g.AddLinuxUIDMapping(uint32(1000), uint32(0), uint32(1000)) - // g.AddLinuxGIDMapping(uint32(1000), uint32(0), uint32(1000)) - // } - return util.RuntimeOutsideValidate(g, func(config *rspec.Spec, state *rspec.State) error { containerNsPath := fmt.Sprintf("/proc/%d/ns/%s", state.Pid, ns) containerNsInode, err := os.Readlink(containerNsPath) @@ -164,12 +155,10 @@ func main() { name string unshareOpt string }{ - {"cgroup", "--cgroup"}, {"ipc", "--ipc"}, {"mnt", "--mount"}, {"net", "--net"}, {"pid", "--pid"}, - {"user", "--user"}, {"uts", "--uts"}, }