Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --device support for Windows #1606

Merged
merged 1 commit into from
Feb 6, 2019
Merged

Add --device support for Windows #1606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/command/container/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, options *createOptio
}
}
copts.env = *opts.NewListOptsRef(&newEnv, nil)
containerConfig, err := parse(flags, copts)
containerConfig, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers; I was discussing some options with @jhowardmsft - it's a bit ugly, but we should (see my other comment) move more of these platform-specific validations out of the CLI. The cli should limit validation to "well-formedness" of values (e.g. if we expect an integer; validate it's an integer), but (if possible) nothing further than that.

if err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
Expand Down
59 changes: 49 additions & 10 deletions cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func addFlags(flags *pflag.FlagSet) *containerOptions {
deviceReadIOps: opts.NewThrottledeviceOpt(opts.ValidateThrottleIOpsDevice),
deviceWriteBps: opts.NewThrottledeviceOpt(opts.ValidateThrottleBpsDevice),
deviceWriteIOps: opts.NewThrottledeviceOpt(opts.ValidateThrottleIOpsDevice),
devices: opts.NewListOpts(validateDevice),
devices: opts.NewListOpts(nil), // devices can only be validated after we know the server OS
env: opts.NewListOpts(opts.ValidateEnv),
envFile: opts.NewListOpts(nil),
expose: opts.NewListOpts(nil),
Expand Down Expand Up @@ -299,7 +299,7 @@ type containerConfig struct {
// a HostConfig and returns them with the specified command.
// If the specified args are not valid, it will return an error.
// nolint: gocyclo
func parse(flags *pflag.FlagSet, copts *containerOptions) (*containerConfig, error) {
func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*containerConfig, error) {
var (
attachStdin = copts.attach.Get("stdin")
attachStdout = copts.attach.Get("stdout")
Expand Down Expand Up @@ -417,10 +417,22 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*containerConfig, err
}
}

// parse device mappings
// validate and parse device mappings. Note we do late validation of the
// device path (as opposed to during flag parsing), as at the time we are
// parsing flags, we haven't yet sent a _ping to the daemon to determine
// what operating system it is.
deviceMappings := []container.DeviceMapping{}
for _, device := range copts.devices.GetAll() {
deviceMapping, err := parseDevice(device)
var (
validated string
deviceMapping container.DeviceMapping
err error
)
validated, err = validateDevice(device, serverOS)
if err != nil {
return nil, err
}
deviceMapping, err = parseDevice(validated, serverOS)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -747,7 +759,19 @@ func parseStorageOpts(storageOpts []string) (map[string]string, error) {
}

// parseDevice parses a device mapping string to a container.DeviceMapping struct
func parseDevice(device string) (container.DeviceMapping, error) {
func parseDevice(device, serverOS string) (container.DeviceMapping, error) {
switch serverOS {
case "linux":
return parseLinuxDevice(device)
case "windows":
return parseWindowsDevice(device)
}
return container.DeviceMapping{}, errors.Errorf("unknown server OS: %s", serverOS)
}

// parseLinuxDevice parses a device mapping string to a container.DeviceMapping struct
// knowing that the target is a Linux daemon
func parseLinuxDevice(device string) (container.DeviceMapping, error) {
src := ""
dst := ""
permissions := "rwm"
Expand Down Expand Up @@ -781,6 +805,12 @@ func parseDevice(device string) (container.DeviceMapping, error) {
return deviceMapping, nil
}

// parseWindowsDevice parses a device mapping string to a container.DeviceMapping struct
// knowing that the target is a Windows daemon
func parseWindowsDevice(device string) (container.DeviceMapping, error) {
return container.DeviceMapping{PathOnHost: device}, nil
}

// validateDeviceCgroupRule validates a device cgroup rule string format
// It will make sure 'val' is in the form:
// 'type major:minor mode'
Expand Down Expand Up @@ -813,14 +843,23 @@ func validDeviceMode(mode string) bool {
}

// validateDevice validates a path for devices
func validateDevice(val string, serverOS string) (string, error) {
switch serverOS {
case "linux":
return validateLinuxPath(val, validDeviceMode)
case "windows":
// Windows does validation entirely server-side
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 We should remove more of these validations from the client, and defer as much as possible to the daemon/api. Doing so is much more maintainable, and makes sure that the daemon is in charge as to "what's valid" and "what not" (which may differ between daemon versions). I'll start working on an epic for that

return val, nil
}
return "", errors.Errorf("unknown server OS: %s", serverOS)
}

// validateLinuxPath is the implementation of validateDevice knowing that the
// target server operating system is a Linux daemon.
// It will make sure 'val' is in the form:
// [host-dir:]container-path[:mode]
// It also validates the device mode.
func validateDevice(val string) (string, error) {
return validatePath(val, validDeviceMode)
}

func validatePath(val string, validator func(string) bool) (string, error) {
func validateLinuxPath(val string, validator func(string) bool) (string, error) {
var containerPath string
var mode string

Expand Down
11 changes: 7 additions & 4 deletions cli/command/container/opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/spf13/pflag"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
"gotest.tools/skip"
)

func TestValidateAttach(t *testing.T) {
Expand Down Expand Up @@ -48,7 +49,7 @@ func parseRun(args []string) (*container.Config, *container.HostConfig, *network
return nil, nil, nil, err
}
// TODO: fix tests to accept ContainerConfig
containerConfig, err := parse(flags, copts)
containerConfig, err := parse(flags, copts, runtime.GOOS)
if err != nil {
return nil, nil, nil, err
}
Expand Down Expand Up @@ -351,6 +352,7 @@ func TestParseWithExpose(t *testing.T) {
}

func TestParseDevice(t *testing.T) {
skip.If(t, runtime.GOOS == "windows") // Windows validates server-side
valids := map[string]container.DeviceMapping{
"/dev/snd": {
PathOnHost: "/dev/snd",
Expand Down Expand Up @@ -393,7 +395,7 @@ func TestParseModes(t *testing.T) {
flags, copts := setupRunFlags()
args := []string{"--pid=container:", "img", "cmd"}
assert.NilError(t, flags.Parse(args))
_, err := parse(flags, copts)
_, err := parse(flags, copts, runtime.GOOS)
assert.ErrorContains(t, err, "--pid: invalid PID mode")

// pid ok
Expand Down Expand Up @@ -615,6 +617,7 @@ func TestParseEntryPoint(t *testing.T) {
}

func TestValidateDevice(t *testing.T) {
skip.If(t, runtime.GOOS == "windows") // Windows validates server-side
valid := []string{
"/home",
"/home:/home",
Expand Down Expand Up @@ -649,13 +652,13 @@ func TestValidateDevice(t *testing.T) {
}

for _, path := range valid {
if _, err := validateDevice(path); err != nil {
if _, err := validateDevice(path, runtime.GOOS); err != nil {
t.Fatalf("ValidateDevice(`%q`) should succeed: error %q", path, err)
}
}

for path, expectedError := range invalid {
if _, err := validateDevice(path); err == nil {
if _, err := validateDevice(path, runtime.GOOS); err == nil {
t.Fatalf("ValidateDevice(`%q`) should have failed validation", path)
} else {
if err.Error() != expectedError {
Expand Down
2 changes: 1 addition & 1 deletion cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func runRun(dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copt
}
}
copts.env = *opts.NewListOptsRef(&newEnv, nil)
containerConfig, err := parse(flags, copts)
containerConfig, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
// just in case the parse does not exit
if err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
Expand Down