Support named pipe mounts for Windows containers

[kevpar]

Adds support to mount named pipes into Windows containers. This support
already exists in hcsshim, so this change just passes them through
correctly in cri. Named pipe mounts must start with "\.\pipe".

Signed-off-by: Kevin Parsons kevpar@microsoft.com

Fixes containerd/containerd#4320

[k8s-ci-robot]

Hi @kevpar. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kevpar]

Looks like appveyor broke on downloading mingw:

Downloading mingw 
  from 'https://downloads.sourceforge.net/mingw-w64/x86_64-5.3.0-release-posix-seh-rt_v4-rev0.7z'
ERROR: The remote file either doesn't exist, is unauthorized, or is forbidden for url 'https://downloads.sourceforge.net/mingw-w64/x86_64-5.3.0-release-posix-seh-rt_v4-rev0.7z'. Exception calling "GetResponse" with "0" argument(s): "The operation has timed out" 

[mikebrow]

/ok-to-test

[mikebrow]

Looks like appveyor broke on downloading mingw:

Downloading mingw 
  from 'https://downloads.sourceforge.net/mingw-w64/x86_64-5.3.0-release-posix-seh-rt_v4-rev0.7z'
ERROR: The remote file either doesn't exist, is unauthorized, or is forbidden for url 'https://downloads.sourceforge.net/mingw-w64/x86_64-5.3.0-release-posix-seh-rt_v4-rev0.7z'. Exception calling "GetResponse" with "0" argument(s): "The operation has timed out" 

happens all the time..

[jterry75]

LGTM

[jterry75]

@kevpar - Looks like one of the unit tests broke. Need to update the assumption there or else fix the code not sure didnt look into it. But good point, can you add a unit test for this in the same file as well for npipe on windows

[mikebrow]

looking good just needs a couple tests

[kevpar]

@mikebrow I've added a test in container_create_windows_test.go, but since this mocks out the OS, it doesn't validate the filepath interactions. Is there anywhere else I should add a test? It looks like the integration tests don't currently run on Windows (unless I've missed something).

[kevpar]

Got an error that seems unrelated to my changes:

I0624 07:55:20.688] pkg/server/image_pull.go:287:3: SA1019: tlsConfig.BuildNameToCertificate is deprecated: NameToCertificate only allows associating a single certificate with a given name. Leave that field nil to let the library select the first compatible chain from Certificates.  (staticcheck)
I0624 07:55:20.688] 		tlsConfig.BuildNameToCertificate()
I0624 07:55:20.688] 		^

I'll take a look at this tomorrow, just posting in case anyone knows why this was hit.

[mikebrow]

@mikebrow I've added a test in container_create_windows_test.go, but since this mocks out the OS, it doesn't validate the filepath interactions. Is there anywhere else I should add a test? It looks like the integration tests don't currently run on Windows (unless I've missed something).

I think your new test is fine for now. To validate the interactions.... Maybe improve the tests in cri-tools critest under pkg/validate/container*.go.

[mikebrow]

Got an error that seems unrelated to my changes:

I0624 07:55:20.688] pkg/server/image_pull.go:287:3: SA1019: tlsConfig.BuildNameToCertificate is deprecated: NameToCertificate only allows associating a single certificate with a given name. Leave that field nil to let the library select the first compatible chain from Certificates.  (staticcheck)
I0624 07:55:20.688] 		tlsConfig.BuildNameToCertificate()
I0624 07:55:20.688] 		^

I'll take a look at this tomorrow, just posting in case anyone knows why this was hit.

let's see... BuildNameToCertificate seems to have been deprecated in go 1.14..
But we are on go 1.13.12.. hmm..
Makefile:

	$(GO) get -d github.com/golangci/golangci-lint/cmd/golangci-lint
	@cd $(GOPATH)/src/github.com/golangci/golangci-lint/cmd/golangci-lint; \
		git checkout v1.18.0; \
		go install

hmm... perhaps there is a golangci-lint already on the path that is a different version? doesn't matter pushing pr to ignore the deprecation lint error.

[mikebrow]

kk fixed ci .. please rebase.

[kevpar]

Looks like all the tests pass now, so I think this is good to go. Thanks @mikebrow for your help!

[mikebrow]

LGTM