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 <cpuguy83@gmail.com>

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")
-		}
-	}
 }

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) {
+func registerMetricsPluginCallback(store *plugin.Store, sockPath string) {
-	getter.Handle(metricsPluginType, func(name string, client *plugins.Client) {
+	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")
 		}
 	})

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...)
+	}
+}

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() {

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()
+}

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
 }