Plumb CRI Devices through to OCI WindowsDevices

There's two mappings of hostpath to IDType and ID in the wild:
- dockershim and dockerd-cri (implicitly via docker) use class/ID
-- The only supported IDType in Docker is 'class'.
-- https://github.com/aarnaud/k8s-directx-device-plugin generates this form
- https://github.com/jterry75/cri (windows_port branch) uses IDType://ID
-- hcsshim's CRI test suite generates this form

`://` is much more easily distinguishable, so I've gone with that one as
the generic separator, with `class/` as a special-case.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

oci/spec_opts.go

@@ -1367,3 +1367,17 @@ func tryReadonlyMounts(mounts []mount.Mount) []mount.Mount {
 	}
 	return mounts
 }
+
+// WithWindowsDevice adds a device exposed to a Windows (WCOW or LCOW) Container
+func WithWindowsDevice(idType, id string) SpecOpts {
+	return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
+		if idType == "" {
+			return errors.New("missing idType")
+		}
+		if s.Windows == nil {
+			s.Windows = &specs.Windows{}
+		}
+		s.Windows.Devices = append(s.Windows.Devices, specs.WindowsDevice{IDType: idType, ID: id})
+		return nil
+	}
+}

oci/spec_opts_test.go

@@ -30,6 +30,9 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/containerd/containerd/content"
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -704,3 +707,75 @@ func TestWithoutMounts(t *testing.T) {
 		t.Fatalf("expected %+v, got %+v", expected, s.Mounts)
 	}
 }
+
+func TestWithWindowsDevice(t *testing.T) {
+	testcases := []struct {
+		name   string
+		idType string
+		id     string
+
+		expectError            bool
+		expectedWindowsDevices []specs.WindowsDevice
+	}{
+		{
+			name:        "empty_idType_and_id",
+			idType:      "",
+			id:          "",
+			expectError: true,
+		},
+		{
+			name:        "empty_idType",
+			idType:      "",
+			id:          "5B45201D-F2F2-4F3B-85BB-30FF1F953599",
+			expectError: true,
+		},
+		{
+			name:   "empty_id",
+			idType: "class",
+			id:     "",
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "", IDType: "class"}},
+		},
+		{
+			name:   "idType_and_id",
+			idType: "class",
+			id:     "5B45201D-F2F2-4F3B-85BB-30FF1F953599",
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "5B45201D-F2F2-4F3B-85BB-30FF1F953599", IDType: "class"}},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			spec := Spec{
+				Version: specs.Version,
+				Root:    &specs.Root{},
+				Windows: &specs.Windows{},
+			}
+
+			opts := []SpecOpts{
+				WithWindowsDevice(tc.idType, tc.id),
+			}
+
+			for _, opt := range opts {
+				if err := opt(nil, nil, nil, &spec); err != nil {
+					if tc.expectError {
+						assert.Error(t, err)
+					} else {
+						require.NoError(t, err)
+					}
+				}
+			}
+
+			if len(tc.expectedWindowsDevices) != 0 {
+				require.NotNil(t, spec.Windows)
+				require.NotNil(t, spec.Windows.Devices)
+				assert.ElementsMatch(t, spec.Windows.Devices, tc.expectedWindowsDevices)
+			} else if spec.Windows != nil && spec.Windows.Devices != nil {
+				assert.ElementsMatch(t, spec.Windows.Devices, tc.expectedWindowsDevices)
+			}
+		})
+	}
+}

pkg/cri/opts/spec_windows.go

@@ -230,3 +230,34 @@ func WithWindowsCredentialSpec(credentialSpec string) oci.SpecOpts {
 		return nil
 	}
 }
+
+// WithDevices sets the provided devices onto the container spec
+func WithDevices(config *runtime.ContainerConfig) oci.SpecOpts {
+	return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) {
+		for _, device := range config.GetDevices() {
+			if device.ContainerPath != "" {
+				return fmt.Errorf("unexpected ContainerPath %s, must be empty", device.ContainerPath)
+			}
+
+			if device.Permissions != "" {
+				return fmt.Errorf("unexpected Permissions %s, must be empty", device.Permissions)
+			}
+
+			hostPath := device.HostPath
+			if strings.HasPrefix(hostPath, "class/") {
+				hostPath = strings.Replace(hostPath, "class/", "class://", 1)
+			}
+
+			splitParts := strings.SplitN(hostPath, "://", 2)
+			if len(splitParts) != 2 {
+				return fmt.Errorf("unrecognised HostPath format %v, must match IDType://ID", device.HostPath)
+			}
+
+			o := oci.WithWindowsDevice(splitParts[0], splitParts[1])
+			if err := o(ctx, client, c, s); err != nil {
+				return fmt.Errorf("failed adding device with HostPath %v: %w", device.HostPath, err)
+			}
+		}
+		return nil
+	}
+}

pkg/cri/opts/spec_windows_test.go

@@ -0,0 +1,217 @@
+/*
+   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 (
+	"context"
+	"testing"
+
+	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/namespaces"
+	"github.com/containerd/containerd/oci"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+)
+
+func TestWithDevices(t *testing.T) {
+	testcases := []struct {
+		name    string
+		devices []*runtime.Device
+		isLCOW  bool
+
+		expectError            bool
+		expectedWindowsDevices []specs.WindowsDevice
+	}{
+		{
+			name: "empty",
+
+			expectError: false,
+		},
+		// The only supported field is HostPath
+		{
+			name:    "empty fields",
+			devices: []*runtime.Device{{}},
+
+			expectError: true,
+		},
+		{
+			name:    "containerPath",
+			devices: []*runtime.Device{{ContainerPath: "something"}},
+
+			expectError: true,
+		},
+		{
+			name:    "permissions",
+			devices: []*runtime.Device{{Permissions: "something"}},
+
+			expectError: true,
+		},
+		// Produced by https://github.com/aarnaud/k8s-directx-device-plugin/blob/0f3db32622daa577c85621941682bee6f9080954/cmd/k8s-device-plugin/main.go
+		// This is also the syntax dockershim and cri-dockerd support (or rather, pass through to docker, which parses this syntax)
+		{
+			name:    "hostPath_docker_style",
+			devices: []*runtime.Device{{HostPath: "class/5B45201D-F2F2-4F3B-85BB-30FF1F953599"}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "5B45201D-F2F2-4F3B-85BB-30FF1F953599", IDType: "class"}},
+		},
+		// Docker _only_ accepts `class` ID Type, so anything else should fail.
+		// See https://github.com/moby/moby/blob/v20.10.13/daemon/oci_windows.go#L283-L294
+		{
+			name:    "hostPath_docker_style_non-class_idtype",
+			devices: []*runtime.Device{{HostPath: "vpci-location-path/5B45201D-F2F2-4F3B-85BB-30FF1F953599"}},
+
+			expectError: true,
+		},
+		// A bunch of examples from https://github.com/microsoft/hcsshim/blob/v0.9.2/test/cri-containerd/container_virtual_device_test.go
+		{
+			name: "hostPath_hcsshim_lcow_gpu",
+			// Not actually a GPU PCIP instance, but my personal machine doesn't have any PCIP devices, so I found one on the 'net.
+			devices: []*runtime.Device{{HostPath: `gpu://PCIP\VEN_8086&DEV_43A2&SUBSYS_72708086&REV_00\3&11583659&0&F5`}},
+			isLCOW:  true,
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: `PCIP\VEN_8086&DEV_43A2&SUBSYS_72708086&REV_00\3&11583659&0&F5`, IDType: "gpu"}},
+		},
+		{
+			name:    "hostPath_hcsshim_wcow_location_path",
+			devices: []*runtime.Device{{HostPath: "vpci-location-path://PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0001)"}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0001)", IDType: "vpci-location-path"}},
+		},
+		{
+			name:    "hostPath_hcsshim_wcow_class_guid",
+			devices: []*runtime.Device{{HostPath: "class://5B45201D-F2F2-4F3B-85BB-30FF1F953599"}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "5B45201D-F2F2-4F3B-85BB-30FF1F953599", IDType: "class"}},
+		},
+		{
+			name: "hostPath_hcsshim_wcow_gpu_hyper-v",
+			// Not actually a GPU PCIP instance, but my personal machine doesn't have any PCIP devices, so I found one on the 'net.
+			devices: []*runtime.Device{{HostPath: `vpci://PCIP\VEN_8086&DEV_43A2&SUBSYS_72708086&REV_00\3&11583659&0&F5`}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: `PCIP\VEN_8086&DEV_43A2&SUBSYS_72708086&REV_00\3&11583659&0&F5`, IDType: "vpci"}},
+		},
+		// Example from https://github.com/microsoft/hcsshim/blob/v0.9.2/test/cri-containerd/container_test.go
+		// According to https://github.com/jterry75/cri/blob/f8e83e63cc027d0e9c0c984f9db3cba58d3672d4/pkg/server/container_create_windows.go#L625-L649
+		// this is intended to generate LinuxDevice entries that the GCS shim in a LCOW container host will remap
+		// into device mounts from its own in-UVM kernel.
+		// From discussion on https://github.com/containerd/containerd/pull/6618, we reject this syntax for now.
+		{
+			name:    "hostPath_hcsshim_lcow_sandbox_device",
+			devices: []*runtime.Device{{HostPath: "/dev/fuse"}},
+			isLCOW:  true,
+
+			expectError: true,
+		},
+		// Some edge cases suggested by the above real-world examples
+		{
+			name:    "hostPath_no_slash",
+			devices: []*runtime.Device{{HostPath: "no_slash"}},
+
+			expectError: true,
+		},
+		{
+			name:    "hostPath_but_no_type",
+			devices: []*runtime.Device{{HostPath: "://5B45201D-F2F2-4F3B-85BB-30FF1F953599"}},
+
+			expectError: true,
+		},
+		{
+			name:    "hostPath_but_no_id",
+			devices: []*runtime.Device{{HostPath: "gpu://"}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "", IDType: "gpu"}},
+		},
+		{
+			name:    "hostPath_dockerstyle_with_slashes_in_id",
+			devices: []*runtime.Device{{HostPath: "class/slashed/id"}},
+
+			expectError:            false,
+			expectedWindowsDevices: []specs.WindowsDevice{{ID: "slashed/id", IDType: "class"}},
+		},
+		{
+			name:    "hostPath_docker_style_non-class_idtypewith_slashes_in_id",
+			devices: []*runtime.Device{{HostPath: "vpci-location-path/slashed/id"}},
+
+			expectError: true,
+		},
+		{
+			name: "hostPath_hcsshim_wcow_location_path_twice",
+			devices: []*runtime.Device{
+				{HostPath: "vpci-location-path://PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0001)"},
+				{HostPath: "vpci-location-path://PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0002)"}},
+
+			expectError: false,
+			expectedWindowsDevices: []specs.WindowsDevice{
+				{ID: "PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0001)", IDType: "vpci-location-path"},
+				{ID: "PCIROOT(0)#PCI(0100)#PCI(0000)#PCI(0000)#PCI(0002)", IDType: "vpci-location-path"},
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			var (
+				ctx = namespaces.WithNamespace(context.Background(), "testing")
+				c   = &containers.Container{ID: t.Name()}
+			)
+
+			config := runtime.ContainerConfig{}
+			config.Devices = tc.devices
+
+			specOpts := []oci.SpecOpts{WithDevices(&config)}
+
+			platform := "windows"
+			if tc.isLCOW {
+				platform = "linux"
+			}
+
+			spec, err := oci.GenerateSpecWithPlatform(ctx, nil, platform, c, specOpts...)
+			if tc.expectError {
+				assert.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			// Ensure we got the right LCOWness in the spec
+			if tc.isLCOW {
+				assert.NotNil(t, spec.Linux)
+			} else {
+				assert.Nil(t, spec.Linux)
+			}
+
+			if len(tc.expectedWindowsDevices) != 0 {
+				require.NotNil(t, spec.Windows)
+				require.NotNil(t, spec.Windows.Devices)
+				assert.Equal(t, spec.Windows.Devices, tc.expectedWindowsDevices)
+			} else if spec.Windows != nil && spec.Windows.Devices != nil {
+				assert.Empty(t, spec.Windows.Devices)
+			}
+
+			if spec.Linux != nil && spec.Linux.Devices != nil {
+				assert.Empty(t, spec.Linux.Devices)
+			}
+		})
+	}
+}

pkg/cri/server/container_create_windows.go

@@ -76,7 +76,7 @@ func (c *criService) containerSpec(
 		oci.WithHostname(sandboxConfig.GetHostname()),
 	)
 
-	specOpts = append(specOpts, customopts.WithWindowsMounts(c.os, config, extraMounts))
+	specOpts = append(specOpts, customopts.WithWindowsMounts(c.os, config, extraMounts), customopts.WithDevices(config))
 
 	// Start with the image config user and override below if RunAsUsername is not "".
 	username := imageConfig.User