From ccdd75760c1f2d2be2dbb21522b67ceab28fd901 Mon Sep 17 00:00:00 2001 From: Renaud Gaubert Date: Fri, 7 Feb 2020 14:17:51 -0800 Subject: [PATCH 1/3] Add the CreateRuntime, CreateContainer and StartContainer Hooks Signed-off-by: Renaud Gaubert --- libcontainer/configs/config.go | 93 ++++++++++++++++++++++------- libcontainer/container_linux.go | 22 +++---- libcontainer/factory_linux_test.go | 10 ++-- libcontainer/init_linux.go | 2 + libcontainer/process_linux.go | 45 ++++++++++---- libcontainer/rootfs_linux.go | 7 +++ libcontainer/specconv/spec_linux.go | 21 +++++-- libcontainer/standard_init_linux.go | 8 +++ libcontainer/state_linux.go | 24 ++++---- 9 files changed, 166 insertions(+), 66 deletions(-) diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index 8c19c4a1958..37d8f94063c 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -8,7 +8,7 @@ import ( "time" "github.com/opencontainers/runtime-spec/specs-go" - + "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -176,7 +176,7 @@ type Config struct { // Hooks are a collection of actions to perform at various container lifecycle events. // CommandHooks are serialized to JSON, but other hooks are not. - Hooks *Hooks + Hooks Hooks // Version is the version of opencontainer specification that is supported. Version string `json:"version"` @@ -203,17 +203,54 @@ type Config struct { RootlessCgroups bool `json:"rootless_cgroups,omitempty"` } -type Hooks struct { +type HookName string +type HookList []Hook +type Hooks map[HookName]HookList + +const ( // Prestart commands are executed after the container namespaces are created, // but before the user supplied command is executed from init. - Prestart []Hook + // Note: This hook is now deprecated + // Prestart commands are called in the Runtime namespace. + Prestart HookName = "prestart" + + // CreateRuntime commands MUST be called as part of the create operation after + // the runtime environment has been created but before the pivot_root has been executed. + // CreateRuntime is called immediately after the deprecated Prestart hook. + // CreateRuntime commands are called in the Runtime Namespace. + CreateRuntime = "createRuntime" + + // CreateContainer commands MUST be called as part of the create operation after + // the runtime environment has been created but before the pivot_root has been executed. + // CreateContainer commands are called in the Container namespace. + CreateContainer = "createContainer" + + // StartContainer commands MUST be called as part of the start operation and before + // the container process is started. + // StartContainer commands are called in the Container namespace. + StartContainer = "startContainer" // Poststart commands are executed after the container init process starts. - Poststart []Hook + // Poststart commands are called in the Runtime Namespace. + Poststart = "poststart" // Poststop commands are executed after the container init process exits. - Poststop []Hook -} + // Poststop commands are called in the Runtime Namespace. + Poststop = "poststop" +) + +var ( + HookNameList = []HookName{Prestart, CreateRuntime, CreateContainer, StartContainer, Poststart, Poststop} +) + +// TODO move this to runtime-spec +// See: https://github.com/opencontainers/runtime-spec/pull/1046 +const ( + Creating = "creating" + Created = "created" + Running = "running" + Stopped = "stopped" +) type Capabilities struct { // Bounding is the set of capabilities checked by the kernel. @@ -228,32 +265,39 @@ type Capabilities struct { Ambient []string } -func (hooks *Hooks) UnmarshalJSON(b []byte) error { - var state struct { - Prestart []CommandHook - Poststart []CommandHook - Poststop []CommandHook +func (hooks HookList) RunHooks(state *specs.State) error { + for i, h := range hooks { + if err := h.Run(state); err != nil { + return errors.Wrapf(err, "Running hook #%d:", i) + } } + return nil +} + +func (hooks *Hooks) UnmarshalJSON(b []byte) error { + var state map[HookName][]CommandHook + if err := json.Unmarshal(b, &state); err != nil { return err } - deserialize := func(shooks []CommandHook) (hooks []Hook) { - for _, shook := range shooks { - hooks = append(hooks, shook) + *hooks = Hooks{} + for n, commandHooks := range state { + if len(commandHooks) == 0 { + continue } - return hooks + (*hooks)[n] = HookList{} + for _, h := range commandHooks { + (*hooks)[n] = append((*hooks)[n], h) + } } - hooks.Prestart = deserialize(state.Prestart) - hooks.Poststart = deserialize(state.Poststart) - hooks.Poststop = deserialize(state.Poststop) return nil } -func (hooks Hooks) MarshalJSON() ([]byte, error) { +func (hooks *Hooks) MarshalJSON() ([]byte, error) { serialize := func(hooks []Hook) (serializableHooks []CommandHook) { for _, hook := range hooks { switch chook := hook.(type) { @@ -268,9 +312,12 @@ func (hooks Hooks) MarshalJSON() ([]byte, error) { } return json.Marshal(map[string]interface{}{ - "prestart": serialize(hooks.Prestart), - "poststart": serialize(hooks.Poststart), - "poststop": serialize(hooks.Poststop), + "prestart": serialize((*hooks)[Prestart]), + "createRuntime": serialize((*hooks)[CreateRuntime]), + "createContainer": serialize((*hooks)[CreateContainer]), + "startContainer": serialize((*hooks)[StartContainer]), + "poststart": serialize((*hooks)[Poststart]), + "poststop": serialize((*hooks)[Poststop]), }) } diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 2c7420ef18e..31543ad9eb9 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -29,6 +29,7 @@ import ( "github.com/checkpoint-restore/go-criu/v4" criurpc "github.com/checkpoint-restore/go-criu/v4/rpc" "github.com/golang/protobuf/proto" + errorsf "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink/nl" "golang.org/x/sys/unix" @@ -379,13 +380,12 @@ func (c *linuxContainer) start(process *Process) error { if err != nil { return err } - for i, hook := range c.config.Hooks.Poststart { - if err := hook.Run(s); err != nil { - if err := ignoreTerminateErrors(parent.terminate()); err != nil { - logrus.Warn(err) - } - return newSystemErrorWithCausef(err, "running poststart hook %d", i) + + if err := c.config.Hooks[configs.Poststart].RunHooks(s); err != nil { + if err := ignoreTerminateErrors(parent.terminate()); err != nil { + logrus.Warn(errorsf.Wrapf(err, "Running Poststart hook")) } + return err } } } @@ -1621,10 +1621,12 @@ func (c *linuxContainer) criuNotifications(resp *criurpc.CriuResp, process *Proc return nil } s.Pid = int(notify.GetPid()) - for i, hook := range c.config.Hooks.Prestart { - if err := hook.Run(s); err != nil { - return newSystemErrorWithCausef(err, "running prestart hook %d", i) - } + + if err := c.config.Hooks[configs.Prestart].RunHooks(s); err != nil { + return err + } + if err := c.config.Hooks[configs.CreateRuntime].RunHooks(s); err != nil { + return err } } case "post-restore": diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index af91aaceef0..0e380fd2a5b 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -157,14 +157,14 @@ func TestFactoryLoadContainer(t *testing.T) { // setup default container config and state for mocking var ( id = "1" - expectedHooks = &configs.Hooks{ - Prestart: []configs.Hook{ + expectedHooks = configs.Hooks{ + configs.Prestart: configs.HookList{ configs.CommandHook{Command: configs.Command{Path: "prestart-hook"}}, }, - Poststart: []configs.Hook{ + configs.Poststart: configs.HookList{ configs.CommandHook{Command: configs.Command{Path: "poststart-hook"}}, }, - Poststop: []configs.Hook{ + configs.Poststop: configs.HookList{ unserializableHook{}, configs.CommandHook{Command: configs.Command{Path: "poststop-hook"}}, }, @@ -201,7 +201,7 @@ func TestFactoryLoadContainer(t *testing.T) { if config.Rootfs != expectedConfig.Rootfs { t.Fatalf("expected rootfs %q but received %q", expectedConfig.Rootfs, config.Rootfs) } - expectedHooks.Poststop = expectedHooks.Poststop[1:] // expect unserializable hook to be skipped + expectedHooks[configs.Poststop] = expectedHooks[configs.Poststop][1:] // expect unserializable hook to be skipped if !reflect.DeepEqual(config.Hooks, expectedHooks) { t.Fatalf("expects hooks %q but received %q", expectedHooks, config.Hooks) } diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 2c304001ad7..5487a223c1d 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -20,6 +20,7 @@ import ( "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/runc/libcontainer/user" "github.com/opencontainers/runc/libcontainer/utils" + "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" @@ -67,6 +68,7 @@ type initConfig struct { ConsoleHeight uint16 `json:"console_height"` RootlessEUID bool `json:"rootless_euid,omitempty"` RootlessCgroups bool `json:"rootless_cgroups,omitempty"` + SpecState *specs.State `json:"spec_state,omitempty"` } type initer interface { diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 50f968d9a06..028fe8ab5e2 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -362,6 +362,9 @@ func (p *initProcess) start() (retErr error) { if err := p.createNetworkInterfaces(); err != nil { return newSystemErrorWithCause(err, "creating network interfaces") } + if err := p.updateSpecState(); err != nil { + return newSystemErrorWithCause(err, "updating the spec state") + } if err := p.sendConfig(); err != nil { return newSystemErrorWithCause(err, "sending config to init process") } @@ -378,9 +381,9 @@ func (p *initProcess) start() (retErr error) { if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil { return newSystemErrorWithCause(err, "setting rlimits for ready process") } - // call prestart hooks + // call prestart and CreateRuntime hooks if !p.config.Config.Namespaces.Contains(configs.NEWNS) { - // Setup cgroup before prestart hook, so that the prestart hook could apply cgroup permissions. + // Setup cgroup before the hook, so that the prestart and CreateRuntime hook could apply cgroup permissions. if err := p.manager.Set(p.config.Config); err != nil { return newSystemErrorWithCause(err, "setting cgroup config for ready process") } @@ -397,11 +400,14 @@ func (p *initProcess) start() (retErr error) { } // initProcessStartTime hasn't been set yet. s.Pid = p.cmd.Process.Pid - s.Status = "creating" - for i, hook := range p.config.Config.Hooks.Prestart { - if err := hook.Run(s); err != nil { - return newSystemErrorWithCausef(err, "running prestart hook %d", i) - } + s.Status = configs.Creating + hooks := p.config.Config.Hooks + + if err := hooks[configs.Prestart].RunHooks(s); err != nil { + return err + } + if err := hooks[configs.CreateRuntime].RunHooks(s); err != nil { + return err } } } @@ -427,11 +433,14 @@ func (p *initProcess) start() (retErr error) { } // initProcessStartTime hasn't been set yet. s.Pid = p.cmd.Process.Pid - s.Status = "creating" - for i, hook := range p.config.Config.Hooks.Prestart { - if err := hook.Run(s); err != nil { - return newSystemErrorWithCausef(err, "running prestart hook %d", i) - } + s.Status = configs.Creating + hooks := p.config.Config.Hooks + + if err := hooks[configs.Prestart].RunHooks(s); err != nil { + return err + } + if err := hooks[configs.CreateRuntime].RunHooks(s); err != nil { + return err } } // Sync with child. @@ -450,7 +459,7 @@ func (p *initProcess) start() (retErr error) { return newSystemErrorWithCause(ierr, "container init") } if p.config.Config.Namespaces.Contains(configs.NEWNS) && !sentResume { - return newSystemError(errors.New("could not synchronise after executing prestart hooks with container process")) + return newSystemError(errors.New("could not synchronise after executing prestart and CreateRuntime hooks with container process")) } if err := unix.Shutdown(int(p.messageSockPair.parent.Fd()), unix.SHUT_WR); err != nil { return newSystemErrorWithCause(err, "shutting down init pipe") @@ -492,6 +501,16 @@ func (p *initProcess) startTime() (uint64, error) { return stat.StartTime, err } +func (p *initProcess) updateSpecState() error { + s, err := p.container.currentOCIState() + if err != nil { + return err + } + + p.config.SpecState = s + return nil +} + func (p *initProcess) sendConfig() error { // send the config to the container's init process, we don't use JSON Encode // here because there might be a problem in JSON decoder in some cases, see: diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 1052b9658fb..b8e792fbb32 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -98,6 +98,13 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig) (err error) { return newSystemErrorWithCausef(err, "changing dir to %q", config.Rootfs) } + s := iConfig.SpecState + s.Pid = unix.Getpid() + s.Status = configs.Creating + if err := iConfig.Config.Hooks[configs.CreateContainer].RunHooks(s); err != nil { + return err + } + if config.NoPivotRoot { err = msMoveRoot(config.Rootfs) } else if config.Namespaces.Contains(configs.NEWNS) { diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index ccae6ad9b4b..6fd98f02753 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -881,20 +881,31 @@ func SetupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) { } func createHooks(rspec *specs.Spec, config *configs.Config) { - config.Hooks = &configs.Hooks{} + config.Hooks = configs.Hooks{} if rspec.Hooks != nil { - for _, h := range rspec.Hooks.Prestart { cmd := createCommandHook(h) - config.Hooks.Prestart = append(config.Hooks.Prestart, configs.NewCommandHook(cmd)) + config.Hooks[configs.Prestart] = append(config.Hooks[configs.Prestart], configs.NewCommandHook(cmd)) + } + for _, h := range rspec.Hooks.CreateRuntime { + cmd := createCommandHook(h) + config.Hooks[configs.CreateRuntime] = append(config.Hooks[configs.CreateRuntime], configs.NewCommandHook(cmd)) + } + for _, h := range rspec.Hooks.CreateContainer { + cmd := createCommandHook(h) + config.Hooks[configs.CreateContainer] = append(config.Hooks[configs.CreateContainer], configs.NewCommandHook(cmd)) + } + for _, h := range rspec.Hooks.StartContainer { + cmd := createCommandHook(h) + config.Hooks[configs.StartContainer] = append(config.Hooks[configs.StartContainer], configs.NewCommandHook(cmd)) } for _, h := range rspec.Hooks.Poststart { cmd := createCommandHook(h) - config.Hooks.Poststart = append(config.Hooks.Poststart, configs.NewCommandHook(cmd)) + config.Hooks[configs.Poststart] = append(config.Hooks[configs.Poststart], configs.NewCommandHook(cmd)) } for _, h := range rspec.Hooks.Poststop { cmd := createCommandHook(h) - config.Hooks.Poststop = append(config.Hooks.Poststop, configs.NewCommandHook(cmd)) + config.Hooks[configs.Poststop] = append(config.Hooks[configs.Poststop], configs.NewCommandHook(cmd)) } } } diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index f5be67c01b7..08e6b4718a4 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -207,6 +207,14 @@ func (l *linuxStandardInit) Init() error { return newSystemErrorWithCause(err, "init seccomp") } } + + s := l.config.SpecState + s.Pid = unix.Getpid() + s.Status = configs.Created + if err := l.config.Config.Hooks[configs.StartContainer].RunHooks(s); err != nil { + return err + } + if err := unix.Exec(name, l.config.Args[0:], os.Environ()); err != nil { return newSystemErrorWithCause(err, "exec user process") } diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go index 92d42418771..aa800c36008 100644 --- a/libcontainer/state_linux.go +++ b/libcontainer/state_linux.go @@ -61,17 +61,21 @@ func destroy(c *linuxContainer) error { } func runPoststopHooks(c *linuxContainer) error { - if c.config.Hooks != nil { - s, err := c.currentOCIState() - if err != nil { - return err - } - for _, hook := range c.config.Hooks.Poststop { - if err := hook.Run(s); err != nil { - return err - } - } + hooks := c.config.Hooks + if hooks == nil { + return nil } + + s, err := c.currentOCIState() + if err != nil { + return err + } + s.Status = configs.Stopped + + if err := hooks[configs.Poststop].RunHooks(s); err != nil { + return err + } + return nil } From 2f7bdf9d3bd69ddaa3bfab0e6c4047c5691aafb0 Mon Sep 17 00:00:00 2001 From: Renaud Gaubert Date: Sat, 9 May 2020 01:05:12 -0700 Subject: [PATCH 2/3] Tests the new Hook Signed-off-by: Renaud Gaubert --- libcontainer/configs/config.go | 4 - libcontainer/configs/config_linux_test.go | 4 + libcontainer/configs/config_test.go | 58 +++++++----- libcontainer/integration/exec_test.go | 109 +++++++++++++--------- libcontainer/specconv/spec_linux_test.go | 51 +++++++++- 5 files changed, 155 insertions(+), 71 deletions(-) diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index 37d8f94063c..ac523b41760 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -239,10 +239,6 @@ const ( Poststop = "poststop" ) -var ( - HookNameList = []HookName{Prestart, CreateRuntime, CreateContainer, StartContainer, Poststart, Poststop} -) - // TODO move this to runtime-spec // See: https://github.com/opencontainers/runtime-spec/pull/1046 const ( diff --git a/libcontainer/configs/config_linux_test.go b/libcontainer/configs/config_linux_test.go index 9c5f0febed6..a16b0982338 100644 --- a/libcontainer/configs/config_linux_test.go +++ b/libcontainer/configs/config_linux_test.go @@ -8,6 +8,10 @@ import ( "testing" ) +var ( + HookNameList = []HookName{Prestart, CreateRuntime, CreateContainer, StartContainer, Poststart, Poststop} +) + func loadConfig(name string) (*Config, error) { f, err := os.Open(filepath.Join("../sample_configs", name)) if err != nil { diff --git a/libcontainer/configs/config_test.go b/libcontainer/configs/config_test.go index c89a764de1a..db9ccaee6ab 100644 --- a/libcontainer/configs/config_test.go +++ b/libcontainer/configs/config_test.go @@ -15,27 +15,29 @@ import ( func TestUnmarshalHooks(t *testing.T) { timeout := time.Second - prestartCmd := configs.NewCommandHook(configs.Command{ - Path: "/var/vcap/hooks/prestart", + hookCmd := configs.NewCommandHook(configs.Command{ + Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, Dir: "/var/vcap", Timeout: &timeout, }) - prestart, err := json.Marshal(prestartCmd.Command) - if err != nil { - t.Fatal(err) - } - hook := configs.Hooks{} - err = hook.UnmarshalJSON([]byte(fmt.Sprintf(`{"Prestart" :[%s]}`, prestart))) + hookJson, err := json.Marshal(hookCmd) if err != nil { t.Fatal(err) } - if !reflect.DeepEqual(hook.Prestart[0], prestartCmd) { - t.Errorf("Expected prestart to equal %+v but it was %+v", - prestartCmd, hook.Prestart[0]) + for _, hookName := range configs.HookNameList { + hooks := configs.Hooks{} + err = hooks.UnmarshalJSON([]byte(fmt.Sprintf(`{"%s" :[%s]}`, hookName, hookJson))) + if err != nil { + t.Fatal(err) + } + + if !reflect.DeepEqual(hooks[hookName], hookCmd) { + t.Errorf("Expected %s to equal %+v but it was %+v", hookName, hookCmd, hooks[hookName]) + } } } @@ -50,8 +52,8 @@ func TestUnmarshalHooksWithInvalidData(t *testing.T) { func TestMarshalHooks(t *testing.T) { timeout := time.Second - prestartCmd := configs.NewCommandHook(configs.Command{ - Path: "/var/vcap/hooks/prestart", + hookCmd := configs.NewCommandHook(configs.Command{ + Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, Dir: "/var/vcap", @@ -59,14 +61,21 @@ func TestMarshalHooks(t *testing.T) { }) hook := configs.Hooks{ - Prestart: []configs.Hook{prestartCmd}, + configs.Prestart: configs.HookList{hookCmd}, + configs.CreateRuntime: configs.HookList{hookCmd}, + configs.CreateContainer: configs.HookList{hookCmd}, + configs.StartContainer: configs.HookList{hookCmd}, + configs.Poststart: configs.HookList{hookCmd}, + configs.Poststop: configs.HookList{hookCmd}, } hooks, err := hook.MarshalJSON() if err != nil { t.Fatal(err) } - h := `{"poststart":null,"poststop":null,"prestart":[{"path":"/var/vcap/hooks/prestart","args":["--pid=123"],"env":["FOO=BAR"],"dir":"/var/vcap","timeout":1000000000}]}` + // Note Marshal seems to output fields in alphabetical order + hookCmdJson := `[{"path":"/var/vcap/hooks/hook","args":["--pid=123"],"env":["FOO=BAR"],"dir":"/var/vcap","timeout":1000000000}]` + h := fmt.Sprintf(`{"createContainer":%[1]s,"createRuntime":%[1]s,"poststart":%[1]s,"poststop":%[1]s,"prestart":%[1]s,"startContainer":%[1]s}`, hookCmdJson) if string(hooks) != h { t.Errorf("Expected hooks %s to equal %s", string(hooks), h) } @@ -75,8 +84,8 @@ func TestMarshalHooks(t *testing.T) { func TestMarshalUnmarshalHooks(t *testing.T) { timeout := time.Second - prestart := configs.NewCommandHook(configs.Command{ - Path: "/var/vcap/hooks/prestart", + hookCmd := configs.NewCommandHook(configs.Command{ + Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, Dir: "/var/vcap", @@ -84,7 +93,12 @@ func TestMarshalUnmarshalHooks(t *testing.T) { }) hook := configs.Hooks{ - Prestart: []configs.Hook{prestart}, + configs.Prestart: configs.HookList{hookCmd}, + configs.CreateRuntime: configs.HookList{hookCmd}, + configs.CreateContainer: configs.HookList{hookCmd}, + configs.StartContainer: configs.HookList{hookCmd}, + configs.Poststart: configs.HookList{hookCmd}, + configs.Poststop: configs.HookList{hookCmd}, } hooks, err := hook.MarshalJSON() if err != nil { @@ -96,8 +110,8 @@ func TestMarshalUnmarshalHooks(t *testing.T) { if err != nil { t.Fatal(err) } - if !reflect.DeepEqual(umMhook.Prestart[0], prestart) { - t.Errorf("Expected hooks to be equal after mashaling -> unmarshaling them: %+v, %+v", umMhook.Prestart[0], prestart) + if !reflect.DeepEqual(umMhook, hook) { + t.Errorf("Expected hooks to be equal after mashaling -> unmarshaling them: %+v, %+v", umMhook, hook) } } @@ -106,14 +120,14 @@ func TestMarshalHooksWithUnexpectedType(t *testing.T) { return nil }) hook := configs.Hooks{ - Prestart: []configs.Hook{fHook}, + configs.CreateRuntime: configs.HookList{fHook}, } hooks, err := hook.MarshalJSON() if err != nil { t.Fatal(err) } - h := `{"poststart":null,"poststop":null,"prestart":null}` + h := `{"createContainer":null,"createRuntime":null,"poststart":null,"poststop":null,"prestart":null,"startContainer":null}` if string(hooks) != h { t.Errorf("Expected hooks %s to equal %s", string(hooks), h) } diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 3808099a132..ada513b557e 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -1118,39 +1118,67 @@ func TestHook(t *testing.T) { } return config.Rootfs, nil } + createFileFromBundle := func(filename, bundle string) error { + root, err := getRootfsFromBundle(bundle) + if err != nil { + return err + } + + f, err := os.Create(filepath.Join(root, filename)) + if err != nil { + return err + } + return f.Close() + } - config.Hooks = &configs.Hooks{ - Prestart: []configs.Hook{ + // Note FunctionHooks can't be serialized to json this means they won't be passed down to the container + // For CreateContainer and StartContainer which run in the container namespace, this means we need to pass Command Hooks. + hookFiles := map[configs.HookName]string{ + configs.Prestart: "prestart", + configs.CreateRuntime: "createRuntime", + configs.CreateContainer: "createContainer", + configs.StartContainer: "startContainer", + configs.Poststart: "poststart", + } + + config.Hooks = configs.Hooks{ + configs.Prestart: configs.HookList{ configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected prestart hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } - - root, err := getRootfsFromBundle(s.Bundle) - if err != nil { - return err - } - f, err := os.Create(filepath.Join(root, "test")) - if err != nil { - return err + return createFileFromBundle(hookFiles[configs.Prestart], s.Bundle) + }), + }, + configs.CreateRuntime: configs.HookList{ + configs.NewFunctionHook(func(s *specs.State) error { + if s.Bundle != expectedBundle { + t.Fatalf("Expected createRuntime hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } - return f.Close() + return createFileFromBundle(hookFiles[configs.CreateRuntime], s.Bundle) + }), + }, + configs.CreateContainer: configs.HookList{ + configs.NewCommandHook(configs.Command{ + Path: "/bin/bash", + Args: []string{"/bin/bash", "-c", fmt.Sprintf("touch ./%s", hookFiles[configs.CreateContainer])}, }), }, - Poststart: []configs.Hook{ + configs.StartContainer: configs.HookList{ + configs.NewCommandHook(configs.Command{ + Path: "/bin/sh", + Args: []string{"/bin/sh", "-c", fmt.Sprintf("touch /%s", hookFiles[configs.StartContainer])}, + }), + }, + configs.Poststart: configs.HookList{ configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected poststart hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } - - root, err := getRootfsFromBundle(s.Bundle) - if err != nil { - return err - } - return ioutil.WriteFile(filepath.Join(root, "test"), []byte("hello world"), 0755) + return createFileFromBundle(hookFiles[configs.Poststart], s.Bundle) }), }, - Poststop: []configs.Hook{ + configs.Poststop: configs.HookList{ configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected poststop hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) @@ -1160,7 +1188,13 @@ func TestHook(t *testing.T) { if err != nil { return err } - return os.RemoveAll(filepath.Join(root, "test")) + + for _, hook := range hookFiles { + if err = os.RemoveAll(filepath.Join(root, hook)); err != nil { + return err + } + } + return nil }), }, } @@ -1173,10 +1207,16 @@ func TestHook(t *testing.T) { container, err := newContainerWithName("test", config) ok(t, err) + // e.g: 'ls /prestart ...' + cmd := "ls " + for _, hook := range hookFiles { + cmd += "/" + hook + " " + } + var stdout bytes.Buffer pconfig := libcontainer.Process{ Cwd: "/", - Args: []string{"sh", "-c", "ls /test"}, + Args: []string{"sh", "-c", cmd}, Env: standardEnvironment, Stdin: nil, Stdout: &stdout, @@ -1188,30 +1228,15 @@ func TestHook(t *testing.T) { // Wait for process waitProcess(&pconfig, t) - outputLs := string(stdout.Bytes()) - - // Check that the ls output has the expected file touched by the prestart hook - if !strings.Contains(outputLs, "/test") { - container.Destroy() - t.Fatalf("ls output doesn't have the expected file: %s", outputLs) - } - - // Check that the file is written by the poststart hook - testFilePath := filepath.Join(rootfs, "test") - contents, err := ioutil.ReadFile(testFilePath) - if err != nil { - t.Fatalf("cannot read file '%s': %s", testFilePath, err) - } - if string(contents) != "hello world" { - t.Fatalf("Expected test file to contain 'hello world'; got '%s'", string(contents)) - } - if err := container.Destroy(); err != nil { t.Fatalf("container destroy %s", err) } - fi, err := os.Stat(filepath.Join(rootfs, "test")) - if err == nil || !os.IsNotExist(err) { - t.Fatalf("expected file to not exist, got %s", fi.Name()) + + for _, hook := range []string{"prestart", "createRuntime", "poststart"} { + fi, err := os.Stat(filepath.Join(rootfs, hook)) + if err == nil || !os.IsNotExist(err) { + t.Fatalf("expected file '%s to not exists, but it does", fi.Name()) + } } } diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 18bb7c7fd35..1a506adc136 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/libcontainer/specconv/spec_linux_test.go @@ -42,6 +42,33 @@ func TestCreateHooks(t *testing.T) { Args: []string{"--some", "thing"}, }, }, + CreateRuntime: []specs.Hook{ + { + Path: "/some/hook/path", + }, + { + Path: "/some/hook2/path", + Args: []string{"--some", "thing"}, + }, + }, + CreateContainer: []specs.Hook{ + { + Path: "/some/hook/path", + }, + { + Path: "/some/hook2/path", + Args: []string{"--some", "thing"}, + }, + }, + StartContainer: []specs.Hook{ + { + Path: "/some/hook/path", + }, + { + Path: "/some/hook2/path", + Args: []string{"--some", "thing"}, + }, + }, Poststart: []specs.Hook{ { Path: "/some/hook/path", @@ -77,19 +104,37 @@ func TestCreateHooks(t *testing.T) { conf := &configs.Config{} createHooks(rspec, conf) - prestart := conf.Hooks.Prestart + prestart := conf.Hooks[configs.Prestart] if len(prestart) != 2 { t.Error("Expected 2 Prestart hooks") } - poststart := conf.Hooks.Poststart + createRuntime := conf.Hooks[configs.CreateRuntime] + + if len(createRuntime) != 2 { + t.Error("Expected 2 createRuntime hooks") + } + + createContainer := conf.Hooks[configs.CreateContainer] + + if len(createContainer) != 2 { + t.Error("Expected 2 createContainer hooks") + } + + startContainer := conf.Hooks[configs.StartContainer] + + if len(startContainer) != 2 { + t.Error("Expected 2 startContainer hooks") + } + + poststart := conf.Hooks[configs.Poststart] if len(poststart) != 3 { t.Error("Expected 3 Poststart hooks") } - poststop := conf.Hooks.Poststop + poststop := conf.Hooks[configs.Poststop] if len(poststop) != 4 { t.Error("Expected 4 Poststop hooks") From 861afa7509a16b415f55de138cff7170481be97a Mon Sep 17 00:00:00 2001 From: Renaud Gaubert Date: Thu, 14 May 2020 00:56:55 +0000 Subject: [PATCH 3/3] Add integration tests for the new runc hooks This patch adds a test based on real world usage of runc hooks (libnvidia-container). We verify that mounting a library inside a container and running ldconfig succeeds. Signed-off-by: Renaud Gaubert --- Dockerfile | 18 +++++++++ Vagrantfile.centos7 | 6 ++- Vagrantfile.fedora32 | 11 +++-- libcontainer/configs/config_test.go | 2 +- tests/integration/helpers.bash | 31 ++++++++++++++ tests/integration/hooks.bats | 63 +++++++++++++++++++++++++++++ tests/integration/multi-arch.bash | 40 +++++++++++++----- 7 files changed, 157 insertions(+), 14 deletions(-) create mode 100644 tests/integration/hooks.bats diff --git a/Dockerfile b/Dockerfile index 6a17ceb5e83..b1511121e69 100644 --- a/Dockerfile +++ b/Dockerfile @@ -67,6 +67,19 @@ RUN mkdir -p /usr/src/criu \ && cd - \ && rm -rf /usr/src/criu +# install skopeo +RUN echo 'deb http://download.opensuse.org/repositories/devel:/kubic:/libcontainers:/stable/Debian_Unstable/ /' > /etc/apt/sources.list.d/devel:kubic:libcontainers:stable.list \ + && wget -nv https://download.opensuse.org/repositories/devel:kubic:libcontainers:stable/Debian_Unstable/Release.key -O- | sudo apt-key add - \ + && apt-get update \ + && apt-get install -y --no-install-recommends skopeo \ + && rm -rf /etc/apt/sources.list.d/devel:kubic:libcontainers:stable.list \ + && apt-get clean \ + && rm -rf /var/cache/apt /var/lib/apt/lists/*; + +# install umoci +RUN curl -o /usr/local/bin/umoci -fsSL https://github.com/opencontainers/umoci/releases/download/v0.4.5/umoci.amd64 \ + && chmod +x /usr/local/bin/umoci + COPY script/tmpmount / WORKDIR /go/src/github.com/opencontainers/runc ENTRYPOINT ["/tmpmount"] @@ -78,4 +91,9 @@ RUN mkdir -p "${ROOTFS}" RUN . tests/integration/multi-arch.bash \ && curl -fsSL `get_busybox` | tar xfJC - "${ROOTFS}" +ENV DEBIAN_ROOTFS /debian +RUN mkdir -p "${DEBIAN_ROOTFS}" +RUN . tests/integration/multi-arch.bash \ + && get_and_extract_debian "$DEBIAN_ROOTFS" + COPY . . diff --git a/Vagrantfile.centos7 b/Vagrantfile.centos7 index 49cb2779f89..7342e4ab1ed 100644 --- a/Vagrantfile.centos7 +++ b/Vagrantfile.centos7 @@ -18,12 +18,16 @@ Vagrant.configure("2") do |config| # install yum packages yum install -y -q epel-release - yum install -y -q gcc git iptables jq libseccomp-devel make + yum install -y -q gcc git iptables jq libseccomp-devel make skopeo yum clean all # install Go curl -fsSL "https://dl.google.com/go/go${GO_VERSION}.linux-amd64.tar.gz" | tar Cxz /usr/local + # Install umoci + curl -o /usr/local/bin/umoci -fsSL https://github.com/opencontainers/umoci/releases/download/v0.4.5/umoci.amd64 + chmod +x /usr/local/bin/umoci + # install bats git clone https://github.com/bats-core/bats-core cd bats-core diff --git a/Vagrantfile.fedora32 b/Vagrantfile.fedora32 index cb4e02860ac..6b3dc652ec0 100644 --- a/Vagrantfile.fedora32 +++ b/Vagrantfile.fedora32 @@ -17,7 +17,7 @@ Vagrant.configure("2") do |config| config exclude kernel,kernel-core config install_weak_deps false update -install iptables gcc make golang-go libseccomp-devel bats jq git-core criu +install iptables gcc make golang-go libseccomp-devel bats jq git-core criu skopeo ts run EOF dnf clean all @@ -31,10 +31,15 @@ EOF cat /root/rootless.key.pub >> /home/rootless/.ssh/authorized_keys chown -R rootless.rootless /home/rootless + # Install umoci + curl -o /usr/local/bin/umoci -fsSL https://github.com/opencontainers/umoci/releases/download/v0.4.5/umoci.amd64 + chmod +x /usr/local/bin/umoci + # Add busybox for libcontainer/integration tests . /vagrant/tests/integration/multi-arch.bash \ - && mkdir /busybox \ - && curl -fsSL $(get_busybox) | tar xfJC - /busybox + && mkdir /busybox /debian \ + && curl -fsSL $(get_busybox) | tar xfJC - /busybox \ + && get_and_extract_debian /debian # Delegate cgroup v2 controllers to rootless user via --systemd-cgroup mkdir -p /etc/systemd/system/user@.service.d diff --git a/libcontainer/configs/config_test.go b/libcontainer/configs/config_test.go index db9ccaee6ab..7d66575db72 100644 --- a/libcontainer/configs/config_test.go +++ b/libcontainer/configs/config_test.go @@ -35,7 +35,7 @@ func TestUnmarshalHooks(t *testing.T) { t.Fatal(err) } - if !reflect.DeepEqual(hooks[hookName], hookCmd) { + if !reflect.DeepEqual(hooks[hookName], configs.HookList{hookCmd}) { t.Errorf("Expected %s to equal %+v but it was %+v", hookName, hookCmd, hooks[hookName]) } } diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index 5db99320150..6fd22aed43e 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -21,6 +21,9 @@ HELLO_FILE=`get_hello` HELLO_IMAGE="$TESTDATA/$HELLO_FILE" HELLO_BUNDLE="$BATS_TMPDIR/hello-world" +# debian image +DEBIAN_BUNDLE="$BATS_TMPDIR/debiantest" + # CRIU PATH CRIU="$(which criu 2>/dev/null || true)" @@ -422,6 +425,27 @@ function setup_hello() { update_config '(.. | select(.? == "sh")) |= "/hello"' } +function setup_debian() { + # skopeo and umoci are not installed on the travis runner + if [ -n "${RUNC_USE_SYSTEMD}" ]; then + return + fi + + setup_recvtty + run mkdir "$DEBIAN_BUNDLE" + + if [ ! -d "$DEBIAN_ROOTFS/rootfs" ]; then + get_and_extract_debian "$DEBIAN_BUNDLE" + fi + + # Use the cached version + if [ ! -d "$DEBIAN_BUNDLE/rootfs" ]; then + cp -r "$DEBIAN_ROOTFS"/* "$DEBIAN_BUNDLE/" + fi + + cd "$DEBIAN_BUNDLE" +} + function teardown_running_container() { runc list # $1 should be a container name such as "test_busybox" @@ -459,3 +483,10 @@ function teardown_hello() { teardown_running_container test_hello run rm -f -r "$HELLO_BUNDLE" } + +function teardown_debian() { + cd "$INTEGRATION_ROOT" + teardown_recvtty + teardown_running_container test_debian + run rm -f -r "$DEBIAN_BUNDLE" +} diff --git a/tests/integration/hooks.bats b/tests/integration/hooks.bats new file mode 100644 index 00000000000..aa2f70bec9e --- /dev/null +++ b/tests/integration/hooks.bats @@ -0,0 +1,63 @@ +#!/usr/bin/env bats + +load helpers + +# CR = CreateRuntime +# CC = CreataContainer +HOOKLIBCR=librunc-hooks-create-runtime.so +HOOKLIBCC=librunc-hooks-create-container.so +LIBPATH="$DEBIAN_BUNDLE/rootfs/lib/" + +function setup() { + umount $LIBPATH/$HOOKLIBCR.1.0.0 &> /dev/null || true + umount $LIBPATH/$HOOKLIBCC.1.0.0 &> /dev/null || true + + teardown_debian + setup_debian +} + +function teardown() { + umount $LIBPATH/$HOOKLIBCR.1.0.0 &> /dev/null || true + umount $LIBPATH/$HOOKLIBCC.1.0.0 &> /dev/null || true + + rm -f $HOOKLIBCR.1.0.0 $HOOKLIBCC.1.0.0 + teardown_debian +} + +@test "runc run (hooks library tests)" { + requires root + requires no_systemd + + # setup some dummy libs + gcc -shared -Wl,-soname,librunc-hooks-create-runtime.so.1 -o "$HOOKLIBCR.1.0.0" + gcc -shared -Wl,-soname,librunc-hooks-create-container.so.1 -o "$HOOKLIBCC.1.0.0" + + current_pwd="$(pwd)" + + # To mount $HOOKLIBCR we need to do that in the container namespace + create_runtime_hook=$(cat <<-EOF + pid=\$(cat - | jq -r '.pid') + touch "$LIBPATH/$HOOKLIBCR.1.0.0" + nsenter -m \$ns -t \$pid mount --bind "$current_pwd/$HOOKLIBCR.1.0.0" "$LIBPATH/$HOOKLIBCR.1.0.0" + EOF) + + create_container_hook="touch ./lib/$HOOKLIBCC.1.0.0 && mount --bind $current_pwd/$HOOKLIBCC.1.0.0 ./lib/$HOOKLIBCC.1.0.0" + + CONFIG=$(jq --arg create_runtime_hook "$create_runtime_hook" --arg create_container_hook "$create_container_hook" ' + .hooks |= . + {"createRuntime": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", $create_runtime_hook]}]} | + .hooks |= . + {"createContainer": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", $create_container_hook]}]} | + .hooks |= . + {"startContainer": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", "ldconfig"]}]} | + .process.args = ["/bin/sh", "-c", "ldconfig -p | grep librunc"]' $DEBIAN_BUNDLE/config.json) + echo "${CONFIG}" > config.json + + runc run test_debian + [ "$status" -eq 0 ] + + echo "Checking create-runtime library" + echo $output | grep $HOOKLIBCR + [ "$?" -eq 0 ] + + echo "Checking create-container library" + echo $output | grep $HOOKLIBCC + [ "$?" -eq 0 ] +} diff --git a/tests/integration/multi-arch.bash b/tests/integration/multi-arch.bash index 5616bf7e100..c0519146b0b 100644 --- a/tests/integration/multi-arch.bash +++ b/tests/integration/multi-arch.bash @@ -1,5 +1,5 @@ #!/bin/bash -get_busybox(){ +get_busybox() { case $(go env GOARCH) in arm64) echo 'https://github.com/docker-library/busybox/raw/dist-arm64v8/glibc/busybox.tar.xz' @@ -10,13 +10,35 @@ get_busybox(){ esac } -get_hello(){ +get_hello() { case $(go env GOARCH) in - arm64) - echo 'hello-world-aarch64.tar' - ;; - *) - echo 'hello-world.tar' - ;; - esac + arm64) + echo 'hello-world-aarch64.tar' + ;; + *) + echo 'hello-world.tar' + ;; + esac +} + +get_and_extract_debian() { + tmp=$(mktemp -d) + cd "$tmp" + + debian="debian:3.11.6" + + case $(go env GOARCH) in + arm64) + skopeo copy docker://arm64v8/debian:buster "oci:$debian" + ;; + *) + skopeo copy docker://amd64/debian:buster "oci:$debian" + ;; + esac + + args="$([ -z "${ROOTLESS_TESTPATH+x}" ] && echo "--rootless")" + umoci unpack $args --image "$debian" "$1" + + cd - + rm -rf "$tmp" }