From ec90839ca302ca53a7d55e4c7f79e7b4779f5e15 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 27 Mar 2018 14:03:23 -0400 Subject: [PATCH] Don't sort plugin mounts slice This was added as part of a53930a04fa81b082aa78e66b342ff19cc63cc5f with the intent to sort the mounts in the plugin config, but this was sorting *all* the mounts from the default OCI spec which is problematic. In reality we don't need to sort this because we are only adding a self-binded mount to flag it as rshared. We may want to look at sorting the plugin mounts before they are added to the OCI spec in the future, but for now I think the existing behavior is fine since the plugin author has control of the order (except for the propagated mount). Signed-off-by: Brian Goff --- integration-cli/fixtures/plugin/plugin.go | 32 ++++++++ integration/plugin/volumes/cmd/cmd_test.go | 1 + integration/plugin/volumes/cmd/dummy/main.go | 19 +++++ .../plugin/volumes/cmd/dummy/main_test.go | 1 + integration/plugin/volumes/helpers_test.go | 73 +++++++++++++++++++ integration/plugin/volumes/main_test.go | 34 +++++++++ integration/plugin/volumes/mounts_test.go | 56 ++++++++++++++ plugin/v2/plugin_linux.go | 5 -- 8 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 integration/plugin/volumes/cmd/cmd_test.go create mode 100644 integration/plugin/volumes/cmd/dummy/main.go create mode 100644 integration/plugin/volumes/cmd/dummy/main_test.go create mode 100644 integration/plugin/volumes/helpers_test.go create mode 100644 integration/plugin/volumes/main_test.go create mode 100644 integration/plugin/volumes/mounts_test.go diff --git a/integration-cli/fixtures/plugin/plugin.go b/integration-cli/fixtures/plugin/plugin.go index 52fb6c3425..e2a5c9bbda 100644 --- a/integration-cli/fixtures/plugin/plugin.go +++ b/integration-cli/fixtures/plugin/plugin.go @@ -155,6 +155,38 @@ func makePluginBundle(inPath string, opts ...CreateOpt) (io.ReadCloser, error) { if err := os.MkdirAll(filepath.Join(inPath, "rootfs", filepath.Dir(p.Entrypoint[0])), 0755); err != nil { return nil, errors.Wrap(err, "error creating plugin rootfs dir") } + + // Ensure the mount target paths exist + for _, m := range p.Mounts { + var stat os.FileInfo + if m.Source != nil { + stat, err = os.Stat(*m.Source) + if err != nil && !os.IsNotExist(err) { + return nil, err + } + } + + if stat == nil || stat.IsDir() { + var mode os.FileMode = 0755 + if stat != nil { + mode = stat.Mode() + } + if err := os.MkdirAll(filepath.Join(inPath, "rootfs", m.Destination), mode); err != nil { + return nil, errors.Wrap(err, "error preparing plugin mount destination path") + } + } else { + if err := os.MkdirAll(filepath.Join(inPath, "rootfs", filepath.Dir(m.Destination)), 0755); err != nil { + return nil, errors.Wrap(err, "error preparing plugin mount destination dir") + } + f, err := os.Create(filepath.Join(inPath, "rootfs", m.Destination)) + if err != nil && !os.IsExist(err) { + return nil, errors.Wrap(err, "error preparing plugin mount destination file") + } + if f != nil { + f.Close() + } + } + } if err := archive.NewDefaultArchiver().CopyFileWithTar(cfg.binPath, filepath.Join(inPath, "rootfs", p.Entrypoint[0])); err != nil { return nil, errors.Wrap(err, "error copying plugin binary to rootfs path") } diff --git a/integration/plugin/volumes/cmd/cmd_test.go b/integration/plugin/volumes/cmd/cmd_test.go new file mode 100644 index 0000000000..1d619dd05e --- /dev/null +++ b/integration/plugin/volumes/cmd/cmd_test.go @@ -0,0 +1 @@ +package cmd diff --git a/integration/plugin/volumes/cmd/dummy/main.go b/integration/plugin/volumes/cmd/dummy/main.go new file mode 100644 index 0000000000..f91b4f3b02 --- /dev/null +++ b/integration/plugin/volumes/cmd/dummy/main.go @@ -0,0 +1,19 @@ +package main + +import ( + "net" + "net/http" +) + +func main() { + l, err := net.Listen("unix", "/run/docker/plugins/plugin.sock") + if err != nil { + panic(err) + } + + server := http.Server{ + Addr: l.Addr().String(), + Handler: http.NewServeMux(), + } + server.Serve(l) +} diff --git a/integration/plugin/volumes/cmd/dummy/main_test.go b/integration/plugin/volumes/cmd/dummy/main_test.go new file mode 100644 index 0000000000..06ab7d0f9a --- /dev/null +++ b/integration/plugin/volumes/cmd/dummy/main_test.go @@ -0,0 +1 @@ +package main diff --git a/integration/plugin/volumes/helpers_test.go b/integration/plugin/volumes/helpers_test.go new file mode 100644 index 0000000000..068f6a3be2 --- /dev/null +++ b/integration/plugin/volumes/helpers_test.go @@ -0,0 +1,73 @@ +package volumes + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/integration-cli/fixtures/plugin" + "github.com/docker/docker/pkg/locker" + "github.com/pkg/errors" +) + +var pluginBuildLock = locker.New() + +// ensurePlugin makes the that a plugin binary has been installed on the system. +// Plugins that have not been installed are built from `cmd/`. +func ensurePlugin(t *testing.T, name string) string { + pluginBuildLock.Lock(name) + defer pluginBuildLock.Unlock(name) + + goPath := os.Getenv("GOPATH") + if goPath == "" { + goPath = "/go" + } + installPath := filepath.Join(goPath, "bin", name) + if _, err := os.Stat(installPath); err == nil { + return installPath + } + + goBin, err := exec.LookPath("go") + if err != nil { + t.Fatal(err) + } + + cmd := exec.Command(goBin, "build", "-o", installPath, "./"+filepath.Join("cmd", name)) + cmd.Env = append(cmd.Env, "CGO_ENABLED=0") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatal(errors.Wrapf(err, "error building basic plugin bin: %s", string(out))) + } + + return installPath +} + +func withSockPath(name string) func(*plugin.Config) { + return func(cfg *plugin.Config) { + cfg.Interface.Socket = name + } +} + +func createPlugin(t *testing.T, client plugin.CreateClient, alias, bin string, opts ...plugin.CreateOpt) { + pluginBin := ensurePlugin(t, bin) + + opts = append(opts, withSockPath("plugin.sock")) + opts = append(opts, plugin.WithBinary(pluginBin)) + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + err := plugin.Create(ctx, client, alias, opts...) + cancel() + + if err != nil { + t.Fatal(err) + } +} + +func asVolumeDriver(cfg *plugin.Config) { + cfg.Interface.Types = []types.PluginInterfaceType{ + {Capability: "volumedriver", Prefix: "docker", Version: "1.0"}, + } +} diff --git a/integration/plugin/volumes/main_test.go b/integration/plugin/volumes/main_test.go new file mode 100644 index 0000000000..e3aa260f8d --- /dev/null +++ b/integration/plugin/volumes/main_test.go @@ -0,0 +1,34 @@ +package volumes // import "github.com/docker/docker/integration/plugin/volumes" + +import ( + "fmt" + "os" + "testing" + + "github.com/docker/docker/internal/test/environment" +) + +var ( + testEnv *environment.Execution +) + +const dockerdBinary = "dockerd" + +func TestMain(m *testing.M) { + var err error + testEnv, err = environment.New() + if err != nil { + fmt.Println(err) + os.Exit(1) + } + if testEnv.OSType != "linux" { + os.Exit(0) + } + err = environment.EnsureFrozenImagesLinux(testEnv) + if err != nil { + fmt.Println(err) + os.Exit(1) + } + testEnv.Print() + os.Exit(m.Run()) +} diff --git a/integration/plugin/volumes/mounts_test.go b/integration/plugin/volumes/mounts_test.go new file mode 100644 index 0000000000..2c464f6607 --- /dev/null +++ b/integration/plugin/volumes/mounts_test.go @@ -0,0 +1,56 @@ +package volumes + +import ( + "context" + "io/ioutil" + "os" + "testing" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/integration-cli/daemon" + "github.com/docker/docker/integration-cli/fixtures/plugin" + "github.com/gotestyourself/gotestyourself/assert" +) + +// TestPluginWithDevMounts tests very specific regression caused by mounts ordering +// (sorted in the daemon). See #36698 +func TestPluginWithDevMounts(t *testing.T) { + t.Parallel() + + d := daemon.New(t, "", dockerdBinary, daemon.Config{}) + d.Start(t, "--iptables=false") + defer d.Stop(t) + + client, err := d.NewClient() + assert.Assert(t, err) + ctx := context.Background() + + testDir, err := ioutil.TempDir("", "test-dir") + assert.Assert(t, err) + defer os.RemoveAll(testDir) + + createPlugin(t, client, "test", "dummy", asVolumeDriver, func(c *plugin.Config) { + root := "/" + dev := "/dev" + mounts := []types.PluginMount{ + {Type: "bind", Source: &root, Destination: "/host", Options: []string{"rbind"}}, + {Type: "bind", Source: &dev, Destination: "/dev", Options: []string{"rbind"}}, + {Type: "bind", Source: &testDir, Destination: "/etc/foo", Options: []string{"rbind"}}, + } + c.PluginConfig.Mounts = append(c.PluginConfig.Mounts, mounts...) + c.PropagatedMount = "/propagated" + c.Network = types.PluginConfigNetwork{Type: "host"} + c.IpcHost = true + }) + + err = client.PluginEnable(ctx, "test", types.PluginEnableOptions{Timeout: 30}) + assert.Assert(t, err) + defer func() { + err := client.PluginRemove(ctx, "test", types.PluginRemoveOptions{Force: true}) + assert.Check(t, err) + }() + + p, _, err := client.PluginInspectWithRaw(ctx, "test") + assert.Assert(t, err) + assert.Assert(t, p.Enabled) +} diff --git a/plugin/v2/plugin_linux.go b/plugin/v2/plugin_linux.go index d0fc97268d..4ad582cd83 100644 --- a/plugin/v2/plugin_linux.go +++ b/plugin/v2/plugin_linux.go @@ -4,7 +4,6 @@ import ( "os" "path/filepath" "runtime" - "sort" "strings" "github.com/docker/docker/api/types" @@ -138,9 +137,5 @@ func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { p.modifyRuntimeSpec(&s) } - sort.Slice(s.Mounts, func(i, j int) bool { - return s.Mounts[i].Destination < s.Mounts[j].Destination - }) - return &s, nil }