Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

agent: add support for guest-hooks (v2) #393

Merged
merged 1 commit into from
Oct 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 55 additions & 19 deletions agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/opencontainers/runc/libcontainer"
"github.com/opencontainers/runc/libcontainer/configs"
_ "github.com/opencontainers/runc/libcontainer/nsenter"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
"golang.org/x/net/context"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -98,25 +99,27 @@ type sandboxStorage struct {
type sandbox struct {
sync.RWMutex

id string
hostname string
containers map[string]*container
channel channel
network network
wg sync.WaitGroup
sharedPidNs namespace
mounts []string
subreaper reaper
server *grpc.Server
pciDeviceMap map[string]string
deviceWatchers map[string](chan string)
sharedUTSNs namespace
sharedIPCNs namespace
running bool
noPivotRoot bool
enableGrpcTrace bool
sandboxPidNs bool
storages map[string]*sandboxStorage
id string
hostname string
containers map[string]*container
channel channel
network network
wg sync.WaitGroup
sharedPidNs namespace
mounts []string
subreaper reaper
server *grpc.Server
pciDeviceMap map[string]string
deviceWatchers map[string](chan string)
sharedUTSNs namespace
sharedIPCNs namespace
guestHooks *specs.Hooks
guestHooksPresent bool
running bool
noPivotRoot bool
enableGrpcTrace bool
sandboxPidNs bool
storages map[string]*sandboxStorage
}

var agentFields = logrus.Fields{
Expand Down Expand Up @@ -240,6 +243,39 @@ func (s *sandbox) setSandboxStorage(path string) bool {
return false
}

// scanGuestHooks will search the given guestHookPath
// for any OCI hooks
func (s *sandbox) scanGuestHooks(guestHookPath string) {
fieldLogger := agentLog.WithField("oci-hook-path", guestHookPath)
fieldLogger.Info("Scanning guest filesystem for OCI hooks")

s.guestHooks.Prestart = findHooks(guestHookPath, "prestart")
s.guestHooks.Poststart = findHooks(guestHookPath, "poststart")
s.guestHooks.Poststop = findHooks(guestHookPath, "poststop")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be overkill, but I wonder if we want to run these three filesystem operations in separate goroutines and wait for them to finish to minimise the time cost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems entirely overkill since every function call is likely to stop after the first ReadDir :), that is to say if <path>/{prestart,poststart,poststop} does not exist or is not a directory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack ;)


if len(s.guestHooks.Prestart) > 0 || len(s.guestHooks.Poststart) > 0 || len(s.guestHooks.Poststop) > 0 {
s.guestHooksPresent = true
} else {
fieldLogger.Warn("Guest hooks were requested but none were found")
}
}

// addGuestHooks will add any guest OCI hooks that were
// found to the OCI spec
func (s *sandbox) addGuestHooks(spec *specs.Spec) {
if spec == nil {
return
}

if spec.Hooks == nil {
spec.Hooks = &specs.Hooks{}
}

spec.Hooks.Prestart = append(spec.Hooks.Prestart, s.guestHooks.Prestart...)
spec.Hooks.Poststart = append(spec.Hooks.Poststart, s.guestHooks.Poststart...)
spec.Hooks.Poststop = append(spec.Hooks.Poststop, s.guestHooks.Poststop...)
}

// unSetSandboxStorage will decrement the sandbox storage
// reference counter. If there aren't any containers using
// that sandbox storage, this method will remove the
Expand Down
52 changes: 52 additions & 0 deletions agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,18 @@ package main
import (
"io/ioutil"
"os"
"path"
"path/filepath"
"reflect"
"strings"
"syscall"
"testing"

"google.golang.org/grpc"

pb "github.com/kata-containers/agent/protocols/grpc"
"github.com/opencontainers/runc/libcontainer"
specs "github.com/opencontainers/runtime-spec/specs-go"
"github.com/stretchr/testify/assert"
"golang.org/x/net/context"
)
Expand Down Expand Up @@ -446,3 +449,52 @@ func TestGetCgroupMountsSuccessful(t *testing.T) {
err = syscall.Unmount(cgprocDir, 0)
assert.Nil(t, err, "%v", err)
}

func TestAddGuestHooks(t *testing.T) {
assert := assert.New(t)

hookPath, err := ioutil.TempDir("", "hooks")
assert.NoError(err)
defer os.RemoveAll(hookPath)

poststopPath := path.Join(hookPath, "poststop")
err = os.Mkdir(poststopPath, 0750)
assert.NoError(err)

dirPath := path.Join(poststopPath, "directory")
err = os.Mkdir(dirPath, 0750)
assert.NoError(err)

normalPath := path.Join(poststopPath, "normalfile")
f, err := os.OpenFile(normalPath, os.O_RDONLY|os.O_CREATE, 0640)
assert.NoError(err)
f.Close()

symlinkPath := path.Join(poststopPath, "symlink")
err = os.Link(normalPath, symlinkPath)
assert.NoError(err)

s := &sandbox{
guestHooks: &specs.Hooks{},
guestHooksPresent: false,
}

s.scanGuestHooks(hookPath)
assert.False(s.guestHooksPresent)

spec := &specs.Spec{}
s.addGuestHooks(spec)
assert.True(len(spec.Hooks.Poststop) == 0)

execPath := path.Join(poststopPath, "executable")
f, err = os.OpenFile(execPath, os.O_RDONLY|os.O_CREATE, 0750)
assert.NoError(err)
f.Close()

s.scanGuestHooks(hookPath)
assert.True(s.guestHooksPresent)

s.addGuestHooks(spec)
assert.True(len(spec.Hooks.Poststop) == 1)
assert.True(strings.Contains(spec.Hooks.Poststop[0].Path, "executable"))
}
81 changes: 55 additions & 26 deletions grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,35 @@ func (a *agentGRPC) rollbackFailingContainerCreation(ctr *container) {
}
}

func (a *agentGRPC) finishCreateContainer(ctr *container, req *pb.CreateContainerRequest, config *configs.Config) (resp *gpb.Empty, err error) {
containerPath := filepath.Join("/tmp/libcontainer", a.sandbox.id)
factory, err := libcontainer.New(containerPath, libcontainer.Cgroupfs)
if err != nil {
return emptyResp, err
}

ctr.container, err = factory.Create(req.ContainerId, config)
if err != nil {
return emptyResp, err
}
ctr.config = *config

ctr.initProcess, err = buildProcess(req.OCI.Process, req.ExecId)
if err != nil {
return emptyResp, err
}

if err = a.execProcess(ctr, ctr.initProcess, true); err != nil {
return emptyResp, err
}

if err := a.updateSharedPidNs(ctr); err != nil {
return emptyResp, err
}

return emptyResp, a.postExecProcess(ctr, ctr.initProcess)
}

func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainerRequest) (resp *gpb.Empty, err error) {
if err := a.createContainerChecks(req); err != nil {
return emptyResp, err
Expand Down Expand Up @@ -591,6 +620,25 @@ func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainer
return emptyResp, err
}

if a.sandbox.guestHooksPresent {
// Add any custom OCI hooks to the spec
a.sandbox.addGuestHooks(ociSpec)

// write the OCI spec to a file so that hooks can read it
err = writeSpecToFile(ociSpec)
if err != nil {
return emptyResp, err
}

// Change cwd because libcontainer assumes the bundle path is the cwd:
// https://github.com/opencontainers/runc/blob/v1.0.0-rc5/libcontainer/specconv/spec_linux.go#L157
oldcwd, err := changeToBundlePath(ociSpec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that the code piece works because golang sets CLONE_FS when creating new M (https://github.com/golang/go/blob/2595fe7fb6f272f9204ca3ef0b0c55e66fb8d90f/src/runtime/os_linux.go#L131) so CWD is shared among all os threads.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that the code piece works because golang sets CLONE_FS when creating new M (https://github.com/golang/go/blob/2595fe7fb6f272f9204ca3ef0b0c55e66fb8d90f/src/runtime/os_linux.go#L131) so CWD is shared among all os threads. We rely on this model because the goroutine might be rescheduled to another os thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like an implementation detail. The runtime could very well store the cwd for each goroutine and then restore it when the goroutine is re-scheduled anywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flx42 I mentioned it because we do depend on the runtime implementation details here. If the Go runtime decides that CWD is an M local property, we are doomed because a goroutine can be rescheduled to another os thread since we do not lock os thread here, and then CWD is messed up in different threads.

if err != nil {
return emptyResp, err
}
defer os.Chdir(oldcwd)
}

// Convert the OCI specification into a libcontainer configuration.
config, err := specconv.CreateLibcontainerConfig(&specconv.CreateOpts{
CgroupName: req.ContainerId,
Expand All @@ -608,32 +656,7 @@ func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainer
return emptyResp, err
}

containerPath := filepath.Join("/tmp/libcontainer", a.sandbox.id)
factory, err := libcontainer.New(containerPath, libcontainer.Cgroupfs)
if err != nil {
return emptyResp, err
}

ctr.container, err = factory.Create(req.ContainerId, config)
if err != nil {
return emptyResp, err
}
ctr.config = *config

ctr.initProcess, err = buildProcess(req.OCI.Process, req.ExecId)
if err != nil {
return emptyResp, err
}

if err = a.execProcess(ctr, ctr.initProcess, true); err != nil {
return emptyResp, err
}

if err := a.updateSharedPidNs(ctr); err != nil {
return emptyResp, err
}

return emptyResp, a.postExecProcess(ctr, ctr.initProcess)
return a.finishCreateContainer(ctr, req, config)
}

func (a *agentGRPC) createContainerChecks(req *pb.CreateContainerRequest) (err error) {
Expand Down Expand Up @@ -1193,6 +1216,12 @@ func (a *agentGRPC) CreateSandbox(ctx context.Context, req *pb.CreateSandboxRequ
a.sandbox.running = true
a.sandbox.sandboxPidNs = req.SandboxPidns
a.sandbox.storages = make(map[string]*sandboxStorage)
a.sandbox.guestHooks = &specs.Hooks{}
a.sandbox.guestHooksPresent = false

if req.GuestHookPath != "" {
a.sandbox.scanGuestHooks(req.GuestHookPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is still silently ignored. I wonder how a user can find it out later on if scanGuestHooks fails here. I think we should at least report it back in CreateSandboxResponse and runtime can decide if it wants to bail out or continue using the hooks-missing image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Through the logs:
https://github.com/kata-containers/agent/pull/393/files/980023ec62b3144ef9f333209290c6215e295538#diff-2fe34e1b6f7f9aece0af74b1635502eeR259
I thought we agreed that it was fine to just log it for now. But I'm happy to change it if we have consensus.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am leaning towards just logging this for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

}

if req.SandboxId != "" {
a.sandbox.id = req.SandboxId
Expand Down
112 changes: 112 additions & 0 deletions oci.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
//
// Copyright (c) 2018 NVIDIA CORPORATION
//
// SPDX-License-Identifier: Apache-2.0
//

package main

import (
"encoding/json"
"errors"
"io/ioutil"
"os"
"path"
"path/filepath"

"github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
)

// OCI config file
const (
ociConfigFile string = "config.json"
ociConfigFileMode os.FileMode = 0444
)

// writeSpecToFile writes the container's OCI spec to "dirname(spec.Root.Path)/config.json"
// This effectively makes the parent directory a valid OCI bundle.
func writeSpecToFile(spec *specs.Spec) error {
bundlePath := filepath.Dir(spec.Root.Path)
configPath := filepath.Join(bundlePath, ociConfigFile)
f, err := os.OpenFile(configPath, os.O_WRONLY|os.O_CREATE, ociConfigFileMode)
if err != nil {
return err
}
defer f.Close()

return json.NewEncoder(f).Encode(spec)
}

// changeToBundlePath changes the cwd to the OCI bundle path defined as
// dirname(spec.Root.Path) and returns the old cwd.
func changeToBundlePath(spec *specs.Spec) (string, error) {
cwd, err := os.Getwd()
if err != nil {
return cwd, err
}

if spec == nil || spec.Root == nil || spec.Root.Path == "" {
return cwd, errors.New("invalid OCI spec")
}

bundlePath := filepath.Dir(spec.Root.Path)
configPath := filepath.Join(bundlePath, ociConfigFile)

// Verify that config.json is present at the root of the bundle path.
if _, err := os.Stat(configPath); err != nil {
return cwd, errors.New("invalid OCI bundle")
}

return cwd, os.Chdir(bundlePath)
}

func isValidHook(file os.FileInfo) (bool, error) {
if file.IsDir() {
return false, errors.New("is a directory")
}

mode := file.Mode()
if (mode & os.ModeSymlink) != 0 {
return false, errors.New("is a symbolic link")
}

perm := mode & os.ModePerm
if (perm & 0111) == 0 {
return false, errors.New("is not executable")
}

return true, nil
}

// findHooks searches guestHookPath for any OCI hooks for a given hookType
func findHooks(guestHookPath, hookType string) (hooksFound []specs.Hook) {
hooksPath := path.Join(guestHookPath, hookType)

files, err := ioutil.ReadDir(hooksPath)
if err != nil {
agentLog.WithError(err).WithField("oci-hook-type", hookType).Info("Skipping hook type")
return
}

for _, file := range files {
name := file.Name()
if ok, err := isValidHook(file); !ok {
agentLog.WithError(err).WithField("oci-hook-name", name).Warn("Skipping hook")
continue
}

agentLog.WithFields(logrus.Fields{
"oci-hook-name": name,
"oci-hook-type": hookType,
}).Info("Adding hook")
hooksFound = append(hooksFound, specs.Hook{
Path: path.Join(hooksPath, name),
Args: []string{name, hookType},
})
}

agentLog.WithField("oci-hook-type", hookType).Infof("Added %d hooks", len(hooksFound))

return
}
Loading