From 0ba6f88a031bf863004fe8fb06512fa79bb48bcf Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 3 Feb 2017 18:05:20 -0800 Subject: [PATCH] cluster/executor: check mounts at start While it is important to not create controllers for an invalid task, certain properties should only be checked immediately before use. Early host validation of mounts prevents resolution of the task Executor when the mounts are not relevant to execution flow. In this case, we have a check for the existence of a bind mount path in a creation function that prevents a task controller from being resolved. Such early validation prevents one from interacting directly with a controller and result in unnecessary error reporting. In accordance with the above, we move the validation of the existence of host bind mount paths to the `Controller.Start` phase. We also call these "checks", as they are valid mounts but reference non-existent paths. Signed-off-by: Stephen J Day Upstream-commit: 92899ffac8ca1136e807dd234e8fa1dd49db7801 Component: engine --- .../cluster/executor/container/adapter.go | 22 +++++++++++++++++++ .../cluster/executor/container/validate.go | 4 ---- .../executor/container/validate_test.go | 6 ++--- .../executor/container/validate_unix_test.go | 3 ++- .../container/validate_windows_test.go | 5 ++++- 5 files changed, 31 insertions(+), 9 deletions(-) diff --git a/components/engine/daemon/cluster/executor/container/adapter.go b/components/engine/daemon/cluster/executor/container/adapter.go index 565af80f2a..a770cd50d5 100644 --- a/components/engine/daemon/cluster/executor/container/adapter.go +++ b/components/engine/daemon/cluster/executor/container/adapter.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "strings" "syscall" "time" @@ -259,7 +260,28 @@ func (c *containerAdapter) create(ctx context.Context) error { return nil } +// checkMounts ensures that the provided mounts won't have any host-specific +// problems at start up. For example, we disallow bind mounts without an +// existing path, which slightly different from the container API. +func (c *containerAdapter) checkMounts() error { + spec := c.container.spec() + for _, mount := range spec.Mounts { + switch mount.Type { + case api.MountTypeBind: + if _, err := os.Stat(mount.Source); os.IsNotExist(err) { + return fmt.Errorf("invalid bind mount source, source path not found: %s", mount.Source) + } + } + } + + return nil +} + func (c *containerAdapter) start(ctx context.Context) error { + if err := c.checkMounts(); err != nil { + return err + } + return c.backend.ContainerStart(c.container.name(), nil, "", "") } diff --git a/components/engine/daemon/cluster/executor/container/validate.go b/components/engine/daemon/cluster/executor/container/validate.go index f970f069b7..af17f5b810 100644 --- a/components/engine/daemon/cluster/executor/container/validate.go +++ b/components/engine/daemon/cluster/executor/container/validate.go @@ -3,7 +3,6 @@ package container import ( "errors" "fmt" - "os" "path/filepath" "github.com/docker/swarmkit/api" @@ -25,9 +24,6 @@ func validateMounts(mounts []api.Mount) error { if !filepath.IsAbs(mount.Source) { return fmt.Errorf("invalid bind mount source, must be an absolute path: %s", mount.Source) } - if _, err := os.Stat(mount.Source); os.IsNotExist(err) { - return fmt.Errorf("invalid bind mount source, source path not found: %s", mount.Source) - } case api.MountTypeVolume: if filepath.IsAbs(mount.Source) { return fmt.Errorf("invalid volume mount source, must not be an absolute path: %s", mount.Source) diff --git a/components/engine/daemon/cluster/executor/container/validate_test.go b/components/engine/daemon/cluster/executor/container/validate_test.go index 5f202d5859..9d98e2c008 100644 --- a/components/engine/daemon/cluster/executor/container/validate_test.go +++ b/components/engine/daemon/cluster/executor/container/validate_test.go @@ -42,10 +42,10 @@ func TestControllerValidateMountBind(t *testing.T) { // with non-existing source if _, err := newTestControllerWithMount(api.Mount{ Type: api.MountTypeBind, - Source: "/some-non-existing-host-path/", + Source: testAbsNonExistent, Target: testAbsPath, - }); err == nil || !strings.Contains(err.Error(), "invalid bind mount source") { - t.Fatalf("expected error, got: %v", err) + }); err != nil { + t.Fatalf("controller should not error at creation: %v", err) } // with proper source diff --git a/components/engine/daemon/cluster/executor/container/validate_unix_test.go b/components/engine/daemon/cluster/executor/container/validate_unix_test.go index 5e122de918..c616eeef93 100644 --- a/components/engine/daemon/cluster/executor/container/validate_unix_test.go +++ b/components/engine/daemon/cluster/executor/container/validate_unix_test.go @@ -3,5 +3,6 @@ package container const ( - testAbsPath = "/foo" + testAbsPath = "/foo" + testAbsNonExistent = "/some-non-existing-host-path/" ) diff --git a/components/engine/daemon/cluster/executor/container/validate_windows_test.go b/components/engine/daemon/cluster/executor/container/validate_windows_test.go index 8eb3e9987a..c346451d3d 100644 --- a/components/engine/daemon/cluster/executor/container/validate_windows_test.go +++ b/components/engine/daemon/cluster/executor/container/validate_windows_test.go @@ -1,5 +1,8 @@ +// +build windows + package container const ( - testAbsPath = `c:\foo` + testAbsPath = `c:\foo` + testAbsNonExistent = `c:\some-non-existing-host-path\` )