From 426e610e43179d58b29c496bc79a53f410a4b1e1 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 13 Dec 2017 15:24:51 -0500 Subject: [PATCH 1/6] Use runtime spec modifier for metrics plugin hook Currently the metrics plugin uses a really hackish host mount with propagated mounts to get the metrics socket into a plugin after the plugin is alreay running. This approach ends up leaking mounts which requires setting the plugin manager root to private, which causes some other issues. With this change, plugin subsystems can register a set of modifiers to apply to the plugin's runtime spec before the plugin is ever started. This will help to generalize some of the customization work that needs to happen for various plugin subsystems (and future ones). Specifically it lets the metrics plugin subsystem append a mount to the runtime spec to mount the metrics socket in the plugin's mount namespace rather than the host's and prevetns any leaking due to this mount. Signed-off-by: Brian Goff --- daemon/metrics.go | 22 ----------------- daemon/metrics_unix.go | 52 +++++++++------------------------------ plugin/defs.go | 15 ++++++++++- plugin/store.go | 43 +++++++++++++++++++++++++++++--- plugin/v2/plugin.go | 11 +++++++++ plugin/v2/plugin_linux.go | 5 ++++ 6 files changed, 81 insertions(+), 67 deletions(-) diff --git a/daemon/metrics.go b/daemon/metrics.go index 92439ad148..fef0b1d18f 100644 --- a/daemon/metrics.go +++ b/daemon/metrics.go @@ -1,10 +1,8 @@ package daemon import ( - "path/filepath" "sync" - "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/plugingetter" metrics "github.com/docker/go-metrics" "github.com/pkg/errors" @@ -132,18 +130,6 @@ func (d *Daemon) cleanupMetricsPlugins() { } } -type metricsPlugin struct { - plugingetter.CompatPlugin -} - -func (p metricsPlugin) sock() string { - return "metrics.sock" -} - -func (p metricsPlugin) sockBase() string { - return filepath.Join(p.BasePath(), "run", "docker") -} - func pluginStartMetricsCollection(p plugingetter.CompatPlugin) error { type metricsPluginResponse struct { Err string @@ -162,12 +148,4 @@ func pluginStopMetricsCollection(p plugingetter.CompatPlugin) { if err := p.Client().Call(metricsPluginType+".StopMetrics", nil, nil); err != nil { logrus.WithError(err).WithField("name", p.Name()).Error("error stopping metrics collector") } - - mp := metricsPlugin{p} - sockPath := filepath.Join(mp.sockBase(), mp.sock()) - if err := mount.Unmount(sockPath); err != nil { - if mounted, _ := mount.Mounted(sockPath); mounted { - logrus.WithError(err).WithField("name", p.Name()).WithField("socket", sockPath).Error("error unmounting metrics socket for plugin") - } - } } diff --git a/daemon/metrics_unix.go b/daemon/metrics_unix.go index 6045939a58..6df00c6541 100644 --- a/daemon/metrics_unix.go +++ b/daemon/metrics_unix.go @@ -5,13 +5,13 @@ package daemon import ( "net" "net/http" - "os" "path/filepath" - "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/plugins" + "github.com/docker/docker/plugin" metrics "github.com/docker/go-metrics" + specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -34,52 +34,22 @@ func (daemon *Daemon) listenMetricsSock() (string, error) { return path, nil } -func registerMetricsPluginCallback(getter plugingetter.PluginGetter, sockPath string) { - getter.Handle(metricsPluginType, func(name string, client *plugins.Client) { +func registerMetricsPluginCallback(store *plugin.Store, sockPath string) { + store.RegisterRuntimeOpt(metricsPluginType, func(s *specs.Spec) { + f := plugin.WithSpecMounts([]specs.Mount{ + {Type: "bind", Source: sockPath, Destination: "/run/docker/metrics.sock", Options: []string{"bind", "ro"}}, + }) + f(s) + }) + store.Handle(metricsPluginType, func(name string, client *plugins.Client) { // Use lookup since nothing in the system can really reference it, no need // to protect against removal - p, err := getter.Get(name, metricsPluginType, plugingetter.Lookup) + p, err := store.Get(name, metricsPluginType, plugingetter.Lookup) if err != nil { return } - mp := metricsPlugin{p} - sockBase := mp.sockBase() - if err := os.MkdirAll(sockBase, 0755); err != nil { - logrus.WithError(err).WithField("name", name).WithField("path", sockBase).Error("error creating metrics plugin base path") - return - } - - defer func() { - if err != nil { - os.RemoveAll(sockBase) - } - }() - - pluginSockPath := filepath.Join(sockBase, mp.sock()) - _, err = os.Stat(pluginSockPath) - if err == nil { - mount.Unmount(pluginSockPath) - } else { - logrus.WithField("path", pluginSockPath).Debugf("creating plugin socket") - f, err := os.OpenFile(pluginSockPath, os.O_CREATE, 0600) - if err != nil { - return - } - f.Close() - } - - if err := mount.Mount(sockPath, pluginSockPath, "none", "bind,ro"); err != nil { - logrus.WithError(err).WithField("name", name).Error("could not mount metrics socket to plugin") - return - } - if err := pluginStartMetricsCollection(p); err != nil { - if err := mount.Unmount(pluginSockPath); err != nil { - if mounted, _ := mount.Mounted(pluginSockPath); mounted { - logrus.WithError(err).WithField("sock_path", pluginSockPath).Error("error unmounting metrics socket from plugin during cleanup") - } - } logrus.WithError(err).WithField("name", name).Error("error while initializing metrics plugin") } }) diff --git a/plugin/defs.go b/plugin/defs.go index 3e930de048..3bbb5f93f9 100644 --- a/plugin/defs.go +++ b/plugin/defs.go @@ -5,12 +5,14 @@ import ( "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/plugin/v2" + specs "github.com/opencontainers/runtime-spec/specs-go" ) // Store manages the plugin inventory in memory and on-disk type Store struct { sync.RWMutex - plugins map[string]*v2.Plugin + plugins map[string]*v2.Plugin + specOpts map[string][]SpecOpt /* handlers are necessary for transition path of legacy plugins * to the new model. Legacy plugins use Handle() for registering an * activation callback.*/ @@ -21,10 +23,14 @@ type Store struct { func NewStore() *Store { return &Store{ plugins: make(map[string]*v2.Plugin), + specOpts: make(map[string][]SpecOpt), handlers: make(map[string][]func(string, *plugins.Client)), } } +// SpecOpt is used for subsystems that need to modify the runtime spec of a plugin +type SpecOpt func(*specs.Spec) + // CreateOpt is used to configure specific plugin details when created type CreateOpt func(p *v2.Plugin) @@ -35,3 +41,10 @@ func WithSwarmService(id string) CreateOpt { p.SwarmServiceID = id } } + +// WithSpecMounts is a SpecOpt which appends the provided mounts to the runtime spec +func WithSpecMounts(mounts []specs.Mount) SpecOpt { + return func(s *specs.Spec) { + s.Mounts = append(s.Mounts, mounts...) + } +} diff --git a/plugin/store.go b/plugin/store.go index 9768c25068..b30fe55339 100644 --- a/plugin/store.go +++ b/plugin/store.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/plugins" "github.com/docker/docker/plugin/v2" + specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -64,6 +65,10 @@ func (ps *Store) GetAll() map[string]*v2.Plugin { func (ps *Store) SetAll(plugins map[string]*v2.Plugin) { ps.Lock() defer ps.Unlock() + + for _, p := range plugins { + ps.setSpecOpts(p) + } ps.plugins = plugins } @@ -90,6 +95,22 @@ func (ps *Store) SetState(p *v2.Plugin, state bool) { p.PluginObj.Enabled = state } +func (ps *Store) setSpecOpts(p *v2.Plugin) { + var specOpts []SpecOpt + for _, typ := range p.GetTypes() { + opts, ok := ps.specOpts[typ.String()] + if ok { + specOpts = append(specOpts, opts...) + } + } + + p.SetSpecOptModifier(func(s *specs.Spec) { + for _, o := range specOpts { + o(s) + } + }) +} + // Add adds a plugin to memory and plugindb. // An error will be returned if there is a collision. func (ps *Store) Add(p *v2.Plugin) error { @@ -99,6 +120,9 @@ func (ps *Store) Add(p *v2.Plugin) error { if v, exist := ps.plugins[p.GetID()]; exist { return fmt.Errorf("plugin %q has the same ID %s as %q", p.Name(), p.GetID(), v.Name()) } + + ps.setSpecOpts(p) + ps.plugins[p.GetID()] = p return nil } @@ -182,20 +206,24 @@ func (ps *Store) GetAllByCap(capability string) ([]plugingetter.CompatPlugin, er return result, nil } +func pluginType(cap string) string { + return fmt.Sprintf("docker.%s/%s", strings.ToLower(cap), defaultAPIVersion) +} + // Handle sets a callback for a given capability. It is only used by network // and ipam drivers during plugin registration. The callback registers the // driver with the subsystem (network, ipam). func (ps *Store) Handle(capability string, callback func(string, *plugins.Client)) { - pluginType := fmt.Sprintf("docker.%s/%s", strings.ToLower(capability), defaultAPIVersion) + typ := pluginType(capability) // Register callback with new plugin model. ps.Lock() - handlers, ok := ps.handlers[pluginType] + handlers, ok := ps.handlers[typ] if !ok { handlers = []func(string, *plugins.Client){} } handlers = append(handlers, callback) - ps.handlers[pluginType] = handlers + ps.handlers[typ] = handlers ps.Unlock() // Register callback with legacy plugin model. @@ -204,6 +232,15 @@ func (ps *Store) Handle(capability string, callback func(string, *plugins.Client } } +// RegisterRuntimeOpt stores a list of SpecOpts for the provided capability. +// These options are applied to the runtime spec before a plugin is started for the specified capability. +func (ps *Store) RegisterRuntimeOpt(cap string, opts ...SpecOpt) { + ps.Lock() + defer ps.Unlock() + typ := pluginType(cap) + ps.specOpts[typ] = append(ps.specOpts[typ], opts...) +} + // CallHandler calls the registered callback. It is invoked during plugin enable. func (ps *Store) CallHandler(p *v2.Plugin) { for _, typ := range p.GetTypes() { diff --git a/plugin/v2/plugin.go b/plugin/v2/plugin.go index ce3257c0cb..a3e95f6cad 100644 --- a/plugin/v2/plugin.go +++ b/plugin/v2/plugin.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/plugins" "github.com/opencontainers/go-digest" + specs "github.com/opencontainers/runtime-spec/specs-go" ) // Plugin represents an individual plugin. @@ -23,6 +24,8 @@ type Plugin struct { Config digest.Digest Blobsums []digest.Digest + modifyRuntimeSpec func(*specs.Spec) + SwarmServiceID string } @@ -250,3 +253,11 @@ func (p *Plugin) Acquire() { func (p *Plugin) Release() { p.AddRefCount(plugingetter.Release) } + +// SetSpecOptModifier sets the function to use to modify the the generated +// runtime spec. +func (p *Plugin) SetSpecOptModifier(f func(*specs.Spec)) { + p.mu.Lock() + p.modifyRuntimeSpec = f + p.mu.Unlock() +} diff --git a/plugin/v2/plugin_linux.go b/plugin/v2/plugin_linux.go index 9590df4f7d..b31a4b4c23 100644 --- a/plugin/v2/plugin_linux.go +++ b/plugin/v2/plugin_linux.go @@ -16,6 +16,7 @@ import ( // InitSpec creates an OCI spec from the plugin's config. func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { s := oci.DefaultSpec() + s.Root = &specs.Root{ Path: p.Rootfs, Readonly: false, // TODO: all plugins should be readonly? settable in config? @@ -126,5 +127,9 @@ func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { caps.Inheritable = append(caps.Inheritable, p.PluginObj.Config.Linux.Capabilities...) caps.Effective = append(caps.Effective, p.PluginObj.Config.Linux.Capabilities...) + if p.modifyRuntimeSpec != nil { + p.modifyRuntimeSpec(&s) + } + return &s, nil } From a53930a04fa81b082aa78e66b342ff19cc63cc5f Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 14 Dec 2017 09:29:11 -0500 Subject: [PATCH 2/6] Plugins perform propagated mount in runtime spec Setting up the mounts on the host increases chances of mount leakage and makes for more cleanup after the plugin has stopped. With this change all mounts for the plugin are performed by the container runtime and automatically cleaned up when the container exits. Signed-off-by: Brian Goff --- daemon/graphdriver/plugin.go | 2 +- plugin/manager.go | 28 +++------------------------- plugin/manager_linux.go | 17 +++++------------ plugin/v2/plugin.go | 11 +++++------ plugin/v2/plugin_linux.go | 21 ++++++++++++++++----- 5 files changed, 30 insertions(+), 49 deletions(-) diff --git a/daemon/graphdriver/plugin.go b/daemon/graphdriver/plugin.go index 5d433e5196..9f560f0381 100644 --- a/daemon/graphdriver/plugin.go +++ b/daemon/graphdriver/plugin.go @@ -23,7 +23,7 @@ func newPluginDriver(name string, pl plugingetter.CompatPlugin, config Options) home := config.Root if !pl.IsV1() { if p, ok := pl.(*v2.Plugin); ok { - if p.PropagatedMount != "" { + if p.PluginObj.Config.PropagatedMount != "" { home = p.PluginObj.Config.PropagatedMount } } diff --git a/plugin/manager.go b/plugin/manager.go index d686443c6f..a163a2aa7e 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -19,7 +19,6 @@ import ( "github.com/docker/docker/layer" "github.com/docker/docker/pkg/authorization" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/pubsub" "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin/v2" @@ -151,16 +150,6 @@ func (pm *Manager) HandleExitEvent(id string) error { os.RemoveAll(filepath.Join(pm.config.ExecRoot, id)) - if p.PropagatedMount != "" { - if err := mount.Unmount(p.PropagatedMount); err != nil { - logrus.Warnf("Could not unmount %s: %v", p.PropagatedMount, err) - } - propRoot := filepath.Join(filepath.Dir(p.Rootfs), "propagated-mount") - if err := mount.Unmount(propRoot); err != nil { - logrus.Warn("Could not unmount %s: %v", propRoot, err) - } - } - pm.mu.RLock() c := pm.cMap[p] if c.exitChan != nil { @@ -239,28 +228,17 @@ func (pm *Manager) reload() error { // todo: restore // check if we need to migrate an older propagated mount from before // these mounts were stored outside the plugin rootfs if _, err := os.Stat(propRoot); os.IsNotExist(err) { - if _, err := os.Stat(p.PropagatedMount); err == nil { - // make sure nothing is mounted here - // don't care about errors - mount.Unmount(p.PropagatedMount) - if err := os.Rename(p.PropagatedMount, propRoot); err != nil { + rootfsProp := filepath.Join(p.Rootfs, p.PluginObj.Config.PropagatedMount) + if _, err := os.Stat(rootfsProp); err == nil { + if err := os.Rename(rootfsProp, propRoot); err != nil { logrus.WithError(err).WithField("dir", propRoot).Error("error migrating propagated mount storage") } - if err := os.MkdirAll(p.PropagatedMount, 0755); err != nil { - logrus.WithError(err).WithField("dir", p.PropagatedMount).Error("error migrating propagated mount storage") - } } } if err := os.MkdirAll(propRoot, 0755); err != nil { logrus.Errorf("failed to create PropagatedMount directory at %s: %v", propRoot, err) } - // TODO: sanitize PropagatedMount and prevent breakout - p.PropagatedMount = filepath.Join(p.Rootfs, p.PluginObj.Config.PropagatedMount) - if err := os.MkdirAll(p.PropagatedMount, 0755); err != nil { - logrus.Errorf("failed to create PropagatedMount directory at %s: %v", p.PropagatedMount, err) - return - } } } } diff --git a/plugin/manager_linux.go b/plugin/manager_linux.go index 65380af06f..00eaf869cc 100644 --- a/plugin/manager_linux.go +++ b/plugin/manager_linux.go @@ -22,7 +22,7 @@ import ( "golang.org/x/sys/unix" ) -func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) (err error) { +func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error { p.Rootfs = filepath.Join(pm.config.Root, p.PluginObj.ID, "rootfs") if p.IsEnabled() && !force { return errors.Wrap(enabledError(p.Name()), "plugin already enabled") @@ -40,20 +40,16 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) (err error) { pm.mu.Unlock() var propRoot string - if p.PropagatedMount != "" { + if p.PluginObj.Config.PropagatedMount != "" { propRoot = filepath.Join(filepath.Dir(p.Rootfs), "propagated-mount") - if err = os.MkdirAll(propRoot, 0755); err != nil { + if err := os.MkdirAll(propRoot, 0755); err != nil { logrus.Errorf("failed to create PropagatedMount directory at %s: %v", propRoot, err) } - if err = mount.MakeRShared(propRoot); err != nil { + if err := mount.MakeRShared(propRoot); err != nil { return errors.Wrap(err, "error setting up propagated mount dir") } - - if err = mount.Mount(propRoot, p.PropagatedMount, "none", "rbind"); err != nil { - return errors.Wrap(err, "error creating mount for propagated mount") - } } rootFS := containerfs.NewLocalContainerFS(filepath.Join(pm.config.Root, p.PluginObj.ID, rootFSFileName)) @@ -63,10 +59,7 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) (err error) { stdout, stderr := makeLoggerStreams(p.GetID()) if err := pm.executor.Create(p.GetID(), *spec, stdout, stderr); err != nil { - if p.PropagatedMount != "" { - if err := mount.Unmount(p.PropagatedMount); err != nil { - logrus.Warnf("Could not unmount %s: %v", p.PropagatedMount, err) - } + if p.PluginObj.Config.PropagatedMount != "" { if err := mount.Unmount(propRoot); err != nil { logrus.Warnf("Could not unmount %s: %v", propRoot, err) } diff --git a/plugin/v2/plugin.go b/plugin/v2/plugin.go index a3e95f6cad..a4c7c535f6 100644 --- a/plugin/v2/plugin.go +++ b/plugin/v2/plugin.go @@ -14,12 +14,11 @@ import ( // Plugin represents an individual plugin. type Plugin struct { - mu sync.RWMutex - PluginObj types.Plugin `json:"plugin"` // todo: embed struct - pClient *plugins.Client - refCount int - PropagatedMount string // TODO: make private - Rootfs string // TODO: make private + mu sync.RWMutex + PluginObj types.Plugin `json:"plugin"` // todo: embed struct + pClient *plugins.Client + refCount int + Rootfs string // TODO: make private Config digest.Digest Blobsums []digest.Digest diff --git a/plugin/v2/plugin_linux.go b/plugin/v2/plugin_linux.go index b31a4b4c23..fa4be7a32c 100644 --- a/plugin/v2/plugin_linux.go +++ b/plugin/v2/plugin_linux.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "sort" "strings" "github.com/docker/docker/api/types" @@ -32,6 +33,17 @@ func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { return nil, errors.WithStack(err) } + if p.PluginObj.Config.PropagatedMount != "" { + pRoot := filepath.Join(filepath.Dir(p.Rootfs), "propagated-mount") + s.Mounts = append(s.Mounts, specs.Mount{ + Source: pRoot, + Destination: p.PluginObj.Config.PropagatedMount, + Type: "bind", + Options: []string{"rbind", "rw", "rshared"}, + }) + s.Linux.RootfsPropagation = "rshared" + } + mounts := append(p.PluginObj.Config.Mounts, types.PluginMount{ Source: &execRoot, Destination: defaultPluginRuntimeDestination, @@ -89,11 +101,6 @@ func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { } } - if p.PluginObj.Config.PropagatedMount != "" { - p.PropagatedMount = filepath.Join(p.Rootfs, p.PluginObj.Config.PropagatedMount) - s.Linux.RootfsPropagation = "rshared" - } - if p.PluginObj.Config.Linux.AllowAllDevices { s.Linux.Resources.Devices = []specs.LinuxDeviceCgroup{{Allow: true, Access: "rwm"}} } @@ -131,5 +138,9 @@ 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 } From 0e5eaf8ee32662182147f5f62c1bfebef66f5c47 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 14 Dec 2017 10:27:10 -0500 Subject: [PATCH 3/6] Ensure plugin returns correctly scoped paths Before this change, volume management was relying on the fact that everything the plugin mounts is visible on the host within the plugin's rootfs. In practice this caused some issues with mount leaks, so we changed the behavior such that mounts are not visible on the plugin's rootfs, but available outside of it, which breaks volume management. To fix the issue, allow the plugin to scope the path correctly rather than assuming that everything is visible in `p.Rootfs`. In practice this is just scoping the `PropagatedMount` paths to the correct host path. Signed-off-by: Brian Goff --- daemon/graphdriver/proxy.go | 3 +- daemon/logger/adapter.go | 5 ++- daemon/logger/plugin.go | 16 ++++----- pkg/plugingetter/getter.go | 2 +- pkg/plugins/plugins_unix.go | 8 ++--- pkg/plugins/plugins_windows.go | 9 +++-- plugin/manager_linux.go | 1 - plugin/v2/plugin.go | 12 ++++--- volume/drivers/adapter.go | 60 +++++++++++++++------------------- volume/drivers/extpoint.go | 8 ++--- volume/testutils/testutils.go | 4 +-- 11 files changed, 60 insertions(+), 68 deletions(-) diff --git a/daemon/graphdriver/proxy.go b/daemon/graphdriver/proxy.go index 81ef872ad9..c365922c1e 100644 --- a/daemon/graphdriver/proxy.go +++ b/daemon/graphdriver/proxy.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io" - "path/filepath" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/containerfs" @@ -143,7 +142,7 @@ func (d *graphDriverProxy) Get(id, mountLabel string) (containerfs.ContainerFS, if ret.Err != "" { err = errors.New(ret.Err) } - return containerfs.NewLocalContainerFS(filepath.Join(d.p.BasePath(), ret.Dir)), err + return containerfs.NewLocalContainerFS(d.p.ScopedPath(ret.Dir)), err } func (d *graphDriverProxy) Put(id string) error { diff --git a/daemon/logger/adapter.go b/daemon/logger/adapter.go index 5817913cbc..7fe49776af 100644 --- a/daemon/logger/adapter.go +++ b/daemon/logger/adapter.go @@ -3,7 +3,7 @@ package logger import ( "io" "os" - "strings" + "path/filepath" "sync" "time" @@ -19,7 +19,6 @@ type pluginAdapter struct { driverName string id string plugin logPlugin - basePath string fifoPath string capabilities Capability logInfo Info @@ -58,7 +57,7 @@ func (a *pluginAdapter) Close() error { a.mu.Lock() defer a.mu.Unlock() - if err := a.plugin.StopLogging(strings.TrimPrefix(a.fifoPath, a.basePath)); err != nil { + if err := a.plugin.StopLogging(filepath.Join("/", "run", "docker", "logging", a.id)); err != nil { return err } diff --git a/daemon/logger/plugin.go b/daemon/logger/plugin.go index bdccea5b21..f293a529a9 100644 --- a/daemon/logger/plugin.go +++ b/daemon/logger/plugin.go @@ -5,7 +5,6 @@ import ( "io" "os" "path/filepath" - "strings" "github.com/docker/docker/api/types/plugins/logdriver" getter "github.com/docker/docker/pkg/plugingetter" @@ -39,18 +38,20 @@ func getPlugin(name string, mode int) (Creator, error) { } d := &logPluginProxy{p.Client()} - return makePluginCreator(name, d, p.BasePath()), nil + return makePluginCreator(name, d, p.ScopedPath), nil } -func makePluginCreator(name string, l *logPluginProxy, basePath string) Creator { +func makePluginCreator(name string, l *logPluginProxy, scopePath func(s string) string) Creator { return func(logCtx Info) (logger Logger, err error) { defer func() { if err != nil { pluginGetter.Get(name, extName, getter.Release) } }() - root := filepath.Join(basePath, "run", "docker", "logging") - if err := os.MkdirAll(root, 0700); err != nil { + + unscopedPath := filepath.Join("/", "run", "docker", "logging") + logRoot := scopePath(unscopedPath) + if err := os.MkdirAll(logRoot, 0700); err != nil { return nil, err } @@ -59,8 +60,7 @@ func makePluginCreator(name string, l *logPluginProxy, basePath string) Creator driverName: name, id: id, plugin: l, - basePath: basePath, - fifoPath: filepath.Join(root, id), + fifoPath: filepath.Join(logRoot, id), logInfo: logCtx, } @@ -77,7 +77,7 @@ func makePluginCreator(name string, l *logPluginProxy, basePath string) Creator a.stream = stream a.enc = logdriver.NewLogEntryEncoder(a.stream) - if err := l.StartLogging(strings.TrimPrefix(a.fifoPath, basePath), logCtx); err != nil { + if err := l.StartLogging(filepath.Join(unscopedPath, id), logCtx); err != nil { return nil, errors.Wrapf(err, "error creating logger") } diff --git a/pkg/plugingetter/getter.go b/pkg/plugingetter/getter.go index b9ffa547a4..b90d4fa3f6 100644 --- a/pkg/plugingetter/getter.go +++ b/pkg/plugingetter/getter.go @@ -17,7 +17,7 @@ const ( type CompatPlugin interface { Client() *plugins.Client Name() string - BasePath() string + ScopedPath(string) string IsV1() bool } diff --git a/pkg/plugins/plugins_unix.go b/pkg/plugins/plugins_unix.go index 02f1da69a1..543f439fc7 100644 --- a/pkg/plugins/plugins_unix.go +++ b/pkg/plugins/plugins_unix.go @@ -2,8 +2,8 @@ package plugins -// BasePath returns the path to which all paths returned by the plugin are relative to. -// For v1 plugins, this always returns the host's root directory. -func (p *Plugin) BasePath() string { - return "/" +// ScopedPath returns the path scoped to the plugin's rootfs. +// For v1 plugins, this always returns the path unchanged as v1 plugins run directly on the host. +func (p *Plugin) ScopedPath(s string) string { + return s } diff --git a/pkg/plugins/plugins_windows.go b/pkg/plugins/plugins_windows.go index 3c8d8feb83..a46e3c109e 100644 --- a/pkg/plugins/plugins_windows.go +++ b/pkg/plugins/plugins_windows.go @@ -1,8 +1,7 @@ package plugins -// BasePath returns the path to which all paths returned by the plugin are relative to. -// For Windows v1 plugins, this returns an empty string, since the plugin is already aware -// of the absolute path of the mount. -func (p *Plugin) BasePath() string { - return "" +// ScopedPath returns the path scoped to the plugin's rootfs. +// For v1 plugins, this always returns the path unchanged as v1 plugins run directly on the host. +func (p *Plugin) ScopedPath(s string) string { + return s } diff --git a/plugin/manager_linux.go b/plugin/manager_linux.go index 00eaf869cc..9670cf8ae4 100644 --- a/plugin/manager_linux.go +++ b/plugin/manager_linux.go @@ -65,7 +65,6 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error { } } } - return pm.pluginPostStart(p, c) } diff --git a/plugin/v2/plugin.go b/plugin/v2/plugin.go index a4c7c535f6..74adba680b 100644 --- a/plugin/v2/plugin.go +++ b/plugin/v2/plugin.go @@ -2,6 +2,7 @@ package v2 import ( "fmt" + "path/filepath" "strings" "sync" @@ -39,10 +40,13 @@ func (e ErrInadequateCapability) Error() string { return fmt.Sprintf("plugin does not provide %q capability", e.cap) } -// BasePath returns the path to which all paths returned by the plugin are relative to. -// For Plugin objects this returns the host path of the plugin container's rootfs. -func (p *Plugin) BasePath() string { - return p.Rootfs +// ScopedPath returns the path scoped to the plugin rootfs +func (p *Plugin) ScopedPath(s string) string { + if p.PluginObj.Config.PropagatedMount != "" && strings.HasPrefix(s, p.PluginObj.Config.PropagatedMount) { + // re-scope to the propagated mount path on the host + return filepath.Join(filepath.Dir(p.Rootfs), "propagated-mount", strings.TrimPrefix(s, p.PluginObj.Config.PropagatedMount)) + } + return filepath.Join(p.Rootfs, s) } // Client returns the plugin client. diff --git a/volume/drivers/adapter.go b/volume/drivers/adapter.go index 54d1ff2630..36aa521098 100644 --- a/volume/drivers/adapter.go +++ b/volume/drivers/adapter.go @@ -2,7 +2,6 @@ package volumedrivers import ( "errors" - "path/filepath" "strings" "time" @@ -16,7 +15,7 @@ var ( type volumeDriverAdapter struct { name string - baseHostPath string + scopePath func(s string) string capabilities *volume.Capability proxy *volumeDriverProxy } @@ -30,10 +29,10 @@ func (a *volumeDriverAdapter) Create(name string, opts map[string]string) (volum return nil, err } return &volumeAdapter{ - proxy: a.proxy, - name: name, - driverName: a.name, - baseHostPath: a.baseHostPath, + proxy: a.proxy, + name: name, + driverName: a.name, + scopePath: a.scopePath, }, nil } @@ -41,13 +40,6 @@ func (a *volumeDriverAdapter) Remove(v volume.Volume) error { return a.proxy.Remove(v.Name()) } -func hostPath(baseHostPath, path string) string { - if baseHostPath != "" { - path = filepath.Join(baseHostPath, path) - } - return path -} - func (a *volumeDriverAdapter) List() ([]volume.Volume, error) { ls, err := a.proxy.List() if err != nil { @@ -57,11 +49,11 @@ func (a *volumeDriverAdapter) List() ([]volume.Volume, error) { var out []volume.Volume for _, vp := range ls { out = append(out, &volumeAdapter{ - proxy: a.proxy, - name: vp.Name, - baseHostPath: a.baseHostPath, - driverName: a.name, - eMount: hostPath(a.baseHostPath, vp.Mountpoint), + proxy: a.proxy, + name: vp.Name, + scopePath: a.scopePath, + driverName: a.name, + eMount: a.scopePath(vp.Mountpoint), }) } return out, nil @@ -79,13 +71,13 @@ func (a *volumeDriverAdapter) Get(name string) (volume.Volume, error) { } return &volumeAdapter{ - proxy: a.proxy, - name: v.Name, - driverName: a.Name(), - eMount: v.Mountpoint, - createdAt: v.CreatedAt, - status: v.Status, - baseHostPath: a.baseHostPath, + proxy: a.proxy, + name: v.Name, + driverName: a.Name(), + eMount: v.Mountpoint, + createdAt: v.CreatedAt, + status: v.Status, + scopePath: a.scopePath, }, nil } @@ -122,13 +114,13 @@ func (a *volumeDriverAdapter) getCapabilities() volume.Capability { } type volumeAdapter struct { - proxy *volumeDriverProxy - name string - baseHostPath string - driverName string - eMount string // ephemeral host volume path - createdAt time.Time // time the directory was created - status map[string]interface{} + proxy *volumeDriverProxy + name string + scopePath func(string) string + driverName string + eMount string // ephemeral host volume path + createdAt time.Time // time the directory was created + status map[string]interface{} } type proxyVolume struct { @@ -149,7 +141,7 @@ func (a *volumeAdapter) DriverName() string { func (a *volumeAdapter) Path() string { if len(a.eMount) == 0 { mountpoint, _ := a.proxy.Path(a.name) - a.eMount = hostPath(a.baseHostPath, mountpoint) + a.eMount = a.scopePath(mountpoint) } return a.eMount } @@ -160,7 +152,7 @@ func (a *volumeAdapter) CachedPath() string { func (a *volumeAdapter) Mount(id string) (string, error) { mountpoint, err := a.proxy.Mount(a.name, id) - a.eMount = hostPath(a.baseHostPath, mountpoint) + a.eMount = a.scopePath(mountpoint) return a.eMount, err } diff --git a/volume/drivers/extpoint.go b/volume/drivers/extpoint.go index c360b37a2b..56f78b9245 100644 --- a/volume/drivers/extpoint.go +++ b/volume/drivers/extpoint.go @@ -25,9 +25,9 @@ var drivers = &driverExtpoint{ const extName = "VolumeDriver" // NewVolumeDriver returns a driver has the given name mapped on the given client. -func NewVolumeDriver(name string, baseHostPath string, c client) volume.Driver { +func NewVolumeDriver(name string, scopePath func(string) string, c client) volume.Driver { proxy := &volumeDriverProxy{c} - return &volumeDriverAdapter{name: name, baseHostPath: baseHostPath, proxy: proxy} + return &volumeDriverAdapter{name: name, scopePath: scopePath, proxy: proxy} } // volumeDriver defines the available functions that volume plugins must implement. @@ -129,7 +129,7 @@ func lookup(name string, mode int) (volume.Driver, error) { return nil, errors.Wrap(err, "error looking up volume plugin "+name) } - d := NewVolumeDriver(p.Name(), p.BasePath(), p.Client()) + d := NewVolumeDriver(p.Name(), p.ScopedPath, p.Client()) if err := validateDriver(d); err != nil { if mode > 0 { // Undo any reference count changes from the initial `Get` @@ -224,7 +224,7 @@ func GetAllDrivers() ([]volume.Driver, error) { continue } - ext := NewVolumeDriver(name, p.BasePath(), p.Client()) + ext := NewVolumeDriver(name, p.ScopedPath, p.Client()) if p.IsV1() { drivers.extensions[name] = ext } diff --git a/volume/testutils/testutils.go b/volume/testutils/testutils.go index 84ab55ff77..5a17a26b77 100644 --- a/volume/testutils/testutils.go +++ b/volume/testutils/testutils.go @@ -178,8 +178,8 @@ func (p *fakePlugin) IsV1() bool { return false } -func (p *fakePlugin) BasePath() string { - return "" +func (p *fakePlugin) ScopedPath(s string) string { + return s } type fakePluginGetter struct { From 37d7b7ddc332541f516d0c41d9ad30b28f2d3e22 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 13 Dec 2017 15:33:15 -0500 Subject: [PATCH 4/6] Revert "Make plugins dir private." This reverts commit 0c2821d6f2de692d105e50a399daa65169697cca. Due to other changes this is no longer needed and resolves some other issues with plugins. Signed-off-by: Brian Goff --- plugin/manager.go | 5 ----- plugin/manager_linux.go | 8 -------- plugin/manager_windows.go | 2 -- 3 files changed, 15 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index a163a2aa7e..2bdf82e56f 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -111,11 +111,6 @@ func NewManager(config ManagerConfig) (*Manager, error) { return nil, errors.Wrapf(err, "failed to mkdir %v", dirName) } } - - if err := setupRoot(manager.config.Root); err != nil { - return nil, err - } - var err error manager.executor, err = config.CreateExecutor(manager) if err != nil { diff --git a/plugin/manager_linux.go b/plugin/manager_linux.go index 9670cf8ae4..1def28c2a0 100644 --- a/plugin/manager_linux.go +++ b/plugin/manager_linux.go @@ -159,13 +159,6 @@ func shutdownPlugin(p *v2.Plugin, c *controller, executor Executor) { } } -func setupRoot(root string) error { - if err := mount.MakePrivate(root); err != nil { - return errors.Wrap(err, "error setting plugin manager root to private") - } - return nil -} - func (pm *Manager) disable(p *v2.Plugin, c *controller) error { if !p.IsEnabled() { return errors.Wrap(errDisabled(p.Name()), "plugin is already disabled") @@ -194,7 +187,6 @@ func (pm *Manager) Shutdown() { shutdownPlugin(p, c, pm.executor) } } - mount.Unmount(pm.config.Root) } func (pm *Manager) upgradePlugin(p *v2.Plugin, configDigest digest.Digest, blobsums []digest.Digest, tmpRootFSDir string, privileges *types.PluginPrivileges) (err error) { diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index ac03d6e639..72ccae72d3 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -26,5 +26,3 @@ func (pm *Manager) restore(p *v2.Plugin) error { // Shutdown plugins func (pm *Manager) Shutdown() { } - -func setupRoot(root string) error { return nil } From e6f784e3df2095366fd99fd42ab5a2d0b451bd07 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 18 Dec 2017 21:28:16 -0500 Subject: [PATCH 5/6] Make sure plugin mounts are cleaned up Signed-off-by: Brian Goff --- plugin/manager.go | 5 +++++ plugin/manager_linux.go | 3 +++ 2 files changed, 8 insertions(+) diff --git a/plugin/manager.go b/plugin/manager.go index 2bdf82e56f..750e3a5349 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker/layer" "github.com/docker/docker/pkg/authorization" "github.com/docker/docker/pkg/ioutils" + "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/pubsub" "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin/v2" @@ -155,6 +156,10 @@ func (pm *Manager) HandleExitEvent(id string) error { if restart { pm.enable(p, c, true) + } else { + if err := mount.RecursiveUnmount(filepath.Join(pm.config.Root, id)); err != nil { + return errors.Wrap(err, "error cleaning up plugin mounts") + } } return nil } diff --git a/plugin/manager_linux.go b/plugin/manager_linux.go index 1def28c2a0..a98702802f 100644 --- a/plugin/manager_linux.go +++ b/plugin/manager_linux.go @@ -187,6 +187,9 @@ func (pm *Manager) Shutdown() { shutdownPlugin(p, c, pm.executor) } } + if err := mount.RecursiveUnmount(pm.config.Root); err != nil { + logrus.WithError(err).Warn("error cleaning up plugin mounts") + } } func (pm *Manager) upgradePlugin(p *v2.Plugin, configDigest digest.Digest, blobsums []digest.Digest, tmpRootFSDir string, privileges *types.PluginPrivileges) (err error) { From 0df654f3d61d8691ee113a8429bcc8ef65786bc7 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 20 Feb 2018 16:57:20 -0500 Subject: [PATCH 6/6] Cleanup volume plugin test with bad assumptions Test made some bad assumptions about on-disk state of volume data. This updates the test to only test based on what the volume API is designed to provide. Signed-off-by: Brian Goff --- .../docker_cli_daemon_plugins_test.go | 27 ++----------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/integration-cli/docker_cli_daemon_plugins_test.go b/integration-cli/docker_cli_daemon_plugins_test.go index 10aa514fe0..c527cb1d8e 100644 --- a/integration-cli/docker_cli_daemon_plugins_test.go +++ b/integration-cli/docker_cli_daemon_plugins_test.go @@ -3,8 +3,6 @@ package main import ( - "os" - "path/filepath" "strings" "github.com/docker/docker/integration-cli/checker" @@ -199,12 +197,6 @@ func (s *DockerDaemonSuite) TestVolumePlugin(c *check.C) { if err != nil { c.Fatalf("Could not install plugin: %v %s", err, out) } - pluginID, err := s.d.Cmd("plugin", "inspect", "-f", "{{.Id}}", pName) - pluginID = strings.TrimSpace(pluginID) - if err != nil { - c.Fatalf("Could not retrieve plugin ID: %v %s", err, pluginID) - } - mountpointPrefix := filepath.Join(s.d.RootDir(), "plugins", pluginID, "rootfs") defer func() { if out, err := s.d.Cmd("plugin", "disable", pName); err != nil { c.Fatalf("Could not disable plugin: %v %s", err, out) @@ -213,11 +205,6 @@ func (s *DockerDaemonSuite) TestVolumePlugin(c *check.C) { if out, err := s.d.Cmd("plugin", "remove", pName); err != nil { c.Fatalf("Could not remove plugin: %v %s", err, out) } - - exists, err := existsMountpointWithPrefix(mountpointPrefix) - c.Assert(err, checker.IsNil) - c.Assert(exists, checker.Equals, false) - }() out, err = s.d.Cmd("volume", "create", "-d", pName, volName) @@ -237,21 +224,11 @@ func (s *DockerDaemonSuite) TestVolumePlugin(c *check.C) { c.Assert(out, checker.Contains, volName) c.Assert(out, checker.Contains, pName) - mountPoint, err := s.d.Cmd("volume", "inspect", volName, "--format", "{{.Mountpoint}}") - if err != nil { - c.Fatalf("Could not inspect volume: %v %s", err, mountPoint) - } - mountPoint = strings.TrimSpace(mountPoint) - out, err = s.d.Cmd("run", "--rm", "-v", volName+":"+destDir, "busybox", "touch", destDir+destFile) c.Assert(err, checker.IsNil, check.Commentf(out)) - path := filepath.Join(s.d.RootDir(), "plugins", pluginID, "rootfs", mountPoint, destFile) - _, err = os.Lstat(path) - c.Assert(err, checker.IsNil) - exists, err := existsMountpointWithPrefix(mountpointPrefix) - c.Assert(err, checker.IsNil) - c.Assert(exists, checker.Equals, true) + out, err = s.d.Cmd("run", "--rm", "-v", volName+":"+destDir, "busybox", "ls", destDir+destFile) + c.Assert(err, checker.IsNil, check.Commentf(out)) } func (s *DockerDaemonSuite) TestGraphdriverPlugin(c *check.C) {