Windows: Ensure image VOLUME temp paths are well-formed.

[aznashwan]

Attempting to run Windows images with VOLUME declarations which are absolute paths would lead to securejoin.SecureJoin() concatenating two absolute paths when determining the host mount dir and then failing to os.Lstat() the resulting path, as it contained an illegal second :.

[aznashwan]

This should alleviate (but not fix) #759, as it enables nerdctl to at least reach the point where it sends the OCI spec over.

[fahedouch]

curious how docker treat this usecase. is it removing colons while while maining the guest path's drive letter ?

[aznashwan]

I am honestly unsure how Docker does it, at it looks like VOLUMEs are not working properly for WCOW, as the mounted volume in the container seems empty.

# Docker CE on Windows 10 in WCOW container mode.
# Image has a `VOLUME C:\test_dir`:
# https://github.com/containerd/containerd/blob/main/integration/images/volume-copy-up/Dockerfile_windows#L29-L37
PS C:\Users\Nashu> docker run  -ti ghcr.io/containerd/volume-copy-up:2.1 cmd /c "dir C:\test_dir"
 Volume in drive C has no label. 
 Volume Serial Number is 7AFF-76D2            
                                              
 Directory of C:\test_dir                     
                                              
# NOTE: should have contained a `test_file` with some `test_contents` in it too.
04/07/2023  06:05 AM    <DIR>          .      
04/07/2023  06:05 AM    <DIR>          ..     
               0 File(s)              0 bytes 
               2 Dir(s)  142,581,915,648 bytes free 

What I can say though is that this tempDir is created here and will have image layers mounted under their individual IDs in it.

In this sense, there should be no collisions when we create/mount the VOLUME based on its mountpoint within that tempDir with whatever might be in the image layers.

@gabriel-samfira did, rightfully point out the possibility of collisions when having things like VOLUME ["C:/test_dir", "/C/test_dir"], which are technically valid and are worth considering.

At this stage, I'm simply tempted to securejoin.SecureJoin(tempDir, anonVolName) as anonVolName is an ID generated right above the join, what do y'all think?

[TBBle]

It's true that Docker does not implement populating the temporary volume on Windows, see https://github.com/moby/moby/blob/dd3b71d17c614f837c4bba18baed9fa2cb31f1a4/daemon/create_windows.go#L42-L70. However, containerd-cri does; that implementation is currently faulty for non-C: drives, see containerd/containerd#8171 and proposed fix at containerd/containerd#8362.

What I can say though is that this tempDir is created here and will have image layers mounted under their individual IDs in it.

This code is separately faulty, I've gone into more detail in my review comment, but this loop produces mounts at paths that the rest of the code does not know about (i.e. not mounted at tempDir). It's also no longer necessary with recent improvements to the containerd code it's calling, as there'll only be one mount returned from s.View on Windows. See containerd/containerd@474a257#diff-2f84625f0f53cdff6461e071ffd95fba797e037a6195d2879ed9fd8bafdb7b5dL89-R97.

I don't think this ever worked, as it appears to have been copied over from containerd-cri (based on this comment which we've just resolved in containerd to unspecial Windows), but is missing the matching code around the secureJoin, which may have been added later (to fix the data copying) and as you can see has now been removed from containerd-cri as well.

At this stage, I'm simply tempted to securejoin.SecureJoin(tempDir, anonVolName) as anonVolName is an ID generated right above the join, what do y'all think?

This won't work, similar to the problem with the Windows mounting loop problem I mentioned above and in my review comment, it generates a source for the copy that cannot exist.

@gabriel-samfira did, rightfully point out the possibility of collisions when having things like VOLUME ["C:/test_dir", "/C/test_dir"], which are technically valid and are worth considering.

We can avoid this by adding entries to mounted after skipping seen volumes. I suspect the use of mounted also needs improvement as it relies on filepath.Clean and I don't think that's even sufficient to ensure a match between VOLUME ["C:\some_path"] and -v <volname>:/some_path, in the current implementation.

(Edit: Oops, your example here actually is two different paths, C:/test_dir and C:/C/test_dir, sorry. I misread that as VOLUME ["C:/test_dir", "/test_dir"]. So that's a different issue.)

[gabriel-samfira]

This issue is similar to: containerd/containerd#8362 (which adds context in the comments)

For non C:\ volumes, we need to skip trying to copy existing contents. For C: volumes, containerd strips the drive letter here:

https://github.com/containerd/containerd/blob/474a257b16f0aa79421038736126080696605e9d/pkg/cri/opts/container.go#L100-L101

I think it should be enough to mimic that.

[gabriel-samfira]

Note: a folder on the host for a potential non C: volume should already exist. We just need to skip the attempt to populate it.

[aznashwan]

For non C:\ volumes, we need to skip trying to copy existing contents. For C: volumes, containerd strips the drive letter

Correct, but it only trims "C:/" specifically, and AFAICT Docker should allow for any drive other than C too.

Considering the names of these host dirs seems to be irrelevant, I suggest we just use anonVolName. (which is actually a random ID generated on the spot)

I'll also log the mapping for some debug-ability...

[gabriel-samfira]

I was thinking of something like this. This should mimic what is/will (be) happening in containerd.

Suggested change

	target, err := joinHostPath(tempDir, x.Mount.Destination)
	if len(x.Mount.Destination) >= 2 && string(x.Mount.Destination[1]) == ":" && strings.EqualFold(string(x.Mount.Destination[0]), "c") {
	target, err := joinHostPath(tempDir, x.Mount.Destination[2:])
	//copying up initial contents of the mount point directory
	if err := copyExistingContents(target, anonVol.Mountpoint); err != nil {
	return nil, nil, nil, err
	}
	}

[TBBle]

This code won't work correctly, AFAICT.

The path being created (target) is the path to the source of the copying in the temporary local mount of the source image's View, so it cannot be an arbitrary path. https://github.com/containerd/nerdctl/pull/2160/files#r1160795974 is the right path-transformation, but we can't continue as (unlike the containerd-cri equivalent to this code) we still need to do the things after the copyExistingContents call to mount the new volume into the system.

When the path to be copied is C:\some\path, then its location is <tempDir>\some\path; when the path to be copied is D:, then there's actually no possible valid path, as Windows container images cannot contain data for any drive but C:.

So perhaps something like what is here now, but the Windows split of the joinHostPath strips C: if present, and if given D:..Z:, returns an empty string which then skips only the copyExistingContents step (the only use of joinHostPath's return value here).

I also suspect this code won't work with current containerd main branch (and not with released containerd) as it has a Windows codepath that mounts the copying source image at a subdirectory of <temp_dir>, so it's impossible to actually generate a correct path right now. This block can simply be removed, as the else path is correct (mount.All(mounts, tempDir)) for current containerd main (along with revendoring to current containerd main, I expect to pick up the new mount implementation.) I'm not sure why this code exists at all, but even containerd 1.1 only returned one Mount object in the array on Windows (and 1.0 didn't support a Windows snapshotter at all).

---

I'm also super suspicious of this code as it looks like it'll replace the contents of an existing volume mounted at a VOLUME-named location with the contents of the image that location, which is way-wrong.

[aznashwan]

After some post-KubeCon delay, I have resorted to adding an explicit non-C-drive check.

I've also added a simple test with containerd's volume-copy-up image FWIW, which is currently expected to fail with this: #924 (comment).

I'll need to investigate further where/if that "fstype" needs setting.

[TBBle]

Per #2160 (review), we're still failing when using non-C: volume mounts, i.e. VOLUME ["D:"] will now error out with an "unsupported" complaint, which appears to blame "Windows containers", but is just a parsing failure in nerdctl.

It also doesn't fix the issue in the runtime.GOOS == "windows" mounting code path which means that the content copying in both use-cases won't work anyway as the data we need is mounted not at tempDir, but a subdirectory of tempDir. I don't understand how the given test-case can pass because of this, which makes me suspect VOLUME is not actually being applied, so the test is seeing the in-image copy of the relevant file.

I'd also get rid of the new logrus.Debugf calls before merging, they seem overly-verbose.

It'd be nice to have tests of the -v code-path this code changes too, but perhaps #924 is covering that? (Per the above-linked comment, I'm not totally clear how -v doesn't clobber existing volume contents if pointed at a directory that contains data already; I suspect we lack a test for that flow on any platform.)

[TBBle]

I don't understand the connection between this error and the linked comment:

• the linked comment is in moby, and the equivalent code in containerd works (i.e. that limitation is historical, not current)
• the linked comment is about how paths like "C:\..." are handled, but we're in code which is not "C:\...".

[TBBle]

This test would also pass if the VOLUME directive is not present or ignored, as the test_file will still exist. Is it possible to test that a host-sourced volume is mounted at that path as well?