diff --git a/agent/service.go b/agent/service.go index 517a1bcd8..ad452faf8 100644 --- a/agent/service.go +++ b/agent/service.go @@ -156,7 +156,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas err = bundleDir.MountRootfs(drive.Path(), "ext4", nil) if err != nil { - return nil, errors.Wrap(err, "failed to mount rootfs") + return nil, errors.Wrapf(err, "failed to mount rootfs %q", drive.Path()) } req.Bundle = bundleDir.RootPath() diff --git a/firecracker-control/local.go b/firecracker-control/local.go index 0c1559d0e..bff365f27 100644 --- a/firecracker-control/local.go +++ b/firecracker-control/local.go @@ -116,8 +116,14 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest) // If we're here, there is no pre-existing shim for this VMID, so we spawn a new one defer shimSocket.Close() - shimDir := vm.ShimDir(ns, id) - err = shimDir.Create() + shimDir, err := vm.ShimDir(ns, id) + if err != nil { + err = errors.Wrapf(err, "failed to build shim path") + s.logger.WithError(err).Error() + return nil, err + } + + err = shimDir.Mkdir() if err != nil { err = errors.Wrapf(err, "failed to create VM dir %q", shimDir.RootPath()) s.logger.WithError(err).Error() @@ -266,7 +272,14 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix cmd := exec.Command(internal.ShimBinaryName, args...) - cmd.Dir = vm.ShimDir(ns, vmID).RootPath() + shimDir, err := vm.ShimDir(ns, vmID) + if err != nil { + err = errors.Wrap(err, "failed to create shim dir") + logger.WithError(err).Error() + return nil, err + } + + cmd.Dir = shimDir.RootPath() shimSocketFile, err := shimSocket.File() if err != nil { diff --git a/internal/vm/dir.go b/internal/vm/dir.go index 4ff33d759..94bb1d20a 100644 --- a/internal/vm/dir.go +++ b/internal/vm/dir.go @@ -19,6 +19,7 @@ import ( "os" "path/filepath" + "github.com/containerd/containerd/identifiers" "github.com/containerd/containerd/runtime/v2/shim" "github.com/containerd/fifo" "github.com/firecracker-microvm/firecracker-containerd/internal" @@ -33,8 +34,16 @@ const ( // ShimDir holds files, sockets and FIFOs scoped to a single shim managing the // VM with the given VMID. It is unique per-VM and containerd namespace. -func ShimDir(namespace, vmID string) Dir { - return Dir(filepath.Join(varRunDir, namespace, vmID)) +func ShimDir(namespace, vmID string) (Dir, error) { + if err := identifiers.Validate(namespace); err != nil { + return "", errors.Wrap(err, "invalid namespace") + } + + if err := identifiers.Validate(vmID); err != nil { + return "", errors.Wrap(err, "invalid vm id") + } + + return Dir(filepath.Join(varRunDir, namespace, vmID)), nil } // Dir represents the root of a firecracker-containerd VM directory, which @@ -46,9 +55,9 @@ func (d Dir) RootPath() string { return string(d) } -// Create will mkdir the RootPath with correct permissions, or no-op if it +// Mkdir will mkdir the RootPath with correct permissions, or no-op if it // already exists -func (d Dir) Create() error { +func (d Dir) Mkdir() error { return os.MkdirAll(d.RootPath(), 0700) } @@ -88,26 +97,45 @@ func (d Dir) FirecrackerMetricsFifoPath() string { // BundleLink returns the path to the symlink to the bundle dir for a given container running // inside the VM of this vm dir. -func (d Dir) BundleLink(containerID string) bundle.Dir { - return bundle.Dir(filepath.Join(d.RootPath(), containerID)) +func (d Dir) BundleLink(containerID string) (bundle.Dir, error) { + if err := identifiers.Validate(containerID); err != nil { + return "", errors.Wrapf(err, "invalid container id %q", containerID) + } + + return bundle.Dir(filepath.Join(d.RootPath(), containerID)), nil } // CreateBundleLink creates the BundleLink by symlinking to the provided bundle dir func (d Dir) CreateBundleLink(containerID string, bundleDir bundle.Dir) error { - return createSymlink(bundleDir.RootPath(), d.BundleLink(containerID).RootPath(), "bundle") + path, err := d.BundleLink(containerID) + if err != nil { + return err + } + + return createSymlink(bundleDir.RootPath(), path.RootPath(), "bundle") } // CreateAddressLink creates a symlink from the VM dir to the bundle dir for the shim address file. // This symlink is read by containerd. // CreateAddressLink assumes that CreateBundleLink has been called. func (d Dir) CreateAddressLink(containerID string) error { - return createSymlink(d.AddrFilePath(), d.BundleLink(containerID).AddrFilePath(), "shim address file") + path, err := d.BundleLink(containerID) + if err != nil { + return err + } + + return createSymlink(d.AddrFilePath(), path.AddrFilePath(), "shim address file") } // CreateShimLogFifoLink creates a symlink from the bundleDir of the provided container. // CreateAddressLink assumes that CreateBundleLink has been called. func (d Dir) CreateShimLogFifoLink(containerID string) error { - return createSymlink(d.BundleLink(containerID).LogFifoPath(), d.LogFifoPath(), "shim log fifo") + path, err := d.BundleLink(containerID) + if err != nil { + return err + } + + return createSymlink(path.LogFifoPath(), d.LogFifoPath(), "shim log fifo") } // WriteAddress will write the actual address file in the VM dir. diff --git a/internal/vm/dir_test.go b/internal/vm/dir_test.go new file mode 100644 index 000000000..7cdef40b1 --- /dev/null +++ b/internal/vm/dir_test.go @@ -0,0 +1,86 @@ +// Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package vm + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +var invalidContainerIDs = []string{"", "id?", "*", "id/1", "id\\"} + +func TestShimDir(t *testing.T) { + tests := []struct { + name string + ns string + id string + outDir string + outErr string + }{ + {name: "empty ns", outErr: "invalid namespace: identifier must not be empty"}, + {name: "ns with /", ns: "/", outErr: `invalid namespace: identifier "/" must match`}, + {name: "ns with ?", ns: "?", outErr: `invalid namespace: identifier "?" must match`}, + {name: "ns with *", ns: "*", outErr: `invalid namespace: identifier "*" must match`}, + {name: "ns with ,", ns: ",", outErr: `invalid namespace: identifier "," must match`}, + {name: "empty id", ns: "test", outErr: "invalid vm id: identifier must not be empty"}, + {name: "id with /", ns: "test", id: "/", outErr: `invalid vm id: identifier "/" must match`}, + {name: "id with ?", ns: "test", id: "?", outErr: `invalid vm id: identifier "?" must match`}, + {name: "id with *", ns: "test", id: "*", outErr: `invalid vm id: identifier "*" must match`}, + {name: "id with ,", ns: "test", id: ",", outErr: `invalid vm id: identifier "," must match`}, + {name: "valid", ns: "ns", id: "1", outDir: "/var/run/firecracker-containerd/ns/1"}, + {name: "valid with dashes", ns: "test-123", id: "123-456", outDir: "/var/run/firecracker-containerd/test-123/123-456"}, + {name: "valid with dots", ns: "test.123", id: "123.456", outDir: "/var/run/firecracker-containerd/test.123/123.456"}, + } + + for _, tc := range tests { + test := tc + t.Run(test.name, func(t *testing.T) { + dir, err := ShimDir(test.ns, test.id) + + if test.outErr != "" { + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), test.outErr), err.Error()) + } else { + assert.NoError(t, err) + assert.EqualValues(t, dir, test.outDir) + } + }) + } +} + +func TestBundleLink(t *testing.T) { + var root Dir = "/root/" + + dir, err := root.BundleLink("1") + assert.NoError(t, err) + assert.EqualValues(t, "/root/1", dir) + + dir, err = root.BundleLink("test-1") + assert.NoError(t, err) + assert.EqualValues(t, "/root/test-1", dir) +} + +func TestBundleLinkInvalidID(t *testing.T) { + var ( + root Dir = "/root/" + err error + ) + + for _, id := range invalidContainerIDs { + _, err = root.BundleLink(id) + assert.Error(t, err) + } +} diff --git a/runtime/service.go b/runtime/service.go index 0ce93b1f1..2342bed03 100644 --- a/runtime/service.go +++ b/runtime/service.go @@ -113,7 +113,9 @@ type service struct { shimCtx context.Context shimCancel func() - vmID string + vmID string + shimDir vm.Dir + config *Config // vmReady is closed once CreateVM has been successfully called @@ -161,10 +163,16 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish namespace = namespaces.Default } + var shimDir vm.Dir vmID := os.Getenv(internal.VMIDEnvVarKey) logger := log.G(shimCtx) if vmID != "" { logger = logger.WithField("vmID", vmID) + + shimDir, err = vm.ShimDir(namespace, vmID) + if err != nil { + return nil, errors.Wrap(err, "invalid shim directory") + } } s := &service{ @@ -176,13 +184,15 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish shimCtx: shimCtx, shimCancel: shimCancel, - vmID: vmID, + vmID: vmID, + shimDir: shimDir, + config: config, vmReady: make(chan struct{}), } - s.stubDriveHandler = newStubDriveHandler(s.shimDir().RootPath(), logger) + s.stubDriveHandler = newStubDriveHandler(s.shimDir.RootPath(), logger) s.startEventForwarders(remotePublisher) err = s.serveFCControl() @@ -453,7 +463,7 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe cmd := firecracker.VMCommandBuilder{}. WithBin(s.config.FirecrackerBinaryPath). - WithSocketPath(s.shimDir().FirecrackerSockPath()). + WithSocketPath(s.shimDir.FirecrackerSockPath()). Build(s.shimCtx) // shimCtx so the VM process is only killed when the shim shuts down // use shimCtx so the VM is killed when the shim shuts down @@ -563,18 +573,14 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM return &empty.Empty{}, nil } -func (s *service) shimDir() vm.Dir { - return vm.ShimDir(s.namespace, s.vmID) -} - func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) { logger := s.logger.WithField("cid", s.machineCID) cfg := firecracker.Config{ - SocketPath: s.shimDir().FirecrackerSockPath(), + SocketPath: s.shimDir.FirecrackerSockPath(), VsockDevices: []firecracker.VsockDevice{{Path: "root", CID: s.machineCID}}, - LogFifo: s.shimDir().FirecrackerLogFifoPath(), - MetricsFifo: s.shimDir().FirecrackerMetricsFifoPath(), + LogFifo: s.shimDir.FirecrackerLogFifoPath(), + MetricsFifo: s.shimDir.FirecrackerMetricsFifoPath(), MachineCfg: machineConfigurationFromProto(s.config, req.MachineCfg), } @@ -662,14 +668,14 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask }).Debug("creating task") bundleDir := bundle.Dir(request.Bundle) - err = s.shimDir().CreateBundleLink(request.ID, bundleDir) + err = s.shimDir.CreateBundleLink(request.ID, bundleDir) if err != nil { err = errors.Wrap(err, "failed to create VM dir bundle link") logger.WithError(err).Error() return nil, err } - err = s.shimDir().CreateAddressLink(request.ID) + err = s.shimDir.CreateAddressLink(request.ID) if err != nil { err = errors.Wrap(err, "failed to create shim address symlink") logger.WithError(err).Error() @@ -983,9 +989,9 @@ func (s *service) Shutdown(requestCtx context.Context, req *taskAPI.ShutdownRequ shutdownErr = multierror.Append(shutdownErr, errors.Wrap(err, "failed to gracefully stop VM")) } - err = os.RemoveAll(s.shimDir().RootPath()) + err = os.RemoveAll(s.shimDir.RootPath()) if err != nil { - shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir().RootPath())) + shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir.RootPath())) } if shutdownErr != nil { diff --git a/runtime/service_test.go b/runtime/service_test.go index 8b711980e..3d8369421 100644 --- a/runtime/service_test.go +++ b/runtime/service_test.go @@ -186,10 +186,10 @@ func TestBuildVMConfiguration(t *testing.T) { svc.stubDriveHandler = newStubDriveHandler(tempDir, svc.logger) // For values that remain constant between tests, they are written here - tc.expectedCfg.SocketPath = svc.shimDir().FirecrackerSockPath() + tc.expectedCfg.SocketPath = svc.shimDir.FirecrackerSockPath() tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{Path: "root", CID: svc.machineCID}} - tc.expectedCfg.LogFifo = svc.shimDir().FirecrackerLogFifoPath() - tc.expectedCfg.MetricsFifo = svc.shimDir().FirecrackerMetricsFifoPath() + tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath() + tc.expectedCfg.MetricsFifo = svc.shimDir.FirecrackerMetricsFifoPath() tc.expectedCfg.Drives[0].PathOnHost = firecracker.String(filepath.Join(tempDir, "stub0")) actualCfg, err := svc.buildVMConfiguration(tc.request)