Skip to content

Commit 70839a3

Browse files
ambarveambarve
committed
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 <[email protected]> (cherry picked from commit bfde58e) Signed-off-by: Amit Barve <[email protected]>
1 parent ec44f6b commit 70839a3

2 files changed

Lines changed: 113 additions & 46 deletions

File tree

‎pkg/cri/opts/spec_windows.go‎

Lines changed: 59 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,61 @@ func cleanMount(p string) string {
6161
return filepath.Clean(p)
6262
}
6363

64+
func parseMount(osi osinterface.OS, mount *runtime.Mount) (*runtimespec.Mount, error) {
65+
var (
66+
dst = mount.GetContainerPath()
67+
src = mount.GetHostPath()
68+
)
69+
// In the case of a named pipe mount on Windows, don't stat the file
70+
// or do other operations that open it, as that could interfere with
71+
// the listening process. filepath.Clean also breaks named pipe
72+
// paths, so don't use it.
73+
if !namedPipePath(src) {
74+
if _, err := osi.Stat(src); err != nil {
75+
// Create the host path if it doesn't exist. This will align
76+
// the behavior with the Linux implementation, but it doesn't
77+
// align with Docker's behavior on Windows.
78+
if !os.IsNotExist(err) {
79+
return nil, fmt.Errorf("failed to stat %q: %w", src, err)
80+
}
81+
if err := osi.MkdirAll(src, 0755); err != nil {
82+
return nil, fmt.Errorf("failed to mkdir %q: %w", src, err)
83+
}
84+
}
85+
var err error
86+
src, err = osi.ResolveSymbolicLink(src)
87+
if err != nil {
88+
return nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err)
89+
}
90+
// hcsshim requires clean path, especially '/' -> '\'. Additionally,
91+
// for the destination, absolute paths should have the C: prefix.
92+
src = filepath.Clean(src)
93+
94+
// filepath.Clean adds a '.' at the end if the path is a
95+
// drive (like Z:, E: etc.). Keeping this '.' in the path
96+
// causes incorrect parameter error when starting the
97+
// container on windows. Remove it here.
98+
if !(len(dst) == 2 && dst[1] == ':') {
99+
dst = filepath.Clean(dst)
100+
if dst[0] == '\\' {
101+
dst = "C:" + dst
102+
}
103+
} else if dst[0] == 'c' || dst[0] == 'C' {
104+
return nil, fmt.Errorf("destination path can not be C drive")
105+
}
106+
}
107+
108+
var options []string
109+
// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
110+
// is readonly. This is different from docker's behavior, but make more sense.
111+
if mount.GetReadonly() {
112+
options = append(options, "ro")
113+
} else {
114+
options = append(options, "rw")
115+
}
116+
return &runtimespec.Mount{Source: src, Destination: dst, Options: options}, nil
117+
}
118+
64119
// WithWindowsMounts sorts and adds runtime and CRI mounts to the spec for
65120
// windows container.
66121
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
110165
}
111166

112167
for _, mount := range mounts {
113-
var (
114-
dst = mount.GetContainerPath()
115-
src = mount.GetHostPath()
116-
)
117-
// In the case of a named pipe mount on Windows, don't stat the file
118-
// or do other operations that open it, as that could interfere with
119-
// the listening process. filepath.Clean also breaks named pipe
120-
// paths, so don't use it.
121-
if !namedPipePath(src) {
122-
if _, err := osi.Stat(src); err != nil {
123-
// Create the host path if it doesn't exist. This will align
124-
// the behavior with the Linux implementation, but it doesn't
125-
// align with Docker's behavior on Windows.
126-
if !os.IsNotExist(err) {
127-
return fmt.Errorf("failed to stat %q: %w", src, err)
128-
}
129-
if err := osi.MkdirAll(src, 0755); err != nil {
130-
return fmt.Errorf("failed to mkdir %q: %w", src, err)
131-
}
132-
}
133-
var err error
134-
src, err = osi.ResolveSymbolicLink(src)
135-
if err != nil {
136-
return fmt.Errorf("failed to resolve symlink %q: %w", src, err)
137-
}
138-
// hcsshim requires clean path, especially '/' -> '\'. Additionally,
139-
// for the destination, absolute paths should have the C: prefix.
140-
src = filepath.Clean(src)
141-
dst = filepath.Clean(dst)
142-
if dst[0] == '\\' {
143-
dst = "C:" + dst
144-
}
145-
}
146-
147-
var options []string
148-
// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
149-
// is readonly. This is different from docker's behavior, but make more sense.
150-
if mount.GetReadonly() {
151-
options = append(options, "ro")
152-
} else {
153-
options = append(options, "rw")
168+
parsedMount, err := parseMount(osi, mount)
169+
if err != nil {
170+
return err
154171
}
155-
s.Mounts = append(s.Mounts, runtimespec.Mount{
156-
Source: src,
157-
Destination: dst,
158-
Options: options,
159-
})
172+
s.Mounts = append(s.Mounts, *parsedMount)
160173
}
161174
return nil
162175
}

‎pkg/cri/opts/spec_windows_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package opts
18+
19+
import (
20+
"fmt"
21+
"strings"
22+
"testing"
23+
24+
osinterface "github.com/containerd/containerd/pkg/os"
25+
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
26+
)
27+
28+
func TestDriveMounts(t *testing.T) {
29+
tests := []struct {
30+
mnt *runtime.Mount
31+
expectedContainerPath string
32+
expectedError error
33+
}{
34+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\foo`}, `D:\foo`, nil},
35+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\`}, `D:\`, nil},
36+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:`}, `D:`, nil},
37+
{&runtime.Mount{HostPath: `\\.\pipe\a_fake_pipe_name_that_shouldnt_exist`, ContainerPath: `\\.\pipe\foo`}, `\\.\pipe\foo`, nil},
38+
// If `C:\` is passed as container path it should continue and forward that to HCS and fail
39+
// to align with docker's behavior.
40+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:\`}, `C:\`, nil},
41+
42+
// If `C:` is passed we can detect and fail immediately.
43+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:`}, ``, fmt.Errorf("destination path can not be C drive")},
44+
}
45+
var realOS osinterface.RealOS
46+
for _, test := range tests {
47+
parsedMount, err := parseMount(realOS, test.mnt)
48+
if err != nil && !strings.EqualFold(err.Error(), test.expectedError.Error()) {
49+
t.Fatalf("expected err: %s, got %s instead", test.expectedError, err)
50+
} else if err == nil && test.expectedContainerPath != parsedMount.Destination {
51+
t.Fatalf("expected container path: %s, got %s instead", test.expectedContainerPath, parsedMount.Destination)
52+
}
53+
}
54+
}

0 commit comments

Comments
 (0)