LCOW: Ensure platform is populated on COPY/ADD

[lowenna]

Signed-off-by: John Howard jhoward@microsoft.com

Fixes #37561, and internal VSO bug 17531561.

The copier object constructor is only using the API/dockerfile requested platform (OS) and ignoring the implicit operating system in the builder state. Hence, on LCOW (see linked issue this fixes) where no platform is passed in the API, a docker build goes down the route of assuming it's Windows paths.

@tonistiigi, @johnstep PTAL. @jterry75 FYI

Note there is an alternate fix I came up with initially. Both fixes work, but I think this is cleaner having the correct OS set at the start of the COPY command as it comes in from the dispatcher. Alternate can be found at 3bf17fc.

Using the same dockerfile from the linked issue, results with this fix (it succeeds and /foo/bar/dockerfile is shown in step 4).

PS E:\docker\build\17531561> docker build -t 17531561 --no-cache .
Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM alpine
 ---> 11cd0b38bc3c
Step 2/4 : ADD . /foo/bar
 ---> 5ff4b6738a94
Step 3/4 : RUN ls -l /
 ---> Running in 602c3ce3695a
total 52
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 bin
drwxr-xr-x    5 root     root           340 Jul 30 18:34 dev
drwxr-xr-x    1 root     root            60 Jul 30 18:34 etc
drwxr-xr-x    3 root     root          4096 Jul 30 18:34 foo
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 home
drwxr-xr-x    5 root     root          4096 Jul  5 14:47 lib
drwxr-xr-x    5 root     root          4096 Jul  5 14:47 media
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 mnt
dr-xr-xr-x   72 root     root             0 Jul 30 18:34 proc
drwx------    2 root     root          4096 Jul  5 14:47 root
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 run
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 sbin
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 srv
dr-xr-xr-x   13 root     root             0 Jul 30 18:34 sys
drwxrwxrwt    2 root     root          4096 Jul  5 14:47 tmp
drwxr-xr-x    7 root     root          4096 Jul  5 14:47 usr
drwxr-xr-x   11 root     root          4096 Jul  5 14:47 var
Removing intermediate container 602c3ce3695a
 ---> 704ea4b1141d
Step 4/4 : RUN ls -lR /foo
 ---> Running in ba0ec5a90cf1
/foo:
total 4
drwxr-xr-x    2 root     root          4096 Jul 30 18:34 bar

/foo/bar:
total 4
-rwxr-xr-x    1 root     root           228 Jul 30 16:51 dockerfile
Removing intermediate container ba0ec5a90cf1
 ---> 4a62ac6e4668
Successfully built 4a62ac6e4668
Successfully tagged 17531561:latest
PS E:\docker\build\17531561>

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@73d73ba). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #37563   +/-   ##
=========================================
  Coverage          ?   35.62%           
=========================================
  Files             ?      610           
  Lines             ?    44903           
  Branches          ?        0           
=========================================
  Hits              ?    15996           
  Misses            ?    26705           
  Partials          ?     2202

[thaJeztah]

Left a comment; I'm not super-familiar with this code, so my comment is just from digging a bit

[thaJeztah]

I notice that after the copier is created, builder.performCopy() is called;

moby/builder/dockerfile/dispatchers.go

Lines 123 to 131 in 492545e

	copier := copierFromDispatchRequest(d, errOnSourceDownload, im)
	defer copier.Cleanup()
	copyInstruction, err := copier.createCopyInstruction(c.SourcesAndDest, "COPY")
	if err != nil {
	return err
	}
	copyInstruction.chownStr = c.Chown
	return d.builder.performCopy(d, copyInstruction)

Looking at that function, that seems to use req.builder.platform, not the value set on the copier.

moby/builder/dockerfile/internals.go

Line 172 in f0585d0

imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.platform)

The value is actually used, but only for path separators, and not persisted in the copy instruction itself;

moby/builder/dockerfile/copy.go

Lines 98 to 106 in b711437

	// Work in platform-specific filepath semantics
	// TODO: This OS switch for paths is NOT correct and should not be supported.
	// Maintained for backwards compatibility
	pathOS := runtime.GOOS
	if o.platform != nil {
	pathOS = o.platform.OS
	}
	inst.dest = fromSlash(args[last], pathOS)
	separator := string(separator(pathOS))

Should a property beaded to copyInstruction, and set to this value?

[lowenna]

@thaJeztah I'll defer to @tonistiigi here. I don't think it matters, but I don't own the builder code. Just fixing a bug...

[tonistiigi]

The platform in

moby/builder/dockerfile/internals.go

Line 172 in f0585d0

imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.platform)

is for the target image of --from not for the current instruction so may be different.

[thaJeztah]

ping @simonferquel PTAL as well

[thaJeztah]

LGTM