Skip to content

Commit f2312e5

Browse files
sipsmasipsma
committed
fixup! Support DriveMount API in CreateVM.
Signed-off-by: Erik Sipsma <[email protected]>
1 parent 51b4aa1 commit f2312e5

8 files changed

+177
-25
lines changed

‎agent/drive_handler.go

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -166,13 +166,13 @@ func isStubDrive(d drive) bool {
166166
}
167167

168168
func (dh driveHandler) MountDrive(ctx context.Context, req *drivemount.MountDriveRequest) (*empty.Empty, error) {
169-
logger := log.G(ctx).WithField("MountDriveRequest", req.String())
170-
logger.Debug()
169+
logger := log.G(ctx)
170+
logger.Debugf("%+v", req.String())
171+
logger = logger.WithField("drive_id", req.DriveID)
171172

172-
driveID := strings.TrimSpace(req.DriveID)
173-
drive, ok := dh.GetDrive(driveID)
173+
drive, ok := dh.GetDrive(req.DriveID)
174174
if !ok {
175-
return nil, fmt.Errorf("Drive %q could not be found", driveID)
175+
return nil, fmt.Errorf("drive %q could not be found", req.DriveID)
176176
}
177177
logger = logger.WithField("drive_path", drive.Path())
178178

@@ -184,7 +184,12 @@ func (dh driveHandler) MountDrive(ctx context.Context, req *drivemount.MountDriv
184184
}
185185

186186
for _, systemDir := range bannedSystemDirs {
187-
if strings.HasPrefix(filepath.Clean(resolvedDest), filepath.Clean(systemDir)) {
187+
isUnder, err := isOrUnderDir(filepath.Clean(resolvedDest), filepath.Clean(systemDir))
188+
if err != nil {
189+
return nil, errors.Errorf("failed to check if %q is under dir %q", resolvedDest, systemDir)
190+
}
191+
192+
if isUnder {
188193
return nil, errors.Errorf(
189194
"drive mount destination %q resolves to path %q under banned system directory %q",
190195
req.DestinationPath, resolvedDest, systemDir,
@@ -229,6 +234,7 @@ func (dh driveHandler) MountDrive(ctx context.Context, req *drivemount.MountDriv
229234
// evalAnySymlinks is similar to filepath.EvalSymlinks, except it will not return an error if part of the
230235
// provided path does not exist. It will evaluate symlinks present in the path up to a component that doesn't
231236
// exist, at which point it will just append the rest of the provided path to what has been resolved so far.
237+
// We validate earlier that input to this function is an absolute path.
232238
func evalAnySymlinks(path string) (string, error) {
233239
curPath := "/"
234240
pathSplit := strings.Split(filepath.Clean(path), "/")
@@ -248,3 +254,13 @@ func evalAnySymlinks(path string) (string, error) {
248254

249255
return curPath, nil
250256
}
257+
258+
// returns whether the given path is the provided baseDir or is under it
259+
func isOrUnderDir(path, baseDir string) (bool, error) {
260+
relPath, err := filepath.Rel(baseDir, path)
261+
if err != nil {
262+
return false, err
263+
}
264+
265+
return !strings.HasPrefix(relPath, "../") && relPath != "..", nil
266+
}

‎agent/drive_handler_test.go

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,137 @@ func TestEvalAnySymlinks(t *testing.T) {
5757
require.NoError(t, err, "failed to get current working dir")
5858
assert.Equal(t, filepath.Join(cwd, nonExistentPath), resolvedPath)
5959
}
60+
61+
func TestIsOrUnderDir(t *testing.T) {
62+
type testcase struct {
63+
baseDir string
64+
path string
65+
expectedTrue bool
66+
expectedErr bool
67+
}
68+
69+
for _, tc := range []testcase{
70+
{
71+
baseDir: "/foo",
72+
path: "/foo/bar",
73+
expectedTrue: true,
74+
expectedErr: false,
75+
},
76+
{
77+
baseDir: "/foo/bar",
78+
path: "/foo/bar/baz",
79+
expectedTrue: true,
80+
expectedErr: false,
81+
},
82+
{
83+
baseDir: "/foo",
84+
path: "/foo",
85+
expectedTrue: true,
86+
expectedErr: false,
87+
},
88+
{
89+
baseDir: "/foo/bar",
90+
path: "/foo/bar",
91+
expectedTrue: true,
92+
expectedErr: false,
93+
},
94+
{
95+
baseDir: "/foo",
96+
path: "/foobar",
97+
expectedTrue: false,
98+
expectedErr: false,
99+
},
100+
{
101+
baseDir: "/foo",
102+
path: "/bar",
103+
expectedTrue: false,
104+
expectedErr: false,
105+
},
106+
{
107+
baseDir: "/foo/bar",
108+
path: "/bar",
109+
expectedTrue: false,
110+
expectedErr: false,
111+
},
112+
{
113+
baseDir: "/foo/bar",
114+
path: "/foo",
115+
expectedTrue: false,
116+
expectedErr: false,
117+
},
118+
{
119+
baseDir: "/foo/bar",
120+
path: "/bar/bar",
121+
expectedTrue: false,
122+
expectedErr: false,
123+
},
124+
{
125+
baseDir: "/foo",
126+
path: "foo",
127+
expectedTrue: false,
128+
expectedErr: true,
129+
},
130+
{
131+
baseDir: "/foo",
132+
path: "bar",
133+
expectedTrue: false,
134+
expectedErr: true,
135+
},
136+
{
137+
baseDir: "/foo",
138+
path: "/foo/../foo",
139+
expectedTrue: true,
140+
expectedErr: false,
141+
},
142+
{
143+
baseDir: "/foo/bar",
144+
path: "/foo/../foo/bar",
145+
expectedTrue: true,
146+
expectedErr: false,
147+
},
148+
{
149+
baseDir: "/foo",
150+
path: "/foo/../bar",
151+
expectedTrue: false,
152+
expectedErr: false,
153+
},
154+
{
155+
baseDir: "/foo",
156+
path: "/foo/..bar",
157+
expectedTrue: true,
158+
expectedErr: false,
159+
},
160+
{
161+
baseDir: "/foo",
162+
path: "/foo/..bar/baz",
163+
expectedTrue: true,
164+
expectedErr: false,
165+
},
166+
{
167+
baseDir: "/",
168+
path: "/",
169+
expectedTrue: true,
170+
expectedErr: false,
171+
},
172+
{
173+
baseDir: "/foo",
174+
path: "/",
175+
expectedTrue: false,
176+
expectedErr: false,
177+
},
178+
{
179+
baseDir: "/",
180+
path: "/foo",
181+
expectedTrue: true,
182+
expectedErr: false,
183+
},
184+
} {
185+
isUnder, err := isOrUnderDir(tc.path, tc.baseDir)
186+
assert.Equalf(t, tc.expectedTrue, isUnder, "unexpected output for isOrUnderDir case %+v", tc)
187+
if tc.expectedErr {
188+
assert.Errorf(t, err, "unexpected error from isOrUnderDir case %+v", tc)
189+
} else {
190+
assert.NoErrorf(t, err, "unexpected error from isOrUnderDir case %+v", tc)
191+
}
192+
}
193+
}

‎agent/service.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
146146
if err != nil {
147147
cleanupErr := ts.doCleanup(taskID, execID)
148148
if cleanupErr != nil {
149-
logger.WithError(err).Error("failed to cleanup task")
149+
logger.WithError(cleanupErr).Error("failed to cleanup task")
150150
}
151151
}
152152
}()
@@ -390,7 +390,7 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
390390
if err != nil {
391391
cleanupErr := ts.doCleanup(taskID, execID)
392392
if cleanupErr != nil {
393-
logger.WithError(err).Error("failed to cleanup task")
393+
logger.WithError(cleanupErr).Error("failed to cleanup task")
394394
}
395395
}
396396
}()

‎internal/fsutil.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,12 +90,14 @@ func ParseProcMountLines(lines ...string) ([]MountInfo, error) {
9090
}
9191

9292
// see man 5 fstab for the format of /proc/mounts
93-
var source string
94-
var dest string
95-
var fstype string
96-
var optionsStr string
97-
var dumpFreq int
98-
var passno int
93+
var (
94+
source string
95+
dest string
96+
fstype string
97+
optionsStr string
98+
dumpFreq int
99+
passno int
100+
)
99101
_, err := fmt.Sscanf(line, "%s %s %s %s %d %d", &source, &dest, &fstype, &optionsStr, &dumpFreq, &passno)
100102
if err != nil {
101103
return nil, errors.Wrapf(err, "failed to parse /proc/mount line %q", line)

‎runtime/drive_handler.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -168,13 +168,13 @@ func (h *stubDriveHandler) InDriveSet(path string) bool {
168168
}
169169

170170
// PatchStubDrive will replace the next available stub drive with the provided drive
171-
func (h *stubDriveHandler) PatchStubDrive(ctx context.Context, client firecracker.MachineIface, pathOnHost string) (*string, error) {
171+
func (h *stubDriveHandler) PatchStubDrive(ctx context.Context, client firecracker.MachineIface, pathOnHost string) (string, error) {
172172
h.mutex.Lock()
173173
defer h.mutex.Unlock()
174174

175175
// Check to see if stubDriveIndex has increased more than the drive amount.
176176
if h.stubDriveIndex >= int64(len(h.drives)) {
177-
return nil, ErrDrivesExhausted
177+
return "", ErrDrivesExhausted
178178
}
179179

180180
d := h.drives[h.stubDriveIndex]
@@ -183,16 +183,16 @@ func (h *stubDriveHandler) PatchStubDrive(ctx context.Context, client firecracke
183183
if d.DriveID == nil {
184184
// this should never happen, but we want to ensure that we never nil
185185
// dereference
186-
return nil, ErrDriveIDNil
186+
return "", ErrDriveIDNil
187187
}
188188

189189
h.drives[h.stubDriveIndex] = d
190190

191191
err := client.UpdateGuestDrive(ctx, firecracker.StringValue(d.DriveID), pathOnHost)
192192
if err != nil {
193-
return nil, err
193+
return "", err
194194
}
195195

196196
h.stubDriveIndex++
197-
return d.DriveID, nil
197+
return firecracker.StringValue(d.DriveID), nil
198198
}

‎runtime/drive_handler_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func TestPatchStubDrive(t *testing.T) {
9797
for i, path := range expectedReplacements {
9898
driveID, err := handler.PatchStubDrive(ctx, client, path)
9999
assert.NoError(t, err, "failed to patch stub drive")
100-
assert.Equal(t, expectedDriveIDs[i], firecracker.StringValue(driveID), "drive ids are not equal")
100+
assert.Equal(t, expectedDriveIDs[i], driveID, "drive ids are not equal")
101101
}
102102
}
103103

‎runtime/service.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,7 @@ func (s *service) mountDrives(requestCtx context.Context, driveMounts []*proto.F
521521
}
522522

523523
_, err = s.driveMountClient.MountDrive(requestCtx, &drivemount.MountDriveRequest{
524-
DriveID: *driveID,
524+
DriveID: driveID,
525525
DestinationPath: driveMount.VMPath,
526526
FilesytemType: driveMount.FilesystemType,
527527
Options: driveMount.Options,
@@ -753,9 +753,8 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
753753
return nil, err
754754
}
755755

756-
var driveID *string
757756
for _, mnt := range request.Rootfs {
758-
driveID, err = s.stubDriveHandler.PatchStubDrive(requestCtx, s.machine, mnt.Source)
757+
driveID, err := s.stubDriveHandler.PatchStubDrive(requestCtx, s.machine, mnt.Source)
759758
if err != nil {
760759
if err == ErrDrivesExhausted {
761760
return nil, errors.Wrapf(errdefs.ErrUnavailable, "no remaining stub drives to be used")
@@ -764,7 +763,7 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
764763
}
765764

766765
_, err = s.driveMountClient.MountDrive(requestCtx, &drivemount.MountDriveRequest{
767-
DriveID: *driveID,
766+
DriveID: driveID,
768767
DestinationPath: vmBundleDir.RootfsPath(),
769768
FilesytemType: "ext4",
770769
Options: nil,

‎runtime/service_integ_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -805,7 +805,8 @@ func TestDriveMount_Isolated(t *testing.T) {
805805
FSImgFile internal.FSImgFile
806806
}{
807807
{
808-
VMPath: "/adrivemnt",
808+
// /systemmount meant to make sure logic doesn't ban this just because it begins with /sys
809+
VMPath: "/systemmount",
809810
FilesystemType: "ext4",
810811
VMMountOptions: []string{"rw", "noatime"},
811812
ContainerPath: "/foo",

0 commit comments

Comments
 (0)