From 70839a344033dbb86ae152b30a747f9258527c9e Mon Sep 17 00:00:00 2001 From: Amit Barve Date: Wed, 11 May 2022 08:19:39 -0700 Subject: [PATCH] Bug fix for mount path handling Currently when handling 'container_path' elements in container mounts we simply call filepath.Clean on those paths. However, filepath.Clean adds an extra '.' if the path is a simple drive letter ('E:' or 'Z:' etc.). These type of paths cause failures (with incorrect parameter error) when creating containers via hcsshim. This commit checks for such paths and doesn't call filepath.Clean on them. It also adds a new check to error out if the destination path is a C drive and moves the dst path checks out of the named pipe condition. Signed-off-by: Amit Barve (cherry picked from commit bfde58e3cdc0d16b24342886db319a2a2b0964b9) Signed-off-by: Amit Barve --- pkg/cri/opts/spec_windows.go | 105 +++++++++++++++++------------- pkg/cri/opts/spec_windows_test.go | 54 +++++++++++++++ 2 files changed, 113 insertions(+), 46 deletions(-) create mode 100644 pkg/cri/opts/spec_windows_test.go diff --git a/pkg/cri/opts/spec_windows.go b/pkg/cri/opts/spec_windows.go index c534c180c475..559657c4a851 100644 --- a/pkg/cri/opts/spec_windows.go +++ b/pkg/cri/opts/spec_windows.go @@ -61,6 +61,61 @@ func cleanMount(p string) string { return filepath.Clean(p) } +func parseMount(osi osinterface.OS, mount *runtime.Mount) (*runtimespec.Mount, error) { + var ( + dst = mount.GetContainerPath() + src = mount.GetHostPath() + ) + // In the case of a named pipe mount on Windows, don't stat the file + // or do other operations that open it, as that could interfere with + // the listening process. filepath.Clean also breaks named pipe + // paths, so don't use it. + if !namedPipePath(src) { + if _, err := osi.Stat(src); err != nil { + // Create the host path if it doesn't exist. This will align + // the behavior with the Linux implementation, but it doesn't + // align with Docker's behavior on Windows. + if !os.IsNotExist(err) { + return nil, fmt.Errorf("failed to stat %q: %w", src, err) + } + if err := osi.MkdirAll(src, 0755); err != nil { + return nil, fmt.Errorf("failed to mkdir %q: %w", src, err) + } + } + var err error + src, err = osi.ResolveSymbolicLink(src) + if err != nil { + return nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err) + } + // hcsshim requires clean path, especially '/' -> '\'. Additionally, + // for the destination, absolute paths should have the C: prefix. + src = filepath.Clean(src) + + // filepath.Clean adds a '.' at the end if the path is a + // drive (like Z:, E: etc.). Keeping this '.' in the path + // causes incorrect parameter error when starting the + // container on windows. Remove it here. + if !(len(dst) == 2 && dst[1] == ':') { + dst = filepath.Clean(dst) + if dst[0] == '\\' { + dst = "C:" + dst + } + } else if dst[0] == 'c' || dst[0] == 'C' { + return nil, fmt.Errorf("destination path can not be C drive") + } + } + + var options []string + // NOTE(random-liu): we don't change all mounts to `ro` when root filesystem + // is readonly. This is different from docker's behavior, but make more sense. + if mount.GetReadonly() { + options = append(options, "ro") + } else { + options = append(options, "rw") + } + return &runtimespec.Mount{Source: src, Destination: dst, Options: options}, nil +} + // WithWindowsMounts sorts and adds runtime and CRI mounts to the spec for // windows container. func WithWindowsMounts(osi osinterface.OS, config *runtime.ContainerConfig, extra []*runtime.Mount) oci.SpecOpts { @@ -110,53 +165,11 @@ func WithWindowsMounts(osi osinterface.OS, config *runtime.ContainerConfig, extr } for _, mount := range mounts { - var ( - dst = mount.GetContainerPath() - src = mount.GetHostPath() - ) - // In the case of a named pipe mount on Windows, don't stat the file - // or do other operations that open it, as that could interfere with - // the listening process. filepath.Clean also breaks named pipe - // paths, so don't use it. - if !namedPipePath(src) { - if _, err := osi.Stat(src); err != nil { - // Create the host path if it doesn't exist. This will align - // the behavior with the Linux implementation, but it doesn't - // align with Docker's behavior on Windows. - if !os.IsNotExist(err) { - return fmt.Errorf("failed to stat %q: %w", src, err) - } - if err := osi.MkdirAll(src, 0755); err != nil { - return fmt.Errorf("failed to mkdir %q: %w", src, err) - } - } - var err error - src, err = osi.ResolveSymbolicLink(src) - if err != nil { - return fmt.Errorf("failed to resolve symlink %q: %w", src, err) - } - // hcsshim requires clean path, especially '/' -> '\'. Additionally, - // for the destination, absolute paths should have the C: prefix. - src = filepath.Clean(src) - dst = filepath.Clean(dst) - if dst[0] == '\\' { - dst = "C:" + dst - } - } - - var options []string - // NOTE(random-liu): we don't change all mounts to `ro` when root filesystem - // is readonly. This is different from docker's behavior, but make more sense. - if mount.GetReadonly() { - options = append(options, "ro") - } else { - options = append(options, "rw") + parsedMount, err := parseMount(osi, mount) + if err != nil { + return err } - s.Mounts = append(s.Mounts, runtimespec.Mount{ - Source: src, - Destination: dst, - Options: options, - }) + s.Mounts = append(s.Mounts, *parsedMount) } return nil } diff --git a/pkg/cri/opts/spec_windows_test.go b/pkg/cri/opts/spec_windows_test.go new file mode 100644 index 000000000000..a2beb6ec2083 --- /dev/null +++ b/pkg/cri/opts/spec_windows_test.go @@ -0,0 +1,54 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License 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 opts + +import ( + "fmt" + "strings" + "testing" + + osinterface "github.com/containerd/containerd/pkg/os" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" +) + +func TestDriveMounts(t *testing.T) { + tests := []struct { + mnt *runtime.Mount + expectedContainerPath string + expectedError error + }{ + {&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\foo`}, `D:\foo`, nil}, + {&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\`}, `D:\`, nil}, + {&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:`}, `D:`, nil}, + {&runtime.Mount{HostPath: `\\.\pipe\a_fake_pipe_name_that_shouldnt_exist`, ContainerPath: `\\.\pipe\foo`}, `\\.\pipe\foo`, nil}, + // If `C:\` is passed as container path it should continue and forward that to HCS and fail + // to align with docker's behavior. + {&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:\`}, `C:\`, nil}, + + // If `C:` is passed we can detect and fail immediately. + {&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:`}, ``, fmt.Errorf("destination path can not be C drive")}, + } + var realOS osinterface.RealOS + for _, test := range tests { + parsedMount, err := parseMount(realOS, test.mnt) + if err != nil && !strings.EqualFold(err.Error(), test.expectedError.Error()) { + t.Fatalf("expected err: %s, got %s instead", test.expectedError, err) + } else if err == nil && test.expectedContainerPath != parsedMount.Destination { + t.Fatalf("expected container path: %s, got %s instead", test.expectedContainerPath, parsedMount.Destination) + } + } +}